| Message ID | 20210924120631.7060da0f.dorota.czaplejewicz@puri.sm (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: imx: Fix rounding | expand |
Hi Dorota, On Fri, Sep 24, 2021 at 12:06:31PM +0200, Dorota Czaplejewicz wrote: > Change rounding to the minimal burst size from 2^n to n. > > This fixes images with sizes that are a multiple of 8 pixels. Could you elaborate on what is currently wrong with images that are a multiple of 8 pixels wide? Or are you rather trying to add support for images that are not a multiple of 8 pixels wide? > > See section 13.7.6.13 CSI Image Parameter Register of the > i.MX 8M Quad Applications Processors Reference Manual. > > Fixes: 451a7b7815d0b ("media: imx: lift CSI and PRP ENC/VF width > alignment restriction") > Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm> > --- > Hi, > > I tested this patch on the Librem 5 with the main camera. > > --Dorota > drivers/staging/media/imx/imx-media-utils.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c > index 5128915a5d6f..a2d8fab32a39 100644 > --- a/drivers/staging/media/imx/imx-media-utils.c > +++ b/drivers/staging/media/imx/imx-media-utils.c > @@ -545,13 +545,13 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix, > } > > /* Round up width for minimum burst size */ > - width = round_up(mbus->width, 8); > + width = round_up(mbus->width, 3); That is not a valid use of the round_up() macro anymore. The second parameter must be a power of 2. regards Philipp
Hi, On Fri, 24 Sep 2021 13:46:15 +0200 Philipp Zabel <p.zabel@pengutronix.de> wrote: > Hi Dorota, > > On Fri, Sep 24, 2021 at 12:06:31PM +0200, Dorota Czaplejewicz wrote: > > Change rounding to the minimal burst size from 2^n to n. > > > > This fixes images with sizes that are a multiple of 8 pixels. > > Could you elaborate on what is currently wrong with images that are a > multiple of 8 pixels wide? Or are you rather trying to add support > for images that are not a multiple of 8 pixels wide? > > > > > See section 13.7.6.13 CSI Image Parameter Register of the > > i.MX 8M Quad Applications Processors Reference Manual. > > > > Fixes: 451a7b7815d0b ("media: imx: lift CSI and PRP ENC/VF width > > alignment restriction") > > Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm> > > --- > > Hi, > > > > I tested this patch on the Librem 5 with the main camera. > > > > --Dorota > > drivers/staging/media/imx/imx-media-utils.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c > > index 5128915a5d6f..a2d8fab32a39 100644 > > --- a/drivers/staging/media/imx/imx-media-utils.c > > +++ b/drivers/staging/media/imx/imx-media-utils.c > > @@ -545,13 +545,13 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix, > > } > > > > /* Round up width for minimum burst size */ > > - width = round_up(mbus->width, 8); > > + width = round_up(mbus->width, 3); > > That is not a valid use of the round_up() macro anymore. > The second parameter must be a power of 2. > > regards > Philipp Thanks for catching that. It turns out I was fixing an image that was a multiple of 4, but not a multiple of 8. The problem that occurred has to do with an image of size 1052x780, which covers the entire sensor in use at 1/4 scale. The sensor was configured to emit 1052*780 8-bit pixels, whereas the CSI portion here rounded up the image, expecting 1056*780. As a result, the userspace received a buffer sized according to the latter, each frame padded with some pixels from the next. As a result, each frame was composed of two or more, with a broken TV effect. The data sheet says that the width parameter "should" be a multiple of 8: > If the input data from the sensor is 8-bit/pixel format, the IMAGE_WIDTH should be a multiple of 8 pixels. Nevertheless, I double-tested this, and changing the relevant rounding from "8" to "4" works flawlessly here. Lower divisors were tested previously and did not work. Does it make sense to accept 4 as a valid divisor, given that rounding is likely not going to help but result in an invalid image anyway (as it did for me)? Cheers, Dorota
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c index 5128915a5d6f..a2d8fab32a39 100644 --- a/drivers/staging/media/imx/imx-media-utils.c +++ b/drivers/staging/media/imx/imx-media-utils.c @@ -545,13 +545,13 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix, } /* Round up width for minimum burst size */ - width = round_up(mbus->width, 8); + width = round_up(mbus->width, 3); /* Round up stride for IDMAC line start address alignment */ if (cc->planar) - stride = round_up(width, 16); + stride = round_up(width, 4); else - stride = round_up((width * cc->bpp) >> 3, 8); + stride = round_up((width * cc->bpp) >> 3, 3); pix->width = width; pix->height = mbus->height;
Change rounding to the minimal burst size from 2^n to n. This fixes images with sizes that are a multiple of 8 pixels. See section 13.7.6.13 CSI Image Parameter Register of the i.MX 8M Quad Applications Processors Reference Manual. Fixes: 451a7b7815d0b ("media: imx: lift CSI and PRP ENC/VF width alignment restriction") Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm> --- Hi, I tested this patch on the Librem 5 with the main camera. --Dorota drivers/staging/media/imx/imx-media-utils.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)