Message-ID: <1c3a4ba0-f787-431d-a05b-865c059b8b90@kernel.org>
Date: Mon, 8 Sep 2025 11:26:12 +0200
From: Hans Verkuil <hverkuil+cisco@...nel.org>
To: Ricardo Ribalda <ribalda@...omium.org>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Hans de Goede <hansg@...nel.org>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, linux-media@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: uvcvideo: Drop stream->mutex

On 25/07/2025 10:59, Ricardo Ribalda wrote:
> On Thu, 24 Jul 2025 at 22:00, Laurent Pinchart
> <laurent.pinchart@...asonboard.com> wrote:
>>
>> On Thu, Jul 24, 2025 at 08:15:48PM +0200, Ricardo Ribalda wrote:
>>> On Thu, 24 Jul 2025 at 17:51, Laurent Pinchart wrote:
>>>>
>>>> (CC'ing Hans Verkuil)
>>>>
>>>> On Thu, Jul 24, 2025 at 05:41:06PM +0200, Ricardo Ribalda wrote:
>>>>> On Thu, 24 Jul 2025 at 14:08, Laurent Pinchart wrote:
>>>>>> On Thu, Jul 17, 2025 at 07:56:45AM +0000, Ricardo Ribalda wrote:
>>>>>>> Since commit c93d73c9c2cf ("media: uvcvideo: Use vb2 ioctl and fop
>>>>>>> helpers"), the IOCTLs are serialized. Due to this there is no more need
>>>>>>> to protect ctrl, cur_format or cur_frame from concurrent access.
>>>>>>>
>>>>>>> Drop stream->mutex after thanking it for years of good service.
>>>>>>>
>>>>>>> Use this opportunity to do fix some CodeStyle.
>>>>>>
>>>>>> Is that about the following change only:
>>>>>>
>>>>>> -       if (format == NULL || frame == NULL) {
>>>>>> +       if (!format || !frame)
>>>>>>
>>>>>> or is there something else I missed ?
>>>>>
>>>>> I believe that's it.
>>>>>
>>>>>>> Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
>>>>>>> ---
>>>>>>>  drivers/media/usb/uvc/uvc_driver.c   |  4 ----
>>>>>>>  drivers/media/usb/uvc/uvc_metadata.c |  8 ++------
>>>>>>>  drivers/media/usb/uvc/uvc_v4l2.c     | 39 ++++++++----------------------------
>>>>>>>  drivers/media/usb/uvc/uvcvideo.h     |  6 ------
>>>>>>>  4 files changed, 10 insertions(+), 47 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
>>>>>>> index 775bede0d93d9b3e5391914aa395326d3de6a3b1..3039e6a533b82dd917050d416c9ced8756d69170 100644
>>>>>>> --- a/drivers/media/usb/uvc/uvc_driver.c
>>>>>>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>>>>>>> @@ -183,8 +183,6 @@ static void uvc_stream_delete(struct uvc_streaming *stream)
>>>>>>>       if (stream->async_wq)
>>>>>>>               destroy_workqueue(stream->async_wq);
>>>>>>>
>>>>>>> -     mutex_destroy(&stream->mutex);
>>>>>>> -
>>>>>>>       usb_put_intf(stream->intf);
>>>>>>>
>>>>>>>       kfree(stream->formats);
>>>>>>> @@ -201,8 +199,6 @@ static struct uvc_streaming *uvc_stream_new(struct uvc_device *dev,
>>>>>>>       if (stream == NULL)
>>>>>>>               return NULL;
>>>>>>>
>>>>>>> -     mutex_init(&stream->mutex);
>>>>>>> -
>>>>>>>       stream->dev = dev;
>>>>>>>       stream->intf = usb_get_intf(intf);
>>>>>>>       stream->intfnum = intf->cur_altsetting->desc.bInterfaceNumber;
>>>>>>> diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
>>>>>>> index 229e08ff323eed9129d835b24ea2e8085bb713b8..d1d4fade634bd3f8b12bbaa75388db42aecc25ea 100644
>>>>>>> --- a/drivers/media/usb/uvc/uvc_metadata.c
>>>>>>> +++ b/drivers/media/usb/uvc/uvc_metadata.c
>>>>>>> @@ -100,14 +100,10 @@ static int uvc_meta_v4l2_set_format(struct file *file, void *fh,
>>>>>>>        * Metadata buffers would still be perfectly parseable, but it's more
>>>>>>>        * consistent and cleaner to disallow that.
>>>>>>>        */
>>>>>>> -     mutex_lock(&stream->mutex);
>>>>>>> -
>>>>>>>       if (vb2_is_busy(&stream->meta.queue.queue))
>>>>>>> -             ret = -EBUSY;
>>>>>>> -     else
>>>>>>> -             stream->meta.format = fmt->dataformat;
>>>>>>> +             return -EBUSY;
>>>>>>>
>>>>>>> -     mutex_unlock(&stream->mutex);
>>>>>>> +     stream->meta.format = fmt->dataformat;
>>>>>>>
>>>>>>>       return ret;
>>>>>>
>>>>>>         return 0;
>>>>>>
>>>>>>>  }
>>>>>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
>>>>>>> index 160f9cf6e6dbdbf39e3eff56a5d5ea1d977fbe22..d7be4d59f0c73b983aa01321f4acc8f8bf6e83ef 100644
>>>>>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>>>>>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>>>>>>> @@ -329,14 +329,12 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
>>>>>>>        * developers test their webcams with the Linux driver as well as with
>>>>>>>        * the Windows driver).
>>>>>>>        */
>>>>>>> -     mutex_lock(&stream->mutex);
>>>>>>>       if (stream->dev->quirks & UVC_QUIRK_PROBE_EXTRAFIELDS)
>>>>>>>               probe->dwMaxVideoFrameSize =
>>>>>>>                       stream->ctrl.dwMaxVideoFrameSize;
>>>>>>>
>>>>>>>       /* Probe the device. */
>>>>>>>       ret = uvc_probe_video(stream, probe);
>>>>>>> -     mutex_unlock(&stream->mutex);
>>>>>>>       if (ret < 0)
>>>>>>>               return ret;
>>>>>>>
>>>>>>> @@ -395,19 +393,15 @@ static int uvc_ioctl_g_fmt(struct file *file, void *fh,
>>>>>>>       struct uvc_streaming *stream = handle->stream;
>>>>>>>       const struct uvc_format *format;
>>>>>>>       const struct uvc_frame *frame;
>>>>>>> -     int ret = 0;
>>>>>>>
>>>>>>>       if (fmt->type != stream->type)
>>>>>>>               return -EINVAL;
>>>>>>>
>>>>>>> -     mutex_lock(&stream->mutex);
>>>>>>>       format = stream->cur_format;
>>>>>>>       frame = stream->cur_frame;
>>>>>>>
>>>>>>> -     if (format == NULL || frame == NULL) {
>>>>>>> -             ret = -EINVAL;
>>>>>>> -             goto done;
>>>>>>> -     }
>>>>>>> +     if (!format || !frame)
>>>>>>> +             return -EINVAL;
>>>>>>>
>>>>>>>       fmt->fmt.pix.pixelformat = format->fcc;
>>>>>>>       fmt->fmt.pix.width = frame->wWidth;
>>>>>>> @@ -419,9 +413,7 @@ static int uvc_ioctl_g_fmt(struct file *file, void *fh,
>>>>>>>       fmt->fmt.pix.xfer_func = format->xfer_func;
>>>>>>>       fmt->fmt.pix.ycbcr_enc = format->ycbcr_enc;
>>>>>>>
>>>>>>> -done:
>>>>>>> -     mutex_unlock(&stream->mutex);
>>>>>>> -     return ret;
>>>>>>> +     return 0;
>>>>>>>  }
>>>>>>>
>>>>>>>  static int uvc_ioctl_s_fmt(struct file *file, void *fh,
>>>>>>> @@ -441,19 +433,14 @@ static int uvc_ioctl_s_fmt(struct file *file, void *fh,
>>>>>>>       if (ret < 0)
>>>>>>>               return ret;
>>>>>>>
>>>>>>> -     mutex_lock(&stream->mutex);
>>>>>>> -     if (vb2_is_busy(&stream->queue.queue)) {
>>>>>>> -             ret = -EBUSY;
>>>>>>> -             goto done;
>>>>>>> -     }
>>>>>>> +     if (vb2_is_busy(&stream->queue.queue))
>>>>>>> +             return -EBUSY;
>>>>>>>
>>>>>>>       stream->ctrl = probe;
>>>>>>>       stream->cur_format = format;
>>>>>>>       stream->cur_frame = frame;
>>>>>>>
>>>>>>> -done:
>>>>>>> -     mutex_unlock(&stream->mutex);
>>>>>>> -     return ret;
>>>>>>> +     return 0;
>>>>>>>  }
>>>>>>>
>>>>>>>  static int uvc_ioctl_g_parm(struct file *file, void *fh,
>>>>>>> @@ -466,9 +453,7 @@ static int uvc_ioctl_g_parm(struct file *file, void *fh,
>>>>>>>       if (parm->type != stream->type)
>>>>>>>               return -EINVAL;
>>>>>>>
>>>>>>> -     mutex_lock(&stream->mutex);
>>>>>>>       numerator = stream->ctrl.dwFrameInterval;
>>>>>>> -     mutex_unlock(&stream->mutex);
>>>>>>>
>>>>>>
>>>>>> You can drop the blank line here.
>>>>>>
>>>>>>>       denominator = 10000000;
>>>>>>>       v4l2_simplify_fraction(&numerator, &denominator, 8, 333);
>>>>>>> @@ -519,12 +504,9 @@ static int uvc_ioctl_s_parm(struct file *file, void *fh,
>>>>>>>       uvc_dbg(stream->dev, FORMAT, "Setting frame interval to %u/%u (%u)\n",
>>>>>>>               timeperframe.numerator, timeperframe.denominator, interval);
>>>>>>>
>>>>>>> -     mutex_lock(&stream->mutex);
>>>>>>>
>>>>>>
>>>>>> Double blank line.
>>>>>>
>>>>>>> -     if (uvc_queue_streaming(&stream->queue)) {
>>>>>>> -             mutex_unlock(&stream->mutex);
>>>>>>> +     if (uvc_queue_streaming(&stream->queue))
>>>>>>>               return -EBUSY;
>>>>>>> -     }
>>>>>>>
>>>>>>>       format = stream->cur_format;
>>>>>>>       frame = stream->cur_frame;
>>>>>>> @@ -556,14 +538,11 @@ static int uvc_ioctl_s_parm(struct file *file, void *fh,
>>>>>>>
>>>>>>>       /* Probe the device with the new settings. */
>>>>>>>       ret = uvc_probe_video(stream, &probe);
>>>>>>> -     if (ret < 0) {
>>>>>>> -             mutex_unlock(&stream->mutex);
>>>>>>> +     if (ret < 0)
>>>>>>>               return ret;
>>>>>>> -     }
>>>>>>>
>>>>>>>       stream->ctrl = probe;
>>>>>>>       stream->cur_frame = frame;
>>>>>>> -     mutex_unlock(&stream->mutex);
>>>>>>>
>>>>>>>       /* Return the actual frame period. */
>>>>>>>       timeperframe.numerator = probe.dwFrameInterval;
>>>>>>> @@ -941,10 +920,8 @@ static int uvc_ioctl_g_selection(struct file *file, void *fh,
>>>>>>>
>>>>>>>       sel->r.left = 0;
>>>>>>>       sel->r.top = 0;
>>>>>>> -     mutex_lock(&stream->mutex);
>>>>>>>       sel->r.width = stream->cur_frame->wWidth;
>>>>>>>       sel->r.height = stream->cur_frame->wHeight;
>>>>>>> -     mutex_unlock(&stream->mutex);
>>>>>>>
>>>>>>>       return 0;
>>>>>>>  }
>>>>>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
>>>>>>> index 757254fc4fe930ae61c9d0425f04d4cd074a617e..86765b9d7935f0888476249c3fb826cd7f36b35c 100644
>>>>>>> --- a/drivers/media/usb/uvc/uvcvideo.h
>>>>>>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>>>>>>> @@ -469,12 +469,6 @@ struct uvc_streaming {
>>>>>>>       const struct uvc_format *cur_format;
>>>>>>>       const struct uvc_frame *cur_frame;
>>>>>>>
>>>>>>> -     /*
>>>>>>> -      * Protect access to ctrl, cur_format, cur_frame and hardware video
>>>>>>> -      * probe control.
>>>>>>> -      */
>>>>>>> -     struct mutex mutex;
>>>>>>> -
>>>>>>
>>>>>> Could you please instead keep this mutex and drop uvc_video_queue.mutex
>>>>>> ? The rationale is that the same lock is now used to protect the queue
>>>>>> operations and to serialize the ioctls. It's therefore a higher-level
>>>>>> lock, which should be stored in the higher-level object, not in the
>>>>>> queue.
>>>>>>
>>>>>> You can then also drop the lock assignment in uvc_queue.c that reads
>>>>>>
>>>>>>         queue->queue.lock = &queue->mutex;
>>>>>>
>>>>>> as videobuf2 and the V4L2 core will use the video device lock when no
>>>>>> queue lock is set. The comment at the top of uvc_queue.c may need to be
>>>>>> updated.

No, you can't drop this. Once the last user (dvb) of the wait_prepare/finish callbacks
is removed, the vb2 code will change to:

https://patchwork.linuxtv.org/project/linux-media/patch/d2c5e21692652db13d55b3a8ec5c8bd04b308c3c.1749106659.git.hverkuil@xs4all.nl/

So just an unconditional mutex_lock(q->lock).

vb2_core_queue_init() will check that q->lock is set and WARN otherwise and return
an error.

It's up to the driver to decide which lock you set q->lock to. Usually that's the
same mutex as vdev->lock.

I think there are very few (if any) drivers that use a queue-specific lock. I once tried
to do that for the vivid driver, but it quickly descended into locking hell. But vivid
is quite complex in that regard, so it is not representative.

>>>>>
>>>>> Are we sure that it is exactly the same?
>>>>>
>>>>> There are places in videobuf2-core.c where we do not use video device lock.
>>>>>
>>>>> Eg:
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/common/videobuf2/videobuf2-core.c#n2056
>>>>>
>>>>> I'd rather keep the assignment to be in the safe side.
>>>>
>>>> There are lots of places where the vdev lock is used is the queue has no
>>>> lock. Hans, was is an oversight not to do it in __vb2_wait_for_done_vb()
>>>> ? If we don't want to support not setting the queue lock that's OK, but
>>>> we should then drop code that uses vdev->lock instead.

vb2 has no knowledge of struct video_device, so it relies on vb2_queue->lock
to know what mutex to unlock/lock. All drivers must set the vb2_queue lock.

As mentioned above, this will become compulsory once the DVB core has been
fixed (still waiting for Mauro to test it).

All V4L2 drivers should set q->lock already. If you know of any that do not,
then let me know.

Regards,

	Hans

>>>>
>>>> We can keep the assignment for the time being to be safe until that
>>>> issue gets resolved, but I'd still like to use the stream mutex instead
>>>> of the queue mutex.
>>>
>>> The problem with using the stream mutex is that the meta device and
>>> the capture device have the same uvc_streaming, but they need a
>>> different mutex.
>>>
>>> So if you do something like this:
>>>
>>> console0 # yavta -c /dev/video1 &
>>>
>>> console1# yavta -c /dev/video0 &
>>>
>>> You end in a deadlock. Where the DQBUF of video1 do not let you use video0
>>
>> Aarrghhh :-(
>>
>> I wouldn't expect a deadlock as DQBUF should release the lock when
>> waiting, but still, aarrrrgghhhhh :-(
>>
>>> We can add a second mutex to uvc_streaming.... but I think this is a
>>> bit overkill.
>>>
>>> Any ideas?
>>
>> I'm thinking it could make sense to move the video_device members of
>> uvc_streaming to uvc_video_queue and rename uvc_video_queue to
>> uvc_video_device. That's a change that should probably be done on top of
>> this patch, as it won't change the location of the mutex.
> 
> I have moved the video_device members.
> 
> But after playing a bit with renaming uvc_video_queue.... It does not
> look like a good idea. To do it properly we also need to rename
> variables and functions and the change will be pretty massive. Any
> future backport to stable is going to be hell....
> 
>>
>>>>>>>       /* Buffers queue. */
>>>>>>>       unsigned int frozen : 1;
>>>>>>>       struct uvc_video_queue queue;
>>>>>>>
>>>>>>> ---
>>>>>>> base-commit: d968e50b5c26642754492dea23cbd3592bde62d8
>>>>>>> change-id: 20250716-uvc-onelocksless-b66658e01f89
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
> 
> 
> 

Powered by blists - more mailing lists