[libcamera-devel] [PATCH 1/3] libcamera: camera_sensor: Add a function to reset the HBLANK interval

[Jacopo Mondi]

Hi David,

On Tue, Nov 22, 2022 at 12:03:41PM +0000, David Plowman via libcamera-devel wrote:
> The existing code to reset the HBLANK interval to the minimum value
> (in the init method) is moved into a separate function that pipeline
> handlers will be able to call.
>
> We no longer reset the HBLANK in init because we may not own the
> camera and the operation may fail, so pipeline handlers will have to
> call the new method for themselves. These calls will be added in
> subsequent commits.
>

This was again part of a discussion with Dave on a recent series for a
camera sensor
https://patchwork.linuxtv.org/project/linux-media/patch/20221005190613.394277-8-jacopo@jmondi.org/#140629

To sum it up: I would have liked to standardize on the idea that when
a new mode is applied on a sensor, the blankings are reset to their
minimum by the driver.

As nowadays every driver behaves as it likes the most, and changing
mode might eventually change the blankings value anyway, I considered
a known behaviour to be more predictable for applications.

To be honest, I overlooked the fact we reset hblank unconditionally
(but only at init() time).

Curious to know what you think on this and what would your PH expect
(I presume it doesn't make much difference, as the IPA wants to
control HBLANK anyway).

> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  include/libcamera/internal/camera_sensor.h |  2 ++
>  src/libcamera/camera_sensor.cpp            | 10 +++++++---
>  2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index b9f4d786..c97b5698 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -68,6 +68,8 @@ public:
>
>  	CameraLens *focusLens() { return focusLens_.get(); }
>
> +	int resetHblank();
> +
>  protected:
>  	std::string logPrefix() const override;
>
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 572a313a..589e9736 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -176,6 +176,11 @@ int CameraSensor::init()
>  	if (ret)
>  		return ret;
>
> +	return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);
> +}
> +
> +int CameraSensor::resetHblank()

I'm not super excited by this, it's a very ad-hoc call for PH, if they
don't know they have to call it explicitly it will be easily missed.

If my idea of "starting in a known state in drivers" doesn't fly, this
could be moved to CameraSensor::setFormat() maybe ? IPAs that control
HBLANK will override it anyway.

> +{
>  	/*
>  	 * 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
> @@ -192,17 +197,16 @@ int CameraSensor::init()
>  	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>();
> +	int ret = 0;
>
>  	if (hblankMin != hblankMax) {
>  		ControlList ctrl(subdev_->controls());
>
>  		ctrl.set(V4L2_CID_HBLANK, hblankMin);
>  		ret = subdev_->setControls(&ctrl);
> -		if (ret)
> -			return ret;
>  	}
>
> -	return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);
> +	return ret;
>  }
>
>  int CameraSensor::validateSensorDriver()
> --
> 2.30.2
>