Hi Gabriel, 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?
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? Manasi > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100020 > Fixes: f7e9b004b8a3 ("drm/i915/fbc: inline intel_fbc_can_choose()") > Reported-by: Dorota Czaplejewicz <[email protected]> > Signed-off-by: Gabriel Krisman Bertazi <[email protected]> > Cc: Paulo Zanoni <[email protected]> > --- > drivers/gpu/drm/i915/intel_fbc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c > b/drivers/gpu/drm/i915/intel_fbc.c > index ded2add18b26..0c99c9b731ee 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -1045,6 +1045,7 @@ void intel_fbc_choose_crtc(struct drm_i915_private > *dev_priv, > struct drm_plane *plane; > struct drm_plane_state *plane_state; > bool crtc_chosen = false; > + bool new_planes = false; > int i; > > mutex_lock(&fbc->lock); > @@ -1066,6 +1067,7 @@ void intel_fbc_choose_crtc(struct drm_i915_private > *dev_priv, > to_intel_plane_state(plane_state); > struct intel_crtc_state *intel_crtc_state; > struct intel_crtc *crtc = to_intel_crtc(plane_state->crtc); > + new_planes = true; > > if (!intel_plane_state->base.visible) > continue; > @@ -1084,7 +1086,7 @@ void intel_fbc_choose_crtc(struct drm_i915_private > *dev_priv, > break; > } > > - if (!crtc_chosen) > + if (new_planes && !crtc_chosen) > fbc->no_fbc_reason = "no suitable CRTC for FBC"; > > out: > -- > 2.11.0 > > _______________________________________________ > Intel-gfx mailing list > [email protected] > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
