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

[Laurent Pinchart]

Hi Jacopo,

On Tue, Jun 30, 2020 at 12:38:37PM +0200, Jacopo Mondi wrote:
> 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 :)

I didn't mean anything, this patch only moves code :-) But I think it
means https://en.wiktionary.org/wiki/ready#Verb.

We can update the comment, but I wouldn't do so in this patch.

> nit apart
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> > +	IPAOperationData ipaConfig;
> > +	ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> > +			nullptr);
> > +
> > +	return 0;
> > +}
> > +
> >  void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData &action)
> >  {
> >  	/*

-- 
Regards,

Laurent Pinchart