[Intel-gfx] [PATCH 07/16] drm/i915: disable FBC on FIFO underruns
Paulo Zanoni
przanoni at gmail.com
Thu Aug 20 06:30:17 PDT 2015
2015-08-19 9:06 GMT-03:00 Ville Syrjälä <ville.syrjala at 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 at 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 at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
More information about the Intel-gfx
mailing list