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

[Jacopo Mondi]

Hi Jean-Micheal,

On Tue, Mar 16, 2021 at 11:18:38AM +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.
>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
> v2:
> - move pipe configuration calculation from validate to configure
> - move ipa configuration from start to configure

This are really 2 (or maybe even 3) separate patches.

Also, when applied on master, this patch does not compile here

../src/libcamera/pipeline/ipu3/ipu3.cpp:633:40: error: too many arguments to function call, expected single argument 'entityControls', have 2 arguments
        data->ipa_->configure(entityControls, config->pipeConfig_.bds);
        ~~~~~~~~~~~~~~~~~~~~~                 ^~~~~~~~~~~~~~~~~~~~~~~
include/libcamera/ipa/ipu3_ipa_proxy.h:44:14: note: 'configure' declared here
        void configure(

> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 45 +++++++++++++---------------
>  1 file changed, 20 insertions(+), 25 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 2ea13ec9..3f49ded3 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -97,21 +97,23 @@ 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_;
>
> +	/* Store the pipe configuration */
> +	ImgUDevice::PipeConfig pipeConfig_;
> +	ImgUDevice::Pipe pipe_{};

Is the initialization needed ?
I would rather zero-initialize pipe_ at every validate() call.

> +
>  private:
>  	/*
>  	 * The IPU3CameraData instance is guaranteed to be valid as long as the
>  	 * 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_;

You don't need this change anymore

>
>  	StreamConfiguration cio2Configuration_;
> -	ImgUDevice::PipeConfig pipeConfig_;
>  };
>
>  class PipelineHandlerIPU3 : public PipelineHandler
> @@ -272,8 +274,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>
>  	LOG(IPU3, Debug) << "CIO2 configuration: " << cio2Configuration_.toString();
>
> -	ImgUDevice::Pipe pipe{};
> -	pipe.input = cio2Configuration_.size;
> +	pipe_.input = cio2Configuration_.size;
>
>  	/*
>  	 * Adjust the configurations if needed and assign streams while
> @@ -349,15 +350,15 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  				cfg->setStream(const_cast<Stream *>(&data_->outStream_));
>  				mainOutputAvailable = false;
>
> -				pipe.main = cfg->size;
> +				pipe_.main = cfg->size;
>  				if (yuvCount == 1)
> -					pipe.viewfinder = pipe.main;
> +					pipe_.viewfinder = pipe_.main;
>
>  				LOG(IPU3, Debug) << "Assigned " << cfg->toString()
>  						 << " to the main output";
>  			} else {
>  				cfg->setStream(const_cast<Stream *>(&data_->vfStream_));
> -				pipe.viewfinder = cfg->size;
> +				pipe_.viewfinder = cfg->size;
>
>  				LOG(IPU3, Debug) << "Assigned " << cfg->toString()
>  						 << " to the viewfinder output";
> @@ -373,16 +374,6 @@ 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()) {
> -			LOG(IPU3, Error) << "Failed to calculate pipe configuration: "
> -					 << "unsupported resolutions.";
> -			return Invalid;
> -		}
> -	}
> -
>  	return status;
>  }
>
> @@ -576,11 +567,15 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  	 * stream has been requested: return here to skip the ImgU configuration
>  	 * part.
>  	 */
> -	ImgUDevice::PipeConfig imguConfig = config->imguConfig();
> -	if (imguConfig.isNull())

You can't drop this, as you the comment explain if not imguConfig was
calculated in validate() it's because !yuvCount, and we should stop
here in configure.

You can keep collecting the pipe_ sizes in validate() and replace this
check with
        if (pipe_.main.isNull() && pipe_.viewfinder.isNull())
                return 0;

> +
> +	config->pipeConfig_ = imgu->calculatePipeConfig(&config->pipe_);
> +	if (config->pipeConfig_.isNull()) {
> +		LOG(IPU3, Error) << "Failed to calculate pipe configuration: "
> +				 << "unsupported resolutions.";

Otherwise this will always fail in case of RAW-only capture.

>  		return 0;
> +	}
>
> -	ret = imgu->configure(imguConfig, &cio2Format);
> +	ret = imgu->configure(config->pipeConfig_, &cio2Format);

Then you can move the ImgU pipe configuration to
ImgUDevice::configure()

>  	if (ret)
>  		return ret;
>
> @@ -633,6 +628,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  		return ret;
>  	}
>
> +	std::map<uint32_t, ControlInfoMap> entityControls;
> +	entityControls.emplace(0, data->cio2_.sensor()->controls());
> +	data->ipa_->configure(entityControls, config->pipeConfig_.bds);
> +

Moving ipa_->configure() here is worth a separate patch
Adding the BDS rectange to IPA::configure() is worth a separate patch

Thanks
  j

>  	return 0;
>  }
>
> @@ -717,7 +716,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)
>
>  int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlList *controls)
>  {
> -	std::map<uint32_t, ControlInfoMap> entityControls;
>  	IPU3CameraData *data = cameraData(camera);
>  	CIO2Device *cio2 = &data->cio2_;
>  	ImgUDevice *imgu = data->imgu_;
> @@ -744,9 +742,6 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis
>  	if (ret)
>  		goto error;
>
> -	entityControls.emplace(0, data->cio2_.sensor()->controls());
> -	data->ipa_->configure(entityControls);
> -
>  	return 0;
>
>  error:
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel