[libcamera-devel] [PATCH 03/11] libcamera: ipu3: imgu: Configure the stat video device as part of configure()

[Kieran Bingham]

Hi Niklas,

On 05/11/2020 00:15, Niklas Söderlund wrote:
> There is no reason to expose and call a separate configureStat() when
> the statistics video device can be configured with the exact same
> parameters as part of configure(). Move the configuration internally to
> the ImgUDevice simplifying the interface, there is no functional change.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/pipeline/ipu3/imgu.cpp |  7 +++++++
>  src/libcamera/pipeline/ipu3/imgu.h   |  7 -------
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 11 -----------
>  3 files changed, 7 insertions(+), 18 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> index a4d74a62f69a0c97..0a3bf62020fd23fb 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> @@ -477,6 +477,13 @@ int ImgUDevice::configure(const PipeConfig &pipeConfig, V4L2DeviceFormat *inputF
>  
>  	LOG(IPU3, Debug) << "ImgU GDC format = " << gdcFormat.toString();
>  
> +	StreamConfiguration statCfg = {};
> +	statCfg.size = inputFormat->size;
> +	V4L2DeviceFormat statFormat;
> +	ret = configureVideoDevice(stat_.get(), PAD_STAT, statCfg, &statFormat);
> +	if (ret)
> +		return ret;
> +

Shouldn't you do something with statCfg or statFormat?
Hrm ... maybe this happens later ...


>  	return 0;
>  }
>  
> diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h
> index c73ac5a5a37cfe0e..37f5ae77c99ff8fe 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.h
> +++ b/src/libcamera/pipeline/ipu3/imgu.h
> @@ -61,13 +61,6 @@ public:
>  					    outputFormat);
>  	}
>  
> -	int configureStat(const StreamConfiguration &cfg,
> -			  V4L2DeviceFormat *outputFormat)
> -	{
> -		return configureVideoDevice(stat_.get(), PAD_STAT, cfg,
> -					    outputFormat);> -	}
> -
>  	int allocateBuffers(unsigned int bufferCount);
>  	void freeBuffers();
>  
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 3f0232bc1eaad048..c559d160084f87e7 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -518,17 +518,6 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  			return ret;
>  	}
>  
> -	/*
> -	 * Apply the largest available format to the stat node.
> -	 * \todo Revise this when we'll actually use the stat node.
> -	 */
> -	StreamConfiguration statCfg = {};
> -	statCfg.size = cio2Format.size;
> -
> -	ret = imgu->configureStat(statCfg, &outputFormat);

was outputFormat ignored/unused before ? perhaps it was...

Ok - Given that this is just about the code move, and I think this is
unused here, that explains why this commit doesn't 'use' the statCfg or
statFormat above - so:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


> -	if (ret)
> -		return ret;
> -
>  	/* Apply the "pipe_mode" control to the ImgU subdevice. */
>  	ControlList ctrls(imgu->imgu_->controls());
>  	ctrls.set(V4L2_CID_IPU3_PIPE_MODE,
> 

-- 
Regards
--
Kieran