Re: [PATCH v3 1/2] virtio-video: Add virtio video device specification

[Dmitry Sepp <dmitry.sepp@opensynergy.com>]

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.

> > +\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.

Best regards,
Dmitry.

> Any feedback is welcome.
> 
> Best regards,
> Keiichi
>