[PATCH v2 0/3] Fix imx7-media-csi format settings

[PATCH v2 0/3] Fix imx7-media-csi format settings

[Alexander Stein]

Hi all,

this v2 splits the previous patch ([1]) into dedicated patches.
Patch 1 removes the calls for converting the pix_format to mbus_framefmt and
back again while applying the hardware limits to width and height.
The bytesperline and sizeimage fields are set correctly as well.
Together with patch 2 v4l2-compliance errors are gone.

Best regards,
Alexander

[1] https://patchwork.linuxtv.org/project/linux-media/patch/20230406135637.257580-1-alexander.stein@ew.tq-group.com/

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

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

-- 
2.34.1

[PATCH v2 1/3] 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 contrains directly.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/media/platform/nxp/imx7-media-csi.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
index b701e823436a8..bd649fd9166fd 100644
--- a/drivers/media/platform/nxp/imx7-media-csi.c
+++ b/drivers/media/platform/nxp/imx7-media-csi.c
@@ -1145,8 +1145,8 @@ 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;
+	u32 stride;
 
 	/*
 	 * Find the pixel format, default to the first supported format if not
@@ -1172,12 +1172,16 @@ __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);
+	v4l_bound_align_image(&pixfmt->width, 1, 0xffff, 8,
+			      &pixfmt->height, 1, 0xffff, 1, 0);
+
+	stride = round_up((pixfmt->width * cc->bpp) / 8, 8);
+	pixfmt->bytesperline = stride;
+	pixfmt->sizeimage = stride * pixfmt->height;
 
 	if (compose) {
-		compose->width = fmt_src.width;
-		compose->height = fmt_src.height;
+		compose->width = pixfmt->width;
+		compose->height = pixfmt->height;
 	}
 
 	return cc;
-- 
2.34.1

Re: [PATCH v2 1/3] 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 09:14:37AM +0200, Alexander Stein wrote:
> There is no need to convert input pixformat to mbus_framefmt and back
> again. Instead apply pixformat width contrains directly.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
>  drivers/media/platform/nxp/imx7-media-csi.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
> index b701e823436a8..bd649fd9166fd 100644
> --- a/drivers/media/platform/nxp/imx7-media-csi.c
> +++ b/drivers/media/platform/nxp/imx7-media-csi.c
> @@ -1145,8 +1145,8 @@ 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;
> +	u32 stride;
>  
>  	/*
>  	 * Find the pixel format, default to the first supported format if not
> @@ -1172,12 +1172,16 @@ __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);

Could you please keep the comment from imx7_csi_mbus_fmt_to_pix_fmt()
here, to indicate where the alignment comes from ?

	/* Round up width for minimum burst size */

We should likely revisit this in the future, I don't think the alignment
is actually needed. This could be recorded already:

	/*
	 * 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);
> +
> +	stride = round_up((pixfmt->width * cc->bpp) / 8, 8);

You can drop the round_up(), as pixfmt->width is now a multiple of 8, so

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

> +	pixfmt->bytesperline = stride;
> +	pixfmt->sizeimage = stride * pixfmt->height;
>  
>  	if (compose) {
> -		compose->width = fmt_src.width;
> -		compose->height = fmt_src.height;
> +		compose->width = pixfmt->width;

This is a change of behaviour, compose->width used to be set to the
unaligned width.

> +		compose->height = pixfmt->height;
>  	}
>  
>  	return cc;

-- 
Regards,

Laurent Pinchart

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

[Alexander Stein]

Hi Laurent,

thanks or your feedback.

Am Dienstag, 18. April 2023, 11:27:13 CEST schrieb Laurent Pinchart:
> Hi Alexander,
> 
> Thank you for the patch.
> 
> On Tue, Apr 18, 2023 at 09:14:37AM +0200, Alexander Stein wrote:
> > There is no need to convert input pixformat to mbus_framefmt and back
> > again. Instead apply pixformat width contrains directly.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > 
> >  drivers/media/platform/nxp/imx7-media-csi.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/media/platform/nxp/imx7-media-csi.c
> > b/drivers/media/platform/nxp/imx7-media-csi.c index
> > b701e823436a8..bd649fd9166fd 100644
> > --- a/drivers/media/platform/nxp/imx7-media-csi.c
> > +++ b/drivers/media/platform/nxp/imx7-media-csi.c
> > @@ -1145,8 +1145,8 @@ 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;
> > 
> > +	u32 stride;
> > 
> >  	/*
> >  	
> >  	 * Find the pixel format, default to the first supported format if 
not
> > 
> > @@ -1172,12 +1172,16 @@ __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);
> 
> Could you please keep the comment from imx7_csi_mbus_fmt_to_pix_fmt()
> here, to indicate where the alignment comes from ?

Sure, why not.

> 	/* Round up width for minimum burst size */
> 
> We should likely revisit this in the future, I don't think the alignment
> is actually needed. This could be recorded already:
> 
> 	/*
> 	 * Round up width for minimum burst size.
> 	 *
> 	 * TODO: Implement configurable stride support, and check what the 
real
> 	 * hardware alignment constraint on the width is.
> 	 */

