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@xxxxxxxxx> 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@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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
