Re: [PATCH] media: staging: ipu3-imgu: Initialise height_per_slice in the stripes

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Bingbu,

On Thu, Sep 23, 2021 at 09:06:32AM +0000, Cao, Bingbu wrote:
> Jean-Michel,
> 
> Firstly, the .height_per_slice could be 0 if your .grid.width larger than 32.

Which .height_per_slice are you talking about ? A field of that name
exists in both ipu3_uapi_acc_param.awb.config.grid and 
struct ipu3_uapi_grid_config and imgu_abi_awb_config.stripes.grid.

They are both computed by the driver, in imgu_css_cfg_acc(). The former
is set to

	acc->awb.config.grid.height_per_slice =
		IMGU_ABI_AWB_MAX_CELLS_PER_SET / acc->awb.config.grid.width,

IMGU_ABI_AWB_MAX_CELLS_PER_SET is equal to 160, so it can only be 0 if
grid.width > 160, which is invalid.

> From your configuration, looks like something wrong in the stripe configuration
> cause not entering the 2 stripes branch.

Why is that ? Isn't it valid for a grid configuration to use a single
stripe, if the image is small enough, or if the grid only covers the
left part of the image ?

> On Wednesday, September 22, 2021 1:54 PM, Jean-Michel Hautbois wrote:
> > 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.
> > 
> > > On Tuesday, September 21, 2021 10:34 PM, Laurent Pinchart wrote:
> > >> On Tue, Sep 21, 2021 at 03:04:37PM +0200, Jean-Michel Hautbois wrote:
> > >>> Hi Sakari, and Tomasz as I have a remark/question for you :-)
> > >>>
> > >>> On 21/09/2021 13:07, Sakari Ailus wrote:
> > >>>> Hi Jean-Michel --- and Bingbu and Tianshu,
> > >>>>
> > >>>> On Thu, Sep 16, 2021 at 07:25:04PM +0200, Jean-Michel Hautbois wrote:
> > >>>>> 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 <jeanmichel.hautbois@ideasonboard.com>
> > >>>>> ---
> > >>>>>  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,
> > >>>>>  					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,
> > >>>>>  			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,
> > >>>>>  			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.
> > >>> 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