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

[Kieran Bingham]

Quoting Naushir Patuck (2024-03-05 07:09:58)
> On Mon, 4 Mar 2024, 10:49 pm Laurent Pinchart, <
> laurent.pinchart at ideasonboard.com> wrote:
> 
> > On Mon, Mar 04, 2024 at 06:41:03PM +0100, Jacopo Mondi wrote:
> > > Hi Laurent
> > >
> > > 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?

--
Kieran




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