Re: [PATCH v2 0/1] Virtio Video V4L2 driver

Keiichi Watanabe
2020-03-13T11:53:30Z

On Fri, Mar 13, 2020 at 7:09 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
>
> Hi Nicolas, Keiichi,
>
> On Freitag, 13. März 2020 08:54:13 CET Keiichi Watanabe wrote:
> > Hi Nicolas,
> >
> > On Fri, Mar 13, 2020 at 11:29 AM Nicolas Dufresne <nicolas@ndufresne.ca>
> wrote:
> > > Hi Dimitry,
> > >
> > > Le jeudi 12 mars 2020 à 11:29 +0100, Dmitry Sepp a écrit :
> > > > Hi Hans,
> > > >
> > > > I'm not sure about crosvm, for us it is probably still feasible to
> > > > implement FWHT in the device (but it is unfortunately not supposed to
> > > > be upstreamed yet).
> > > >
> > > > The main question is what would be the proper user-space tool to test
> > > > that? Is v4l2-ctl OK for that? As for gstreamer, AFAIK it does not
> > > > respect the v4l2 Video Decoder Interface Spec and we have seen some
> > > > issues with it.>
> > > GStreamer element has been implemented to support what real drivers do,
> > > not what the spec suggest should be done. AFAIC, not all drivers have
> > > been updated with all the new features required by the spec. And the
> > > new features are not compatible with drivers that are not ported, so it
> > > creates a lot of complexity for userspace to stay backward compatible.
> > > Though, the spec should allow drivers to stay backward compatible, if
> > > not, we'd be very happy to learn why.
> > >
> > > About the other issues, I'd be very happy to learn what they are, it's
> > > the first feedback I get from your team thus far. Please feel free to
> > > file issues or send me (or gstreamer-devel list) an email.
> > >
> > > In term of userspace, please consider FFMPEG also, as it's support for
> > > V4L2 Stateful CODECs has gained momentum. It is again implemented to
> > > support real drivers, but started much more recently targetting the
> > > Qualcomm/Venus driver, so it didn't have to suffer all the legacy and
> > > directions changes in the V4L2 API since 2011.
> > >
> > > As for the virtio video driver, it will remain useless to non-
> > > chromeos/chrome users as long as it's not supported by any userspace.
> > > I'd be very happy so see more contribution into FFMPEG and GStreamer to
> > > implement the features that, for now, are only implemented in the
> > > spec.
> > >
> > > From your comment, bridging a Linux driver in the host through virtio-
> > > video seems like a huge tasks. I believe this is an important issue to
> > > address in the long term. If you could provide more details, I think it
> > > would benefit to readers understanding.
> >
> > Just to clarify, Dmitry is not working for ChromeOS but for another
> > hypervisor (, which is not OSS?). So, users are not limited to
> > ChromeOS.
> > But, yeah, I agree that it's important to make it easy to use
> > virtio-video driver without special hardware.
> >
> > Let me explain what is the remaining task.
> > To test virtio-video driver, we need to take care of four parts:
> > Guest user application, guest virtio-video driver (this patch), host
> > virtio-video device, and host video codec device.
> > We have several options for each parts:
> >
> > Guest userspace | Guest kernel | Host userspace hypervisor | Host video
> > device
> > ---------------------------------------------------------------------------
> > - * FFMPEG | virtio-video | virtio-video device in | * real driver *
> > GStreamer | driver | * crosvm | * vicodec *
> > v4l2-ctl | | * Dmitry's hypervisor | * SW en/decoder
> > | | * QEMU? |
> >
> > The remaining task I commented is implementation of virtio-video
> > device in a host hypervisor.
>
> Oh, there is no Dmitry's hypervisor unfortunately :) It is the COQOS
> Automotive hypervisor. And of course it does not use SW codecs on the host
> side. Yes, correct, the hypervisor is not OSS for obvious reasons.

Ah, I meant Dmitry's team's hypervisor:) The name of COQOS slipped my
mind. Sorry!

I added SW codecs in host device column because it's technically
possible to use SW codec as a backend video device for testing
purposes.
But, the word "platform codecs" sounds more general. The updated table
looks better to me.

>
> For us one of the most important guest side users is Android's multimedia
> subsystem, so I'll update the table in this way:
>
> Guest userspace | Guest kernel | Host userspace hypervisor | Host video device
> ----------------------------------------------------------------------------
> * FFMPEG | virtio-video | virtio-video device in | * real driver
> * GStreamer | driver | * crosvm | * vicodec
> * Codec2 HAL | virtio-video | * COQOS hypervisor | * Platform Codecs
> | | * QEMU? |
>
> > What a virtio-video device does is forwarding guest driver's
> > virtio-video commands to host's video decoder/encoder, and vice versa.
> > If we use vicodec as a backend host video device, this part's task is
> > protocol translation between virtio-video and V4L2 stateful API.
> >
>
> There is some translation anyway, so from my point of view the problem to
> implement this is not that difficult, it just requires some more time. This
> could be a foundation for some QEMU codec device.

