| Message ID | 1439588061-18064-8-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > > 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; It's a singleshot (perhaps a couple in flight during the switch off), so there's no need for a permanent allocation. Fight the bloat! -Chris
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. > > 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
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". > >> >> 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
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. 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@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
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. 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? 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
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
On Thu, Aug 20, 2015 at 06:00:02PM +0300, Ville Syrjälä wrote: > 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>: > > > 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. E.g. on hsw with vga + hdmi/dp (which now happens easily since igt creates a fake vga output, just run kms_pipe_crc_basic) underruns happen reliably on modeset and then don't work anywhere. We also seem to fail to restore them somehow :( -Daniel
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)); } /**
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. 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(+)