* Re: media: imx: imx7-media-csi: Get rid of superfluous call to imx7_csi_mbus_fm t_to_pix_fmt [not found] <CAJ+vNU0BOVLTL17ofgHwtexbpuMYwH_aGUC==EXABUtHHiv_ag@mail.gmail.com> @ 2023-07-17 9:57 ` Alexander Stein 2023-07-19 17:27 ` Tim Harvey 0 siblings, 1 reply; 8+ messages in thread From: Alexander Stein @ 2023-07-17 9:57 UTC (permalink / raw) To: Tim Harvey Cc: Pengutronix Kernel Team, NXP Linux Team, linux-media, Linux ARM Mailing List, Fabio Estevam, Sascha Hauer, Mauro Carvalho Chehab, Rui Miguel Silva, Shawn Guo, Laurent Pinchart, Schrempf Frieder 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. > 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. Best regards, Alexander > best regards, > > Tim > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arc > h/arm64/boot/dts/freescale/imx8mm-venice-gw72xx-0x-imx219.dtso -- 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: media: imx: imx7-media-csi: Get rid of superfluous call to imx7_csi_mbus_fm t_to_pix_fmt 2023-07-17 9:57 ` media: imx: imx7-media-csi: Get rid of superfluous call to imx7_csi_mbus_fm t_to_pix_fmt Alexander Stein @ 2023-07-19 17:27 ` Tim Harvey 2023-07-20 6:49 ` Alexander Stein 0 siblings, 1 reply; 8+ messages in thread From: Tim Harvey @ 2023-07-19 17:27 UTC (permalink / raw) To: Alexander Stein Cc: Pengutronix Kernel Team, NXP Linux Team, linux-media, Linux ARM Mailing List, Fabio Estevam, Sascha Hauer, Mauro Carvalho Chehab, Rui Miguel Silva, Shawn Guo, Laurent Pinchart, Schrempf Frieder 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 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: media: imx: imx7-media-csi: Get rid of superfluous call to imx7_csi_mbus_fm t_to_pix_fmt 2023-07-19 17:27 ` Tim Harvey @ 2023-07-20 6:49 ` Alexander Stein 2023-07-20 15:12 ` Tim Harvey 0 siblings, 1 reply; 8+ messages in thread From: Alexander Stein @ 2023-07-20 6:49 UTC (permalink / raw) To: Tim Harvey Cc: Pengutronix Kernel Team, NXP Linux Team, linux-media, Linux ARM Mailing List, Fabio Estevam, Sascha Hauer, Mauro Carvalho Chehab, Rui Miguel Silva, Shawn Guo, Laurent Pinchart, Schrempf Frieder 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: media: imx: imx7-media-csi: Get rid of superfluous call to imx7_csi_mbus_fm t_to_pix_fmt 2023-07-20 6:49 ` Alexander Stein @ 2023-07-20 15:12 ` Tim Harvey 2023-07-20 15:52 ` Fabio Estevam 2023-07-20 18:47 ` Fabio Estevam 0 siblings, 2 replies; 8+ messages in thread From: Tim Harvey @ 2023-07-20 15:12 UTC (permalink / raw) To: Alexander Stein Cc: Pengutronix Kernel Team, NXP Linux Team, linux-media, Linux ARM Mailing List, Fabio Estevam, Sascha Hauer, Mauro Carvalho Chehab, Rui Miguel Silva, Shawn Guo, Laurent Pinchart, Schrempf Frieder On Wed, Jul 19, 2023 at 11:49 PM Alexander Stein <alexander.stein@ew.tq-group.com> 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@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? 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: media: imx: imx7-media-csi: Get rid of superfluous call to imx7_csi_mbus_fm t_to_pix_fmt 2023-07-20 15:12 ` Tim Harvey @ 2023-07-20 15:52 ` Fabio Estevam 2023-07-20 16:04 ` Fabio Estevam 2023-07-20 18:47 ` Fabio Estevam 1 sibling, 1 reply; 8+ messages in thread From: Fabio Estevam @ 2023-07-20 15:52 UTC (permalink / raw) To: Tim Harvey Cc: Alexander Stein, Pengutronix Kernel Team, NXP Linux Team, linux-media, Linux ARM Mailing List, Sascha Hauer, Mauro Carvalho Chehab, Rui Miguel Silva, Shawn Guo, Laurent Pinchart, Schrempf Frieder Hi Tim, On Thu, Jul 20, 2023 at 12:12 PM Tim Harvey <tharvey@gateworks.com> wrote: > 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. I tested capture on imx8mm-evk and imx8mn-evk boards on linux-next 20230720. Both of them are working here, so I was not able to reproduce the probe failure you reported. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: media: imx: imx7-media-csi: Get rid of superfluous call to imx7_csi_mbus_fm t_to_pix_fmt 2023-07-20 15:52 ` Fabio Estevam @ 2023-07-20 16:04 ` Fabio Estevam 0 siblings, 0 replies; 8+ messages in thread From: Fabio Estevam @ 2023-07-20 16:04 UTC (permalink / raw) To: Tim Harvey Cc: Alexander Stein, Pengutronix Kernel Team, NXP Linux Team, linux-media, Linux ARM Mailing List, Sascha Hauer, Mauro Carvalho Chehab, Rui Miguel Silva, Shawn Guo, Laurent Pinchart, Schrempf Frieder Hi Tim, On Thu, Jul 20, 2023 at 12:52 PM Fabio Estevam <festevam@gmail.com> wrote: > I tested capture on imx8mm-evk and imx8mn-evk boards on linux-next 20230720. > > Both of them are working here, so I was not able to reproduce the > probe failure you reported. Does the change below help? --- a/arch/arm64/boot/dts/freescale/imx8mm-venice-gw72xx-0x-imx219.dtso +++ b/arch/arm64/boot/dts/freescale/imx8mm-venice-gw72xx-0x-imx219.dtso @@ -62,25 +62,12 @@ &mipi_csi { status = "okay"; ports { - #address-cells = <1>; - #size-cells = <0>; - - port@0 { - reg = <0>; - + port { imx8mm_mipi_csi_in: endpoint { remote-endpoint = <&imx219_to_mipi_csi2>; data-lanes = <1 2>; }; }; - - port@1 { - reg = <1>; - - imx8mm_mipi_csi_out: endpoint { - remote-endpoint = <&csi_in>; - }; - }; }; }; _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: media: imx: imx7-media-csi: Get rid of superfluous call to imx7_csi_mbus_fm t_to_pix_fmt 2023-07-20 15:12 ` Tim Harvey 2023-07-20 15:52 ` Fabio Estevam @ 2023-07-20 18:47 ` Fabio Estevam 2023-07-20 19:09 ` Fabio Estevam 1 sibling, 1 reply; 8+ messages in thread From: Fabio Estevam @ 2023-07-20 18:47 UTC (permalink / raw) To: Tim Harvey Cc: Alexander Stein, Pengutronix Kernel Team, NXP Linux Team, linux-media, Linux ARM Mailing List, Sascha Hauer, Mauro Carvalho Chehab, Rui Miguel Silva, Shawn Guo, Laurent Pinchart, Schrempf Frieder On Thu, Jul 20, 2023 at 12:12 PM Tim Harvey <tharvey@gateworks.com> wrote: > 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. Does it work if we do like this? --- a/drivers/media/platform/nxp/imx7-media-csi.c +++ b/drivers/media/platform/nxp/imx7-media-csi.c @@ -1108,6 +1108,17 @@ static int imx7_csi_video_g_fmt_vid_cap(struct file *file, void *fh, return 0; } +/* borrowed from drivers/media/v4l2-core/v4l2-common.c */ +static unsigned int clamp_roundup(unsigned int x, unsigned int min, + unsigned int max, unsigned int alignment) +{ + x = clamp(x, min, max); + if (alignment) + x = round_up(x, alignment); + + return x; +} + static const struct imx7_csi_pixfmt * __imx7_csi_video_try_fmt(struct v4l2_pix_format *pixfmt, struct v4l2_rect *compose) @@ -1137,8 +1148,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, walign); pixfmt->bytesperline = pixfmt->width * cc->bpp / 8; pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height; _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: media: imx: imx7-media-csi: Get rid of superfluous call to imx7_csi_mbus_fm t_to_pix_fmt 2023-07-20 18:47 ` Fabio Estevam @ 2023-07-20 19:09 ` Fabio Estevam 0 siblings, 0 replies; 8+ messages in thread From: Fabio Estevam @ 2023-07-20 19:09 UTC (permalink / raw) To: Tim Harvey Cc: Alexander Stein, Pengutronix Kernel Team, NXP Linux Team, linux-media, Linux ARM Mailing List, Sascha Hauer, Mauro Carvalho Chehab, Rui Miguel Silva, Shawn Guo, Laurent Pinchart, Schrempf Frieder On Thu, Jul 20, 2023 at 3:47 PM Fabio Estevam <festevam@gmail.com> wrote: > Does it work if we do like this? > > --- a/drivers/media/platform/nxp/imx7-media-csi.c > +++ b/drivers/media/platform/nxp/imx7-media-csi.c > @@ -1108,6 +1108,17 @@ static int imx7_csi_video_g_fmt_vid_cap(struct > file *file, void *fh, > return 0; > } > > +/* borrowed from drivers/media/v4l2-core/v4l2-common.c */ > +static unsigned int clamp_roundup(unsigned int x, unsigned int min, > + unsigned int max, unsigned int alignment) > +{ > + x = clamp(x, min, max); > + if (alignment) > + x = round_up(x, alignment); > + > + return x; > +} > + > static const struct imx7_csi_pixfmt * > __imx7_csi_video_try_fmt(struct v4l2_pix_format *pixfmt, > struct v4l2_rect *compose) > @@ -1137,8 +1148,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, walign); Actually the height should be: pixfmt->height = clamp_roundup(pixfmt->height, 1, 0xffff, 1); _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-07-20 19:09 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CAJ+vNU0BOVLTL17ofgHwtexbpuMYwH_aGUC==EXABUtHHiv_ag@mail.gmail.com>
2023-07-17 9:57 ` media: imx: imx7-media-csi: Get rid of superfluous call to imx7_csi_mbus_fm t_to_pix_fmt Alexander Stein
2023-07-19 17:27 ` Tim Harvey
2023-07-20 6:49 ` Alexander Stein
2023-07-20 15:12 ` Tim Harvey
2023-07-20 15:52 ` Fabio Estevam
2023-07-20 16:04 ` Fabio Estevam
2023-07-20 18:47 ` Fabio Estevam
2023-07-20 19:09 ` Fabio Estevam
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox