[i-g-t] i915/query: Enable QUERY_TOPOLOGY_INFO for all gens

Submitted by Summers, Stuart on Feb. 9, 2019, 12:11 a.m.

Details

Message ID 20190209001146.14527-1-stuart.summers@intel.com
State New
Series "i915/query: Enable QUERY_TOPOLOGY_INFO for all gens"
Headers show

Commit Message

Summers, Stuart Feb. 9, 2019, 12:11 a.m.
Currently the QUERY_TOPOLOGY_INFO IOCTL is only being tested on haswell,
broadwell, skylake, kabylake, and coffeelake. The i915 driver doesn't
prevent other platforms from accessing this ioctl, so modify IGT to at
least test the interface, even if the platform/gen doesn't contain multiple
GTs.

Signed-off-by: Stuart Summers <stuart.summers@intel.com>
---
 tests/i915/query.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

diff --git a/tests/i915/query.c b/tests/i915/query.c
index 3e7fd140..e4757661 100644
--- a/tests/i915/query.c
+++ b/tests/i915/query.c
@@ -428,9 +428,6 @@  test_query_topology_known_pci_ids(int fd, int devid)
 	int n_slices = 0, n_subslices = 0;
 	int s, ss;
 
-	/* The GT size on some Broadwell skus is not defined, skip those. */
-	igt_skip_on(dev_info->gt == 0);
-
 	memset(&item, 0, sizeof(item));
 	item.query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
 	i915_query_items(fd, &item, 1);
@@ -477,7 +474,8 @@  test_query_topology_known_pci_ids(int fd, int devid)
 		igt_assert_eq(n_subslices, 3 * 3);
 		break;
 	default:
-		igt_assert(false);
+		/* nothing to check */
+		break;
 	}
 
 	free(topo_info);
@@ -524,9 +522,6 @@  igt_main
 
 	igt_subtest("query-topology-known-pci-ids") {
 		igt_require(query_topology_supported(fd));
-		igt_require(IS_HASWELL(devid) || IS_BROADWELL(devid) ||
-			    IS_SKYLAKE(devid) || IS_KABYLAKE(devid) ||
-			    IS_COFFEELAKE(devid));
 		test_query_topology_known_pci_ids(fd, devid);
 	}
 

Comments

Chris Wilson Feb. 9, 2019, 12:21 a.m.
Quoting Stuart Summers (2019-02-09 00:11:46)
> Currently the QUERY_TOPOLOGY_INFO IOCTL is only being tested on haswell,
> broadwell, skylake, kabylake, and coffeelake.

No, it's tested everywhere it's supported across the other tests. Only
this particular test is skipped if there is no hardcoded truth value as 
only a small number of known pci-ids are supplied.

Skip is the more accurate result (since the test is not doing anything it
sets out to do, and tests should be reasonably succinct), but you could
move the skip down into the switch() so that we don't repeat ourselves.
-Chris
Summers, Stuart Feb. 9, 2019, 12:32 a.m.
On Sat, 2019-02-09 at 00:21 +0000, Chris Wilson wrote:
> Quoting Stuart Summers (2019-02-09 00:11:46)
> > Currently the QUERY_TOPOLOGY_INFO IOCTL is only being tested on
> > haswell,
> > broadwell, skylake, kabylake, and coffeelake.
> 
> No, it's tested everywhere it's supported across the other tests.
> Only
> this particular test is skipped if there is no hardcoded truth value
> as 
> only a small number of known pci-ids are supplied.
> 
> Skip is the more accurate result (since the test is not doing
> anything it
> sets out to do, and tests should be reasonably succinct), but you
> could
> move the skip down into the switch() so that we don't repeat
> ourselves.
> -Chris

My reasoning for removing this was to exercise the asserts we are
calling within i915_query_items() for those platforms which do not
implement dev_info->gt. So we are checking something here, even if it
doesn't specifically analyze the slices/subslices for each platform.

I don't think the way it is currently implemented is wrong, but feel
the test could be used to potentially catch a bug if the driver were to
miss something in its dev_info structure for some platform, for
instance.

If you feel strongly we should keep this the way it is, I can drop
this.

Thanks,
Stuart
Chris Wilson Feb. 9, 2019, 12:38 a.m.
Quoting Summers, Stuart (2019-02-09 00:32:06)
> On Sat, 2019-02-09 at 00:21 +0000, Chris Wilson wrote:
> > Quoting Stuart Summers (2019-02-09 00:11:46)
> > > Currently the QUERY_TOPOLOGY_INFO IOCTL is only being tested on
> > > haswell,
> > > broadwell, skylake, kabylake, and coffeelake.
> > 
> > No, it's tested everywhere it's supported across the other tests.
> > Only
> > this particular test is skipped if there is no hardcoded truth value
> > as 
> > only a small number of known pci-ids are supplied.
> > 
> > Skip is the more accurate result (since the test is not doing
> > anything it
> > sets out to do, and tests should be reasonably succinct), but you
> > could
> > move the skip down into the switch() so that we don't repeat
> > ourselves.
> 
> My reasoning for removing this was to exercise the asserts we are
> calling within i915_query_items() for those platforms which do not
> implement dev_info->gt. So we are checking something here, even if it
> doesn't specifically analyze the slices/subslices for each platform.

But they are already covered. This test is very specifically about
comparing the reported topology for a small set of known topologies and
verifying they match.
-Chris
Summers, Stuart Feb. 9, 2019, 12:42 a.m.
On Sat, 2019-02-09 at 00:38 +0000, Chris Wilson wrote:
> Quoting Summers, Stuart (2019-02-09 00:32:06)
> > On Sat, 2019-02-09 at 00:21 +0000, Chris Wilson wrote:
> > > Quoting Stuart Summers (2019-02-09 00:11:46)
> > > > Currently the QUERY_TOPOLOGY_INFO IOCTL is only being tested on
> > > > haswell,
> > > > broadwell, skylake, kabylake, and coffeelake.
> > > 
> > > No, it's tested everywhere it's supported across the other tests.
> > > Only
> > > this particular test is skipped if there is no hardcoded truth
> > > value
> > > as 
> > > only a small number of known pci-ids are supplied.
> > > 
> > > Skip is the more accurate result (since the test is not doing
> > > anything it
> > > sets out to do, and tests should be reasonably succinct), but you
> > > could
> > > move the skip down into the switch() so that we don't repeat
> > > ourselves.
> > 
> > My reasoning for removing this was to exercise the asserts we are
> > calling within i915_query_items() for those platforms which do not
> > implement dev_info->gt. So we are checking something here, even if
> > it
> > doesn't specifically analyze the slices/subslices for each
> > platform.
> 
> But they are already covered. This test is very specifically about
> comparing the reported topology for a small set of known topologies
> and
> verifying they match.
> -Chris
Very true, and in more depth than this. I'll drop this. Thanks for the
feedback!