[PATCH RESEND] drm: i915: Preserve old FBC status for update without new planes

[PATCH RESEND] drm: i915: Preserve old FBC status for update without 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 in the previous commit, if the
current commit doesn't include new plane update, it tries to overwrite
no_fbc_reason to "no suitable CRTC for FBC".  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, which should skip, to fail on Haswell.

Changes since v1:
  - Update commit message (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

✓ Fi.CI.BAT: success for drm: i915: Preserve old FBC status for update without new planes (rev2)

[Patchwork]

== Series Details ==

Series: drm: i915: Preserve old FBC status for update without new planes (rev2)
URL   : https://patchwork.freedesktop.org/series/24635/
State : success

== Summary ==

Series 24635v2 drm: i915: Preserve old FBC status for update without new planes
https://patchwork.freedesktop.org/api/1.0/series/24635/revisions/2/mbox/

Test kms_busy:
        Subgroup basic-flip-default-b:
                pass       -> DMESG-WARN (fi-skl-6700hq) fdo#101144 +2
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                pass       -> FAIL       (fi-skl-6700hq) fdo#101154 +7

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

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:439s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:430s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:574s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:507s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:485s
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:434s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:414s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:419s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:490s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:460s
fi-kbl-7500u     total:278  pass:255  dwarn:5   dfail:0   fail:0   skip:18  time:469s
fi-kbl-7560u     total:278  pass:263  dwarn:5   dfail:0   fail:0   skip:10  time:570s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:469s
fi-skl-6700hq    total:278  pass:229  dwarn:1   dfail:0   fail:26  skip:22  time:408s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:464s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:502s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:440s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:532s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:400s

2c9abf8ec88cf4b3d3735976bb0ff7a4991946b2 drm-tip: 2017y-06m-01d-13h-29m-05s UTC integration manifest
72395e7 drm: i915: Preserve old FBC status for update without new planes

== Logs ==

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

Re: [PATCH RESEND] drm: i915: Preserve old FBC status for update without new planes

[Manasi Navare]

The modified commit message looks good to me.

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>


On Thu, Jun 01, 2017 at 12:36:08PM -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 in the previous commit, if the
> current commit doesn't include new plane update, it tries to overwrite
> no_fbc_reason to "no suitable CRTC for FBC".  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, which should skip, to fail on Haswell.
> 
> Changes since v1:
>   - Update commit message (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

Re: [PATCH RESEND] drm: i915: Preserve old FBC status for update without new planes

[Paulo Zanoni]

Em Qui, 2017-06-01 às 16:09 -0700, Manasi Navare escreveu:
> The modified commit message looks good to me.
> 
> Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

As a longer term plan we could try to think some way to reduce the
complexity between the Kernel and IGT interaction here: maybe there's a
way to make things work without having to preserve no_fbc_reason
between multiple commits. But I don't have any specific ideas, so
suggestions are welcome.


> 
> On Thu, Jun 01, 2017 at 12:36:08PM -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 in the previous commit,
> > if the
> > current commit doesn't include new plane update, it tries to
> > overwrite
> > no_fbc_reason to "no suitable CRTC for FBC".  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, which should skip, to fail on Haswell.
> > 
> > Changes since v1:
> >   - Update commit message (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

Re: [PATCH RESEND] drm: i915: Preserve old FBC status for update without new planes

[Chris Wilson]

Quoting Gabriel Krisman Bertazi (2017-06-01 16:36:08)
> 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 in the previous commit, if the
> current commit doesn't include new plane update, it tries to overwrite
> no_fbc_reason to "no suitable CRTC for FBC".  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, which should skip, to fail on Haswell.
> 
> Changes since v1:
>   - Update commit message (Manasi)
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100020
> Fixes: f7e9b004b8a3 ("drm/i915/fbc: inline intel_fbc_can_choose()")

I just don't see the test case as being a good reason to claim the
kernel behaviour is broken. The kernel may report any of the reasons as
the one that caused FBC to be disabled (they are all valid, it is only
the order in which we test, or the order in which we set the reason that
determines the "reason" reported via debugfs). The test itself looks to
be at fault for rejecting a valid FBC failure and claiming it to be a
test failure. The test is far too fragile and dependent upon assumptions
about the kernel internals.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RESEND] drm: i915: Preserve old FBC status for update without new planes

[Ville Syrjälä]

On Fri, Jun 09, 2017 at 08:24:59PM +0100, Chris Wilson wrote:
> Quoting Gabriel Krisman Bertazi (2017-06-01 16:36:08)
> > 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 in the previous commit, if the
> > current commit doesn't include new plane update, it tries to overwrite
> > no_fbc_reason to "no suitable CRTC for FBC".  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, which should skip, to fail on Haswell.
> > 
> > Changes since v1:
> >   - Update commit message (Manasi)
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100020
> > Fixes: f7e9b004b8a3 ("drm/i915/fbc: inline intel_fbc_can_choose()")
> 
> I just don't see the test case as being a good reason to claim the
> kernel behaviour is broken. The kernel may report any of the reasons as
> the one that caused FBC to be disabled (they are all valid, it is only
> the order in which we test, or the order in which we set the reason that
> determines the "reason" reported via debugfs). The test itself looks to
> be at fault for rejecting a valid FBC failure and claiming it to be a
> test failure. The test is far too fragile and dependent upon assumptions
> about the kernel internals.

Just a random idea that popped to my head (probably not for the first
time); I think the most informative option would be to make the kernel
report a separate fbc reason for each pipe. That way at least each pipe
would have one clear reason for refusing fbc. Currently some of the
reasons are per-pipe and some are global, which leads to this kind of
confusion.

But that of course doesn't solve all the problems if the test continues
to make fragile assumptions about the kernel behaviour.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RESEND] drm: i915: Preserve old FBC status for update without new planes

[Paulo Zanoni]

Em Sex, 2017-06-09 às 22:40 +0300, Ville Syrjälä escreveu:
> On Fri, Jun 09, 2017 at 08:24:59PM +0100, Chris Wilson wrote:
> > Quoting Gabriel Krisman Bertazi (2017-06-01 16:36:08)
> > > 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 in the previous commit,
> > > if the
> > > current commit doesn't include new plane update, it tries to
> > > overwrite
> > > no_fbc_reason to "no suitable CRTC for FBC".  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, which should skip, to fail on Haswell.
> > > 
> > > Changes since v1:
> > >   - Update commit message (Manasi)
> > > 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100020
> > > Fixes: f7e9b004b8a3 ("drm/i915/fbc: inline
> > > intel_fbc_can_choose()")
> > 
> > I just don't see the test case as being a good reason to claim the
> > kernel behaviour is broken. The kernel may report any of the
> > reasons as
> > the one that caused FBC to be disabled (they are all valid, it is
> > only
> > the order in which we test, or the order in which we set the reason
> > that
> > determines the "reason" reported via debugfs). The test itself
> > looks to
> > be at fault for rejecting a valid FBC failure and claiming it to be
> > a
> > test failure. The test is far too fragile and dependent upon
> > assumptions
> > about the kernel internals.
> 
> Just a random idea that popped to my head (probably not for the first
> time); I think the most informative option would be to make the
> kernel
> report a separate fbc reason for each pipe. That way at least each
> pipe
> would have one clear reason for refusing fbc. Currently some of the
> reasons are per-pipe and some are global, which leads to this kind of
> confusion.

That makes a lot of sense. We can definitely consider it.

But this also includes the problem that multiple modesets on the same
pipe change no_fbc_reason, so at some point we lose the information
that FBC once failed on this pipe due to the lack of stolen memory, so
I'm not 100% sure this would fix the specific bug addressed by this
patch.

Also, it increases the complexity of the debugfs interface instead of
reducing, so I'd be more inclined to implement proposals that involve
killing no_fbc_reason.

Sometimes I wonder if we should start using tracing or actual (debug-
only) events to signal user space about FBC being
enabled/disabled/whatever. Part of the issue is that i915 keeps polling
debugfs for state, so it can lose info sometimes.

> 
> But that of course doesn't solve all the problems if the test
> continues
> to make fragile assumptions about the kernel behaviour.

It's a trade-off: the more we know (assume) about the Kernel, the more
we can try to test and also the more can go wrong like it went here.

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

Re: [PATCH RESEND] drm: i915: Preserve old FBC status for update without new planes

[Gabriel Krisman Bertazi]

Paulo Zanoni <paulo.r.zanoni@intel.com> writes:

> Em Sex, 2017-06-09 às 22:40 +0300, Ville Syrjälä escreveu:
>> On Fri, Jun 09, 2017 at 08:24:59PM +0100, Chris Wilson wrote:

>> Just a random idea that popped to my head (probably not for the first
>> time); I think the most informative option would be to make the
>> kernel
>> report a separate fbc reason for each pipe. That way at least each
>> pipe
>> would have one clear reason for refusing fbc. Currently some of the
>> reasons are per-pipe and some are global, which leads to this kind of
>> confusion.
>
> That makes a lot of sense. We can definitely consider it.
>
> But this also includes the problem that multiple modesets on the same
> pipe change no_fbc_reason, so at some point we lose the information
> that FBC once failed on this pipe due to the lack of stolen memory, so
> I'm not 100% sure this would fix the specific bug addressed by this
> patch.

It would definitely improve the situation, despite not making a
difference for this specific case.

>
> Also, it increases the complexity of the debugfs interface instead of
> reducing, so I'd be more inclined to implement proposals that involve
> killing no_fbc_reason.
>
> Sometimes I wonder if we should start using tracing or actual (debug-
> only) events to signal user space about FBC being
> enabled/disabled/whatever. Part of the issue is that i915 keeps polling
> debugfs for state, so it can lose info sometimes.

I think this is what would make the most sense to improve the testcase.
Is it possible to increase the buffer logged by fbc_status, such that we
don't miss old errors recently reported?  Is that a good idea for a
debugfs interface?  That would maybe minimize the complexity of the
kernel-userspace interface.


>
>> 
>> But that of course doesn't solve all the problems if the test
>> continues
>> to make fragile assumptions about the kernel behaviour.
>
> It's a trade-off: the more we know (assume) about the Kernel, the more
> we can try to test and also the more can go wrong like it went here.
>
>> 

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

Re: [PATCH RESEND] drm: i915: Preserve old FBC status for update without new planes

[Paulo Zanoni]

Em Sex, 2017-06-09 às 20:24 +0100, Chris Wilson escreveu:
> Quoting Gabriel Krisman Bertazi (2017-06-01 16:36:08)
> > 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 in the previous commit,
> > if the
> > current commit doesn't include new plane update, it tries to
> > overwrite
> > no_fbc_reason to "no suitable CRTC for FBC".  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, which should skip, to fail on Haswell.
> > 
> > Changes since v1:
> >   - Update commit message (Manasi)
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100020
> > Fixes: f7e9b004b8a3 ("drm/i915/fbc: inline intel_fbc_can_choose()")
> 
> I just don't see the test case as being a good reason to claim the
> kernel behaviour is broken. 

I agree we can't say the Kernel is broken here, especially since this
involves a debugfs interface. Still, there's an IGT failure that we
should fix somehow.


> The kernel may report any of the reasons as
> the one that caused FBC to be disabled (they are all valid, it is
> only
> the order in which we test, or the order in which we set the reason
> that
> determines the "reason" reported via debugfs). The test itself looks
> to
> be at fault for rejecting a valid FBC failure and claiming it to be a
> test failure. The test is far too fragile and dependent upon
> assumptions
> about the kernel internals.

I agree with your points, that's why I said in the other email that as
a longer term plan we need to think of a way to reduce the complexity
in the interaction between kms_frontbuffer_tracking and the Kernel.

The biggest issue here is to find a way to differentiate between "is
FBC disabled because there's not enough stolen memory" and "is FBC
disabled because there's a bug in the Kernel?". Perhaps we could try to
do these checks all in advance, in the fixture, and let the subtests
assume a safe environment. 

One idea would be to just let it fail, and then when QA/CI starts
reporting these failures we tell them to either increase the amount of
stolen memory or use older monitors.

Or maybe there's something else we could do. I'm open to suggestions
here.

> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RESEND] drm: i915: Preserve old FBC status for update without new planes

[Gabriel Krisman Bertazi]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> I just don't see the test case as being a good reason to claim the
> kernel behaviour is broken. The kernel may report any of the reasons as
> the one that caused FBC to be disabled (they are all valid, it is only
> the order in which we test, or the order in which we set the reason that
> determines the "reason" reported via debugfs). The test itself looks to
> be at fault for rejecting a valid FBC failure and claiming it to be a
> test failure. The test is far too fragile and dependent upon assumptions
> about the kernel internals.

Hi Chris,

Thanks for the feedback.

Forget the testcase for a moment.  The good reason for the patch is that
the kernel wrongly reports the cause for the FBC failure, and that is what
I am trying to fix.  The current error message is meaningless to track
the real problem, and actually invalid, in my opinion, as that is simply
the correct behavior when choosing from a commit with no planes - There
is nothing to choose from.  In fact, if the commit without planes was
issued against a functional FBC, intel_fbc_choose_crtc() would not
execute and the previous compressed crtc would still be used.  In the
testcase itself, the message is only updated because of a quirk to
enabling CRC on HSW, which should be not related to FBC at all.

It is non-sense to report no suitable CRTC in this case.  This is only
hides a real problem, whatever that might be.

The current behavior is only a side effect inserted by an unrelated
commit f7e9b004b8a3 ("drm/i915/fbc: inline intel_fbc_can_choose()"),
which inadvertently changed the behavior when there isn't any planes in
the commit.  The result is a meaningless fbc_status, that doesn't
correspond to what is going on with the FBC.  This only happened to be
exposed by the unrelated testcase, frontbuffer_tracking, which has flaws
of its own to be addressed, as you pointed out.

My understanding is that we should correct the information provided by
debugfs with this patch AND also improve the testcase, as Paulo
suggested in his first email.

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