Hi Bingbu,

On 22/09/2021 06:33, Cao, Bingbu wrote:

Jean-Michel,

Thanks for you patch.
What is the value of .config.grid_cfg.width for your low resolutions?

I don't know if a 1920x1280 output is a low resolution, but the grid is
configured as:
- grid_cfg.width = 79
- grid_cfg.height = 24
- grid_cfg.block_width_log2 = 4
- grid_cfg.block_height_log2 = 6

Here is a full debug output of the AWB part in imgu_css_cfg_acc():

acc->stripe.down_scaled_stripes[0].width: 1280
acc->stripe.down_scaled_stripes[0].height: 1536
acc->stripe.down_scaled_stripes[0].offset: 0
acc->stripe.bds_out_stripes[0].width: 1280
acc->stripe.bds_out_stripes[0].height: 1536
acc->stripe.bds_out_stripes[0].offset: 0
acc->acc->awb.stripes[0].grid.width: 79
acc->awb.stripes[0].grid.block_width_log2: 4
acc->acc->awb.stripes[0].grid.height: 24
acc->awb.stripes[0].grid.block_height_log2: 6
acc->awb.stripes[0].grid.x_start: 0
acc->awb.stripes[0].grid.x_end: 1263
acc->awb.stripes[0].grid.y_start: 0
acc->awb.stripes[0].grid.y_end: 1535
acc->stripe.down_scaled_stripes[1].width: 1280
acc->stripe.down_scaled_stripes[1].height: 1536
acc->stripe.down_scaled_stripes[1].offset: 1024
acc->stripe.bds_out_stripes[1].width: 1280
acc->stripe.bds_out_stripes[1].height: 1536
acc->stripe.bds_out_stripes[1].offset: 1024
acc->acc->awb.stripes[1].grid.width: 79
acc->awb.stripes[1].grid.block_width_log2: 4
acc->acc->awb.stripes[1].grid.height: 24
acc->awb.stripes[1].grid.block_height_log2: 6
acc->awb.stripes[1].grid.x_start: 0
acc->awb.stripes[1].grid.x_end: 1263
acc->awb.stripes[1].grid.y_start: 0
acc->awb.stripes[1].grid.y_end: 1535

This has been outputted with: https://paste.debian.net/1212791/

The examples I gave before were 1280x720 output and not 1920x1080, here
are they:
- without the patch: https://pasteboard.co/hHo4QkVUSk8e.png
- with the patch: https://pasteboard.co/YUGUvS5tD0bo.png

As you can see we have the same behaviour.

Thanks,
JM

________________________
BRs,  
Bingbu Cao 

quoted

-----Original Message-----
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Sent: Tuesday, September 21, 2021 10:34 PM
To: Jean-Michel Hautbois <redacted>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>; tfiga@google.com; linux-
media@vger.kernel.org; Qiu, Tian Shu [off-list ref]; Cao,
Bingbu [off-list ref]
Subject: Re: [PATCH] media: staging: ipu3-imgu: Initialise
height_per_slice in the stripes

On Tue, Sep 21, 2021 at 03:04:37PM +0200, Jean-Michel Hautbois wrote:

quoted

Hi Sakari, and Tomasz as I have a remark/question for you :-)

On 21/09/2021 13:07, Sakari Ailus wrote:

quoted

Hi Jean-Michel --- and Bingbu and Tianshu,

On Thu, Sep 16, 2021 at 07:25:04PM +0200, Jean-Michel Hautbois wrote:

quoted

While playing with low resolutions for the grid, it appeared that
height_per_slice is not initialised if we are not using both
stripes for the calculations. This pattern occurs three times:
- for the awb_fr processing block
- for the af processing block
- for the awb processing block

The idea of this small portion of code is to reduce complexity in
loading the statistics, it could be done also when only one stripe
is used. Fix it by getting this initialisation code outside of the
else() test case.

Signed-off-by: Jean-Michel Hautbois
[off-list ref]
---
 drivers/staging/media/ipu3/ipu3-css-params.c | 44
++++++++++----------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/media/ipu3/ipu3-css-params.c

b/drivers/staging/media/ipu3/ipu3-css-params.c
index e9d6bd9e9332..05da7dbdca78 100644

--- a/drivers/staging/media/ipu3/ipu3-css-params.c
+++ b/drivers/staging/media/ipu3/ipu3-css-params.c

@@ -2428,16 +2428,16 @@ int imgu_css_cfg_acc(struct imgu_css *css,

unsigned int pipe,

quoted

quoted

quoted

 					acc->awb_fr.stripes[1].grid_cfg.width,
 					b_w_log2);
 		acc->awb_fr.stripes[1].grid_cfg.x_end = end;
