search

zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.91k stars 6.64k forks source link

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

Open CharlesDias opened 5 months ago

CharlesDias commented 5 months ago

Is your enhancement proposal related to a problem? Please describe. The current ov5640 does not support the parallel interface, it supports MIPI CSI-2 interface.

Describe the solution you'd like Improve the ov5640 video driver to provide parallel interface support as well.

Additional context Anyone planning on implementing this improvement?

Recently, the OV5640 video driver was added. However, it only supports the MIPI CSI-2 interface. I plan to implement this driver update, if no one is implementing it, and validate it on a STMicroelectronics board via DCMI driver.

/cc @ngphibang @kartben @erwango @ABOSTM @FRASTM @josuah @loicpoulain @danieldegrasse

josuah commented 5 months ago

I have worked a bit with that sensor in the past in parallel (non-MIPI) mode, so could give a hand. Available in the chat.

This might be the first sensor that would be possible to configure in either modes.

There is a source = <&phandle>; devicetree property that would allow the controller to configure the sensor into the correct mode, maybe that is the way.

On Linux, the MIPI parameters are passed as part of the endpoint:

ports {
        port {
                endpoint {
                        mipi-lanes = <1 2 3 4>;
                        remote-endpoint = <&other>;
                };
        }
};

That opens these possibilities:

CharlesDias commented 5 months ago

Thanks for your availability, @josuah! I am not a specialist, but could be a good opportunity to learn more about camera sensor and Zephyr drivers as well.

CharlesDias commented 5 months ago

This was the partial DTS that I'm using in the initial tests with OV5640 sensor and ST DCMI driver on STM32H7B3I-DK board.

&i2c4 {
    pinctrl-0 = <&i2c4_scl_pd12 &i2c4_sda_pd13>;
    pinctrl-names = "default";
    status = "okay";
    clock-frequency = <I2C_BITRATE_FAST>;

    ov5640: ov5640@3c {
        compatible = "ovti,ov5640";
        reg = <0x3c>;
        status = "okay";
        reset-gpios = <&gpioa 0 GPIO_ACTIVE_LOW>;
        powerdown-gpios = <&gpioa 7 GPIO_ACTIVE_HIGH>;

        port {
            ov5640_ep_out: endpoint {
                remote-endpoint = <&dcmi_ep_in>;
            };
        };
    };
};

&dcmi {

    status = "okay";
    sensor = <&ov5640>;
    pinctrl-0 = <&dcmi_hsync_pa4 &dcmi_pixclk_pa6 &dcmi_vsync_pb7
        &dcmi_d0_pc6 &dcmi_d1_pc7 &dcmi_d2_pg10 &dcmi_d3_pc9
        &dcmi_d4_pc11 &dcmi_d5_pd3 &dcmi_d6_pb8 &dcmi_d7_pb9>;
    pinctrl-names = "default";
    bus-width = <8>;
    hsync-active = <0>;
    vsync-active = <0>;
    pixelclk-active = <1>;
    capture-rate = <1>;
    dmas = <&dma1 0 75 (STM32_DMA_PERIPH_TO_MEMORY | STM32_DMA_PERIPH_NO_INC |
        STM32_DMA_MEM_INC | STM32_DMA_PERIPH_8BITS | STM32_DMA_MEM_32BITS |
        STM32_DMA_PRIORITY_HIGH) STM32_DMA_FIFO_1_4>;

    port {
        dcmi_ep_in: endpoint {
            remote-endpoint = <&ov5640_ep_out>;
        };
    };
};

https://github.com/zephyrproject-rtos/zephyr/assets/27859307/1bf60853-ae4d-4bd8-a28b-a6dcab5d5a8f

CharlesDias commented 5 months ago

@josuah, if I got your suggestion, the previous DTS should be updated to something like below? I'm using the interface-typeproperty to specify the interface.

Parallel interface is used.

&i2c4 {
    pinctrl-0 = <&i2c4_scl_pd12 &i2c4_sda_pd13>;
    pinctrl-names = "default";
    status = "okay";
    clock-frequency = <I2C_BITRATE_FAST>;

    ov5640: ov5640@3c {
        compatible = "ovti,ov5640";
        reg = <0x3c>;
        status = "okay";

        reset-gpios = <&gpioa 0 GPIO_ACTIVE_LOW>;
        powerdown-gpios = <&gpioa 7 GPIO_ACTIVE_HIGH>;

            interface-type = "dvp";
        port {
            dvp_ep_out: endpoint {
            bus-width = <8>;
            hsync-active = <0>;
            vsync-active = <0>;
            pixelclk-active = <1>;
                remote-endpoint = <&dcmi_ep_in>;
            };
        };
    };
};

&dcmi {

    status = "okay";
    sensor = <&ov5640>;
    pinctrl-0 = <&dcmi_hsync_pa4 &dcmi_pixclk_pa6 &dcmi_vsync_pb7
        &dcmi_d0_pc6 &dcmi_d1_pc7 &dcmi_d2_pg10 &dcmi_d3_pc9
        &dcmi_d4_pc11 &dcmi_d5_pd3 &dcmi_d6_pb8 &dcmi_d7_pb9>;
    pinctrl-names = "default";
    capture-rate = <1>;
    dmas = <&dma1 0 75 (STM32_DMA_PERIPH_TO_MEMORY | STM32_DMA_PERIPH_NO_INC |
        STM32_DMA_MEM_INC | STM32_DMA_PERIPH_8BITS | STM32_DMA_MEM_32BITS |
        STM32_DMA_PRIORITY_HIGH) STM32_DMA_FIFO_1_4>;

    port {
        dcmi_ep_in: endpoint {
            remote-endpoint = <&dvp_ep_out>;
        };
    };
};

Parallel interface is used.

&i2c4 {
    pinctrl-0 = <&i2c4_scl_pd12 &i2c4_sda_pd13>;
    pinctrl-names = "default";
    status = "okay";
    clock-frequency = <I2C_BITRATE_FAST>;

    ov5640: ov5640@3c {
        compatible = "ovti,ov5640";
        reg = <0x3c>;
        status = "okay";

        reset-gpios = <&gpioa 0 GPIO_ACTIVE_LOW>;
        powerdown-gpios = <&gpioa 7 GPIO_ACTIVE_HIGH>;

            interface-type = "mipi";
        port {
            mipi_ep_out: endpoint {
                data-lanes = <1 2>;
                clock-lanes = <0>;
                bus-width = <2>;
                remote-endpoint = <&mipi_csi_ep_in>;
            };
        };
    };
};

