On Wed, Jul 19, 2023 at 11:49 PM Alexander Stein
<alexander.stein@xxxxxxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxxxxx> 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?
Yes, enforcing alignment of 2^8 is definitely not what we want.
>
> --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<--
clamp_roundup is static in v4l2-common.c so it would have to be
something like the following:
pixfmt->width = round_up(clamp(pixfmt->width, (unsigned int)1,
(unsigned int)0xffff), walign);
pixfmt->height = round_up(clamp(pixfmt->height, (unsigned
int)1, (unsigned int)0xffff), walign);
the above works. I'm not sure how to keep the compiler from
complaining about types other than typecasting the min/max values as
above.
>
> 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.
> >
have you tested mipi-csi with linux-next recently and observed it not
probing? I assume there is another issue awaiting us when linux-next
gets merged.
best regards,
Tim
