Subject: Re: [PATCH v3] media: add virtio-media driver

From: Mauro Carvalho Chehab <[email-masked]>
Date: Tue, 27 May 2025 16:42:43 +0200

Em Tue, 27 May 2025 23:03:39 +0900
Alexandre Courbot <[email-masked]> escreveu:

> On Tue, May 27, 2025 at 10:35 PM Mauro Carvalho Chehab
> <[email-masked]> wrote:
> >
> > Em Tue, 27 May 2025 22:21:42 +0900
> > Alexandre Courbot <[email-masked]> escreveu:
> >  
> > > On Tue, May 27, 2025 at 6:13 PM Mauro Carvalho Chehab
> > > <[email-masked]> wrote:  
> > > >
> > > > Em Tue, 27 May 2025 15:14:50 +0900
> > > > "Alexandre Courbot" <[email-masked]> escreveu:
> > > >  
> > > > > Hi Mauro,
> > > > >
> > > > > On Mon May 26, 2025 at 9:13 PM JST, Mauro Carvalho Chehab wrote:  
> > > > > > Hi Michael,
> > > > > >
> > > > > > Em Sat, 12 Apr 2025 13:08:01 +0900
> > > > > > Alexandre Courbot <[email-masked]> escreveu:
> > > > > >  
> > > > > >> Add the first version of the virtio-media driver.
> > > > > >>
> > > > > >> This driver acts roughly as a V4L2 relay between user-space and the
> > > > > >> virtio virtual device on the host, so it is relatively simple, yet
> > > > > >> unconventional. It doesn't use VB2 or other frameworks typically used in
> > > > > >> a V4L2 driver, and most of its complexity resides in correctly and
> > > > > >> efficiently building the virtio descriptor chain to pass to the host,
> > > > > >> avoiding copies whenever possible. This is done by
> > > > > >> scatterlist_builder.[ch].
> > > > > >>
> > > > > >> virtio_media_ioctls.c proxies each supported ioctl to the host, using
> > > > > >> code generated through macros for ioctls that can be forwarded directly,
> > > > > >> which is most of them.
> > > > > >>
> > > > > >> virtio_media_driver.c provides the expected driver hooks, and support
> > > > > >> for mmapping and polling.
> > > > > >>
> > > > > >>  This version supports MMAP buffers, while USERPTR buffers can also be
> > > > > >>  enabled through a driver option. DMABUF support is still pending.  
> > > > > >
> > > > > > It sounds that you applied this one at the virtio tree, but it hasn't
> > > > > > being reviewed or acked by media maintainers.
> > > > > >
> > > > > > Please drop it.
> > > > > >
> > > > > > Alexandre,
> > > > > >
> > > > > > Please send media patches to media maintainers, c/c other subsystem
> > > > > > maintainers, as otherwise they might end being merged without a
> > > > > > proper review.  
> > > > >
> > > > > Sorry about that, I put everyone in "To:" without giving it a second
> > > > > thought.
> > > > >  
> > > > > >
> > > > > > In this particular case, we need to double-check if this won't cause
> > > > > > any issues, in special with regards to media locks and mutexes.  
> > > > >
> > > > > Agreed, I am not 100% confident about that part myself.
> > > > >  
> > > > > >
> > > > > > I'll try to look on it after this merge window, as it is too late
> > > > > > for it to be applied during this one.  
> > > > >
> > > > > Appreciate that - given the high traffic on the list I was worried that
> > > > > this patch would eventually be overlooked. Not making it for this merge
> > > > > window should not be a problem, so please take the time you need.  
> > > >
> > > > Provided that your patch was caught by patchwork, it won't be lost:
> > > >         https://patchwork.linuxtv.org/project/linux-media/patch/[email-masked]/
> > > >
> > > > Please notice that our CI got a number of checkpatch issues there.
> > > > Please check and fix the non-false-positive ones.  
> > >
> > > Will do. The macro-related ones are false-positives AFAICT. Some of
> > > the "lines should not end with a '('" are actually the result of
> > > applying clang-format with the kernel-provided style...  
> >
> > I don't know any lint tool that honors kernel style. The best one
> > is checkpatch with the auto-correcting parameter in pedantic mode,
> > but still one needs to manually review its output.
> >  
> > >  
> > > >
> > > > Btw, I was looking at:
> > > >
> > > >         https://github.com/chromeos/virtio-media
> > > >
> > > > (I'm assuming that this is the QEMU counterpart, right?)  
> > >
> > > crosvm actually, but QEMU support is also being worked on.  
> >
> > Do you have already QEMU patches? The best is to have the Kernel driver
> > submitted altogether with QEMU, as Kernel developers need it to do the
> > tests. In my case, I never use crosvm, and I don't have any Chromebook
> > anymore.  
> 
> IIRC Albert Esteve was working on this, maybe he can share the current status.

That would be nice.

> 
> Note that crosvm does not require a Chromebook, you can build and run
> it pretty easily on a regular PC. I have put together a document to
> help with that:
> 
> https://github.com/chromeos/virtio-media/blob/main/TRY_IT_OUT.md
> 
> It also shows how to proxy a host UVC camera into the guest and use
> some of the "fake" devices I talked about earlier.

Ok, I'll look on it, thanks for the hint!

> If following these
> steps is too much friction, just reading through the document should
> give you a decent idea of what virtio-media can do.
> 
> >  
> > > > And I noticed something weird there:
> > > >
> > > >         "Unsupported ioctls
> > > >
> > > >          A few ioctls are replaced by other, more suitable mechanisms. If being requested these ioctls, the device must return the same response as it would for an unknown ioctl, i.e. ENOTTY.
> > > >
> > > >             VIDIOC_QUERYCAP is replaced by reading the configuration area.
> > > >             VIDIOC_DQBUF is replaced by a dedicated event.
> > > >             VIDIOC_DQEVENT is replaced by a dedicated event."
> > > >
> > > > While this could be ok for cromeOS, this will be broken for guests with
> > > > Linux, as all Linux applications rely on VIDIOC_QUERYCAP and VIDIOC_DQBUF
> > > > to work. Please implement support for it, as otherwise we won't even be
> > > > able to test the driver with the v4l2-compliance tool (*).  
> > >
> > > The phrasing was a bit confusing. The guest driver does support these
> > > ioctls, and passes v4l2-compliance. So there is no problem here.  
> >
> > Please add v4l2-compliance output on the next patch series.  
> 
> Certainly. Note that the output is dependent on what the host provides
> for a device. If it e.g. proxies something that is not fully
> compliant, the guest will reflect the same errors.

Makes sense. In this case, the best is to add at the PR the v4l2-compliance
output for both the host real V4L2 driver and the guest one.

> 
> >  
> > > Where these ioctls are not supported is between the guest and the
> > > host, i.e. as ioctls encapsulated into a virtio request. For QUERYCAP,
> > > that's because the virtio configuration area already provides the same
> > > information. For DQBUF and DQEVENT, that's because we want to serve
> > > these asynchronously for performance reasons (hence the dedicated
> > > events).
> > >
> > > But these differences don't affect guest user applications which will
> > > be able to perform these ioctls exactly as they expect.  
> >
> > OK. Better to let it clear then at the documentation.
> >  
> > > >
> > > > (*) Passing at v4l2-compliance is a requirement for any media driver
> > > >     to be merged.
> > > >
> > > > With regards to testing, what's the expected testing scenario?
> > > > My guess is that, as a virtio device, a possible test scenario would be
> > > > to have the UVC camera from the host OS mapped using virtio-camera into
> > > > the guest OS, allowing a V4L2 application running at the guest to map the
> > > > camera from the host, right?  
> > >
> > > That's one of the scenarios, yes.  
> >
> > Ok, this is the easiest test scenario for media developers.
> >  
> > > Another one is to expose an accelerated decoder on the host to the guest.  
> >  
> > > One can also create
> > > "fake" devices backed by software on the host for testing purposes.  
> >
> > That sounds interesting. It probably makes sense to have some test
> > scenario using such fake devices plus v4l2-compliance test to check
> > for regressions running on some CI engine.  
> 
> Yes, regression catching in a CI is one of the use-cases we thought about.
> 
> >  
> > > That's actually the benefit of using V4L2 as a guest/host protocol:
> > > the same guest and the same software stack on the host can be used to  
> 
> I made a typo here: "the same guest DRIVER". That's an important point. :)

It was clear enough to me ;-)

> Cheers,
> Alex.