[PATCH] media: i2c: ov5647: use our own mutex for the ctrl lock

[PATCH] media: i2c: ov5647: use our own mutex for the ctrl lock

[Xiaolei Wang]

__v4l2_ctrl_handler_setup() and __v4l2_ctrl_modify_range()
contains an assertion to verify that the v4l2_ctrl_handler::lock
is held, as it should only be called when the lock has already
been acquired. Therefore use our own mutex for the ctrl lock,
otherwise a warning will be  reported.

Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
---
 drivers/media/i2c/ov5647.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index e193fef4fced..4e14eefba577 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -1288,9 +1288,12 @@ static int ov5647_init_controls(struct ov5647 *sensor)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
 	int hblank, exposure_max, exposure_def;
+	struct v4l2_ctrl_handler *hdl = &sensor->ctrls;
 
 	v4l2_ctrl_handler_init(&sensor->ctrls, 9);
 
+	hdl->lock = &sensor->lock;
+
 	v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
 			  V4L2_CID_AUTOGAIN, 0, 1, 1, 0);
 
-- 
2.43.0

Re: [PATCH] media: i2c: ov5647: use our own mutex for the ctrl lock

[johannes.goede]

Hi,

On 1-Dec-25 1:00 AM, Xiaolei Wang wrote:
> __v4l2_ctrl_handler_setup() and __v4l2_ctrl_modify_range()
> contains an assertion to verify that the v4l2_ctrl_handler::lock
> is held, as it should only be called when the lock has already
> been acquired. Therefore use our own mutex for the ctrl lock,
> otherwise a warning will be  reported.
> 
> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>

Generally speaking as a default locking setup for sensor
drivers we are moving in the direction of removing driver
specific locks and instead using the control-handler
lock everywhere, including using it as the active state
lock, see e.g. :

https://lore.kernel.org/linux-media/20250313184314.91410-14-hdegoede@redhat.com/

which sets ov02c10->sd.state_lock = ov02c10->ctrl_handler.lock
and then removes a bunch of manual mutex_lock / unlock calls
since all ops which get called with a sd_state will already
have the lock called when operating on the active_state
(and when called in try mode they should not touch anything
needing locking).

Note if you also want to make the ctrl_handler lock
the active state lock then you need to add calls to
v4l2_subdev_init_finalize() / v4l2_subdev_cleanup()
to allocate the active-state to probe().

Regards,

Hans




> ---
>  drivers/media/i2c/ov5647.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index e193fef4fced..4e14eefba577 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -1288,9 +1288,12 @@ static int ov5647_init_controls(struct ov5647 *sensor)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
>  	int hblank, exposure_max, exposure_def;
> +	struct v4l2_ctrl_handler *hdl = &sensor->ctrls;
>  
>  	v4l2_ctrl_handler_init(&sensor->ctrls, 9);
>  
> +	hdl->lock = &sensor->lock;
> +
>  	v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
>  			  V4L2_CID_AUTOGAIN, 0, 1, 1, 0);
>  

Re: [PATCH] media: i2c: ov5647: use our own mutex for the ctrl lock

[Sakari Ailus]

Hi Hans, Xiaolei,

On Mon, Dec 01, 2025 at 10:31:59AM +0100, johannes.goede@oss.qualcomm.com wrote:
> Hi,
> 
> On 1-Dec-25 1:00 AM, Xiaolei Wang wrote:
> > __v4l2_ctrl_handler_setup() and __v4l2_ctrl_modify_range()
> > contains an assertion to verify that the v4l2_ctrl_handler::lock
> > is held, as it should only be called when the lock has already
> > been acquired. Therefore use our own mutex for the ctrl lock,
> > otherwise a warning will be  reported.
> > 
> > Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
> 
> Generally speaking as a default locking setup for sensor
> drivers we are moving in the direction of removing driver
> specific locks and instead using the control-handler
> lock everywhere, including using it as the active state
> lock, see e.g. :
> 
> https://lore.kernel.org/linux-media/20250313184314.91410-14-hdegoede@redhat.com/
> 
> which sets ov02c10->sd.state_lock = ov02c10->ctrl_handler.lock
> and then removes a bunch of manual mutex_lock / unlock calls
> since all ops which get called with a sd_state will already
> have the lock called when operating on the active_state
> (and when called in try mode they should not touch anything
> needing locking).
> 
> Note if you also want to make the ctrl_handler lock
> the active state lock then you need to add calls to
> v4l2_subdev_init_finalize() / v4l2_subdev_cleanup()
> to allocate the active-state to probe().

