| Message ID | 20220322013119.4262-1-hpa@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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@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;
Hi Jean-Michel, On Tue, Mar 22, 2022 at 2:17 PM Jean-Michel Hautbois <jeanmichel.hautbois@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@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 -- BR, Kate
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@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@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
On Tue, Mar 22, 2022 at 09:48:21AM +0100, Jean-Michel Hautbois via libcamera-devel wrote: > On 22/03/2022 09:29, Kate Hsuan wrote: > > On Tue, Mar 22, 2022 at 2:17 PM Jean-Michel Hautbois wrote: > >> 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@redhat.com> > >>> --- > >>> The kernel patch is shown as following URL. > >>> https://lore.kernel.org/linux-media/CAEth8oES8abPO4p7eFv43PwDXuxeOmg1661YtVvykBPrkagzKg@mail.gmail.com/T/#mb02fa73ce9e3089a4619c318badb2047a3ac39e2 This patch will only appear in the kernel in v5.19, so at best we'll have to wait for that kernel to be released. We should however support older kernel versions, so a runtime kernel version check is likely needed. Does the Chrome OS kernel integrate that kernel fix too ? > >>> --- > >>> 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.
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;
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. Signed-off-by: Kate Hsuan <hpa@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(-)