| Message ID | 20250215-vop2-hdmi1-disp-modes-v1-3-81962a7151d6@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Improve Rockchip VOP2 display modes handling on RK3588 HDMI1 | expand |
Hi Cristian, On Sat, 15 Feb 2025 02:55:39 +0200, Cristian Ciocaltea wrote: >The HDMI1 PHY PLL clock source cannot be added directly to vop node in >rk3588-base.dtsi, along with the HDMI0 related one, because HDMI1 is an >optional feature and its PHY node belongs to a separate (extra) DT file. > >Therefore, add the HDMI1 PHY PLL clock source to VOP2 by overwriting its >clocks & clock-names properties in the extra DT file. There are boards that only use hdmi1 such as ROCK 5 ITX. So there are two choices for this board: 1, Enable hdptxphy0 as dependency of vop although it is not really used. 2, Overwrite vop node at board dts to make it only use hdptxphy1 like: &vop { clocks = <&cru ACLK_VOP>, <&cru HCLK_VOP>, <&cru DCLK_VOP0>, <&cru DCLK_VOP1>, <&cru DCLK_VOP2>, <&cru DCLK_VOP3>, <&cru PCLK_VOP_ROOT>, <&hdptxphy1>; clock-names = "aclk", "hclk", "dclk_vp0", "dclk_vp1", "dclk_vp2", "dclk_vp3", "pclk_vop", "pll_hdmiphy1"; }; What do you think of these two method? Best regards, Jianfeng
Am Montag, 17. Februar 2025, 03:44:37 MEZ schrieb Jianfeng Liu: > Hi Cristian, > > On Sat, 15 Feb 2025 02:55:39 +0200, Cristian Ciocaltea wrote: > >The HDMI1 PHY PLL clock source cannot be added directly to vop node in > >rk3588-base.dtsi, along with the HDMI0 related one, because HDMI1 is an > >optional feature and its PHY node belongs to a separate (extra) DT file. > > > >Therefore, add the HDMI1 PHY PLL clock source to VOP2 by overwriting its > >clocks & clock-names properties in the extra DT file. > > There are boards that only use hdmi1 such as ROCK 5 ITX. So there are two > choices for this board: > > 1, Enable hdptxphy0 as dependency of vop although it is not really used. > > 2, Overwrite vop node at board dts to make it only use hdptxphy1 like: > > &vop { > clocks = <&cru ACLK_VOP>, > <&cru HCLK_VOP>, > <&cru DCLK_VOP0>, > <&cru DCLK_VOP1>, > <&cru DCLK_VOP2>, > <&cru DCLK_VOP3>, > <&cru PCLK_VOP_ROOT>, > <&hdptxphy1>; > clock-names = "aclk", > "hclk", > "dclk_vp0", > "dclk_vp1", > "dclk_vp2", > "dclk_vp3", > "pclk_vop", > "pll_hdmiphy1"; > }; > > What do you think of these two method? Going by the code in patch1 (+drm-misc) we have: vop2->pll_hdmiphy0 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy0"); + vop2->pll_hdmiphy1 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy1"); So the clock-reference to hdptxphy0 should just result in vop2->pll_hdmiphy0 being NULL and thus ignored further on?
On 2/17/25 4:33 PM, Heiko Stübner wrote: > Am Montag, 17. Februar 2025, 03:44:37 MEZ schrieb Jianfeng Liu: >> Hi Cristian, >> >> On Sat, 15 Feb 2025 02:55:39 +0200, Cristian Ciocaltea wrote: >>> The HDMI1 PHY PLL clock source cannot be added directly to vop node in >>> rk3588-base.dtsi, along with the HDMI0 related one, because HDMI1 is an >>> optional feature and its PHY node belongs to a separate (extra) DT file. >>> >>> Therefore, add the HDMI1 PHY PLL clock source to VOP2 by overwriting its >>> clocks & clock-names properties in the extra DT file. >> >> There are boards that only use hdmi1 such as ROCK 5 ITX. So there are two >> choices for this board: >> >> 1, Enable hdptxphy0 as dependency of vop although it is not really used. >> >> 2, Overwrite vop node at board dts to make it only use hdptxphy1 like: >> >> &vop { >> clocks = <&cru ACLK_VOP>, >> <&cru HCLK_VOP>, >> <&cru DCLK_VOP0>, >> <&cru DCLK_VOP1>, >> <&cru DCLK_VOP2>, >> <&cru DCLK_VOP3>, >> <&cru PCLK_VOP_ROOT>, >> <&hdptxphy1>; >> clock-names = "aclk", >> "hclk", >> "dclk_vp0", >> "dclk_vp1", >> "dclk_vp2", >> "dclk_vp3", >> "pclk_vop", >> "pll_hdmiphy1"; >> }; >> >> What do you think of these two method? > > Going by the code in patch1 (+drm-misc) we have: > vop2->pll_hdmiphy0 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy0"); > + > vop2->pll_hdmiphy1 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy1"); > > So the clock-reference to hdptxphy0 should just result in vop2->pll_hdmiphy0 > being NULL and thus ignored further on? Yep, that is the intended behavior. @Jianfeng: Did you encounter any particular issue with the current approach?
Hi,
On Tue, 18 Feb 2025 01:33:34 +0200, Cristian Ciocaltea wrote:
>@Jianfeng: Did you encounter any particular issue with the current approach?
This patch is adding a dependency of hdptxphy1 to vop for all rk3588
boards, but not all rk3588 boards have dual hdmi, armsom sige7 is an
example. At runtime I will get errors because there is no hdptxphy1
enabled in devicetree:
[ 2.945675] rockchip-drm display-subsystem: [drm] *ERROR* failed to get pll_hdmiphy1
Overwrting vop node at board level to remove the dependency of hdptxphy1
can fix the issue.
Best regards,
Jianfeng
Hi Cristian, No matter one or two hdmi ports the rk3588 boards have, most of devicetrees in mainline kernel only have hdmi0 supported. After applying this patch their hdmi0 support is broken. So I recommend moving the vop clk part to board level devicetree. Then support of hdmi0 won't be broken, and board maintainers can add HDMI1 PHY PLL clk when they are adding hdmi1 support. I can add support for orangepi 5 max and armsom w3 for reference by other developers. Best regards, Jianfeng
Am Dienstag, 18. Februar 2025, 10:52:16 MEZ schrieb Jianfeng Liu: > Hi Cristian, > > No matter one or two hdmi ports the rk3588 boards have, most of > devicetrees in mainline kernel only have hdmi0 supported. After applying > this patch their hdmi0 support is broken. > > So I recommend moving the vop clk part to board level devicetree. > Then support of hdmi0 won't be broken, and board maintainers can add > HDMI1 PHY PLL clk when they are adding hdmi1 support. I can add support > for orangepi 5 max and armsom w3 for reference by other developers. better, fix the VOP2 driver - both for the existing hdmi0 + this hdmi1 please. I.e. the clock is optional, and the error you are seeing comes from the vop2->pll_hdmiphy1 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy1"); if (IS_ERR(vop2->pll_hdmiphy1)) { drm_err(vop2->drm, "failed to get pll_hdmiphy1\n"); return PTR_ERR(vop2->pll_hdmiphy1); } part. clk_get_optional is supposed to return NULL when clock-retrieval causes a ENOENT error. Seemingly going to a clock controller in a disabled node returns a different error? So I guess step1, check what error is actually returned. Step2 check if clk_get_optional need to be adapted or alternatively catch the error in the vop2 and set the clock to NULL ourself in that case. hdptxphy0 + hdpxphy1 _are_ valid supplies for the vop, so their reference should be in the soc-dtsi and the kernel code should just figure things out correctly. Wiggling with clocks in each board will cause headaches down the road. Heiko
Hi Heiko, On Tue, 18 Feb 2025 11:00:57 +0100, Heiko Stübnerwrote: >So I guess step1, check what error is actually returned. I have checked that the return value is -517: rockchip-drm display-subsystem: [drm] *ERROR* failed to get pll_hdmiphy1 with -517 >Step2 check if clk_get_optional need to be adapted or alternatively >catch the error in the vop2 and set the clock to NULL ourself in that case. I tried the following patch to set the clock to NULL when clk_get_optional failed with value -517, and hdmi0 is working now. There are also some boards like rock 5 itx which only use hdmi1, I think we should also add this logic to vop2->pll_hdmiphy0. @@ -3733,6 +3751,15 @@ static int vop2_bind(struct device *dev, struct device *master, void *data) return PTR_ERR(vop2->pll_hdmiphy0); } + vop2->pll_hdmiphy1 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy1"); + if (IS_ERR(vop2->pll_hdmiphy1)) { + drm_err(vop2->drm, "failed to get pll_hdmiphy1 with %d\n", vop2->pll_hdmiphy1); + if (vop2->pll_hdmiphy1 == -EPROBE_DEFER) + vop2->pll_hdmiphy1 = NULL; + else + return PTR_ERR(vop2->pll_hdmiphy1); + } + vop2->irq = platform_get_irq(pdev, 0); if (vop2->irq < 0) { drm_err(vop2->drm, "cannot find irq for vop2\n"); Best regards, Jianfeng
Hi, On Tue, Feb 18, 2025 at 08:17:46PM +0800, Jianfeng Liu wrote: > On Tue, 18 Feb 2025 11:00:57 +0100, Heiko Stübnerwrote: > >So I guess step1, check what error is actually returned. > > I have checked that the return value is -517: > > rockchip-drm display-subsystem: [drm] *ERROR* failed to get pll_hdmiphy1 with -517 > > >Step2 check if clk_get_optional need to be adapted or alternatively > >catch the error in the vop2 and set the clock to NULL ourself in that case. > > I tried the following patch to set the clock to NULL when clk_get_optional > failed with value -517, and hdmi0 is working now. There are also some > boards like rock 5 itx which only use hdmi1, I think we should also add > this logic to vop2->pll_hdmiphy0. > > @@ -3733,6 +3751,15 @@ static int vop2_bind(struct device *dev, struct device *master, void *data) > return PTR_ERR(vop2->pll_hdmiphy0); > } > > + vop2->pll_hdmiphy1 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy1"); > + if (IS_ERR(vop2->pll_hdmiphy1)) { > + drm_err(vop2->drm, "failed to get pll_hdmiphy1 with %d\n", vop2->pll_hdmiphy1); > + if (vop2->pll_hdmiphy1 == -EPROBE_DEFER) > + vop2->pll_hdmiphy1 = NULL; > + else > + return PTR_ERR(vop2->pll_hdmiphy1); > + } > + This first of all shows, that we should replace drm_err in this place with dev_err_probe(), which hides -EPROBE_DEFER errors by default and instead captures the reason for /sys/kernel/debug/devices_deferred. Second what you are doing in the above suggestion will break kernel configurations where VOP is built-in and the HDMI PHY is build as a module. But I also think it would be better to have the clocks defined in the SoC level DT. I suppose that means vop2_bind would have to check if the HDMI controller <ID> is enabled and only requests pll_hdmiphy<ID> based on that. Considering there is the OF graph pointing from VOP to the enabled HDMI controllers, it should be able to do that. Greetings, -- Sebastian > vop2->irq = platform_get_irq(pdev, 0); > if (vop2->irq < 0) { > drm_err(vop2->drm, "cannot find irq for vop2\n"); > > Best regards, > Jianfeng
Am Dienstag, 18. Februar 2025, 15:13:07 MEZ schrieb Sebastian Reichel: > Hi, > > On Tue, Feb 18, 2025 at 08:17:46PM +0800, Jianfeng Liu wrote: > > On Tue, 18 Feb 2025 11:00:57 +0100, Heiko Stübnerwrote: > > >So I guess step1, check what error is actually returned. > > > > I have checked that the return value is -517: > > > > rockchip-drm display-subsystem: [drm] *ERROR* failed to get pll_hdmiphy1 with -517 > > > > >Step2 check if clk_get_optional need to be adapted or alternatively > > >catch the error in the vop2 and set the clock to NULL ourself in that case. > > > > I tried the following patch to set the clock to NULL when clk_get_optional > > failed with value -517, and hdmi0 is working now. There are also some > > boards like rock 5 itx which only use hdmi1, I think we should also add > > this logic to vop2->pll_hdmiphy0. > > > > @@ -3733,6 +3751,15 @@ static int vop2_bind(struct device *dev, struct device *master, void *data) > > return PTR_ERR(vop2->pll_hdmiphy0); > > } > > > > + vop2->pll_hdmiphy1 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy1"); > > + if (IS_ERR(vop2->pll_hdmiphy1)) { > > + drm_err(vop2->drm, "failed to get pll_hdmiphy1 with %d\n", vop2->pll_hdmiphy1); > > + if (vop2->pll_hdmiphy1 == -EPROBE_DEFER) > > + vop2->pll_hdmiphy1 = NULL; > > + else > > + return PTR_ERR(vop2->pll_hdmiphy1); > > + } > > + > > This first of all shows, that we should replace drm_err in this > place with dev_err_probe(), which hides -EPROBE_DEFER errors by > default and instead captures the reason for /sys/kernel/debug/devices_deferred. > > Second what you are doing in the above suggestion will break kernel > configurations where VOP is built-in and the HDMI PHY is build as a > module. > > But I also think it would be better to have the clocks defined in the > SoC level DT. I suppose that means vop2_bind would have to check if > the HDMI controller <ID> is enabled and only requests pll_hdmiphy<ID> > based on that. Considering there is the OF graph pointing from VOP > to the enabled HDMI controllers, it should be able to do that. I was more thinking about fixing the correct thing, with something like: ----------- 8< ---------- diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index cf7720b9172f..50faafbf5dda 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -5258,6 +5258,10 @@ of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec) if (!clkspec) return ERR_PTR(-EINVAL); + /* Check if node in clkspec is in disabled/fail state */ + if (!of_device_is_available(clkspec->np)) + return ERR_PTR(-ENOENT); + mutex_lock(&of_clk_mutex); list_for_each_entry(provider, &of_clk_providers, link) { if (provider->node == clkspec->np) { ----------- 8< ---------- Because right now the clk framework does not handle nodes in failed/disabled state and would defer indefinitly.
Hi, On Tue, Feb 18, 2025 at 03:53:06PM +0100, Heiko Stübner wrote: > Am Dienstag, 18. Februar 2025, 15:13:07 MEZ schrieb Sebastian Reichel: > > On Tue, Feb 18, 2025 at 08:17:46PM +0800, Jianfeng Liu wrote: > > > On Tue, 18 Feb 2025 11:00:57 +0100, Heiko Stübnerwrote: > > > >So I guess step1, check what error is actually returned. > > > > > > I have checked that the return value is -517: > > > > > > rockchip-drm display-subsystem: [drm] *ERROR* failed to get pll_hdmiphy1 with -517 > > > > > > >Step2 check if clk_get_optional need to be adapted or alternatively > > > >catch the error in the vop2 and set the clock to NULL ourself in that case. > > > > > > I tried the following patch to set the clock to NULL when clk_get_optional > > > failed with value -517, and hdmi0 is working now. There are also some > > > boards like rock 5 itx which only use hdmi1, I think we should also add > > > this logic to vop2->pll_hdmiphy0. > > > > > > @@ -3733,6 +3751,15 @@ static int vop2_bind(struct device *dev, struct device *master, void *data) > > > return PTR_ERR(vop2->pll_hdmiphy0); > > > } > > > > > > + vop2->pll_hdmiphy1 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy1"); > > > + if (IS_ERR(vop2->pll_hdmiphy1)) { > > > + drm_err(vop2->drm, "failed to get pll_hdmiphy1 with %d\n", vop2->pll_hdmiphy1); > > > + if (vop2->pll_hdmiphy1 == -EPROBE_DEFER) > > > + vop2->pll_hdmiphy1 = NULL; > > > + else > > > + return PTR_ERR(vop2->pll_hdmiphy1); > > > + } > > > + > > > > This first of all shows, that we should replace drm_err in this > > place with dev_err_probe(), which hides -EPROBE_DEFER errors by > > default and instead captures the reason for /sys/kernel/debug/devices_deferred. > > > > Second what you are doing in the above suggestion will break kernel > > configurations where VOP is built-in and the HDMI PHY is build as a > > module. > > > > But I also think it would be better to have the clocks defined in the > > SoC level DT. I suppose that means vop2_bind would have to check if > > the HDMI controller <ID> is enabled and only requests pll_hdmiphy<ID> > > based on that. Considering there is the OF graph pointing from VOP > > to the enabled HDMI controllers, it should be able to do that. > > > I was more thinking about fixing the correct thing, with something like: > > ----------- 8< ---------- > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index cf7720b9172f..50faafbf5dda 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -5258,6 +5258,10 @@ of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec) > if (!clkspec) > return ERR_PTR(-EINVAL); > > + /* Check if node in clkspec is in disabled/fail state */ > + if (!of_device_is_available(clkspec->np)) > + return ERR_PTR(-ENOENT); > + > mutex_lock(&of_clk_mutex); > list_for_each_entry(provider, &of_clk_providers, link) { > if (provider->node == clkspec->np) { > ----------- 8< ---------- > > Because right now the clk framework does not handle nodes in > failed/disabled state and would defer indefinitly. Also LGTM. Greetings, -- Sebastian
Hi, On 2/18/25 6:05 PM, Sebastian Reichel wrote: > Hi, > > On Tue, Feb 18, 2025 at 03:53:06PM +0100, Heiko Stübner wrote: >> Am Dienstag, 18. Februar 2025, 15:13:07 MEZ schrieb Sebastian Reichel: >>> On Tue, Feb 18, 2025 at 08:17:46PM +0800, Jianfeng Liu wrote: >>>> On Tue, 18 Feb 2025 11:00:57 +0100, Heiko Stübnerwrote: >>>>> So I guess step1, check what error is actually returned. >>>> >>>> I have checked that the return value is -517: >>>> >>>> rockchip-drm display-subsystem: [drm] *ERROR* failed to get pll_hdmiphy1 with -517 >>>> >>>>> Step2 check if clk_get_optional need to be adapted or alternatively >>>>> catch the error in the vop2 and set the clock to NULL ourself in that case. >>>> >>>> I tried the following patch to set the clock to NULL when clk_get_optional >>>> failed with value -517, and hdmi0 is working now. There are also some >>>> boards like rock 5 itx which only use hdmi1, I think we should also add >>>> this logic to vop2->pll_hdmiphy0. >>>> >>>> @@ -3733,6 +3751,15 @@ static int vop2_bind(struct device *dev, struct device *master, void *data) >>>> return PTR_ERR(vop2->pll_hdmiphy0); >>>> } >>>> >>>> + vop2->pll_hdmiphy1 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy1"); >>>> + if (IS_ERR(vop2->pll_hdmiphy1)) { >>>> + drm_err(vop2->drm, "failed to get pll_hdmiphy1 with %d\n", vop2->pll_hdmiphy1); >>>> + if (vop2->pll_hdmiphy1 == -EPROBE_DEFER) >>>> + vop2->pll_hdmiphy1 = NULL; >>>> + else >>>> + return PTR_ERR(vop2->pll_hdmiphy1); >>>> + } >>>> + >>> >>> This first of all shows, that we should replace drm_err in this >>> place with dev_err_probe(), which hides -EPROBE_DEFER errors by >>> default and instead captures the reason for /sys/kernel/debug/devices_deferred. >>> >>> Second what you are doing in the above suggestion will break kernel >>> configurations where VOP is built-in and the HDMI PHY is build as a >>> module. >>> >>> But I also think it would be better to have the clocks defined in the >>> SoC level DT. I suppose that means vop2_bind would have to check if >>> the HDMI controller <ID> is enabled and only requests pll_hdmiphy<ID> >>> based on that. Considering there is the OF graph pointing from VOP >>> to the enabled HDMI controllers, it should be able to do that. >> >> >> I was more thinking about fixing the correct thing, with something like: >> >> ----------- 8< ---------- >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >> index cf7720b9172f..50faafbf5dda 100644 >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c >> @@ -5258,6 +5258,10 @@ of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec) >> if (!clkspec) >> return ERR_PTR(-EINVAL); >> >> + /* Check if node in clkspec is in disabled/fail state */ >> + if (!of_device_is_available(clkspec->np)) >> + return ERR_PTR(-ENOENT); >> + >> mutex_lock(&of_clk_mutex); >> list_for_each_entry(provider, &of_clk_providers, link) { >> if (provider->node == clkspec->np) { >> ----------- 8< ---------- >> >> Because right now the clk framework does not handle nodes in >> failed/disabled state and would defer indefinitly. > > Also LGTM. Thank you all for the feedback and proposed solutions! I'm currently on leave and without access to any testing hw, but I'll be back in a couple of days. Regards, Cristian
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi index 97e55990e0524ed447d182cef416190822bf67be..1df8845bdc264b07601add3747b273f92091e7fa 100644 --- a/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi @@ -542,3 +542,24 @@ pcie30phy: phy@fee80000 { status = "disabled"; }; }; + +&vop { + clocks = <&cru ACLK_VOP>, + <&cru HCLK_VOP>, + <&cru DCLK_VOP0>, + <&cru DCLK_VOP1>, + <&cru DCLK_VOP2>, + <&cru DCLK_VOP3>, + <&cru PCLK_VOP_ROOT>, + <&hdptxphy0>, + <&hdptxphy1>; + clock-names = "aclk", + "hclk", + "dclk_vp0", + "dclk_vp1", + "dclk_vp2", + "dclk_vp3", + "pclk_vop", + "pll_hdmiphy0", + "pll_hdmiphy1"; +};
VOP2 on RK3588 is able to use the HDMI PHY PLL as an alternative and more accurate pixel clock source to improve handling of display modes up to 4K@60Hz on video ports 0, 1 and 2. The HDMI1 PHY PLL clock source cannot be added directly to vop node in rk3588-base.dtsi, along with the HDMI0 related one, because HDMI1 is an optional feature and its PHY node belongs to a separate (extra) DT file. Therefore, add the HDMI1 PHY PLL clock source to VOP2 by overwriting its clocks & clock-names properties in the extra DT file. Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)