| Message ID | 1303314128-21102-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 20 Apr 2011 16:42:08 +0100, Chris Wilson <chris@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. > - 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.
On Wed, 20 Apr 2011 10:36:29 -0700, Keith Packard <keithp@keithp.com> wrote: > On Wed, 20 Apr 2011 16:42:08 +0100, Chris Wilson <chris@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
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 0daefca..819afca 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1470,7 +1470,6 @@ intel_dp_link_down(struct intel_dp *intel_dp) if (!HAS_PCH_CPT(dev) && I915_READ(intel_dp->output_reg) & DP_PIPEB_SELECT) { - struct intel_crtc *intel_crtc = to_intel_crtc(intel_dp->base.base.crtc); /* Hardware workaround: leaving our transcoder select * set to transcoder B while it's off will prevent the * corresponding HDMI output on transcoder A. @@ -1485,7 +1484,7 @@ intel_dp_link_down(struct intel_dp *intel_dp) /* Changes to enable or select take place the vblank * after being written. */ - intel_wait_for_vblank(dev, intel_crtc->pipe); + intel_wait_for_crtc_vblank_safe(intel_dp->base.base.crtc); } I915_WRITE(intel_dp->output_reg, DP & ~DP_PORT_EN); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index f5b0d83..aeb1b98 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -290,6 +290,13 @@ extern struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev, int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data, struct drm_file *file_priv); extern void intel_wait_for_vblank(struct drm_device *dev, int pipe); +static inline void intel_wait_for_crtc_vblank_safe(struct drm_crtc *crtc) +{ + if (crtc) + intel_wait_for_vblank(crtc->dev, to_intel_crtc(crtc)->pipe); + else + msleep(50); +} extern void intel_wait_for_pipe_off(struct drm_device *dev, int pipe); extern struct drm_crtc *intel_get_load_detect_pipe(struct intel_encoder *intel_encoder, struct drm_connector *connector,
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. Reported-and-tested-by: Bryan Christ <bryan.christ@gmail.com> References: https://bugs.freedesktop.org/show_bug.cgi?id=36314 Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: stable@kernel.org --- drivers/gpu/drm/i915/intel_dp.c | 3 +-- drivers/gpu/drm/i915/intel_drv.h | 7 +++++++ 2 files changed, 8 insertions(+), 2 deletions(-)