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

[Naushir Patuck]

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.



> > > -   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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240305/86e929cf/attachment.htm>