Message ID | 20220224094313.233347-1-jacopo@jmondi.org |
---|---|
Headers | show |
Series | media: ov5640: Rework the clock tree programming for MIPI | expand |
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.