[PATCH 1/2] drm/i915: Add MOCS state dump to debugfs

[PATCH 1/2] drm/i915: Add MOCS state dump to debugfs

[Stuart Summers]

User applications might need to verify hardware configuration
of the MOCS entries. To facilitate this debug, add a new debugfs
entry to allow a dump of the MOCS state to verify expected values
are set by i915.

Signed-off-by: Stuart Summers <stuart.summers@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_mocs.c | 50 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gt/intel_mocs.h |  3 ++
 drivers/gpu/drm/i915/i915_debugfs.c  | 12 +++++++
 3 files changed, 65 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.c b/drivers/gpu/drm/i915/gt/intel_mocs.c
index 728704bbbe18..fea8ef2fd2aa 100644
--- a/drivers/gpu/drm/i915/gt/intel_mocs.c
+++ b/drivers/gpu/drm/i915/gt/intel_mocs.c
@@ -625,6 +625,56 @@ int intel_mocs_emit(struct i915_request *rq)
 	return 0;
 }
 
+static void
+intel_mocs_dump_l3cc_table(struct intel_gt *gt, struct drm_printer *p)
+{
+	struct intel_uncore *uncore = gt->uncore;
+	struct drm_i915_mocs_table table;
+	unsigned int i;
+
+	if (!get_mocs_settings(gt, &table))
+		return;
+
+	drm_printf(p, "l3cc:\n");
+
+	for (i = 0; i < table.n_entries / 2; i++) {
+		u32 reg = intel_uncore_read(uncore, GEN9_LNCFCMOCS(i));
+
+		drm_printf(p, "  MOCS[%d]: 0x%x\n", i * 2, reg & 0xffff);
+		drm_printf(p, "  MOCS[%d]: 0x%x\n", i * 2 + 1, reg >> 16);
+	}
+}
+
+static void
+intel_mocs_dump_global(struct intel_gt *gt, struct drm_printer *p)
+{
+	struct intel_uncore *uncore = gt->uncore;
+	struct drm_i915_mocs_table table;
+	unsigned int i;
+
+	GEM_BUG_ON(!HAS_GLOBAL_MOCS_REGISTERS(gt->i915));
+
+	if (!get_mocs_settings(gt, &table))
+		return;
+
+	if (GEM_DEBUG_WARN_ON(table.size > table.n_entries))
+		return;
+
+	drm_printf(p, "global:\n");
+
+	for (i = 0; i < table.n_entries; i++)
+		drm_printf(p, "  MOCS[%d]: 0x%x\n",
+			   i, intel_uncore_read(uncore, GEN12_GLOBAL_MOCS(i)));
+}
+
+void intel_mocs_show_info(struct intel_gt *gt, struct drm_printer *p)
+{
+	intel_mocs_dump_l3cc_table(gt, p);
+
+	if (HAS_GLOBAL_MOCS_REGISTERS(gt->i915))
+		intel_mocs_dump_global(gt, p);
+}
+
 void intel_mocs_init(struct intel_gt *gt)
 {
 	intel_mocs_init_l3cc_table(gt);
diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.h b/drivers/gpu/drm/i915/gt/intel_mocs.h
index 2ae816b7ca19..0ef95ce818d3 100644
--- a/drivers/gpu/drm/i915/gt/intel_mocs.h
+++ b/drivers/gpu/drm/i915/gt/intel_mocs.h
@@ -24,6 +24,8 @@
 #ifndef INTEL_MOCS_H
 #define INTEL_MOCS_H
 
+#include <drm/drm_print.h>
+
 /**
  * DOC: Memory Objects Control State (MOCS)
  *
@@ -55,6 +57,7 @@ struct intel_gt;
 
 void intel_mocs_init(struct intel_gt *gt);
 void intel_mocs_init_engine(struct intel_engine_cs *engine);
+void intel_mocs_show_info(struct intel_gt *gt, struct drm_printer *p);
 
 int intel_mocs_emit(struct i915_request *rq);
 
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 3b15266c54fd..1aa022eb2c3d 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -41,6 +41,7 @@
 
 #include "gem/i915_gem_context.h"
 #include "gt/intel_reset.h"
+#include "gt/intel_mocs.h"
 #include "gt/uc/intel_guc_submission.h"
 
 #include "i915_debugfs.h"
@@ -76,6 +77,16 @@ static int i915_capabilities(struct seq_file *m, void *data)
 	return 0;
 }
 
+static int show_mocs_info(struct seq_file *m, void *data)
+{
+	struct drm_i915_private *i915 = node_to_i915(m->private);
+	struct drm_printer p = drm_seq_file_printer(m);
+
+	intel_mocs_show_info(&i915->gt, &p);
+
+	return 0;
+}
+
 static char get_pin_flag(struct drm_i915_gem_object *obj)
 {
 	return obj->pin_global ? 'p' : ' ';
@@ -4352,6 +4363,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_sseu_status", i915_sseu_status, 0},
 	{"i915_drrs_status", i915_drrs_status, 0},
 	{"i915_rps_boost_info", i915_rps_boost_info, 0},
+	{"i915_mocs_info", show_mocs_info, 0},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
-- 
2.22.0

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

[PATCH 2/2] drm/i915: Return true by default in mocs settings

[Stuart Summers]

Reduce code by defaulting to true in the MOCS settings
initialization function. Set to false for unexpected
platforms.

Signed-off-by: Stuart Summers <stuart.summers@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_mocs.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.c b/drivers/gpu/drm/i915/gt/intel_mocs.c
index fea8ef2fd2aa..ffd105c53ff4 100644
--- a/drivers/gpu/drm/i915/gt/intel_mocs.c
+++ b/drivers/gpu/drm/i915/gt/intel_mocs.c
@@ -291,31 +291,28 @@ static bool get_mocs_settings(struct intel_gt *gt,
 			      struct drm_i915_mocs_table *table)
 {
 	struct drm_i915_private *i915 = gt->i915;
-	bool result = false;
+	bool result = true;
 
 	if (INTEL_GEN(i915) >= 12) {
 		table->size  = ARRAY_SIZE(tigerlake_mocs_table);
 		table->table = tigerlake_mocs_table;
 		table->n_entries = GEN11_NUM_MOCS_ENTRIES;
-		result = true;
 	} else if (IS_GEN(i915, 11)) {
 		table->size  = ARRAY_SIZE(icelake_mocs_table);
 		table->table = icelake_mocs_table;
 		table->n_entries = GEN11_NUM_MOCS_ENTRIES;
-		result = true;
 	} else if (IS_GEN9_BC(i915) || IS_CANNONLAKE(i915)) {
 		table->size  = ARRAY_SIZE(skylake_mocs_table);
 		table->n_entries = GEN9_NUM_MOCS_ENTRIES;
 		table->table = skylake_mocs_table;
-		result = true;
 	} else if (IS_GEN9_LP(i915)) {
 		table->size  = ARRAY_SIZE(broxton_mocs_table);
 		table->n_entries = GEN9_NUM_MOCS_ENTRIES;
 		table->table = broxton_mocs_table;
-		result = true;
 	} else {
 		WARN_ONCE(INTEL_GEN(i915) >= 9,
 			  "Platform that should have a MOCS table does not.\n");
+		result = false;
 	}
 
 	/* WaDisableSkipCaching:skl,bxt,kbl,glk */
-- 
2.22.0

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

Re: [PATCH 2/2] drm/i915: Return true by default in mocs settings

[Kumar Valsan, Prathap]

On Wed, Aug 07, 2019 at 01:55:56PM -0700, Stuart Summers wrote:
> Reduce code by defaulting to true in the MOCS settings
> initialization function. Set to false for unexpected
> platforms.
> 
> Signed-off-by: Stuart Summers <stuart.summers@intel.com>

Reviewed-by:Prathap Kumar Valsan <prathap.kumar.valsan@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_mocs.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.c b/drivers/gpu/drm/i915/gt/intel_mocs.c
> index fea8ef2fd2aa..ffd105c53ff4 100644
> --- a/drivers/gpu/drm/i915/gt/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_mocs.c
> @@ -291,31 +291,28 @@ static bool get_mocs_settings(struct intel_gt *gt,
>  			      struct drm_i915_mocs_table *table)
>  {
>  	struct drm_i915_private *i915 = gt->i915;
> -	bool result = false;
> +	bool result = true;
>  
>  	if (INTEL_GEN(i915) >= 12) {
>  		table->size  = ARRAY_SIZE(tigerlake_mocs_table);
>  		table->table = tigerlake_mocs_table;
>  		table->n_entries = GEN11_NUM_MOCS_ENTRIES;
> -		result = true;
>  	} else if (IS_GEN(i915, 11)) {
>  		table->size  = ARRAY_SIZE(icelake_mocs_table);
>  		table->table = icelake_mocs_table;
>  		table->n_entries = GEN11_NUM_MOCS_ENTRIES;
> -		result = true;
>  	} else if (IS_GEN9_BC(i915) || IS_CANNONLAKE(i915)) {
>  		table->size  = ARRAY_SIZE(skylake_mocs_table);
>  		table->n_entries = GEN9_NUM_MOCS_ENTRIES;
>  		table->table = skylake_mocs_table;
> -		result = true;
>  	} else if (IS_GEN9_LP(i915)) {
>  		table->size  = ARRAY_SIZE(broxton_mocs_table);
>  		table->n_entries = GEN9_NUM_MOCS_ENTRIES;
>  		table->table = broxton_mocs_table;
> -		result = true;
>  	} else {
>  		WARN_ONCE(INTEL_GEN(i915) >= 9,
>  			  "Platform that should have a MOCS table does not.\n");
> +		result = false;
>  	}
>  
>  	/* WaDisableSkipCaching:skl,bxt,kbl,glk */
> -- 
> 2.22.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

✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915: Add MOCS state dump to debugfs

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Add MOCS state dump to debugfs
URL   : https://patchwork.freedesktop.org/series/64868/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_6651 -> Patchwork_13908
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_13908 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_13908, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13908/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_13908:

### IGT changes ###

#### Possible regressions ####

  * igt@debugfs_test@read_all_entries:
    - fi-kbl-guc:         [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6651/fi-kbl-guc/igt@debugfs_test@read_all_entries.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13908/fi-kbl-guc/igt@debugfs_test@read_all_entries.html
    - fi-kbl-8809g:       [PASS][3] -> [DMESG-WARN][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6651/fi-kbl-8809g/igt@debugfs_test@read_all_entries.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13908/fi-kbl-8809g/igt@debugfs_test@read_all_entries.html

  * igt@runner@aborted:
    - fi-kbl-8809g:       NOTRUN -> [FAIL][5]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13908/fi-kbl-8809g/igt@runner@aborted.html
    - fi-kbl-guc:         NOTRUN -> [FAIL][6]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13908/fi-kbl-guc/igt@runner@aborted.html

  
Known issues
------------

  Here are the changes found in Patchwork_13908 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_mmap_gtt@basic-read:
    - fi-icl-u3:          [PASS][7] -> [DMESG-WARN][8] ([fdo#107724]) +1 similar issue
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6651/fi-icl-u3/igt@gem_mmap_gtt@basic-read.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13908/fi-icl-u3/igt@gem_mmap_gtt@basic-read.html

  * igt@i915_selftest@live_client:
    - fi-skl-guc:         [PASS][9] -> [DMESG-WARN][10] ([fdo#110943])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6651/fi-skl-guc/igt@i915_selftest@live_client.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13908/fi-skl-guc/igt@i915_selftest@live_client.html

  * igt@i915_selftest@live_reset:
    - fi-icl-u2:          [PASS][11] -> [INCOMPLETE][12] ([fdo#107713])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6651/fi-icl-u2/igt@i915_selftest@live_reset.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13908/fi-icl-u2/igt@i915_selftest@live_reset.html

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-kbl-7567u:       [PASS][13] -> [WARN][14] ([fdo#109380])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6651/fi-kbl-7567u/igt@kms_chamelium@common-hpd-after-suspend.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13908/fi-kbl-7567u/igt@kms_chamelium@common-hpd-after-suspend.html

  * igt@kms_pipe_crc_basic@read-crc-pipe-c:
    - fi-kbl-7567u:       [PASS][15] -> [SKIP][16] ([fdo#109271]) +23 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6651/fi-kbl-7567u/igt@kms_pipe_crc_basic@read-crc-pipe-c.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13908/fi-kbl-7567u/igt@kms_pipe_crc_basic@read-crc-pipe-c.html

  * igt@prime_vgem@basic-fence-flip:
    - fi-kbl-7500u:       [PASS][17] -> [SKIP][18] ([fdo#109271]) +23 similar issues
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6651/fi-kbl-7500u/igt@prime_vgem@basic-fence-flip.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13908/fi-kbl-7500u/igt@prime_vgem@basic-fence-flip.html

  
#### Possible fixes ####

  * igt@i915_selftest@live_execlists:
    - fi-skl-gvtdvm:      [DMESG-FAIL][19] ([fdo#111108]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6651/fi-skl-gvtdvm/igt@i915_selftest@live_execlists.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13908/fi-skl-gvtdvm/igt@i915_selftest@live_execlists.html

  * igt@kms_busy@basic-flip-a:
    - fi-kbl-7567u:       [SKIP][21] ([fdo#109271] / [fdo#109278]) -> [PASS][22] +2 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6651/fi-kbl-7567u/igt@kms_busy@basic-flip-a.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13908/fi-kbl-7567u/igt@kms_busy@basic-flip-a.html

  * igt@kms_busy@basic-flip-c:
    - fi-kbl-7500u:       [SKIP][23] ([fdo#109271] / [fdo#109278]) -> [PASS][24] +2 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6651/fi-kbl-7500u/igt@kms_busy@basic-flip-c.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13908/fi-kbl-7500u/igt@kms_busy@basic-flip-c.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109380]: https://bugs.freedesktop.org/show_bug.cgi?id=109380
  [fdo#110943]: https://bugs.freedesktop.org/show_bug.cgi?id=110943
  [fdo#111108]: https://bugs.freedesktop.org/show_bug.cgi?id=111108


Participating hosts (55 -> 46)
------------------------------

  Missing    (9): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-hsw-peppy fi-byt-squawks fi-bsw-cyan fi-icl-y fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_6651 -> Patchwork_13908

  CI-20190529: 20190529
  CI_DRM_6651: ab4a22e74ebc9f8bb2226a95d72c778da51126a2 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5125: 35d81d01b1599b4bc4df0e09e25f6f531eed4f8a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13908: 53c4082f753063b43c1b85be6d92e369e325f15e @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

53c4082f7530 drm/i915: Return true by default in mocs settings
0778b4cf421b drm/i915: Add MOCS state dump to debugfs

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13908/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Add MOCS state dump to debugfs

[Chris Wilson]

Quoting Stuart Summers (2019-08-07 21:55:55)
> User applications might need to verify hardware configuration
> of the MOCS entries. To facilitate this debug, add a new debugfs
> entry to allow a dump of the MOCS state to verify expected values
> are set by i915.

User applications + debugfs? It's not an avenue for ABI.

If you really want to provide the settings back to userspace, look at
something like an i915_query or sysfs.

Or if you just mean igt, then add a Testcase:

If you just need to validate that we are setting and restoring them,
selftests.

If you need them for debugging errors, add them to the error state.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Add MOCS state dump to debugfs

[Stuart Summers]

On Wed, 2019-08-07 at 22:29 +0100, Chris Wilson wrote:
> Quoting Stuart Summers (2019-08-07 21:55:55)
> > User applications might need to verify hardware configuration
> > of the MOCS entries. To facilitate this debug, add a new debugfs
> > entry to allow a dump of the MOCS state to verify expected values
> > are set by i915.
> 
> User applications + debugfs? It's not an avenue for ABI.
> 
> If you really want to provide the settings back to userspace, look at
> something like an i915_query or sysfs.
> 
> Or if you just mean igt, then add a Testcase:
> 
> If you just need to validate that we are setting and restoring them,
> selftests.
> 
> If you need them for debugging errors, add them to the error state.

This was probably poorly worded, you're right. I'll update the commit
message to be more specific.

I do want this for debugging, but not sure error state is the right
place. This is for debugging performance issues, so no specific
failures. If you feel sysfs or i915_query are more correct here, I can
look at adding this there instead. Is there a reason we don't want this
in debugfs specifically?

Thanks,
Stuart

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

Re: [PATCH 1/2] drm/i915: Add MOCS state dump to debugfs

[Chris Wilson]

Quoting Stuart Summers (2019-08-07 22:48:55)
> On Wed, 2019-08-07 at 22:29 +0100, Chris Wilson wrote:
> > Quoting Stuart Summers (2019-08-07 21:55:55)
> > > User applications might need to verify hardware configuration
> > > of the MOCS entries. To facilitate this debug, add a new debugfs
> > > entry to allow a dump of the MOCS state to verify expected values
> > > are set by i915.
> > 
> > User applications + debugfs? It's not an avenue for ABI.
> > 
> > If you really want to provide the settings back to userspace, look at
> > something like an i915_query or sysfs.
> > 
> > Or if you just mean igt, then add a Testcase:
> > 
> > If you just need to validate that we are setting and restoring them,
> > selftests.
> > 
> > If you need them for debugging errors, add them to the error state.
> 
> This was probably poorly worded, you're right. I'll update the commit
> message to be more specific.
> 
> I do want this for debugging, but not sure error state is the right
> place. This is for debugging performance issues, so no specific
> failures. If you feel sysfs or i915_query are more correct here, I can
> look at adding this there instead. Is there a reason we don't want this
> in debugfs specifically?

No, it was just the wording implied to me you had a use case for
clients, not just debugging the kernel.

Adding it to the error state (see i915_gpu_info) is not too bad an idea
if you need a sledgehammer to inspect the GPU state while a batch is
executing, but really it just sounds like you want to automate checking
the mocs registers against "ideal" state. They should be static, so once
they are set, so long as we are confident and check that they do not
change nor can be scribbled over by userspace, you only need to scan the
source :)

I will add that I wish we took a more complete snapshot of interesting
registers for the error state.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Add MOCS state dump to debugfs

[Stuart Summers]

On Wed, 2019-08-07 at 23:01 +0100, Chris Wilson wrote:
> Quoting Stuart Summers (2019-08-07 22:48:55)
> > On Wed, 2019-08-07 at 22:29 +0100, Chris Wilson wrote:
> > > Quoting Stuart Summers (2019-08-07 21:55:55)
> > > > User applications might need to verify hardware configuration
> > > > of the MOCS entries. To facilitate this debug, add a new
> > > > debugfs
> > > > entry to allow a dump of the MOCS state to verify expected
> > > > values
> > > > are set by i915.
> > > 
> > > User applications + debugfs? It's not an avenue for ABI.
> > > 
> > > If you really want to provide the settings back to userspace,
> > > look at
> > > something like an i915_query or sysfs.
> > > 
> > > Or if you just mean igt, then add a Testcase:
> > > 
> > > If you just need to validate that we are setting and restoring
> > > them,
> > > selftests.
> > > 
> > > If you need them for debugging errors, add them to the error
> > > state.
> > 
> > This was probably poorly worded, you're right. I'll update the
> > commit
> > message to be more specific.
> > 
> > I do want this for debugging, but not sure error state is the right
> > place. This is for debugging performance issues, so no specific
> > failures. If you feel sysfs or i915_query are more correct here, I
> > can
> > look at adding this there instead. Is there a reason we don't want
> > this
> > in debugfs specifically?
> 
> No, it was just the wording implied to me you had a use case for
> clients, not just debugging the kernel.
> 
> Adding it to the error state (see i915_gpu_info) is not too bad an
> idea
> if you need a sledgehammer to inspect the GPU state while a batch is
> executing, but really it just sounds like you want to automate
> checking
> the mocs registers against "ideal" state. They should be static, so
> once
> they are set, so long as we are confident and check that they do not
> change nor can be scribbled over by userspace, you only need to scan
> the
> source :)
> 
> I will add that I wish we took a more complete snapshot of
> interesting
> registers for the error state.

I guess my question is about intent of the error state. I can add it
there, but do we want this to indicate any register state we might want
to investigate, even if the registers are "correct", but just need
review based on current behavior?

Thanks,
Stuart

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

Re: [PATCH 1/2] drm/i915: Add MOCS state dump to debugfs

[Chris Wilson]

Quoting Stuart Summers (2019-08-08 00:00:17)
> On Wed, 2019-08-07 at 23:01 +0100, Chris Wilson wrote:
> > Quoting Stuart Summers (2019-08-07 22:48:55)
> > > On Wed, 2019-08-07 at 22:29 +0100, Chris Wilson wrote:
> > > > Quoting Stuart Summers (2019-08-07 21:55:55)
> > > > > User applications might need to verify hardware configuration
> > > > > of the MOCS entries. To facilitate this debug, add a new
> > > > > debugfs
> > > > > entry to allow a dump of the MOCS state to verify expected
> > > > > values
> > > > > are set by i915.
> > > > 
> > > > User applications + debugfs? It's not an avenue for ABI.
> > > > 
> > > > If you really want to provide the settings back to userspace,
> > > > look at
> > > > something like an i915_query or sysfs.
> > > > 
> > > > Or if you just mean igt, then add a Testcase:
> > > > 
> > > > If you just need to validate that we are setting and restoring
> > > > them,
> > > > selftests.
> > > > 
> > > > If you need them for debugging errors, add them to the error
> > > > state.
> > > 
> > > This was probably poorly worded, you're right. I'll update the
> > > commit
> > > message to be more specific.
> > > 
> > > I do want this for debugging, but not sure error state is the right
> > > place. This is for debugging performance issues, so no specific
> > > failures. If you feel sysfs or i915_query are more correct here, I
> > > can
> > > look at adding this there instead. Is there a reason we don't want
> > > this
> > > in debugfs specifically?
> > 
> > No, it was just the wording implied to me you had a use case for
> > clients, not just debugging the kernel.
> > 
> > Adding it to the error state (see i915_gpu_info) is not too bad an
> > idea
> > if you need a sledgehammer to inspect the GPU state while a batch is
> > executing, but really it just sounds like you want to automate
> > checking
> > the mocs registers against "ideal" state. They should be static, so
> > once
> > they are set, so long as we are confident and check that they do not
> > change nor can be scribbled over by userspace, you only need to scan
> > the
> > source :)
> > 
> > I will add that I wish we took a more complete snapshot of
> > interesting
> > registers for the error state.
> 
> I guess my question is about intent of the error state. I can add it
> there, but do we want this to indicate any register state we might want
> to investigate, even if the registers are "correct", but just need
> review based on current behavior?

It was created for debugging userspace batches (later added to hang
detection as a means of automatically grabbing the hopefully relevant
batch). As such it's a motley collection of information that at some
point proved useful. If you can make use of it, and find it more useful
to have the mocs registers in the same snapshot as the user batch,
please do include it. (Fwiw, I would like to extend the error state with
a bunch of { offset:0xfoo, value:0xbar } given a set of tables listing
the interesting regs. There just hasn't been an urgent need. Also on
that wishlist is devcoredump.)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Add MOCS state dump to debugfs

[Stuart Summers]

On Thu, 2019-08-08 at 00:12 +0100, Chris Wilson wrote:
> Quoting Stuart Summers (2019-08-08 00:00:17)
> > On Wed, 2019-08-07 at 23:01 +0100, Chris Wilson wrote:
> > > Quoting Stuart Summers (2019-08-07 22:48:55)
> > > > On Wed, 2019-08-07 at 22:29 +0100, Chris Wilson wrote:
> > > > > Quoting Stuart Summers (2019-08-07 21:55:55)
> > > > > > User applications might need to verify hardware
> > > > > > configuration
> > > > > > of the MOCS entries. To facilitate this debug, add a new
> > > > > > debugfs
> > > > > > entry to allow a dump of the MOCS state to verify expected
> > > > > > values
> > > > > > are set by i915.
> > > > > 
> > > > > User applications + debugfs? It's not an avenue for ABI.
> > > > > 
> > > > > If you really want to provide the settings back to userspace,
> > > > > look at
> > > > > something like an i915_query or sysfs.
> > > > > 
> > > > > Or if you just mean igt, then add a Testcase:
> > > > > 
> > > > > If you just need to validate that we are setting and
> > > > > restoring
> > > > > them,
> > > > > selftests.
> > > > > 
> > > > > If you need them for debugging errors, add them to the error
> > > > > state.
> > > > 
> > > > This was probably poorly worded, you're right. I'll update the
> > > > commit
> > > > message to be more specific.
> > > > 
> > > > I do want this for debugging, but not sure error state is the
> > > > right
> > > > place. This is for debugging performance issues, so no specific
> > > > failures. If you feel sysfs or i915_query are more correct
> > > > here, I
> > > > can
> > > > look at adding this there instead. Is there a reason we don't
> > > > want
> > > > this
> > > > in debugfs specifically?
> > > 
> > > No, it was just the wording implied to me you had a use case for
> > > clients, not just debugging the kernel.
> > > 
> > > Adding it to the error state (see i915_gpu_info) is not too bad
> > > an
> > > idea
> > > if you need a sledgehammer to inspect the GPU state while a batch
> > > is
> > > executing, but really it just sounds like you want to automate
> > > checking
> > > the mocs registers against "ideal" state. They should be static,
> > > so
> > > once
> > > they are set, so long as we are confident and check that they do
> > > not
> > > change nor can be scribbled over by userspace, you only need to
> > > scan
> > > the
> > > source :)
> > > 
> > > I will add that I wish we took a more complete snapshot of
> > > interesting
> > > registers for the error state.
> > 
> > I guess my question is about intent of the error state. I can add
> > it
> > there, but do we want this to indicate any register state we might
> > want
> > to investigate, even if the registers are "correct", but just need
> > review based on current behavior?
> 
> It was created for debugging userspace batches (later added to hang
> detection as a means of automatically grabbing the hopefully relevant
> batch). As such it's a motley collection of information that at some
> point proved useful. If you can make use of it, and find it more
> useful
> to have the mocs registers in the same snapshot as the user batch,
> please do include it. (Fwiw, I would like to extend the error state
> with
> a bunch of { offset:0xfoo, value:0xbar } given a set of tables
> listing
> the interesting regs. There just hasn't been an urgent need. Also on
> that wishlist is devcoredump.)

Ok perfect, thanks for the history here! I'll rework this into the
error state. If the intent is a catch-all where we can easily see the
state of the GPU at any given time, I agree having a large list of
registers we dump here for review would be really interesting.

Thanks,
Stuart

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

Re: [PATCH 1/2] drm/i915: Add MOCS state dump to debugfs

[Kumar Valsan, Prathap]

On Wed, Aug 07, 2019 at 01:55:55PM -0700, Stuart Summers wrote:
> User applications might need to verify hardware configuration
> of the MOCS entries. To facilitate this debug, add a new debugfs
> entry to allow a dump of the MOCS state to verify expected values
> are set by i915.
> 
> Signed-off-by: Stuart Summers <stuart.summers@intel.com>

Acked-by: Prathap Kumar Valsan <prathap.kumar.valsan@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_mocs.c | 50 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/gt/intel_mocs.h |  3 ++
>  drivers/gpu/drm/i915/i915_debugfs.c  | 12 +++++++
>  3 files changed, 65 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.c b/drivers/gpu/drm/i915/gt/intel_mocs.c
> index 728704bbbe18..fea8ef2fd2aa 100644
> --- a/drivers/gpu/drm/i915/gt/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_mocs.c
> @@ -625,6 +625,56 @@ int intel_mocs_emit(struct i915_request *rq)
>  	return 0;
>  }
>  
> +static void
> +intel_mocs_dump_l3cc_table(struct intel_gt *gt, struct drm_printer *p)
> +{
> +	struct intel_uncore *uncore = gt->uncore;
> +	struct drm_i915_mocs_table table;
> +	unsigned int i;
> +
> +	if (!get_mocs_settings(gt, &table))
> +		return;
> +
> +	drm_printf(p, "l3cc:\n");
> +
> +	for (i = 0; i < table.n_entries / 2; i++) {
> +		u32 reg = intel_uncore_read(uncore, GEN9_LNCFCMOCS(i));
> +
> +		drm_printf(p, "  MOCS[%d]: 0x%x\n", i * 2, reg & 0xffff);
> +		drm_printf(p, "  MOCS[%d]: 0x%x\n", i * 2 + 1, reg >> 16);
> +	}
> +}
> +
> +static void
> +intel_mocs_dump_global(struct intel_gt *gt, struct drm_printer *p)
> +{
> +	struct intel_uncore *uncore = gt->uncore;
> +	struct drm_i915_mocs_table table;
> +	unsigned int i;
> +
> +	GEM_BUG_ON(!HAS_GLOBAL_MOCS_REGISTERS(gt->i915));
> +
> +	if (!get_mocs_settings(gt, &table))
> +		return;
> +
> +	if (GEM_DEBUG_WARN_ON(table.size > table.n_entries))
> +		return;
> +
> +	drm_printf(p, "global:\n");
> +
> +	for (i = 0; i < table.n_entries; i++)
> +		drm_printf(p, "  MOCS[%d]: 0x%x\n",
> +			   i, intel_uncore_read(uncore, GEN12_GLOBAL_MOCS(i)));
> +}
> +
> +void intel_mocs_show_info(struct intel_gt *gt, struct drm_printer *p)
> +{
> +	intel_mocs_dump_l3cc_table(gt, p);
> +
> +	if (HAS_GLOBAL_MOCS_REGISTERS(gt->i915))
> +		intel_mocs_dump_global(gt, p);
> +}
> +
>  void intel_mocs_init(struct intel_gt *gt)
>  {
>  	intel_mocs_init_l3cc_table(gt);
> diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.h b/drivers/gpu/drm/i915/gt/intel_mocs.h
> index 2ae816b7ca19..0ef95ce818d3 100644
> --- a/drivers/gpu/drm/i915/gt/intel_mocs.h
> +++ b/drivers/gpu/drm/i915/gt/intel_mocs.h
> @@ -24,6 +24,8 @@
>  #ifndef INTEL_MOCS_H
>  #define INTEL_MOCS_H
>  
> +#include <drm/drm_print.h>
> +
>  /**
>   * DOC: Memory Objects Control State (MOCS)
>   *
> @@ -55,6 +57,7 @@ struct intel_gt;
>  
>  void intel_mocs_init(struct intel_gt *gt);
>  void intel_mocs_init_engine(struct intel_engine_cs *engine);
> +void intel_mocs_show_info(struct intel_gt *gt, struct drm_printer *p);
>  
>  int intel_mocs_emit(struct i915_request *rq);
>  
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 3b15266c54fd..1aa022eb2c3d 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -41,6 +41,7 @@
>  
>  #include "gem/i915_gem_context.h"
>  #include "gt/intel_reset.h"
> +#include "gt/intel_mocs.h"
>  #include "gt/uc/intel_guc_submission.h"
>  
>  #include "i915_debugfs.h"
> @@ -76,6 +77,16 @@ static int i915_capabilities(struct seq_file *m, void *data)
>  	return 0;
>  }
>  
> +static int show_mocs_info(struct seq_file *m, void *data)
> +{
> +	struct drm_i915_private *i915 = node_to_i915(m->private);
> +	struct drm_printer p = drm_seq_file_printer(m);
> +
> +	intel_mocs_show_info(&i915->gt, &p);
> +
> +	return 0;
> +}
> +
>  static char get_pin_flag(struct drm_i915_gem_object *obj)
>  {
>  	return obj->pin_global ? 'p' : ' ';
> @@ -4352,6 +4363,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
>  	{"i915_sseu_status", i915_sseu_status, 0},
>  	{"i915_drrs_status", i915_drrs_status, 0},
>  	{"i915_rps_boost_info", i915_rps_boost_info, 0},
> +	{"i915_mocs_info", show_mocs_info, 0},
>  };
>  #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
>  
> -- 
> 2.22.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