[PATCH] drm/amdgpu: clip the ref divider max value at 100

[Shashank Sharma]

On 04/11/20 5:24 pm, Christian König wrote:
> Am 04.11.20 um 11:40 schrieb Sharma, Shashank:
>> [AMD Public Use]
>>
>> Hello Christian,
>> Yes, that 100 is hardcoded in Radeon, and git blame says it was one of your patches which made it 100 from 128 😊.
>> Would you mind having a look at commit id: 4b21ce1b4b5d262e7d4656b8ececc891fc3cb806 ?
> Ah, yes that one :)
>
> Yeah the background is that this was just an educated guess because I 
> couldn't find anybody which could tell me what the real limits of the 
> PLL is.
>
> Looks like we just forgot to apply that patch to amdgpu.
>
> Regards,
> Christian.


Cool, I was wondering if we should let this patch go through some elaborated Pixel clock testing on different resolution, or is it ok to go ahead with this patch based on the experience from Radeon driver change ?

Regards

Shashank

>> Regards
>> Shashank
>> -----Original Message-----
>> From: Koenig, Christian <Christian.Koenig at amd.com>
>> Sent: Wednesday, November 4, 2020 3:35 PM
>> To: Sharma, Shashank <Shashank.Sharma at amd.com>; amd-gfx at lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Qin, Eddy <Eddy.Qin at amd.com>
>> Subject: Re: [PATCH] drm/amdgpu: clip the ref divider max value at 100
>>
>> Am 03.11.20 um 18:13 schrieb Shashank Sharma:
>>> This patch limits the ref_div_max value to 100, during the calculation
>>> of PLL feedback reference divider. With current value (128), the
>>> produced fb_ref_div value generates unstable output at particular
>>> frequencies. Radeon driver limits this value at 100.
>> Mhm, is that 100 hard coded in radeon? I have no idea where that is coming from.
>>
>> Best would probably to grab a hardware engineer and try to figure out what the real maximums for the PLL is to still produce a stable signal.
>>
>> Christian.
>>
>>> On Oland, when we try to setup mode 2048x1280 at 60 (a bit weird, I
>>> know), it demands a clock of 221270 Khz. It's been observed that the
>>> PLL calculations using values 128 and 100 are vastly different, and
>>> look like this:
>>>
>>> +------------------------------------------+
>>> |Parameter    |AMDGPU        |Radeon       |
>>> |             |              |             |
>>> +-------------+----------------------------+
>>> |Clock feedback              |             |
>>> |divider max  |  128         |   100       |
>>> |cap value    |              |             |
>>> |             |              |             |
>>> |             |              |             |
>>> +------------------------------------------+
>>> |ref_div_max  |              |             |
>>> |             |  42          |  20         |
>>> |             |              |             |
>>> |             |              |             |
>>> +------------------------------------------+
>>> |ref_div      |  42          |  20         |
>>> |             |              |             |
>>> +------------------------------------------+
>>> |fb_div       |  10326       |  8195       |
>>> +------------------------------------------+
>>> |fb_div       |  1024        |  163        |
>>> +------------------------------------------+
>>> |fb_dev_p     |  4           |  9          |
>>> |frac fb_de^_p|              |             |
>>> +----------------------------+-------------+
>>>
>>> With ref_div_max value clipped at 100, AMDGPU driver can also drive
>>> videmode 2048x1280 at 60 (221Mhz) and produce proper output without any
>>> blanking and distortion on the screen.
>>>
>>> PS: This value was changed from 128 to 100 in Radeon driver also, here:
>>> https://github.com/freedesktop/drm-tip/commit/4b21ce1b4b5d262e7d4656b8
>>> ececc891fc3cb806
>>>
>>> Cc: Alex Deucher <Alexander.Deucher at amd.com>
>>> Cc: Christian König <christian.koenig at amd.com>
>>> Cc: Eddy Qin <Eddy.Qin at amd.com>
>>>
>>> Signed-off-by: Shashank Sharma <shashank.sharma at amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_pll.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pll.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pll.c
>>> index 1f2305b7bd13..23a2e1ebf78a 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pll.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pll.c
>>> @@ -85,7 +85,7 @@ static void amdgpu_pll_get_fb_ref_div(unsigned nom, unsigned den, unsigned post_
>>>    				      unsigned *fb_div, unsigned *ref_div)
>>>    {
>>>    	/* limit reference * post divider to a maximum */
>>> -	ref_div_max = min(128 / post_div, ref_div_max);
>>> +	ref_div_max = min(100 / post_div, ref_div_max);
>>>    
>>>    	/* get matching reference and feedback divider */
>>>    	*ref_div = min(max(DIV_ROUND_CLOSEST(den, post_div), 1u),
>>> ref_div_max);