[libcamera-devel] [PATCH] ipa: ipu3: af: enforce grid size restrictions

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thu, Mar 31, 2022 at 01:10:32PM +0100, Kieran Bingham via libcamera-devel wrote:
> Provide enforcement of the selection of the block_{width,height}_log2
> parameters to the capabilities of the hardware.
> 
> While this selection is currently hardcoded to the minimum, providing
> the restriction now allows for further dynamic sizing in the future and
> documents the restrictions directly in code, making use of the already
> existing constants.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> 
> Note, this is kept separate from my other AF improvement series, as it
> can be applied independantly - but due to a compilation failure if
> kAfMinGridBlockWidth, kAfMaxGridBlockWidth kAfMinGridBlockHeight, and
> kAfMaxGridBlockHeight are not used, this patch must be applied before
> that series can be integrated.
> 
>  src/ipa/ipu3/algorithms/af.cpp | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp
> index 6c25d02ec7f0..abfaad6164f1 100644
> --- a/src/ipa/ipu3/algorithms/af.cpp
> +++ b/src/ipa/ipu3/algorithms/af.cpp
> @@ -139,6 +139,22 @@ int Af::configure(IPAContext &context, const IPAConfigInfo &configInfo)
>  	grid.height = kAfMinGridHeight;
>  	grid.block_width_log2 = kAfMinGridBlockWidth;
>  	grid.block_height_log2 = kAfMinGridBlockHeight;
> +
> +	/*
> +	 * \todo - while this clamping code is effectively a no-op, it satisfies
> +	 * the compiler that the definitions regarding the hardware restrictions

"... that the definitions of the hardware limits constants are used" ?

"satisfy ... that" sounds a bit weird to me, but maybe it's just me.

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

> +	 * are used, and paves the way to support dynamic grid sizing in the
> +	 * future. While the block_{width,height}_log2 remain assigned to the
> +	 * minimum, this code should be optimized out by the compiler.
> +	 */
> +	grid.block_width_log2 = std::clamp(grid.block_width_log2,
> +					   kAfMinGridBlockWidth,
> +					   kAfMaxGridBlockWidth);
> +
> +	grid.block_height_log2 = std::clamp(grid.block_height_log2,
> +					    kAfMinGridBlockHeight,
> +					    kAfMaxGridBlockHeight);
> +
>  	grid.height_per_slice = kAfDefaultHeightPerSlice;
>  
>  	/* Position the AF grid in the center of the BDS output. */

-- 
Regards,

Laurent Pinchart