Sounds good. I was already suspecting actual stride support is not supported, 
as FBUF_PARA is set to 0 (in non-interlaced mode).

> > +	v4l_bound_align_image(&pixfmt->width, 1, 0xffff, 8,
> > +			      &pixfmt->height, 1, 0xffff, 1, 0);
> > +
> > +	stride = round_up((pixfmt->width * cc->bpp) / 8, 8);
> 
> You can drop the round_up(), as pixfmt->width is now a multiple of 8, so
> 
> 	pixfmt->bytesperline = pixfmt->width * cc->bpp / 8;

But that is only identical if cc->bpp == 8, or am I missing something here?

> > +	pixfmt->bytesperline = stride;
> > +	pixfmt->sizeimage = stride * pixfmt->height;
> > 
> >  	if (compose) {
> > 
> > -		compose->width = fmt_src.width;
> > -		compose->height = fmt_src.height;
> > +		compose->width = pixfmt->width;
> 
> This is a change of behaviour, compose->width used to be set to the
> unaligned width.

Oh, you are right. I'll fix that.

Best regards,
Alexander

> > +		compose->height = pixfmt->height;
> > 
> >  	}
> >  	
> >  	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/

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

[Laurent Pinchart]

Hi Alexander,

On Tue, Apr 18, 2023 at 12:00:43PM +0200, Alexander Stein wrote:
> Am Dienstag, 18. April 2023, 11:27:13 CEST schrieb Laurent Pinchart:
> > On Tue, Apr 18, 2023 at 09:14:37AM +0200, Alexander Stein wrote:
> > > There is no need to convert input pixformat to mbus_framefmt and back
> > > again. Instead apply pixformat width contrains directly.
> > > 
> > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > ---
> > > 
> > >  drivers/media/platform/nxp/imx7-media-csi.c | 14 +++++++++-----
> > >  1 file changed, 9 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/nxp/imx7-media-csi.c
> > > b/drivers/media/platform/nxp/imx7-media-csi.c index
> > > b701e823436a8..bd649fd9166fd 100644
> > > --- a/drivers/media/platform/nxp/imx7-media-csi.c
> > > +++ b/drivers/media/platform/nxp/imx7-media-csi.c
> > > @@ -1145,8 +1145,8 @@ 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;
> > > +	u32 stride;
> > > 
> > >  	/*
> > >  	 * Find the pixel format, default to the first supported format if not
> > > @@ -1172,12 +1172,16 @@ __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);
> > 
> > Could you please keep the comment from imx7_csi_mbus_fmt_to_pix_fmt()
> > here, to indicate where the alignment comes from ?
> 
> Sure, why not.
> 
> > 	/* Round up width for minimum burst size */
> > 
> > We should likely revisit this in the future, I don't think the alignment
> > is actually needed. This could be recorded already:
> > 
> > 	/*
> > 	 * Round up width for minimum burst size.
> > 	 *
> > 	 * TODO: Implement configurable stride support, and check what the real
> > 	 * hardware alignment constraint on the width is.
> > 	 */
> 
> Sounds good. I was already suspecting actual stride support is not supported, 
> as FBUF_PARA is set to 0 (in non-interlaced mode).
> 
> > > +	v4l_bound_align_image(&pixfmt->width, 1, 0xffff, 8,
> > > +			      &pixfmt->height, 1, 0xffff, 1, 0);
> > > +
> > > +	stride = round_up((pixfmt->width * cc->bpp) / 8, 8);
> > 
> > You can drop the round_up(), as pixfmt->width is now a multiple of 8, so
> > 
> > 	pixfmt->bytesperline = pixfmt->width * cc->bpp / 8;
> 
> But that is only identical if cc->bpp == 8, or am I missing something here?

