Message-ID: <20250617140602.GD10006@pendragon.ideasonboard.com>
Date: Tue, 17 Jun 2025 17:06:02 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Ricardo Ribalda <ribalda@...omium.org>
Cc: Hans de Goede <hdegoede@...hat.com>, Hans Verkuil <hans@...erkuil.nl>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 5/5] media: uvcvideo: Use prio state from v4l2_device

On Tue, Jun 17, 2025 at 01:09:38PM +0200, Ricardo Ribalda wrote:
> On Tue, 17 Jun 2025 at 12:07, Laurent Pinchart
> <laurent.pinchart@...asonboard.com> wrote:
> >
> > On Mon, Jun 16, 2025 at 08:30:08PM +0200, Ricardo Ribalda wrote:
> > > On Mon, 16 Jun 2025 at 17:24, Ricardo Ribalda <ribalda@...omium.org> wrote:
> > > >
> > > > Currently, a UVC device can have multiple chains, and each chain maintains
> > > > its own priority state. While this behavior is technically correct for UVC,
> > > > uvcvideo is the *only* V4L2 driver that does not utilize the priority state
> > > > defined within `v4l2_device`.
> > > >
> > > > This patch modifies uvcvideo to use the `v4l2_device` priority state. While
> > > > this might not be strictly "correct" for uvcvideo's multi-chain design, it
> > > > aligns uvcvideo with the rest of the V4L2 drivers, providing "correct enough"
> > > > behavior and enabling code cleanup in v4l2-core. Also, multi-chain
> > > > devices are extremely rare, they are typically implemented as two
> > > > independent usb devices.
> > >
> > > As the cover letter says, this last patch 5/5 is a RFC. We can decide
> > > if it is worth to keep it or not.
> > >
> > > The pros is that we can do some cleanup in the core,
> >
> > What cleanups would that be ?
> >
> > > the cons is that it might break kAPI.
> >
> > Multi-chain devices are essentially multiple video devices inside a
> > single USB function. They are exposed as completely separate devices to
> > userspace, having the priority ioctls on one chain impact the other
> > chain wouldn't make much sense to me. I think we should drop this patch.
> 
> Ack, let's drop it.
> 
> PS: Do you know about a multi chain device that is commercially
> available? I would love to buy one for testing.
> Also do you know any "output" device that I can buy?

The only output device I've worked with was a custom camera developed by
a customer that had a "UVC to VGA" output path. It was a loooooong time
ago, I don't have a device to test UVC output anymore.

> > > I checked in the debian sourcecode and I could only find a user of
> > > PRIORITY for dvb and was optional.
> >
> > We could discuss deprecating the priority ioctls overall if we think
> > they're not useful (and used) by userspace. I was however considering
> > using them in libcamera though, to prevent other applications from
> > modifying the camera configuration behind the library's back.
> 
> For the record:
> From: https://codesearch.debian.net/search?q=VIDIOC_S_PRIORITY
> If I am not wrong, this is the only relevant usecase:
> https://sources.debian.org/src/zvbi/0.2.44-1/daemon/proxyd.c/?hl=1523#L1523
> 
> O_EXCL does not work for you?

I haven't tried it, but tt's defined as

    O_EXCL Ensure that this call creates the file: if this flag is
	   specified in conjunction with O_CREAT, and pathname already
	   exists, then open() fails with the error EEXIST.

	   When these two flags are specified, symbolic links are not
	   followed: if pathname is a symbolic link, then open() fails
	   regardless of where the symbolic link points.

	   In general, the behavior of O_EXCL is undefined if it is used
	   without O_CREAT.  There is one exception: on Linux 2.6 and
	   later, O_EXCL can be used without O_CREAT  if  pathname
	   refers to a block device.  If the block device is in use by
	   the system (e.g., mounted), open() fails with the error
	   EBUSY.

so I don't expect it would work.

It's not a big deal, libcamera doesn't get exclusive access to video
devices today and the world doesn't collapse (at least not because of
this specific issue). And we don't have priority support on subdevs, so
we couldn't solve the whole problem anyway.

> > > > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> > > > ---
> > > >  drivers/media/usb/uvc/uvc_driver.c | 2 --
> > > >  drivers/media/usb/uvc/uvcvideo.h   | 1 -
> > > >  2 files changed, 3 deletions(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > index accfb4ca3c72cb899185ddc8ecf4e29143d58fc6..e3795e40f14dc325e5bd120f5f45b60937841641 100644
> > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > @@ -1728,7 +1728,6 @@ static struct uvc_video_chain *uvc_alloc_chain(struct uvc_device *dev)
> > > >         INIT_LIST_HEAD(&chain->entities);
> > > >         mutex_init(&chain->ctrl_mutex);
> > > >         chain->dev = dev;
> > > > -       v4l2_prio_init(&chain->prio);
> > > >
> > > >         return chain;
> > > >  }
> > > > @@ -2008,7 +2007,6 @@ int uvc_register_video_device(struct uvc_device *dev,
> > > >         vdev->fops = fops;
> > > >         vdev->ioctl_ops = ioctl_ops;
> > > >         vdev->release = uvc_release;
> > > > -       vdev->prio = &stream->chain->prio;
> > > >         vdev->queue = &queue->queue;
> > > >         vdev->lock = &queue->mutex;
> > > >         if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
> > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > > index 3e6d2d912f3a1cfcf63b2bc8edd3f86f3da305db..5ed9785d59c698cc7e0ac69955b892f932961617 100644
> > > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > > @@ -354,7 +354,6 @@ struct uvc_video_chain {
> > > >                                                  * uvc_fh.pending_async_ctrls
> > > >                                                  */
> > > >
> > > > -       struct v4l2_prio_state prio;            /* V4L2 priority state */
> > > >         u32 caps;                               /* V4L2 chain-wide caps */
> > > >         u8 ctrl_class_bitmap;                   /* Bitmap of valid classes */
> > > >  };

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists