[libcamera-devel] [PATCH] ipa: ipu3: af: Set default grid block width to the minimum value

[Jean-Michel Hautbois]

On 22/03/2022 09:29, Kate Hsuan wrote:
> Hi Jean-Michel,
> 
> On Tue, Mar 22, 2022 at 2:17 PM Jean-Michel Hautbois
> <jeanmichel.hautbois at ideasonboard.com> wrote:
>>
>> Hi Kate,
>>
>> On 22/03/2022 02:31, Kate Hsuan via libcamera-devel wrote:
>>> Since x       coordinate is incorrectly computed by a kernel issue, the block width
>>> should be set to 4 to prevent using the second stripe when setting the AF scene
>>> to the centre of the image. A kernel patch had fixed this issue. Therefore, this
>>> value can be set to the default minimum value.
>>
>> If this is working fine with a width of 256 (2**4 * 16), why do you want
>> to change it to have a smaller focus region ?
>>
>>>
>>> Signed-off-by: Kate Hsuan <hpa at redhat.com>
>>> ---
>>> The kernel patch is shown as following URL.
>>> https://lore.kernel.org/linux-media/CAEth8oES8abPO4p7eFv43PwDXuxeOmg1661YtVvykBPrkagzKg@mail.gmail.com/T/#mb02fa73ce9e3089a4619c318badb2047a3ac39e2
>>> ---
>>>    src/ipa/ipu3/algorithms/af.h | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h
>>> index 13c7e0e8..108fcd18 100644
>>> --- a/src/ipa/ipu3/algorithms/af.h
>>> +++ b/src/ipa/ipu3/algorithms/af.h
>>> @@ -20,7 +20,7 @@ static constexpr uint8_t kAfMinGridWidth = 16;
>>>    static constexpr uint8_t kAfMinGridHeight = 16;
>>>    static constexpr uint8_t kAfMaxGridWidth = 32;
>>>    static constexpr uint8_t kAfMaxGridHeight = 24;
>>> -static constexpr uint16_t kAfMinGridBlockWidth = 4;
>>> +static constexpr uint16_t kAfMinGridBlockWidth = 3;
>>>    static constexpr uint16_t kAfMinGridBlockHeight = 3;
>>>    static constexpr uint16_t kAfMaxGridBlockWidth = 6;
>>>    static constexpr uint16_t kAfMaxGridBlockHeight = 6;
>>
> 
> The kernel patch fixed the issue on the second (rightmost) stripe configuration.
> This means we can set x_start more than 640.
> Also, according to the chomiumOS implementation, it set
> kAfMinGridBlockWidth to 3. So, it may align with chromium OS
> implementation after the patch is merged to kernel release.
> https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/refs/heads/master/hal/intel/include/ia_imaging/af_public.h#36

I get this, but it there a strong reason to do this ? Are we 
experimenting bad behaviour when the grid is bigger in full resolution 
maybe ? I would like to be sure it is not just "because CrOS does it" 
:-) at least we need to know why.

JM