[libcamera-devel] [PATCH] libcamera: camera_sensor: only access V4L_CID_HBLANK if existing

[Kieran Bingham]

Hi Alain,

Quoting Alain Volmat via libcamera-devel (2023-11-13 10:08:51)
> Correct a crash in CameraSensor::init when trying to set the
> V4L2_CID_HBLANK control on sensor not implementing this control.
> The HBLANK sensor not being mandatory for non-RAW sensors, it can
> happen that the sensor does not expose this control.
> Perform check against availability of the control prior to usage in
> order to avoid the crash.
> 
> Signed-off-by: Alain Volmat <alain.volmat at foss.st.com>
> ---
>  src/libcamera/camera_sensor.cpp | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index d9261672..0ef78d9c 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -197,17 +197,19 @@ int CameraSensor::init()
>          * \todo The control API ought to have a flag to specify if a control
>          * is read-only which could be used below.
>          */
> -       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) {
> -               ControlList ctrl(subdev_->controls());
> -
> -               ctrl.set(V4L2_CID_HBLANK, hblankMin);
> -               ret = subdev_->setControls(&ctrl);
> -               if (ret)
> -                       return ret;
> +       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) {
> +                       ControlList ctrl(subdev_->controls());
> +
> +                       ctrl.set(V4L2_CID_HBLANK, hblankMin);
> +                       ret = subdev_->setControls(&ctrl);
> +                       if (ret)
> +                               return ret;
> +               }

Do we have a way to know if we're dealing with a raw or yuv/rgb sensor
in the CameraSensor class presently?

It might be beneficial to make sure we are explicit that this is not
required when !rawSensor here.

I see in validateSensorDriver() we already check V4L2_CID_HBLANK is a
mandatoryControl after a check on 

 if (!bayerFormat_)

so I guess that's our current switch point.

Is the HBLANK usable at all if there is no bayerFormat?

I'm also weary about the usage at CameraSensor::sensorInfo() where there
is a call to:


        /*
         * Retrieve the pixel rate, line length and minimum/maximum frame
         * duration through V4L2 controls. Support for the V4L2_CID_PIXEL_RATE,
         * V4L2_CID_HBLANK and V4L2_CID_VBLANK controls is mandatory.
         */
        ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,
                                                   V4L2_CID_HBLANK,
                                                   V4L2_CID_VBLANK });
        if (ctrls.empty()) {
                LOG(CameraSensor, Error)
                        << "Failed to retrieve camera info controls";
                return -EINVAL;
        }

Does supporting !HBLANK require a larger rework ? or is this the only
place that you have hit which requires a check?

--
Kieran



>         }
>  
>         return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);
> -- 
> 2.25.1
>