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