[Intel-gfx] [PATCH] drm/i915/dp: Be paranoid in case we disable a DP before it is attached

[Chris Wilson]

On Wed, 20 Apr 2011 10:36:29 -0700, Keith Packard <keithp at keithp.com> wrote:
> On Wed, 20 Apr 2011 16:42:08 +0100, Chris Wilson <chris at 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