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: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).

> >>> 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