[PATCH v3 0/4] Fix imx7-media-csi format settings

[PATCH v3 0/4] Fix imx7-media-csi format settings

[Alexander Stein]

Hi,

this v3 incorporates feedback and includes a new patch, provided by
Laurent Pinchart.

Thanks,
Alexander

Alexander Stein (3):
  media: imx: imx7-media-csi: Get rid of superfluous call to
    imx7_csi_mbus_fmt_to_pix_fmt
  media: imx: imx7-media-csi: Remove interlave fields
  media: imx: imx7-media-csi: Lift width constraints for 8bpp formats

Laurent Pinchart (1):
  media: imx: imx7-media-csi: Init default format with
    __imx7_csi_video_try_fmt()

 drivers/media/platform/nxp/imx7-media-csi.c | 97 ++++++---------------
 1 file changed, 28 insertions(+), 69 deletions(-)

-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 1/4] media: imx: imx7-media-csi: Get rid of superfluous call to imx7_csi_mbus_fmt_to_pix_fmt

[Alexander Stein]

There is no need to convert input pixformat to mbus_framefmt and back
again. Instead apply pixformat width constrains directly.
Assign compose values before adjusting pixformat height/width.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes in v3:
* Move compose assignment before width adjustments
* Add comments regarding width multiples
* Remove unneeded stride rounding

 drivers/media/platform/nxp/imx7-media-csi.c | 22 ++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
index b701e823436a8..b149374b07ee1 100644
--- a/drivers/media/platform/nxp/imx7-media-csi.c
+++ b/drivers/media/platform/nxp/imx7-media-csi.c
@@ -1145,9 +1145,13 @@ static const struct imx7_csi_pixfmt *
 __imx7_csi_video_try_fmt(struct v4l2_pix_format *pixfmt,
 			 struct v4l2_rect *compose)
 {
-	struct v4l2_mbus_framefmt fmt_src;
 	const struct imx7_csi_pixfmt *cc;
 
+	if (compose) {
+		compose->width = pixfmt->width;
+		compose->height = pixfmt->height;
+	}
+
 	/*
 	 * Find the pixel format, default to the first supported format if not
 	 * found.
@@ -1172,13 +1176,17 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format *pixfmt,
 		}
 	}
 
-	v4l2_fill_mbus_format(&fmt_src, pixfmt, 0);
-	imx7_csi_mbus_fmt_to_pix_fmt(pixfmt, &fmt_src, cc);
+	/*
+	 * Round up width for minimum burst size.
+	 *
+	 * TODO: Implement configurable stride support, and check what the real
+	 * hardware alignment constraint on the width is.
+	 */
+	v4l_bound_align_image(&pixfmt->width, 1, 0xffff, 8,
+			      &pixfmt->height, 1, 0xffff, 1, 0);
 
-	if (compose) {
-		compose->width = fmt_src.width;
-		compose->height = fmt_src.height;
-	}
+	pixfmt->bytesperline = pixfmt->width * cc->bpp / 8;
+	pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height;
 
 	return cc;
 }
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 2/4] media: imx: imx7-media-csi: Remove interlave fields

[Alexander Stein]

Interlaced mode is currently not supported, so disable fields in try_fmt.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes in v3:
* Remove left-over interlace mode check

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

diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
index b149374b07ee1..1315f5743b76f 100644
--- a/drivers/media/platform/nxp/imx7-media-csi.c
+++ b/drivers/media/platform/nxp/imx7-media-csi.c
@@ -1162,20 +1162,6 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format *pixfmt,
 		cc = imx7_csi_find_pixel_format(pixfmt->pixelformat);
 	}
 
