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

[Kieran Bingham]

Quoting Jacopo Mondi (2023-11-21 09:47:28)
> 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 ?

I did. It sounded rejected to me.

https://patchwork.libcamera.org/cover/19187/

"""
> Only modified one ControlInfo constructor is modified which is used by the
> V4L2Device class to allow this flag to be set, as setting it for a non-v4l2
> control probably does not make sense at this point.

This is the part that bothers me a bit. If the feature is only used
internally, it shouldn't be exposed in the public API.

One possible workaround would be to add flag controls as being settable
in a request and as being reported in metadata. This is a feature that
is useful for applications, and it could then be used internally do
indicate read-only internal controls.
"""

--
Kieran


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