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

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

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

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?

best regards,

Tim
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mm-venice-gw72xx-0x-imx219.dtso

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

[Alexander Stein]

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/

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

[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

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

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

[Alexander Stein]

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/

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

[Tim Harvey]

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

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

[Fabio Estevam]

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.

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

[Fabio Estevam]

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

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

[Fabio Estevam]

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;

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

[Fabio Estevam]

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