Re: [PATCH 07/16] drm/i915: disable FBC on FIFO underruns

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Aug 20, 2015 at 11:29:29AM -0300, Paulo Zanoni wrote:
> 2015-08-20 10:58 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > On Thu, Aug 20, 2015 at 10:30:17AM -0300, Paulo Zanoni wrote:
> >> 2015-08-19 9:06 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> >> > On Fri, Aug 14, 2015 at 06:34:12PM -0300, Paulo Zanoni wrote:
> >> >> If we want to try to enable FBC by default on any platform we need to
> >> >> be on the safe side and disable it in case we get an underrun while
> >> >> FBC is enabled on the corresponding pipe. We currently already have
> >> >> other reasons for FIFO underruns on our driver, but the ones I saw
> >> >> with FBC lead to black screens that only go away when you disable FBC.
> >> >
> >> > We don't try to deal with underruns on other platforms either, and yes
> >> > on some, cough, chv, cough, they can definitely blank out the screen
> >> > until the next modeset. On even older platforms it's even worse and an
> >> > underrun can kill the display engine until display reset/reboot :(
> >> > Especially annoying on gen2 where we have no reset support. So I'm not
> >> > entirely convinced FBC deserves special treatment here.
> >>
> >> I don't understand your logic. To me, it sounds like "you're proposing
> >> adding airbags to new cars, but that is not going to protect against
> >> every type of accident, and it's also not going to help the cars that
> >> are already manufactured, so I don't think front-collisions of new
> >> cars deserve special treatment".
> >
> > Currently FBC is more like like "driving backwards on one wheel". I'm
> > not sure there's much point in trying to make it fault tolerant while we
> > know the code is still broken.
> 
> That's why this series contains other patches fixing the other
> problems.

Well the whole intel_update_fbc() thing is still very much a fail wrt.
locking. But maybe you're planning on resurrecting my fbc_score idea?

Also now I that I glanced at the code, intel_fbc_flush() looks busted.
To actually nuke the FBC state you need to wait at least until the next
vblank before re-enabling FBC. That was one reason I had the vblank
notify thingy in my "fix FBC" series.

> That's also why I recently added a few thousands of lines of
> code to IGT. Besides, no one is proposing to enable FBC by default on
> every single platform. I don't think "feature X doesn't work at the
> moment" is a reason to reject a patch that improves feature X.
> 
> > Once it's otherwise known to be solid,
> > then it might make sense, although a much cooler thing would be if we
> > could actually detect when the display has failed entirely and recover
> > it somehow.
> >
> > Oh and to make the protection mechanism actually kick in reliably you
> > would somehow need to address the problems with the underrun interrupts.
> 
> Which problems?

The fact that we disable them as soon as one occurs, and even worse the
error interrupt is a shared one on many platforms, so one underrun any
pipe or some other unrelated error kills underrun detection everywhere.

> 
> When I tested this patch, it was working very reliably: we detected
> the underrun and disabled FBC 100% of the time.
> 
> It's also worth noticing that the cause of the underrun is actually
> fixed by a later patch of this series, so we don't really need this
> patch as part of the whole "fix FBC" task, but I really think it's
> better to have it than not have it, just in case we regress somehow.
> 
> > At the moment, to continue your car analogy, it's like the airbags would
> > refuse to deploy in a real crash if you happened to ding the mailbox
> > while pulling out of your driveway.
> >
> >>
> >> >
> >> >>
> >> >> The current FIFO underrun I see happens when the CFB is using the top
> >> >> 8mb of stolen memory. This is reproducible with a 2560x1440 DP Monitor
> >> >> on a system with 32mb of stolen memory. On this case, all the IGT
> >> >> tests fail while checking the screen CRC. A later patch on this series
> >> >> will fix this problem for real.
> >> >>
> >> >> With this patch, the tests will start failing while checking if FBC is
> >> >> enabled instead of failing while comparing the CRC of the black screen
> >> >> against the correct CRC. So this patch is not hiding any IGT bugs: the
> >> >> tests still fail, but now they fail with a different reason.
> >> >>
> >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/i915/i915_drv.h            |  5 +++
> >> >>  drivers/gpu/drm/i915/intel_drv.h           |  1 +
> >> >>  drivers/gpu/drm/i915/intel_fbc.c           | 61 ++++++++++++++++++++++++++++++
> >> >>  drivers/gpu/drm/i915/intel_fifo_underrun.c |  2 +
> >> >>  4 files changed, 69 insertions(+)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> >> index 4fd7858..e74a844 100644
> >> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> >> @@ -926,6 +926,11 @@ struct i915_fbc {
> >> >>               struct drm_framebuffer *fb;
> >> >>       } *fbc_work;
> >> >>
> >> >> +     struct intel_fbc_underrun_work {
> >> >> +             struct work_struct work;
> >> >> +             struct intel_crtc *crtc;
> >> >> +     } underrun_work;
> >> >> +
> >> >>       enum no_fbc_reason {
> >> >>               FBC_OK, /* FBC is enabled */
> >> >>               FBC_UNSUPPORTED, /* FBC is not supported by this chipset */
> >> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> >> index 81b7d77..246925d 100644
> >> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> >> @@ -1247,6 +1247,7 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
> >> >>                    unsigned int frontbuffer_bits, enum fb_op_origin origin);
> >> >>  const char *intel_no_fbc_reason_str(enum no_fbc_reason reason);
> >> >>  void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
> >> >> +void intel_fbc_handle_fifo_underrun(struct intel_crtc *crtc);
> >> >>
> >> >>  /* intel_hdmi.c */
> >> >>  void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port);
> >> >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> >> >> index a63d10a..28e569c 100644
> >> >> --- a/drivers/gpu/drm/i915/intel_fbc.c
> >> >> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> >> >> @@ -955,6 +955,65 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
> >> >>       mutex_unlock(&dev_priv->fbc.lock);
> >> >>  }
> >> >>
> >> >> +static void intel_fbc_underrun_work_fn(struct work_struct *__work)
> >> >> +{
> >> >> +     struct intel_fbc_underrun_work *work =
> >> >> +             container_of(__work, struct intel_fbc_underrun_work, work);
> >> >> +     struct intel_crtc *crtc = work->crtc;
> >> >> +     struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> >> >> +
> >> >> +     mutex_lock(&dev_priv->fbc.lock);
> >> >> +     if (!intel_fbc_enabled(dev_priv) || dev_priv->fbc.crtc != crtc)
> >> >> +             goto out;
> >> >> +
> >> >> +     DRM_DEBUG_KMS("Disabling FBC due to FIFO underrun.\n");
> >> >> +     i915.enable_fbc = 0;
> >> >> +     __intel_fbc_disable(dev_priv);
> >> >> +
> >> >> +out:
> >> >> +     work->crtc = NULL;
> >> >> +     mutex_unlock(&dev_priv->fbc.lock);
> >> >> +}
> >> >> +
> >> >> +/**
> >> >> + * intel_fbc_handle_fifo_underrun - handle FIFO underruns if FBC is enabled
> >> >> + * @crtc: the CRTC that caused the underrun
> >> >> + *
> >> >> + * Although we can't know for sure what caused an underrun, one of the possible
> >> >> + * reasons is FBC. And on the FBC case, the user may have a black screen until
> >> >> + * FBC is disabled. So whenever a FIFO underrun happens while FBC is enabled,
> >> >> + * disable FBC just because it may help.
> >> >> + *
> >> >> + * We disable FBC by changing the i915 param, so FBC won't come back on the next
> >> >> + * frame just to cause another underrun. Test suites can force FBC back by
> >> >> + * changing the module parameter again through sysfs.
> >> >> + */
> >> >> +void intel_fbc_handle_fifo_underrun(struct intel_crtc *crtc)
> >> >> +{
> >> >> +     struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> >> >> +     struct intel_fbc_underrun_work *work = &dev_priv->fbc.underrun_work;
> >> >> +
> >> >> +     if (!dev_priv->fbc.enable_fbc)
> >> >> +             return;
> >> >> +
> >> >> +     /* These checks are unlocked. We can't grab the lock since we're in the
> >> >> +      * IRQ handler. OTOH, grabbing it wouldn't help much since the underrun
> >> >> +      * interrupt is asynchronous. Still, this a "try to recover because we
> >> >> +      * have already failed" function, so let's at least try, and if we fail,
> >> >> +      * we'll probably be able to try again next frame when another underrun
> >> >> +      * happens. We'll also do the checks again in the work function, where
> >> >> +      * we can grab the lock. */
> >> >> +     if (!intel_fbc_enabled(dev_priv) || dev_priv->fbc.crtc != crtc)
> >> >> +             return;
> >> >> +
> >> >> +     /* Something already scheduled it. */
> >> >> +     if (work->crtc != NULL)
> >> >> +             return;
> >> >> +
> >> >> +     work->crtc = crtc;
> >> >> +     schedule_work(&work->work);
> >> >> +}
> >> >> +
> >> >>  /**
> >> >>   * intel_fbc_init - Initialize FBC
> >> >>   * @dev_priv: the i915 device
> >> >> @@ -966,6 +1025,8 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
> >> >>       enum pipe pipe;
> >> >>
> >> >>       mutex_init(&dev_priv->fbc.lock);
> >> >> +     INIT_WORK(&dev_priv->fbc.underrun_work.work,
> >> >> +               intel_fbc_underrun_work_fn);
> >> >>
> >> >>       if (!HAS_FBC(dev_priv)) {
> >> >>               dev_priv->fbc.enabled = false;
> >> >> diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> >> >> index 54daa66..90e60fb 100644
> >> >> --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> >> >> +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> >> >> @@ -356,6 +356,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
> >> >>       if (intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false))
> >> >>               DRM_ERROR("CPU pipe %c FIFO underrun\n",
> >> >>                         pipe_name(pipe));
> >> >> +
> >> >> +     intel_fbc_handle_fifo_underrun(to_intel_crtc(crtc));
> >> >>  }
> >> >>
> >> >>  /**
> >> >> --
> >> >> 2.4.6
> >> >>
> >> >> _______________________________________________
> >> >> Intel-gfx mailing list
> >> >> Intel-gfx@lists.freedesktop.org
> >> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> >
> >> > --
> >> > Ville Syrjälä
> >> > Intel OTC
> >> > _______________________________________________
> >> > Intel-gfx mailing list
> >> > Intel-gfx@lists.freedesktop.org
> >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>
> >>
> >>
> >> --
> >> Paulo Zanoni
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx