staging: media: ipu3: Fix AF x_start position when rightmost stripe is used

PATCH Superseded Kate Hsuan 2 participants 4 messages Mar 14, 2022 – Mar 16, 2022

Subsystems (3)

Media (V4L/DVB) Staging Intel IPU3 CSI-2 imgu driver

lore

Share & tools

lore

Kate Hsuan

author

2022-03-14 10:15 UTC #0 lore raw

PATCH v3 1/2 staging: media: ipu3: Fix AF x_start position when rightmost stripe is used

For the AF configuration, if the rightmost stripe is used, the AF scene will be at the incorrect location of the sensor.

The AF coordinate may be set to the right part of the sensor. This configuration would lead to x_start being greater than the down_scaled_stripes offset and the leftmost stripe would be disabled and only the rightmost stripe is used to control the AF coordinate. If the x_start doesn't perform any adjustments, the AF coordinate will be at the wrong place of the sensor since down_scaled_stripes offset would be the new zero of the coordinate system.

In this patch, if only the rightmost stripe is used, x_start should minus down_scaled_stripes offset to maintain its correctness of AF scene coordinate.

Changes in v2:
1. Remove the setting of the first stripe.

Signed-off-by Kate Hsuan <[email protected]>

---
 drivers/staging/media/ipu3/ipu3-css-params.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/staging/media/ipu3/ipu3-css-params.c +4 -0

