2015-08-20 10:58 GMT-03:00 Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>:
> 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.
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@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
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
