| Message ID | 1371048364-6281-1-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. Can you please exemplify the "someone is changing the pointers around" case? I need to make sure I fully understand this. > > 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. > > v2: Add a big comment explaining what's going on. Requested by Paulo. > > Cc: Paulo Zanoni <przanoni@gmail.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_irq.c | 40 +++++++++++++++------------------------- > 1 file changed, 15 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index bb26555..e80c610 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,29 +292,19 @@ 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); > + /* > + * NOTE: Pre-LPT has a fixed cpu pipe -> pch transcoder mapping, but LPT > + * has has only one pch transcoder A that all pipes can use. To avoid s/has has/has/ > + * racy pch transcoder -> pipe lookups from interrupt code simply store > + * the underrun statistics in crtc A. Since we never expose this > + * anywhere nor use it outside of the fifo underrun code here using the We do expose it to the user as an error message. On Haswell we call cpt_set_fifo_underrun_reporting, and you recently added a message there saying "uncleared pch fifo underrun on pipe %i". This is wrong since we'll always print "pipe A" on Haswell. If we reword the error message to say "on PCH transcoder A" then we'll be always correct. > + * "wrong" crtc on LPT won't cause issues. > + */ > > spin_lock_irqsave(&dev_priv->irq_lock, flags); > > @@ -326,7 +316,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 >
On Thu, Jun 27, 2013 at 05:45:29PM -0300, Paulo Zanoni wrote: > 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. > > Can you please exemplify the "someone is changing the pointers around" > case? I need to make sure I fully understand this. While we do a modeset we change the encoder->crtc pointers around and potentially zero them. Now we're lucky and we don't ever dereference that pointer, so we can't OOPS on it (without locks or other tricks you can read non-NULL when checking the pointer and then again NULL when dereferencing). So here it's just that we could use different crtcs to keep track of the fifo_underrun_reporting_disabled bit. Nothing truely bad can happen, it just forces one to think for a bit too long ;-) btw there's a funny bug on module reloading where we seem to hit a fifo underrun before the crtcs are properly set up. Which means we follow the pipe_to_crtc pointers into NULL-land :( Different patch though. -Daniel > > > > > > 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. > > > > v2: Add a big comment explaining what's going on. Requested by Paulo. > > > > Cc: Paulo Zanoni <przanoni@gmail.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > drivers/gpu/drm/i915/i915_irq.c | 40 +++++++++++++++------------------------- > > 1 file changed, 15 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > index bb26555..e80c610 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,29 +292,19 @@ 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); > > + /* > > + * NOTE: Pre-LPT has a fixed cpu pipe -> pch transcoder mapping, but LPT > > + * has has only one pch transcoder A that all pipes can use. To avoid > > s/has has/has/ > > > + * racy pch transcoder -> pipe lookups from interrupt code simply store > > + * the underrun statistics in crtc A. Since we never expose this > > + * anywhere nor use it outside of the fifo underrun code here using the > > We do expose it to the user as an error message. On Haswell we call > cpt_set_fifo_underrun_reporting, and you recently added a message > there saying "uncleared pch fifo underrun on pipe %i". This is wrong > since we'll always print "pipe A" on Haswell. If we reword the error > message to say "on PCH transcoder A" then we'll be always correct. > > > > + * "wrong" crtc on LPT won't cause issues. > > + */ > > > > spin_lock_irqsave(&dev_priv->irq_lock, flags); > > > > @@ -326,7 +316,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 > > > > > > -- > Paulo Zanoni
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index bb26555..e80c610 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,29 +292,19 @@ 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); + /* + * NOTE: Pre-LPT has a fixed cpu pipe -> pch transcoder mapping, but LPT + * has has only one pch transcoder A that all pipes can use. To avoid + * racy pch transcoder -> pipe lookups from interrupt code simply store + * the underrun statistics in crtc A. Since we never expose this + * anywhere nor use it outside of the fifo underrun code here using the + * "wrong" crtc on LPT won't cause issues. + */ spin_lock_irqsave(&dev_priv->irq_lock, flags); @@ -326,7 +316,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. v2: Add a big comment explaining what's going on. Requested by Paulo. Cc: Paulo Zanoni <przanoni@gmail.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_irq.c | 40 +++++++++++++++------------------------- 1 file changed, 15 insertions(+), 25 deletions(-)