<div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 4 Mar 2024, 10:49 pm Laurent Pinchart, <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Mon, Mar 04, 2024 at 06:41:03PM +0100, Jacopo Mondi wrote:<br>
> Hi Laurent<br>
> <br>
> On Fri, Mar 01, 2024 at 11:21:06PM +0200, Laurent Pinchart wrote:<br>
> > The CameraSensor class tests if the sensor HBLANK control is read-only<br>
> > by comparing the minimum and maximum values, and documents this as being<br>
> > a workaround for the lack of a read-only control flag in V4L2. This is<br>
> > incorrect, as the V4L2 API provides such a flag. Use it to replace the<br>
> > workaround.<br>
> ><br>
> > Signed-off-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank" rel="noreferrer">laurent.pinchart@ideasonboard.com</a>><br>
> > ---<br>
> >  src/libcamera/sensor/camera_sensor.cpp | 27 +++++++-------------------<br>
> >  1 file changed, 7 insertions(+), 20 deletions(-)<br>
> ><br>
> > diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp<br>
> > index 86ad9a85371c..402025566544 100644<br>
> > --- a/src/libcamera/sensor/camera_sensor.cpp<br>
> > +++ b/src/libcamera/sensor/camera_sensor.cpp<br>
> > @@ -188,28 +188,15 @@ int CameraSensor::init()<br>
> >      * Set HBLANK to the minimum to start with a well-defined line length,<br>
> >      * allowing IPA modules that do not modify HBLANK to use the sensor<br>
> >      * minimum line length in their calculations.<br>
> > -    *<br>
> > -    * At present, there is no way of knowing if a control is read-only.<br>
> > -    * As a workaround, assume that if the minimum and maximum values of<br>
> > -    * the V4L2_CID_HBLANK control are the same, it implies the control<br>
> > -    * is read-only.<br>
> > -    *<br>
> > -    * \todo The control API ought to have a flag to specify if a control<br>
> > -    * is read-only which could be used below.<br>
> >      */<br>
> <br>
> Weird, I'm sure V4L2_CTRL_FLAG_READ_ONLY was there in 2022 when this<br>
> has been introduced. Maybe the API we had to check flags was (or is)<br>
> not the best one and we decided to compare values ?<br>
<br>
It puzzled me too, I really don't recall why we didn't use it. Naush,<br>
does it ring a bell ?<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">My patch at <a href="https://patchwork.libcamera.org/patch/17936/">https://patchwork.libcamera.org/patch/17936/</a> did make use of this flag. But it never got merged. </div><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> > -   if (ctrls.infoMap()->find(V4L2_CID_HBLANK) != ctrls.infoMap()->end()) {<br>
> > -           const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);<br>
> > -           const int32_t hblankMin = hblank.min().get<int32_t>();<br>
> > -           const int32_t hblankMax = hblank.max().get<int32_t>();<br>
> > +   const struct v4l2_query_ext_ctrl *hblankInfo = subdev_->controlInfo(V4L2_CID_HBLANK);<br>
> > +   if (hblankInfo && !(hblankInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) {<br>
> > +           ControlList ctrl(subdev_->controls());<br>
> <br>
> This could be fast-tracked too!<br>
> <br>
> Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank" rel="noreferrer">jacopo.mondi@ideasonboard.com</a>><br>
> <br>
> > -           if (hblankMin != hblankMax) {<br>
> > -                   ControlList ctrl(subdev_->controls());<br>
> > -<br>
> > -                   ctrl.set(V4L2_CID_HBLANK, hblankMin);<br>
> > -                   ret = subdev_->setControls(&ctrl);<br>
> > -                   if (ret)<br>
> > -                           return ret;<br>
> > -           }<br>
> > +           ctrl.set(V4L2_CID_HBLANK, static_cast<int32_t>(hblankInfo->minimum));<br>
> > +           ret = subdev_->setControls(&ctrl);<br>
> > +           if (ret)<br>
> > +                   return ret;<br>
> >     }<br>
> ><br>
> >     return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div></div>