[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [PATCH] Update virtio input device specification
On Wed, Apr 05, 2017 at 02:21:06PM +0200, Ladi Prosek wrote:
> 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.
Like this?
#define EVIOCGMTSLOTS(len) _IOC(_IOC_READ, 'E', 0x0a, len)
And how are the ioctl numbers relevant here?
Are the codes actually in
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/input-event-codes.h
by any chance?
Is anything besides codes relevant?
> But in general it's hard to be any more specific since it's
> a live header file.
How will readers of our spec figure it out?
You can refer to a fixed version too if that helps.
> >> 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.
Let's assume linux gains ability to generate new events on top of old
ones.
Do you want to send both in all cases?
If you only send multitouch how do you handle old guests?
If you send both how does new guest know it should
ignore old events?
> >> @@ -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".
The point is simple - we need to say what it does.
> >> \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.
I'm not really sure what to do here. Are we really exposing
such a large interface that we can't document it?
This is what the spec is about, if people have to dig into
code they can and will interpret it incorrectly, they
will make assumptions based on a specific implementation and
then their implementation will not work when you make
changes to the host.
> 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?
Second sentence seems to be dufficient provided bitmap is
defined in the header. What does this sentence about ioctl mean?
Can we just drop it?
> >> \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]