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

[Jean-Michel Hautbois]

Hi Jacopo,

On 13/03/2021 09:31, Jacopo Mondi wrote:
> Hello Jean-Micheal,
> 
> 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.
>> 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 :-).

> Thanks
>   j
> 
>> +		if (imguConfig.isNull()) {
>>  			LOG(IPU3, Error) << "Failed to calculate pipe configuration: "
>>  					 << "unsupported resolutions.";
>>  			return Invalid;
>>  		}
>> +		data_->pipeConfig_ = imguConfig;
>>  	}
>>
>>  	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;
>>
>> --
>> 2.27.0
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
> 

-- 
Regards,
JM