&mipi_csi {
    status = "okay";

    /* Add other configs. /*

    port {
        mipi_csi_ep_in: endpoint {
            remote-endpoint = <&mipi_ep_out>;
        };
    };
};
josuah commented 5 months ago

That would be something like this in Linux (with the mipi-lanes data-lanes (I misremembered) on the mipi_csi controller block I think), but currently, there is no semantics given in Zephyr for the remote-endpoint. Configuring it is pure documentation and not used in the driver.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/st/stm32mp15xx-dhcor-avenger96.dtsi#n156

&dcmi {
        pinctrl-names = "default", "sleep";
        pinctrl-0 = <&dcmi_pins_c>;
        pinctrl-1 = <&dcmi_sleep_pins_c>;
        status = "disabled";

        port {
                dcmi_0: endpoint {
                        remote-endpoint = <&stmipi_2>;
                        bus-type = <5>;
                        bus-width = <8>;
                        pclk-sample = <0>;
                };
        };
};

And another example when MIPI is used:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/nxp/imx/imx6q-h100.dts#n337

&mipi_csi {
        status = "okay";

        port {
                mipi_csi2_in: endpoint {
                        remote-endpoint = <&tc358743_out>;
                        data-lanes = <1 2 3 4>;
                        clock-lanes = <0>;
                        clock-noncontinuous;
                        link-frequencies = /bits/ 64 <297000000>;
                };
        };
};

Although Zephyr does not have any of these, and configuring them here might not be necessary to get them to work.

Even having any remote-endpoint declared at all is just symbolic currently.

CharlesDias commented 5 months ago

Thanks, @josuah ! I'll take a better look at those links.

josuah commented 5 months ago

In Linux, the sensor detects whether it is configured on a MIPI bus or on a DVP/DMCI bus: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/i2c/ov5640.c#n2712

This means a OV5640 camera devicetree node would be nested into either a MIPI or DMCI node/block, and this would sets which type it is configured with.

I think there lack a devicetree bindings declaration on-bus: mipi; which would allow to introduce that difference in type.

But of course, MIPI devices aare "on-bus" for their configuration interface: I2C, not MIPI or CSI!

This is the "on-bus" for their remote-endpopint that is considered here: sensor->ep.bus_type

This means we would probably need something like this to be merged first: #73023

Maybe a simple devicetree configuration could be done in the meantime? So that you are not blocked by this. Some bus-type = with a comment mentioning #73023, at least to allow other members to give their feedback on this.

ngphibang commented 5 months ago

+1 from me. I think this would be a nice extension for ov5640. FYI, we are currently adding framerate changing to this driver but this shouldn't be a problem.

I think there lack a devicetree bindings declaration on-bus: mipi; which would allow to introduce that difference in type.

But of course, MIPI devices aare "on-bus" for their configuration interface: I2C, not MIPI or CSI!

This is the "on-bus" for their remote-endpopint that is considered here: sensor->ep.bus_type

This means we would probably need something like this to be merged first: https://github.com/zephyrproject-rtos/zephyr/pull/73023

@josuah , @CharlesDias : For this CSI / DVP (parallel) extension, as you said, we just need a "bus-type" property defined in the devicetree. We should group all these common properties into a "video-interfaces.yaml" binding (to avoid repeating them for each driver), some thing like this which I picked from Linux. Then, in the driver, we can get the property by : DT_PROP(DT_INST_CHILD(DT_INST_CHILD(id, port), endpoint), bus_type).

This will be similar to what actually done for display, as here.

Support of remote-endpoint in Zephyr would be nicer but I think it is not a blocking point for this case.

josuah commented 5 months ago

some thing like https://github.com/zephyrproject-rtos/zephyr/pull/74415 which I picked from Linux

Exciting! This is some good way to structure the various video drivers.

Then, in the driver, we can get the property by : DT_PROP(DT_INST_CHILD(DT_INST_CHILD(id, port), endpoint), bus_type).

I agree and this is good to have it this way in the meantime.

@CharlesDias would a bus-type property on the devicetree work for you?

Something like this:

&i2c4 {
        port {
            mipi_ep_out: endpoint {
                bus-type = <...>;
            };
        };
    };
};
CharlesDias commented 5 months ago

@CharlesDias would a bus-type property on the devicetree work for you?

@josuah, I think so!

Thank you, @josuah and @ngphibang! I'll wait a little more for other members' to confirm that no one is working on this.

CharlesDias commented 5 months ago

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/st/stm32mp15xx-dhcor-avenger96.dtsi#n156

Hi, @josuah and @ngphibang. I hope you can help me with the following question. On the Linux driver, from the link above, we have duplicate properties declared for both endpoints, dcmi and i2c. I thought this duplication strange. My questions are:

  1. Will we do this duplication on Zephyr?
  2. On Linux, the bus-type is defined into DCMI endpoint instead of OV5640 endpoint, but I believe for Zephyr it doesn't make sense. So, will we add the bus-type in the OV5640 endpoint, right?

Thanks!

&i2c4 {
    stmipi: stmipi@14 {
                . . . 
        ports {
                 . . .
            port@2 {
                reg = <2>;
                stmipi_2: endpoint {
                    bus-width = <8>;
                    hsync-active = <0>;
                    vsync-active = <0>;
                    pclk-sample = <0>;
                    remote-endpoint = <&dcmi_0>;
                };
            };
        };
    };

&dcmi {
        . . . 

    port {
        dcmi_0: endpoint {
            remote-endpoint = <&stmipi_2>;
            bus-type = <5>;
            bus-width = <8>;
            pclk-sample = <0>;
        };
    };
};
ngphibang commented 5 months ago

@CharlesDias

So, in any of above cases, I think we should define bus-type in ov5640 node.

ngphibang commented 5 months ago

For the "duplicated" properties, do you mean bus-width and pclk-sample ? IMO, it is because these properties are not necessarily identical for sensor and dcmi (although we see that they are the same in most cases), for example, pclk-sample can be rising edge for ov5640 and both rising / falling edge for dcmi (?) when there is signal modification on the bus, e.g. a logic signal inverter is present. Anyways, in ov5640 driver, you can see it is obtained from the endpoint of ov5640 here and in dcmi driver, it is also obtained from endpoint of dcmi node here as separate properties.

CharlesDias commented 5 months ago

So, in any of above cases, I think we should define bus-type in ov5640 node.

Also make sense for me to define the bus-type in ov5640 node. I will continue with that in mind.

For the "duplicated" properties, do you mean bus-width and pclk-sample ? IMO, it is because these properties does not necessarily have the same value for sensor and dcmi (although we see that they are the same in devicetree),

I did not know that! :-)

Thanks so much @ngphibang for the explanation!

ngphibang commented 3 months ago

Hi @CharlesDias , FYI, I finalized the video binding PR and put it into review (it was in Draft). An example of using this can be found here and here. Sorry for the delay.

You could also rebase the DCMI binding on this common binding if you want.

Thanks !

CharlesDias commented 2 months ago

Thanks, @ngphibang! Well done!

CodingGhost commented 3 weeks ago

@CharlesDias I am trying to use the ov5460 on a STM32H7a3zi. Im using latest Zephyr master but I get video_ov5640: Unsupported pixel format or resolution.

Is there any PR I need for this to run thats not yet in master? Or did I do something wrong? Im using the lvgl sample but removed all lvgl code as I dont have a display.

josuah commented 3 weeks ago

@CodingGhost I suspect this is related to a different PR/issue, and opened a discussion about it here:

Do feel free to comment the OV5640 video driver and DVP support here, though let's troubleshoot your issue on #80371 if that's ok. :)