[libcamera-devel] [PATCH v1 5/9] libcamera: pipeline: raspberrypi: Move configureIPA() to RPiCameraData

[Jacopo Mondi]

Hi Laurent,

On Mon, Jun 29, 2020 at 02:19:30AM +0300, Laurent Pinchart wrote:
> The PipelineHandlerRPi::configureIPA() function accesses plenty of
> member data from the RPiCameraData class and no member from the
> PipelineHandlerRPi class. Move it to RPiCameraData where it logically
> belongs.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 117 +++++++++---------
>  1 file changed, 58 insertions(+), 59 deletions(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 3b5cdf1e1849..0f9237a7f346 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -311,6 +311,8 @@ public:
>  	void frameStarted(uint32_t sequence);
>
>  	int loadIPA();
> +	int configureIPA();
> +
>  	void queueFrameAction(unsigned int frame, const IPAOperationData &action);
>
>  	/* bufferComplete signal handlers. */
> @@ -396,8 +398,6 @@ private:
>  		return static_cast<RPiCameraData *>(PipelineHandler::cameraData(camera));
>  	}
>
> -	int configureIPA(Camera *camera);
> -
>  	int queueAllBuffers(Camera *camera);
>  	int prepareBuffers(Camera *camera);
>  	void freeBuffers(Camera *camera);
> @@ -752,7 +752,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  	crop.y = (sensorFormat.size.height - crop.height) >> 1;
>  	data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
>
> -	ret = configureIPA(camera);
> +	ret = data->configureIPA();
>  	if (ret)
>  		LOG(RPI, Error) << "Failed to configure the IPA: " << ret;
>
> @@ -968,62 +968,6 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
>  	return true;
>  }
>
> -int PipelineHandlerRPi::configureIPA(Camera *camera)
> -{
> -	std::map<unsigned int, IPAStream> streamConfig;
> -	std::map<unsigned int, const ControlInfoMap &> entityControls;
> -	RPiCameraData *data = cameraData(camera);
> -
> -	/* Get the device format to pass to the IPA. */
> -	V4L2DeviceFormat sensorFormat;
> -	data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);
> -	/* Inform IPA of stream configuration and sensor controls. */
> -	unsigned int i = 0;
> -	for (auto const &stream : data->isp_) {
> -		if (stream.isExternal()) {
> -			streamConfig[i] = {
> -				.pixelFormat = stream.configuration().pixelFormat,
> -				.size = stream.configuration().size
> -			};
> -		}
> -	}
> -	entityControls.emplace(0, data->unicam_[Unicam::Image].dev()->controls());
> -	entityControls.emplace(1, data->isp_[Isp::Input].dev()->controls());
> -
> -	/* Allocate the lens shading table via vcsm and pass to the IPA. */
> -	if (!data->lsTable_) {
> -		data->lsTable_ = data->vcsm_.alloc("ls_grid", MAX_LS_GRID_SIZE);
> -		uintptr_t ptr = reinterpret_cast<uintptr_t>(data->lsTable_);
> -
> -		if (!data->lsTable_)
> -			return -ENOMEM;
> -
> -		/*
> -		 * The vcsm allocation will always be in the memory region
> -		 * < 32-bits to allow Videocore to access the memory.
> -		 */
> -		IPAOperationData op;
> -		op.operation = RPI_IPA_EVENT_LS_TABLE_ALLOCATION;
> -		op.data = { static_cast<uint32_t>(ptr & 0xffffffff),
> -			    data->vcsm_.getVCHandle(data->lsTable_) };
> -		data->ipa_->processEvent(op);
> -	}
> -
> -	CameraSensorInfo sensorInfo = {};
> -	int ret = data->sensor_->sensorInfo(&sensorInfo);
> -	if (ret) {
> -		LOG(RPI, Error) << "Failed to retrieve camera sensor info";
> -		return ret;
> -	}
> -
> -	/* Ready the IPA - it must know about the sensor resolution. */
> -	IPAOperationData ipaConfig;
> -	data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> -			      ipaConfig, nullptr);
> -
> -	return 0;
> -}
> -
>  int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
>  {
>  	RPiCameraData *data = cameraData(camera);
> @@ -1161,6 +1105,61 @@ int RPiCameraData::loadIPA()
>  	return ipa_->start();
>  }
>
> +int RPiCameraData::configureIPA()
> +{
> +	std::map<unsigned int, IPAStream> streamConfig;
> +	std::map<unsigned int, const ControlInfoMap &> entityControls;
> +
> +	/* Get the device format to pass to the IPA. */
> +	V4L2DeviceFormat sensorFormat;
> +	unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);
> +	/* Inform IPA of stream configuration and sensor controls. */
> +	unsigned int i = 0;
> +	for (auto const &stream : isp_) {
> +		if (stream.isExternal()) {
> +			streamConfig[i] = {
> +				.pixelFormat = stream.configuration().pixelFormat,
> +				.size = stream.configuration().size
> +			};
> +		}
> +	}
> +	entityControls.emplace(0, unicam_[Unicam::Image].dev()->controls());
> +	entityControls.emplace(1, isp_[Isp::Input].dev()->controls());
> +
> +	/* Allocate the lens shading table via vcsm and pass to the IPA. */
> +	if (!lsTable_) {
> +		lsTable_ = vcsm_.alloc("ls_grid", MAX_LS_GRID_SIZE);
> +		uintptr_t ptr = reinterpret_cast<uintptr_t>(lsTable_);
> +
> +		if (!lsTable_)
> +			return -ENOMEM;
> +
> +		/*
> +		 * The vcsm allocation will always be in the memory region
> +		 * < 32-bits to allow Videocore to access the memory.
> +		 */
> +		IPAOperationData op;
> +		op.operation = RPI_IPA_EVENT_LS_TABLE_ALLOCATION;
> +		op.data = { static_cast<uint32_t>(ptr & 0xffffffff),
> +			    vcsm_.getVCHandle(lsTable_) };
> +		ipa_->processEvent(op);
> +	}
> +
> +	CameraSensorInfo sensorInfo = {};
> +	int ret = sensor_->sensorInfo(&sensorInfo);
> +	if (ret) {
> +		LOG(RPI, Error) << "Failed to retrieve camera sensor info";
> +		return ret;
> +	}
> +
> +	/* Ready the IPA - it must know about the sensor resolution. */

"Ready the IPA" ? I didn't get what you mean :)

nit apart
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

> +	IPAOperationData ipaConfig;
> +	ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> +			nullptr);
> +
> +	return 0;
> +}
> +
>  void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData &action)
>  {
>  	/*
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel