[libcamera-devel] ipa: ipu3: Correct context during configure()

17475 diff mbox series

The introduction of the FCQueue in the IPU3 inadvertently introduced a
bug which cleared the initialisation of the session configuration
immediately after some parameters had been set.

Furthermore, it cleared and never re-initialised the sensor line
duration property, which was previously only set during the call to
init().

Move the clearing of the contexts from the updateSessionConfiguration()
call to the earliest opportunity in configure(), and immediately
re-initialise the sensor parameters.

Bug: https://bugs.libcamera.org/show_bug.cgi?id=151
Fixes: 85c5c47325ab ("ipa: ipu3: Use the FCQueue")
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/ipa/ipu3/ipu3.cpp | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Hi Kieran,

Thank you for the patch.

On Fri, Sep 30, 2022 at 08:08:21PM +0100, Kieran Bingham via libcamera-devel wrote:
> The introduction of the FCQueue in the IPU3 inadvertently introduced a
> bug which cleared the initialisation of the session configuration
> immediately after some parameters had been set.
> 
> Furthermore, it cleared and never re-initialised the sensor line
> duration property, which was previously only set during the call to
> init().
> 
> Move the clearing of the contexts from the updateSessionConfiguration()
> call to the earliest opportunity in configure(), and immediately
> re-initialise the sensor parameters.
> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=151
> Fixes: 85c5c47325ab ("ipa: ipu3: Use the FCQueue")
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index d1ea081d595d..7fb62c749fde 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -217,11 +217,6 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)
>  	int32_t minGain = v4l2Gain.min().get<int32_t>();
>  	int32_t maxGain = v4l2Gain.max().get<int32_t>();
>  
> -	/* Clear the IPA context before the streaming session. */
> -	context_.configuration = {};
> -	context_.activeState = {};
> -	context_.frameContexts.clear();
> -
>  	/*
>  	 * When the AGC computes the new exposure values for a frame, it needs
>  	 * to know the limits for shutter speed and analogue gain.
> @@ -498,6 +493,14 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>  
>  	lensCtrls_ = configInfo.lensControls;
>  
> +	/* Clear the IPA context for the new streaming session. */
> +	context_.activeState = {};
> +	context_.configuration = {};
> +	context_.frameContexts.clear();
> +
> +	/* Intialise the sensor configuration */
> +	context_.configuration.sensor.lineDuration = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate;

This could be dropped from init(), but that can be done in a different
patch, we should revisit the init()/configure() split in the IPU3 IPA
module implementation.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
>  	/*
>  	 * Compute the sensor V4L2 controls to be used by the algorithms and
>  	 * to be set on the sensor.

On Fri, Sep 30, 2022 at 08:08:21PM +0100, Kieran Bingham via libcamera-devel wrote:
> The introduction of the FCQueue in the IPU3 inadvertently introduced a
> bug which cleared the initialisation of the session configuration
> immediately after some parameters had been set.
>
> Furthermore, it cleared and never re-initialised the sensor line
> duration property, which was previously only set during the call to
> init().
>
> Move the clearing of the contexts from the updateSessionConfiguration()
> call to the earliest opportunity in configure(), and immediately
> re-initialise the sensor parameters.
>
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=151
> Fixes: 85c5c47325ab ("ipa: ipu3: Use the FCQueue")
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index d1ea081d595d..7fb62c749fde 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -217,11 +217,6 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)
>  	int32_t minGain = v4l2Gain.min().get<int32_t>();
>  	int32_t maxGain = v4l2Gain.max().get<int32_t>();
>
> -	/* Clear the IPA context before the streaming session. */
> -	context_.configuration = {};
> -	context_.activeState = {};
> -	context_.frameContexts.clear();
> -
>  	/*
>  	 * When the AGC computes the new exposure values for a frame, it needs
>  	 * to know the limits for shutter speed and analogue gain.
> @@ -498,6 +493,14 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>
>  	lensCtrls_ = configInfo.lensControls;
>
> +	/* Clear the IPA context for the new streaming session. */
> +	context_.activeState = {};
> +	context_.configuration = {};
> +	context_.frameContexts.clear();
> +
> +	/* Intialise the sensor configuration */
> +	context_.configuration.sensor.lineDuration = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate;

It is currently calculated at init time one, so it's correct to move
it here (possibily removing it from init(), it doesn't seem to be used
in any function called before configure().)

It might make sense to intiialize lineDuration in updateControls(),
but that would make it less clear that updateControls() shall be
called before updateSessionConfiguration(), so I guess it's fine to
have it here.

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> +
>  	/*
>  	 * Compute the sensor V4L2 controls to be used by the algorithms and
>  	 * to be set on the sensor.
> --
> 2.34.1
>