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

[Gabriel Krisman Bertazi]

Chris Wilson <chris at chris-wilson.co.uk> writes:

> 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.

Hi Chris,

Thanks for the feedback.

Forget the testcase for a moment.  The good reason for the patch is that
the kernel wrongly reports the cause for the FBC failure, and that is what
I am trying to fix.  The current error message is meaningless to track
the real problem, and actually invalid, in my opinion, as that is simply
the correct behavior when choosing from a commit with no planes - There
is nothing to choose from.  In fact, if the commit without planes was
issued against a functional FBC, intel_fbc_choose_crtc() would not
execute and the previous compressed crtc would still be used.  In the
testcase itself, the message is only updated because of a quirk to
enabling CRC on HSW, which should be not related to FBC at all.

It is non-sense to report no suitable CRTC in this case.  This is only
hides a real problem, whatever that might be.

The current behavior is only a side effect inserted by an unrelated
commit f7e9b004b8a3 ("drm/i915/fbc: inline intel_fbc_can_choose()"),
which inadvertently changed the behavior when there isn't any planes in
the commit.  The result is a meaningless fbc_status, that doesn't
correspond to what is going on with the FBC.  This only happened to be
exposed by the unrelated testcase, frontbuffer_tracking, which has flaws
of its own to be addressed, as you pointed out.

My understanding is that we should correct the information provided by
debugfs with this patch AND also improve the testcase, as Paulo
suggested in his first email.

-- 
Gabriel Krisman Bertazi