I agree with the above, but the driver is old and it uses its own lock to
serialise access to its data structures while it uses the control lock
separately. So this looks like a bugfix that could be backported.

I wonder if anyone still has a system with this sensor.

-- 
Regards,

Sakari Ailus

Re: [PATCH] media: i2c: ov5647: use our own mutex for the ctrl lock

[Jacopo Mondi]

Hello

On Mon, Dec 01, 2025 at 03:02:11PM +0200, Sakari Ailus wrote:
> Hi Hans, Xiaolei,
>
> On Mon, Dec 01, 2025 at 10:31:59AM +0100, johannes.goede@oss.qualcomm.com wrote:
> > Hi,
> >
> > On 1-Dec-25 1:00 AM, Xiaolei Wang wrote:
> > > __v4l2_ctrl_handler_setup() and __v4l2_ctrl_modify_range()
> > > contains an assertion to verify that the v4l2_ctrl_handler::lock
> > > is held, as it should only be called when the lock has already
> > > been acquired. Therefore use our own mutex for the ctrl lock,
> > > otherwise a warning will be  reported.
> > >
> > > Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
> >
> > Generally speaking as a default locking setup for sensor
> > drivers we are moving in the direction of removing driver
> > specific locks and instead using the control-handler
> > lock everywhere, including using it as the active state
> > lock, see e.g. :
> >
> > https://lore.kernel.org/linux-media/20250313184314.91410-14-hdegoede@redhat.com/
> >
> > which sets ov02c10->sd.state_lock = ov02c10->ctrl_handler.lock
> > and then removes a bunch of manual mutex_lock / unlock calls
> > since all ops which get called with a sd_state will already
> > have the lock called when operating on the active_state
> > (and when called in try mode they should not touch anything
> > needing locking).
> >
> > Note if you also want to make the ctrl_handler lock
> > the active state lock then you need to add calls to
> > v4l2_subdev_init_finalize() / v4l2_subdev_cleanup()
> > to allocate the active-state to probe().
>
> I agree with the above, but the driver is old and it uses its own lock to
> serialise access to its data structures while it uses the control lock
> separately. So this looks like a bugfix that could be backported.
>
> I wonder if anyone still has a system with this sensor.

ov5647 is the rpi camera module v1, so I guess it's still around even
if a bit old. Dave in cc is the expert and maintainer of this driver.

Jai has a series in review to upstream all the remaining BSP patches
for this driver.
https://lore.kernel.org/all/20251118-b4-rpi-ov5647-v2-0-5e78e7cb7f9b@ideasonboard.com/

I'll cc him as well

>
> --
> Regards,
>
> Sakari Ailus
>

Re: [PATCH] media: i2c: ov5647: use our own mutex for the ctrl lock

[Dave Stevenson]

Hi Sakari

On Mon, 1 Dec 2025 at 13:58, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
>
> Hello
>
> On Mon, Dec 01, 2025 at 03:02:11PM +0200, Sakari Ailus wrote:
> > Hi Hans, Xiaolei,
> >
> > On Mon, Dec 01, 2025 at 10:31:59AM +0100, johannes.goede@oss.qualcomm.com wrote:
> > > Hi,
> > >
> > > On 1-Dec-25 1:00 AM, Xiaolei Wang wrote:
> > > > __v4l2_ctrl_handler_setup() and __v4l2_ctrl_modify_range()
> > > > contains an assertion to verify that the v4l2_ctrl_handler::lock
> > > > is held, as it should only be called when the lock has already
> > > > been acquired. Therefore use our own mutex for the ctrl lock,
> > > > otherwise a warning will be  reported.
> > > >
> > > > Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
> > >
> > > Generally speaking as a default locking setup for sensor
> > > drivers we are moving in the direction of removing driver
> > > specific locks and instead using the control-handler
> > > lock everywhere, including using it as the active state
> > > lock, see e.g. :
> > >
> > > https://lore.kernel.org/linux-media/20250313184314.91410-14-hdegoede@redhat.com/
> > >
> > > which sets ov02c10->sd.state_lock = ov02c10->ctrl_handler.lock
> > > and then removes a bunch of manual mutex_lock / unlock calls
> > > since all ops which get called with a sd_state will already
> > > have the lock called when operating on the active_state
> > > (and when called in try mode they should not touch anything
> > > needing locking).
> > >
> > > Note if you also want to make the ctrl_handler lock
> > > the active state lock then you need to add calls to
> > > v4l2_subdev_init_finalize() / v4l2_subdev_cleanup()
> > > to allocate the active-state to probe().
> >
> > I agree with the above, but the driver is old and it uses its own lock to
> > serialise access to its data structures while it uses the control lock
> > separately. So this looks like a bugfix that could be backported.
> >
> > I wonder if anyone still has a system with this sensor.
>
> ov5647 is the rpi camera module v1, so I guess it's still around even
> if a bit old. Dave in cc is the expert and maintainer of this driver.

