Bug 77673 - [bisected] moving moire starting with "drm/radeon: improve PLL params if we don't match exactly v2"
Summary: [bisected] moving moire starting with "drm/radeon: improve PLL params if we d...
Status: RESOLVED FIXED
Alias: None
Product: DRI
Classification: Unclassified
Component: DRM/Radeon (show other bugs)
Version: DRI git
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: Default DRI bug account
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-04-19 15:38 UTC by Jaime Velasco Juan
Modified: 2014-04-20 10:34 UTC (History)
0 users

See Also:
i915 platform:
i915 features:

Attachments
Output of 'xrandr --prop' (1.63 KB, text/plain)
2014-04-19 15:38 UTC, Jaime Velasco Juan 		no flags Details
dmesg of working version (104.41 KB, text/plain)
2014-04-19 15:39 UTC, Jaime Velasco Juan 		no flags Details
dmesg of broken version (98.89 KB, text/plain)
2014-04-19 15:39 UTC, Jaime Velasco Juan 		no flags Details
Possible fix. (1.15 KB, patch)
2014-04-19 16:58 UTC, Christian König 		no flags Details | Splinter Review
View All

Description Jaime Velasco Juan 2014-04-19 15:38:22 UTC 
Created attachment 97606 [details]
Output of 'xrandr --prop'

Hello, I'm testing drm-next and have found this problem. The display shows some kind of moving wavy pattern, noticeable in midtones and grandients. I've bisected it to the mentioned commit. The display is the internal LVDS panel, I haven't tested external displays.


$ git bisect log
git bisect start
# good: [aa3a38dd40b610de206fb04777cc5469ce057de8] drm/radeon: update CI DPM powertune settings
git bisect good aa3a38dd40b610de206fb04777cc5469ce057de8
# bad: [bc00aa0ebfed7edc1ca91ec2d9b0d5b04c5e06a8] drm: bochs: drop unused struct fields
git bisect bad bc00aa0ebfed7edc1ca91ec2d9b0d5b04c5e06a8
# bad: [8bf5dda3777f2a477e3c448b38e18f5e1b502eae] drm/radeon/si: make sure mc ucode is loaded before checking the size
git bisect bad 8bf5dda3777f2a477e3c448b38e18f5e1b502eae
# good: [8a6a70662fc93c2c43ba48de3e46bf7e860e18cc] drm/radeon: re-enable mclk dpm on R7 260X asics
git bisect good 8a6a70662fc93c2c43ba48de3e46bf7e860e18cc
# good: [aeb0c2d8d3fc60ea931f807275166ea8abe1feeb] drm/radeon: memory leak on bo reservation failure. v2
git bisect good aeb0c2d8d3fc60ea931f807275166ea8abe1feeb
# bad: [9b17c2afd74b5f56497ffacbcac85ee2cf0a1c1b] drm/radeon: improve PLL params if we don't match exactly v2
git bisect bad 9b17c2afd74b5f56497ffacbcac85ee2cf0a1c1b
# first bad commit: [9b17c2afd74b5f56497ffacbcac85ee2cf0a1c1b] drm/radeon: improve PLL params if we don't match exactly v2

(the commit ID are local, as I've rebased drm-next and drm-fixes on v3.14).

I attach two kernel logs booting and doing:
echo 0xe >/sys/module/drm/parameters/debug
xrand --rate 50
xrand --rate 60

I think the relevant difference is (first line is good, next is buggy):
- [drm:radeon_compute_pll_avivo] 122000 - 13725, pll dividers - fb: 122.0 ref: 6, post 4
+ [drm:radeon_compute_pll_avivo] 122000 - 12198, pll dividers - fb: 126.5 ref: 7, post 4
Comment 1 Jaime Velasco Juan 2014-04-19 15:39:05 UTC 
Created attachment 97607 [details]
dmesg of working version
Comment 2 Jaime Velasco Juan 2014-04-19 15:39:37 UTC 
Created attachment 97608 [details]
dmesg of broken version
Comment 3 Alex Deucher 2014-04-19 15:51:35 UTC 
Looks like the new pll computation is not respecting the RADEON_PLL_USE_FRAC_FB_DIV flag.  Older asics shouldn't use fraction fb dividers.
Comment 4 Alex Deucher 2014-04-19 16:00:02 UTC 
(In reply to comment #3)
> Looks like the new pll computation is not respecting the
> RADEON_PLL_USE_FRAC_FB_DIV flag.  Older asics shouldn't use fraction fb
> dividers.

Nevermind, we always set the RADEON_PLL_USE_FRAC_FB_DIV for LVDS if spread spectrum is enabled.  Looks like we aren't respecting RADEON_PLL_USE_REF_DIV properly.
Comment 5 Christian König 2014-04-19 16:58:27 UTC 
Created attachment 97609 [details] [review]
Possible fix.
Comment 6 Christian König 2014-04-19 17:00:48 UTC 
Please try the attached patch.

We now always use the fixed reference divider if both RADEON_PLL_USE_REF_DIV and RADEON_PLL_USE_FRAC_FB_DIV are set.

If only RADEON_PLL_USE_REF_DIV is set then we only use the fixed value as minimum limit.
Comment 7 Jaime Velasco Juan 2014-04-19 17:43:28 UTC 
(In reply to comment #6)
> Please try the attached patch.
> 
> We now always use the fixed reference divider if both RADEON_PLL_USE_REF_DIV
> and RADEON_PLL_USE_FRAC_FB_DIV are set.
> 
> If only RADEON_PLL_USE_REF_DIV is set then we only use the fixed value as
> minimum limit.

The path fixes the issue. Thanks!
Comment 8 Alex Deucher 2014-04-20 01:20:05 UTC 
(In reply to comment #6)
> Please try the attached patch.
> 
> We now always use the fixed reference divider if both RADEON_PLL_USE_REF_DIV
> and RADEON_PLL_USE_FRAC_FB_DIV are set.
> 
> If only RADEON_PLL_USE_REF_DIV is set then we only use the fixed value as
> minimum limit.

Does that make sense?  Wouldn't we want to use use the fixed value if RADEON_PLL_USE_REF_DIV regardless of whether RADEON_PLL_USE_FRAC_FB_DIV is set?
Comment 9 Christian König 2014-04-20 08:09:37 UTC 
(In reply to comment #8)
> (In reply to comment #6)
> > Please try the attached patch.
> > 
> > We now always use the fixed reference divider if both RADEON_PLL_USE_REF_DIV
> > and RADEON_PLL_USE_FRAC_FB_DIV are set.
> > 
> > If only RADEON_PLL_USE_REF_DIV is set then we only use the fixed value as
> > minimum limit.
> 
> Does that make sense?  Wouldn't we want to use use the fixed value if
> RADEON_PLL_USE_REF_DIV regardless of whether RADEON_PLL_USE_FRAC_FB_DIV is
> set?

I don't really know, it's just the way how the old code handled this.

If RADEON_PLL_USE_REF_DIV was set the RADEON_PLL_USE_FRAC_FB_DIV branch just used the value as provided, but the none RADEON_PLL_USE_FRAC_FB_DIV branch still tried to increment the vale.
Comment 10 Christian König 2014-04-20 10:34:23 UTC 
Marking as fixed.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.