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

[Jacopo Mondi]

Hi Kieran

On Mon, Nov 20, 2023 at 10:27:32PM +0000, Kieran Bingham via libcamera-devel wrote:
> Hi Alain,
>
> 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 ?

>
>
> > 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)) {
>
> 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.
>
> --
> Kieran
>
>
> > +                       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);
> > --
> > 2.25.1
> >