Bug 13196

May have been fixed by 10988c5e6ec0f3c40d56bbf209b7976627cca706 today

nope :(

*** Bug 13297 has been marked as a duplicate of this bug. ***

btw, what might contribute to the problem is this pretty bold behavior:

(**) Option "dpms" "off"
(**) intel(0): DPMS enabled

:-D

so the problem might have existed for a longer time, but didn't become apparent for dpms being properly disabled.

We actually do restore PIPEACONF after calling the DPMS off functions:
   if ((pI830->saveDPLL_A & DPLL_VCO_ENABLE) &&
       (pI830->saveDPLL_A & DPLL_VGA_MODE_DIS))
           OUTREG(PIPEACONF, pI830->savePIPEACONF);
but maybe one of the saveDPLL_A checks is failing on your machine...  Does the
log have the value of DPLL_A somewhere?

at startup, there is this:
(II) intel(0):            PIPEACONF: 0x80000000 (enabled, single-wide)
(II) intel(0):               DPLL_A: 0x808b0000 (enabled, non-dvo, VGA, default clock, DAC/serial mode, p1 = 13, p2 = 4)

the complete log is attached to bug #13312.

well, yes ... *of course* the check is failing - after all, all vts *are* in vga text mode.
i'm not sure whether the pipeaconf restoration should be duplicated with the inverse condition to after the *VGA* registers are set or just completely moved down and the condition removed. or maybe the vga restore should be moved in front of restoring the other registers.

Yeah, this is clearly a problem.  However, we'll need to be careful about the fix if we're going to prevent yet another regression...

I think we just need to restore the VGA clock divisor regs before restoring the PIPEnCONF regs, but we may also need to restore the VGA MSR reg as well.

Cc'ing Peter Clifton so he can test (or find a tester hopefully) so we don't regress on 8xx chips.

Please try the attached patch.

Created attachment 12783 [details] [review]
Restore pipeconf fix

Move VGA clock restore to where the rest of the clock stuff is done and remove the VGA checks prior to restoring the PIPEnCONF regs.

Adding Peter to make sure we don't regress.  Peter, please see the last few comments.

works like a charm. :-)

i still wonder what this dpll condition is good for at all - i mean, we are restoring the old state, so why try to fix it?

The clock corresponding to the pipe we're about to enable has to be on before we can enable the pipe, or the chip will hang.  The checks are there to protect against a possibly bad initial state (maybe one where the BIOS enabled both clocks & pipes, then turned off a clock w/o disabling its pipe or something).

I've contacted the user who was seeing crashes before, and will see if he can test a package with the patch attached here.

No,this patch didn't work on G33 and 965,after VT to console,i still got a dark screen

have you rebooted in between? remember that this restores the state prior to starting the server, so it won't help if a bad server disabled the pipe previously.

*** Bug 13399 has been marked as a duplicate of this bug. ***

Ok,today I re-test Jesse's patch with latest git code on G33 and 965,it works,so Jesse you could commit it now.

Ok, I just want to make sure Peter's contact tests this before I commit.  I don't want to fix a regression by adding one. :)

*** Bug 13328 has been marked as a duplicate of this bug. ***

Has anyone else observed that this patch breaks VT support (VTs turn into a black screen with vertical technicolor stripes)? Maybe it's another patch, I don't know. Other than this, what is preventing the merging of this patch at the moment?

I'm waiting on Peter's tester to make sure we don't regress 8xx users.

Ben, can you narrow down your VT switch problem a little more?  If it really is this patch causing your problem, I'd definitely want to get it fixed before pushing...

Created attachment 13032 [details] [review]
Pipe & plane enable fix

I looked at the docs in some detail today (though not the 855 ones, they're still on order), and it looks like moving up the VGA register restore is the right thing to do.  However, I also noticed that we were unconditionally enabling display planes, possibly at the wrong time, so this patch tries to fix that.  I'm really hoping this one works for both 9xx and 8xx users...

I'm back-porting against the older driver to see if I can get this tested on 855 hardware.

A couple of notes though..

  /* If the pipe A PLL is active, we can restore the pipe & plane config */

Was copy-pasted to the Pipe B case too, without fixing the A -> B lettering.

Should the section:

   /* If we enabled pipe B, enable its corresponding plane */

Be nested under the test similar to:

   if(xf86_config->num_crtc == 2) 
   {
   }

(Like the rest of the "B" restore code?

I fixed the comment, thanks for looking.

The pipe B restore could go under the num_crtc == 2 check, I'll move it there (and move the pipe A restore up above).  However the posted code should be safe since we check the actual reg before writing the other values; my initial thinking was to do both after everything else, but it's probably best to keep the pipe code together.

Created attachment 13038 [details] [review]
Updated patch incorporating Peter's feedback

Shuffle the code around a little and fix the comment.

still works on i845g :)

Created attachment 13169 [details] [review]
Let PLLs settle longer

The last patch didn't work.  Given the logs I've seen so far, I can't see why we would crash on programming the pipe regs unless we weren't waiting long enough for the PLLs to settle after programming them.  So this patch removes the conditions around the pipeconf reg writes and adds some more delays after the PLL register writes.

I think we still have some bugs in the hardware state save/restore, but afaict they shouldn't be causing the crashes people are seeing on 855.

Please test this out and let me know how it goes.

vga_mode isn't used once assigned.
isn't there some status bit to indicate that the pll has settled? several times 10ms is an awful lot of time (enough for some (more) displays to go to sleep). maybe setting up the two pipes can be interleaved once it is known to work?

Yeah, the vga_mode flag was something I thought I'd use then ended up abandoning.  It's gone in my personal tree.

Yeah, 10ms is a long time and I'm sure it could be less, but I still want to test the theory that we need to wait for the PLLs a little longer.  Unfortunately, the only way I know of to see if the PLLs are ready is to write a pipe or palette register.  If the chip crashes, it wasn't ready.  If not, it must have been. :)

If this approach works, someone with a test machine could do a binary search for a good, low value.  But we already have lots of 30+ms waits in there due to the i830WaitForVblank, so it's not like the driver will get much faster.

I pushed the last patch to at least get people with 9xx chipsets working again, but I'll leave this bug open until we fix 855GM too.

fwiw, still works on i845g (deja vu?) :)

Still works on g33.  Just to remind you of the original problem, vanilla 2.2.0 without this patch or alternatively the previous "Restore pipeconf fix" patch immediately locks X (i.e., no access to keyboard, monitor in power save mode, with the only way out to ssh in to the box and do a shutdown) for my ASUS P5K-V with g33 chipset  and no ADD2 card.  This lockup occurred even for just the X -probeonly command.  So I am very happy to see this new patch still solves the g33 problem.

*** Bug 13744 has been marked as a duplicate of this bug. ***

I took the latest git head in order to test this patch, but even Xorg come up, the text consoles using framebuffer were totally black, I couldn't see anything there. When I switch to Xorg is fine.

I suspected of the patch to I reverted commit f69b48fe24ef94dac44b8123884ca71df675be4b and tried again. Now text consoles works and Xorg as well, but of course the bug is still present.

Core 2 duo, intel 965GM with Debian sid and Linux 2.6.24rc6.

Same problem as Raul in #33 but only in several times...

Sounds like this particular problem is fixed at the cost of adding a regression documented in 13867 (855 machines can't suspend/resume anymore).