From: Keiichi Watanabe <keiichiw@chromium.org>
To: Dmitry Sepp <dmitry.sepp@opensynergy.com>
Cc: virtio-dev@lists.oasis-open.org,
"Linux Media Mailing List" <linux-media@vger.kernel.org>,
"Alexandre Courbot" <acourbot@chromium.org>,
"Alex Lau" <alexlau@chromium.org>,
"Daniel Vetter" <daniel@ffwll.ch>,
"Dylan Reid" <dgreid@chromium.org>,
"David Staessens" <dstaessens@chromium.org>,
"Enrico Granata" <egranata@google.com>,
"Frediano Ziglio" <fziglio@redhat.com>,
"Hans Verkuil" <hverkuil@xs4all.nl>,
"Gerd Hoffmann" <kraxel@redhat.com>,
"Stéphane Marchesin" <marcheu@chromium.org>,
"Pawel Osciak" <posciak@chromium.org>,
spice-devel@lists.freedesktop.org,
"David Stevens" <stevensd@chromium.org>,
"Tomasz Figa" <tfiga@chromium.org>,
uril@redhat.com,
"Samiullah Khawaja" <samiullah.khawaja@opensynergy.com>,
"Kiran Pawar" <kiran.pawar@opensynergy.com>
Subject: Re: [PATCH v3 1/2] virtio-video: Add virtio video device specification
Date: Fri, 29 May 2020 23:21:49 +0900 [thread overview]
Message-ID: <CAD90VcZuvDj+-fdM89w-gYH=v4vc7x=R3sn032An4-vAX6hN0A@mail.gmail.com> (raw)
In-Reply-To: <3754945.rEp4dnK8H5@os-lin-dmo>
Hi Dmitry,
On Wed, May 27, 2020 at 9:12 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
>
> Hi Keiichi,
>
> On Montag, 18. Mai 2020 07:17:53 CEST Keiichi Watanabe wrote:
> > > +struct virtio_video_stream_create {
> > > + struct virtio_video_cmd_hdr hdr;
> > > + le32 in_mem_type; /* One of VIRTIO_VIDEO_MEM_TYPE_* types */
> > > + le32 out_mem_type; /* One of VIRTIO_VIDEO_MEM_TYPE_* types */
> > > + le32 coded_format; /* One of VIRTIO_VIDEO_FORMAT_* types */
> > > + u8 padding[4];
> > > + u8 tag[64];
> > > +};
> > > +\end{lstlisting}
> > > +\begin{description}
> > > +\item[\field{in_mem_type, out_mem_type}] is a type of buffer
> > > + management for input /output buffers. The driver MUST set a value in
> > > + \field{enum virtio_video_mem_type} that the device reported a
> > > + corresponding feature bit.
> > > +\begin{description}
> > > +\item[\field{VIRTIO_VIDEO_MEM_TYPE_GUEST_PAGES}] Use guest pages.
> > > +\end{description}
> > > +\item[\field{coded_format}] is the encoded format that will be
> > > + processed.
> > > +\item[\field{tag}] is the name associated with this stream. The tag
> > > + MUST be encoded in UTF-8 and NUL-terminated.
> >
> > I wonder why we need this "tag" field. I have kept this field from
> > Dmitry's first proposal, where this was called "char debug_name[64]".
> > However, on second thought, I have no idea what is the necessity to
> > have this field. Our VMM implementation in ChromeOS simply ignores
> > this field.
> > If OpenSynergy's implementation relies on this field, I'm curious
> > about the usage. We might want to have an enum value instead of this
> > field where arbitrary values can be stored.
> >
>
> The use of this field is not so clear because it was renamed. In fact, one can
> have an idea how it is used by simply looking at the driver code: the field is
> useful to know about the guest client app that uses the context. If someone
> wants to store arbitrary values, they have 64 bytes to do so with this so-
> called tag.
Hmm, though I understand this can be useful for you, I don't think we
should support it in the standard.
For the first example, I feel something is not abstracted well if you
want to send some information from a user app to the host device. User
applications shouldn't have a way to send messages to hardware
directly.
For the second example, who is "someone"? Driver or device? In any
case, I don't think it's the right way. They should extend existing
structs or add commands or feature flags, I think. Also, if arbitrary
values are allowed, the field won't be used correctly except in cases
where both driver implementation and device implementation are
available. This is against what the spec should be: virtio protocol
must work independently from the implementations.
Of course, it's obviously okay to have it as a downstream extension in
your product's local repository.
>
> > > +\end{description}
> > > +
> > > +The driver MUST set \field{stream_id} in \field{virtio_video_cmd_hdr}
> > > +to an integer that is not used before. If a used value is passed as
> > > +\field{stream_id}, the device MUST reports an error with
> > > +VIRTIO_VIDEO_RESP_ERR_INVALID_STREAM_ID.
> >
> > I'm wondering if we can't generate stream_id in the host side so that
> > we will have less error control code. In the current design, both the
> > device and the driver have error checks; the device must check that a
> > given ID is available and the driver must check if the device didn't
> > return the INVALID_STREAM_ID error. Instead, by generating IDs in the
> > device, we will be free from this type of failure. Same for
> > resource_id in RESOURCE_CREATE.
> >
> > I guess this design originally came from the virtio-gpu protocol.
> > However, I couldn't find a benefit of adopting the same design here.
> >
>
> Honestly I don't see too much difference: device still needs to check whether
> the id provided by the driver within some particular command is correct. If it
> is not, it will return an error. The driver needs to check (or skip checking)
> for an error either way as long as it is possible for the driver code to send
> a wrong number.
I'm talking about creation commands only. So, other commands won't be affected.
Let me try to explain my idea in a different way. The relationship
between the driver and the device can be seen as a client-server
model.
The client (driver) sends a request and the server (device) sends a
response by processing or generating some data.
Thus, I feel it's more natural that new data, including IDs, are
generated and provided by the device.
Best regards,
Keiichi
>
> Best regards,
> Dmitry.
>
> > Any feedback is welcome.
> >
> > Best regards,
> > Keiichi
> >
>
>
next prev parent reply other threads:[~2020-05-29 14:22 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-06 10:20 [PATCH v3 0/2] Virtio video device specification Keiichi Watanabe
2020-02-06 10:20 ` [PATCH v3 1/2] virtio-video: Add virtio " Keiichi Watanabe
2020-02-25 9:59 ` Gerd Hoffmann
2020-02-27 7:24 ` Keiichi Watanabe
2020-02-27 9:28 ` Gerd Hoffmann
2020-03-04 4:31 ` Alexandre Courbot
2020-03-04 6:42 ` Gerd Hoffmann
2020-03-04 10:07 ` Alexandre Courbot
2020-03-23 12:07 ` Keiichi Watanabe
2020-03-23 13:28 ` Dmitry Sepp
2020-03-23 15:48 ` Keiichi Watanabe
2020-03-25 9:47 ` Dmitry Sepp
2020-03-27 3:35 ` Keiichi Watanabe
2020-03-30 9:53 ` Dmitry Sepp
2020-04-06 9:32 ` Alexandre Courbot
2020-04-06 11:46 ` Keiichi Watanabe
2020-04-07 9:21 ` Dmitry Sepp
2020-04-09 10:46 ` Keiichi Watanabe
2020-04-17 8:08 ` Dmitry Sepp
2020-04-20 9:57 ` Keiichi Watanabe
2020-04-21 8:38 ` Dmitry Sepp
2020-04-24 11:42 ` Keiichi Watanabe
2020-04-27 14:28 ` Dmitry Sepp
2020-04-07 14:49 ` Dmitry Sepp
2020-04-09 10:46 ` Keiichi Watanabe
2020-04-09 13:13 ` Dmitry Sepp
2020-04-24 11:45 ` Keiichi Watanabe
2020-04-27 9:33 ` Dmitry Sepp
2020-05-18 5:17 ` Keiichi Watanabe
2020-05-27 12:12 ` Dmitry Sepp
2020-05-29 14:21 ` Keiichi Watanabe [this message]
2020-06-01 7:19 ` Alexandre Courbot
2020-02-06 10:20 ` [PATCH v3 2/2] virtio-video: Define a feature for exported objects from different virtio devices Keiichi Watanabe
2020-02-25 10:01 ` Gerd Hoffmann
2020-02-27 7:24 ` Keiichi Watanabe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAD90VcZuvDj+-fdM89w-gYH=v4vc7x=R3sn032An4-vAX6hN0A@mail.gmail.com' \
--to=keiichiw@chromium.org \
--cc=acourbot@chromium.org \
--cc=alexlau@chromium.org \
--cc=daniel@ffwll.ch \
--cc=dgreid@chromium.org \
--cc=dmitry.sepp@opensynergy.com \
--cc=dstaessens@chromium.org \
--cc=egranata@google.com \
--cc=fziglio@redhat.com \
--cc=hverkuil@xs4all.nl \
--cc=kiran.pawar@opensynergy.com \
--cc=kraxel@redhat.com \
--cc=linux-media@vger.kernel.org \
--cc=marcheu@chromium.org \
--cc=posciak@chromium.org \
--cc=samiullah.khawaja@opensynergy.com \
--cc=spice-devel@lists.freedesktop.org \
--cc=stevensd@chromium.org \
--cc=tfiga@chromium.org \
--cc=uril@redhat.com \
--cc=virtio-dev@lists.oasis-open.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).