Re: [PATCH v10 12/24] drm/rockchip: dw_hdmi: drop mode_valid hook

[Alex Bee <knaerzche@gmail.com>]

Am 11.04.22 um 09:53 schrieb Sascha Hauer:
> On Sun, Apr 10, 2022 at 01:31:23PM +0200, Alex Bee wrote:
>> Am 08.04.22 um 13:22 schrieb Sascha Hauer:
>>> The driver checks if the pixel clock of the given mode matches an entry
>>> in the mpll config table. The frequencies in the mpll table are meant as
>>> a frequency range up to which the entry works, not as a frequency that
>>> must match the pixel clock. The downstream Kernel also does not have
>>> this check, so drop it to allow for more display resolutions.
>>>
>>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>>> ---
>>>
>> You're correct: That frequency is meant to be greater or equal. But I'm
>> not sure if it makes sense to completely drop it - what happens for
>> clocks rates > 600 MHz which might be supported by later generation
>> sinks (HDMI 2.1 or later)?
>> As these are not supported by the IPs/PHYs currently supported by that
>> driver a reason a simple
>>
>>         int i;
>>
>>
>>
>>         for (i = 0; mpll_cfg[i].mpixelclock != (~0UL); i++) {
>>
>> -               if (pclk == mpll_cfg[i].mpixelclock) {
>>
>> +               if (pclk >= mpll_cfg[i].mpixelclock) {
> 
> Should be <=

Sure, sorry 'bout that.
> 
> No other user currently in the tree has this check.
I dont't think that's true - it might not be checked against mpll table,
but max pixelclocks are checked in meson_encoder_hdmi_mode_valid for
amlogic, in sun8i_dw_hdmi_mode_valid_a83t and
sun8i_dw_hdmi_mode_valid_h6 for allwinner and rcar_hdmi_mode_valid for
rcar platform. There is no other point in rockchip dw-hdmi platform
driver where this is currently checked.

> hdmi_phy_configure_dwc_hdmi_3d_tx() has this:
> 
>> 	/* PLL/MPLL Cfg - always match on final entry */
>> 	for (; mpll_config->mpixelclock != ~0UL; mpll_config++)
>> 		if (mpixelclock <= mpll_config->mpixelclock)
>> 			break;
>>
>> 	for (; curr_ctrl->mpixelclock != ~0UL; curr_ctrl++)
>> 		if (mpixelclock <= curr_ctrl->mpixelclock)
>> 			break;
>>
>> 	for (; phy_config->mpixelclock != ~0UL; phy_config++)
>> 		if (mpixelclock <= phy_config->mpixelclock)
>> 			break;
>>
>> 	if (mpll_config->mpixelclock == ~0UL ||
>> 	    curr_ctrl->mpixelclock == ~0UL ||
>> 	    phy_config->mpixelclock == ~0UL)
>> 		return -EINVAL;
> 
> This means we already have this check and others already in the generic
> part of the dw-hdmi driver. Should we maybe do the above in
> dw_hdmi_bridge_mode_valid() instead of doing it in the users?
I guess that's not possible, due to the different kind of phys which are
used for dw-hdmi. The checks you are refering to, are only done for dw
hdmi tx phys, but not for "vendor phys" (like for RK3328 and RK3228, for
example) - those have have own drivers which are handled only in dw-hdmi
platform driver.
Therefore this check should remain here for Rockchip also.

Regards,
Alex
> 
> Sascha
> 
>