| Message ID | 1371037046-3732-14-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2013/6/12 Daniel Vetter <daniel.vetter@ffwll.ch>: > It's racy: There's no guarantee that we won't walk this code (due to a > pch fifo underrun interrupt) while someone is changing the pointers > around. > > The only reason we do this is to use the righ crtc for the pch fifo > underrun accounting. But we never expose this to userspace, so > essentially no one really cares if we use the "wrong" crtc. > > So let's just rip it out. > > With this patch fifo underrun code will always use crtc A for tracking > underruns on the (only) pch transcoder on LPT. Please add a big comment in the source code explaining the hack and its reason. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_irq.c | 33 +++++++-------------------------- > 1 file changed, 7 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index bb26555..b98ea4e 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -193,13 +193,13 @@ static void ibx_display_interrupt_update(struct drm_i915_private *dev_priv, > POSTING_READ(SDEIMR); > } > > -static void ibx_set_fifo_underrun_reporting(struct intel_crtc *crtc, > +static void ibx_set_fifo_underrun_reporting(struct drm_device *dev, > + enum transcoder pch_transcoder, > bool enable) > { > - struct drm_device *dev = crtc->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > - uint32_t bit = (crtc->pipe == PIPE_A) ? SDE_TRANSA_FIFO_UNDER : > - SDE_TRANSB_FIFO_UNDER; > + uint32_t bit = (pch_transcoder == TRANSCODER_A) ? > + SDE_TRANSA_FIFO_UNDER : SDE_TRANSB_FIFO_UNDER; > > ibx_display_interrupt_update(dev_priv, bit, > enable ? bit : 0); > @@ -292,30 +292,11 @@ bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev, > bool enable) > { > struct drm_i915_private *dev_priv = dev->dev_private; > - enum pipe p; > - struct drm_crtc *crtc; > - struct intel_crtc *intel_crtc; > + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pch_transcoder]; > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > unsigned long flags; > bool ret; > > - if (HAS_PCH_LPT(dev)) { > - crtc = NULL; > - for_each_pipe(p) { > - struct drm_crtc *c = dev_priv->pipe_to_crtc_mapping[p]; > - if (intel_pipe_has_type(c, INTEL_OUTPUT_ANALOG)) { > - crtc = c; > - break; > - } > - } > - if (!crtc) { > - DRM_ERROR("PCH FIFO underrun, but no CRTC using the PCH found\n"); > - return false; > - } > - } else { > - crtc = dev_priv->pipe_to_crtc_mapping[pch_transcoder]; > - } > - intel_crtc = to_intel_crtc(crtc); > - > spin_lock_irqsave(&dev_priv->irq_lock, flags); > > ret = !intel_crtc->pch_fifo_underrun_disabled; > @@ -326,7 +307,7 @@ bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev, > intel_crtc->pch_fifo_underrun_disabled = !enable; > > if (HAS_PCH_IBX(dev)) > - ibx_set_fifo_underrun_reporting(intel_crtc, enable); > + ibx_set_fifo_underrun_reporting(dev, pch_transcoder, enable); > else > cpt_set_fifo_underrun_reporting(dev, pch_transcoder, enable); > > -- > 1.8.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index bb26555..b98ea4e 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -193,13 +193,13 @@ static void ibx_display_interrupt_update(struct drm_i915_private *dev_priv, POSTING_READ(SDEIMR); } -static void ibx_set_fifo_underrun_reporting(struct intel_crtc *crtc, +static void ibx_set_fifo_underrun_reporting(struct drm_device *dev, + enum transcoder pch_transcoder, bool enable) { - struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; - uint32_t bit = (crtc->pipe == PIPE_A) ? SDE_TRANSA_FIFO_UNDER : - SDE_TRANSB_FIFO_UNDER; + uint32_t bit = (pch_transcoder == TRANSCODER_A) ? + SDE_TRANSA_FIFO_UNDER : SDE_TRANSB_FIFO_UNDER; ibx_display_interrupt_update(dev_priv, bit, enable ? bit : 0); @@ -292,30 +292,11 @@ bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev, bool enable) { struct drm_i915_private *dev_priv = dev->dev_private; - enum pipe p; - struct drm_crtc *crtc; - struct intel_crtc *intel_crtc; + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pch_transcoder]; + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); unsigned long flags; bool ret; - if (HAS_PCH_LPT(dev)) { - crtc = NULL; - for_each_pipe(p) { - struct drm_crtc *c = dev_priv->pipe_to_crtc_mapping[p]; - if (intel_pipe_has_type(c, INTEL_OUTPUT_ANALOG)) { - crtc = c; - break; - } - } - if (!crtc) { - DRM_ERROR("PCH FIFO underrun, but no CRTC using the PCH found\n"); - return false; - } - } else { - crtc = dev_priv->pipe_to_crtc_mapping[pch_transcoder]; - } - intel_crtc = to_intel_crtc(crtc); - spin_lock_irqsave(&dev_priv->irq_lock, flags); ret = !intel_crtc->pch_fifo_underrun_disabled; @@ -326,7 +307,7 @@ bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev, intel_crtc->pch_fifo_underrun_disabled = !enable; if (HAS_PCH_IBX(dev)) - ibx_set_fifo_underrun_reporting(intel_crtc, enable); + ibx_set_fifo_underrun_reporting(dev, pch_transcoder, enable); else cpt_set_fifo_underrun_reporting(dev, pch_transcoder, enable);
It's racy: There's no guarantee that we won't walk this code (due to a pch fifo underrun interrupt) while someone is changing the pointers around. The only reason we do this is to use the righ crtc for the pch fifo underrun accounting. But we never expose this to userspace, so essentially no one really cares if we use the "wrong" crtc. So let's just rip it out. With this patch fifo underrun code will always use crtc A for tracking underruns on the (only) pch transcoder on LPT. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_irq.c | 33 +++++++-------------------------- 1 file changed, 7 insertions(+), 26 deletions(-)