If the diff is smaller when the value of ref_div_max is 100, set the value of ref_div_max to 100. Otherwise, set the value of ref_div_max to 128. In this way, the value of ref_div_max can be determined according to the value of diff, in order to adapt to all monitors. After our verification on different monitors, those monitors that have a blank screen problem when the value of ref_div_max is 100 or 128 can work normally under this scheme.
Well sorry to say that, but once more: This approach is just blank nonsense!
The problem with a larger reference/post dividers is that you your intermediate frequency becomes to large and the PLL starts to swing between two frequencies which are above and below the desired result.
So a maximum ref_div of 128 is simply not acceptable for the hardware.
Regards,
Christian.
Sorry to trouble you, we ran some tests on this patch and want to communicate with you.
static void amdgpu_pll_get_fb_ref_div(struct amdgpu_device *adev, unsigned nom, unsigned den, unsigned post_div,
unsigned fb_div_max, unsigned ref_div_max,
unsigned *fb_div, unsigned *ref_div)
{
/* limit reference * post divider to a maximum */
if (adev->family == AMDGPU_FAMILY_SI)
ref_div_max = min(100 / post_div, ref_div_max);
else
ref_div_max = min(128 / 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);
*fb_div = DIV_ROUND_CLOSEST(nom * *ref_div * post_div, den);
/* limit fb divider to its maximum */
if (*fb_div > fb_div_max) {
*ref_div = DIV_ROUND_CLOSEST(*ref_div * fb_div_max, *fb_div);
*fb_div = fb_div_max;
}
}
For R520, the max value of the ref_div_max in this function used to be 128. In order to fix the black screen problem of some monitors, it was changed to 100. From the commit message at the time, we learned that this was a temporary solution. When the value of ref_div_max is 100, it will also cause problems with some monitors.
void amdgpu_pll_compute(struct amdgpu_device *adev, struct amdgpu_pll *pll, u32 freq, u32 *dot_clock_p,
u32 *fb_div_p,
u32 *frac_fb_div_p,
u32 *ref_div_p,
u32 *post_div_p)
{
......
/* now search for a post divider */
if (pll->flags & AMDGPU_PLL_PREFER_MINM_OVER_MAXP)
post_div_best = post_div_min;
else
post_div_best = post_div_max;
diff_best = ~0;
for (post_div = post_div_min; post_div <= post_div_max; ++post_div) {
unsigned diff;
amdgpu_pll_get_fb_ref_div(adev, nom, den, post_div, fb_div_max,
ref_div_max, &fb_div, &ref_div);
diff = abs(target_clock - (pll->reference_freq * fb_div) /
(ref_div * post_div));
if (diff < diff_best || (diff == diff_best &&
!(pll->flags & AMDGPU_PLL_PREFER_MINM_OVER_MAXP))) {
post_div_best = post_div;
diff_best = diff;
}
}
post_div = post_div_best;
......
}
This piece of code in the above function is to find a post_div, so that the value of diff = abs(target_clock - (pll->reference_freq * fb_div) /(ref_div * post_div)) is the smallest. The pixel clock closest to the target frequency when the value of the diff is the smallest. The values of the reference divider(ref_div) and feedback divider(fb_div) in the above formula are affected by ref_div_max, so when the value of ref_div_max is different, the value of diff may also be different. The larger value of diff may cause the blank screen problem of some monitors.
A value of 100 or 128 for ref_div_max is not suitable for all monitors. So we set it to 100 and 128 in turn to calculate the corresponding ref_div and fb_div values, and then calculate diff = abs(target_clock - (pll->reference_freq * fb_div) /(ref_div * post_div)). If the diff is smaller when the value of ref_div_max is 100, set the value of ref_div_max to 100. Otherwise, set the value of ref_div_max to 128. In this way, the value of ref_div_max can be determined according to the value of diff, in order to adapt to all monitors. After our verification on different monitors, those monitors that have a blank screen problem when the value of ref_div_max is 100 or 128 can work normally under this scheme.
We believe that temporarily adopting this method to replace the previous method can improve the compatibility of the graphics card.
Best Regards.
----
主 题：Re: 回复: Re: [PATCH] drm:Fix the blank screen problem of some 1920x1080 75Hz monitors using R520 graphics card
日 期：2022-09-05 19:12
发件人：Christian König
收件人：钟沛alexander.deucher@xxxxxxxxxxxxxxxxx@amd.comairlied@linux.iedaniel@ffwll.chisabbasso@xxxxxxxxxx
Am 05.09.22 um 10:10 schrieb 钟沛:
Thanks for your reply!
We found that in the amdgpu_pll_compute function, when the target_clock is the value contained in the drm_dmt_modes defined in drm_edid.c, the diff is 0. When target_clock is some special value, we cannot find a diff value of 0, so we need to find the smallest diff value to fit the current target_clock. For the monitor that has the blank screen problem here, we found that when the ref_div_max is 128, the diff value is smaller and the blank screen problem can be solved. We tested some other monitors and added log printing to the code. We found that this change did not affect those monitors, and in the analysis of the logs, we found that the solution with a smaller diff value always displayed normally.
Changing the value of ref_div_max from 128 to 100 can solve the blank screen problem of some monitors, but it will also cause some normal monitors to go black, so is it a more reasonable solution to determine the value of ref_div_max according to the value of diff?
Nope, exactly that's just utterly nonsense.
What we could maybe do is to prefer a smaller ref_div over a larger ref_div, but I don't see how this will help you.
Regards,
Christian.
Thank you for taking the time to read my email.
Best Regards.
----
主 题：Re: [PATCH] drm:Fix the blank screen problem of some 1920x1080 75Hz monitors using R520 graphics card
日 期：2022-09-05 14:05
发件人：Christian König
收件人：钟沛a lexander.deucher@xxxxxxxxxxxxxxxxx@amd.comairlied@linux.iedaniel@ffwll.chisabbasso@xxxxxxxxxx
Am 05.09.22 um 05:23 schrieb zhongpei:
> We found that in the scenario of AMD R520 graphics card
> and some 1920x1080 monitors,when we switch the refresh rate
> of the monitor to 75Hz,the monitor will have a blank screen problem,
> and the restart cannot be restored.After testing, it is found that
> when we limit the maximum value of ref_div_max to 128,
> the problem can be solved.In order to keep the previous modification
> to be compatible with other monitors,we added a judgment
> when finding the minimum diff value in the loop of the
> amdgpu_pll_compute/radeon_compute_pll_avivo function.
> If no diff value of 0 is found when the maximum value of ref_div_max
> is limited to 100,continue to search when it is 128,
> and take the parameter with the smallest diff value.
Well that's at least better than what I've seen in previous tries to fix
this.
But as far as I can see this will certainly break some other monitors,
so that is pretty much a NAK.
Regards,
Christian.
>
> Signed-off-by: zhongpei <zhongpei@xxxxxxxxxx>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_pll.c | 17 +++++++++++++----
> drivers/gpu/drm/radeon/radeon_display.c | 15 +++++++++++----
> 2 files changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pll.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pll.c
> index 0bb2466d539a..0c298faa0f94 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pll.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pll.c
> @@ -84,12 +84,13 @@ static void amdgpu_pll_reduce_ratio(unsigned *nom, unsigned *den,
> static void amdgpu_pll_get_fb_ref_div(struct amdgpu_device *adev, unsigned int nom,
> unsigned int den, unsigned int post_div,
> unsigned int fb_div_max, unsigned int ref_div_max,
> - unsigned int *fb_div, unsigned int *ref_div)
> + unsigned int ref_div_limit, unsigned int *fb_div,
> + unsigned int *ref_div)
> {
>
> /* limit reference * post divider to a maximum */
> if (adev->family == AMDGPU_FAMILY_SI)
> - ref_div_max = min(100 / post_div, ref_div_max);
> + ref_div_max = min(ref_div_limit / post_div, ref_div_max);
> else
> ref_div_max = min(128 / post_div, ref_div_max);
>
> @@ -136,6 +137,7 @@ void amdgpu_pll_compute(struct amdgpu_device *adev,
> unsigned ref_div_min, ref_div_max, ref_div;
> unsigned post_div_best, diff_best;
> unsigned nom, den;
> + unsigned ref_div_limit, ref_limit_best;
>
> /* determine allowed feedback divider range */
> fb_div_min = pll->min_feedback_div;
> @@ -204,11 +206,12 @@ void amdgpu_pll_compute(struct amdgpu_device *adev,
> else
> post_div_best = post_div_max;
> diff_best = ~0;
> + ref_div_limit = ref_limit_best = 100;
>
> for (post_div = post_div_min; post_div <= post_div_max; ++post_div) {
> unsigned diff;
> amdgpu_pll_get_fb_ref_div(adev, nom, den, post_div, fb_div_max,
> - ref_div_max, &fb_div, &ref_div);
> + ref_div_max, ref_div_limit, &fb_div, &ref_div);
> diff = abs(target_clock - (pll->reference_freq * fb_div) /
> (ref_div * post_div));
>
> @@ -217,13 +220,19 @@ void amdgpu_pll_compute(struct amdgpu_device *adev,
>
> post_div_best = post_div;
> diff_best = diff;
> + ref_limit_best = ref_div_limit;
> }
> + if (post_div >= post_div_max && diff_best != 0 && ref_div_limit != 128) {
> + ref_div_limit = 128;
> + post_div = post_div_min - 1;
> + }
> +
> }
> post_div = post_div_best;
>
> /* get the feedback and reference divider for the optimal value */
> amdgpu_pll_get_fb_ref_div(adev, nom, den, post_div, fb_div_max, ref_div_max,
> - &fb_div, &ref_div);
> + ref_limit_best, &fb_div, &ref_div);
>
> /* reduce the numbers to a simpler ratio once more */
> /* this also makes sure that the reference divider is large enough */
> diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
> index f12675e3d261..0fcbf45a68db 100644
> --- a/drivers/gpu/drm/radeon/radeon_display.c
> +++ b/drivers/gpu/drm/radeon/radeon_display.c
> @@ -925,10 +925,10 @@ static void avivo_reduce_ratio(unsigned *nom, unsigned *den,
> */
> static void avivo_get_fb_ref_div(unsigned nom, unsigned den, unsigned post_div,
> unsigned fb_div_max, unsigned ref_div_max,
> - unsigned *fb_div, unsigned *ref_div)
> + unsigned ref_div_limit, unsigned *fb_div, unsigned *ref_div)
> {
> /* limit reference * post divider to a maximum */
> - ref_div_max = max(min(100 / post_div, ref_div_max), 1u);
> + ref_div_max = max(min(ref_div_limit / post_div, ref_div_max), 1u);
>
> /* get matching reference and feedback divider */
> *ref_div = min(max(den/post_div, 1u), ref_div_max);
> @@ -971,6 +971,7 @@ void radeon_compute_pll_avivo(struct radeon_pll *pll,
> unsigned ref_div_min, ref_div_max, ref_div;
> unsigned post_div_best, diff_best;
> unsigned nom, den;
> + unsigned ref_div_limit, ref_limit_best;
>
> /* determine allowed feedback divider range */
> fb_div_min = pll->min_feedback_div;
> @@ -1042,11 +1043,12 @@ void radeon_compute_pll_avivo(struct radeon_pll *pll,
> else
> post_div_best = post_div_max;
> diff_best = ~0;
> + ref_div_limit = ref_limit_best = 100;
>
> for (post_div = post_div_min; post_div <= post_div_max; ++post_div) {
> unsigned diff;
> avivo_get_fb_ref_div(nom, den, post_div, fb_div_max,
> - ref_div_max, &fb_div, &ref_div);
> + ref_div_max, ref_div_limit, &fb_div, &ref_div);
> diff = abs(target_clock - (pll->reference_freq * fb_div) /
> (ref_div * post_div));
>
> @@ -1055,13 +1057,18 @@ void radeon_compute_pll_avivo(struct radeon_pll *pll,
>
> post_div_best = post_div;
> diff_best = diff;
> + ref_limit_best = ref_div_limit;
> + }
> + if (post_div >= post_div_max && diff_best != 0 && ref_div_limit != 128) {
> + ref_div_limit = 128;
> + post_div = post_div_min - 1;
> }
> }
> post_div = post_div_best;
>
> /* get the feedback and reference divider for the optimal value */
> avivo_get_fb_ref_div(nom, den, post_div, fb_div_max, ref_div_max,
> - &fb_div, &ref_div);
> + ref_limit_best, &fb_div, &ref_div);
>
> /* reduce the numbers to a simpler ratio once more */
> /* this also makes sure that the reference divider is large enough */
