Re: [PATCH] drm/rockchip: vop2: Drop unused if_dclk_rate variable

[Andy Yan <andy.yan@rock-chips.com>]

Hi Cristian:

On 1/5/24 03:12, Cristian Ciocaltea wrote:
> Hi Heiko,
> 
> On 1/4/24 17:58, Heiko Stübner wrote:
>> Hi Christian, Andy,
>>
>> Am Donnerstag, 4. Januar 2024, 15:39:50 CET schrieb Cristian Ciocaltea:
>>> Commit 5a028e8f062f ("drm/rockchip: vop2: Add support for rk3588")
>>> introduced a variable which ended up being unused.  Remove it.
>>>
>>> rockchip_drm_vop2.c:1688:23: warning: variable ‘if_dclk_rate’ set but not used [-Wunused-but-set-variable]
>>>
>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>
>> in general, please don't send non-series patches as replies to other patches.
>> It confuses tooling like b4 way too often, as this patch is not designated
>> as a 2/2 (similar to the first one not being 1/2).
> 
> That was unintentional, sorry!  I wrongly assumed 'git send-email' is
> able to correctly handle multiple patches which are not part of a
> series.  I'm not sure if the '--no-thread' flag would have helped.
> 
>>> ---
>>>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>> index 44508c2dd614..923985d4161b 100644
>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>> @@ -1685,7 +1685,6 @@ static unsigned long rk3588_calc_cru_cfg(struct vop2_video_port *vp, int id,
>>>   	unsigned long dclk_core_rate = v_pixclk >> 2;
>>>   	unsigned long dclk_rate = v_pixclk;
>>>   	unsigned long dclk_out_rate;
>>> -	unsigned long if_dclk_rate;
>>>   	unsigned long if_pixclk_rate;
>>>   	int K = 1;
>>>   
>>> @@ -1700,7 +1699,6 @@ static unsigned long rk3588_calc_cru_cfg(struct vop2_video_port *vp, int id,
>>>   		}
>>>   
>>>   		if_pixclk_rate = (dclk_core_rate << 1) / K;
>>> -		if_dclk_rate = dclk_core_rate / K;
>>>   		/*
>>>   		 * *if_pixclk_div = dclk_rate / if_pixclk_rate;
>>>   		 * *if_dclk_div = dclk_rate / if_dclk_rate;
>>>   		 */
>> 		*if_pixclk_div = 2;
>> 		*if_dclk_div = 4;
>>
>> with the code continuing with those static constants but the comment
>> showing a forumula, I do hope Andy can provide a bit of insight into
>> what is happening here.
>>
>> I.e. I'd really like to understand if that really is just a remnant or
>> something different is needed.
> 
> The current implementation is not able to handle all display modes
> supported by connected displays, e.g. in my testing environment I
> encountered issues with 2560x1440-75.00Hz, 2048x1152-60.00Hz,
> 1024x768-60.00Hz.  Additionally, it doesn't seem to cope well with
> non-integer refresh rates like 59.94, 29.97, 23.98, etc.

I think this is because the thee PLL from cru can't divide accurate clock for these
non-integer refresh rates.
> 
> My temporary workaround relies on using the HDMI PHY PLL in conjunction
> with a downstream-based hack to compute the clock rates.  I'm not sure
> that would be an upstreamable solution, so I would let Andy shed some
> light on the topic.

Yes, use PLL from HDMI PHY can give more flexible clock rates to support more display mode.
We also use it in our bsp kernel, but one thing should  keep in mind that  use HDMI PHY pll
as dclk source can only work for HDMI 2.0 or bellow, if can't be used in hdmi 2.1 mode(such as 4K120),
so we need to switch the clock source by hdmi work mode.

The difficult thing is how to make this accepted by upstream.
> 
> Thanks,
> Cristian
> 
>>
>> Heiko
>>
>>
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip