* [PATCH] drm: i915: Preserve old FBC status if update with no new planes
@ 2017-05-16 0:33 Gabriel Krisman Bertazi
2017-05-16 0:49 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-05-16 23:45 ` [PATCH] " Manasi Navare
0 siblings, 2 replies; 6+ messages in thread
From: Gabriel Krisman Bertazi @ 2017-05-16 0:33 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* ✓ Fi.CI.BAT: success for drm: i915: Preserve old FBC status if update with no new planes
2017-05-16 0:33 [PATCH] drm: i915: Preserve old FBC status if update with no new planes Gabriel Krisman Bertazi
@ 2017-05-16 0:49 ` Patchwork
2017-05-16 23:45 ` [PATCH] " Manasi Navare
1 sibling, 0 replies; 6+ messages in thread
From: Patchwork @ 2017-05-16 0:49 UTC (permalink / raw)
To: Gabriel Krisman Bertazi; +Cc: intel-gfx
== 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm: i915: Preserve old FBC status if update with no new planes
2017-05-16 0:33 [PATCH] drm: i915: Preserve old FBC status if update with no new planes Gabriel Krisman Bertazi
2017-05-16 0:49 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2017-05-16 23:45 ` Manasi Navare
2017-05-17 1:27 ` Gabriel Krisman Bertazi
1 sibling, 1 reply; 6+ messages in thread
From: Manasi Navare @ 2017-05-16 23:45 UTC (permalink / raw)
To: Gabriel Krisman Bertazi; +Cc: intel-gfx, Paulo Zanoni
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm: i915: Preserve old FBC status if update with no new planes
2017-05-16 23:45 ` [PATCH] " Manasi Navare
@ 2017-05-17 1:27 ` Gabriel Krisman Bertazi
2017-05-17 1:52 ` Manasi Navare
0 siblings, 1 reply; 6+ messages in thread
From: Gabriel Krisman Bertazi @ 2017-05-17 1:27 UTC (permalink / raw)
To: Manasi Navare; +Cc: intel-gfx, Paulo Zanoni
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm: i915: Preserve old FBC status if update with no new planes
2017-05-17 1:27 ` Gabriel Krisman Bertazi
@ 2017-05-17 1:52 ` Manasi Navare
2017-05-17 18:45 ` Manasi Navare
0 siblings, 1 reply; 6+ messages in thread
From: Manasi Navare @ 2017-05-17 1:52 UTC (permalink / raw)
To: Gabriel Krisman Bertazi; +Cc: intel-gfx, Paulo Zanoni
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm: i915: Preserve old FBC status if update with no new planes
2017-05-17 1:52 ` Manasi Navare
@ 2017-05-17 18:45 ` Manasi Navare
0 siblings, 0 replies; 6+ messages in thread
From: Manasi Navare @ 2017-05-17 18:45 UTC (permalink / raw)
To: Gabriel Krisman Bertazi; +Cc: intel-gfx, Paulo Zanoni
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-17 18:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-16 0:33 [PATCH] drm: i915: Preserve old FBC status if update with no new planes Gabriel Krisman Bertazi
2017-05-16 0:49 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-05-16 23:45 ` [PATCH] " Manasi Navare
2017-05-17 1:27 ` Gabriel Krisman Bertazi
2017-05-17 1:52 ` Manasi Navare
2017-05-17 18:45 ` Manasi Navare
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.