| Message ID | 87fugv6ogy.fsf@dilma.collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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.
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();