diff mbox series

[v4,4/4] drm/rockchip: dw_hdmi: discard modes with unachievable pixelclocks

Message ID 20230207084452.1069656-5-s.hauer@pengutronix.de (mailing list archive)
State New, archived
Headers show
Series drm/rockchip: dw_hdmi: Add 4k@30 support | expand

Commit Message

Sascha Hauer Feb. 7, 2023, 8:44 a.m. UTC
The Rockchip PLL drivers are currently table based and support only
the most common pixelclocks. Discard all modes we cannot achieve
at all. Normally the desired pixelclocks have an exact match in the
PLL driver, nevertheless allow for a 0.1% error just in case.

Tested-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
Tested-by: Michael Riesch <michael.riesch@wolfvision.net>
Tested-by: Dan Johansen <strit@manjaro.org>
Link: https://lore.kernel.org/r/20230118132213.2911418-4-s.hauer@pengutronix.de
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Jonas Karlman Feb. 7, 2023, 11:01 a.m. UTC | #1
Hi Sascha,

On 2023-02-07 09:44, Sascha Hauer wrote:
> The Rockchip PLL drivers are currently table based and support only
> the most common pixelclocks. Discard all modes we cannot achieve
> at all. Normally the desired pixelclocks have an exact match in the
> PLL driver, nevertheless allow for a 0.1% error just in case.
> 
> Tested-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
> Tested-by: Michael Riesch <michael.riesch@wolfvision.net>
> Tested-by: Dan Johansen <strit@manjaro.org>
> Link: https://lore.kernel.org/r/20230118132213.2911418-4-s.hauer@pengutronix.de
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index feba6b9becd6c..725952811752b 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -256,10 +256,14 @@ dw_hdmi_rockchip_mode_valid(struct dw_hdmi *dw_hdmi, void *data,
>  {
>  	struct rockchip_hdmi *hdmi = data;
>  	const struct dw_hdmi_mpll_config *mpll_cfg = rockchip_mpll_cfg;
> -	int pclk = mode->clock * 1000;
> +	int rpclk, pclk = mode->clock * 1000;
>  	bool exact_match = hdmi->plat_data->phy_force_vendor;
>  	int i;
>  
> +	rpclk = clk_round_rate(hdmi->ref_clk, pclk);
> +	if (abs(rpclk - pclk) > pclk / 1000)
> +		return MODE_NOCLOCK;

The ref_clk is optional and rk3228/rk3328 dts do not supply a ref or vpll clock.

Regards,
Jonas

> +
>  	for (i = 0; mpll_cfg[i].mpixelclock != (~0UL); i++) {
>  		/*
>  		 * For vendor specific phys force an exact match of the pixelclock
Sascha Hauer Feb. 7, 2023, 12:51 p.m. UTC | #2
On Tue, Feb 07, 2023 at 11:01:26AM +0000, Jonas Karlman wrote:
> Hi Sascha,
> 
> On 2023-02-07 09:44, Sascha Hauer wrote:
> > The Rockchip PLL drivers are currently table based and support only
> > the most common pixelclocks. Discard all modes we cannot achieve
> > at all. Normally the desired pixelclocks have an exact match in the
> > PLL driver, nevertheless allow for a 0.1% error just in case.
> > 
> > Tested-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
> > Tested-by: Michael Riesch <michael.riesch@wolfvision.net>
> > Tested-by: Dan Johansen <strit@manjaro.org>
> > Link: https://lore.kernel.org/r/20230118132213.2911418-4-s.hauer@pengutronix.de
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > index feba6b9becd6c..725952811752b 100644
> > --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > @@ -256,10 +256,14 @@ dw_hdmi_rockchip_mode_valid(struct dw_hdmi *dw_hdmi, void *data,
> >  {
> >  	struct rockchip_hdmi *hdmi = data;
> >  	const struct dw_hdmi_mpll_config *mpll_cfg = rockchip_mpll_cfg;
> > -	int pclk = mode->clock * 1000;
> > +	int rpclk, pclk = mode->clock * 1000;
> >  	bool exact_match = hdmi->plat_data->phy_force_vendor;
> >  	int i;
> >  
> > +	rpclk = clk_round_rate(hdmi->ref_clk, pclk);
> > +	if (abs(rpclk - pclk) > pclk / 1000)
> > +		return MODE_NOCLOCK;
> 
> The ref_clk is optional and rk3228/rk3328 dts do not supply a ref or vpll clock.

That's a bit unfortunate as we can't do this check then on these SoCs.

The clock is likely actually there in the system and maybe even in the
clock driver, just not wired up to the HDMI. I don't know which one it
is though, so I am afraid there's not much I can do about it other than
just skipping the check when the clock is not there.

Sascha
Jonas Karlman Feb. 7, 2023, 4:29 p.m. UTC | #3
Hi Sascha,

On 2023-02-07 13:51, Sascha Hauer wrote:
> On Tue, Feb 07, 2023 at 11:01:26AM +0000, Jonas Karlman wrote:
>> Hi Sascha,
>>
>> On 2023-02-07 09:44, Sascha Hauer wrote:
>>> The Rockchip PLL drivers are currently table based and support only
>>> the most common pixelclocks. Discard all modes we cannot achieve
>>> at all. Normally the desired pixelclocks have an exact match in the
>>> PLL driver, nevertheless allow for a 0.1% error just in case.
>>>
>>> Tested-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
>>> Tested-by: Michael Riesch <michael.riesch@wolfvision.net>
>>> Tested-by: Dan Johansen <strit@manjaro.org>
>>> Link: https://lore.kernel.org/r/20230118132213.2911418-4-s.hauer@pengutronix.de>>>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>>> ---
>>>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>> index feba6b9becd6c..725952811752b 100644
>>> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>> @@ -256,10 +256,14 @@ dw_hdmi_rockchip_mode_valid(struct dw_hdmi *dw_hdmi, void *data,
>>>  {
>>>  	struct rockchip_hdmi *hdmi = data;
>>>  	const struct dw_hdmi_mpll_config *mpll_cfg = rockchip_mpll_cfg;
>>> -	int pclk = mode->clock * 1000;
>>> +	int rpclk, pclk = mode->clock * 1000;
>>>  	bool exact_match = hdmi->plat_data->phy_force_vendor;
>>>  	int i;
>>>  
>>> +	rpclk = clk_round_rate(hdmi->ref_clk, pclk);
>>> +	if (abs(rpclk - pclk) > pclk / 1000)
>>> +		return MODE_NOCLOCK;
>>
>> The ref_clk is optional and rk3228/rk3328 dts do not supply a ref or vpll clock.
> 
> That's a bit unfortunate as we can't do this check then on these SoCs.
> 
> The clock is likely actually there in the system and maybe even in the
> clock driver, just not wired up to the HDMI. I don't know which one it
> is though, so I am afraid there's not much I can do about it other than
> just skipping the check when the clock is not there.

For rk3228/rk3328 I think just skipping the rate check when there is
no ref_clk should be fine.

The clock is provided by phy-rockchip-inno-hdmi.c for rk3228/rk3328. And
I recall we used to add ref/vpll clock to device tree to do something
similar in LibreELEC with our initial work-in-progress HDMI2.0 patches.

We have since then opted to just filter modes based on clk_round_rate
for !phy_force_vendor and instead extend the inno-hdmi pll table with
common hdmi/dvi clock rates, see [1] for the current state of RK HDMI2.0
related patches we use in LibreELEC.

Hopefully I can find some time in coming weeks to restart the work of
testing and sending those patches upstream, rebased on top of this series.

[1] https://github.com/Kwiboo/linux-rockchip/compare/v6.2-rc7...rockchip-6.2-hdmi2.0

Regards,
Jonas

> 
> Sascha
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index feba6b9becd6c..725952811752b 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -256,10 +256,14 @@  dw_hdmi_rockchip_mode_valid(struct dw_hdmi *dw_hdmi, void *data,
 {
 	struct rockchip_hdmi *hdmi = data;
 	const struct dw_hdmi_mpll_config *mpll_cfg = rockchip_mpll_cfg;
-	int pclk = mode->clock * 1000;
+	int rpclk, pclk = mode->clock * 1000;
 	bool exact_match = hdmi->plat_data->phy_force_vendor;
 	int i;
 
+	rpclk = clk_round_rate(hdmi->ref_clk, pclk);
+	if (abs(rpclk - pclk) > pclk / 1000)
+		return MODE_NOCLOCK;
+
 	for (i = 0; mpll_cfg[i].mpixelclock != (~0UL); i++) {
 		/*
 		 * For vendor specific phys force an exact match of the pixelclock