If cc->bpp is equal to 16, you will get

 	pixfmt->bytesperline = pixfmt->width * 2;

which will still be a multiple of 8 as width is a multiple of 8.

> > > +	pixfmt->bytesperline = stride;
> > > +	pixfmt->sizeimage = stride * pixfmt->height;
> > > 
> > >  	if (compose) {
> > > 
> > > -		compose->width = fmt_src.width;
> > > -		compose->height = fmt_src.height;
> > > +		compose->width = pixfmt->width;
> > 
> > This is a change of behaviour, compose->width used to be set to the
> > unaligned width.
> 
> Oh, you are right. I'll fix that.
> 
> > > +		compose->height = pixfmt->height;
> > > 
> > >  	}
> > >  	
> > >  	return cc;

-- 
Regards,

Laurent Pinchart

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

[Alexander Stein]

Hi Laurent,

Am Dienstag, 18. April 2023, 12:05:55 CEST schrieb Laurent Pinchart:
> Hi Alexander,
> 
> On Tue, Apr 18, 2023 at 12:00:43PM +0200, Alexander Stein wrote:
> > Am Dienstag, 18. April 2023, 11:27:13 CEST schrieb Laurent Pinchart:
> > > On Tue, Apr 18, 2023 at 09:14:37AM +0200, Alexander Stein wrote:
> > > > There is no need to convert input pixformat to mbus_framefmt and back
> > > > again. Instead apply pixformat width contrains directly.
> > > > 
> > > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > > ---
> > > > 
> > > >  drivers/media/platform/nxp/imx7-media-csi.c | 14 +++++++++-----
> > > >  1 file changed, 9 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/platform/nxp/imx7-media-csi.c
> > > > b/drivers/media/platform/nxp/imx7-media-csi.c index
> > > > b701e823436a8..bd649fd9166fd 100644
> > > > --- a/drivers/media/platform/nxp/imx7-media-csi.c
> > > > +++ b/drivers/media/platform/nxp/imx7-media-csi.c
> > > > @@ -1145,8 +1145,8 @@ 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;
> > > > 
> > > > +	u32 stride;
> > > > 
> > > >  	/*
> > > >  	
> > > >  	 * Find the pixel format, default to the first supported format if
> > > >  	 not
> > > > 
> > > > @@ -1172,12 +1172,16 @@ __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);
> > > 
> > > Could you please keep the comment from imx7_csi_mbus_fmt_to_pix_fmt()
> > > here, to indicate where the alignment comes from ?
> > 
> > Sure, why not.
> > 
> > > 	/* Round up width for minimum burst size */
> > > 
> > > We should likely revisit this in the future, I don't think the alignment
> > > 
> > > is actually needed. This could be recorded already:
> > > 	/*
> > > 	
> > > 	 * Round up width for minimum burst size.
> > > 	 *
> > > 	 * TODO: Implement configurable stride support, and check what the 
real
> > > 	 * hardware alignment constraint on the width is.
> > > 	 */
> > 
> > Sounds good. I was already suspecting actual stride support is not
> > supported, as FBUF_PARA is set to 0 (in non-interlaced mode).
> > 
> > > > +	v4l_bound_align_image(&pixfmt->width, 1, 0xffff, 8,
> > > > +			      &pixfmt->height, 1, 0xffff, 1, 0);
> > > > +
> > > > +	stride = round_up((pixfmt->width * cc->bpp) / 8, 8);
> > > 
> > > You can drop the round_up(), as pixfmt->width is now a multiple of 8, so
> > > 
> > > 	pixfmt->bytesperline = pixfmt->width * cc->bpp / 8;
> > 
> > But that is only identical if cc->bpp == 8, or am I missing something
> > here?
> 
> If cc->bpp is equal to 16, you will get
> 
>  	pixfmt->bytesperline = pixfmt->width * 2;
> 
> which will still be a multiple of 8 as width is a multiple of 8.