-
-		/*
-		 * To reduce complexity of debubbling and loading
-		 * statistics fix grid_height_per_slice to 1 for both
-		 * stripes.
-		 */
-		for (i = 0; i < stripes; i++)
-			acc->awb_fr.stripes[i].grid_cfg.height_per_slice = 1;
 	}

+	/*
+	 * To reduce complexity of debubbling and loading
+	 * statistics fix grid_height_per_slice to 1 for both
+	 * stripes.
+	 */
+	for (i = 0; i < stripes; i++)
+		acc->awb_fr.stripes[i].grid_cfg.height_per_slice = 1;
+
 	if (imgu_css_awb_fr_ops_calc(css, pipe, &acc->awb_fr))
 		return -EINVAL;

@@ -2591,15 +2591,15 @@ int imgu_css_cfg_acc(struct imgu_css *css,

unsigned int pipe,

quoted

quoted

quoted

 			imgu_css_grid_end(acc->af.stripes[1].grid_cfg.x_start,
 					  acc->af.stripes[1].grid_cfg.width,
 					  b_w_log2);
-
-		/*
-		 * To reduce complexity of debubbling and loading statistics
-		 * fix grid_height_per_slice to 1 for both stripes
-		 */
-		for (i = 0; i < stripes; i++)
-			acc->af.stripes[i].grid_cfg.height_per_slice = 1;
 	}

+	/*
+	 * To reduce complexity of debubbling and loading statistics
+	 * fix grid_height_per_slice to 1 for both stripes
+	 */
+	for (i = 0; i < stripes; i++)
+		acc->af.stripes[i].grid_cfg.height_per_slice = 1;
+
 	if (imgu_css_af_ops_calc(css, pipe, &acc->af))
 		return -EINVAL;

@@ -2660,15 +2660,15 @@ int imgu_css_cfg_acc(struct imgu_css *css,

unsigned int pipe,

quoted

quoted

quoted

 			imgu_css_grid_end(acc->awb.stripes[1].grid.x_start,
 					  acc->awb.stripes[1].grid.width,
 					  b_w_log2);
-
-		/*
-		 * To reduce complexity of debubbling and loading statistics
-		 * fix grid_height_per_slice to 1 for both stripes
-		 */
-		for (i = 0; i < stripes; i++)
-			acc->awb.stripes[i].grid.height_per_slice = 1;
 	}

+	/*
+	 * To reduce complexity of debubbling and loading statistics
+	 * fix grid_height_per_slice to 1 for both stripes
+	 */
+	for (i = 0; i < stripes; i++)
+		acc->awb.stripes[i].grid.height_per_slice = 1;
+
 	if (imgu_css_awb_ops_calc(css, pipe, &acc->awb))
 		return -EINVAL;

While it seems like a sensible idea to initialise arguments to
firmware, does this have an effect on the statistics format? If so,
can the existing user space cope with that?

To try and figure that out, we have tested several grid configurations
and inspected the captured statistics. We have converted the
statistics in an image, rendering each cell as a pixel whose red,
green and blue components are the cell's red, green and blue averages.
This turned out to be a very effectice tool to quickly visualize AWB

statistics.

quoted

We have made a lot of tests with different output resolutions, from a
small one up to the full-scale one.

Here is one example of a statistics output with a ViewFinder
configured as 1920x1280, with a BDS output configuration set to
2304x1536 (sensor is 2592x1944).

Without the patch, configuring a 79x45 grid of 16x16 cells we obtain
the
image: https://pasteboard.co/g4nC4fHjbVER.png.
We can notice a weird padding every two lines and it seems to be
missing half of the frame.

With the patch applied, the same configuration gives us the image:
https://pasteboard.co/rzap6axIvVdu.png

We can clearly see the one padding pixel on the right, and the frame
is all there, as expected.

Tomasz: We're concerned that this patch may have an impact on the
ChromeOS Intel Camera HAL with the IPU3. Is it possible for someone to
review and test this please?

As shown by the images above, this is a real fix. It only affects grid
configurations that use a single stripe (left or right), so either
"small" resolutions (less than 1280 pixels at the BDS output if I recall
correctly), or grid configurations that span the left part of the image
with higher resolutions. The latter is probably unlikely. For the former,
it may affect the binary library, especially if it includes a workaround
for the bug.

Still, this change is good I believe, so it should be upstreamed.

--
Regards,

Laurent Pinchart