-	/* Allow IDMAC interweave but enforce field order from source. */
-	if (V4L2_FIELD_IS_INTERLACED(pixfmt->field)) {
-		switch (pixfmt->field) {
-		case V4L2_FIELD_SEQ_TB:
-			pixfmt->field = V4L2_FIELD_INTERLACED_TB;
-			break;
-		case V4L2_FIELD_SEQ_BT:
-			pixfmt->field = V4L2_FIELD_INTERLACED_BT;
-			break;
-		default:
-			break;
-		}
-	}
-
 	/*
 	 * Round up width for minimum burst size.
 	 *
@@ -1187,6 +1173,7 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format *pixfmt,
 
 	pixfmt->bytesperline = pixfmt->width * cc->bpp / 8;
 	pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height;
+	pixfmt->field = V4L2_FIELD_NONE;
 
 	return cc;
 }
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 3/4] media: imx: imx7-media-csi: Lift width constraints for 8bpp formats

[Alexander Stein]

For 8-bit formats the image_width just needs to be a multiple of 8 pixels
others just a multiple of 4 pixels.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes in v3:
* Fix commit message (Only 8-bit formats needs multiple of 8 pixels)

 drivers/media/platform/nxp/imx7-media-csi.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
index 1315f5743b76f..730c9c57bf4bc 100644
--- a/drivers/media/platform/nxp/imx7-media-csi.c
+++ b/drivers/media/platform/nxp/imx7-media-csi.c
@@ -1146,6 +1146,7 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format *pixfmt,
 			 struct v4l2_rect *compose)
 {
 	const struct imx7_csi_pixfmt *cc;
+	u32 walign;
 
 	if (compose) {
 		compose->width = pixfmt->width;
@@ -1162,13 +1163,19 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format *pixfmt,
 		cc = imx7_csi_find_pixel_format(pixfmt->pixelformat);
 	}
 
+	/* Refer to CSI_IMAG_PARA.IMAGE_WIDTH description */
+	if (cc->bpp == 8)
+		walign = 8;
+	else
+		walign = 4;
+
 	/*
 	 * Round up width for minimum burst size.
 	 *
 	 * TODO: Implement configurable stride support, and check what the real
 	 * hardware alignment constraint on the width is.
 	 */
-	v4l_bound_align_image(&pixfmt->width, 1, 0xffff, 8,
+	v4l_bound_align_image(&pixfmt->width, 1, 0xffff, walign,
 			      &pixfmt->height, 1, 0xffff, 1, 0);
 
 	pixfmt->bytesperline = pixfmt->width * cc->bpp / 8;
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 4/4] media: imx: imx7-media-csi: Init default format with __imx7_csi_video_try_fmt()

[Alexander Stein]

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Use the __imx7_csi_video_try_fmt() helper function to initialize the
default format at probe time. This improves consistency by using the
same code path for both default initialization and validation at
runtime, and allows dropping the now unused imx7_csi_find_pixel_format()
function.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes in v3:
* New in v3

 drivers/media/platform/nxp/imx7-media-csi.c | 55 +++------------------
 1 file changed, 6 insertions(+), 49 deletions(-)

diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
index 730c9c57bf4bc..b5c8c951eac49 100644
--- a/drivers/media/platform/nxp/imx7-media-csi.c
+++ b/drivers/media/platform/nxp/imx7-media-csi.c
@@ -1014,39 +1014,6 @@ static int imx7_csi_enum_mbus_formats(u32 *code, u32 index)
 	return -EINVAL;
 }
 
-static int imx7_csi_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
-					const struct v4l2_mbus_framefmt *mbus,
-					const struct imx7_csi_pixfmt *cc)
-{
-	u32 width;
-	u32 stride;
-
-	if (!cc) {
-		cc = imx7_csi_find_mbus_format(mbus->code);
-		if (!cc)
-			return -EINVAL;
-	}
-
-	/* Round up width for minimum burst size */
-	width = round_up(mbus->width, 8);
-
-	/* Round up stride for IDMAC line start address alignment */
-	stride = round_up((width * cc->bpp) >> 3, 8);
-
-	pix->width = width;
-	pix->height = mbus->height;
-	pix->pixelformat = cc->fourcc;
-	pix->colorspace = mbus->colorspace;
-	pix->xfer_func = mbus->xfer_func;
-	pix->ycbcr_enc = mbus->ycbcr_enc;
-	pix->quantization = mbus->quantization;
-	pix->field = mbus->field;
-	pix->bytesperline = stride;
-	pix->sizeimage = stride * pix->height;
-
-	return 0;
-}
-
 /* -----------------------------------------------------------------------------
  * Video Capture Device - IOCTLs
  */
