[libcamera-devel] [PATCH] libcamera: camera_sensor: fix HBLANK RO check

[Laurent Pinchart]

Hello,

Resurecting this thread, as I've recently sent a similar patch
(https://patchwork.libcamera.org/patch/19608/).

On Tue, Nov 21, 2023 at 01:12:05PM +0000, 📷-dev wrote:
> Quoting Jacopo Mondi (2023-11-21 09:47:28)
> > On Mon, Nov 20, 2023 at 10:27:32PM +0000, Kieran Bingham via libcamera-devel wrote:
> > > Quoting Alain Volmat via libcamera-devel (2023-11-20 18:45:29)
> > > > Perform the HBLANK readonly check by looking at the v4l2_query_ext_ctrl
> > > > struct for the V4L2_CID_HBLANK instead of checking for min/max values.
> > >
> > > Aha, you beat me to it. (ref:
> > > https://patchwork.libcamera.org/patch/19189/)
> > 
> > Thanks for digging it out
> > 
> > > I would say we need to fix both here and src/ipa/rpi/common/ipa_base.cpp
> > > with the same change though. Could you also update that and compile test
> > > please?
> > 
> > Why not resume the work done by Naush then and re-propose his series ?
> > Alain could you maybe consider that ?
> 
> I did. It sounded rejected to me.
> 
> https://patchwork.libcamera.org/cover/19187/
> 
> """
> > Only modified one ControlInfo constructor is modified which is used by the
> > V4L2Device class to allow this flag to be set, as setting it for a non-v4l2
> > control probably does not make sense at this point.
> 
> This is the part that bothers me a bit. If the feature is only used
> internally, it shouldn't be exposed in the public API.
> 
> One possible workaround would be to add flag controls as being settable
> in a request and as being reported in metadata. This is a feature that
> is useful for applications, and it could then be used internally do
> indicate read-only internal controls.
> """

I think it makes sense to decouple the two issues, fixing the hack as
done in this patch, and considering extensions to ControlInfo
separately.

> > > > Signed-off-by: Alain Volmat <alain.volmat at foss.st.com>
> > > > ---
> > > >  src/libcamera/camera_sensor.cpp | 17 ++++-------------
> > > >  1 file changed, 4 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > > index 0ef78d9c..3281c1f9 100644
> > > > --- a/src/libcamera/camera_sensor.cpp
> > > > +++ b/src/libcamera/camera_sensor.cpp
> > > > @@ -188,21 +188,12 @@ int CameraSensor::init()
> > > >          * Set HBLANK to the minimum to start with a well-defined line length,
> > > >          * allowing IPA modules that do not modify HBLANK to use the sensor
> > > >          * minimum line length in their calculations.
> > > > -        *
> > > > -        * At present, there is no way of knowing if a control is read-only.
> > > > -        * As a workaround, assume that if the minimum and maximum values of
> > > > -        * the V4L2_CID_HBLANK control are the same, it implies the control
> > > > -        * is read-only.
> > > > -        *
> > > > -        * \todo The control API ought to have a flag to specify if a control
> > > > -        * is read-only which could be used below.
> > > >          */
> > > >         if (ctrls.infoMap()->find(V4L2_CID_HBLANK) != ctrls.infoMap()->end()) {
> > > > -               const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);
> > > > -               const int32_t hblankMin = hblank.min().get<int32_t>();
> > > > -               const int32_t hblankMax = hblank.max().get<int32_t>();
> > > > -
> > > > -               if (hblankMin != hblankMax) {
> > > > +               const struct v4l2_query_ext_ctrl *hblankInfo = subdev_->controlInfo(V4L2_CID_HBLANK);
> > > > +               if (!(hblankInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) {

The patch I sent drops the ctrls.infoMap()->find(V4L2_CID_HBLANK) check
above, and instead checks that hblankInfo is not null. The rest of the
series drops the ctrls variable from the context where tha HBLANK
handling is located, so I would prefer keeping that. 

> > >
> > > I don't know if it actually helps or makes sense yet, but if I got here
> > > first I was going to see if it would make sense to put a helper into the
> > > v4l2_device class to make this more succinct (but still V4L2 specific
> > > rather than Control specific).
> > >
> > > Either with or without a helper - both locations covered would earn a
> > > tag from me.
> > >
> > > > +                       const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);
> > > > +                       const int32_t hblankMin = hblank.min().get<int32_t>();
> > > >                         ControlList ctrl(subdev_->controls());
> > > >
> > > >                         ctrl.set(V4L2_CID_HBLANK, hblankMin);

-- 
Regards,

Laurent Pinchart