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

[Paulo Zanoni]

Em Sex, 2017-06-09 às 22:40 +0300, Ville Syrjälä escreveu:
> On Fri, Jun 09, 2017 at 08:24:59PM +0100, Chris Wilson wrote:
> > 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. 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.
> 
> Just a random idea that popped to my head (probably not for the first
> time); I think the most informative option would be to make the
> kernel
> report a separate fbc reason for each pipe. That way at least each
> pipe
> would have one clear reason for refusing fbc. Currently some of the
> reasons are per-pipe and some are global, which leads to this kind of
> confusion.

That makes a lot of sense. We can definitely consider it.

But this also includes the problem that multiple modesets on the same
pipe change no_fbc_reason, so at some point we lose the information
that FBC once failed on this pipe due to the lack of stolen memory, so
I'm not 100% sure this would fix the specific bug addressed by this
patch.

Also, it increases the complexity of the debugfs interface instead of
reducing, so I'd be more inclined to implement proposals that involve
killing no_fbc_reason.

Sometimes I wonder if we should start using tracing or actual (debug-
only) events to signal user space about FBC being
enabled/disabled/whatever. Part of the issue is that i915 keeps polling
debugfs for state, so it can lose info sometimes.

> 
> But that of course doesn't solve all the problems if the test
> continues
> to make fragile assumptions about the kernel behaviour.

It's a trade-off: the more we know (assume) about the Kernel, the more
we can try to test and also the more can go wrong like it went here.

>