I agree that it's not technically difficult. It's a kind of inversion
of translation that virtio-video driver does.

Best regards,
Keiichi

>
> Best regards,
> Dmitry.
>
> > Though there are two virtio-video device implementation in the world
> > at least (one in crosvm and the other in Dmitry's team's hypervisor),
> > I guess both hypervisor implementations are not easy to use without
> > special devices.
> > So, we might want to have one more virtio-video device implementation
> > in QEMU in the long term.
> > It will be somehow similar to virtio-gpu device that is already
> > implemented in QEMU:
> > https://github.com/qemu/qemu/blob/master/hw/display/virtio-gpu.c
> >
> > Best regards,
> > Keiichi
> >
> > > > Best regards,
> > > > Dmitry.
> > > >
> > > > On Donnerstag, 12. März 2020 10:54:35 CET Hans Verkuil wrote:
> > > > > On 3/12/20 10:49 AM, Keiichi Watanabe wrote:
> > > > > > Hi Hans,
> > > > > >
> > > > > > On Wed, Mar 11, 2020 at 10:26 PM Hans Verkuil <hverkuil@xs4all.nl>
> wrote:
> > > > > > > Hi Dmitry,
> > > > > > >
> > > > > > > On 2/18/20 9:27 PM, Dmitry Sepp wrote:
> > > > > > > > Hi all,
> > > > > > > >
> > > > > > > > This is a v4l2 virtio video driver for the virtio-video device
> > > > > > > > specification v3 [1].
> > > > > > > >
> > > > > > > > The first version of the driver was introduced here [2].
> > > > > > > >
> > > > > > > > Changes v1 -> v2:
> > > > > > > > * support the v3 spec (mostly)
> > > > > > > > * add a module parameter to ask for pages from ZONE_DMA
> > > > > > > >
> > > > > > > > What is not implemented:
> > > > > > > > * Plane layout flags should be used to propagate number of
> > > > > > > > planes to
> > > > > > > >
> > > > > > > > user-space
> > > > > > > >
> > > > > > > > * There is no real use of stream creation with bitstream format
> > > > > > > > in the
> > > > > > > >
> > > > > > > > parameter list. The driver just uses the first bitstream
> > > > > > > > format from
> > > > > > > > the list.
> > > > > > > >
> > > > > > > > * Setting bitrate is done in a different way compared to the
> > > > > > > > spec. This
> > > > > > > >
> > > > > > > > is because it has been already agreed on that the way the spec
> > > > > > > > currently describes it requires changes.
> > > > > > > >
> > > > > > > > Potential improvements:
> > > > > > > > * Do not send stream_create from open. Use corresponding state
> > > > > > > > machine
> > > > > > > >
> > > > > > > > condition to do this.
> > > > > > > >
> > > > > > > > * Do not send stream_destroy from close. Do it in reqbufs(0).
> > > > > > > > * Cache format and control settings. Reduce calls to the device.
> > > > > > >
> > > > > > > Some general notes:
> > > > > > >
> > > > > > > Before this can be merged it needs to pass v4l2-compliance.
> > > > > > >
> > > > > > > I also strongly recommend adding support for V4L2_PIX_FMT_FWHT to
> > > > > > > allow testing with the vicodec emulation driver. This will also
> > > > > > > allow testing all sorts of corner cases without requiring special
> > > > > > > hardware.
> > > > > >
> > > > > > I agree that it's great if we could test virtio-video with vicodec,
> > > > > > but I wonder if supporting FWHT is actually needed for the initial
> > > > > > patch.
> > > > > > Though it wouldn't be difficult to support FWHT in the driver, we
> > > > > > also
> > > > > > needs to support it in the host's hypervisor to test it. It's not
> > > > > > easy
> > > > > > for our hypervisor to support FWHT, as it doesn't talk to v4l2
> > > > > > driver
> > > > > > directly.
> > > > > > Without the host-side implementation, it makes no sense to support
> > > > > > it.
> > > > > > Also, if we support FWHT, we should have the format in a list of
> > > > > > supported formats in the virtio specification. However, I'm not sure
> > > > > > if FWHT is a general codec enough to be added in the spec, which
> > > > > > shouldn't be specific to Linux.
> > > > >
> > > > > Good point, I didn't know that.
> > > > >
> > > > > Is it possible to add support for FWHT under a linux debug/test
> > > > > option?
> > > > >
> > > > > It's really the main reason for having this, since you would never use
> > > > > this in production code. But it is so nice to have for regression
> > > > > testing.
> > > > >
> > > > > Regards,
> > > > >
> > > > > Hans
> > > > > >
> > > > > > Best regards,
> > > > > > Keiichi
> > > > > >
> > > > > > > Regards,
> > > > > > >
> > > > > > > Hans
> > > > > > > >
> > > > > > > > Best regards,
> > > > > > > > Dmitry.
> > > > > > > >
> > > > > > > > [1] https://markmail.org/message/dmw3pr4fuajvarth
> > > > > > > > [2] https://markmail.org/message/wnnv6r6myvgb5at6
>
>