[PATCH v3 0/3] media: imx: imx7-media-csi: Fix width alignment

[PATCH v3 0/3] media: imx: imx7-media-csi: Fix width alignment

[Laurent Pinchart]

Hello,

This patch series is an attempt to fix the issue reported by Tim Harvey
in [1].

Both Alexander Stein and Fabio Estevam gave this a try in [2] and [3]
respectively, with Alexander's self-nacking his patches and Fabio's
solution receiving requests for changes during review.

Compared to those attempts, this version implements a simpler fix for
the issue (in patch 1/3), before addressing a TODO item (patch 2/3) and
cleaning up includes as a drive-by improvement (patch 3/3).

The series doesn't need to be merged in one go. Patch 1/3 can get merged
as a fix for v6.5, while patches 2/3 and 3/3 can wait until v6.6.

Tim, would you be able to test this ?

[1] https://lore.kernel.org/linux-media/CAJ+vNU0BOVLTL17ofgHwtexbpuMYwH_aGUC==EXABUtHHiv_ag@mail.gmail.com/
[2] https://lore.kernel.org/linux-media/20230720074129.3680269-1-alexander.stein@ew.tq-group.com
[3] https://lore.kernel.org/linux-media/20230720222543.1740198-1-festevam@gmail.com

Fabio Estevam (1):
  media: imx: imx7-media-csi: Fix applying format constraints

Laurent Pinchart (2):
  media: imx: imx7-media-csi: Fix frame sizes enumeration
  media: imx: imx7-media-csi: Include headers explicitly

 drivers/media/platform/nxp/imx7-media-csi.c | 48 +++++++++++++++------
 1 file changed, 34 insertions(+), 14 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH v3 1/3] media: imx: imx7-media-csi: Fix applying format constraints

[Laurent Pinchart]

From: Fabio Estevam <festevam@denx.de>

v4l_bound_align_image() aligns to a multiple of 2 to the power of
walign, not to walign. Depending on the pixel format, this causes the
image width to be aligned to 16 or 256 pixels instead of 4 or 8 as
required by the hardware. Fix it by rounding and clamping the width and
height manually.

Reported-by: Tim Harvey <tharvey@gateworks.com>
Closes: https://lore.kernel.org/linux-media/CAJ+vNU0BOVLTL17ofgHwtexbpuMYwH_aGUC==EXABUtHHiv_ag@mail.gmail.com
Fixes: 6f482c4729d9 ("media: imx: imx7-media-csi: Get rid of superfluous call to imx7_csi_mbus_fmt_to_pix_fmt")
Co-developed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
Signed-off-by: Fabio Estevam <festevam@denx.de>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v2:

- Don't export clamp_roundup() from v4l2-common.c
- Simply clamp the height as no alignment is needed
- Add required includes
---
 drivers/media/platform/nxp/imx7-media-csi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
index 2ec1f3cd56a0..5684ecd2e3fe 100644
--- a/drivers/media/platform/nxp/imx7-media-csi.c
+++ b/drivers/media/platform/nxp/imx7-media-csi.c
@@ -9,7 +9,9 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
+#include <linux/math.h>
 #include <linux/mfd/syscon.h>
+#include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_graph.h>
@@ -1137,8 +1139,9 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format *pixfmt,
 	 * TODO: Implement configurable stride support.
 	 */
 	walign = 8 * 8 / cc->bpp;
-	v4l_bound_align_image(&pixfmt->width, 1, 0xffff, walign,
-			      &pixfmt->height, 1, 0xffff, 1, 0);
+	pixfmt->width = clamp(round_up(pixfmt->width, walign), walign,
+			      round_down(65535U, walign));
+	pixfmt->height = clamp(pixfmt->height, 1U, 65535U);
 
 	pixfmt->bytesperline = pixfmt->width * cc->bpp / 8;
 	pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height;
-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/3] media: imx: imx7-media-csi: Fix applying format constraints

[Alexander Stein]

Hi Laurent,

thanks for picking this up.

