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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Bingbu,

On 28/09/2021 03:21, Cao, Bingbu wrote:
> 
> 
> ________________________
> BRs,  
> Bingbu Cao 
> 
>> -----Original Message-----
>> From: Jean-Michel Hautbois <jeanmichel.hautbois@xxxxxxxxxxxxxxxx>
>> Sent: Thursday, September 23, 2021 7:57 PM
>> To: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>; Cao, Bingbu
>> <bingbu.cao@xxxxxxxxx>
>> Cc: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>; tfiga@xxxxxxxxxx; linux-
>> media@xxxxxxxxxxxxxxx; Qiu, Tian Shu <tian.shu.qiu@xxxxxxxxx>
>> Subject: Re: [PATCH] media: staging: ipu3-imgu: Initialise
>> height_per_slice in the stripes
>>
>> Hi Bingbu, Laurent,
>>
>> On 23/09/2021 12:49, Laurent Pinchart wrote:
>>> Hi Bingbu,
>>>
>>> On Thu, Sep 23, 2021 at 10:29:33AM +0000, Cao, Bingbu wrote:
>>>> On Thursday, September 23, 2021 5:46 PM, Laurent Pinchart wrote:
>>>>> 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.
>>>>
>>>> For awb_fr and af, it could be 0 if the .config.grid_cfg.width > 32.
>>>
>>> Indeed, my bad. I was focussing on the AWB statistics.
>>>
>>> What are the implications of a height_per_slice value of 0 ?
>>>
>>> While we are on this topic, what is a "slice" ? Does it matter for the
>>> user, as in does it have an impact on the statistics values, or on how
>>> they're arranged in memory, or is it an implementation detail of the
>>> firmware that has no consequence on what can be seen by the user ?
>>> (The "user" here is the code that reads the statistics in userspace).
>>>
>>>>>> 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
>>>>
>>>> Are these dumps from 1920x1280 output?
>>>
>>> Jean-Michel, could you comment on this ?
>>>
>>> Note that the grid is configured with 79 cells of 16 pixels, covering
>>> 1264 pixels horizontally. That's not the full image for a 1920 pixels
>>> output, and will probably not be done in practice, but there's nothing
>>> preventing the grid from covering part of the image only.
>>>
>>
>> It is a dump for a 1920x1280 output.
>> If it can help, the configuration set in ImgU is:
>>   IF: 2592x1728
>>   BDS: 2304x1536
>>   GDC: 1920x1280
> 
> Jean-Michel,
> 
> It looks you are trying to use 2 stripes and the grid size is 2528x1536, and
> the awb.config.grid.x_end should be larger than the 
> bds_out_stripes[0].width - 10, it would not hit any 1 stripe condition.
> 
> could you also share your awb.config.grid?

I already shared it:
- grid_cfg.width = 79
- grid_cfg.height = 24
- grid_cfg.block_width_log2 = 4
- grid_cfg.block_height_log2 = 6
start_x and start_y are set to 0.


> 
>>
>>
>>>>>>> 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:
>>>>>>>>>> On 21/09/2021 13:07, Sakari Ailus wrote:
>>>>>>>>>>> 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.
>>>



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux