Bug 24462

Summary: r600: spread spectrum: flickering screen (bisected)
Product: Drivers Reporter: Luca Tettamanti (kronos.it)
Component: Video(DRI - non Intel)Assignee: drivers_video-dri
Status: CLOSED CODE_FIX    
Severity: normal CC: alexdeucher, florian, maciej.rutecki, rjw
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 2.6.37-rc5 Subsystem:
Regression: Yes Bisected commit-id:
Bug Depends on:    
Bug Blocks: 21782    
Attachments: possible fix
M76 vbios
possible fix
possible fix
better fix
new patch

Description Luca Tettamanti 2010-12-08 16:18:38 UTC
2.6.37-rc5 exhibits a slight screen (notebook panel) flickering, mostly noticeable around text (e.g. white-on-black in xterm) with occasional horizontal lines; using 2.6.36 as known "good" kernel I've bisected the issue down to this commit:

ba032a58d1f320039e7850fb6e8651695c1aa571 is the first bad commit
commit ba032a58d1f320039e7850fb6e8651695c1aa571
Author: Alex Deucher <alexdeucher@gmail.com>
Date:   Mon Oct 4 17:13:01 2010 -0400

    drm/radeon/kms: rework spread spectrum handling
    
    This patch reworks spread spectrum handling to enable it
    properly on lvds and DP/eDP links.  It also fixes several
    bugs in the old spread spectrum code.
    
    - Use the ss recommended reference divider if available
    when calculating the pll
    - Use the proper ss command tables on pre-DCE3 asics
    - Avoid reading past the end of the ss info tables
    - Enable ss on evergreen asics (lvds, dp, tmds)
    - Enable ss on DP/eDP links
    
    Signed-off-by: Alex Deucher <alexdeucher@gmail.com>
    Signed-off-by: Dave Airlie <airlied@redhat.com>

:040000 040000 2017e6abb9a12586686bcddb61479f2b158c9d7c 7e6117db061ef286ab6822455c65cf5492de7eaa M      drivers

Hardware is a M76:
01:00.0 VGA compatible controller: ATI Technologies Inc M76 [Radeon Mobility HD 2600 Series]

dmesg is attached.
Comment 1 Alex Deucher 2010-12-08 17:11:56 UTC
Created attachment 39252 [details]
possible fix

Does this patch help?
Comment 2 Luca Tettamanti 2010-12-08 20:27:11 UTC
(In reply to comment #1)
> Created an attachment (id=39252) [details]
> possible fix
> 
> Does this patch help?

No, flickering it's still there. The parameters passed to EnableSpreadSpectrumOnPPLL with 2.6.36 and 2.6.37-rc5 are:

ss->percentage = 0x1e
ss->type = 0x1
ss->step = 0x1
ss->delay = 0x2a
ss->range = 0x2
pll_id = 0x0

but 2.6.36 uses ENABLE_SPREAD_SPECTRUM_ON_PPLL_PS_ALLOCATION, while the newer kernel uses ENABLE_LVDS_SS_PARAMETERS_V2 for the parameters (the only difference is that the latter does not have ucPpll); is this correct?
Comment 3 Alex Deucher 2010-12-08 20:39:57 UTC
Please attach a copy of your vbios:
(as root)
(use lspci to get the bus id)
cd /sys/bus/pci/devices/<pci bus id>
echo 1 > rom
cat rom > /tmp/vbios.rom
echo 0 > rom
Comment 4 Luca Tettamanti 2010-12-08 20:47:55 UTC
Created attachment 39342 [details]
M76 vbios
Comment 5 Alex Deucher 2010-12-08 21:14:47 UTC
Created attachment 39362 [details]
possible fix

(In reply to comment #2)
> but 2.6.36 uses ENABLE_SPREAD_SPECTRUM_ON_PPLL_PS_ALLOCATION, while the newer
> kernel uses ENABLE_LVDS_SS_PARAMETERS_V2 for the parameters (the only
> difference is that the latter does not have ucPpll); is this correct?

That is correct.  I suspect it's the pll that's problematic:
if (ss_enabled) {
        if (ss->refdiv) {
                pll->flags |= RADEON_PLL_USE_REF_DIV;
                pll->reference_div = ss->refdiv;
        }
}

It uses a fixed reference divider of 2 in your case.  Does the attached patch help?  Please also try this patch in conjunction with the patch in comment 1.
Comment 6 Rafael J. Wysocki 2010-12-08 22:12:17 UTC
First-Bad-Commit : ba032a58d1f320039e7850fb6e8651695c1aa571

Handled-By : Alex Deucher <alexdeucher@gmail.com>
Comment 7 Luca Tettamanti 2010-12-08 22:32:06 UTC
(In reply to comment #5)
> Does the attached patch
> help?  Please also try this patch in conjunction with the patch in comment 1.

Still not working, even with the first patch applied.
Comment 8 Alex Deucher 2010-12-08 23:05:27 UTC
Created attachment 39372 [details]
possible fix

This patch disables use of the ss fixed ref divide and should fix it.
Comment 9 Luca Tettamanti 2010-12-09 20:12:05 UTC
(In reply to comment #8)
> This patch disables use of the ss fixed ref divide and should fix it.

Yes, taking out that code worked. I'm of course available for further testing if you're not satisfied with this workaround ;)
Comment 10 Florian Mickler 2010-12-17 00:43:13 UTC
Patch: https://bugzilla.kernel.org/attachment.cgi?id=39372
Comment 11 Alex Deucher 2010-12-20 06:57:15 UTC
I've sent the patch to Dave.  It should show up in his next drm pull request.
Comment 12 Rafael J. Wysocki 2010-12-29 22:48:03 UTC
Fixed by commit e5fd205 drm/radeon/kms: disable ss fixed ref divide .
Comment 13 Alex Deucher 2011-02-11 06:57:54 UTC
Luca, can you try re-enabling this code with 2.6.38-rc4 or newer?  Do you still have problems?
Comment 14 Alex Deucher 2011-02-11 07:32:50 UTC
Created attachment 47282 [details]
better fix

Can you try the following patch against 2.6.38-rc4 or newer and make sure all is working well?
Comment 15 Luca Tettamanti 2011-02-11 16:09:40 UTC
(In reply to comment #14)
> Created an attachment (id=47282) [details]
> better fix
> 
> Can you try the following patch against 2.6.38-rc4 or newer and make sure all
> is working well?

2.6.38-rc4 plus the patch looks good, thanks!
Comment 16 Alex Deucher 2011-02-12 01:17:21 UTC
Created attachment 47482 [details]
new patch

Can you try this patch?  Try uncommenting the following lines to see if either of those flags work as well.  Try them individually and together if possible and report back which, if any, helps.

/*pll->flags |= RADEON_PLL_PREFER_MINM_OVER_MAXP;*/

/*if (ASIC_IS_AVIVO(rdev))
        pll->flags |= RADEON_PLL_USE_FRAC_FB_DIV;*/

Thanks!
Comment 17 Luca Tettamanti 2011-02-14 13:56:01 UTC
(In reply to comment #16)
> Created an attachment (id=47482) [details]
> new patch
> 
> Can you try this patch?  Try uncommenting the following lines to see if
> either
> of those flags work as well.  Try them individually and together if possible
> and report back which, if any, helps.
> 
> /*pll->flags |= RADEON_PLL_PREFER_MINM_OVER_MAXP;*/
> 
> /*if (ASIC_IS_AVIVO(rdev))
>         pll->flags |= RADEON_PLL_USE_FRAC_FB_DIV;*/

The first one alone works, the second one alone does *not* work, both together work fine.