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

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

Em Tue, 27 May 2025 10:23:32 -0400
"Michael S. Tsirkin" <mst@redhat.com> escreveu:

> On Mon, May 26, 2025 at 02:13:16PM +0200, 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.
> > 
> > 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.
> > 
> > I'll try to look on it after this merge window, as it is too late
> > for it to be applied during this one.
> > 
> > Regards,
> > Mauro  
> 
> New drivers generally can be merged during the merge window,
> especially early. 

Sure, but this one was not reviewed or tested yet by media maintainers,
nor its submission came with the tests from the regression tool
we use (v4l2-compliance). In particular, we need to double-check
if it won't cause any issues with the complex set of mutexes and
spinlocks that we have within the core.

There is an additional concern related to V4L2: on media, only one
process is allowed to have exclusive streaming access to the
device: all other opens on the same device get permission denied
(by default - there is an optional ioctl that allows a process
to "abdicate" its streaming rights). We need to double-check how this
is implemented and how this would behavior when multiple VMs have
the driver installed and might try to use (or not), and how this
would affect the host access to the device.

There are also some coding style issues that cause our CI to
complain. Those are minor and could be fixed by a separate patch,
but better to have them placed altogether as otherwise our CI
will keep complaining about until the fix is merged.

On other words, this driver is not ready for merge yet.
We need some time to test and review it properly.

> It's up to you though.
> I can keep it in next for now, so it gets some coverage by
> tools scanning that tree.

Sure, feel free to keep it on next if you prefer so. Just
please don't submit it upstream while we don't review and
properly test it.

Thanks!
Mauro