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

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

[Gabriel Krisman Bertazi]

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(-)

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

✓ Fi.CI.BAT: success for drm: i915: Preserve old FBC status if update with no new planes

[Patchwork]

== Series Details ==

Series: drm: i915: Preserve old FBC status if update with no new planes
URL   : https://patchwork.freedesktop.org/series/24471/
State : success

== Summary ==

Series 24471v1 drm: i915: Preserve old FBC status if update with no new planes
https://patchwork.freedesktop.org/api/1.0/series/24471/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:446s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:433s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:578s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:520s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:489s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:418s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:412s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:424s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:489s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:470s
fi-kbl-7500u     total:278  pass:255  dwarn:5   dfail:0   fail:0   skip:18  time:467s
fi-kbl-7560u     total:278  pass:263  dwarn:5   dfail:0   fail:0   skip:10  time:629s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:470s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time:589s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:470s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:501s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:437s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:534s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time:400s
fi-byt-j1900 failed to collect. IGT log at Patchwork_4704/fi-byt-j1900/igt.log

9b25870f9fa4548ec2bb40e42fa28f35db2189e1 drm-tip: 2017y-05m-15d-15h-47m-31s UTC integration manifest
2ad3fb5 drm: i915: Preserve old FBC status if update with no new planes

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4704/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Manasi Navare]

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

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

[Gabriel Krisman Bertazi]

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.

-- 
Gabriel Krisman Bertazi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Manasi Navare]

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

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