Raspberry Pi stopped selling OV5647 in about 2016 after Omnivision
gave a last-time-buy date in 2014/5, and we brought out the v2 camera
module based on imx219. However there are still modules being sold
even now - stick "OV5647" into eBay or Amazon and you'll get loads of
hits.

We still support the modules, but have little enthusiasm for investing
significant development effort into it whilst it remains functional.

As this is a bug fix for a genuine issue and has minimal impact, I'd
be tempted to accept it. Reworking the driver to use the same mutex
and all the subdev state can be done at a separate time (unless
Xiaolei is really keen to do it now).

  Dave

> Jai has a series in review to upstream all the remaining BSP patches
> for this driver.
> https://lore.kernel.org/all/20251118-b4-rpi-ov5647-v2-0-5e78e7cb7f9b@ideasonboard.com/
>
> I'll cc him as well
>
> >
> > --
> > Regards,
> >
> > Sakari Ailus
> >

Re: [PATCH] media: i2c: ov5647: use our own mutex for the ctrl lock

[xiaolei wang]

Hi

On 12/2/25 00:06, Dave Stevenson wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
> Hi Sakari
>
> On Mon, 1 Dec 2025 at 13:58, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
>> Hello
>>
>> On Mon, Dec 01, 2025 at 03:02:11PM +0200, Sakari Ailus wrote:
>>> Hi Hans, Xiaolei,
>>>
>>> On Mon, Dec 01, 2025 at 10:31:59AM +0100, johannes.goede@oss.qualcomm.com wrote:
>>>> Hi,
>>>>
>>>> On 1-Dec-25 1:00 AM, Xiaolei Wang wrote:
>>>>> __v4l2_ctrl_handler_setup() and __v4l2_ctrl_modify_range()
>>>>> contains an assertion to verify that the v4l2_ctrl_handler::lock
>>>>> is held, as it should only be called when the lock has already
>>>>> been acquired. Therefore use our own mutex for the ctrl lock,
>>>>> otherwise a warning will be  reported.
>>>>>
>>>>> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
>>>> Generally speaking as a default locking setup for sensor
>>>> drivers we are moving in the direction of removing driver
>>>> specific locks and instead using the control-handler
>>>> lock everywhere, including using it as the active state
>>>> lock, see e.g. :
>>>>
>>>> https://lore.kernel.org/linux-media/20250313184314.91410-14-hdegoede@redhat.com/
>>>>
>>>> which sets ov02c10->sd.state_lock = ov02c10->ctrl_handler.lock
>>>> and then removes a bunch of manual mutex_lock / unlock calls
>>>> since all ops which get called with a sd_state will already
>>>> have the lock called when operating on the active_state
>>>> (and when called in try mode they should not touch anything
>>>> needing locking).
>>>>
>>>> Note if you also want to make the ctrl_handler lock
>>>> the active state lock then you need to add calls to
>>>> v4l2_subdev_init_finalize() / v4l2_subdev_cleanup()
>>>> to allocate the active-state to probe().
>>> I agree with the above, but the driver is old and it uses its own lock to
>>> serialise access to its data structures while it uses the control lock
>>> separately. So this looks like a bugfix that could be backported.
>>>
>>> I wonder if anyone still has a system with this sensor.
>> ov5647 is the rpi camera module v1, so I guess it's still around even
>> if a bit old. Dave in cc is the expert and maintainer of this driver.
> Raspberry Pi stopped selling OV5647 in about 2016 after Omnivision
> gave a last-time-buy date in 2014/5, and we brought out the v2 camera
> module based on imx219. However there are still modules being sold
> even now - stick "OV5647" into eBay or Amazon and you'll get loads of
> hits.
>
> We still support the modules, but have little enthusiasm for investing
> significant development effort into it whilst it remains functional.
>
> As this is a bug fix for a genuine issue and has minimal impact, I'd
> be tempted to accept it. Reworking the driver to use the same mutex
> and all the subdev state can be done at a separate time (unless
> Xiaolei is really keen to do it now).
Thanks for the feedback and support. I appreciate the context about
the OV5647 module.

Currently, I think maintaining the current minimal fix is ​​better, as 
it also

facilitates stable backporting. The driver rework can be done separately

in the future

thanks

xiaolei

>
>    Dave
>
>> Jai has a series in review to upstream all the remaining BSP patches
>> for this driver.
>> https://lore.kernel.org/all/20251118-b4-rpi-ov5647-v2-0-5e78e7cb7f9b@ideasonboard.com/
>>
>> I'll cc him as well
>>
>>> --
>>> Regards,
>>>
>>> Sakari Ailus
>>>

Re: [PATCH] media: i2c: ov5647: use our own mutex for the ctrl lock

[Sakari Ailus]

Hi Xiaolei,

On Mon, Dec 01, 2025 at 08:00:26AM +0800, Xiaolei Wang wrote:
> __v4l2_ctrl_handler_setup() and __v4l2_ctrl_modify_range()
> contains an assertion to verify that the v4l2_ctrl_handler::lock
> is held, as it should only be called when the lock has already
> been acquired. Therefore use our own mutex for the ctrl lock,
> otherwise a warning will be  reported.
> 
> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
> ---
>  drivers/media/i2c/ov5647.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index e193fef4fced..4e14eefba577 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -1288,9 +1288,12 @@ static int ov5647_init_controls(struct ov5647 *sensor)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
>  	int hblank, exposure_max, exposure_def;
> +	struct v4l2_ctrl_handler *hdl = &sensor->ctrls;
>  
>  	v4l2_ctrl_handler_init(&sensor->ctrls, 9);
>  
> +	hdl->lock = &sensor->lock;

You can use sensor->ctrls here; otherwise change the existing users first
(but that should go to a separate patch anyway).

> +
>  	v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
>  			  V4L2_CID_AUTOGAIN, 0, 1, 1, 0);
>  

-- 
Kind regards,

Sakari Ailus

Re: [PATCH] media: i2c: ov5647: use our own mutex for the ctrl lock

[xiaolei wang]

On 12/4/25 18:11, Sakari Ailus wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
> Hi Xiaolei,
>
> On Mon, Dec 01, 2025 at 08:00:26AM +0800, Xiaolei Wang wrote:
>> __v4l2_ctrl_handler_setup() and __v4l2_ctrl_modify_range()
>> contains an assertion to verify that the v4l2_ctrl_handler::lock
>> is held, as it should only be called when the lock has already
>> been acquired. Therefore use our own mutex for the ctrl lock,
>> otherwise a warning will be  reported.
>>
>> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
>> ---
>>   drivers/media/i2c/ov5647.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
>> index e193fef4fced..4e14eefba577 100644
>> --- a/drivers/media/i2c/ov5647.c
>> +++ b/drivers/media/i2c/ov5647.c
>> @@ -1288,9 +1288,12 @@ static int ov5647_init_controls(struct ov5647 *sensor)
>>   {
>>        struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
>>        int hblank, exposure_max, exposure_def;
>> +     struct v4l2_ctrl_handler *hdl = &sensor->ctrls;
>>
>>        v4l2_ctrl_handler_init(&sensor->ctrls, 9);
>>
>> +     hdl->lock = &sensor->lock;
> You can use sensor->ctrls here; otherwise change the existing users first
> (but that should go to a separate patch anyway).

OK，I will send version 2.

thanks

xiaolei

>
>> +
>>        v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
>>                          V4L2_CID_AUTOGAIN, 0, 1, 1, 0);
>>
> --
> Kind regards,
>
> Sakari Ailus