@@ -1608,22 +1575,14 @@ static struct imx7_csi_vb2_buffer *imx7_csi_video_next_buf(struct imx7_csi *csi)
 	return buf;
 }
 
-static int imx7_csi_video_init_format(struct imx7_csi *csi)
+static void imx7_csi_video_init_format(struct imx7_csi *csi)
 {
-	struct v4l2_mbus_framefmt format = { };
-
-	format.code = IMX7_CSI_DEF_MBUS_CODE;
-	format.width = IMX7_CSI_DEF_PIX_WIDTH;
-	format.height = IMX7_CSI_DEF_PIX_HEIGHT;
-	format.field = V4L2_FIELD_NONE;
+	struct v4l2_pix_format *pixfmt = &csi->vdev_fmt;
 
-	imx7_csi_mbus_fmt_to_pix_fmt(&csi->vdev_fmt, &format, NULL);
-	csi->vdev_compose.width = format.width;
-	csi->vdev_compose.height = format.height;
+	pixfmt->width = IMX7_CSI_DEF_PIX_WIDTH;
+	pixfmt->height = IMX7_CSI_DEF_PIX_HEIGHT;
 
-	csi->vdev_cc = imx7_csi_find_pixel_format(csi->vdev_fmt.pixelformat);
-
-	return 0;
+	csi->vdev_cc = __imx7_csi_video_try_fmt(pixfmt, &csi->vdev_compose);
 }
 
 static int imx7_csi_video_register(struct imx7_csi *csi)
@@ -1636,9 +1595,7 @@ static int imx7_csi_video_register(struct imx7_csi *csi)
 	vdev->v4l2_dev = v4l2_dev;
 
 	/* Initialize the default format and compose rectangle. */
-	ret = imx7_csi_video_init_format(csi);
-	if (ret < 0)
-		return ret;
+	imx7_csi_video_init_format(csi);
 
 	/* Register the video device. */
 	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/4] media: imx: imx7-media-csi: Get rid of superfluous call to imx7_csi_mbus_fmt_to_pix_fmt

[Laurent Pinchart]

Hi Alexander,

Thank you for the patch.

On Tue, Apr 18, 2023 at 02:20:38PM +0200, Alexander Stein wrote:
> There is no need to convert input pixformat to mbus_framefmt and back
> again. Instead apply pixformat width constrains directly.
> Assign compose values before adjusting pixformat height/width.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> Changes in v3:
> * Move compose assignment before width adjustments
> * Add comments regarding width multiples
> * Remove unneeded stride rounding
> 
>  drivers/media/platform/nxp/imx7-media-csi.c | 22 ++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
> index b701e823436a8..b149374b07ee1 100644
> --- a/drivers/media/platform/nxp/imx7-media-csi.c
> +++ b/drivers/media/platform/nxp/imx7-media-csi.c
> @@ -1145,9 +1145,13 @@ static const struct imx7_csi_pixfmt *
>  __imx7_csi_video_try_fmt(struct v4l2_pix_format *pixfmt,
>  			 struct v4l2_rect *compose)
>  {
> -	struct v4l2_mbus_framefmt fmt_src;
>  	const struct imx7_csi_pixfmt *cc;
>  
> +	if (compose) {
> +		compose->width = pixfmt->width;
> +		compose->height = pixfmt->height;
> +	}
> +
>  	/*
>  	 * Find the pixel format, default to the first supported format if not
>  	 * found.
> @@ -1172,13 +1176,17 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format *pixfmt,
>  		}
>  	}
>  
> -	v4l2_fill_mbus_format(&fmt_src, pixfmt, 0);
> -	imx7_csi_mbus_fmt_to_pix_fmt(pixfmt, &fmt_src, cc);
> +	/*
> +	 * Round up width for minimum burst size.
> +	 *
> +	 * TODO: Implement configurable stride support, and check what the real
> +	 * hardware alignment constraint on the width is.
> +	 */
> +	v4l_bound_align_image(&pixfmt->width, 1, 0xffff, 8,
> +			      &pixfmt->height, 1, 0xffff, 1, 0);
>  
> -	if (compose) {
> -		compose->width = fmt_src.width;
> -		compose->height = fmt_src.height;
> -	}
> +	pixfmt->bytesperline = pixfmt->width * cc->bpp / 8;
> +	pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height;
>  
>  	return cc;
>  }

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/4] media: imx: imx7-media-csi: Remove interlave fields

