[libcamera-devel] [PATCH 02/11] ipa: ipu3: Improve the documentation of BDS grid

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Mon, Sep 13, 2021 at 04:58:01PM +0200, Jean-Michel Hautbois wrote:
> The BDS grid has its own limitations for the log2 width and height for
> each cell of pixels. Use constexpr for those and comment it to help
> understanding their usage.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 90053069..c71f2a6c 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -345,6 +345,8 @@ int IPAIPU3::start()
>  }
>  
>  /**
> + * \brief Calculate a grid for the AWB statistics
> + *
>   * This function calculates a grid for the AWB algorithm in the IPU3 firmware.
>   * Its input is the BDS output size calculated in the ImgU.
>   * It is limited for now to the simplest method: find the lesser error
> @@ -364,16 +366,24 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)
>  	/* Set the BDS output size in the IPAConfiguration structure */
>  	context_.configuration.grid.bdsOutputSize = bdsOutputSize;
>  
> -	for (uint32_t widthShift = 3; widthShift <= 7; ++widthShift) {
> +	/*
> +	 * The log2 of the width and height of each cell is limited by the ImgU
> +	 * in the interval [3, 7] according to the kernel header.

s/in the interval/to the interval/

> +	 */
> +	constexpr uint32_t kCellMin = 3;
> +	constexpr uint32_t kCellMax = 7;

I think these should be kCellMinShift and kCellMaxShift. Should they
also be moved to the top of the file with kMaxCellWidthPerSet and
kMaxCellHeightPerSet ?

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

> +
> +	for (uint32_t widthShift = kCellMin; widthShift <= kCellMax; ++widthShift) {
>  		uint32_t width = std::min(kMaxCellWidthPerSet,
>  					  bdsOutputSize.width >> widthShift);
>  		width = width << widthShift;
> -		for (uint32_t heightShift = 3; heightShift <= 7; ++heightShift) {
> +
> +		for (uint32_t heightShift = kCellMin; heightShift <= kCellMax; ++heightShift) {
>  			int32_t height = std::min(kMaxCellHeightPerSet,
>  						  bdsOutputSize.height >> heightShift);
>  			height = height << heightShift;
>  			uint32_t error  = std::abs(static_cast<int>(width - bdsOutputSize.width))
> -							+ std::abs(static_cast<int>(height - bdsOutputSize.height));
> +					+ std::abs(static_cast<int>(height - bdsOutputSize.height));
>  
>  			if (error > minError)
>  				continue;

-- 
Regards,

Laurent Pinchart