index d9e3c3785075..5a8c07f34756 100644
@@ -2556,6 +2556,10 @@ int imgu_css_cfg_acc(struct imgu_css *css, unsigned int pipe,
 		/* Enable only for rightmost stripe, disable left */
 		acc->af.stripes[0].grid_cfg.y_start &=
 			~IPU3_UAPI_GRID_Y_START_EN;
+		acc->af.stripes[1].grid_cfg.x_start -=
+			acc->stripe.down_scaled_stripes[1].offset;
+		acc->af.stripes[1].grid_cfg.x_end -=
+			acc->stripe.down_scaled_stripes[1].offset;
 	} else if (acc->af.config.grid_cfg.x_end <=
 		   acc->stripe.bds_out_stripes[0].width - min_overlap) {
 		/* Enable only for leftmost stripe, disable right */

Signature · 1 line

2.35.1

Kate Hsuan

author

2022-03-14 10:15 UTC #1 lore raw

PATCH v3 2/2 staging: media: ipu3: fixing stripe1 x_end is greater than BDS width

If both stripes are enabled, an improper AF grid configuration leads the AF scene width to be wilder than the BDS width. Also, the second stripe x_end is estimated based on the remaining AF scene width. Therefore, the second stripe x_end will be greater than the second stripe BDS width.

In this patch, if the second stripe x_end is greater than BDS width, x_end will be set to BDS width.

Signed-off-by Kate Hsuan <[email protected]>

---
 drivers/staging/media/ipu3/ipu3-css-params.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/staging/media/ipu3/ipu3-css-params.c +4 -0

index 5a8c07f34756..8923760aa913 100644
@@ -2593,6 +2593,10 @@ int imgu_css_cfg_acc(struct imgu_css *css, unsigned int pipe,
 					  acc->af.stripes[1].grid_cfg.width,
 					  b_w_log2);
 
+		if (acc->af.stripes[1].grid_cfg.x_end >= acc->stripe.bds_out_stripes[1].width)
+			acc->af.stripes[1].grid_cfg.x_end =
+				acc->stripe.bds_out_stripes[1].width - min_overlap;
+
 		/*
 		 * To reduce complexity of debubbling and loading statistics
 		 * fix grid_height_per_slice to 1 for both stripes

Signature · 1 line

2.35.1

Sakari Ailus

maintainer

2022-03-16 09:02 UTC #2 lore raw

In reply to Kate Hsuan

Hi Kate,

Quoted 28 lines · Kate Hsuan

For the AF configuration, if the rightmost stripe is used, the AF scene will be at the incorrect location of the sensor.
The AF coordinate may be set to the right part of the sensor. This configuration would lead to x_start being greater than the down_scaled_stripes offset and the leftmost stripe would be disabled and only the rightmost stripe is used to control the AF coordinate. If the x_start doesn't perform any adjustments, the AF coordinate will be at the wrong place of the sensor since down_scaled_stripes offset would be the new zero of the coordinate system.
In this patch, if only the rightmost stripe is used, x_start should minus down_scaled_stripes offset to maintain its correctness of AF scene coordinate.
Changes in v2:
1. Remove the setting of the first stripe.
Signed-off-by Kate Hsuan <[email protected]>
---
 drivers/staging/media/ipu3/ipu3-css-params.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/staging/media/ipu3/ipu3-css-params.c +0 -0
index d9e3c3785075..5a8c07f34756 100644

drivers/staging/media/ipu3/ipu3-css-params.c int imgu_css_cfg_acc(struct imgu_css *css, unsigned int pipe, · line 2562

Quoted 7 lines

drivers/staging/media/ipu3/ipu3-css-params.c +4 -0

@@ -2556,6 +2556,10 @@ int imgu_css_cfg_acc(struct imgu_css *css, unsigned int pipe,
 		/* Enable only for rightmost stripe, disable left */
 		acc->af.stripes[0].grid_cfg.y_start &=
 			~IPU3_UAPI_GRID_Y_START_EN;
+		acc->af.stripes[1].grid_cfg.x_start -=
+			acc->stripe.down_scaled_stripes[1].offset;
+		acc->af.stripes[1].grid_cfg.x_end -=
+			acc->stripe.down_scaled_stripes[1].offset;

Could you calculate the values the same way as for the two stripes case, as I've been asking since v1?

Quoted 3 lines

drivers/staging/media/ipu3/ipu3-css-params.c +0 -0

 	} else if (acc->af.config.grid_cfg.x_end <=
 		   acc->stripe.bds_out_stripes[0].width - min_overlap) {
 		/* Enable only for leftmost stripe, disable right */

Signature · 3 lines

Regards,

Sakari Ailus

Kate Hsuan

author

2022-03-16 09:33 UTC #3 lore raw

Hi Sakari,

Quoted 27 lines · Sakari Ailus

Hi Kate,
Quoted 23 lines · Kate Hsuan
For the AF configuration, if the rightmost stripe is used, the AF scene will be at the incorrect location of the sensor.
The AF coordinate may be set to the right part of the sensor. This configuration would lead to x_start being greater than the down_scaled_stripes offset and the leftmost stripe would be disabled and only the rightmost stripe is used to control the AF coordinate. If the x_start doesn't perform any adjustments, the AF coordinate will be at the wrong place of the sensor since down_scaled_stripes offset would be the new zero of the coordinate system.
In this patch, if only the rightmost stripe is used, x_start should minus down_scaled_stripes offset to maintain its correctness of AF scene coordinate.
Changes in v2:
1. Remove the setting of the first stripe.
Signed-off-by Kate Hsuan <[email protected]>
---
 drivers/staging/media/ipu3/ipu3-css-params.c | 4 ++++
 1 file changed, 4 insertions(+)

Quoted 15 lines

drivers/staging/media/ipu3/ipu3-css-params.c int imgu_css_cfg_acc(struct imgu_css *css, unsigned int pipe, · line 2562

Quoted 12 lines

drivers/staging/media/ipu3/ipu3-css-params.c +4 -0

index d9e3c3785075..5a8c07f34756 100644
@@ -2556,6 +2556,10 @@ int imgu_css_cfg_acc(struct imgu_css *css, unsigned int pipe,
              /* Enable only for rightmost stripe, disable left */
              acc->af.stripes[0].grid_cfg.y_start &=
                      ~IPU3_UAPI_GRID_Y_START_EN;
+             acc->af.stripes[1].grid_cfg.x_start -=
+                     acc->stripe.down_scaled_stripes[1].offset;
+             acc->af.stripes[1].grid_cfg.x_end -=
+                     acc->stripe.down_scaled_stripes[1].offset;

Could you calculate the values the same way as for the two stripes case, as I've been asking since v1?

Sorry for my misunderstanding of the comments.
You mean x_end could be estimated by the width here and could use the same way as for two stripes case to estimate this.
I'll correct this in my v4 patch. :)

Thank you.

Quoted 10 lines

drivers/staging/media/ipu3/ipu3-css-params.c int imgu_css_cfg_acc(struct imgu_css *css, unsigned int pipe, · line 2565
Quoted 4 lines
drivers/staging/media/ipu3/ipu3-css-params.c +0 -0
      } else if (acc->af.config.grid_cfg.x_end <=
                 acc->stripe.bds_out_stripes[0].width - min_overlap) {
              /* Enable only for leftmost stripe, disable right */
--
Regards,
Sakari Ailus

Signature · 2 lines

BR,
Kate