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