From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 57BB52F3639; Mon, 8 Sep 2025 09:26:15 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1757323578; cv=none; b=mOPzuLLge0/jT8iFk9QJ2iUNirr+F7FXYdrm0ORXnGou5zpNLk2MNexo2vocHuOxP9WtAcGVyl3XUVekQ4ApKt6w9nSqFGNB441/wbKdHsF1/G7vb7zJ0sxUI4SuMcYmknFlPl4iEgsvBUnWjz8ra1tmZ/K3goCN2gOC3UpO/OQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1757323578; c=relaxed/simple; bh=0AWaQxaQlIIqNlgSXKoasUU1wHEMugd/32PLRNlrnys=; h=Message-ID:Date:MIME-Version:From:Subject:To:Cc:References: In-Reply-To:Content-Type; b=i5kPpP8duP/3F6jUYNpbmkqN7Y0XRWUf1vSIvTGCi+KNYrV69No2PZHCjtMBOTaltMDBaQppOa+Edim9/+kBwJjRDXEsXYadA05n4xQ0UF0bWlvNZ5s4Ym4eJ3KmfXcKVpucW7Ejh8Jg4nrWGrRVyt8CWlaMVr0QXBcBN16LDyY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=IDuoZ167; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="IDuoZ167" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 90701C4CEF1; Mon, 8 Sep 2025 09:26:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1757323575; bh=0AWaQxaQlIIqNlgSXKoasUU1wHEMugd/32PLRNlrnys=; h=Date:From:Subject:To:Cc:References:In-Reply-To:From; b=IDuoZ1674J6bwXjvEP79XzyQly8WVfJbMJ8LTnhKsDg/EKueF+N4VvP+gqFOawWbw XLKPZWOJsuMmuYWoJBMpu6le4dec1FMR/P4lVTmXM2/ux+PT9v2E3mfeXrCdGJyyTP NQHTAcZNd8blghaR55gquxn9d9avRjgTENmxBGt6K31pwKGvn5ZMxhQs1ogr+kWLut lTxv9u1cVnMQJENItxdb85ISF+4NTFtlcYYXq5aHCfYx7W3Yotmv8C2/77GKalmHOU w/DvZ7ptxK55LaKjmB346TdcbJFXdyt8T7VEN/ywNQUe6OCs29jWxxnEVSF7TB8seu KOse4LhmQO9bw== Message-ID: <1c3a4ba0-f787-431d-a05b-865c059b8b90@kernel.org> Date: Mon, 8 Sep 2025 11:26:12 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Hans Verkuil Subject: Re: [PATCH] media: uvcvideo: Drop stream->mutex To: Ricardo Ribalda , Laurent Pinchart Cc: Hans de Goede , Mauro Carvalho Chehab , linux-media@vger.kernel.org, linux-kernel@vger.kernel.org References: <20250717-uvc-onelocksless-v1-1-91a1b834186a@chromium.org> <20250724120840.GL11202@pendragon.ideasonboard.com> <20250724155101.GA17890@pendragon.ideasonboard.com> <20250724200014.GT11202@pendragon.ideasonboard.com> Content-Language: en-US, nl In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 25/07/2025 10:59, Ricardo Ribalda wrote: > On Thu, 24 Jul 2025 at 22:00, Laurent Pinchart > 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 >>>>>>> --- >>>>>>> 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 > > >