[PATCH/RFC 17/32] libcamera: camera_sensor: Test for read-only HBLANK with READ_ONLY flag

[Laurent Pinchart]

On Tue, Mar 05, 2024 at 08:58:25AM +0000, Kieran Bingham wrote:
> Quoting Naushir Patuck (2024-03-05 07:09:58)
> > On Mon, 4 Mar 2024, 10:49 pm Laurent Pinchart wrote:
> > > On Mon, Mar 04, 2024 at 06:41:03PM +0100, Jacopo Mondi wrote:
> > > > On Fri, Mar 01, 2024 at 11:21:06PM +0200, Laurent Pinchart wrote:
> > > > > The CameraSensor class tests if the sensor HBLANK control is read-only
> > > > > by comparing the minimum and maximum values, and documents this as being
> > > > > a workaround for the lack of a read-only control flag in V4L2. This is
> > > > > incorrect, as the V4L2 API provides such a flag. Use it to replace the
> > > > > workaround.
> > > > >
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > > ---
> > > > >  src/libcamera/sensor/camera_sensor.cpp | 27 +++++++-------------------
> > > > >  1 file changed, 7 insertions(+), 20 deletions(-)
> > > > >
> > > > > diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp
> > > > > index 86ad9a85371c..402025566544 100644
> > > > > --- a/src/libcamera/sensor/camera_sensor.cpp
> > > > > +++ b/src/libcamera/sensor/camera_sensor.cpp
> > > > > @@ -188,28 +188,15 @@ 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.
> > > > >      */
> > > >
> > > > Weird, I'm sure V4L2_CTRL_FLAG_READ_ONLY was there in 2022 when this
> > > > has been introduced. Maybe the API we had to check flags was (or is)
> > > > not the best one and we decided to compare values ?
> > >
> > > It puzzled me too, I really don't recall why we didn't use it. Naush,
> > > does it ring a bell ?
> > >
> > 
> > My patch at https://patchwork.libcamera.org/patch/17936/ did make use of
> > this flag. But it never got merged.
> 
> Indeed, and I rebased Naush' patches to:
> 
>  - https://patchwork.libcamera.org/cover/19187/
> 
> Which seemed overall to be rejected, and Alain came along with:
> 
>  - https://patchwork.libcamera.org/patch/19214/
> 
> which I thought would get merged, but Alain hasn't been able to submit a
> new version with the comments handled.
> 
> 
> Does something in this series handle the equivalant change at
> src/ipa/rpi/common/ipa_base.cpp as well?

No it doesn't, I've only addressed the CameraSensor side. I've just
replied to Alain's patch, which had fallen through the cracks (sorry
about that).

> > > > > -   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>();
> > > > > +   const struct v4l2_query_ext_ctrl *hblankInfo = subdev_->controlInfo(V4L2_CID_HBLANK);
> > > > > +   if (hblankInfo && !(hblankInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) {
> > > > > +           ControlList ctrl(subdev_->controls());
> > > >
> > > > This could be fast-tracked too!
> > > >
> > > > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > > >
> > > > > -           if (hblankMin != hblankMax) {
> > > > > -                   ControlList ctrl(subdev_->controls());
> > > > > -
> > > > > -                   ctrl.set(V4L2_CID_HBLANK, hblankMin);
> > > > > -                   ret = subdev_->setControls(&ctrl);
> > > > > -                   if (ret)
> > > > > -                           return ret;
> > > > > -           }
> > > > > +           ctrl.set(V4L2_CID_HBLANK, static_cast<int32_t>(hblankInfo->minimum));
> > > > > +           ret = subdev_->setControls(&ctrl);
> > > > > +           if (ret)
> > > > > +                   return ret;
> > > > >     }
> > > > >
> > > > >     return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);

-- 
Regards,

Laurent Pinchart