Am Dienstag, 25. Juli 2023, 22:02:47 CEST schrieb Laurent Pinchart:
> From: Fabio Estevam <festevam@denx.de>
> 
> v4l_bound_align_image() aligns to a multiple of 2 to the power of
> walign, not to walign. Depending on the pixel format, this causes the
> image width to be aligned to 16 or 256 pixels instead of 4 or 8 as
> required by the hardware. Fix it by rounding and clamping the width and
> height manually.
> 
> Reported-by: Tim Harvey <tharvey@gateworks.com>
> Closes:
> https://lore.kernel.org/linux-media/CAJ+vNU0BOVLTL17ofgHwtexbpuMYwH_aGUC==E
> XABUtHHiv_ag@mail.gmail.com Fixes: 6f482c4729d9 ("media: imx:
> imx7-media-csi: Get rid of superfluous call to
> imx7_csi_mbus_fmt_to_pix_fmt") Co-developed-by: Alexander Stein
> <alexander.stein@ew.tq-group.com> Signed-off-by: Alexander Stein
> <alexander.stein@ew.tq-group.com>
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v2:
> 
> - Don't export clamp_roundup() from v4l2-common.c
> - Simply clamp the height as no alignment is needed
> - Add required includes
> ---
>  drivers/media/platform/nxp/imx7-media-csi.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx7-media-csi.c
> b/drivers/media/platform/nxp/imx7-media-csi.c index
> 2ec1f3cd56a0..5684ecd2e3fe 100644
> --- a/drivers/media/platform/nxp/imx7-media-csi.c
> +++ b/drivers/media/platform/nxp/imx7-media-csi.c
> @@ -9,7 +9,9 @@
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/interrupt.h>
> +#include <linux/math.h>
>  #include <linux/mfd/syscon.h>
> +#include <linux/minmax.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_graph.h>
> @@ -1137,8 +1139,9 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format
> *pixfmt, * TODO: Implement configurable stride support.
>  	 */
>  	walign = 8 * 8 / cc->bpp;
> -	v4l_bound_align_image(&pixfmt->width, 1, 0xffff, walign,
> -			      &pixfmt->height, 1, 0xffff, 1, 0);
> +	pixfmt->width = clamp(round_up(pixfmt->width, walign), walign,
> +			      round_down(65535U, walign));
> +	pixfmt->height = clamp(pixfmt->height, 1U, 65535U);

Actually I have a slight preference for 0xffff over 65535, just because it is 
indicating this is some (maximum) register value. But it may just be me.
I'm okay either way:
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>

Thanks
Alexander

> 
>  	pixfmt->bytesperline = pixfmt->width * cc->bpp / 8;
>  	pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height;


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

[PATCH v3 2/3] media: imx: imx7-media-csi: Fix frame sizes enumeration

[Laurent Pinchart]

Enumeration of the minimum, maximum and step values for the image width
does not take hardware constraints into account. Fix it.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/nxp/imx7-media-csi.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
index 5684ecd2e3fe..3aa7978d3f8a 100644
--- a/drivers/media/platform/nxp/imx7-media-csi.c
+++ b/drivers/media/platform/nxp/imx7-media-csi.c
@@ -1076,6 +1076,7 @@ static int imx7_csi_video_enum_framesizes(struct file *file, void *fh,
 					  struct v4l2_frmsizeenum *fsize)
 {
 	const struct imx7_csi_pixfmt *cc;
+	u32 walign;
 
 	if (fsize->index > 0)
 		return -EINVAL;
@@ -1085,16 +1086,17 @@ static int imx7_csi_video_enum_framesizes(struct file *file, void *fh,
 		return -EINVAL;
 
 	/*
-	 * TODO: The constraints are hardware-specific and may depend on the
-	 * pixel format. This should come from the driver using
-	 * imx_media_capture.
+	 * The width alignment is 8 bytes as indicated by the
+	 * CSI_IMAG_PARA.IMAGE_WIDTH documentation. Convert it to pixels.
 	 */
+	walign = 8 * 8 / cc->bpp;
+
 	fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS;
-	fsize->stepwise.min_width = 1;
-	fsize->stepwise.max_width = 65535;
+	fsize->stepwise.min_width = walign;
+	fsize->stepwise.max_width = round_down(65535U, walign);
 	fsize->stepwise.min_height = 1;
 	fsize->stepwise.max_height = 65535;
-	fsize->stepwise.step_width = 1;
+	fsize->stepwise.step_width = walign;
 	fsize->stepwise.step_height = 1;
 
 	return 0;
-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 2/3] media: imx: imx7-media-csi: Fix frame sizes enumeration

[Alexander Stein]

Hi Laurent,

thanks for picking up my suggestion.

Am Dienstag, 25. Juli 2023, 22:02:48 CEST schrieb Laurent Pinchart:
> Enumeration of the minimum, maximum and step values for the image width
> does not take hardware constraints into account. Fix it.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/platform/nxp/imx7-media-csi.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx7-media-csi.c
> b/drivers/media/platform/nxp/imx7-media-csi.c index
> 5684ecd2e3fe..3aa7978d3f8a 100644
> --- a/drivers/media/platform/nxp/imx7-media-csi.c
> +++ b/drivers/media/platform/nxp/imx7-media-csi.c
> @@ -1076,6 +1076,7 @@ static int imx7_csi_video_enum_framesizes(struct file
> *file, void *fh, struct v4l2_frmsizeenum *fsize)
>  {
>  	const struct imx7_csi_pixfmt *cc;
> +	u32 walign;
> 
>  	if (fsize->index > 0)
>  		return -EINVAL;
> @@ -1085,16 +1086,17 @@ static int imx7_csi_video_enum_framesizes(struct
> file *file, void *fh, return -EINVAL;
> 
>  	/*
> -	 * TODO: The constraints are hardware-specific and may depend on the
> -	 * pixel format. This should come from the driver using
> -	 * imx_media_capture.
> +	 * The width alignment is 8 bytes as indicated by the
> +	 * CSI_IMAG_PARA.IMAGE_WIDTH documentation. Convert it to pixels.
>  	 */
> +	walign = 8 * 8 / cc->bpp;
> +
>  	fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS;
> -	fsize->stepwise.min_width = 1;
> -	fsize->stepwise.max_width = 65535;
> +	fsize->stepwise.min_width = walign;
> +	fsize->stepwise.max_width = round_down(65535U, walign);
>  	fsize->stepwise.min_height = 1;
>  	fsize->stepwise.max_height = 65535;
> -	fsize->stepwise.step_width = 1;
> +	fsize->stepwise.step_width = walign;
>  	fsize->stepwise.step_height = 1;
> 
>  	return 0;

Looks good to me.
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

[PATCH v3 3/3] media: imx: imx7-media-csi: Include headers explicitly

[Laurent Pinchart]

Include all the headers that the driver needs explicitly instead of
relying on indirect inclusion. While at it, drop a few unneeded headers.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/nxp/imx7-media-csi.c | 27 ++++++++++++++++-----
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
index 3aa7978d3f8a..95e9f22f21be 100644
--- a/drivers/media/platform/nxp/imx7-media-csi.c
+++ b/drivers/media/platform/nxp/imx7-media-csi.c
@@ -3,31 +3,46 @@
  * V4L2 Capture CSI Subdev for Freescale i.MX6UL/L / i.MX7 SOC
  *
  * Copyright (c) 2019 Linaro Ltd
- *
  */
 
 #include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/container_of.h>
 #include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/err.h>
 #include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
 #include <linux/math.h>
-#include <linux/mfd/syscon.h>
 #include <linux/minmax.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
-#include <linux/of_graph.h>
-#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
-#include <linux/regmap.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/string.h>
+#include <linux/timekeeping.h>
 #include <linux/types.h>
 
+#include <media/media-device.h>
+#include <media/media-entity.h>
+#include <media/v4l2-async.h>
+#include <media/v4l2-common.h>
+#include <media/v4l2-dev.h>
 #include <media/v4l2-device.h>
-#include <media/v4l2-fwnode.h>
+#include <media/v4l2-fh.h>
 #include <media/v4l2-ioctl.h>
 #include <media/v4l2-mc.h>
 #include <media/v4l2-subdev.h>
+#include <media/videobuf2-core.h>
 #include <media/videobuf2-dma-contig.h>
+#include <media/videobuf2-v4l2.h>
 
 #define IMX7_CSI_PAD_SINK		0
 #define IMX7_CSI_PAD_SRC		1
-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 3/3] media: imx: imx7-media-csi: Include headers explicitly

[Alexander Stein]

Hi Laurent,

thanks for the additional cleanup.

Am Dienstag, 25. Juli 2023, 22:02:49 CEST schrieb Laurent Pinchart:
> Include all the headers that the driver needs explicitly instead of
> relying on indirect inclusion. While at it, drop a few unneeded headers.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/platform/nxp/imx7-media-csi.c | 27 ++++++++++++++++-----
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx7-media-csi.c
> b/drivers/media/platform/nxp/imx7-media-csi.c index
> 3aa7978d3f8a..95e9f22f21be 100644
> --- a/drivers/media/platform/nxp/imx7-media-csi.c
> +++ b/drivers/media/platform/nxp/imx7-media-csi.c
> @@ -3,31 +3,46 @@
>   * V4L2 Capture CSI Subdev for Freescale i.MX6UL/L / i.MX7 SOC
>   *
>   * Copyright (c) 2019 Linaro Ltd
> - *
>   */
> 
>  #include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/container_of.h>
>  #include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/err.h>
>  #include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/jiffies.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
>  #include <linux/math.h>
> -#include <linux/mfd/syscon.h>
>  #include <linux/minmax.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/of.h>
> -#include <linux/of_graph.h>
> -#include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
> -#include <linux/regmap.h>
> +#include <linux/property.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
> +#include <linux/string.h>
> +#include <linux/timekeeping.h>
>  #include <linux/types.h>
> 
> +#include <media/media-device.h>
> +#include <media/media-entity.h>
> +#include <media/v4l2-async.h>
> +#include <media/v4l2-common.h>
> +#include <media/v4l2-dev.h>
>  #include <media/v4l2-device.h>
> -#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-fh.h>
>  #include <media/v4l2-ioctl.h>
>  #include <media/v4l2-mc.h>
>  #include <media/v4l2-subdev.h>
> +#include <media/videobuf2-core.h>
>  #include <media/videobuf2-dma-contig.h>
> +#include <media/videobuf2-v4l2.h>
> 
>  #define IMX7_CSI_PAD_SINK		0
>  #define IMX7_CSI_PAD_SRC		1

Looks good to me.
Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v3 0/3] media: imx: imx7-media-csi: Fix width alignment

[Tim Harvey]

On Tue, Jul 25, 2023 at 1:02 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hello,
>
> This patch series is an attempt to fix the issue reported by Tim Harvey
> in [1].
>
> Both Alexander Stein and Fabio Estevam gave this a try in [2] and [3]
> respectively, with Alexander's self-nacking his patches and Fabio's
> solution receiving requests for changes during review.
>
> Compared to those attempts, this version implements a simpler fix for
> the issue (in patch 1/3), before addressing a TODO item (patch 2/3) and
> cleaning up includes as a drive-by improvement (patch 3/3).
>
> The series doesn't need to be merged in one go. Patch 1/3 can get merged
> as a fix for v6.5, while patches 2/3 and 3/3 can wait until v6.6.
>
> Tim, would you be able to test this ?
>
> [1] https://lore.kernel.org/linux-media/CAJ+vNU0BOVLTL17ofgHwtexbpuMYwH_aGUC==EXABUtHHiv_ag@mail.gmail.com/
> [2] https://lore.kernel.org/linux-media/20230720074129.3680269-1-alexander.stein@ew.tq-group.com
> [3] https://lore.kernel.org/linux-media/20230720222543.1740198-1-festevam@gmail.com
>
> Fabio Estevam (1):
>   media: imx: imx7-media-csi: Fix applying format constraints
>
> Laurent Pinchart (2):
>   media: imx: imx7-media-csi: Fix frame sizes enumeration
>   media: imx: imx7-media-csi: Include headers explicitly
>
>  drivers/media/platform/nxp/imx7-media-csi.c | 48 +++++++++++++++------
>  1 file changed, 34 insertions(+), 14 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart
>

Laurent,

Thanks for working on this. This series does not apply directly on
v6.5-rc6 which is where the regression is that 'media: imx:
imx7-media-csi: Fix applying format constraints' fixes. Regardless
I've manually applied the series on top of v5.6-rc6 and it does
resolve the regression. Note that Fabio's v2 fix applies fine on top
of v6.5-rc6 and resolves the regression in v6.5.

Tested-by: Tim Harvey <tharvey@gateworks.com> # imx8mm-gw72xx-0x with
imx219 image sensor

I'm not clear how to go about getting the first patch in the series
properly applied to v5.6 to resolve the regression.

Best regards,

Tim