| Message ID | 20230720222543.1740198-1-festevam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] media: imx: imx7-media-csi: Fix applying format constraints | expand |
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.
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