| Message ID | 20211208151230.3695378-1-s.hauer@pengutronix.de |
|---|---|
| Headers | show |
| Series | drm/rockchip: RK356x VOP2 support | expand |
Hi Sascha, Am Mittwoch, 8. Dezember 2021, 16:12:30 CET schrieb Sascha Hauer: > On the rk3568 we have this (simplified) situation: > > .--------. .-----. .---------. > -| hpll |--.--| /n |----|dclk_vop0|- > `--------´ | `-----´ `---------´ > | .-----. .---------. > `--| /m |----|dclk_vop1|- > | `-----´ `---------´ > | .---------. > `-------------|hdmi_ref |- > `---------´ > > hpll is the PLL that drives the HDMI reference clock and the pixel > clocks for the different CRTCs (dclk_vop0/1). Between the pixel clocks > and the hpll there are programmable dividers whereas the HDMI reference > clock is directly connected to the hpll. > > For the HDMI output to work the pixel clock must be the same as the HDMI > reference clock, hence the dividers must be programmed to 1. Normally a > rate change on dclk_vop0/1 propagates through to the hpll and the clock > framework picks a suitable combination of hpll and divider settings. by > accident it picks a divider setting of 1 for the standard 1080p case, > but other divider settings for most other resolutions leaving the HDMI > port non working. > > This patch is not a solution, it merely puts the finger in the wound. We > leave out the divider for the composite clock for dclk_vop0 which then > leaves the divider at the bootloader default setting of 1. I assume > the divider is disturbing only for the HDMI case, but needed for other > outputs. Any thoughts how this can be handled? I'm not even sure if/how the common clock framework keeps track of diverging wishes to parent-rates :-) . But I do see two direct issues in the _existing_ code. dclk_vop0/1 uses CLK_SET_RATE_PARENT so is allowed to change the rates of its parent clock(s). Its parent clocks are not only hpll but can also be vpll, gpll and cpll. So this can cause even more mayhem, if the ccf for example decides to select the gpll and then change its rate,which may result in a lot of peripherals getting their rates changed under them ;-) . On the other hand I see in the clock driver that hdmi-ref is not allowed to change its parent rate, so can only select between hpll and hpll_ph0 (1/2 the rate?). So I guess, one way could be: - add CLK_SET_RATE_PARENT to the hdmi-ref clock - drop CLK_SET_RATE_PARENT from the dclks - make sure hdmi-clock is set before the dclk Heiko > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > drivers/clk/rockchip/clk-rk3568.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/rockchip/clk-rk3568.c b/drivers/clk/rockchip/clk-rk3568.c > index 69a9e8069a486..2d04d8253ca22 100644 > --- a/drivers/clk/rockchip/clk-rk3568.c > +++ b/drivers/clk/rockchip/clk-rk3568.c > @@ -1038,8 +1038,8 @@ static struct rockchip_clk_branch rk3568_clk_branches[] __initdata = { > RK3568_CLKGATE_CON(20), 8, GFLAGS), > GATE(HCLK_VOP, "hclk_vop", "hclk_vo", 0, > RK3568_CLKGATE_CON(20), 9, GFLAGS), > - COMPOSITE(DCLK_VOP0, "dclk_vop0", hpll_vpll_gpll_cpll_p, CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, > - RK3568_CLKSEL_CON(39), 10, 2, MFLAGS, 0, 8, DFLAGS, > + COMPOSITE_NODIV(DCLK_VOP0, "dclk_vop0", hpll_vpll_gpll_cpll_p, CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, > + RK3568_CLKSEL_CON(39), 10, 2, MFLAGS, > RK3568_CLKGATE_CON(20), 10, GFLAGS), > COMPOSITE(DCLK_VOP1, "dclk_vop1", hpll_vpll_gpll_cpll_p, CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, > RK3568_CLKSEL_CON(40), 10, 2, MFLAGS, 0, 8, DFLAGS, >
Hi, On 12/8/21 4:12 PM, Sascha Hauer wrote: > From: Andy Yan <andy.yan@rock-chips.com> > > The VOP2 unit is found on Rockchip SoCs beginning with rk3566/rk3568. > It replaces the VOP unit found in the older Rockchip SoCs. > > This driver has been derived from the downstream Rockchip Kernel and > heavily modified: > > - All nonstandard DRM properties have been removed > - dropped struct vop2_plane_state and pass around less data between > functions > - Dropped all DRM_FORMAT_* not known on upstream > - rework register access to get rid of excessively used macros > - Drop all waiting for framesyncs > > The driver is tested with HDMI and MIPI-DSI display on a RK3568-EVB > board. Overlay support is tested with the modetest utility. AFBC support > on the cluster windows is tested with weston-simple-dmabuf-egl on > weston using the (yet to be upstreamed) panfrost driver support. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- [..] > + > +static const struct of_device_id vop2_dt_match[] = { > + { > + .compatible = "rockchip,rk3568-vop", > + .data = &rk3568_vop > + }, { > + .compatible = "rockchip,rk3568-vop", Maybe use: .compatible = "rockchip,rk3566-vop", > + .data = &rk3566_vop > + }, { > + }, Maybe sort this list alphabetical based on compatible in case later more SoCs are added. rk3566 rk3568 === The structure layout size above could be reduced for if we get more compatible strings additions. Example vop1: static const struct of_device_id vop_driver_dt_match[] = { { .compatible = "rockchip,rk3036-vop", .data = &rk3036_vop }, { .compatible = "rockchip,rk3126-vop", .data = &rk3126_vop }, { .compatible = "rockchip,px30-vop-big", .data = &px30_vop_big }, { .compatible = "rockchip,px30-vop-lit", .data = &px30_vop_lit }, { .compatible = "rockchip,rk3066-vop", .data = &rk3066_vop }, { .compatible = "rockchip,rk3188-vop", .data = &rk3188_vop }, { .compatible = "rockchip,rk3288-vop", .data = &rk3288_vop }, { .compatible = "rockchip,rk3368-vop", .data = &rk3368_vop }, { .compatible = "rockchip,rk3366-vop", .data = &rk3366_vop }, { .compatible = "rockchip,rk3399-vop-big", .data = &rk3399_vop_big }, { .compatible = "rockchip,rk3399-vop-lit", .data = &rk3399_vop_lit }, { .compatible = "rockchip,rk3228-vop", .data = &rk3228_vop }, { .compatible = "rockchip,rk3328-vop", .data = &rk3328_vop }, {}, }; > +}; > +MODULE_DEVICE_TABLE(of, vop2_dt_match); > + > +static int vop2_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + > + return component_add(dev, &vop2_component_ops); > +} > + > +static int vop2_remove(struct platform_device *pdev) > +{ > + component_del(&pdev->dev, &vop2_component_ops); > + > + return 0; > +} > + > +struct platform_driver vop2_platform_driver = { > + .probe = vop2_probe, > + .remove = vop2_remove, > + .driver = { > + .name = "rockchip-vop2", > + .of_match_table = of_match_ptr(vop2_dt_match), > + }, > +}; >
Hi, Could add a patch version to the subject? On 12/8/21 4:12 PM, Sascha Hauer wrote: > This enabled the VOP2 display controller along with hdmi and the > required port routes which is enough to get a picture out of the > hdmi port of the board. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > .../boot/dts/rockchip/rk3568-evb1-v10.dts | 31 +++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts > index 184e2aa2416af..b1b0963fa8525 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts > +++ b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts > @@ -7,6 +7,7 @@ > /dts-v1/; > #include <dt-bindings/gpio/gpio.h> > #include <dt-bindings/pinctrl/rockchip.h> > +#include <dt-bindings/soc/rockchip,vop2.h> > #include "rk3568.dtsi" > > / { > @@ -106,6 +107,12 @@ &gmac1m1_rgmii_clk > status = "okay"; > }; > > +&hdmi { > + status = "okay"; > + avdd-0v9-supply = <&vdda0v9_image>; > + avdd-1v8-supply = <&vcca1v8_image>; status below > +}; === Example from rk3066a-mk808.dts In dtsi: hdmi { ports { #address-cells = <1>; #size-cells = <0>; hdmi_in: port@0 { reg = <0>; #address-cells = <1>; #size-cells = <0>; }; hdmi_out: port@1 { reg = <1>; }; === In dts: hdmi-con { compatible = "hdmi-connector"; type = "c"; port { hdmi_con_in: endpoint { remote-endpoint = <&hdmi_out_con>; }; }; }; === &hdmi_out { hdmi_out_con: endpoint { remote-endpoint = <&hdmi_con_in>; }; }; === > + > &i2c0 { > status = "okay"; > > @@ -390,3 +397,27 @@ &sdmmc0 { > &uart2 { > status = "okay"; > }; > + > +&vop { > + status = "okay"; > + assigned-clocks = <&cru DCLK_VOP0>, <&cru DCLK_VOP1>; > + assigned-clock-parents = <&pmucru PLL_HPLL>, <&cru PLL_VPLL>; status below > +}; > + > +&vop_mmu { > + status = "okay"; > +}; > + > +&hdmi_in { > + hdmi_in_vp0: endpoint@0 { > + reg = <0>; > + remote-endpoint = <&vp0_out_hdmi>; > + }; > +}; > + > +&vp0 { > + vp0_out_hdmi: endpoint@RK3568_VOP2_EP_HDMI { > + reg = <RK3568_VOP2_EP_HDMI>; > + remote-endpoint = <&hdmi_in_vp0>; > + }; > +}; >
On Wed, Dec 08, 2021 at 05:51:43PM +0100, Heiko Stübner wrote: > Hi Sascha, > > Am Mittwoch, 8. Dezember 2021, 16:12:30 CET schrieb Sascha Hauer: > > On the rk3568 we have this (simplified) situation: > > > > .--------. .-----. .---------. > > -| hpll |--.--| /n |----|dclk_vop0|- > > `--------´ | `-----´ `---------´ > > | .-----. .---------. > > `--| /m |----|dclk_vop1|- > > | `-----´ `---------´ > > | .---------. > > `-------------|hdmi_ref |- > > `---------´ > > > > hpll is the PLL that drives the HDMI reference clock and the pixel > > clocks for the different CRTCs (dclk_vop0/1). Between the pixel clocks > > and the hpll there are programmable dividers whereas the HDMI reference > > clock is directly connected to the hpll. > > > > For the HDMI output to work the pixel clock must be the same as the HDMI > > reference clock, hence the dividers must be programmed to 1. Normally a > > rate change on dclk_vop0/1 propagates through to the hpll and the clock > > framework picks a suitable combination of hpll and divider settings. by > > accident it picks a divider setting of 1 for the standard 1080p case, > > but other divider settings for most other resolutions leaving the HDMI > > port non working. > > > > This patch is not a solution, it merely puts the finger in the wound. We > > leave out the divider for the composite clock for dclk_vop0 which then > > leaves the divider at the bootloader default setting of 1. I assume > > the divider is disturbing only for the HDMI case, but needed for other > > outputs. Any thoughts how this can be handled? > > I'm not even sure if/how the common clock framework keeps track of > diverging wishes to parent-rates :-) . I don't think the common clock framework tries to keep track of that. > > But I do see two direct issues in the _existing_ code. > > dclk_vop0/1 uses CLK_SET_RATE_PARENT so is allowed to change > the rates of its parent clock(s). > > Its parent clocks are not only hpll but can also be vpll, gpll and cpll. > So this can cause even more mayhem, if the ccf for example decides > to select the gpll and then change its rate,which may result in a lot > of peripherals getting their rates changed under them ;-) . Right, we can only allow the CLK_SET_RATE_PARENT parent flag on the dclk clocks when the parent is HPLL. Since we can't be sure that HPLL is the parent we have to remove the flag. > > On the other hand I see in the clock driver that hdmi-ref is not allowed > to change its parent rate, so can only select between hpll and hpll_ph0 > (1/2 the rate?). > > So I guess, one way could be: > - add CLK_SET_RATE_PARENT to the hdmi-ref clock > - drop CLK_SET_RATE_PARENT from the dclks > - make sure hdmi-clock is set before the dclk That solves it for the HDMI case. I can imagine that for a LVDS user the CLK_SET_RATE_PARENT flag on the dclks is quite handy to get a PLL frequency suitable for the display. Otherwise he would have to set a suitable PLL frequency using assigned-clock-rates in the device tree. That's still possible so this might be a good compromise. Sascha
Hi Johan, On Wed, Dec 08, 2021 at 05:59:16PM +0100, Johan Jonker wrote: > Hi, > > On 12/8/21 4:12 PM, Sascha Hauer wrote: > > From: Andy Yan <andy.yan@rock-chips.com> > > > > The VOP2 unit is found on Rockchip SoCs beginning with rk3566/rk3568. > > It replaces the VOP unit found in the older Rockchip SoCs. > > > > This driver has been derived from the downstream Rockchip Kernel and > > heavily modified: > > > > - All nonstandard DRM properties have been removed > > - dropped struct vop2_plane_state and pass around less data between > > functions > > - Dropped all DRM_FORMAT_* not known on upstream > > - rework register access to get rid of excessively used macros > > - Drop all waiting for framesyncs > > > > The driver is tested with HDMI and MIPI-DSI display on a RK3568-EVB > > board. Overlay support is tested with the modetest utility. AFBC support > > on the cluster windows is tested with weston-simple-dmabuf-egl on > > weston using the (yet to be upstreamed) panfrost driver support. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > [..] > > > + > > +static const struct of_device_id vop2_dt_match[] = { > > + { > > + .compatible = "rockchip,rk3568-vop", > > + .data = &rk3568_vop > > + }, { > > > + .compatible = "rockchip,rk3568-vop", > > Maybe use: > .compatible = "rockchip,rk3566-vop", Copy/Paste bug. Will fix. > > > + .data = &rk3566_vop > > + }, { > > + }, > > Maybe sort this list alphabetical based on compatible in case later more > SoCs are added. > > rk3566 > rk3568 Ok. > > === > > The structure layout size above could be reduced for if we get more > compatible strings additions. > > Example vop1: > > static const struct of_device_id vop_driver_dt_match[] = { > { .compatible = "rockchip,rk3036-vop", > .data = &rk3036_vop }, > { .compatible = "rockchip,rk3126-vop", > .data = &rk3126_vop }, > { .compatible = "rockchip,px30-vop-big", > .data = &px30_vop_big }, > { .compatible = "rockchip,px30-vop-lit", > .data = &px30_vop_lit }, > { .compatible = "rockchip,rk3066-vop", > .data = &rk3066_vop }, > { .compatible = "rockchip,rk3188-vop", > .data = &rk3188_vop }, > { .compatible = "rockchip,rk3288-vop", > .data = &rk3288_vop }, > { .compatible = "rockchip,rk3368-vop", > .data = &rk3368_vop }, > { .compatible = "rockchip,rk3366-vop", > .data = &rk3366_vop }, > { .compatible = "rockchip,rk3399-vop-big", > .data = &rk3399_vop_big }, > { .compatible = "rockchip,rk3399-vop-lit", > .data = &rk3399_vop_lit }, > { .compatible = "rockchip,rk3228-vop", > .data = &rk3228_vop }, > { .compatible = "rockchip,rk3328-vop", > .data = &rk3328_vop }, > {}, It's shorter, but ugly ;) That's only my personal taste though, I don't care much. Sascha
This is the second round of the vop2 series. There are still some issues open, but I thought it's about time to let people see and test it. I integrated the review feedback I got from v1. Other changes include: All framesync waiting is gone from the driver which makes it more straight forward. To accomplish this the port_mux setup is now static in the driver. This means each video port has a fixed maximum number of planes which is less flexible but much easier to handle. I also removed much of the register mapping and shadow register handling around struct vop_reg. This basically resembles regmap and can eventually replaced by regmap. Some places are still left in the driver, I plan to remove those in later versions. I think I have found the issue why only 1080p resolutions work, this seems to be an issue in the way the clock tree is arranged. See the last patch in this series which points to the problem, so far I don't have a good solution for it. As usual, all comments and feedback welcome. Sascha Changes since v1: - drop all unnecessary waiting for frames within atomic modeset and plane update - Cluster subwin support removed - gamma support removed - unnecessary irq_lock removed - interrupt handling simplified - simplified zpos handling - drop is_alpha_support(), use fb->format->has_alpha instead - use devm_regulator_get() rather than devm_regulator_get_optional() for hdmi regulators - Use fixed number of planes per video port - Drop homegrown regmap code from vop2 driver (not complete yet) - Add separate include file for vop2 driver to not pollute the vop include Andy Yan (1): drm: rockchip: Add VOP2 driver Benjamin Gaignard (1): dt-bindings: display: rockchip: dw-hdmi: Add compatible for rk3568 HDMI Michael Riesch (1): arm64: dts: rockchip: enable vop2 and hdmi tx on quartz64a Sascha Hauer (15): drm/rockchip: dw_hdmi: Do not leave clock enabled in error case drm/rockchip: dw_hdmi: rename vpll clock to reference clock drm/rockchip: dw_hdmi: add rk3568 support drm/rockchip: dw_hdmi: add regulator support dt-bindings: display: rockchip: dw-hdmi: Make unwedge pinctrl optional dt-bindings: display: rockchip: dw-hdmi: Allow "ref" as clock name dt-bindings: display: rockchip: dw-hdmi: Add regulator support arm64: dts: rockchip: rk3399: reorder hmdi clocks dt-bindings: display: rockchip: Add binding for VOP2 arm64: dts: rockchip: rk356x: Add VOP2 nodes arm64: dts: rockchip: rk356x: Add HDMI nodes arm64: dts: rockchip: rk3568-evb: Enable VOP2 and hdmi drm/encoder: Add of_graph port to struct drm_encoder drm/rockchip: Make VOP driver optional [HACK, RFC] clk: rk3568: do not divide dclk_vop0 .../display/rockchip/rockchip,dw-hdmi.yaml | 14 +- .../display/rockchip/rockchip-vop2.yaml | 118 + arch/arm64/boot/dts/rockchip/rk3399.dtsi | 6 +- .../boot/dts/rockchip/rk3566-quartz64-a.dts | 31 + arch/arm64/boot/dts/rockchip/rk3566.dtsi | 4 + .../boot/dts/rockchip/rk3568-evb1-v10.dts | 31 + arch/arm64/boot/dts/rockchip/rk3568.dtsi | 4 + arch/arm64/boot/dts/rockchip/rk356x.dtsi | 75 + drivers/clk/rockchip/clk-rk3568.c | 4 +- drivers/gpu/drm/rockchip/Kconfig | 14 + drivers/gpu/drm/rockchip/Makefile | 4 +- drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 107 +- drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 3 +- drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 7 +- drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 2 + drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 15 + drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 2636 +++++++++++++++++ drivers/gpu/drm/rockchip/rockchip_drm_vop2.h | 625 ++++ drivers/gpu/drm/rockchip/rockchip_vop2_reg.c | 505 ++++ include/drm/drm_encoder.h | 2 + include/dt-bindings/soc/rockchip,vop2.h | 14 + 21 files changed, 4193 insertions(+), 28 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_vop2.h create mode 100644 drivers/gpu/drm/rockchip/rockchip_vop2_reg.c create mode 100644 include/dt-bindings/soc/rockchip,vop2.h