2013/6/12 Daniel Vetter <daniel.vetter at 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 at gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at 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
