Re: [PATCH RESEND i-g-t 2/2] kms_frontbuffer_tracking: Don't poke compressing status for old cpus

Re: [PATCH RESEND i-g-t 2/2] kms_frontbuffer_tracking: Don't poke compressing status for old cpus

[Gabriel Krisman Bertazi]

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

> Ouch... Good catch!
>
> Can you please move the logic to the setup_fbc() function?
>
> if (gen < 7)
> 	opt.fbc_check_compression = false;
>
> This way we avoid redoing the same check a trillion times during
> kms_frontbuffer_tracking execution.
>
> Also, I think this one also lacks a Bugzilla tag:
> https://bugs.freedesktop.org/show_bug.cgi?id=100677

Hmm, yeah, I forgot CC'ing the list when resending. Sorry about that.

Thanks for reviewing.  Can you take a look at the following version
instead?

>8
Subject: [PATCH] kms_frontbuffer_tracking: Don't poke compressing status for
 old cpus

Commit 2804afc606f8 ("kms_frontbuffer_tracking: fix compression
checking") removes the check whether the kernel supports reporting the
compression status before asserting on it.  This breaks the test for no
good reason on old CPUs (SNB and earlier) where the kernel can't report
the compression status.  Instead, we can check if the cpu doesn't
support reporting and adopt the same behavior as if
--no-fbc-compression-check was used.

Changes since v1:
  - Move verification to setup_fbc (Paulo)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100677
Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.co.uk>
---
 tests/kms_frontbuffer_tracking.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index b49f1abacae8..a655730b4ad3 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -1524,6 +1524,7 @@ static bool fbc_supported_on_chipset(void)
 static void setup_fbc(void)
 {
 	drmModeConnectorPtr c = get_connector(prim_mode_params.connector_id);
+	int devid = intel_get_drm_devid(drm.fd);
 
 	if (!fbc_supported_on_chipset()) {
 		igt_info("Can't test FBC: not supported on this chipset\n");
@@ -1540,6 +1541,11 @@ static void setup_fbc(void)
 			 "pipe A\n");
 		return;
 	}
+
+	/* Early Generations are not able to report compression status. */
+	if (!AT_LEAST_GEN(devid, 7))
+		opt.fbc_check_compression = false;
+
 	fbc.can_test = true;
 
 	fbc_setup_last_action();
-- 
2.11.0

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

Re: [PATCH RESEND i-g-t 2/2] kms_frontbuffer_tracking: Don't poke compressing status for old cpus

[Ville Syrjälä]

On Wed, Apr 26, 2017 at 02:57:49PM -0300, Gabriel Krisman Bertazi wrote:
> Paulo Zanoni <paulo.r.zanoni@intel.com> writes:
> 
> > Ouch... Good catch!
> >
> > Can you please move the logic to the setup_fbc() function?
> >
> > if (gen < 7)
> > 	opt.fbc_check_compression = false;
> >
> > This way we avoid redoing the same check a trillion times during
> > kms_frontbuffer_tracking execution.
> >
> > Also, I think this one also lacks a Bugzilla tag:
> > https://bugs.freedesktop.org/show_bug.cgi?id=100677
> 
> Hmm, yeah, I forgot CC'ing the list when resending. Sorry about that.
> 
> Thanks for reviewing.  Can you take a look at the following version
> instead?
> 
> >8
> Subject: [PATCH] kms_frontbuffer_tracking: Don't poke compressing status for
>  old cpus
> 
> Commit 2804afc606f8 ("kms_frontbuffer_tracking: fix compression
> checking") removes the check whether the kernel supports reporting the
> compression status before asserting on it.  This breaks the test for no
> good reason on old CPUs (SNB and earlier) where the kernel can't report
> the compression status.  Instead, we can check if the cpu doesn't
> support reporting and adopt the same behavior as if

I have a feeling I asked this before, but why aren't we just fixing
the kernel to report it correctly? For any platform with FBC2 it
should be trivial, for FBC1 slightly more complicate as you probably
have to check each individual tag.

> --no-fbc-compression-check was used.
> 
> Changes since v1:
>   - Move verification to setup_fbc (Paulo)
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100677
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.co.uk>
> ---
>  tests/kms_frontbuffer_tracking.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
> index b49f1abacae8..a655730b4ad3 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -1524,6 +1524,7 @@ static bool fbc_supported_on_chipset(void)
>  static void setup_fbc(void)
>  {
>  	drmModeConnectorPtr c = get_connector(prim_mode_params.connector_id);
> +	int devid = intel_get_drm_devid(drm.fd);
>  
>  	if (!fbc_supported_on_chipset()) {
>  		igt_info("Can't test FBC: not supported on this chipset\n");
> @@ -1540,6 +1541,11 @@ static void setup_fbc(void)
>  			 "pipe A\n");
>  		return;
>  	}
> +
> +	/* Early Generations are not able to report compression status. */
> +	if (!AT_LEAST_GEN(devid, 7))
> +		opt.fbc_check_compression = false;
> +
>  	fbc.can_test = true;
>  
>  	fbc_setup_last_action();
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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 i-g-t 2/2] kms_frontbuffer_tracking: Don't poke compressing status for old cpus

[Paulo Zanoni]

Em Qua, 2017-04-26 às 21:12 +0300, Ville Syrjälä escreveu:
> On Wed, Apr 26, 2017 at 02:57:49PM -0300, Gabriel Krisman Bertazi
> wrote:
> > 
> > Paulo Zanoni <paulo.r.zanoni@intel.com> writes:
> > 
> > > 
> > > Ouch... Good catch!
> > > 
> > > Can you please move the logic to the setup_fbc() function?
> > > 
> > > if (gen < 7)
> > > 	opt.fbc_check_compression = false;
> > > 
> > > This way we avoid redoing the same check a trillion times during
> > > kms_frontbuffer_tracking execution.
> > > 
> > > Also, I think this one also lacks a Bugzilla tag:
> > > https://bugs.freedesktop.org/show_bug.cgi?id=100677
> > 
> > Hmm, yeah, I forgot CC'ing the list when resending. Sorry about
> > that.
> > 
> > Thanks for reviewing.  Can you take a look at the following version
> > instead?
> > 
> > > 
> > > 8
> > Subject: [PATCH] kms_frontbuffer_tracking: Don't poke compressing
> > status for
> >  old cpus
> > 
> > Commit 2804afc606f8 ("kms_frontbuffer_tracking: fix compression
> > checking") removes the check whether the kernel supports reporting
> > the
> > compression status before asserting on it.  This breaks the test
> > for no
> > good reason on old CPUs (SNB and earlier) where the kernel can't
> > report
> > the compression status.  Instead, we can check if the cpu doesn't
> > support reporting and adopt the same behavior as if
> 
> I have a feeling I asked this before, but why aren't we just fixing
> the kernel to report it correctly? For any platform with FBC2 it
> should be trivial,

Right, I see there's a reg for that for ILK/SNB.

>  for FBC1 slightly more complicate as you probably
> have to check each individual tag.

I didn't check the docs for that.

Maybe we should change the comment from "early generations are not able
to report compression status" to something more accurate like "the
Kernel doesn't report compression status for early generations".


There are quite a few different ways to solve the problem involved in
this patch, and some of the would remove the need to check for platform
generations in the user space side. An example alternative would be to
always print "Compressing: " and then put "no" when FBC is disabled and
"unknown" for platforms where we don't know what to print. In fact it's
still on my TODO list to add a ton more information to i915_fbc_status,
but I'm not going to work on that soon. And there's always the problem
with having to sync Kernel and IGT.

Anyway, the current patch plugs the current hole, so I think further
improvements to this area can come on top of it:

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


> 
> > 
> > --no-fbc-compression-check was used.
> > 
> > Changes since v1:
> >   - Move verification to setup_fbc (Paulo)
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100677
> > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.co.uk>
> > ---
> >  tests/kms_frontbuffer_tracking.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/tests/kms_frontbuffer_tracking.c
> > b/tests/kms_frontbuffer_tracking.c
> > index b49f1abacae8..a655730b4ad3 100644
> > --- a/tests/kms_frontbuffer_tracking.c
> > +++ b/tests/kms_frontbuffer_tracking.c
> > @@ -1524,6 +1524,7 @@ static bool fbc_supported_on_chipset(void)
> >  static void setup_fbc(void)
> >  {
> >  	drmModeConnectorPtr c =
> > get_connector(prim_mode_params.connector_id);
> > +	int devid = intel_get_drm_devid(drm.fd);
> >  
> >  	if (!fbc_supported_on_chipset()) {
> >  		igt_info("Can't test FBC: not supported on this
> > chipset\n");
> > @@ -1540,6 +1541,11 @@ static void setup_fbc(void)
> >  			 "pipe A\n");
> >  		return;
> >  	}
> > +
> > +	/* Early Generations are not able to report compression
> > status. */
> > +	if (!AT_LEAST_GEN(devid, 7))
> > +		opt.fbc_check_compression = false;
> > +
> >  	fbc.can_test = true;
> >  
> >  	fbc_setup_last_action();
> > -- 
> > 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 RESEND i-g-t 2/2] kms_frontbuffer_tracking: Don't poke compressing status for old cpus

[Petri Latvala]

On Wed, Apr 26, 2017 at 03:36:16PM -0300, Paulo Zanoni wrote:
> > I have a feeling I asked this before, but why aren't we just fixing
> > the kernel to report it correctly? For any platform with FBC2 it
> > should be trivial,
> 
> Right, I see there's a reg for that for ILK/SNB.
> 
> >  for FBC1 slightly more complicate as you probably
> > have to check each individual tag.
> 
> I didn't check the docs for that.
> 
> Maybe we should change the comment from "early generations are not able
> to report compression status" to something more accurate like "the
> Kernel doesn't report compression status for early generations".
> 
> 
> There are quite a few different ways to solve the problem involved in
> this patch, and some of the would remove the need to check for platform
> generations in the user space side. An example alternative would be to
> always print "Compressing: " and then put "no" when FBC is disabled and
> "unknown" for platforms where we don't know what to print. In fact it's
> still on my TODO list to add a ton more information to i915_fbc_status,
> but I'm not going to work on that soon. And there's always the problem
> with having to sync Kernel and IGT.
> 
> Anyway, the current patch plugs the current hole, so I think further
> improvements to this area can come on top of it:
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>



Pushed this patch, thanks.


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

Re: [PATCH RESEND i-g-t 1/2] tests/kms_frontbuffer_tracking: Skip if CRTC not selected

[Gabriel Krisman Bertazi]

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

> Em Qui, 2017-04-20 às 11:11 -0300, Gabriel Krisman Bertazi escreveu:
>> After Linux commit f7e9b004b8a3 ("drm/i915/fbc: inline
>> intel_fbc_can_choose()"), no_fbc_reason will be updated to a new
>> error
>> message if we don't have a plane in the new state, which should make
>> the
>> test skip, but the test code doesn't catch that message and tries to
>
> What was the previous message that was making the test skip? I was not
> expecting that patch would affect kms_frontbuffer_tracking behavior.

The previous message was "FBC disabled: not enough stolen memory".  You
got me wondering if we should even enter that function if
intel_fbc_alloc_cfb failed.  The patch I mentioned starts triggering the
failure because it changes the error message even if we never enter the
for_each_new_plane_in_state loop in intel_fbc_choose_crtc.

Does that makes sense?  Let me take another look at the code and see
what I can come up with.


>
>> execute the test, triggering a test failure.
>
> Anyway, I'm trying to understand the situation here. I'm not sure if
> this fix is correct, I'm wondering if it's a problem in how the Kernel
> sets the messages. Please explain in more details why this patch does
> what it does.
>

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

Re: [PATCH RESEND i-g-t 1/2] tests/kms_frontbuffer_tracking: Skip if CRTC not selected

[Zanoni, Paulo R]

Em Qua, 2017-04-26 às 15:40 -0300, Gabriel Krisman Bertazi escreveu:
> Paulo Zanoni <paulo.r.zanoni@intel.com> writes:
> 
> > 
> > Em Qui, 2017-04-20 às 11:11 -0300, Gabriel Krisman Bertazi
> > escreveu:
> > > 
> > > After Linux commit f7e9b004b8a3 ("drm/i915/fbc: inline
> > > intel_fbc_can_choose()"), no_fbc_reason will be updated to a new
> > > error
> > > message if we don't have a plane in the new state, which should
> > > make
> > > the
> > > test skip, but the test code doesn't catch that message and tries
> > > to
> > 
> > What was the previous message that was making the test skip? I was
> > not
> > expecting that patch would affect kms_frontbuffer_tracking
> > behavior.
> 
> The previous message was "FBC disabled: not enough stolen
> memory".  You
> got me wondering if we should even enter that function if
> intel_fbc_alloc_cfb failed.  The patch I mentioned starts triggering
> the
> failure because it changes the error message even if we never enter
> the
> for_each_new_plane_in_state loop in intel_fbc_choose_crtc.
> 
> Does that makes sense?  Let me take another look at the code and see
> what I can come up with.

I think I understand the problem now. My problem with the current
approach is that it changes the kms_frontbuffer_tracking behavior for
more cases than the one affected by this bug, so there's a risk we're
going to end up skipping when we should really be failing. Any case
where we end up not finding a suitable CRTC will be skipped, instead of
only skipping where we can't find a suitable CRTC due to not having
enough stolen memory.

I was already planning on maybe switching opt.small_modes to default as
true. This should help not only on cases where there's not enough
stolen memory, but also on cases where the monitor resolution is too
big for FBC. This would fix the bug.

The reason why I still didn't switch opt.small_modes to true by default
is because I didn't check if doing this will introduce any other
unintended regressions. It shouldn't, but we'll never know until we
actually try. Maybe we should check this with the CI team.

For our CI machines specifically, I think we should also consider just
going to their BIOSes and increasing the amount of stolen memory. We
really want to try to minimize the amount of SKIPs in cases where we
could be testing stuff, so increasing the amount of stolen memory
really sounds the ideal solution for CI.

So 3 possible solutions (and maybe we should do them all):

- Switch opt.small_modes to true by default, and check if this exposes
new problems.
- Increase the amount of stolen memory for CI machines.
- Somehow go back to the previous behavior where the Kernel tells us
that it tried to enable FBC but there's not enough stolen memory.

Thanks,
Paulo

> 
> 
> > 
> > 
> > > 
> > > execute the test, triggering a test failure.
> > 
> > Anyway, I'm trying to understand the situation here. I'm not sure
> > if
> > this fix is correct, I'm wondering if it's a problem in how the
> > Kernel
> > sets the messages. Please explain in more details why this patch
> > does
> > what it does.
> > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx