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

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

Em Tue, 27 May 2025 15:14:50 +0900
"Alexandre Courbot" <gnurou@gmail.com> 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 <gnurou@gmail.com> 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/20250412-virtio-media-v3-1-97dc94c18398@gmail.com/

Please notice that our CI got a number of checkpatch issues there. 
Please check and fix the non-false-positive ones.

Btw, I was looking at:

	https://github.com/chromeos/virtio-media

(I'm assuming that this is the QEMU counterpart, right?)

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 (*).

(*) 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?

Regards,
Mauro