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

[Manasi Navare]

On Tue, May 16, 2017 at 06:52:17PM -0700, Manasi Navare wrote:
> On Tue, May 16, 2017 at 10:27:33PM -0300, Gabriel Krisman Bertazi wrote:
> > Manasi Navare <manasi.d.navare at 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".
> > >
> > >

Could you reword this commit message like I mentioned above?
Everything else looks good to me.

Manasi

> > >> 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 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 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx