[libcamera-devel] [PATCH 1/2] libcamera: ipu3: Move Imgu configuration to IPU3CameraData

[Jacopo Mondi]

Hello Jean-Micheal,

On Fri, Mar 12, 2021 at 02:03:03PM +0100, Jean-Michel Hautbois wrote:
> The IPU3 IPA needs to know the BayerDownScaler (BDS) configuration to
> calculate the statistics grids.
> This patch makes it possible for PipelineHandlerIPU3::start() to access
> the BDS configuration and later pass the rectangle to the IPU3 IPA
> configure method.
>

The alternative would be having the pipe configuration retrieved from
the ImgU device at start() which would require having the ImgUDevice
stateful, which will be very painful to implement due to the awful
pipe configuration procedure.

Removing 'const' from the data_ field doesn't worry me too much, but
smells like we're taking a shortcut. I'm fine with that for the time
being.

> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 00da2bb2..0d133031 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -70,6 +70,8 @@ public:
>  	CIO2Device cio2_;
>  	ImgUDevice *imgu_;
>
> +	ImgUDevice::PipeConfig pipeConfig_;
> +
>  	Stream outStream_;
>  	Stream vfStream_;
>  	Stream rawStream_;
> @@ -97,7 +99,6 @@ public:
>  	Status validate() override;
>
>  	const StreamConfiguration &cio2Format() const { return cio2Configuration_; }
> -	const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; }
>
>  	/* Cache the combinedTransform_ that will be applied to the sensor */
>  	Transform combinedTransform_;
> @@ -108,10 +109,9 @@ private:
>  	 * corresponding Camera instance is valid. In order to borrow a
>  	 * reference to the camera data, store a new reference to the camera.
>  	 */
> -	const IPU3CameraData *data_;
> +	IPU3CameraData *data_;
>
>  	StreamConfiguration cio2Configuration_;
> -	ImgUDevice::PipeConfig pipeConfig_;
>  };
>
>  class PipelineHandlerIPU3 : public PipelineHandler
> @@ -375,12 +375,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>
>  	/* Only compute the ImgU configuration if a YUV stream has been requested. */
>  	if (yuvCount) {
> -		pipeConfig_ = data_->imgu_->calculatePipeConfig(&pipe);
> -		if (pipeConfig_.isNull()) {
> +		ImgUDevice::PipeConfig imguConfig = data_->imgu_->calculatePipeConfig(&pipe);

Why do you need to go through an intermediate variable ?
Can't you assign data_->pipeConfig_ directly here ?

Thanks
  j

> +		if (imguConfig.isNull()) {
>  			LOG(IPU3, Error) << "Failed to calculate pipe configuration: "
>  					 << "unsupported resolutions.";
>  			return Invalid;
>  		}
> +		data_->pipeConfig_ = imguConfig;
>  	}
>
>  	return status;
> @@ -576,7 +577,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  	 * stream has been requested: return here to skip the ImgU configuration
>  	 * part.
>  	 */
> -	ImgUDevice::PipeConfig imguConfig = config->imguConfig();
> +	ImgUDevice::PipeConfig imguConfig = data->pipeConfig_;
>  	if (imguConfig.isNull())
>  		return 0;
>
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel