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

[Kieran Bingham]

Hi All,

On 15/03/2021 02:53, Laurent Pinchart wrote:
> 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().

I think this is the right way forwards.

Our interfaces throughout should be consistent, so we should configure
in configure().

In this instance I think I recall a message from Laurent highlighting
that the IPU3 is currently buggy because it calls configure after it has
started which is incorrect.

So first we need to move the call to configure the IPA before we start
it, and then ensure we configure everything correctly before starting
anything ;-)

--
Kieran

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