[Laurent Pinchart]

Hi Alexander,

Thank you for the patch.

In the subject line, "interlave" is misspelled. I'd write "Remove
incorrect interlacing support"

On Tue, Apr 18, 2023 at 02:20:39PM +0200, Alexander Stein wrote:
> Interlaced mode is currently not supported, so disable fields in try_fmt.

And here,

The driver doesn't currently support interlacing, but due to legacy
leftovers, it accepts values for the pixel format "field" field other
than V4L2_FIELD_NONE. Fix it by hardcoding V4L2_FIELD_NONE. Proper
interlacing support can be implemented later if desired.

With this,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I can apply those changes directly to my tree if you would prefer
avoiding a v4.

> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> Changes in v3:
> * Remove left-over interlace mode check
> 
>  drivers/media/platform/nxp/imx7-media-csi.c | 15 +--------------
>  1 file changed, 1 insertion(+), 14 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
> index b149374b07ee1..1315f5743b76f 100644
> --- a/drivers/media/platform/nxp/imx7-media-csi.c
> +++ b/drivers/media/platform/nxp/imx7-media-csi.c
> @@ -1162,20 +1162,6 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format *pixfmt,
>  		cc = imx7_csi_find_pixel_format(pixfmt->pixelformat);
>  	}
>  
> -	/* Allow IDMAC interweave but enforce field order from source. */
> -	if (V4L2_FIELD_IS_INTERLACED(pixfmt->field)) {
> -		switch (pixfmt->field) {
> -		case V4L2_FIELD_SEQ_TB:
> -			pixfmt->field = V4L2_FIELD_INTERLACED_TB;
> -			break;
> -		case V4L2_FIELD_SEQ_BT:
> -			pixfmt->field = V4L2_FIELD_INTERLACED_BT;
> -			break;
> -		default:
> -			break;
> -		}
> -	}
> -
>  	/*
>  	 * Round up width for minimum burst size.
>  	 *
> @@ -1187,6 +1173,7 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format *pixfmt,
>  
>  	pixfmt->bytesperline = pixfmt->width * cc->bpp / 8;
>  	pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height;
> +	pixfmt->field = V4L2_FIELD_NONE;
>  
>  	return cc;
>  }

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/4] media: imx: imx7-media-csi: Lift width constraints for 8bpp formats

[Laurent Pinchart]

Hi Alexander,

Thank you for the patch.

The commit message should state "Lift width constraint for 16bpp
formats". I would also phrase is "Relax" instead of "Lift" as it's not
completely lifted.

On Tue, Apr 18, 2023 at 02:20:40PM +0200, Alexander Stein wrote:
> For 8-bit formats the image_width just needs to be a multiple of 8 pixels
> others just a multiple of 4 pixels.

This is a bit terse, and I think a word or two are missing. It could be
improved:

The driver unconditionally aligns the image width to multiples of 8
pixels. The real alignment constraint is 8 bytes, as indicated by the
CSI_IMAG_PARA.IMAGE_WIDTH documentation that calls for 8 pixel alignment
for 8bpp formats and 4 pixel alignment for other formats.

> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> Changes in v3:
> * Fix commit message (Only 8-bit formats needs multiple of 8 pixels)
> 
>  drivers/media/platform/nxp/imx7-media-csi.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
> index 1315f5743b76f..730c9c57bf4bc 100644
> --- a/drivers/media/platform/nxp/imx7-media-csi.c
> +++ b/drivers/media/platform/nxp/imx7-media-csi.c
> @@ -1146,6 +1146,7 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format *pixfmt,
>  			 struct v4l2_rect *compose)
>  {
>  	const struct imx7_csi_pixfmt *cc;
> +	u32 walign;
>  
>  	if (compose) {
>  		compose->width = pixfmt->width;
> @@ -1162,13 +1163,19 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format *pixfmt,
>  		cc = imx7_csi_find_pixel_format(pixfmt->pixelformat);
>  	}
>  
> +	/* Refer to CSI_IMAG_PARA.IMAGE_WIDTH description */
> +	if (cc->bpp == 8)
> +		walign = 8;
> +	else
> +		walign = 4;

