| Message ID | 1303343599-18509-4-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 20 Apr 2011 16:53:17 -0700, Ben Widawsky <ben@bwidawsk.net> wrote: > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> Just to annoy you, this needs to be split up into the various categories of fixes. Because... > static void ironlake_crtc_dpms(struct drm_crtc *crtc, int mode) > @@ -3067,9 +3074,12 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) > intel_disable_pll(dev_priv, pipe); > > intel_crtc->active = false; > + > + mutex_lock(&dev->struct_mutex); > intel_update_fbc(dev); > intel_update_watermarks(dev); > intel_clear_scanline_wait(dev); > + mutex_unlock(&dev->struct_mutex); > } This is overly correct. You can put a comment here to say that we will never attempt to use FORCEWAKE here and that these registers are protected by the mode_config lock. Except for intel_clear_scanline_wait, but that itself is is longing to be killed now. If we haven't fixed the underlying bug that we were working around by now, we have been too lax. -Chris
On Thu, Apr 21, 2011 at 07:18:24AM +0100, Chris Wilson wrote: > On Wed, 20 Apr 2011 16:53:17 -0700, Ben Widawsky <ben@bwidawsk.net> wrote: > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > Just to annoy you, this needs to be split up into the various categories > of fixes. Because... > > > static void ironlake_crtc_dpms(struct drm_crtc *crtc, int mode) > > @@ -3067,9 +3074,12 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) > > intel_disable_pll(dev_priv, pipe); > > > > intel_crtc->active = false; > > + > > + mutex_lock(&dev->struct_mutex); > > intel_update_fbc(dev); > > intel_update_watermarks(dev); > > intel_clear_scanline_wait(dev); > > + mutex_unlock(&dev->struct_mutex); > > } > > This is overly correct. You can put a comment here to say that we will > never attempt to use FORCEWAKE here and that these registers are protected > by the mode_config lock. Except for intel_clear_scanline_wait, but that > itself is is longing to be killed now. If we haven't fixed the underlying > bug that we were working around by now, we have been too lax. > -Chris I don't understand what you're asking for. I'm pretty convinced I need the mutex protected intel_update_fbc, because the call trace could be: intel_update_fbc() intel_enable_fbc() ironlake_enable_fbc() sandybridge_blit_fbc_update() gen6_gt_force_wake_get() Could you elaborate? Ben
On Fri, Apr 22, 2011 at 09:20:17AM -0700, Ben Widawsky wrote: > On Thu, Apr 21, 2011 at 07:18:24AM +0100, Chris Wilson wrote: > > On Wed, 20 Apr 2011 16:53:17 -0700, Ben Widawsky <ben@bwidawsk.net> wrote: > > > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > > > Just to annoy you, this needs to be split up into the various categories > > of fixes. Because... > > > > > static void ironlake_crtc_dpms(struct drm_crtc *crtc, int mode) > > > @@ -3067,9 +3074,12 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) > > > intel_disable_pll(dev_priv, pipe); > > > > > > intel_crtc->active = false; > > > + > > > + mutex_lock(&dev->struct_mutex); > > > intel_update_fbc(dev); > > > intel_update_watermarks(dev); > > > intel_clear_scanline_wait(dev); > > > + mutex_unlock(&dev->struct_mutex); > > > } > > > > This is overly correct. You can put a comment here to say that we will > > never attempt to use FORCEWAKE here and that these registers are protected > > by the mode_config lock. Except for intel_clear_scanline_wait, but that > > itself is is longing to be killed now. If we haven't fixed the underlying > > bug that we were working around by now, we have been too lax. > > -Chris > > I don't understand what you're asking for. I'm pretty convinced I need > the mutex protected intel_update_fbc, because the call trace could be: > > intel_update_fbc() > intel_enable_fbc() > ironlake_enable_fbc() > sandybridge_blit_fbc_update() > gen6_gt_force_wake_get() > > > Could you elaborate? > > Ben Crap. I wasn't paying attention. You're right, I did this to be symmetric with the other code. I can remove it if you prefer.
On Fri, 22 Apr 2011 09:20:17 -0700, Ben Widawsky <ben@bwidawsk.net> wrote: > I don't understand what you're asking for. I'm pretty convinced I need > the mutex protected intel_update_fbc, because the call trace could be: > > intel_update_fbc() > intel_enable_fbc() > ironlake_enable_fbc() > sandybridge_blit_fbc_update() > gen6_gt_force_wake_get() > > > Could you elaborate? It's a pre-ILK code path, so no magic GT handling required. -Chris
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 3cb0722..94b38f3 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -873,6 +873,7 @@ static int i915_cur_delayinfo(struct seq_file *m, void *unused) int max_freq; /* RPSTAT1 is in the GT power well */ + mutex_lock(&dev->struct_mutex); gen6_gt_force_wake_get(dev_priv); rpstat = I915_READ(GEN6_RPSTAT1); @@ -919,6 +920,7 @@ static int i915_cur_delayinfo(struct seq_file *m, void *unused) max_freq * 50); gen6_gt_force_wake_put(dev_priv); + mutex_unlock(&dev->struct_mutex); } else { seq_printf(m, "no P-state info available\n"); } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f6780cf..51dcb3f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2873,7 +2873,11 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) ironlake_pch_enable(crtc); intel_crtc_load_lut(crtc); + + mutex_lock(&dev->struct_mutex); intel_update_fbc(dev); + mutex_unlock(&dev->struct_mutex); + intel_crtc_update_cursor(crtc, true); } @@ -2969,8 +2973,11 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) intel_crtc->active = false; intel_update_watermarks(dev); + + mutex_lock(&dev->struct_mutex); intel_update_fbc(dev); intel_clear_scanline_wait(dev); + mutex_unlock(&dev->struct_mutex); } static void ironlake_crtc_dpms(struct drm_crtc *crtc, int mode) @@ -3067,9 +3074,12 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) intel_disable_pll(dev_priv, pipe); intel_crtc->active = false; + + mutex_lock(&dev->struct_mutex); intel_update_fbc(dev); intel_update_watermarks(dev); intel_clear_scanline_wait(dev); + mutex_unlock(&dev->struct_mutex); } static void i9xx_crtc_dpms(struct drm_crtc *crtc, int mode) @@ -6860,6 +6870,7 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv) * userspace... */ I915_WRITE(GEN6_RC_STATE, 0); + mutex_lock(&dev_priv->dev->struct_mutex); gen6_gt_force_wake_get(dev_priv); /* disable the counters and set deterministic thresholds */ @@ -6959,6 +6970,7 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv) I915_WRITE(GEN6_PMINTRMSK, 0); gen6_gt_force_wake_put(dev_priv); + mutex_unlock(&dev_priv->dev->struct_mutex); } void intel_enable_clock_gating(struct drm_device *dev)
Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_debugfs.c | 2 ++ drivers/gpu/drm/i915/intel_display.c | 12 ++++++++++++ 2 files changed, 14 insertions(+), 0 deletions(-)