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

[Ville Syrjälä]

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.

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

-- 
Ville Syrjälä
Intel OTC