Re: [PATCH 3/4] drm/i915: Move underrun work from fbc to fifo_underrun.

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, Feb 26, 2018 at 09:00:50PM +0000, Chris Wilson wrote:
> Quoting Rodrigo Vivi (2018-02-26 20:53:08)
> > -void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv)
> > -{
> > -       struct intel_fbc *fbc = &dev_priv->fbc;
> > -
> > -       if (!fbc_supported(dev_priv))
> > -               return;
> > -
> > -       /* There's no guarantee that underrun_detected won't be set to true
> > -        * right after this check and before the work is scheduled, but that's
> > -        * not a problem since we'll check it again under the work function
> > -        * while FBC is locked. This check here is just to prevent us from
> > -        * unnecessarily scheduling the work, and it relies on the fact that we
> > -        * never switch underrun_detect back to false after it's true. */
> > -       if (READ_ONCE(fbc->underrun_detected))
> > -               return;
> > -
> > -       schedule_work(&fbc->underrun_work);
> > -}
> 
> > +static void intel_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv)
> > +{
> > +       if (!HAS_FBC(dev_priv))
> > +               return;
> > +
> > +       spin_lock(&dev_priv->underrun.lock);
> > +
> > +       if (dev_priv->underrun.detected)
> > +               goto out;
> > +       dev_priv->underrun.detected = true;
> > +
> > +       schedule_work(&dev_priv->underrun.work);
> > +out:
> > +       spin_unlock(&dev_priv->underrun.lock);
> 
> This locking (or bool) isn't required by the following patch either. But
> I presume the boolean is printed at some point, although that's probably
> less useful than whatever FBC/SAGV would say about being disabled.

I honestly didn't fully followed the initial goal of checking the detection
before scheduling the work.... So I assumed it was some underrun storm
and a mechanism to avoid multiple calls to fbc disable...

But yeap... I think we could live without it if no storm is possible
or if internal sagv and fbc are already handling those properly.

> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx