diff mbox

[v2,2/3] drm/i915: add a selftest for the mmio_bases table

Message ID 20180308234629.27021-2-daniele.ceraolospurio@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Daniele Ceraolo Spurio March 8, 2018, 11:46 p.m. UTC 
Check that the entries are in reverse gen order and that the first entry
and all the following entries with gen > 0 have an mmio base set.

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/intel_engine_cs.c             |  1 +
 .../gpu/drm/i915/selftests/i915_mock_selftests.h   |  1 +
 drivers/gpu/drm/i915/selftests/intel_engine_cs.c   | 48 ++++++++++++++++++++++
 3 files changed, 50 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/selftests/intel_engine_cs.c

Comments

Chris Wilson March 9, 2018, 12:47 a.m. UTC | #1 
Quoting Daniele Ceraolo Spurio (2018-03-08 23:46:28)
> Check that the entries are in reverse gen order and that the first entry
> and all the following entries with gen > 0 have an mmio base set.
> 
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c             |  1 +
>  .../gpu/drm/i915/selftests/i915_mock_selftests.h   |  1 +
>  drivers/gpu/drm/i915/selftests/intel_engine_cs.c   | 48 ++++++++++++++++++++++
>  3 files changed, 50 insertions(+)
>  create mode 100644 drivers/gpu/drm/i915/selftests/intel_engine_cs.c
> 
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 08711665061c..a33171d82aee 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -2131,4 +2131,5 @@ void intel_disable_engine_stats(struct intel_engine_cs *engine)
>  
>  #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
>  #include "selftests/mock_engine.c"
> +#include "selftests/intel_engine_cs.c"
>  #endif
> diff --git a/drivers/gpu/drm/i915/selftests/i915_mock_selftests.h b/drivers/gpu/drm/i915/selftests/i915_mock_selftests.h
> index 9a48aa441743..2842f93ca29e 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_mock_selftests.h
> +++ b/drivers/gpu/drm/i915/selftests/i915_mock_selftests.h
> @@ -23,3 +23,4 @@ selftest(vma, i915_vma_mock_selftests)
>  selftest(evict, i915_gem_evict_mock_selftests)
>  selftest(gtt, i915_gem_gtt_mock_selftests)
>  selftest(hugepages, i915_gem_huge_page_mock_selftests)
> +selftest(engine, intel_engine_cs_mock_selftests)

Plonk this after uncore. It's a lowlevel sanity check that should come
before we start looking at features.

> diff --git a/drivers/gpu/drm/i915/selftests/intel_engine_cs.c b/drivers/gpu/drm/i915/selftests/intel_engine_cs.c
> new file mode 100644
> index 000000000000..8ef453905520
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/selftests/intel_engine_cs.c
> @@ -0,0 +1,48 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0
> + *
> + * Copyright © 2018 Intel Corporation
> + */
> +
> +#include "../i915_selftest.h"
> +
> +static int intel_mmio_bases_check(void)
> +{
> +       const struct engine_info *info;
> +       int i, j;
> +       u32 gen;
> +       s32 prev;
> +
> +       for (i = 0; i < ARRAY_SIZE(intel_engines); i++) {
> +               info = &intel_engines[i];
> +
> +               for (prev = -1, j = MAX_MMIO_BASES -1; j >= 0; j--) {
> +                       gen = info->mmio_bases[j].gen;
> +
> +                       if (prev >= (s32)gen) {
> +                               pr_err("%s: engine[%d]: mmio base for gen %x "
> +                                       "is before the one for gen %x\n",
> +                                      __func__, i, gen, prev);
> +                               return -EINVAL;
> +                       }
> +
> +                       if ((j == 0 || gen > 0) && !info->mmio_bases[j].base) {

Ok, setting gen=0 upset us. Make that gen=1 in the previous patch.

Looping backwards here definitely seems to make it harder than it needs
to be. We only need to validate the array as seen by the algorithm so,

u8 prev = U8_MAX;
for (j = 0; j < MAX_MMIO_BASES; j++) {
	u8 gen = info->mmio_bases[j].gen;

	if (gen >= prev) {
		...
		return -EINVAL;

	}

	if (gen == 0)
		break;
	
	if (!info->mmio.bases[j].base) {
		...
		return -EINVAL;
	}

	prev = gen;
}
pr_info("%s: min gen supported for %s = %d\n", __func__, magic_engine_name(i), prev);

I'm not sure how we could automate that check (not without hardcoding
the same information twice), so just print it.

> +
> +int intel_engine_cs_mock_selftests(void)
> +{
> +       return intel_mmio_bases_check();

I'd stick this in a
	static const struct i915_subtest tests[] = {
		SUBTEST(mmio_bases_check),
	};

	return i915_subtests(tests, NULL);
just for the convenience of adding more.
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 08711665061c..a33171d82aee 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -2131,4 +2131,5 @@  void intel_disable_engine_stats(struct intel_engine_cs *engine)
 
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftests/mock_engine.c"
+#include "selftests/intel_engine_cs.c"
 #endif
diff --git a/drivers/gpu/drm/i915/selftests/i915_mock_selftests.h b/drivers/gpu/drm/i915/selftests/i915_mock_selftests.h
index 9a48aa441743..2842f93ca29e 100644
--- a/drivers/gpu/drm/i915/selftests/i915_mock_selftests.h
+++ b/drivers/gpu/drm/i915/selftests/i915_mock_selftests.h
@@ -23,3 +23,4 @@  selftest(vma, i915_vma_mock_selftests)
 selftest(evict, i915_gem_evict_mock_selftests)
 selftest(gtt, i915_gem_gtt_mock_selftests)
 selftest(hugepages, i915_gem_huge_page_mock_selftests)
+selftest(engine, intel_engine_cs_mock_selftests)
diff --git a/drivers/gpu/drm/i915/selftests/intel_engine_cs.c b/drivers/gpu/drm/i915/selftests/intel_engine_cs.c
new file mode 100644
index 000000000000..8ef453905520
--- /dev/null
+++ b/drivers/gpu/drm/i915/selftests/intel_engine_cs.c
@@ -0,0 +1,48 @@ 
+/*
+ * SPDX-License-Identifier: GPL-2.0
+ *
+ * Copyright © 2018 Intel Corporation
+ */
+
+#include "../i915_selftest.h"
+
+static int intel_mmio_bases_check(void)
+{
+	const struct engine_info *info;
+	int i, j;
+	u32 gen;
+	s32 prev;
+
+	for (i = 0; i < ARRAY_SIZE(intel_engines); i++) {
+		info = &intel_engines[i];
+
+		for (prev = -1, j = MAX_MMIO_BASES -1; j >= 0; j--) {
+			gen = info->mmio_bases[j].gen;
+
+			if (prev >= (s32)gen) {
+				pr_err("%s: engine[%d]: mmio base for gen %x "
+					"is before the one for gen %x\n",
+				       __func__, i, gen, prev);
+				return -EINVAL;
+			}
+
+			if ((j == 0 || gen > 0) && !info->mmio_bases[j].base) {
+				pr_err("%s: engine[%d]: invalid mmio base (%x) "
+					"for gen %x at entry %u\n",
+				       __func__, i, info->mmio_bases[j].base, gen, j);
+				return -EINVAL;
+			}
+
+			/* we can have multiple empty entries in a row */
+			if (gen > 0)
+				prev = gen;
+		}
+	}
+
+	return 0;
+}
+
+int intel_engine_cs_mock_selftests(void)
+{
+	return intel_mmio_bases_check();
+}