[libcamera-devel] [PATCH 01/11] pipeline: ipu3: Check if sensor supports test pattern control

[Laurent Pinchart]

Hi Dan,

Thank you for the patch.

On Sat, Mar 18, 2023 at 11:40:04PM +0000, Daniel Scally via libcamera-devel wrote:
> The IPU3 pipeline calls CameraSensor::setTestPatternMode() in ::start().
> That control is not a libcamera mandatory control and so might not be
> present for a sensor. Check for its presence before trying to set the
> control to avoid uneccessary failures.
> 
> Fixes: acf8d028e ("libcamera: pipeline: ipu3: Apply a requested test pattern mode")
> Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
> ---
> I'll patch this control into the ov7251 driver upstream as the sensor does have
> a test pattern mode, but still - it's not mandatory!
> 
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 355cb0cb..d0d55651 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -722,10 +722,14 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis
>  	int ret;
>  
>  	/* Disable test pattern mode on the sensor, if any. */
> -	ret = cio2->sensor()->setTestPatternMode(
> -		controls::draft::TestPatternModeEnum::TestPatternModeOff);
> -	if (ret)
> -		return ret;
> +	const ControlInfoMap &sensorControls = cio2->sensor()->controls();
> +
> +	if (sensorControls.find(&controls::draft::TestPatternMode) != sensorControls.end()) {
> +		ret = cio2->sensor()->setTestPatternMode(
> +			controls::draft::TestPatternModeEnum::TestPatternModeOff);

CameraSensor::setTestPatternMode() starts with

	if (testPatternMode_ == mode)
		return 0;

which was meant to make the function a no-op if the sensor doesn't
support test patterns. It seems the CameraSensor class may be missing
initialization of the testPatternMode_ member, which may explain why you
get an error. Initializing it in the constructor would be a better fix.

> +		if (ret)
> +			return ret;
> +	}
>  
>  	/* Allocate buffers for internal pipeline usage. */
>  	ret = allocateBuffers(camera);

-- 
Regards,

Laurent Pinchart