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@xxxxxxxxxxxxxxx>:
> > 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. 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.
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@xxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxxxxxxxxxxxxxx
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
