[libcamera-devel] [PATCH] libcamera: camera_sensor: only access V4L_CID_HBLANK if existing
Alain Volmat
alain.volmat at foss.st.com
Mon Nov 13 15:45:08 CET 2023
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 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.
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
> >
More information about the libcamera-devel
mailing list