Unnamed repository; edit this file 'description' to name the repository.
 help / color / mirror / Atom feed
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
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
> >
>
>

  reply	other threads:[~2020-05-29 14:24 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-06 10:20 [PATCH v3 0/2] Virtio " 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

Unnamed repository; edit this file 'description' to name the repository.

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://archive.lwn.net:8080/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ http://archive.lwn.net:8080/linux-media \
		linux-media@vger.kernel.org lwn-linux-media@archive.lwn.net
	public-inbox-index linux-media

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://archive.lwn.net/lwn.kernel.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git