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 > >> -----Original Message----- >> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> >> Sent: Tuesday, September 21, 2021 10:34 PM >> To: Jean-Michel Hautbois <jeanmichel.hautbois@xxxxxxxxxxxxxxxx> >> Cc: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>; tfiga@xxxxxxxxxx; linux- >> media@xxxxxxxxxxxxxxx; Qiu, Tian Shu <tian.shu.qiu@xxxxxxxxx>; Cao, >> Bingbu <bingbu.cao@xxxxxxxxx> >> 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: >>> 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@xxxxxxxxxxxxxxxx> >>>>> --- >>>>> 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
