| Message ID | 20170516003304.32121-1-krisman@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <dorota.czaplejewicz@collabora.co.uk> > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.co.uk> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > 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 > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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.
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
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@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@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
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:
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 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. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100020 Fixes: f7e9b004b8a3 ("drm/i915/fbc: inline intel_fbc_can_choose()") Reported-by: Dorota Czaplejewicz <dorota.czaplejewicz@collabora.co.uk> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.co.uk> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_fbc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)