Re: [PATCH] drm: i915: Preserve old FBC status if update with no new planes

[Manasi Navare <manasi.d.navare@intel.com>]

On Tue, May 16, 2017 at 10:27:33PM -0300, Gabriel Krisman Bertazi wrote:
> Manasi Navare <manasi.d.navare@intel.com> writes:
> 
> Hi Manasi,
> 
> > So the purpose of this patch is to avoid overwriting the no_fbc_reason
> > field during atomic_check in case there is no plane update so that
> > it retains the actual failure message from previous atomic commit operation
> > failure where it failed to enable fbc in intel_fbc_can_enable() during
> > the post plane update right?
> 
> yes, correct.
> 
> > On Mon, May 15, 2017 at 09:33:04PM -0300, Gabriel Krisman Bertazi wrote:
> >> 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 beforehand for whatever reason,
> >> we don't bail early, but we change the no_fbc_reason to "no suitable
> >> CRTC for FBC", which simply hides the real reason why the FBC wasn't
> >
> > I think this can be reworded a bit like " Although, if the FBC setup failed
> > in the previous commit, if the current commit doesnt include new plane update,
> > it tries to overwrite no_fbc_reason to "no suitable CRTC for FBC".
> >
> >
> >> initialized.  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@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 to fail on Haswell.
> >
> > So the problem here is just a wrong error message.
> > How does a wrong error message cause the IGT test to fail?
> 
> igt is prepared to skip the test on boxes where there isn't enough
> stolen memory, but since we overwrite that message, the test will
> execute and fail.  We discussed earlier on the list about adding a new
> check to igt for the "no suitable CRTC for FBC" message, but that could
> end up hiding other real error conditions.
>

Ok, yes then this fix makes sense. In that case it looks good to me.

Manasi 
> -- 
> Gabriel Krisman Bertazi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx