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

[Jonas Karlman <jonas@kwiboo.se>]

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
>