That's true only for 16 bpp. If you are using e.g. 'RG10' this assumption is 
not guaranteed.  Apparently it happens to be the case for 1080p on my imx327 
sensor though.

Best regards,
Alexander

> > > > +	pixfmt->bytesperline = stride;
> > > > +	pixfmt->sizeimage = stride * pixfmt->height;
> > > > 
> > > >  	if (compose) {
> > > > 
> > > > -		compose->width = fmt_src.width;
> > > > -		compose->height = fmt_src.height;
> > > > +		compose->width = pixfmt->width;
> > > 
> > > This is a change of behaviour, compose->width used to be set to the
> > > unaligned width.
> > 
> > Oh, you are right. I'll fix that.
> > 
> > > > +		compose->height = pixfmt->height;
> > > > 
> > > >  	}
> > > >  	
> > > >  	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/

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

[Laurent Pinchart]

Hi Alexander,

On Tue, Apr 18, 2023 at 12:08:07PM +0200, Alexander Stein wrote:
> Am Dienstag, 18. April 2023, 12:05:55 CEST schrieb Laurent Pinchart:
> > On Tue, Apr 18, 2023 at 12:00:43PM +0200, Alexander Stein wrote:
> > > Am Dienstag, 18. April 2023, 11:27:13 CEST schrieb Laurent Pinchart:
> > > > On Tue, Apr 18, 2023 at 09:14:37AM +0200, Alexander Stein wrote:
> > > > > There is no need to convert input pixformat to mbus_framefmt and back
> > > > > again. Instead apply pixformat width contrains directly.
> > > > > 
> > > > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > > > ---
> > > > > 
> > > > >  drivers/media/platform/nxp/imx7-media-csi.c | 14 +++++++++-----
> > > > >  1 file changed, 9 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/platform/nxp/imx7-media-csi.c
> > > > > b/drivers/media/platform/nxp/imx7-media-csi.c index
> > > > > b701e823436a8..bd649fd9166fd 100644
> > > > > --- a/drivers/media/platform/nxp/imx7-media-csi.c
> > > > > +++ b/drivers/media/platform/nxp/imx7-media-csi.c
> > > > > @@ -1145,8 +1145,8 @@ 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;
> > > > > +	u32 stride;
> > > > > 
> > > > >  	/*
> > > > >  	 * Find the pixel format, default to the first supported format if not
> > > > > @@ -1172,12 +1172,16 @@ __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);
> > > > 
> > > > Could you please keep the comment from imx7_csi_mbus_fmt_to_pix_fmt()
> > > > here, to indicate where the alignment comes from ?
> > > 
> > > Sure, why not.
> > > 
> > > > 	/* Round up width for minimum burst size */
> > > > 
> > > > We should likely revisit this in the future, I don't think the alignment
> > > > is actually needed. This could be recorded already:
> > > > 
> > > > 	/*
> > > > 	
> > > > 	 * Round up width for minimum burst size.
> > > > 	 *
> > > > 	 * TODO: Implement configurable stride support, and check what the real
> > > > 	 * hardware alignment constraint on the width is.
> > > > 	 */
> > > 
> > > Sounds good. I was already suspecting actual stride support is not
> > > supported, as FBUF_PARA is set to 0 (in non-interlaced mode).
> > > 
> > > > > +	v4l_bound_align_image(&pixfmt->width, 1, 0xffff, 8,
> > > > > +			      &pixfmt->height, 1, 0xffff, 1, 0);
> > > > > +
> > > > > +	stride = round_up((pixfmt->width * cc->bpp) / 8, 8);
> > > > 
> > > > You can drop the round_up(), as pixfmt->width is now a multiple of 8, so
> > > > 
> > > > 	pixfmt->bytesperline = pixfmt->width * cc->bpp / 8;
> > > 
> > > But that is only identical if cc->bpp == 8, or am I missing something
> > > here?
> > 
> > If cc->bpp is equal to 16, you will get
> > 
> >  	pixfmt->bytesperline = pixfmt->width * 2;
> > 
> > which will still be a multiple of 8 as width is a multiple of 8.
> 
> That's true only for 16 bpp. If you are using e.g. 'RG10' this assumption is 
> not guaranteed.  Apparently it happens to be the case for 1080p on my imx327 
> sensor though.

For 10-bit bayer formats, cc->bpp is 16. The only bpp values used by the
driver are 8 and 16.

> > > > > +	pixfmt->bytesperline = stride;
> > > > > +	pixfmt->sizeimage = stride * pixfmt->height;
> > > > > 
> > > > >  	if (compose) {
> > > > > 
> > > > > -		compose->width = fmt_src.width;
> > > > > -		compose->height = fmt_src.height;
> > > > > +		compose->width = pixfmt->width;
> > > > 
> > > > This is a change of behaviour, compose->width used to be set to the
> > > > unaligned width.
> > > 
> > > Oh, you are right. I'll fix that.
> > > 
> > > > > +		compose->height = pixfmt->height;
> > > > > 
> > > > >  	}
> > > > >  	
> > > > >  	return cc;

-- 
Regards,

Laurent Pinchart

[PATCH v2 2/3] 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>
---
 drivers/media/platform/nxp/imx7-media-csi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
index bd649fd9166fd..1508f6ba20e91 100644
--- a/drivers/media/platform/nxp/imx7-media-csi.c
+++ b/drivers/media/platform/nxp/imx7-media-csi.c
@@ -1178,6 +1178,7 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format *pixfmt,
 	stride = round_up((pixfmt->width * cc->bpp) / 8, 8);
 	pixfmt->bytesperline = stride;
 	pixfmt->sizeimage = stride * pixfmt->height;