Would the following convey the purpose better ?

	/*
	 * 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;

> +
>  	/*
>  	 * Round up width for minimum burst size.
>  	 *
>  	 * TODO: Implement configurable stride support, and check what the real
>  	 * hardware alignment constraint on the width is.
>  	 */

We can now drop the second part of the sentence :-) The first line is
actually not very accurate anymore. How about

	/*
	 * The width alignment is 8 bytes as indicated by the
	 * CSI_IMAG_PARA.IMAGE_WIDTH documentation. Convert it to pixels.
	 *
  	 * 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);

> -	v4l_bound_align_image(&pixfmt->width, 1, 0xffff, 8,
> +	v4l_bound_align_image(&pixfmt->width, 1, 0xffff, walign,
>  			      &pixfmt->height, 1, 0xffff, 1, 0);
>  
>  	pixfmt->bytesperline = pixfmt->width * cc->bpp / 8;

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/4] media: imx: imx7-media-csi: Lift width constraints for 8bpp formats

[Alexander Stein]

Hi Laurent,

thanks for the feedback.

Am Dienstag, 18. April 2023, 17:59:47 CEST schrieb Laurent Pinchart:
> Hi Alexander,
> 
> Thank you for the patch.
> 
> The commit message should state "Lift width constraint for 16bpp
> formats".

To be pedantic it should be called "Lift width constraint for non-8bpp 
formats" :)

> I would also phrase is "Relax" instead of "Lift" as it's not
> completely lifted.

That's true, this subtle difference should be accounted for. Thanks.

> On Tue, Apr 18, 2023 at 02:20:40PM +0200, Alexander Stein wrote:
> > For 8-bit formats the image_width just needs to be a multiple of 8 pixels
> > others just a multiple of 4 pixels.
> 
> This is a bit terse, and I think a word or two are missing. It could be
> improved:
> 
> The driver unconditionally aligns the image width to multiples of 8
> pixels. The real alignment constraint is 8 bytes, as indicated by the
> CSI_IMAG_PARA.IMAGE_WIDTH documentation that calls for 8 pixel alignment
> for 8bpp formats and 4 pixel alignment for other formats.

Thanks for this suggestion. This sounds much better.

> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > Changes in v3:
> > * Fix commit message (Only 8-bit formats needs multiple of 8 pixels)
> > 
> >  drivers/media/platform/nxp/imx7-media-csi.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/platform/nxp/imx7-media-csi.c
> > b/drivers/media/platform/nxp/imx7-media-csi.c index
> > 1315f5743b76f..730c9c57bf4bc 100644
> > --- a/drivers/media/platform/nxp/imx7-media-csi.c
> > +++ b/drivers/media/platform/nxp/imx7-media-csi.c
> > @@ -1146,6 +1146,7 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format
> > *pixfmt,> 
> >  			 struct v4l2_rect *compose)
> >  
> >  {
> >  
> >  	const struct imx7_csi_pixfmt *cc;
> > 
> > +	u32 walign;
> > 
> >  	if (compose) {
> >  	
> >  		compose->width = pixfmt->width;
> > 
> > @@ -1162,13 +1163,19 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format
> > *pixfmt,> 
> >  		cc = imx7_csi_find_pixel_format(pixfmt->pixelformat);
> >  	
> >  	}
> > 
> > +	/* Refer to CSI_IMAG_PARA.IMAGE_WIDTH description */
> > +	if (cc->bpp == 8)
> > +		walign = 8;
> > +	else
> > +		walign = 4;
> 
> Would the following convey the purpose better ?
> 
> 	/*
> 	 * 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;

I was wondering how to shorten this calculation without using ? operator, 
nice.

> > +
> > 
> >  	/*
> >  	
> >  	 * Round up width for minimum burst size.
> >  	 *
> >  	 * TODO: Implement configurable stride support, and check what the 
real
> >  	 * hardware alignment constraint on the width is.
> >  	 */
> 
> We can now drop the second part of the sentence :-) The first line is
> actually not very accurate anymore. How about
> 
> 	/*
> 	 * The width alignment is 8 bytes as indicated by the
> 	 * CSI_IMAG_PARA.IMAGE_WIDTH documentation. Convert it to pixels.
> 	 *
>   	 * 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);

That's neat, thanks. I'll update accordingly.

Best regards,
Alexander

> > -	v4l_bound_align_image(&pixfmt->width, 1, 0xffff, 8,
> > +	v4l_bound_align_image(&pixfmt->width, 1, 0xffff, walign,
> > 
> >  			      &pixfmt->height, 1, 0xffff, 1, 0);
> >  	
> >  	pixfmt->bytesperline = pixfmt->width * cc->bpp / 8;


-- 
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/



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/4] media: imx: imx7-media-csi: Remove interlave fields

[Alexander Stein]

Hi Laurent,

Am Dienstag, 18. April 2023, 17:43:51 CEST schrieb Laurent Pinchart:
> Hi Alexander,
> 
> Thank you for the patch.
> 
> In the subject line, "interlave" is misspelled. I'd write "Remove
> incorrect interlacing support"

Sounds much better, thanks. I'll change that.

> On Tue, Apr 18, 2023 at 02:20:39PM +0200, Alexander Stein wrote:
> > Interlaced mode is currently not supported, so disable fields in try_fmt.
> 
> And here,
> 
> The driver doesn't currently support interlacing, but due to legacy
> leftovers, it accepts values for the pixel format "field" field other
> than V4L2_FIELD_NONE. Fix it by hardcoding V4L2_FIELD_NONE. Proper
> interlacing support can be implemented later if desired.
> 
> With this,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> I can apply those changes directly to my tree if you would prefer
> avoiding a v4.

Thanks, as Patch 3/4 need another round anyway. I'll update accordingly and 
resend.

Best regards,
Alexander

> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > Changes in v3:
> > * Remove left-over interlace mode check
> > 
> >  drivers/media/platform/nxp/imx7-media-csi.c | 15 +--------------
> >  1 file changed, 1 insertion(+), 14 deletions(-)
> > 
> > diff --git a/drivers/media/platform/nxp/imx7-media-csi.c
> > b/drivers/media/platform/nxp/imx7-media-csi.c index
> > b149374b07ee1..1315f5743b76f 100644
> > --- a/drivers/media/platform/nxp/imx7-media-csi.c
> > +++ b/drivers/media/platform/nxp/imx7-media-csi.c
> > @@ -1162,20 +1162,6 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format
> > *pixfmt,> 
> >  		cc = imx7_csi_find_pixel_format(pixfmt->pixelformat);
> >  	
> >  	}
> > 
> > -	/* Allow IDMAC interweave but enforce field order from source. */
> > -	if (V4L2_FIELD_IS_INTERLACED(pixfmt->field)) {
> > -		switch (pixfmt->field) {
> > -		case V4L2_FIELD_SEQ_TB:
> > -			pixfmt->field = V4L2_FIELD_INTERLACED_TB;
> > -			break;
> > -		case V4L2_FIELD_SEQ_BT:
> > -			pixfmt->field = V4L2_FIELD_INTERLACED_BT;
> > -			break;
> > -		default:
> > -			break;
> > -		}
> > -	}
> > -
> > 
> >  	/*
> >  	
> >  	 * Round up width for minimum burst size.
> >  	 *
> > 
> > @@ -1187,6 +1173,7 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format
> > *pixfmt,> 
> >  	pixfmt->bytesperline = pixfmt->width * cc->bpp / 8;
> >  	pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height;
> > 
> > +	pixfmt->field = V4L2_FIELD_NONE;
> > 
> >  	return cc;
> >  
> >  }


-- 
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/



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel