Re: media: imx: imx7-media-csi: Get rid of superfluous call to imx7_csi_mbus_fm t_to_pix_fmt

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hi Tim,

Am Mittwoch, 19. Juli 2023, 19:27:54 CEST schrieb Tim Harvey:
> On Mon, Jul 17, 2023 at 2:57 AM Alexander Stein
> 
> <alexander.stein@ew.tq-group.com> wrote:
> > Hi Tim,
> > 
> > Am Freitag, 14. Juli 2023, 03:34:15 CEST schrieb Tim Harvey:
> > > Alexander,
> > > 
> > > I found that commit 6f482c4729d9: ("media: imx: imx7-media-csi: Get
> > > rid of superfluous call to imx7_csi_mbus_fmt_to_pix_fmt") introduced
> > > an issue causing me to not be able to capture anymore on an imx8mm
> > > with an imx219 camera.
> > > 
> > > I'm using a RaspberryPi Camera v2 which has an IMX219 8MP camera module:
> > > - https://datasheets.raspberrypi.com/camera/camera-v2-schematics.pdf
> > > - has its own on-board 24MHz osc so no clock required from baseboard
> > > - pin 11 enables 1.8V and 2.8V LDO which is connected to a GPIO I use
> > > as a regulator enable
> > > 
> > > I'm using the imx8mm-venice-gw72xx-0x-imx219 dt overlay [1] to test
> > > this.
> > > 
> > > Here is some additional information about how I'm using the camera
> > > module:
> > > # cat /sys/bus/media/devices/media*/model
> > > imx-media
> > > hantro-vpu
> > > hantro-vpu
> > > # cat /sys/class/video4linux/video*/name
> > > csi capture
> > > nxp,imx8mm-vpu-g1-dec
> > > nxp,imx8mq-vpu-g2-dec
> > > # enable imx219 to csi link
> > > media-ctl --reset
> > > media-ctl -l "'imx219 2-0010':0 -> 'csis-32e30000.mipi-csi':0 [1]"
> > > # configure for 640x480 raw8
> > > media-ctl -v -V "'imx219 2-0010':0 [fmt:SRGGB8/640x480 field:none]"
> > > media-ctl -v -V "'csis-32e30000.mipi-csi':0 [fmt:SRGGB8/640x480
> > > field:none]" media-ctl -v -V "'csi':0 [fmt:SRGGB8/640x480 field:none]"
> > > # configure for RGGB (8-bit bayer), 640x480 resolution
> > > v4l2-ctl --device /dev/video0
> > > --set-fmt-video=width=640,height=480,pixelformat=RGGB --verbose
> > > 
> > > before commit 6f482c4729d9: ("media: imx: imx7-media-csi: Get rid of
> > > superfluous call to imx7_csi_mbus_fmt_to_pix_fmt") this would report
> > > back 640x480 resolution:
> > > VIDIOC_QUERYCAP: ok
> > > VIDIOC_G_FMT: ok
> > > VIDIOC_S_FMT: ok
> > > 
> > > Format Video Capture:
> > >         Width/Height      : 640/480
> > >         Pixel Format      : 'RGGB' (8-bit Bayer RGRG/GBGB)
> > >         Field             : None
> > >         Bytes per Line    : 640
> > >         Size Image        : 307200
> > >         Colorspace        : Default
> > >         Transfer Function : Default (maps to Rec. 709)
> > >         YCbCr/HSV Encoding: Default (maps to ITU-R 601)
> > >         Quantization      : Default (maps to Full Range)
> > > 
> > >         Flags             :
> > > And after the commit it reports back an invalid 768x480 resolution:
> > > VIDIOC_QUERYCAP: ok
> > > VIDIOC_G_FMT: ok
> > > VIDIOC_S_FMT: ok
> > > 
> > > Format Video Capture:
> > >         Width/Height      : 768/480
> > >         Pixel Format      : 'RGGB' (8-bit Bayer RGRG/GBGB)
> > >         Field             : None
> > >         Bytes per Line    : 768
> > >         Size Image        : 368640
> > >         Colorspace        : Default
> > >         Transfer Function : Default (maps to Rec. 709)
> > >         YCbCr/HSV Encoding: Default (maps to ITU-R 601)
> > >         Quantization      : Default (maps to Full Range)
> > > 
> > >         Flags             :
> > > This resolution and frame size mis-match causes issues for example
> > > when using gstreamer to capture and stream frames.
> > 
> > Oh, that's weird. Can you check what the call to v4l_bound_align_image()
> > inside __imx7_csi_video_try_fmt() is actually doing? Check walign, width
> > before and after the call. From a glance that seems to be the only way
> > width could be modified.
> 
> with debugging added in __imx7_csi_video_try_fmt:
> 
> root@jammy-venice:~# dmesg | grep csi
> [    0.038495] platform 32e30000.mipi-csi: Fixed dependency cycle(s)
> with /soc@0/bus@32c00000/csi@32e20000/port/endpoint
> [    1.195055] __imx7_csi_video_try_fmt walign=4 bpp=16 640/480
> [    1.200746] __imx7_csi_video_try_fmt walign=4 bpp=16
> bytesperline=1280 sizeimage=614400 640/480
> [    1.209633] imx7-csi 32e20000.csi: Registered csi capture as /dev/video0
> [    1.775321] i2c 2-0010: Fixed dependency cycle(s) with
> /soc@0/bus@32c00000/mipi-csi@32e30000/ports/port@0/endpoint
> [    2.010372] imx-mipi-csis 32e30000.mipi-csi: lanes: 2, freq: 333000000
> ^^^ this is kernel init and looks fine
> 
> # media-ctl -l "'imx219 2-0010':0 -> 'csis-32e30000.mipi-csi':0 [1]"
> # media-ctl -v -V "'imx219 2-0010':0 [fmt:SRGGB8/640x480 field:none]"
> # media-ctl -v -V "'csis-32e30000.mipi-csi':0 [fmt:SRGGB8/640x480
> field:none]" # media-ctl -v -V "'csi':0 [fmt:SRGGB8/640x480 field:none]"
> ^^^ nothing changes here during link config (no calls to
> __imx7_csi_video_try_fmt)
> 
> # v4l2-ctl --device /dev/video0
> --set-fmt-video=width=640,height=480,pixelformat=RGGB --verbose
> VIDIOC_QUERYCAP: ok
> [  120.367874] __imx7_csi_video_try_fmt walign=8 bpp=8 640/480
> ^^^ before calling v4l_bound_align_image()
> 
> VIDIOC_G_FMT: ok
> [  120.374974] __imx7_csi_video_try_fmt walign=8 bpp=8
> bytesperline=768 sizeimage=368640 768/480
> ^^^ after calling v4l_bound_align_image() width is changed from 640 to 768

So this call is the culprit, I noticed the important change to before. The 
alignment enforced in clamp_align(), called by v4l_bound_align_image(), is a 
multiple of 2^align, _not_ a multiple of align. So obviously 640 is increased 
to the next multiple of 2^8 (256) which is 768. Can you give the following 
diff a try?

--8<--
diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/
platform/nxp/imx7-media-csi.c
index 2f9302fc7570..a040665eca7c 100644
--- a/drivers/media/platform/nxp/imx7-media-csi.c
+++ b/drivers/media/platform/nxp/imx7-media-csi.c
@@ -1137,9 +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->width, 1, 0xffff, 1);
        pixfmt->bytesperline = pixfmt->width * cc->bpp / 8;
        pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height;
        pixfmt->field = V4L2_FIELD_NONE;
--8<--

Thanks and best regards,
Alexander

> VIDIOC_S_FMT: ok
> Format Video Capture:
>         Width/Height      : 768/480
>         Pixel Format      : 'RGGB' (8-bit Bayer RGRG/GBGB)
>         Field             : None
>         Bytes per Line    : 768
>         Size Image        : 368640
>         Colorspace        : Default
>         Transfer Function : Default (maps to Rec. 709)
>         YCbCr/HSV Encoding: Default (maps to ITU-R 601)
>         Quantization      : Default (maps to Full Range)
> 
>         Flags             :
> > > I noticed you have several outstanding patches pending for
> > > imx7-media-csi... perhaps there is something there you already know of
> > > that addresses this issue?
> > 
> > There nothing pending AFAICS. Everything has been integrated into
> > linux-next at least.
> 
> v6.5-rc2 shows the issue I've been discussing but linux-next
> next-20230718 doesn't even probe mipi-csi and I'm not clear why yet:
> 
> with linux-next next-20230718:
> # dmesg | grep csi
> [    1.172711] imx7-csi 32e20000.csi: Registered csi capture as /dev/video0
> [    1.179674] imx7-csi 32e20000.csi: error -ENOTCONN: Failed to get
> remote endpoint
> [    1.187513] imx7-csi: probe of 32e20000.csi failed with error -107
> [    1.757693] i2c 2-0010: Fixed dependency cycle(s) with
> /soc@0/bus@32c00000/mipi-csi@32e30000/ports/port@0/endpoint
> [   16.352048] platform 32e30000.mipi-csi: deferred probe pending
> # cat /sys/kernel/debug/devices_deferred
> 32e30000.mipi-csi       platform: wait for supplier
> /soc@0/bus@32c00000/csi@32e20000/port/endpoint
> 
> In this tree both drivers/media/i2c/imx219.c and
> drivers/media/platform/nxp/imx7-media-csi.c are at the same patch
> level and I'm not sure what else is involved here to look at.
> 
> It seems mipi-dsi and mipi-csi are extremely fragile and require
> constant regression testing unfortunately.
> 
> best regards,
> 
> Tim


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