+	pixfmt->field = V4L2_FIELD_NONE;
 
 	if (compose) {
 		compose->width = pixfmt->width;
-- 
2.34.1

Re: [PATCH v2 2/3] media: imx: imx7-media-csi: Remove interlave fields

[Laurent Pinchart]

Hi Alexander,

Thank you for the patch.

On Tue, Apr 18, 2023 at 09:14:38AM +0200, Alexander Stein wrote:
> Interlaced mode is currently not supported, so disable fields in try_fmt.
>
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
>  drivers/media/platform/nxp/imx7-media-csi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
> index bd649fd9166fd..1508f6ba20e91 100644
> --- a/drivers/media/platform/nxp/imx7-media-csi.c
> +++ b/drivers/media/platform/nxp/imx7-media-csi.c
> @@ -1178,6 +1178,7 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format *pixfmt,

You should also drop the following code in the same function:

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

>  	stride = round_up((pixfmt->width * cc->bpp) / 8, 8);
>  	pixfmt->bytesperline = stride;
>  	pixfmt->sizeimage = stride * pixfmt->height;
> +	pixfmt->field = V4L2_FIELD_NONE;
>  
>  	if (compose) {
>  		compose->width = pixfmt->width;

-- 
Regards,

Laurent Pinchart

[PATCH v2 3/3] 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 4 pixels.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 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 1508f6ba20e91..5240670476b2b 100644
--- a/drivers/media/platform/nxp/imx7-media-csi.c
+++ b/drivers/media/platform/nxp/imx7-media-csi.c
@@ -1147,6 +1147,7 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format *pixfmt,
 {
 	const struct imx7_csi_pixfmt *cc;
 	u32 stride;
+	u32 walign;
 
 	/*
 	 * Find the pixel format, default to the first supported format if not
@@ -1172,7 +1173,13 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format *pixfmt,
 		}
 	}
 
-	v4l_bound_align_image(&pixfmt->width, 1, 0xffff, 8,
+	/* Refer to CSI_IMAG_PARA.IMAGE_WIDTH description */
+	if (cc->bpp == 8)
+		walign = 8;
+	else
+		walign = 4;
+
+	v4l_bound_align_image(&pixfmt->width, 1, 0xffff, walign,
 			      &pixfmt->height, 1, 0xffff, 1, 0);
 
 	stride = round_up((pixfmt->width * cc->bpp) / 8, 8);
-- 
2.34.1

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

[Laurent Pinchart]

Hi Alexander,

Thank you for the patch.

On Tue, Apr 18, 2023 at 09:14:39AM +0200, Alexander Stein wrote:
> For 8-bit formats the image_width just needs to be a multiple of 4 pixels.

This doesn't match the code below. I think the code is right, the commit
message should be updated.

> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
>  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 1508f6ba20e91..5240670476b2b 100644
> --- a/drivers/media/platform/nxp/imx7-media-csi.c
> +++ b/drivers/media/platform/nxp/imx7-media-csi.c
> @@ -1147,6 +1147,7 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format *pixfmt,
>  {
>  	const struct imx7_csi_pixfmt *cc;
>  	u32 stride;
> +	u32 walign;
>  
>  	/*
>  	 * Find the pixel format, default to the first supported format if not
> @@ -1172,7 +1173,13 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format *pixfmt,
>  		}
>  	}
>  
> -	v4l_bound_align_image(&pixfmt->width, 1, 0xffff, 8,
> +	/* Refer to CSI_IMAG_PARA.IMAGE_WIDTH description */
> +	if (cc->bpp == 8)
> +		walign = 8;
> +	else
> +		walign = 4;
> +
> +	v4l_bound_align_image(&pixfmt->width, 1, 0xffff, walign,
>  			      &pixfmt->height, 1, 0xffff, 1, 0);
>  
>  	stride = round_up((pixfmt->width * cc->bpp) / 8, 8);

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

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>
---
Hi Alexander,

This is an additional cleanup that applies on top of "[PATCH v2 0/3] Fix
imx7-media-csi format settings". I've only compile-tested it as I'm
currently lacking access to test hardware. Would you be able to test the
patch ? If so, could you please include it in the v2 of your series ?
---
 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 5240670476b2..e52d617eea59 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
  */
@@ -1618,22 +1585,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 = { };
+	struct v4l2_pix_format *pixfmt = &csi->vdev_fmt;
 
-	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;
+	pixfmt->width = IMX7_CSI_DEF_PIX_WIDTH;
+	pixfmt->height = IMX7_CSI_DEF_PIX_HEIGHT;
 
-	imx7_csi_mbus_fmt_to_pix_fmt(&csi->vdev_fmt, &format, NULL);
-	csi->vdev_compose.width = format.width;
-	csi->vdev_compose.height = format.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)
@@ -1646,9 +1605,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);
-- 
Regards,

Laurent Pinchart

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

[Alexander Stein]

Hi Laurent,

thanks for the nice cleanup.

Am Dienstag, 18. April 2023, 12:04:17 CEST schrieb Laurent Pinchart:
> 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>
> ---
> Hi Alexander,
> 
> This is an additional cleanup that applies on top of "[PATCH v2 0/3] Fix
> imx7-media-csi format settings". I've only compile-tested it as I'm
> currently lacking access to test hardware. Would you be able to test the
> patch ? If so, could you please include it in the v2 of your series ?

I can't detect any difference in 'media-ctl -p' right after boot, so I
assume the initialization is identical. LGTM, I'll include in v3 of my
series.

Thanks,
Alexander

> ---
>  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
> 5240670476b2..e52d617eea59 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
>   */
> @@ -1618,22 +1585,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 = { };
> +	struct v4l2_pix_format *pixfmt = &csi->vdev_fmt;
> 
> -	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;
> +	pixfmt->width = IMX7_CSI_DEF_PIX_WIDTH;
> +	pixfmt->height = IMX7_CSI_DEF_PIX_HEIGHT;
> 
> -	imx7_csi_mbus_fmt_to_pix_fmt(&csi->vdev_fmt, &format, NULL);
> -	csi->vdev_compose.width = format.width;
> -	csi->vdev_compose.height = format.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)
> @@ -1646,9 +1605,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);


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