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

19207 diff mbox series

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@foss.st.com>
---
 src/libcamera/camera_sensor.cpp | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

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

Hi Kieran,

On Mon, Nov 13, 2023 at 01:27:54PM +0000, Kieran Bingham wrote:
> 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@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.

My understanding is that HBLANK is mandatory in case of RAW sensors but
optional for yuv/rgb sensors.  However even if optional in case of
yuv/rgb sensor, it might be the only way to control the framerate
sometimes (as with the gc2145 for which we've decided to only support
hblank/vblank since it will support both yuv/rgb and raw).

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

Yes, on GC2145 that's usable.

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

At least this function only make sense in case of raw format and will
return rapidly EINVAL when called on a rgb/yuv format sensor, so this
point won't get reached for a yuv/rgb sensor.

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

Hi Alain

On Mon, Nov 13, 2023 at 11:08:51AM +0100, Alain Volmat via libcamera-devel wrote:
> 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@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.
>  	 */

Actually the above comment suggest that there's no way to know if a
control is RO, while I see

	if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) &&
	    vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY))

in other parts of the class.

This is unrelated to this change though

> -	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()) {

Most YUV/RGB sensor drivers I know of do not support HBLANK, so I
would have expected this issue to be hit earlier than this. Possibly
libcamera is not very populare with YUV/RGB sensor :)

> +		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;
> +		}

The patch looks good to me
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

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

On Wed, Nov 15, 2023 at 03:57:23PM +0100, Jacopo Mondi via libcamera-devel wrote:
> Hi Alain
> 
> On Mon, Nov 13, 2023 at 11:08:51AM +0100, Alain Volmat via libcamera-devel wrote:
> > 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@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.
> >  	 */
> 
> Actually the above comment suggest that there's no way to know if a
> control is RO, while I see
> 
> 	if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) &&
> 	    vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY))
> 
> in other parts of the class.
> 
> This is unrelated to this change though
> 
> > -	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()) {
> 
> Most YUV/RGB sensor drivers I know of do not support HBLANK, so I
> would have expected this issue to be hit earlier than this. Possibly
> libcamera is not very populare with YUV/RGB sensor :)
> 
> > +		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;
> > +		}
> 
> The patch looks good to me
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Likewise;

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

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