Re: 回复: Re: 回复: Re: [PATCH] drm:Fix the blank screen problem of some 1920x1080 75Hz monitors using R520 graphics card
Christian König
christian.koenig at amd.com
Wed Sep 14 08:40:35 UTC 2022
> 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.
Am 14.09.22 um 04:17 schrieb 钟沛:
>
> 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 at amd.comXinhui.Pan@amd.comairlied at linux.iedaniel@ffwll.chisabbasso at riseup.net
>
>
>
> 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
> *收件人：*钟沛alexander.deucher at amd.comXinhui.Pan@amd.comairlied at linux.iedaniel@ffwll.chisabbasso at riseup.net
>
>
>
> 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 at kylinos.cn>
> > ---
> > 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 */
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20220914/af0a7197/attachment-0001.htm>
More information about the amd-gfx
mailing list