[libcamera-devel] [PATCH 2/2] libcamera: ipu3: Pass the BDS rectangle at IPA configure call

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Fri, Mar 12, 2021 at 02:03:04PM +0100, Jean-Michel Hautbois wrote:
> The IPU3 IPA will need the BDS configuration when the AWB/AGC algorithm
> will be integrated.
> In order to do that, the configure() interface needs to be modified.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  include/libcamera/ipa/ipu3.mojom     | 2 +-
>  src/ipa/ipu3/ipu3.cpp                | 7 +++++--
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +++-
>  3 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> index 6ee11333..71b61c60 100644
> --- a/include/libcamera/ipa/ipu3.mojom
> +++ b/include/libcamera/ipa/ipu3.mojom
> @@ -30,7 +30,7 @@ interface IPAIPU3Interface {
>  	start() => (int32 ret);
>  	stop();
>  
> -	configure(map<uint32, ControlInfoMap> entityControls) => ();
> +	configure(map<uint32, ControlInfoMap> entityControls, Rectangle bds) => ();

bds stands for Bayer Down Scaler if I'm not mistaken. What is this
rectangle ? Is it the input size to the BDS, the input crop rectangle,
the output crop rectangle, the output size, or something else ? Naming
the variable accordingly would be more readable.

>  
>  	mapBuffers(array<IPABuffer> buffers);
>  	unmapBuffers(array<uint32> ids);
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index b63e58be..4bce5c27 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -35,7 +35,9 @@ public:
>  	int start() override { return 0; }
>  	void stop() override {}
>  
> -	void configure(const std::map<uint32_t, ControlInfoMap> &entityControls) override;
> +	void configure(
> +		const std::map<uint32_t, ControlInfoMap> &entityControls,
> +		const Rectangle &bds) override;
>  
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> @@ -62,7 +64,8 @@ private:
>  	uint32_t maxGain_;
>  };
>  
> -void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls)
> +void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls,
> +			[[maybe_unused]] const Rectangle &bds)
>  {
>  	if (entityControls.empty())
>  		return;
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 0d133031..4958d2fe 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -722,6 +722,7 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis
>  	IPU3CameraData *data = cameraData(camera);
>  	CIO2Device *cio2 = &data->cio2_;
>  	ImgUDevice *imgu = data->imgu_;
> +	Rectangle bds{ 0, 0, data->pipeConfig_.bds };
>  	int ret;
>  
>  	/* Allocate buffers for internal pipeline usage. */
> @@ -746,7 +747,8 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis
>  		goto error;
>  
>  	entityControls.emplace(0, data->cio2_.sensor()->controls());
> -	data->ipa_->configure(entityControls);
> +

You can move the bds variable declaration right here.

> +	data->ipa_->configure(entityControls, bds);

With these two comments addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>  
>  	return 0;
>  

-- 
Regards,

Laurent Pinchart