[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [PATCH] Update virtio input device specification
Apologies for the delay.
On Fri, Mar 24, 2017 at 9:55 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Fri, Mar 24, 2017 at 11:17:59AM +0100, Ladi Prosek wrote:
>> * added VIRTIO_INPUT_CFG_ID_DEVIDS / virtio_input_devids
>> * added normative statements and moved them all to
>> \devicenormative and \drivernormative sections
>> * minor tweaks
>>
>> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>
> Thanks for taking this up!
>
> I think you should start sending non-incremental
> patches eventually. Soe comments below.
Ok, I'll send the next version against master.
>
>> ---
>>
>> This patch is based on Gerd's
>> https://www.kraxel.org/cgit/virtio-spec/log/?h=virtio-input
>>
>> It is addressing feedback given in
>> https://lists.oasis-open.org/archives/virtio-dev/201605/msg00003.html
>> and adding some more content.
>>
>> Thanks!
>> Ladi
>>
>>
>> conformance.tex | 18 ++++++++++++
>> virtio-input.tex | 87 ++++++++++++++++++++++++++++++++++++++++++++------------
>> 2 files changed, 87 insertions(+), 18 deletions(-)
>>
>> diff --git a/conformance.tex b/conformance.tex
>> index 7b7df32..2486e9a 100644
>> --- a/conformance.tex
>> +++ b/conformance.tex
>> @@ -145,6 +145,15 @@ An SCSI host driver MUST conform to the following normative statements:
>> \item \ref{drivernormative:Device Types / SCSI Host Device / Device Operation / Device Operation: eventq}
>> \end{itemize}
>>
>> +\subsection{Input Driver Conformance}\label{sec:Conformance / Driver Conformance / Input Driver Conformance}
>> +
>> +An input driver MUST conform to the following normative statements:
>> +
>> +\begin{itemize}
>> +\item \ref{drivernormative:Device Types / Input Device / Device Initialization}
>> +\item \ref{drivernormative:Device Types / Input Device / Device Operation}
>> +\end{itemize}
>> +
>> \section{Device Conformance}\label{sec:Conformance / Device Conformance}
>>
>> A device MUST conform to the following normative statements:
>> @@ -265,6 +274,15 @@ An SCSI host device MUST conform to the following normative statements:
>> \item \ref{devicenormative:Device Types / SCSI Host Device / Device Operation / Device Operation: eventq}
>> \end{itemize}
>>
>> +\subsection{Input Device Conformance}\label{sec:Conformance / Device Conformance / Input Device Conformance}
>> +
>> +An input device MUST conform to the following normative statements:
>> +
>> +\begin{itemize}
>> +\item \ref{devicenormative:Device Types / Input Device / Device Initialization}
>> +\item \ref{devicenormative:Device Types / Input Device / Device Operation}
>> +\end{itemize}
>> +
>> \section{Legacy Interface: Transitional Device and
>> Transitional Driver Conformance}\label{sec:Conformance / Legacy
>> Interface: Transitional Device and
>> diff --git a/virtio-input.tex b/virtio-input.tex
>> index 9151880..eb3e154 100644
>> --- a/virtio-input.tex
>> +++ b/virtio-input.tex
>> @@ -1,7 +1,8 @@
>> \section{Input Device}\label{sec:Device Types / Input Device}
>>
>> The virtio input device can be used to create virtual human interface
>> -devices such as keyboards, mice and tablets. It basically sends linux
>> +devices such as keyboards, mice and tablets. An instance of the virtio
>> +device represents one such input device. It sends Linux
>> input layer events over virtio.
>> See \href{https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/input.h}{include/uapi/linux/input.h}
>
> which parts of the above are of interest?
Of input.h? It has IOCTL definitions referenced further down in this
document. But in general it's hard to be any more specific since it's
a live header file.
>> and \href{https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/input-event-codes.h}{include/uapi/linux/input-event-codes.h}
>
> One thing missing is how should driver handle events
> it does not recognize.
Ok, will add an explicit clause that the driver should ignore events
it doesn't recognize.
> Also, is there a chance device will be interested in knowing
> which events does driver recognize?
I don't think that the Linux input backend can do anything useful with
that kind of information. It's probably not worth trying to invent a
sophisticated feature negotiation scheme here.
>> @@ -32,6 +33,7 @@ enum virtio_input_config_select {
>> VIRTIO_INPUT_CFG_UNSET = 0x00,
>> VIRTIO_INPUT_CFG_ID_NAME = 0x01,
>> VIRTIO_INPUT_CFG_ID_SERIAL = 0x02,
>> + VIRTIO_INPUT_CFG_ID_DEVIDS = 0x03,
>> VIRTIO_INPUT_CFG_PROP_BITS = 0x10,
>> VIRTIO_INPUT_CFG_EV_BITS = 0x11,
>> VIRTIO_INPUT_CFG_ABS_INFO = 0x12,
>> @@ -44,6 +46,13 @@ struct virtio_input_absinfo {
>> le32 flat;
>> };
>>
>> +struct virtio_input_devids {
>> + le16 bustype;
>> + le16 vendor;
>> + le16 product;
>> + le16 version;
>> +};
>> +
>> struct virtio_input_config {
>> u8 select;
>> u8 subsel;
>> @@ -53,44 +62,53 @@ struct virtio_input_config {
>> char string[128];
>> u8 bitmap[128];
>> struct virtio_input_absinfo abs;
>> + struct virtio_input_devids ids;
>> } u;
>> };
>> \end{lstlisting}
>>
>> -To query a specific piece of information the driver MUST set
>> -\field{select} and \field{subsel} accordingly, then check \field{size}
>> -to see and how much information is available. \field{size} can be
>> +To query a specific piece of information the driver sets
>> +\field{select} and \field{subsel} accordingly, then checks \field{size}
>> +to see how much information is available. \field{size} can be
>> zero if no information is available. Strings do not include a
>> -terminating NUL byte.
>> +NUL terminator.
>>
>> \begin{description}
>>
>> \item[VIRTIO_INPUT_CFG_ID_NAME]
>> -\field{subsel} is not used and MUST be zero.
>> +\field{subsel} is zero.
>> Returns the name of the device, in \field{u.string}.
>>
>> -Same as EVIOCGNAME ioctl for linux evdev devices.
>> +Same as EVIOCGNAME ioctl for Linux evdev devices.
>>
>> \item[VIRTIO_INPUT_CFG_ID_SERIAL]
>> -\field{subsel} is not used and MUST be zero.
>> +\field{subsel} is zero.
>> Returns the serial number of the device, in \field{u.string}.
>>
>> +\item[VIRTIO_INPUT_CFG_ID_DEVIDS]
>> +\field{subsel} is zero.
>> +Returns ID information of the device, in \field{u.ids}.
>> +
>> +Same as EVIOCGID ioctl for Linux evdev devices.
>> +
>
> Above line confuses more than it clarifies IMHO.
>
> Looks like that ioctl returns
> 42 struct input_id {
> 43 __u16 bustype;
> 44 __u16 vendor;
> 45 __u16 product;
> 46 __u16 version;
> 47 };
>
> Hos is this the same? Maybe "similar to"?
Ok, will change to "similar to".
>> \item[VIRTIO_INPUT_CFG_PROP_BITS]
>> -\field{subsel} is not used and MUST be zero.
>> +\field{subsel} is zero.
>> Returns input properties (INPUT_PROP_*) of the device, in \field{u.bitmap}.
>>
>> +Same as EVIOCGPROP ioctl for Linux evdev devices.
>> +
>
> Above line confuses more than it clarifies IMHO.
>
> Referred to headers contain no information about this ioctl.
I see EVIOCGPROP in input.h. With no further details, but that's true
for most of the stuff in the headers.
Ideally evdev would have a good documentation that we can point to.
The headers are not sufficient, readers will have to dig into Linux
source code to understand the semantics. That's unfortunate but the
alternative would be to provide an exhaustive documentation of the
Linux input layer in this spec and keep it up to date with the
implementation. And that's not feasible.
We can try to put a carefully worded sentence here that would be
general enough so that no reasonable changes to the Linux input layer
will invalidate it. Something like
"The semantics of this device configuration follows that of the
EVIOCGPROP ioctl for Linux evdev devices. Input device properties are
returned in a bitmap which is to be interpreted using the INPUT_PROP_*
constants."
Would that be acceptable?
>> \item[VIRTIO_INPUT_CFG_EV_BITS]
>> \field{subsel} specifies the event type (EV_*). If \field{size} is
>> -non-zero the event type is supported and a bitmap the of supported
>> +non-zero the event type is supported and a bitmap of supported
>> event codes is returned in \field{u.bitmap}.
>>
>> -Same as EVIOCGBIT ioctl.
>> +Same as EVIOCGBIT ioctl for Linux evdev devices.
>>
>> \item[VIRTIO_INPUT_CFG_ABS_INFO]
>> \field{subsel} specifies the absolute axes (ABS_*).
>> -Informations about the axis will be returned in \field{u.abs}.
>> +Information about the axis will be returned in \field{u.abs}.
>>
>> -Same as EVIOCGABS ioctl.
>> +Same as EVIOCGABS ioctl for Linux evdev devices.
>>
>> \end{description}
>>
>> @@ -101,19 +119,33 @@ Same as EVIOCGABS ioctl.
>> \item The eventq is populated with receive buffers.
>> \end{enumerate}
>>
>> +\drivernormative{\subsubsection}{Device Initialization}{Device Types / Input Device / Device Initialization}
>> +A driver MUST set both \field{select} and \field{subsel} when querying
>> + device configuration, in any order.
>> +
>> +A driver MUST NOT write to configuration fields other than \field{select}
>> + and \field{subsel}.
>> +
>> +A driver SHOULD check the \field{size} field before accessing the
>> + configuration information.
>> +
>> +\devicenormative{\subsubsection}{Device Initialization}{Device Types / Input Device / Device Initialization}
>> +A device MUST set the \field{size} field to zero if it doesn't support a
>> + given \field{select} and \field{subsel} combination.
>> +
>> \subsection{Device Operation}\label{sec:Device Types / Input Device / Device Operation}
>>
>> \begin{enumerate}
>> \item Input events such as press and release events for keys and
>> - buttons and motion events are sent from the device to the driver
>> - using the eventq.
>> -\item Status feedback such as keyboard led updates are sent from the
>> + buttons, and motion events for pointing devices are sent from
>> + the device to the driver using the eventq.
>> +\item Status feedback such as keyboard LED updates are sent from the
>> driver to the device using the statusq.
>> \item Both queues use the same virtio_input_event struct.
>> \field{type}, \field{code} and \field{value} are filled according to
>> - the linux input layer (evdev) interface, except that the fields are
>> + the Linux input layer (evdev) interface, except that the fields are
>> in little endian byte order whereas the evdev ioctl interface uses
>> - native endian.
>> + native endian-ness.
>> \end{enumerate}
>>
>> \begin{lstlisting}
>> @@ -123,3 +155,22 @@ struct virtio_input_event {
>> le32 value;
>> };
>> \end{lstlisting}
>> +
>> +\drivernormative{\subsubsection}{Device Operation}{Device Types / Input Device / Device Operation}
>> +
>> +A driver SHOULD keep the eventq populated with buffers. These buffers
>> + MUST be device-writable and MUST be at least the size of
>> + struct virtio_input_event.
>> +
>> +Buffers placed into the statusq by a driver MUST be at least the size
>> + of struct virtio_input_event.
>> +
>> +\devicenormative{\subsubsection}{Device Operation}{Device Types / Input Device / Device Operation}
>> +
>> +A device MAY drop input events if the eventq does not have enough
>> + available buffers. It SHOULD NOT drop individual input events if
>> + they are part of a sequence forming one input device update. For
>> + example, a pointing device update typically consists of several
>> + input events, one for each axis, and a terminating EV_SYN event.
>> + A device SHOULD either buffer or drop the entire sequence.
>> +
>> --
>> 2.7.4
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]