Bug 29248

Created attachment 37373 [details]
Xorg.0.log (before)

Created attachment 37374 [details]
Xorg.0.log (after)

Created attachment 37375 [details]
dmesg (before)

Created attachment 37376 [details]
dmesg (after)

Created attachment 37377 [details]
xrandr --verbose

Created attachment 37378 [details]
intel_reg_dumper (before)

Created attachment 37379 [details]
intel_reg_dumper (after)

Created attachment 37380 [details]
vbios dump

I also have the output of intel_gpu_dump, but they are around 2 megabytes each. If you want them, tell me and I will attach both to this bug.

intel_gpu_dump, or rather the current equivalent i915_error_state, are only interesting after a GPU hang.

Besides the pipe that is disabled in both but is cleaned up after resume (which hopefully makes no difference...), there is one enabled bit that gets flipped:

Before:
PIPEACONF: 0xc000005c (enabled, active, 6bpc)

After:
PIPEACONF: 0xc0000054 (enabled, active, 6bpc)

which means prior to resume we have temporal dithering 0x3<<2, and after spatial temporal dithering 0x2<<2. No wonder it flickers and looks bad. (Temporal dithering by itself is a test mode!) Judging from the code, the bios set the spatial dithering 2 mode and we or'ed in spatial dithering 1, which accidentally enable the test mode.

Created attachment 37382 [details] [review]
Clear any previous dither mode

Cesar, thanks for the excellent bug report. I am pretty certain that the attached patch is correct, if you do have the chance to test it, I would welcome your tested-by.

> We cannot the initial configuration [...]

This sentence no verb.

What is the difference between Spatial Temporal 1 and Spatial Temporal 2, and why do we want one while the BIOS enabled the other? (Just curious.)

I will try to compile tonight a kernel with your patch over v2.6.35-rc6-19-g86c65a7 (currently the latest Linus git, and what I am guessing the rawhide kernel I am using is based on) and see what happens.

Tested-by: Cesar Eduardo Barros <cesarb@cesarb.net>

The patch in attachment #37382 [details] [review] works as expected. Tested on v2.6.35-rc6-19-g86c65a7.

Created attachment 37405 [details]
intel_reg_dumper (patched)

(In reply to comment #13)
> > We cannot the initial configuration [...]
> 
> This sentence no verb.

It was meant to be "We cannot assume..." I always seem to submit patches with at least one grammatical error. :(

> What is the difference between Spatial Temporal 1 and Spatial Temporal 2, and
> why do we want one while the BIOS enabled the other? (Just curious.)

The docs do not say. The second mode is also referred to as a test mode, but it doesn't say what it is intended to test either.

It got pushed upstream quite quickly (good timing and the patch is trivial):

commit a392a10367508930607a17ab60b4148f86adf2bc
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Sun Jul 25 23:09:13 2010 +0100

    drm/i915: Clear any existing dither mode prior to enabling spatial dithering
    
    We cannot the initial configuration set by the BIOS not to have a dither
    mode enabled which conflicts with our enabling the Spatial Temporal 1
    dither mode for PCH. In particular, the BIOS may either enable temporal
    dithering or the Spatial Temporal 2 with the result that we enable pure
    temporal dithering. Temporal dithering looks bad and is perceived as a
    flicker.
    
    Fixes:
    
      Bug 29248 - [Arrandale] Annoying flicker on internal panel, goes away
                  after suspend to RAM
      https://bugs.freedesktop.org/show_bug.cgi?id=29248
    
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
    Signed-off-by: Eric Anholt <eric@anholt.net>

However, I see that Eric encountered further problems with using an external DP after LVDS enabled dithering on the pipe. The solution was just to moving the clear earlier in the function...