[libcamera-devel] [PATCH v3 6/6] ipa: ipu3: Do not set controls during configure

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wed, Mar 24, 2021 at 03:01:25PM +0000, Kieran Bingham wrote:
> The configure operation is synchronous and should not send events back
> to the pipeline handler.
> 
> If information needs to be returned from configure it should be handled
> through the interface directly.
> 
> Move the initial call to setControls() out of configure() and into the
> start() method which is called after the IPA running_ state is updated.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index a5c5e029f465..34a907f23ef5 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -32,7 +32,7 @@ public:
>  	{
>  		return 0;
>  	}
> -	int start() override { return 0; }
> +	int start() override;
>  	void stop() override {}
>  
>  	void configure(const std::map<uint32_t, ControlInfoMap> &entityControls,
> @@ -63,6 +63,13 @@ private:
>  	uint32_t maxGain_;
>  };
>  
> +int IPAIPU3::start()
> +{
> +	setControls(0);

This will send an asynchronous event to the pipeline handler, which will
process it after starting the device. It defeats a little bit the point
of setting initial controls, doesn't it ?

The IPU3 IPA protocol seems ill-defined here, as it uses a frame
counter, and it's not clear if that counter is 0-based or 1-based.

All this doesn't matter much right now as we never call setControls() at
runtime, so we could merge the patch as is, but please keep in mind that
the mechanism is likely broken, and JM would then need to fix it
(including redesigning part of the IPA protocol) as part of his pending
IPU3 IPA series. This will involve figuring out how to properly interact
with delayed controls.

> +
> +	return 0;
> +}
> +
>  void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls,
>  			[[maybe_unused]] const Size &bdsOutputSize)
>  {
> @@ -90,8 +97,6 @@ void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls
>  	minGain_ = std::max(itGain->second.min().get<int32_t>(), 1);
>  	maxGain_ = itGain->second.max().get<int32_t>();
>  	gain_ = maxGain_;
> -
> -	setControls(0);
>  }
>  
>  void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)

-- 
Regards,

Laurent Pinchart