On Wed, 20 Apr 2011 10:36:29 -0700, Keith Packard <kei...@keithp.com> wrote: > On Wed, 20 Apr 2011 16:42:08 +0100, Chris Wilson <ch...@chris-wilson.co.uk> > wrote: > > Given that the hardware may be left in a random condition by the BIOS, > > it is conceivable that we then attempt to clear the DP_PIPEB_SELECT bit > > without us ever enabling/attaching the DP encoder to a pipe. Thus > > causing a NULL deference when we attempt to wait for a vblank on that > > crtc. > > Is this because we're assuming that the only way DP_PIPEB_SELECT could > have been set is by our driver? That does seem like a bad assumption on > our part.
Everywhere we make the assumption that we are in sole charge of the hw. What I considered was extending the scrubbing we do on takeover so that the output registers are consistent with our expectations. I'm worried that we leave some state in a register not normally touched by us that is affecting system behaviour - our history is littered with such examples, and a large part of modesetting init is already spent tidying up registers. > > - intel_wait_for_vblank(dev, intel_crtc->pipe); > > + intel_wait_for_crtc_vblank_safe(intel_dp->base.base.crtc); > > I actually think that simply placing the in-line function here in the > code is clearer -- it makes it obvious that we aren't counting on there > being a crtc assigned to this output. I suspect that we have other locations where the crtc is not necessarily attached to the encoder when we need to insert a delay, hence why I introduced a function. As an aside, I'm still worried by all the wait for blank timeouts. My preferred solution is not to have the check there at all, but that looked to be much more invasive. Hindsight also says that on the msleep path we are missing a mmiowb. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
