mbox series

[v5,00/27] media: ov5640: Rework the clock tree programming for MIPI

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

Message

Jacopo Mondi Feb. 24, 2022, 9:42 a.m. UTC
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

Comments

Hugues Fruchet April 7, 2022, 4:24 p.m. UTC | #1
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
> 
> 
>
Hugues Fruchet April 7, 2022, 4:25 p.m. UTC | #2
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.
Sakari Ailus April 8, 2022, 11:05 a.m. UTC | #3
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.
Hugues Fruchet April 26, 2022, 12:32 p.m. UTC | #4
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.