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

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Sun, Mar 14, 2021 at 08:19:01PM +0100, Jean-Michel Hautbois wrote:
> On 13/03/2021 09:31, Jacopo Mondi wrote:
> > 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.

Blank line.

> >> 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.
> 
> Yes, there is no real easy straightforward way unfortunately...
> 
> >> 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 ?
> 
> Obviously yes, I can, thanks :-).
> 
> >> +		if (imguConfig.isNull()) {
> >>  			LOG(IPU3, Error) << "Failed to calculate pipe configuration: "
> >>  					 << "unsupported resolutions.";
> >>  			return Invalid;
> >>  		}
> >> +		data_->pipeConfig_ = imguConfig;

I'm sorry, that's a no-go. validate() isn't supposed to modify the
current state of the camera. An application can configure the camera,
generate a new configuration, validate it, and then throw it away. You
would end up storing the second configuration here.

You could instead store the pipe config in IPU3CameraData in
PipelineHandlerIPU3::configure(), that would be simpler. A better option
would be to move the IPA configure() call from
PipelineHandlerIPU3::start() to PipelineHandlerIPU3::configure().

> >>  	}
> >>
> >>  	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;
> >>

-- 
Regards,

Laurent Pinchart