[Intel-gfx] [PATCH RESEND] drm: i915: Preserve old FBC status for update without new planes

[Paulo Zanoni]

Em Sex, 2017-06-09 às 20:24 +0100, Chris Wilson escreveu:
> Quoting Gabriel Krisman Bertazi (2017-06-01 16:36:08)
> > If the atomic commit doesn't include any new plane, there is no
> > need to
> > choose a new CRTC for FBC, and the intel_fbc_choose_crtc() will
> > bail out
> > early.  Although, if the FBC setup failed in the previous commit,
> > if the
> > current commit doesn't include new plane update, it tries to
> > overwrite
> > no_fbc_reason to "no suitable CRTC for FBC".  For that scenario, it
> > is
> > better that we simply keep the old message in-place to make
> > debugging
> > easier.
> > 
> > A scenario where this happens is with the
> > igt at kms_frontbuffer_tracking@fbc-suspend testcase when executed on
> > a
> > Haswell system with not enough stolen memory.  When enabling the
> > CRTC,
> > the FBC setup will be correctly initialized to a specific CRTC, but
> > won't be enabled, since there is not enough memory.  The testcase
> > will
> > then enable CRC checking, which requires a quirk for Haswell, which
> > issues a new atomic commit that doesn't update the planes.  Since
> > that
> > update doesn't include any new planes (and the FBC wasn't enabled),
> > intel_fbc_choose_crtc() will not find any suitable CRTC, but update
> > the
> > error message, hiding the lack of memory information, which is the
> > actual cause of the initialization failure.  As a result, this
> > causes
> > that test, which should skip, to fail on Haswell.
> > 
> > Changes since v1:
> >   - Update commit message (Manasi)
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100020
> > Fixes: f7e9b004b8a3 ("drm/i915/fbc: inline intel_fbc_can_choose()")
> 
> I just don't see the test case as being a good reason to claim the
> kernel behaviour is broken. 

I agree we can't say the Kernel is broken here, especially since this
involves a debugfs interface. Still, there's an IGT failure that we
should fix somehow.


> The kernel may report any of the reasons as
> the one that caused FBC to be disabled (they are all valid, it is
> only
> the order in which we test, or the order in which we set the reason
> that
> determines the "reason" reported via debugfs). The test itself looks
> to
> be at fault for rejecting a valid FBC failure and claiming it to be a
> test failure. The test is far too fragile and dependent upon
> assumptions
> about the kernel internals.

I agree with your points, that's why I said in the other email that as
a longer term plan we need to think of a way to reduce the complexity
in the interaction between kms_frontbuffer_tracking and the Kernel.

The biggest issue here is to find a way to differentiate between "is
FBC disabled because there's not enough stolen memory" and "is FBC
disabled because there's a bug in the Kernel?". Perhaps we could try to
do these checks all in advance, in the fixture, and let the subtests
assume a safe environment. 

One idea would be to just let it fail, and then when QA/CI starts
reporting these failures we tell them to either increase the amount of
stolen memory or use older monitors.

Or maybe there's something else we could do. I'm open to suggestions
here.

> -Chris