Re: [PATCH v5 04/10] media: imx219: Make control handler ops for PIXEL_RATE NULL

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Tue, Jun 09, 2026 at 04:56:48PM +0100, Dave Stevenson wrote:
> On Tue, 9 Jun 2026 at 16:15, Laurent Pinchart wrote:
> > On Tue, Jun 09, 2026 at 04:55:09PM +0200, Hans Verkuil wrote:
> > > On 09/06/2026 08:29, Jacopo Mondi wrote:
> > > > On Mon, Jun 08, 2026 at 05:42:00PM +0300, Laurent Pinchart wrote:
> > > >> On Mon, Jun 08, 2026 at 04:47:03PM +0300, Sakari Ailus wrote:
> > > >>> On Mon, Jun 08, 2026 at 01:27:55PM +0300, Laurent Pinchart wrote:
> > > >>>> On Mon, Jun 08, 2026 at 01:21:22PM +0300, Sakari Ailus wrote:
> > > >>>>> On Mon, Jun 08, 2026 at 11:24:26AM +0300, Laurent Pinchart wrote:
> > > >>>>>> On Mon, Jun 08, 2026 at 11:14:32AM +0300, Sakari Ailus wrote:
> > > >>>>>>> On Mon, Jun 08, 2026 at 11:03:38AM +0300, Laurent Pinchart wrote:
> > > >>>>>>>> On Mon, Jun 08, 2026 at 09:53:17AM +0200, Jacopo Mondi wrote:
> > > >>>>>>>>> Hi Laurent
> > > >>>>>>>>>   sorry if I reply in place of Sakari but I got this fresh
> > > >>>>>>>>
> > > >>>>>>>> Thanks :-)
> > > >>>>>>>>
> > > >>>>>>>>> On Mon, Jun 08, 2026 at 10:36:53AM +0300, Laurent Pinchart wrote:
> > > >>>>>>>>>> On Mon, Jun 08, 2026 at 12:53:50AM +0300, Sakari Ailus wrote:
> > > >>>>>>>>>>> The PIXEL_RATE control exists to convey the value to the userspace and has
> > > >>>>>>>>>>> no configuration that would need to be programmed to the sensor. Make the
> > > >>>>>>>>>>> control handler ops for the PIXEL_RATE control NULL and avoid a warning
> > > >>>>>>>>>>> (as well as returning an error) from the driver.
> > > >>>>>>>>>>
> > > >>>>>>>>>> I thought the standard way to handle pixel rate being read only was to
> > > >>>>>>>>>> set the V4L2_CTRL_FLAG_READ_ONLY flag, like we do for e.g.
> > > >>>>>>>>>> V4L2_CID_LINK_FREQ. Is that not correct ?
> > > >>>>>>>>>
> > > >>>>>>>>> PIXEL_RATE is RO by default
> > > >>>>>>>>>
> > > >>>>>>>>> drivers/media/v4l2-core/v4l2-ctrls-defs.c:      case V4L2_CID_PIXEL_RATE:
> > > >>>>>>>>> drivers/media/v4l2-core/v4l2-ctrls-defs.c-              *type = V4L2_CTRL_TYPE_INTEGER64;
> > > >>>>>>>>> drivers/media/v4l2-core/v4l2-ctrls-defs.c-              *flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > >>>>>>>>> drivers/media/v4l2-core/v4l2-ctrls-defs.c-              break;
> > > >>>>>>>>>
> > > >>>>>>>>> The purpose of setting the ctrl_ops member to NULL is to avoid having
> > > >>>>>>>>> to handle RO controls in the driver implementation of .s_ctrl().
> > > >>>>>>>>
> > > >>>>>>>> Shouldn't the V4L2 control framework avoid .s_ctrl() calls for read-only
> > > >>>>>>>> controls ? I thought it did already.
> > > >>>>>>>
> > > >>>>>>> The control may be read-only on the UAPI but the driver could still do
> > > >>>>>>> something about it in its s_ctrl() callback. I don't know if any driver
> > > >>>>>>> depends on this though.
> > > >>>>>>
> > > >>>>>> It seems to be one of the many areas where control handling should be
> > > >>>>>> simplified for drivers.
> > > >>>>>>
> > > >>>>>> In any case, the imx219 driver creates the V4L2_CID_LINK_FREQ control
> > > >>>>>> with a non-NULL ops pointer, sets the V4L2_CTRL_FLAG_READ_ONLY flag, and
> > > >>>>>> does not handle V4L2_CID_LINK_FREQ in imx219_set_ctrl(). If there's an
> > > >>>>>> issue for V4L2_CID_PIXEL_RATE there is also an issue for
> > > >>>>>> V4L2_CID_LINK_FREQ.
> > > >>>>>
> > > >>>>> The ops should be set to NULL for link_freq as well.
> > > >>>>>
> > > >>>>>> Maybe the best short term fix would be to drop the dev_info() in the
> > > >>>>>> default case of the ctrl->id switch in imx219_set_ctrl() ?
> > > >>>>>
> > > >>>>> Any reason why not to set ops NULL instead?
> > > >>>>
> > > >>>> Because that seems to be a hack. Drivers shouldn't have to set a NULL
> > > >>>> ops pointer for read-only controls, when there's already a read-only
> > > >>>> flag. I'd like to simplify the code on the driver side and handle this
> > > >>>> in the control framework, not adding yet another arcane rule that most
> > > >>>> driver authors will not be aware of.
> > > >>>
> > > >>> I don't think I'd necessarily call it a hack.
> > > >>
> > > >> It's still yet another undocumented behaviour to will be copied through
> > > >> cargo-cult in a subset of drivers, making the code base more difficult
> > > >> to understand and maintain. The fact that this patch addressed the
> > > >> PIXEL_RATE control but not the LINK_FREQUENCY control proves my concerns
> > > >> are valid :-)
> > > >>
> > > >> I'd like to see one scheme clearly documented, and used by all drivers.
> > > >> Let's first focus on selecting one scheme and documenting it. Hans'
> > > >> opinion would be useful.
> > > >
> > > > We discussed this very same matter a few months ago.
> > > >
> > > > Before having the framework handling this, by not calling into the
> > > > driver's s_ctrl for RO controls, all users in-tree shall be checked to
> > > > make sure they're actually not doing something with those RO controls.
> > > >
> > > > I even started a branch to check all drivers one-by-one and first set
> > > > they're ops to NULL. I quickly got discouraged by the amount of work
> > > > required and gave up.
> > >
> > > Userspace cannot set RO controls, EACCES is returned.
> > >
> > > Drivers can set RO controls (after all, RO just means that userspace can't
> > > change it, but drivers can). And yes, s_ctrl (if present) will be called
> > > in that case. Generally RO controls will have a NULL ops pointer, but there
> > > may be cases where something needs to be done in s_ctrl.
> > >
> > > I can't remember that ever being needed, but I may be wrong, and in any case
> > > the same control framework is used by out-of-tree drivers, so it is not
> > > something I would want to change.
> >
> > Not that we should break out-of-tree drivers just for the fun of it, I
> > don't see that as being by itself a good enough reason to avoid a change
> > to an in-kernel API.
> >
> > > Perhaps include/media/v4l2-ctrls.h should be improved to mention that for
> > > RO controls ops should probably be set to NULL unless you have a really
> > > good reason not to.
> >
> > I'm fine with this patch if we clearly document this is the way to go
> > (and with the LINK_FREQ control being addressed as well).
> 
> We do have a slight inconsistency in that v4l2_ctrl_modify_range() and
> v4l2_ctrl_s_ctrl call the handler for READ_ONLY controls, but
> __v4l2_ctrl_handler_setup() does not.
> 
> I hit that on my recent IMX355 patchset where the V4L2_CID_HBLANK
> control had been set appropriately and I needed to set the LLP
> register. I initially added the handler, only to find it wasn't
> called.

Hans, any objection against changing that behaviour and call .s_ctrl()
from __v4l2_ctrl_handler_setup() too ?

> I have no issues with the patch, so it gets an
> Acked-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> > > >>> The control may be changeable, but not by the user. If the driver is just
> > > >>> setting the value without going through the control framework, control
> > > >>> events will be omitted.

-- 
Regards,

Laurent Pinchart