| Message ID | 20220224094313.233347-1-jacopo@jmondi.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | media: ov5640: Rework the clock tree programming for MIPI | expand |
Hello folks, On Thu, Feb 24, 2022 at 10:42:46AM +0100, Jacopo Mondi wrote: > A branch for testing based on the most recent media-master is available at > https://git.sr.ht/~jmondi_/linux #jmondi/media-master/ov5640-v5 The set has been around for quite some time without tangible functional changes, please do let me know if you have concerns merging it. Thanks.
On 3/4/22 2:09 AM, Sakari Ailus wrote: > Hello folks, > > On Thu, Feb 24, 2022 at 10:42:46AM +0100, Jacopo Mondi wrote: >> A branch for testing based on the most recent media-master is available at >> https://git.sr.ht/~jmondi_/linux #jmondi/media-master/ov5640-v5 > > The set has been around for quite some time without tangible functional > changes, please do let me know if you have concerns merging it. > > Thanks. > > -- > Sakari Ailus > Hello Sakari, Jacopo, I retested this series and on my side, the regression in version 3 is gone, I can capture images fine now YUYV@1280x720 . I also retested 1920x1080 in RAW, and it works fine. You can add my : Tested-by: Eugen Hristev <eugen.hristev@microchip.com> I am not sure if you have to add it to the whole series, because I only tested parallel interface with atmel ISC controller on sama5d2_xplained. Up to your judgment to add it to the patches that impact the parallel interface. Thanks again Jacopo for improving the support for this sensor. Eugen
Hi Eugen On Fri, Mar 04, 2022 at 08:41:23AM +0000, Eugen.Hristev@microchip.com wrote: > On 3/4/22 2:09 AM, Sakari Ailus wrote: > > > Hello folks, > > > > On Thu, Feb 24, 2022 at 10:42:46AM +0100, Jacopo Mondi wrote: > >> A branch for testing based on the most recent media-master is available at > >> https://git.sr.ht/~jmondi_/linux #jmondi/media-master/ov5640-v5 > > > > The set has been around for quite some time without tangible functional > > changes, please do let me know if you have concerns merging it. > > > > Thanks. > > > > -- > > Sakari Ailus > > > > > Hello Sakari, Jacopo, > > I retested this series and on my side, the regression in version 3 is > gone, I can capture images fine now YUYV@1280x720 . > I also retested 1920x1080 in RAW, and it works fine. > fiuuuu, I can breath again now :) > You can add my : > Tested-by: Eugen Hristev <eugen.hristev@microchip.com> > > I am not sure if you have to add it to the whole series, because I only > tested parallel interface with atmel ISC controller on sama5d2_xplained. > Up to your judgment to add it to the patches that impact the parallel > interface. > > Thanks again Jacopo for improving the support for this sensor. Thank you for testing with parallel mode, I would have made lot of people unhappy otherwise! > > Eugen
On Fri, Mar 04, 2022 at 09:45:29AM +0100, Jacopo Mondi wrote: > > Thanks again Jacopo for improving the support for this sensor. > > Thank you for testing with parallel mode, I would have made lot of > people unhappy otherwise! Thanks, guys! Applied to my tree.
Hi Jacopo, On 24/02/2022 11:42, Jacopo Mondi wrote: > v1: > https://patchwork.linuxtv.org/project/linux-media/list/?series=7249 > v2: > https://patchwork.linuxtv.org/project/linux-media/list/?series=7311 > v3: > https://patchwork.linuxtv.org/project/linux-media/list/?series=7385 > v4: > https://patchwork.linuxtv.org/project/linux-media/list/?series=7389 > > A branch for testing based on the most recent media-master is available at > https://git.sr.ht/~jmondi_/linux #jmondi/media-master/ov5640-v5 I tested these with DRA76 EVM & CAL, using CAL's legacy non-MC mode. It doesn't work. I think there are two problems: - CAL uses mbus codes like MEDIA_BUS_FMT_UYVY8_2X8 for CSI-2, not 1X16. OV5640 used to support 2X8, but now it doesn't. - OV5640 sets the default code to MEDIA_BUS_FMT_UYVY8_2X8, even for CSI-2 where it doesn't support MEDIA_BUS_FMT_UYVY8_2X8. I'd like to just change CAL and drop the 2X8 support and instead use 1X16, but then any sensor that uses 2X8 would work. So I guess I need to change the code to support both. Anyway, both of those issues might also surface on other platforms, as ov5640 behavior has changed. Tomi
Moi, On Wed, Mar 23, 2022 at 10:51:04AM +0200, Tomi Valkeinen wrote: > Hi Jacopo, > > On 24/02/2022 11:42, Jacopo Mondi wrote: > > v1: > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7249 > > v2: > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7311 > > v3: > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7385 > > v4: > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7389 > > > > A branch for testing based on the most recent media-master is available at > > https://git.sr.ht/~jmondi_/linux #jmondi/media-master/ov5640-v5 > > I tested these with DRA76 EVM & CAL, using CAL's legacy non-MC mode. It > doesn't work. I think there are two problems: > > - CAL uses mbus codes like MEDIA_BUS_FMT_UYVY8_2X8 for CSI-2, not 1X16. > OV5640 used to support 2X8, but now it doesn't. > > - OV5640 sets the default code to MEDIA_BUS_FMT_UYVY8_2X8, even for CSI-2 > where it doesn't support MEDIA_BUS_FMT_UYVY8_2X8. > > I'd like to just change CAL and drop the 2X8 support and instead use 1X16, > but then any sensor that uses 2X8 would work. So I guess I need to change > the code to support both. How many drivers there are using 2X8 formats on CSI-2? I remember there has been probably few, and it should be easy to fix them. It's another question whether the 2X8 format should be kept for compatibility. Perhaps the driver could allow setting it, but should then return 1X16 when queried.
Hi Tomi, On Wed, Mar 23, 2022 at 10:51:04AM +0200, Tomi Valkeinen wrote: > On 24/02/2022 11:42, Jacopo Mondi wrote: > > v1: > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7249 > > v2: > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7311 > > v3: > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7385 > > v4: > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7389 > > > > A branch for testing based on the most recent media-master is available at > > https://git.sr.ht/~jmondi_/linux #jmondi/media-master/ov5640-v5 > > I tested these with DRA76 EVM & CAL, using CAL's legacy non-MC mode. It > doesn't work. I think there are two problems: > > - CAL uses mbus codes like MEDIA_BUS_FMT_UYVY8_2X8 for CSI-2, not 1X16. > OV5640 used to support 2X8, but now it doesn't. > > - OV5640 sets the default code to MEDIA_BUS_FMT_UYVY8_2X8, even for > CSI-2 where it doesn't support MEDIA_BUS_FMT_UYVY8_2X8. > > I'd like to just change CAL and drop the 2X8 support and instead use > 1X16, but then any sensor that uses 2X8 would work. So I guess I need to > change the code to support both. We really need to phase out 2X8 for CSI-2. Can you add a warning in that case if you support both ? > Anyway, both of those issues might also surface on other platforms, as > ov5640 behavior has changed.
Hi Tomi thanks for testing On Wed, Mar 23, 2022 at 10:51:04AM +0200, Tomi Valkeinen wrote: > Hi Jacopo, > > On 24/02/2022 11:42, Jacopo Mondi wrote: > > v1: > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7249 > > v2: > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7311 > > v3: > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7385 > > v4: > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7389 > > > > A branch for testing based on the most recent media-master is available at > > https://git.sr.ht/~jmondi_/linux #jmondi/media-master/ov5640-v5 > > I tested these with DRA76 EVM & CAL, using CAL's legacy non-MC mode. It > doesn't work. I think there are two problems: > > - CAL uses mbus codes like MEDIA_BUS_FMT_UYVY8_2X8 for CSI-2, not 1X16. > OV5640 used to support 2X8, but now it doesn't. > > - OV5640 sets the default code to MEDIA_BUS_FMT_UYVY8_2X8, even for CSI-2 > where it doesn't support MEDIA_BUS_FMT_UYVY8_2X8. This might be worth an additional patch that decides what default format to use based on the bus type. > > I'd like to just change CAL and drop the 2X8 support and instead use 1X16, > but then any sensor that uses 2X8 would work. So I guess I need to change > the code to support both. > > Anyway, both of those issues might also surface on other platforms, as > ov5640 behavior has changed. > I'm afraid sooner or later this should have happened ? I think CSI-2 receivers should be updated, but I share your concerns about breaking other platforms. On one side we shouldn't be breaking userspace and this change might break some assumptions in users' pipeline configuration scripts and could prevent drivers that used to work together from being compatible at all. On the other side we would never be able to change anything at all if such a change is expected to happen atomically on all platforms and sensors. As the change is so trivial I guess it's fair to expect users of bridge drivers not compatible with 1X16 to fix them, but I cannot tell if it's an acceptable policy or not. As Sakari suggested we could also move all CSI-2 transmitters to use 1X16 and have receivers adjust as soon as someone detects a breakage. I can revert the change that restricts the enumerated format to the currently in use bus type[1] if desired, but I would prefer receivers to adjust when needed. Is this acceptable ? Thanks j [1] "media: ov5640: Split DVP and CSI-2 formats > Tomi
On Wed, Mar 23, 2022 at 10:50:19AM +0100, Jacopo Mondi wrote: > Hi Tomi thanks for testing > > On Wed, Mar 23, 2022 at 10:51:04AM +0200, Tomi Valkeinen wrote: > > Hi Jacopo, > > > > On 24/02/2022 11:42, Jacopo Mondi wrote: > > > v1: > > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7249 > > > v2: > > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7311 > > > v3: > > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7385 > > > v4: > > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7389 > > > > > > A branch for testing based on the most recent media-master is available at > > > https://git.sr.ht/~jmondi_/linux #jmondi/media-master/ov5640-v5 > > > > I tested these with DRA76 EVM & CAL, using CAL's legacy non-MC mode. It > > doesn't work. I think there are two problems: > > > > - CAL uses mbus codes like MEDIA_BUS_FMT_UYVY8_2X8 for CSI-2, not 1X16. > > OV5640 used to support 2X8, but now it doesn't. > > > > - OV5640 sets the default code to MEDIA_BUS_FMT_UYVY8_2X8, even for CSI-2 > > where it doesn't support MEDIA_BUS_FMT_UYVY8_2X8. > > This might be worth an additional patch that decides what default > format to use based on the bus type. > > > I'd like to just change CAL and drop the 2X8 support and instead use 1X16, > > but then any sensor that uses 2X8 would work. So I guess I need to change > > the code to support both. > > > > Anyway, both of those issues might also surface on other platforms, as > > ov5640 behavior has changed. > > I'm afraid sooner or later this should have happened ? > > I think CSI-2 receivers should be updated, but I share your concerns > about breaking other platforms. > > On one side we shouldn't be breaking userspace and this change might > break some assumptions in users' pipeline configuration scripts and > could prevent drivers that used to work together from being > compatible at all. > > On the other side we would never be able to change anything at all if > such a change is expected to happen atomically on all platforms and > sensors. > > As the change is so trivial I guess it's fair to expect users of > bridge drivers not compatible with 1X16 to fix them, but I cannot tell > if it's an acceptable policy or not. > > As Sakari suggested we could also move all CSI-2 transmitters to use 1X16 > and have receivers adjust as soon as someone detects a breakage. > > I can revert the change that restricts the enumerated format to the > currently in use bus type[1] if desired, but I would prefer receivers > to adjust when needed. Is this acceptable ? That would be my preference too. How about implementing Sakari's suggestion of turning the 2X8 formats into 1X16 in .set_fmt() for CSI-2 ? That way we'll minimize any risk of breakage for userspace. Host-side drivers that use the OV5640 will still need to be converted from 2X8 to 1X16, but that's in-kernel only and should be manageable. > [1] "media: ov5640: Split DVP and CSI-2 formats
Hi On Wed, Mar 23, 2022 at 12:41:47PM +0200, Laurent Pinchart wrote: > On Wed, Mar 23, 2022 at 10:50:19AM +0100, Jacopo Mondi wrote: > > Hi Tomi thanks for testing > > > > On Wed, Mar 23, 2022 at 10:51:04AM +0200, Tomi Valkeinen wrote: > > > Hi Jacopo, > > > > > > On 24/02/2022 11:42, Jacopo Mondi wrote: > > > > v1: > > > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7249 > > > > v2: > > > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7311 > > > > v3: > > > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7385 > > > > v4: > > > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7389 > > > > > > > > A branch for testing based on the most recent media-master is available at > > > > https://git.sr.ht/~jmondi_/linux #jmondi/media-master/ov5640-v5 > > > > > > I tested these with DRA76 EVM & CAL, using CAL's legacy non-MC mode. It > > > doesn't work. I think there are two problems: > > > > > > - CAL uses mbus codes like MEDIA_BUS_FMT_UYVY8_2X8 for CSI-2, not 1X16. > > > OV5640 used to support 2X8, but now it doesn't. > > > > > > - OV5640 sets the default code to MEDIA_BUS_FMT_UYVY8_2X8, even for CSI-2 > > > where it doesn't support MEDIA_BUS_FMT_UYVY8_2X8. > > > > This might be worth an additional patch that decides what default > > format to use based on the bus type. > > > > > I'd like to just change CAL and drop the 2X8 support and instead use 1X16, > > > but then any sensor that uses 2X8 would work. So I guess I need to change > > > the code to support both. > > > > > > Anyway, both of those issues might also surface on other platforms, as > > > ov5640 behavior has changed. > > > > I'm afraid sooner or later this should have happened ? > > > > I think CSI-2 receivers should be updated, but I share your concerns > > about breaking other platforms. > > > > On one side we shouldn't be breaking userspace and this change might > > break some assumptions in users' pipeline configuration scripts and > > could prevent drivers that used to work together from being > > compatible at all. > > > > On the other side we would never be able to change anything at all if > > such a change is expected to happen atomically on all platforms and > > sensors. > > > > As the change is so trivial I guess it's fair to expect users of > > bridge drivers not compatible with 1X16 to fix them, but I cannot tell > > if it's an acceptable policy or not. > > > > As Sakari suggested we could also move all CSI-2 transmitters to use 1X16 > > and have receivers adjust as soon as someone detects a breakage. > > > > I can revert the change that restricts the enumerated format to the > > currently in use bus type[1] if desired, but I would prefer receivers > > to adjust when needed. Is this acceptable ? > > That would be my preference too. How about implementing Sakari's > suggestion of turning the 2X8 formats into 1X16 in .set_fmt() for CSI-2 It's quite a tedious job, as for each driver that uses 2X8, if one doesn't know the device by name, searching online or in the driver for hints about what interface it uses (or discerning it from the manufacturing year) takes quite some time. I tried at the best of my knowledge, focusing on image sensor and ignoring video converters as most of them have an analogue output, for which I'm not sure what version is more opportune. The only outlier I have seen is ov5645, which is a CSI-2 sensor but uses 2X8. I've fixed that and will send a patch. > ? That way we'll minimize any risk of breakage for userspace. Host-side As the format will be adjusted, if drivers do not mess-up and select something completely different, we should be safer yes. It's a best-effort approach though. > drivers that use the OV5640 will still need to be converted from 2X8 to > 1X16, but that's in-kernel only and should be manageable. > Agreed, I don't think fixing all receivers should block this series (speaking of which: Sakari you have collected the series in one of your branches, but it is not part of the media pull request for v5.18-rc1. Should I expect this to be post-poned to v5.19 ?) Thanks j > > [1] "media: ov5640: Split DVP and CSI-2 formats > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Mar 28, 2022 at 04:57:28PM +0200, Jacopo Mondi wrote: > (speaking of which: Sakari you have collected the series in one of > your branches, but it is not part of the media pull request for > v5.18-rc1. Should I expect this to be post-poned to v5.19 ?) Yes.
Hi Jacopo, Many thanks for this huge work ! I have tested the serie on ST platform setup using OV5640 CSI-2. I have not yet tested the OV5640 parallel mode but will do also. Find below the main feedback, I have replied with more details on each related patches: 1) "2X8" mediabus code support broken, I have reverted the patch to continue testing 2) frame interval support dropped in flavour to vblank control: I have proposed a patch to support both 3) some resolutions/framerate not supported (MAX_VTS to increase) 4) JPEG framerate is divided by 2 with 1280x720@15, 1280x720@30, 1920x1080@15 => I have not found a relevant patch to overcome this, seems linked to the OV5640_LINK_RATE_MAX limit (mipi_div). RAW8 not tested but I can also (on my side, this is functional only for resolutions >= 720p). Here is the summary of resolutions/format tested with this serie and my patches on top: * QCIF 176x144 RGB565 15fps => OK, got 15 * QCIF 176x144 YUYV 15fps => OK, got 15 * QCIF 176x144 JPEG 15fps => OK, got 15 * QCIF 176x144 RGB565 30fps => OK, got 30 * QCIF 176x144 YUYV 30fps => OK, got 30 * QCIF 176x144 JPEG 30fps => OK, got 30 * QVGA 320x240 RGB565 15fps => OK, got 15 * QVGA 320x240 YUYV 15fps => OK, got 15 * QVGA 320x240 JPEG 15fps => OK, got 15 * QVGA 320x240 RGB565 30fps => OK, got 30 * QVGA 320x240 YUYV 30fps => OK, got 30 * QVGA 320x240 JPEG 30fps => OK, got 30 * VGA 640x480 RGB565 15fps => OK, got 15 * VGA 640x480 YUYV 15fps => OK, got 15 * VGA 640x480 JPEG 15fps => OK, got 15 * VGA 640x480 RGB565 30fps => OK, got 30 * VGA 640x480 YUYV 30fps => OK, got 30 * VGA 640x480 JPEG 30fps => OK, got 30 * 480p 720x480 RGB565 15fps => OK, got 15 * 480p 720x480 YUYV 15fps => OK, got 15 * 480p 720x480 JPEG 15fps => OK, got 15 * 480p 720x480 RGB565 30fps => OK, got 30 * 480p 720x480 YUYV 30fps => OK, got 30 * 480p 720x480 JPEG 30fps => OK, got 30 * XGA 1024x768 RGB565 15fps => OK, got 15 * XGA 1024x768 YUYV 15fps => OK, got 15 * XGA 1024x768 JPEG 15fps => OK, got 15 * XGA 1024x768 RGB565 30fps => OK, got 30 * XGA 1024x768 YUYV 30fps => OK, got 30 * XGA 1024x768 JPEG 30fps => OK, got 30 * 720p 1280x720 RGB565 15fps => OK, got 15 * 720p 1280x720 YUYV 15fps => OK, got 15 * 720p 1280x720 JPEG 15fps => KO: got 7 ===============================^^ [10917.171528] ov5640 1-003c: rate=62000000, freq=248000000, htot=1600, height=720, vblank=1863 * 720p 1280x720 RGB565 30fps => OK, got 30 * 720p 1280x720 YUYV 30fps => OK, got 30 * 720p 1280x720 JPEG 30fps => KO: got 15 ===============================^^ [10921.317180] ov5640 1-003c: rate=62000000, freq=248000000, htot=1600, height=720, vblank=560 * HD 1920x1080 RGB565 15fps => OK, got 15 * HD 1920x1080 YUYV 15fps => OK, got 15 * HD 1920x1080 JPEG 15fps => KO: got 7 ===============================^^ [10925.810657] ov5640 1-003c: rate=74000000, freq=296000000, htot=2234, height=1080, vblank=1128 * 5Mp 2592x1944 RGB565 15fps => OK, got 15 * 5Mp 2592x1944 YUYV 15fps => OK, got 15 * 5Mp 2592x1944 JPEG 15fps => OK, got 15 Best regards, Hugues. On 2/24/22 10:42 AM, Jacopo Mondi wrote: > v1: > https://patchwork.linuxtv.org/project/linux-media/list/?series=7249 > v2: > https://patchwork.linuxtv.org/project/linux-media/list/?series=7311 > v3: > https://patchwork.linuxtv.org/project/linux-media/list/?series=7385 > v4: > https://patchwork.linuxtv.org/project/linux-media/list/?series=7389 > > A branch for testing based on the most recent media-master is available at > https://git.sr.ht/~jmondi_/linux #jmondi/media-master/ov5640-v5 > > v5 (Sakari): > - Stay strictly in 80 cols > - use clamp_t to avoid explicit cast > - use ov5640_timings() where possible > > v4: > - Very minor update. Added tags and reworked enum_mbus_format as suggested > by Laurent. > > v3: > The series has now grown by 4 patches and the driver is now even larger > being the formats and the timings for DVP and CSI-2 defined separately. > > Tested in CSI-2 mode with UYVY, RGB565, SBGGR and RGB24 in all supported modes. > > Tested format and sizes enumeration with the new formats definition. > > Tested frame rate handling: > > vblank = ( duration msec * pixe_rate MHz / htot - height) > > 640x480 YUYV 15FPS (default 30 FPS) > > duration = 666666 msec > pixel_rate = 48 Mhz > htot = 1600 > vtot = 1999 > vblank = vtot - height = 1519 > > $ v4l2-ctl -d /dev/v4l-subdev4 -c 0x009e0901=1519 > $ yavta -f YUYV -s 640x480 -c100 --skip 7 /dev/video0 > ... > 10 (2) [-] any 11 614400 B 2189.317617 2189.317629 15.244 fps ts mono/EoF > 11 (3) [-] any 12 614400 B 2189.383212 2189.383224 15.245 fps ts mono/EoF > 12 (4) [-] any 13 614400 B 2189.448810 2189.448821 15.244 fps ts mono/EoF > 13 (5) [-] any 14 614400 B 2189.514405 2189.514417 15.245 fps ts mono/EoF > 14 (6) [-] any 15 614400 B 2189.580002 2189.580015 15.245 fps ts mono/EoF > .. > > 2592x1944 YUVV 15 FPS (default) > $ yavta -f YUYV -s 2592x1944 -c100 --skip 7 /dev/video0 > ... > 6 (6) [-] any 7 10077696 B 2438.377592 2438.377605 15.009 fps ts mono/EoF > 7 (7) [-] any 8 10077696 B 2438.444219 2438.444233 15.009 fps ts mono/EoF > 8 (0) [-] any 9 10077696 B 2438.510846 2438.510860 15.009 fps ts mono/EoF > 9 (1) [-] any 10 10077696 B 2438.577474 2438.577488 15.009 fps ts mono/EoF > 10 (2) [-] any 11 10077696 B 2438.644101 2438.644116 15.009 fps ts mono/EoF > 11 (3) [-] any 12 10077696 B 2438.710727 2438.710740 15.009 fps ts mono/EoF > 12 (4) [-] any 13 10077696 B 2438.777358 2438.777370 15.008 fps ts mono/EoF > 13 (5) [-] any 14 10077696 B 2438.843984 2438.843998 15.009 fps ts mono/EoF > 14 (6) [-] any 15 10077696 B 2438.910611 2438.910623 15.009 fps ts mono/EoF > 15 (7) [-] any 16 10077696 B 2438.977238 2438.977252 15.009 fps ts mono/EoF > 16 (0) [-] any 17 10077696 B 2439.043865 2439.043877 15.009 fps ts mono > ... > > > To enable higher FPS the LINK_FREQ control should be made writable to increase > the pixel rate > > 640x480 YUYV 60 FPS (pixel_rate = 96 Mhz) > > $ yavta -f YUYV -s 640x480 -c100 --skip 7 /dev/video0 > ... > 9 (1) [-] any 10 614400 B 57.098649 57.098667 59.995 fps ts mono/EoF > 10 (2) [-] any 11 614400 B 57.115314 57.115332 60.006 fps ts mono/EoF > 11 (3) [-] any 12 614400 B 57.131978 57.131994 60.010 fps ts mono/EoF > 12 (4) [-] any 13 614400 B 57.148645 57.148664 59.999 fps ts mono/EoF > 13 (5) [-] any 14 614400 B 57.165310 57.165328 60.006 fps ts mono/EoF > 14 (6) [-] any 15 614400 B 57.181977 57.181996 59.999 fps ts mono/EoF > 15 (7) [-] any 16 614400 B 57.198642 57.198660 60.006 fps ts mono/EoF > > Changelog: > > v2->v3: > > - Eugen (thanks) reported regression in DVP mode :( > To maintain the DVP timings un-changed in this version the mode definition now > looks like > > /* 640x480 */ > .id = OV5640_MODE_VGA_640_480, > .dn_mode = SUBSAMPLING, > .pixel_rate = OV5640_PIXEL_RATE_48M, > .width = 640, > .height = 480, > .dvp_timings = { > .analog_crop = { > .left = 0, > .top = 4, > .width = 2624, > .height = 1944, > }, > .crop = { > .left = 16, > .top = 6, > .width = 640, > .height = 480, > }, > .htot = 1896, > .vblank_def = 600, > .max_fps = OV5640_60_FPS > }, > .csi2_timings = { > .analog_crop = { > /* Feed the full valid pixel array to the ISP. */ > .left = OV5640_PIXEL_ARRAY_LEFT, > .top = OV5640_PIXEL_ARRAY_TOP, > .width = OV5640_PIXEL_ARRAY_WIDTH, > .height = OV5640_PIXEL_ARRAY_HEIGHT, > }, > .crop = { > /* Maintain a minimum digital crop processing margins. */ > .left = 2, > .top = 4, > .width = 640, > .height = 480, > }, > .htot = 1600, > .vblank_def = 520, > }, > .reg_data = ov5640_setting_low_res, > .reg_data_size = ARRAY_SIZE(ov5640_setting_low_res), > > with a .dvp_timings and a .csi2_timings members to separate the two. > Is it nice ? No it's not, but it should help maintaining DVP users happy. > > Eugen: if you are willing to run another test round to confirm if this version > does not regress DVP it would be great :) > > - Split image formats between CSI-2 and DVP > - Remove RGB888 as per the CSIS discussion with Laurent > - Removed register tables for modes < 720 as they're all equal > - Minor fixes on Laurent's comments > - Add Adam's tag > > v1 -> v2: > - rework the modes definition to process the full pixel array > - rework get_selection to report the correct BOUND and DEFAULT targets > - implement init_cfg > - minor style changes as suggested by Laurent > - test with 1 data lane > > Jacopo Mondi (27): > media: ov5640: Add pixel rate to modes > media: ov5604: Re-arrange modes definition > media: ov5640: Add ov5640_is_csi2() function > media: ov5640: Associate bpp with formats > media: ov5640: Add LINK_FREQ control > media: ov5640: Update pixel_rate and link_freq > media: ov5640: Rework CSI-2 clock tree > media: ov5640: Rework timings programming > media: ov5640: Fix 720x480 in RGB888 mode > media: ov5640: Split DVP and CSI-2 timings > media: ov5640: Provide timings accessor > media: ov5640: Re-sort per-mode register tables > media: ov5640: Remove duplicated mode settings > media: ov5640: Remove ov5640_mode_init_data > media: ov5640: Add HBLANK control > media: ov5640: Add VBLANK control > media: ov5640: Change CSI-2 timings to comply with FPS > media: ov5640: Implement init_cfg > media: ov5640: Implement get_selection > media: ov5640: Limit frame_interval to DVP mode only > media: ov5640: Register device properties > media: ov5640: Add RGB565_1X16 format > media: ov5640: Add BGR888 format > media: ov5640: Restrict sizes to mbus code > media: ov5640: Adjust format to bpp in s_fmt > media: ov5640: Split DVP and CSI-2 formats > media: ov5640: Move format mux config in format > > drivers/media/i2c/ov5640.c | 1615 ++++++++++++++++++++++++++---------- > 1 file changed, 1160 insertions(+), 455 deletions(-) > > -- > 2.35.0 > > >
Hi Jacopo, On 3/23/22 10:50 AM, Jacopo Mondi wrote: > Hi Tomi thanks for testing > > On Wed, Mar 23, 2022 at 10:51:04AM +0200, Tomi Valkeinen wrote: >> Hi Jacopo, >> >> On 24/02/2022 11:42, Jacopo Mondi wrote: >>> v1: >>> https://patchwork.linuxtv.org/project/linux-media/list/?series=7249 >>> v2: >>> https://patchwork.linuxtv.org/project/linux-media/list/?series=7311 >>> v3: >>> https://patchwork.linuxtv.org/project/linux-media/list/?series=7385 >>> v4: >>> https://patchwork.linuxtv.org/project/linux-media/list/?series=7389 >>> >>> A branch for testing based on the most recent media-master is available at >>> https://git.sr.ht/~jmondi_/linux #jmondi/media-master/ov5640-v5 >> >> I tested these with DRA76 EVM & CAL, using CAL's legacy non-MC mode. It >> doesn't work. I think there are two problems: >> >> - CAL uses mbus codes like MEDIA_BUS_FMT_UYVY8_2X8 for CSI-2, not 1X16. >> OV5640 used to support 2X8, but now it doesn't. >> >> - OV5640 sets the default code to MEDIA_BUS_FMT_UYVY8_2X8, even for CSI-2 >> where it doesn't support MEDIA_BUS_FMT_UYVY8_2X8. > > This might be worth an additional patch that decides what default > format to use based on the bus type. > >> >> I'd like to just change CAL and drop the 2X8 support and instead use 1X16, >> but then any sensor that uses 2X8 would work. So I guess I need to change >> the code to support both. >> >> Anyway, both of those issues might also surface on other platforms, as >> ov5640 behavior has changed. >> I am facing the same "2X8" compatibility problem on ST platforms: - st-mipid02 CSI-2 to parallel bridge [1] must be enhanced to support 1X16 formats - dcmi controller [2] must also be enhanced to support 1X16 and extra code to support 1X16 input to 2X8 output (as we only have as input the V4L2 format, not the mediabus one) => with current code, support with OV5640 is broken. I feel that your proposal to let OV5640 accept 2X8 then silently switch to 1X16 can do the job without breaking dcmi/bridge support but need further testing to confirm. Appart from that I don't really understand the logic behind naming "1X16" for CSI-2 serial formats, if "2X8" means 2 bytes to send one pixel, I would expect that "1X16" means 1 word to send one pixel (16bits wide bus), so how to differentiate 16 bits // code from CSI-2 code ? For the time being I have reverted this commit in order to test the other topics of this patchset. [1] drivers/media/i2c/st-mipid02.c [2] drivers/media/platform/stm32/stm32-dcmi.c > > I'm afraid sooner or later this should have happened ? > > I think CSI-2 receivers should be updated, but I share your concerns > about breaking other platforms. > > On one side we shouldn't be breaking userspace and this change might > break some assumptions in users' pipeline configuration scripts and > could prevent drivers that used to work together from being > compatible at all. > > On the other side we would never be able to change anything at all if > such a change is expected to happen atomically on all platforms and > sensors. > > As the change is so trivial I guess it's fair to expect users of > bridge drivers not compatible with 1X16 to fix them, but I cannot tell > if it's an acceptable policy or not. > > As Sakari suggested we could also move all CSI-2 transmitters to use 1X16 > and have receivers adjust as soon as someone detects a breakage. > > I can revert the change that restricts the enumerated format to the > currently in use bus type[1] if desired, but I would prefer receivers > to adjust when needed. Is this acceptable ? > > Thanks > j > > [1] "media: ov5640: Split DVP and CSI-2 formats > >> Tomi > > Best regards, Hugues.
Hi Hugues, On Thu, Apr 07, 2022 at 06:25:11PM +0200, Hugues FRUCHET - FOSS wrote: > Hi Jacopo, > > On 3/23/22 10:50 AM, Jacopo Mondi wrote: > > Hi Tomi thanks for testing > > > > On Wed, Mar 23, 2022 at 10:51:04AM +0200, Tomi Valkeinen wrote: > > > Hi Jacopo, > > > > > > On 24/02/2022 11:42, Jacopo Mondi wrote: > > > > v1: > > > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7249 > > > > v2: > > > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7311 > > > > v3: > > > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7385 > > > > v4: > > > > https://patchwork.linuxtv.org/project/linux-media/list/?series=7389 > > > > > > > > A branch for testing based on the most recent media-master is available at > > > > https://git.sr.ht/~jmondi_/linux #jmondi/media-master/ov5640-v5 > > > > > > I tested these with DRA76 EVM & CAL, using CAL's legacy non-MC mode. It > > > doesn't work. I think there are two problems: > > > > > > - CAL uses mbus codes like MEDIA_BUS_FMT_UYVY8_2X8 for CSI-2, not 1X16. > > > OV5640 used to support 2X8, but now it doesn't. > > > > > > - OV5640 sets the default code to MEDIA_BUS_FMT_UYVY8_2X8, even for CSI-2 > > > where it doesn't support MEDIA_BUS_FMT_UYVY8_2X8. > > > > This might be worth an additional patch that decides what default > > format to use based on the bus type. > > > > > > > > I'd like to just change CAL and drop the 2X8 support and instead use 1X16, > > > but then any sensor that uses 2X8 would work. So I guess I need to change > > > the code to support both. > > > > > > Anyway, both of those issues might also surface on other platforms, as > > > ov5640 behavior has changed. > > > > > I am facing the same "2X8" compatibility problem on ST platforms: > - st-mipid02 CSI-2 to parallel bridge [1] must be enhanced to support 1X16 > formats > - dcmi controller [2] must also be enhanced to support 1X16 and extra code > to support 1X16 input to 2X8 output (as we only have as input the V4L2 > format, not the mediabus one) > => with current code, support with OV5640 is broken. > > I feel that your proposal to let OV5640 accept 2X8 then silently switch to > 1X16 can do the job without breaking dcmi/bridge support but need further > testing to confirm. > > Appart from that I don't really understand the logic behind naming "1X16" > for CSI-2 serial formats, if "2X8" means 2 bytes to send one pixel, I would > expect that "1X16" means 1 word to send one pixel (16bits wide bus), so how > to differentiate 16 bits // code from CSI-2 code ? Please see: <URL:https://hverkuil.home.xs4all.nl/spec/userspace-api/v4l/subdev-formats.html#v4l2-mbus-pixelcode> I.e. st-mipid02 and dcmi drivers should be fixed.
Hi Sakari, On 4/8/22 1:05 PM, Sakari Ailus wrote: > Hi Hugues, > > On Thu, Apr 07, 2022 at 06:25:11PM +0200, Hugues FRUCHET - FOSS wrote: >> Hi Jacopo, >> >> On 3/23/22 10:50 AM, Jacopo Mondi wrote: >>> Hi Tomi thanks for testing >>> >>> On Wed, Mar 23, 2022 at 10:51:04AM +0200, Tomi Valkeinen wrote: >>>> Hi Jacopo, >>>> >>>> On 24/02/2022 11:42, Jacopo Mondi wrote: >>>>> v1: >>>>> https://patchwork.linuxtv.org/project/linux-media/list/?series=7249 >>>>> v2: >>>>> https://patchwork.linuxtv.org/project/linux-media/list/?series=7311 >>>>> v3: >>>>> https://patchwork.linuxtv.org/project/linux-media/list/?series=7385 >>>>> v4: >>>>> https://patchwork.linuxtv.org/project/linux-media/list/?series=7389 >>>>> >>>>> A branch for testing based on the most recent media-master is available at >>>>> https://git.sr.ht/~jmondi_/linux #jmondi/media-master/ov5640-v5 >>>> >>>> I tested these with DRA76 EVM & CAL, using CAL's legacy non-MC mode. It >>>> doesn't work. I think there are two problems: >>>> >>>> - CAL uses mbus codes like MEDIA_BUS_FMT_UYVY8_2X8 for CSI-2, not 1X16. >>>> OV5640 used to support 2X8, but now it doesn't. >>>> >>>> - OV5640 sets the default code to MEDIA_BUS_FMT_UYVY8_2X8, even for CSI-2 >>>> where it doesn't support MEDIA_BUS_FMT_UYVY8_2X8. >>> >>> This might be worth an additional patch that decides what default >>> format to use based on the bus type. >>> >>>> >>>> I'd like to just change CAL and drop the 2X8 support and instead use 1X16, >>>> but then any sensor that uses 2X8 would work. So I guess I need to change >>>> the code to support both. >>>> >>>> Anyway, both of those issues might also surface on other platforms, as >>>> ov5640 behavior has changed. >>>> >> >> I am facing the same "2X8" compatibility problem on ST platforms: >> - st-mipid02 CSI-2 to parallel bridge [1] must be enhanced to support 1X16 >> formats >> - dcmi controller [2] must also be enhanced to support 1X16 and extra code >> to support 1X16 input to 2X8 output (as we only have as input the V4L2 >> format, not the mediabus one) >> => with current code, support with OV5640 is broken. >> >> I feel that your proposal to let OV5640 accept 2X8 then silently switch to >> 1X16 can do the job without breaking dcmi/bridge support but need further >> testing to confirm. >> >> Appart from that I don't really understand the logic behind naming "1X16" >> for CSI-2 serial formats, if "2X8" means 2 bytes to send one pixel, I would >> expect that "1X16" means 1 word to send one pixel (16bits wide bus), so how >> to differentiate 16 bits // code from CSI-2 code ? > > Please see: > > <URL:https://hverkuil.home.xs4all.nl/spec/userspace-api/v4l/subdev-formats.html#v4l2-mbus-pixelcode> > > I.e. st-mipid02 and dcmi drivers should be fixed. > OK, so "single clock cycle" convention for serial bus (copy/paste here for the sake of clarity): 4.13.3.4.1.1. Media Bus Pixel Codes The media bus pixel codes document parallel formats. Should the pixel data be transported over a serial bus, the media bus pixel code that describes a parallel format that transfers a sample on a single clock cycle is used. For instance, both MEDIA_BUS_FMT_BGR888_1X24 and MEDIA_BUS_FMT_BGR888_3X8 are used on parallel busses for transferring an 8 bits per sample BGR data, whereas on serial busses the data in this format is only referred to using MEDIA_BUS_FMT_BGR888_1X24. This is because there is effectively only a single way to transport that format on the serial busses. I'll try to change both dcmi and mipid02 in that way... Best regards, Hugues.
v1: https://patchwork.linuxtv.org/project/linux-media/list/?series=7249 v2: https://patchwork.linuxtv.org/project/linux-media/list/?series=7311 v3: https://patchwork.linuxtv.org/project/linux-media/list/?series=7385 v4: https://patchwork.linuxtv.org/project/linux-media/list/?series=7389 A branch for testing based on the most recent media-master is available at https://git.sr.ht/~jmondi_/linux #jmondi/media-master/ov5640-v5 v5 (Sakari): - Stay strictly in 80 cols - use clamp_t to avoid explicit cast - use ov5640_timings() where possible v4: - Very minor update. Added tags and reworked enum_mbus_format as suggested by Laurent. v3: The series has now grown by 4 patches and the driver is now even larger being the formats and the timings for DVP and CSI-2 defined separately. Tested in CSI-2 mode with UYVY, RGB565, SBGGR and RGB24 in all supported modes. Tested format and sizes enumeration with the new formats definition. Tested frame rate handling: vblank = ( duration msec * pixe_rate MHz / htot - height) 640x480 YUYV 15FPS (default 30 FPS) duration = 666666 msec pixel_rate = 48 Mhz htot = 1600 vtot = 1999 vblank = vtot - height = 1519 $ v4l2-ctl -d /dev/v4l-subdev4 -c 0x009e0901=1519 $ yavta -f YUYV -s 640x480 -c100 --skip 7 /dev/video0 ... 10 (2) [-] any 11 614400 B 2189.317617 2189.317629 15.244 fps ts mono/EoF 11 (3) [-] any 12 614400 B 2189.383212 2189.383224 15.245 fps ts mono/EoF 12 (4) [-] any 13 614400 B 2189.448810 2189.448821 15.244 fps ts mono/EoF 13 (5) [-] any 14 614400 B 2189.514405 2189.514417 15.245 fps ts mono/EoF 14 (6) [-] any 15 614400 B 2189.580002 2189.580015 15.245 fps ts mono/EoF .. 2592x1944 YUVV 15 FPS (default) $ yavta -f YUYV -s 2592x1944 -c100 --skip 7 /dev/video0 ... 6 (6) [-] any 7 10077696 B 2438.377592 2438.377605 15.009 fps ts mono/EoF 7 (7) [-] any 8 10077696 B 2438.444219 2438.444233 15.009 fps ts mono/EoF 8 (0) [-] any 9 10077696 B 2438.510846 2438.510860 15.009 fps ts mono/EoF 9 (1) [-] any 10 10077696 B 2438.577474 2438.577488 15.009 fps ts mono/EoF 10 (2) [-] any 11 10077696 B 2438.644101 2438.644116 15.009 fps ts mono/EoF 11 (3) [-] any 12 10077696 B 2438.710727 2438.710740 15.009 fps ts mono/EoF 12 (4) [-] any 13 10077696 B 2438.777358 2438.777370 15.008 fps ts mono/EoF 13 (5) [-] any 14 10077696 B 2438.843984 2438.843998 15.009 fps ts mono/EoF 14 (6) [-] any 15 10077696 B 2438.910611 2438.910623 15.009 fps ts mono/EoF 15 (7) [-] any 16 10077696 B 2438.977238 2438.977252 15.009 fps ts mono/EoF 16 (0) [-] any 17 10077696 B 2439.043865 2439.043877 15.009 fps ts mono ... To enable higher FPS the LINK_FREQ control should be made writable to increase the pixel rate 640x480 YUYV 60 FPS (pixel_rate = 96 Mhz) $ yavta -f YUYV -s 640x480 -c100 --skip 7 /dev/video0 ... 9 (1) [-] any 10 614400 B 57.098649 57.098667 59.995 fps ts mono/EoF 10 (2) [-] any 11 614400 B 57.115314 57.115332 60.006 fps ts mono/EoF 11 (3) [-] any 12 614400 B 57.131978 57.131994 60.010 fps ts mono/EoF 12 (4) [-] any 13 614400 B 57.148645 57.148664 59.999 fps ts mono/EoF 13 (5) [-] any 14 614400 B 57.165310 57.165328 60.006 fps ts mono/EoF 14 (6) [-] any 15 614400 B 57.181977 57.181996 59.999 fps ts mono/EoF 15 (7) [-] any 16 614400 B 57.198642 57.198660 60.006 fps ts mono/EoF Changelog: v2->v3: - Eugen (thanks) reported regression in DVP mode :( To maintain the DVP timings un-changed in this version the mode definition now looks like /* 640x480 */ .id = OV5640_MODE_VGA_640_480, .dn_mode = SUBSAMPLING, .pixel_rate = OV5640_PIXEL_RATE_48M, .width = 640, .height = 480, .dvp_timings = { .analog_crop = { .left = 0, .top = 4, .width = 2624, .height = 1944, }, .crop = { .left = 16, .top = 6, .width = 640, .height = 480, }, .htot = 1896, .vblank_def = 600, .max_fps = OV5640_60_FPS }, .csi2_timings = { .analog_crop = { /* Feed the full valid pixel array to the ISP. */ .left = OV5640_PIXEL_ARRAY_LEFT, .top = OV5640_PIXEL_ARRAY_TOP, .width = OV5640_PIXEL_ARRAY_WIDTH, .height = OV5640_PIXEL_ARRAY_HEIGHT, }, .crop = { /* Maintain a minimum digital crop processing margins. */ .left = 2, .top = 4, .width = 640, .height = 480, }, .htot = 1600, .vblank_def = 520, }, .reg_data = ov5640_setting_low_res, .reg_data_size = ARRAY_SIZE(ov5640_setting_low_res), with a .dvp_timings and a .csi2_timings members to separate the two. Is it nice ? No it's not, but it should help maintaining DVP users happy. Eugen: if you are willing to run another test round to confirm if this version does not regress DVP it would be great :) - Split image formats between CSI-2 and DVP - Remove RGB888 as per the CSIS discussion with Laurent - Removed register tables for modes < 720 as they're all equal - Minor fixes on Laurent's comments - Add Adam's tag v1 -> v2: - rework the modes definition to process the full pixel array - rework get_selection to report the correct BOUND and DEFAULT targets - implement init_cfg - minor style changes as suggested by Laurent - test with 1 data lane Jacopo Mondi (27): media: ov5640: Add pixel rate to modes media: ov5604: Re-arrange modes definition media: ov5640: Add ov5640_is_csi2() function media: ov5640: Associate bpp with formats media: ov5640: Add LINK_FREQ control media: ov5640: Update pixel_rate and link_freq media: ov5640: Rework CSI-2 clock tree media: ov5640: Rework timings programming media: ov5640: Fix 720x480 in RGB888 mode media: ov5640: Split DVP and CSI-2 timings media: ov5640: Provide timings accessor media: ov5640: Re-sort per-mode register tables media: ov5640: Remove duplicated mode settings media: ov5640: Remove ov5640_mode_init_data media: ov5640: Add HBLANK control media: ov5640: Add VBLANK control media: ov5640: Change CSI-2 timings to comply with FPS media: ov5640: Implement init_cfg media: ov5640: Implement get_selection media: ov5640: Limit frame_interval to DVP mode only media: ov5640: Register device properties media: ov5640: Add RGB565_1X16 format media: ov5640: Add BGR888 format media: ov5640: Restrict sizes to mbus code media: ov5640: Adjust format to bpp in s_fmt media: ov5640: Split DVP and CSI-2 formats media: ov5640: Move format mux config in format drivers/media/i2c/ov5640.c | 1615 ++++++++++++++++++++++++++---------- 1 file changed, 1160 insertions(+), 455 deletions(-) -- 2.35.0