[v2] media: imx: imx7-media-csi: Fix applying format constraints

  From: Fabio Estevam <festevam@denx.de>

v4l_bound_align_image() aligns to a multiple power of 2 of walign, but the
result only needs to be a multiple of walign.

This causes a 640x480 sensor that used to report:

	Width/Height      : 640/480
        
to incorrectly report:

	Width/Height      : 768/480

Fix this problem by doing the correct alignment via clamp_roundup().

Reported-by: Tim Harvey <tharvey@gateworks.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>
---
Changes since v1:
- Export clamp_roundup().

 drivers/media/platform/nxp/imx7-media-csi.c | 4 ++--
 drivers/media/v4l2-core/v4l2-common.c       | 5 +++--
 include/media/v4l2-common.h                 | 2 ++
 3 files changed, 7 insertions(+), 4 deletions(-)
  

On Thu, Jul 20, 2023 at 3:25 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> From: Fabio Estevam <festevam@denx.de>
>
> v4l_bound_align_image() aligns to a multiple power of 2 of walign, but the
> result only needs to be a multiple of walign.
>
> This causes a 640x480 sensor that used to report:
>
>         Width/Height      : 640/480
>
> to incorrectly report:
>
>         Width/Height      : 768/480
>
> Fix this problem by doing the correct alignment via clamp_roundup().
>
> Reported-by: Tim Harvey <tharvey@gateworks.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>
> ---
> Changes since v1:
> - Export clamp_roundup().
>
>  drivers/media/platform/nxp/imx7-media-csi.c | 4 ++--
>  drivers/media/v4l2-core/v4l2-common.c       | 5 +++--
>  include/media/v4l2-common.h                 | 2 ++
>  3 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
> index 0bd2613b9320..f3c506fc19c4 100644
> --- a/drivers/media/platform/nxp/imx7-media-csi.c
> +++ b/drivers/media/platform/nxp/imx7-media-csi.c
> @@ -1137,8 +1137,8 @@ __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_roundup(pixfmt->width, 1, 0xffff, walign);
> +       pixfmt->height = clamp_roundup(pixfmt->height, 1, 0xffff, 1);
>
>         pixfmt->bytesperline = pixfmt->width * cc->bpp / 8;
>         pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height;
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index bee1535b04d3..3e8c16bcb0f6 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -106,8 +106,8 @@ static unsigned int clamp_align(unsigned int x, unsigned int min,
>         return x;
>  }
>
> -static unsigned int clamp_roundup(unsigned int x, unsigned int min,
> -                                  unsigned int max, unsigned int alignment)
> +unsigned int clamp_roundup(unsigned int x, unsigned int min,
> +                          unsigned int max, unsigned int alignment)
>  {
>         x = clamp(x, min, max);
>         if (alignment)
> @@ -115,6 +115,7 @@ static unsigned int clamp_roundup(unsigned int x, unsigned int min,
>
>         return x;
>  }
> +EXPORT_SYMBOL(clamp_roundup);
>
>  void v4l_bound_align_image(u32 *w, unsigned int wmin, unsigned int wmax,
>                            unsigned int walign,
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index d278836fd9cb..7059b99f4afa 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -521,6 +521,8 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
>                      u32 width, u32 height);
>  int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat,
>                         u32 width, u32 height);
> +unsigned int clamp_roundup(unsigned int x, unsigned int min,
> +                          unsigned int max, unsigned int alignment);
>
>  /**
>   * v4l2_get_link_freq - Get link rate from transmitter
> --
> 2.34.1
>

Thanks Alexander and Fabio!

Acked-by: Tim Harvey <tharvey@gateworks.com>

tested on a imx8mm-gw72xx-0x with imx8mm-gw72xx-0x-imx219 overlay

Best regards,

Tim
  

Am Freitag, 21. Juli 2023, 00:25:43 CEST schrieb Fabio Estevam:
> From: Fabio Estevam <festevam@denx.de>
> 
> v4l_bound_align_image() aligns to a multiple power of 2 of walign, but the
> result only needs to be a multiple of walign.
> 
> This causes a 640x480 sensor that used to report:
> 
> 	Width/Height      : 640/480
> 
> to incorrectly report:
> 
> 	Width/Height      : 768/480
> 
> Fix this problem by doing the correct alignment via clamp_roundup().
> 
> Reported-by: Tim Harvey <tharvey@gateworks.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>
> ---
> Changes since v1:
> - Export clamp_roundup().
> 
>  drivers/media/platform/nxp/imx7-media-csi.c | 4 ++--
>  drivers/media/v4l2-core/v4l2-common.c       | 5 +++--
>  include/media/v4l2-common.h                 | 2 ++
>  3 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx7-media-csi.c
> b/drivers/media/platform/nxp/imx7-media-csi.c index
> 0bd2613b9320..f3c506fc19c4 100644
> --- a/drivers/media/platform/nxp/imx7-media-csi.c
> +++ b/drivers/media/platform/nxp/imx7-media-csi.c
> @@ -1137,8 +1137,8 @@ __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_roundup(pixfmt->width, 1, 0xffff, walign);
> +	pixfmt->height = clamp_roundup(pixfmt->height, 1, 0xffff, 1);
> 
>  	pixfmt->bytesperline = pixfmt->width * cc->bpp / 8;
>  	pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height;
> diff --git a/drivers/media/v4l2-core/v4l2-common.c
> b/drivers/media/v4l2-core/v4l2-common.c index bee1535b04d3..3e8c16bcb0f6
> 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -106,8 +106,8 @@ static unsigned int clamp_align(unsigned int x, unsigned
> int min, return x;
>  }
> 
> -static unsigned int clamp_roundup(unsigned int x, unsigned int min,
> -				   unsigned int max, unsigned int 
alignment)
> +unsigned int clamp_roundup(unsigned int x, unsigned int min,
> +			   unsigned int max, unsigned int alignment)
>  {
>  	x = clamp(x, min, max);
>  	if (alignment)
> @@ -115,6 +115,7 @@ static unsigned int clamp_roundup(unsigned int x,
> unsigned int min,
> 
>  	return x;
>  }
> +EXPORT_SYMBOL(clamp_roundup);
> 
>  void v4l_bound_align_image(u32 *w, unsigned int wmin, unsigned int wmax,
>  			   unsigned int walign,
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index d278836fd9cb..7059b99f4afa 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -521,6 +521,8 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32
> pixelformat, u32 width, u32 height);
>  int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32
> pixelformat, u32 width, u32 height);
> +unsigned int clamp_roundup(unsigned int x, unsigned int min,
> +			   unsigned int max, unsigned int alignment);
> 
>  /**
>   * v4l2_get_link_freq - Get link rate from transmitter

I expect this to work, but I'm not so sure about exporting clamp_roundup().
I would rather use v4l2_apply_frmsize_constraints() instead, but this requires 
additional work, refer to the TODO in imx7_csi_video_enum_framesizes().
See also my first (broken) try in [1].

As a short term fix, using clamp_roundup() is okay to me. But I'm thinking 
about using a copy of clamp_roundup() as in v1 instead,so to avoid an eventual 
unexport of clamp_roundup in the future. Maybe this concern doesn't matter, I 
guess the maintainers can tell.

Best regards,
Alexander

[1] https://lore.kernel.org/linux-media/202307210050.s7hfCvwG-lkp@intel.com/T/
#ma88811098cb96f3698a8248d903f0c3455febec7
  

On Fri, Jul 21, 2023 at 07:48:52AM +0200, Alexander Stein wrote:
> Am Freitag, 21. Juli 2023, 00:25:43 CEST schrieb Fabio Estevam:
> > From: Fabio Estevam <festevam@denx.de>
> > 
> > v4l_bound_align_image() aligns to a multiple power of 2 of walign, but the
> > result only needs to be a multiple of walign.
> > 
> > This causes a 640x480 sensor that used to report:
> > 
> > 	Width/Height      : 640/480
> > 
> > to incorrectly report:
> > 
> > 	Width/Height      : 768/480
> > 
> > Fix this problem by doing the correct alignment via clamp_roundup().
> > 
> > Reported-by: Tim Harvey <tharvey@gateworks.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>
> > ---
> > Changes since v1:
> > - Export clamp_roundup().
> > 
> >  drivers/media/platform/nxp/imx7-media-csi.c | 4 ++--
> >  drivers/media/v4l2-core/v4l2-common.c       | 5 +++--
> >  include/media/v4l2-common.h                 | 2 ++
> >  3 files changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/platform/nxp/imx7-media-csi.c
> > b/drivers/media/platform/nxp/imx7-media-csi.c index
> > 0bd2613b9320..f3c506fc19c4 100644
> > --- a/drivers/media/platform/nxp/imx7-media-csi.c
> > +++ b/drivers/media/platform/nxp/imx7-media-csi.c
> > @@ -1137,8 +1137,8 @@ __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_roundup(pixfmt->width, 1, 0xffff, walign);
> > +	pixfmt->height = clamp_roundup(pixfmt->height, 1, 0xffff, 1);
> > 
> >  	pixfmt->bytesperline = pixfmt->width * cc->bpp / 8;
> >  	pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height;
> > diff --git a/drivers/media/v4l2-core/v4l2-common.c
> > b/drivers/media/v4l2-core/v4l2-common.c index bee1535b04d3..3e8c16bcb0f6
> > 100644
> > --- a/drivers/media/v4l2-core/v4l2-common.c
> > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > @@ -106,8 +106,8 @@ static unsigned int clamp_align(unsigned int x, unsigned
> > int min, return x;
> >  }
> > 
> > -static unsigned int clamp_roundup(unsigned int x, unsigned int min,
> > -				   unsigned int max, unsigned int 
> alignment)
> > +unsigned int clamp_roundup(unsigned int x, unsigned int min,
> > +			   unsigned int max, unsigned int alignment)
> >  {
> >  	x = clamp(x, min, max);
> >  	if (alignment)
> > @@ -115,6 +115,7 @@ static unsigned int clamp_roundup(unsigned int x,
> > unsigned int min,
> > 
> >  	return x;
> >  }
> > +EXPORT_SYMBOL(clamp_roundup);
> > 
> >  void v4l_bound_align_image(u32 *w, unsigned int wmin, unsigned int wmax,
> >  			   unsigned int walign,
> > diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> > index d278836fd9cb..7059b99f4afa 100644
> > --- a/include/media/v4l2-common.h
> > +++ b/include/media/v4l2-common.h
> > @@ -521,6 +521,8 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32
> > pixelformat, u32 width, u32 height);
> >  int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32
> > pixelformat, u32 width, u32 height);
> > +unsigned int clamp_roundup(unsigned int x, unsigned int min,
> > +			   unsigned int max, unsigned int alignment);
> > 
> >  /**
> >   * v4l2_get_link_freq - Get link rate from transmitter
> 
> I expect this to work, but I'm not so sure about exporting clamp_roundup().

I agree. The function name is too generic to be exported by V4L2. It
should be moved to include/linux/minmax.h (and the implementation to a
corresponding source file if it's too large to be inlined) if we want to
export it.

> I would rather use v4l2_apply_frmsize_constraints() instead, but this requires 
> additional work, refer to the TODO in imx7_csi_video_enum_framesizes().

That TODO is definitely outdated, it would be nice to address it. I'll
submit a patch.

> See also my first (broken) try in [1].
> 
> As a short term fix, using clamp_roundup() is okay to me. But I'm thinking 
> about using a copy of clamp_roundup() as in v1 instead,so to avoid an eventual 
> unexport of clamp_roundup in the future. Maybe this concern doesn't matter, I 
> guess the maintainers can tell.
> 
> Best regards,
> Alexander
> 
> [1] https://lore.kernel.org/linux-media/202307210050.s7hfCvwG-lkp@intel.com/T/
> #ma88811098cb96f3698a8248d903f0c3455febec7
  

Hi Fabio,

Thank you for the patch.

On Thu, Jul 20, 2023 at 07:25:43PM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <festevam@denx.de>
> 
> v4l_bound_align_image() aligns to a multiple power of 2 of walign, but the
> result only needs to be a multiple of walign.
> 
> This causes a 640x480 sensor that used to report:
> 
> 	Width/Height      : 640/480
>         
> to incorrectly report:
> 
> 	Width/Height      : 768/480
> 
> Fix this problem by doing the correct alignment via clamp_roundup().
> 
> Reported-by: Tim Harvey <tharvey@gateworks.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>
> ---
> Changes since v1:
> - Export clamp_roundup().
> 
>  drivers/media/platform/nxp/imx7-media-csi.c | 4 ++--
>  drivers/media/v4l2-core/v4l2-common.c       | 5 +++--
>  include/media/v4l2-common.h                 | 2 ++
>  3 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
> index 0bd2613b9320..f3c506fc19c4 100644
> --- a/drivers/media/platform/nxp/imx7-media-csi.c
> +++ b/drivers/media/platform/nxp/imx7-media-csi.c
> @@ -1137,8 +1137,8 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format *pixfmt,
>  	 * TODO: Implement configurable stride support.
>  	 */framesize
>  	walign = 8 * 8 / cc->bpp;
> -	v4l_bound_align_image(&pixfmt->width, 1, 0xffff, walign,
> -			      &pixfmt->height, 1, 0xffff, 1, 0);
> +	pixfmt->width = clamp_roundup(pixfmt->width, 1, 0xffff, walign);
> +	pixfmt->height = clamp_roundup(pixfmt->height, 1, 0xffff, 1);
>  
>  	pixfmt->bytesperline = pixfmt->width * cc->bpp / 8;
>  	pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height;
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index bee1535b04d3..3e8c16bcb0f6 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -106,8 +106,8 @@ static unsigned int clamp_align(unsigned int x, unsigned int min,
>  	return x;
>  }
>  
> -static unsigned int clamp_roundup(unsigned int x, unsigned int min,
> -				   unsigned int max, unsigned int alignment)
> +unsigned int clamp_roundup(unsigned int x, unsigned int min,
> +			   unsigned int max, unsigned int alignment)
>  {
>  	x = clamp(x, min, max);
>  	if (alignment)
> @@ -115,6 +115,7 @@ static unsigned int clamp_roundup(unsigned int x, unsigned int min,
>  
>  	return x;
>  }
> +EXPORT_SYMBOL(clamp_roundup);
>  
>  void v4l_bound_align_image(u32 *w, unsigned int wmin, unsigned int wmax,
>  			   unsigned int walign,
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index d278836fd9cb..7059b99f4afa 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -521,6 +521,8 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
>  		     u32 width, u32 height);
>  int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat,
>  			u32 width, u32 height);
> +unsigned int clamp_roundup(unsigned int x, unsigned int min,
> +			   unsigned int max, unsigned int alignment);

The function name is too generic to be exported by V4L2. It should move
to include/linux/minmax.h (and the implementation to a corresponding
source file if it's too large to be inlined) if you want to export it.

I've submitted a v3 of this patch, along with two other patches, and
CC'ed you. The series retains your authorship on this patch. Could you
please review it ?

>  
>  /**
>   * v4l2_get_link_freq - Get link rate from transmitter
  

Hi Laurent,

On Tue, Jul 25, 2023 at 5:06 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

> I've submitted a v3 of this patch, along with two other patches, and
> CC'ed you. The series retains your authorship on this patch. Could you
> please review it ?

v3 looks good to me, thanks.

Tim said on another thread he would be out on vacation for two weeks,
so he would not be able to
test it soon.

I manually hardcoded Tim's value on the driver so that I could
reproduce the original 480 to 768 width miscalculation.

With v3, the width is reported correctly as 480.

Thanks
  

On Tue, Jul 25, 2023 at 7:51 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Laurent,
>
> On Tue, Jul 25, 2023 at 5:06 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>
> > I've submitted a v3 of this patch, along with two other patches, and
> > CC'ed you. The series retains your authorship on this patch. Could you
> > please review it ?
>
> v3 looks good to me, thanks.
>
> Tim said on another thread he would be out on vacation for two weeks,
> so he would not be able to
> test it soon.
>
> I manually hardcoded Tim's value on the driver so that I could
> reproduce the original 480 to 768 width miscalculation.
>
> With v3, the width is reported correctly as 480.

Sorry, I meant the width is correctly calculated as 640 here.

So all is good with v3.