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

[Kieran Bingham]

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

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?



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