Add parallel interface (DVP) support for ov5640 driver #76124

CharlesDias · 2024-07-19T22:15:52Z

Related to the issue #74353.

These PRs are requires:

This PR is related to:

boards: shields: add suport for st_b_cams_omv_mb1683 #76143

Testing on STM32H7B3I-DK

~~west build -p -b stm32h7b3i_dk --shield st_b_cams_omv_mb1683 samples/subsys/video/capture_to_lvgl/~~
west build -p -b stm32h7b3i_dk --shield st_b_cams_omv_mb1683 samples/drivers/video/capture_to_lvgl/

document_5147857391924020551.mp4

CharlesDias · 2024-07-21T21:31:50Z

Dear,

Please, @ngphibang, @josuah, @erwango, and @loicpoulain, could you take a look at the draft PR to check if I am on the right path?

Thanks!

josuah

Thank you for this addition!
Progressively, more features of these complex sensors can be integrated from Linux and other sources into Zephyr.

This looks like almost ready to go. See some few feedback inline. Feel free to dismiss it or address it as relevant.

drivers/video/ov5640.c

josuah · 2024-07-21T22:20:43Z

drivers/video/ov5640.c

 	if (ret) {
-		LOG_ERR("Unable to set virtual channel");
+		LOG_ERR("Unable to initialize the sensor with DVP parameters");
 		return -EIO;
 	}


This looks all correct, but it would be less confusing to have it inside the if to group it semantically.

drivers/video/ov5640.c

josuah · 2024-07-21T22:28:04Z

drivers/video/ov5640.c

-	fmt.width = 1280;
-	fmt.height = 720;
+	if (ov5640_is_dvp(dev)) {
+		/* Set default format to 480x272 RGB565 */


Comment: This looks like one particular display rather than a common resolution format, though it is also good that the defaults did get tested on actual hardware.

Since this version of the DVP feature does not support the current resolution configuration (1280x720), it would be better to set it up for the minimal resolution supported by DVP.

Ah right I understand better now thank you.
Did you try various resolutions and noticed that 480x272 RGB565 was the highest one supported by DVP?
I do not see this resolution anywhere on Linux or the datasheet.
Just curious. :)

Ah right I understand better now thank you.
Did you try various resolutions and noticed that 480x272 RGB565 was the highest one supported by DVP?
I do not see this resolution anywhere on Linux or the datasheet.
Just curious. :)

DVP should support resolutions higher than 480x272, but due to my board's memory (I'm using only the internal SRAM) and display limitations, I was able to test 160x120, 320x240, and 480x272.

For future version, I'll try to use the external SDRAM and others resolution.

drivers/video/ov5640.c

josuah · 2024-07-22T01:29:37Z

This might need rebasing once #76144 (not in draft state) is merged.
Though, this is currently feature-freeze for v3.7.0

ngphibang · 2024-07-22T15:31:40Z

dts/bindings/video/ovti,ov5640.yaml

@@ -5,7 +5,7 @@ description: OV5640 CMOS video sensor

 compatible: "ovti,ov5640"

-include: i2c-device.yaml
+include: [i2c-device.yaml, video-interfaces.yaml]


I think this will not have any effects as the properties defined in video-interfaces.yaml are for an endpoint node, not for the ov5640 node itself. The difficulty here (and the reason why I let #74415 always as draft) is that we cannot know the endpoint is actually a child or a grand child node of the video device node (e.g. ov5640) as ports node is optional, so that we don't know if we should use 1 or 2 levels of child-binding in the video-interfaces.yaml:

child-binding:
child-binding:

So, I think the solution for now is that either we:

Don't include video-interfaces.yaml, and just declare the properties in the devicetree. It will compile without any problem. The video-interfaces.yaml will still serve as a reference.

Or add compatible: "nxp,zephyr-endpoint" in the endpoint node and in video-interfaces.yaml

@josuah Do you have a solution for this ?

Dear, @ngphibang and @josuah,

If I change from include: [i2c-device.yaml, video-interfaces.yaml] to include: i2c-device.yaml, I got the compile error below:

devicetree error: 'bus-type' appears in /soc/i2c@58001c00/ov5640@3c in /home/charlesdias/zephyrproject/zephyr/build/zephyr/zephyr.dts.pre, but is not declared in 'properties:' in /home/charlesdias/zephyrproject/zephyr/dts/bindings/video/ovti,ov5640.yaml

I think that the video-interfaces.yaml is making an effect on the ov5640 node. Does this make sense?

I think it's because you declared these properties on the ov5640 node. All the properties in video-interfaces.yaml should be declared in the endpoint subnode (together with remote-endpoint)

CharlesDias · 2024-07-22T21:01:15Z

Thanks, @josuah and @ngphibang for your suggestions. I'll fix them during the weekend.

ngphibang · 2024-07-22T23:18:38Z

@CharlesDias Thanks for adding this dvp mode to the ov5640 driver. As being said last time in your raised issue, we were also working on supporting changing frame rates and some controls to the ov5640 driver at the same time. Here is the PR. Sorry that we couldn't push our PR earlier to facilitate your work due to some difficulties in the impelmentation.

If possible, could you take into account our changes ? (IMO, some commits in our PR could make the dvp support easier and your PR is a bigger change so it seems easier to rebase your change on ours than vice versa ?)

I am available for any helps if needed.

CharlesDias · 2024-07-24T10:50:20Z

If possible, could you take into account our changes ? (IMO, some commits in our PR could make the dvp support easier and your PR is a bigger change so it seems easier to rebase your change on ours than vice versa ?)

For sure, @ngphibang, I'll do the rebase by the end of the week. Thanks for being available!

Run clang format before making any changes. Signed-off-by: Phi Bang Nguyen <[email protected]>

Add some control IDs: - VIDEO_CID_CAMERA_HUE - VIDEO_CID_POWER_LINE_FREQUENCY - VIDEO_CID_PIXEL_RATE which is needed for changing frame rate Signed-off-by: Phi Bang Nguyen <[email protected]>

Color bar is one type of test patterns. Rename it to VIDEO_CID_CAMERA_TEST_PATTERN to be more generic. Signed-off-by: Phi Bang Nguyen <[email protected]>

Add some minor fixes and optimizations: - Fix coding style - Rename some variables - Use the array variable size directly instead of defining a macro Signed-off-by: Phi Bang Nguyen <[email protected]> Signed-off-by: Trung Hieu Le <[email protected]>

Add support for 4 test pattern modes: - Color bar - Color bar rolling - Square - Square rolling Signed-off-by: Phi Bang Nguyen <[email protected]>

Add some controls: - hue - saturation - brightness - contrast - gain - hflip - vflip - power line frequency filter - pixel rate (read-only) which is needed for changing frame rate Signed-off-by: Farah Fliss <[email protected]> Signed-off-by: Phi Bang Nguyen <[email protected]>

Add support for changing frame rate Signed-off-by: Trung Hieu Le <[email protected]> Signed-off-by: Phi Bang Nguyen <[email protected]>

CharlesDias · 2024-09-14T21:21:35Z

Rebase with #76144

Improve the ov5640 video driver to provide parallel interface (DVP) support Signed-off-by: Charles Dias <[email protected]>

CharlesDias · 2024-09-14T21:24:42Z

Apply review suggestions.

josuah

Thank you for this other cycle of modifications!

One remark as I learn the new .bus_type API myself.
If you wish it is possible to convert the PR to a draft and remove [DNM] to say that this is a work in progress not to be merged yet.

drivers/video/ov5640.c

CharlesDias · 2024-09-16T12:59:39Z

If you wish it is possible to convert the PR to a draft and remove [DNM] to say that this is a work in progress not to be merged

Hi @josuah, I considered this initial version ready to go (apart from the reviewer's suggestions and the PR dependencies). I added the [DNM] to avoid merging it before the dependencies. I'm not sure if this is the right approach.

josuah · 2024-09-16T13:44:39Z

initial version ready [...] avoid merging it before the dependencies [...] the right approach

The DNM label gives me a hint that this might be made for this purpose but I would not bet an arm on it...
Learning a bit every day!

CharlesDias changed the title ~~Ov5640 add parallel interface~~ Improve the ov5640 video driver to provide parallel interface (DVP) support Jul 19, 2024

CharlesDias marked this pull request as draft July 19, 2024 22:16

zephyrbot added the area: Video Video subsystem label Jul 19, 2024

zephyrbot requested a review from loicpoulain July 19, 2024 22:16

CharlesDias mentioned this pull request Jul 19, 2024

Improve the ov5640 video driver to provide parallel interface (DVP) support. #74353

Open

3 tasks

CharlesDias force-pushed the ov5640_add_parallel_interface branch from 635e3a1 to 73e0ea7 Compare July 21, 2024 19:36

CharlesDias changed the title ~~Improve the ov5640 video driver to provide parallel interface (DVP) support~~ Add parallel interface (DVP) support for ov5640 driver Jul 21, 2024

CharlesDias force-pushed the ov5640_add_parallel_interface branch from 73e0ea7 to 77c2eeb Compare July 21, 2024 19:57

josuah reviewed Jul 21, 2024

View reviewed changes

ngphibang reviewed Jul 22, 2024

View reviewed changes

CharlesDias mentioned this pull request Jul 22, 2024

boards: shields: add suport for st_b_cams_omv_mb1683 #76143

Open

josuah mentioned this pull request Jul 27, 2024

drivers: video: image sensor list (from Linux, others) #76378

Open

kartben self-requested a review August 12, 2024 22:14

ngphibang and others added 7 commits September 11, 2024 10:15

drivers: video-controls: Run clang-format

3092199

Run clang format before making any changes. Signed-off-by: Phi Bang Nguyen <[email protected]>

drivers: video-controls: Add some control IDs

25de21e

Add some control IDs: - VIDEO_CID_CAMERA_HUE - VIDEO_CID_POWER_LINE_FREQUENCY - VIDEO_CID_PIXEL_RATE which is needed for changing frame rate Signed-off-by: Phi Bang Nguyen <[email protected]>

drivers: video-controls: Rename colorbar to test pattern

1b43c6d

Color bar is one type of test patterns. Rename it to VIDEO_CID_CAMERA_TEST_PATTERN to be more generic. Signed-off-by: Phi Bang Nguyen <[email protected]>

drivers: video: ov5640: Add support for test patterns

4aae841

Add support for 4 test pattern modes: - Color bar - Color bar rolling - Square - Square rolling Signed-off-by: Phi Bang Nguyen <[email protected]>

drivers: video: ov5640: Add support for changing frame rate

65eb6a8

Add support for changing frame rate Signed-off-by: Trung Hieu Le <[email protected]> Signed-off-by: Phi Bang Nguyen <[email protected]>

CharlesDias force-pushed the ov5640_add_parallel_interface branch from 77c2eeb to c0038bd Compare September 14, 2024 21:20

drivers: video: add DVP support to ov5640

7e2dfdf

Improve the ov5640 video driver to provide parallel interface (DVP) support Signed-off-by: Charles Dias <[email protected]>

CharlesDias force-pushed the ov5640_add_parallel_interface branch from c0038bd to 7e2dfdf Compare September 14, 2024 21:24

CharlesDias changed the title ~~Add parallel interface (DVP) support for ov5640 driver~~ [DNM] Add parallel interface (DVP) support for ov5640 driver Sep 14, 2024

CharlesDias marked this pull request as ready for review September 14, 2024 21:48

josuah reviewed Sep 15, 2024

View reviewed changes

drivers/video/ov5640.c Show resolved Hide resolved

drivers/video/ov5640.c Show resolved Hide resolved

drivers/video/ov5640.c Show resolved Hide resolved

drivers/video/ov5640.c Outdated Show resolved Hide resolved

josuah added the DNM This PR should not be merged (Do Not Merge) label Sep 16, 2024

josuah changed the title ~~[DNM] Add parallel interface (DVP) support for ov5640 driver~~ Add parallel interface (DVP) support for ov5640 driver Sep 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add parallel interface (DVP) support for ov5640 driver #76124

Add parallel interface (DVP) support for ov5640 driver #76124

CharlesDias commented Jul 19, 2024 •

edited

Loading

CharlesDias commented Jul 21, 2024

josuah left a comment

josuah Jul 21, 2024

CharlesDias Sep 14, 2024

josuah Jul 21, 2024

CharlesDias Sep 14, 2024

josuah Sep 15, 2024

CharlesDias Sep 16, 2024 •

edited

Loading

josuah commented Jul 22, 2024

ngphibang Jul 22, 2024 •

edited

Loading

CharlesDias Jul 22, 2024 •

edited

Loading

ngphibang Jul 22, 2024 •

edited

Loading

CharlesDias commented Jul 22, 2024

ngphibang commented Jul 22, 2024

CharlesDias commented Jul 24, 2024

CharlesDias commented Sep 14, 2024

CharlesDias commented Sep 14, 2024

josuah left a comment

CharlesDias commented Sep 16, 2024

josuah commented Sep 16, 2024

Add parallel interface (DVP) support for ov5640 driver #76124

Are you sure you want to change the base?

Add parallel interface (DVP) support for ov5640 driver #76124

Conversation

CharlesDias commented Jul 19, 2024 • edited Loading

CharlesDias commented Jul 21, 2024

josuah left a comment

Choose a reason for hiding this comment

josuah Jul 21, 2024

Choose a reason for hiding this comment

CharlesDias Sep 14, 2024

Choose a reason for hiding this comment

josuah Jul 21, 2024

Choose a reason for hiding this comment

CharlesDias Sep 14, 2024

Choose a reason for hiding this comment

josuah Sep 15, 2024

Choose a reason for hiding this comment

CharlesDias Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

josuah commented Jul 22, 2024

ngphibang Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

CharlesDias Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

ngphibang Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

CharlesDias commented Jul 22, 2024

ngphibang commented Jul 22, 2024

CharlesDias commented Jul 24, 2024

CharlesDias commented Sep 14, 2024

CharlesDias commented Sep 14, 2024

josuah left a comment

Choose a reason for hiding this comment

CharlesDias commented Sep 16, 2024

josuah commented Sep 16, 2024

CharlesDias commented Jul 19, 2024 •

edited

Loading

CharlesDias Sep 16, 2024 •

edited

Loading

ngphibang Jul 22, 2024 •

edited

Loading

CharlesDias Jul 22, 2024 •

edited

Loading

ngphibang Jul 22, 2024 •

edited

Loading