[libcamera-devel,1/2] libcamera: ipu3: Move Imgu configuration to IPU3CameraData
diff mbox series

Message ID 20210312130304.34698-2-jeanmichel.hautbois@ideasonboard.com
State Not Applicable
Headers show
Series
  • Prepare IPU3 pipeline to pass BDS to IPA
Related show

Commit Message

Jean-Michel Hautbois March 12, 2021, 1:03 p.m. UTC
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.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Jacopo Mondi March 13, 2021, 8:31 a.m. UTC | #1
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.

> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@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 ?

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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Jean-Michel Hautbois March 14, 2021, 7:19 p.m. UTC | #2
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@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@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
Laurent Pinchart March 15, 2021, 2:53 a.m. UTC | #3
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@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;
> >>
Kieran Bingham March 15, 2021, 1:08 p.m. UTC | #4
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@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;
>>>>
>

Patch
diff mbox series

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);
+		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;