* [PATCH v2] drm/i915: Use the engine name directly in the error_state file
@ 2018-01-10 1:21 Michel Thierry
2018-01-10 1:42 ` ✓ Fi.CI.BAT: success for drm/i915: Use the engine name directly in the error_state file (rev2) Patchwork
` (4 more replies)
0 siblings, 5 replies; 10+ messages in thread
From: Michel Thierry @ 2018-01-10 1:21 UTC (permalink / raw)
To: intel-gfx
Instead of using local string names that we will have to keep
maintaining, use the engine->name directly.
v2: Better invalid engine_id handling, capture_bo will not be able know
the engine_id and end up with -1 (Michal).
Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_gpu_error.c | 33 ++++++++++++++++++++-------------
1 file changed, 20 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 94499c24f279..422e302161e5 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -34,16 +34,22 @@
#include "i915_drv.h"
-static const char *engine_str(int engine)
-{
- switch (engine) {
- case RCS: return "render";
- case VCS: return "bsd";
- case BCS: return "blt";
- case VECS: return "vebox";
- case VCS2: return "bsd2";
- default: return "";
- }
+static inline const char *intel_engine_name(struct intel_engine_cs *engine)
+{
+ return engine ? engine->name : "";
+}
+
+static inline struct intel_engine_cs *
+intel_engine_lookup(struct drm_i915_private *i915, int engine_id)
+{
+ if (engine_id < 0 || engine_id >= I915_NUM_ENGINES)
+ return NULL;
+ return i915->engine[engine_id];
+}
+
+static const char *engine_str(struct drm_i915_private *i915, int engine_id)
+{
+ return intel_engine_name(intel_engine_lookup(i915, engine_id));
}
static const char *tiling_flag(int tiling)
@@ -345,7 +351,7 @@ static void print_error_buffers(struct drm_i915_error_state_buf *m,
err_puts(m, purgeable_flag(err->purgeable));
err_puts(m, err->userptr ? " userptr" : "");
err_puts(m, err->engine != -1 ? " " : "");
- err_puts(m, engine_str(err->engine));
+ err_puts(m, engine_str(m->i915, err->engine));
err_puts(m, i915_cache_level_str(m->i915, err->cache_level));
if (err->name)
@@ -417,7 +423,8 @@ static void error_print_engine(struct drm_i915_error_state_buf *m,
{
int n;
- err_printf(m, "%s command stream:\n", engine_str(ee->engine_id));
+ err_printf(m, "%s command stream:\n", engine_str(m->i915,
+ ee->engine_id));
err_printf(m, " IDLE?: %s\n", yesno(ee->idle));
err_printf(m, " START: 0x%08x\n", ee->start);
err_printf(m, " HEAD: 0x%08x [0x%08x]\n", ee->head, ee->rq_head);
@@ -633,7 +640,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
if (error->engine[i].hangcheck_stalled &&
error->engine[i].context.pid) {
err_printf(m, "Active process (on ring %s): %s [%d], score %d\n",
- engine_str(i),
+ engine_str(m->i915, i),
error->engine[i].context.comm,
error->engine[i].context.pid,
error->engine[i].context.ban_score);
--
2.15.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Use the engine name directly in the error_state file (rev2)
2018-01-10 1:21 [PATCH v2] drm/i915: Use the engine name directly in the error_state file Michel Thierry
@ 2018-01-10 1:42 ` Patchwork
2018-01-10 2:33 ` ✓ Fi.CI.IGT: " Patchwork
` (3 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2018-01-10 1:42 UTC (permalink / raw)
To: Michel Thierry; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Use the engine name directly in the error_state file (rev2)
URL : https://patchwork.freedesktop.org/series/36215/
State : success
== Summary ==
Series 36215v2 drm/i915: Use the engine name directly in the error_state file
https://patchwork.freedesktop.org/api/1.0/series/36215/revisions/2/mbox/
Test debugfs_test:
Subgroup read_all_entries:
dmesg-fail -> DMESG-WARN (fi-elk-e7500) fdo#103989 +2
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-c:
fail -> PASS (fi-skl-6700k2) fdo#104108
fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:420s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:426s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:372s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:482s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:281s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:486s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:488s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:469s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:456s
fi-elk-e7500 total:229 pass:172 dwarn:10 dfail:0 fail:0 skip:46
fi-gdg-551 total:288 pass:179 dwarn:0 dfail:0 fail:1 skip:108 time:277s
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:513s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:390s
fi-hsw-4770r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:408s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:411s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:462s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:411s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:466s
fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:497s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:452s
fi-kbl-r total:288 pass:260 dwarn:1 dfail:0 fail:0 skip:27 time:504s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:427s
fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:509s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:530s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:491s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:472s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:430s
fi-snb-2520m total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:536s
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:396s
Blacklisted hosts:
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:572s
fi-cnl-y2 total:288 pass:258 dwarn:3 dfail:0 fail:0 skip:27 time:538s
fi-glk-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:470s
fi-pnv-d510 failed to collect. IGT log at Patchwork_7635/fi-pnv-d510/igt.log
c6410238e3c4f1282ef9709b65f553a555cf43eb drm-tip: 2018y-01m-09d-18h-59m-27s UTC integration manifest
dd4b445e4fc2 drm/i915: Use the engine name directly in the error_state file
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7635/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* ✓ Fi.CI.IGT: success for drm/i915: Use the engine name directly in the error_state file (rev2)
2018-01-10 1:21 [PATCH v2] drm/i915: Use the engine name directly in the error_state file Michel Thierry
2018-01-10 1:42 ` ✓ Fi.CI.BAT: success for drm/i915: Use the engine name directly in the error_state file (rev2) Patchwork
@ 2018-01-10 2:33 ` Patchwork
2018-01-12 20:47 ` [PATCH v2] drm/i915: Use the engine name directly in the error_state file Michel Thierry
` (2 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2018-01-10 2:33 UTC (permalink / raw)
To: Michel Thierry; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Use the engine name directly in the error_state file (rev2)
URL : https://patchwork.freedesktop.org/series/36215/
State : success
== Summary ==
Test kms_flip:
Subgroup flip-vs-absolute-wf_vblank-interruptible:
fail -> PASS (shard-hsw) fdo#100368
Test drv_suspend:
Subgroup fence-restore-tiled2untiled:
pass -> SKIP (shard-snb) fdo#104218
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#104218 https://bugs.freedesktop.org/show_bug.cgi?id=104218
shard-hsw total:2576 pass:1463 dwarn:1 dfail:0 fail:9 skip:1101 time:8291s
shard-snb total:2576 pass:1243 dwarn:1 dfail:0 fail:11 skip:1319 time:7159s
Blacklisted hosts:
shard-apl total:2554 pass:1572 dwarn:1 dfail:0 fail:26 skip:951 time:11952s
shard-kbl total:2558 pass:1695 dwarn:1 dfail:0 fail:24 skip:835 time:9378s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7635/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] drm/i915: Use the engine name directly in the error_state file
2018-01-10 1:21 [PATCH v2] drm/i915: Use the engine name directly in the error_state file Michel Thierry
2018-01-10 1:42 ` ✓ Fi.CI.BAT: success for drm/i915: Use the engine name directly in the error_state file (rev2) Patchwork
2018-01-10 2:33 ` ✓ Fi.CI.IGT: " Patchwork
@ 2018-01-12 20:47 ` Michel Thierry
2018-01-15 17:15 ` Tvrtko Ursulin
2018-01-18 18:17 ` Chris Wilson
4 siblings, 0 replies; 10+ messages in thread
From: Michel Thierry @ 2018-01-12 20:47 UTC (permalink / raw)
To: intel-gfx, Chris Wilson, Michal Wajdeczko
On 1/9/2018 5:21 PM, Michel Thierry wrote:
> Instead of using local string names that we will have to keep
> maintaining, use the engine->name directly.
>
> v2: Better invalid engine_id handling, capture_bo will not be able know
> the engine_id and end up with -1 (Michal).
>
Hi,
Fi.CI.IGT didn't catch any failure, apart from intel_error_decode (which
didn't complain), I'm not sure which other tool could be using these names.
And now that there are icl patches, this will help there too.
> Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_gpu_error.c | 33 ++++++++++++++++++++-------------
> 1 file changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 94499c24f279..422e302161e5 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -34,16 +34,22 @@
>
> #include "i915_drv.h"
>
> -static const char *engine_str(int engine)
> -{
> - switch (engine) {
> - case RCS: return "render";
> - case VCS: return "bsd";
> - case BCS: return "blt";
> - case VECS: return "vebox";
> - case VCS2: return "bsd2";
> - default: return "";
> - }
> +static inline const char *intel_engine_name(struct intel_engine_cs *engine)
> +{
> + return engine ? engine->name : "";
> +}
> +
> +static inline struct intel_engine_cs *
> +intel_engine_lookup(struct drm_i915_private *i915, int engine_id)
> +{
> + if (engine_id < 0 || engine_id >= I915_NUM_ENGINES)
> + return NULL;
> + return i915->engine[engine_id];
> +}
> +
> +static const char *engine_str(struct drm_i915_private *i915, int engine_id)
> +{
> + return intel_engine_name(intel_engine_lookup(i915, engine_id));
> }
>
> static const char *tiling_flag(int tiling)
> @@ -345,7 +351,7 @@ static void print_error_buffers(struct drm_i915_error_state_buf *m,
> err_puts(m, purgeable_flag(err->purgeable));
> err_puts(m, err->userptr ? " userptr" : "");
> err_puts(m, err->engine != -1 ? " " : "");
> - err_puts(m, engine_str(err->engine));
> + err_puts(m, engine_str(m->i915, err->engine));
> err_puts(m, i915_cache_level_str(m->i915, err->cache_level));
>
> if (err->name)
> @@ -417,7 +423,8 @@ static void error_print_engine(struct drm_i915_error_state_buf *m,
> {
> int n;
>
> - err_printf(m, "%s command stream:\n", engine_str(ee->engine_id));
> + err_printf(m, "%s command stream:\n", engine_str(m->i915,
> + ee->engine_id));
> err_printf(m, " IDLE?: %s\n", yesno(ee->idle));
> err_printf(m, " START: 0x%08x\n", ee->start);
> err_printf(m, " HEAD: 0x%08x [0x%08x]\n", ee->head, ee->rq_head);
> @@ -633,7 +640,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
> if (error->engine[i].hangcheck_stalled &&
> error->engine[i].context.pid) {
> err_printf(m, "Active process (on ring %s): %s [%d], score %d\n",
> - engine_str(i),
> + engine_str(m->i915, i),
> error->engine[i].context.comm,
> error->engine[i].context.pid,
> error->engine[i].context.ban_score);
> --
> 2.15.1
>
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] drm/i915: Use the engine name directly in the error_state file
2018-01-10 1:21 [PATCH v2] drm/i915: Use the engine name directly in the error_state file Michel Thierry
` (2 preceding siblings ...)
2018-01-12 20:47 ` [PATCH v2] drm/i915: Use the engine name directly in the error_state file Michel Thierry
@ 2018-01-15 17:15 ` Tvrtko Ursulin
2018-01-16 18:33 ` Michel Thierry
2018-01-18 18:17 ` Chris Wilson
4 siblings, 1 reply; 10+ messages in thread
From: Tvrtko Ursulin @ 2018-01-15 17:15 UTC (permalink / raw)
To: Michel Thierry, intel-gfx
On 10/01/2018 01:21, Michel Thierry wrote:
> Instead of using local string names that we will have to keep
> maintaining, use the engine->name directly.
>
> v2: Better invalid engine_id handling, capture_bo will not be able know
> the engine_id and end up with -1 (Michal).
>
> Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_gpu_error.c | 33 ++++++++++++++++++++-------------
> 1 file changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 94499c24f279..422e302161e5 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -34,16 +34,22 @@
>
> #include "i915_drv.h"
>
> -static const char *engine_str(int engine)
> -{
> - switch (engine) {
> - case RCS: return "render";
> - case VCS: return "bsd";
> - case BCS: return "blt";
> - case VECS: return "vebox";
> - case VCS2: return "bsd2";
> - default: return "";
> - }
> +static inline const char *intel_engine_name(struct intel_engine_cs *engine)
> +{
> + return engine ? engine->name : "";
> +}
> +
> +static inline struct intel_engine_cs *
> +intel_engine_lookup(struct drm_i915_private *i915, int engine_id)
> +{
> + if (engine_id < 0 || engine_id >= I915_NUM_ENGINES)
> + return NULL;
> + return i915->engine[engine_id];
> +}
> +
> +static const char *engine_str(struct drm_i915_private *i915, int engine_id)
> +{
> + return intel_engine_name(intel_engine_lookup(i915, engine_id));
> }
Feels like a bit of an overkill to have three functions to this trivial
thing but meh. Could also maybe cheat and have engine_id as unsigned int
and so would only need to check for >= I915_NUM_ENGINES.
Anyway, I peeked in intel_error_decode source and couldn't spot anything
that looked it would break. You checked it by running it? Assuming you did:
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
>
> static const char *tiling_flag(int tiling)
> @@ -345,7 +351,7 @@ static void print_error_buffers(struct drm_i915_error_state_buf *m,
> err_puts(m, purgeable_flag(err->purgeable));
> err_puts(m, err->userptr ? " userptr" : "");
> err_puts(m, err->engine != -1 ? " " : "");
> - err_puts(m, engine_str(err->engine));
> + err_puts(m, engine_str(m->i915, err->engine));
> err_puts(m, i915_cache_level_str(m->i915, err->cache_level));
>
> if (err->name)
> @@ -417,7 +423,8 @@ static void error_print_engine(struct drm_i915_error_state_buf *m,
> {
> int n;
>
> - err_printf(m, "%s command stream:\n", engine_str(ee->engine_id));
> + err_printf(m, "%s command stream:\n", engine_str(m->i915,
> + ee->engine_id));
> err_printf(m, " IDLE?: %s\n", yesno(ee->idle));
> err_printf(m, " START: 0x%08x\n", ee->start);
> err_printf(m, " HEAD: 0x%08x [0x%08x]\n", ee->head, ee->rq_head);
> @@ -633,7 +640,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
> if (error->engine[i].hangcheck_stalled &&
> error->engine[i].context.pid) {
> err_printf(m, "Active process (on ring %s): %s [%d], score %d\n",
> - engine_str(i),
> + engine_str(m->i915, i),
> error->engine[i].context.comm,
> error->engine[i].context.pid,
> error->engine[i].context.ban_score);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] drm/i915: Use the engine name directly in the error_state file
2018-01-15 17:15 ` Tvrtko Ursulin
@ 2018-01-16 18:33 ` Michel Thierry
2018-01-17 15:15 ` Chris Wilson
0 siblings, 1 reply; 10+ messages in thread
From: Michel Thierry @ 2018-01-16 18:33 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
On 1/15/2018 9:15 AM, Tvrtko Ursulin wrote:
>
> On 10/01/2018 01:21, Michel Thierry wrote:
>> Instead of using local string names that we will have to keep
>> maintaining, use the engine->name directly.
>>
>> v2: Better invalid engine_id handling, capture_bo will not be able know
>> the engine_id and end up with -1 (Michal).
>>
>> Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>> drivers/gpu/drm/i915/i915_gpu_error.c | 33
>> ++++++++++++++++++++-------------
>> 1 file changed, 20 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
>> b/drivers/gpu/drm/i915/i915_gpu_error.c
>> index 94499c24f279..422e302161e5 100644
>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
>> @@ -34,16 +34,22 @@
>> #include "i915_drv.h"
>> -static const char *engine_str(int engine)
>> -{
>> - switch (engine) {
>> - case RCS: return "render";
>> - case VCS: return "bsd";
>> - case BCS: return "blt";
>> - case VECS: return "vebox";
>> - case VCS2: return "bsd2";
>> - default: return "";
>> - }
>> +static inline const char *intel_engine_name(struct intel_engine_cs
>> *engine)
>> +{
>> + return engine ? engine->name : "";
>> +}
>> +
>> +static inline struct intel_engine_cs *
>> +intel_engine_lookup(struct drm_i915_private *i915, int engine_id)
>> +{
>> + if (engine_id < 0 || engine_id >= I915_NUM_ENGINES)
>> + return NULL;
>> + return i915->engine[engine_id];
>> +}
>> +
>> +static const char *engine_str(struct drm_i915_private *i915, int
>> engine_id)
>> +{
>> + return intel_engine_name(intel_engine_lookup(i915, engine_id));
>> }
>
> Feels like a bit of an overkill to have three functions to this trivial
> thing but meh. Could also maybe cheat and have engine_id as unsigned int
> and so would only need to check for >= I915_NUM_ENGINES.
>
> Anyway, I peeked in intel_error_decode source and couldn't spot anything
> that looked it would break. You checked it by running it? Assuming you did:
>
Thanks. Yes, I did and also compared the error state output
(intel_error_decode also still works). The only change is that the batch
/ ring / HWSP sections now use the engine->name (e.g. they will use rcs0
instead of render, vcs0 instead of bsd, etc.).
I didn't see anything in IGT complaining (probably because IGT usually
reads dmesg and there we already use engine->name), but there must be
something else out there that will complain.
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Regards,
>
> Tvrtko
>
>> static const char *tiling_flag(int tiling)
>> @@ -345,7 +351,7 @@ static void print_error_buffers(struct
>> drm_i915_error_state_buf *m,
>> err_puts(m, purgeable_flag(err->purgeable));
>> err_puts(m, err->userptr ? " userptr" : "");
>> err_puts(m, err->engine != -1 ? " " : "");
>> - err_puts(m, engine_str(err->engine));
>> + err_puts(m, engine_str(m->i915, err->engine));
>> err_puts(m, i915_cache_level_str(m->i915, err->cache_level));
>> if (err->name)
>> @@ -417,7 +423,8 @@ static void error_print_engine(struct
>> drm_i915_error_state_buf *m,
>> {
>> int n;
>> - err_printf(m, "%s command stream:\n", engine_str(ee->engine_id));
>> + err_printf(m, "%s command stream:\n", engine_str(m->i915,
>> + ee->engine_id));
>> err_printf(m, " IDLE?: %s\n", yesno(ee->idle));
>> err_printf(m, " START: 0x%08x\n", ee->start);
>> err_printf(m, " HEAD: 0x%08x [0x%08x]\n", ee->head, ee->rq_head);
>> @@ -633,7 +640,7 @@ int i915_error_state_to_str(struct
>> drm_i915_error_state_buf *m,
>> if (error->engine[i].hangcheck_stalled &&
>> error->engine[i].context.pid) {
>> err_printf(m, "Active process (on ring %s): %s [%d],
>> score %d\n",
>> - engine_str(i),
>> + engine_str(m->i915, i),
>> error->engine[i].context.comm,
>> error->engine[i].context.pid,
>> error->engine[i].context.ban_score);
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] drm/i915: Use the engine name directly in the error_state file
2018-01-16 18:33 ` Michel Thierry
@ 2018-01-17 15:15 ` Chris Wilson
2018-01-18 0:39 ` Michel Thierry
0 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2018-01-17 15:15 UTC (permalink / raw)
To: Michel Thierry, Tvrtko Ursulin, intel-gfx
Quoting Michel Thierry (2018-01-16 18:33:32)
>
>
> On 1/15/2018 9:15 AM, Tvrtko Ursulin wrote:
> >
> > On 10/01/2018 01:21, Michel Thierry wrote:
> >> Instead of using local string names that we will have to keep
> >> maintaining, use the engine->name directly.
> >>
> >> v2: Better invalid engine_id handling, capture_bo will not be able know
> >> the engine_id and end up with -1 (Michal).
> >>
> >> Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> ---
> >> drivers/gpu/drm/i915/i915_gpu_error.c | 33
> >> ++++++++++++++++++++-------------
> >> 1 file changed, 20 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> >> b/drivers/gpu/drm/i915/i915_gpu_error.c
> >> index 94499c24f279..422e302161e5 100644
> >> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> >> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> >> @@ -34,16 +34,22 @@
> >> #include "i915_drv.h"
> >> -static const char *engine_str(int engine)
> >> -{
> >> - switch (engine) {
> >> - case RCS: return "render";
> >> - case VCS: return "bsd";
> >> - case BCS: return "blt";
> >> - case VECS: return "vebox";
> >> - case VCS2: return "bsd2";
> >> - default: return "";
> >> - }
> >> +static inline const char *intel_engine_name(struct intel_engine_cs
> >> *engine)
> >> +{
> >> + return engine ? engine->name : "";
> >> +}
> >> +
> >> +static inline struct intel_engine_cs *
> >> +intel_engine_lookup(struct drm_i915_private *i915, int engine_id)
> >> +{
> >> + if (engine_id < 0 || engine_id >= I915_NUM_ENGINES)
> >> + return NULL;
> >> + return i915->engine[engine_id];
> >> +}
> >> +
> >> +static const char *engine_str(struct drm_i915_private *i915, int
> >> engine_id)
> >> +{
> >> + return intel_engine_name(intel_engine_lookup(i915, engine_id));
> >> }
> >
> > Feels like a bit of an overkill to have three functions to this trivial
> > thing but meh. Could also maybe cheat and have engine_id as unsigned int
> > and so would only need to check for >= I915_NUM_ENGINES.
Overkill? Yes! If you are concerned about the back mapping, just put a
pointer back to the engine (ee->engine). We are not going to reallocate
it after module load, right? RIGHT?!!! :)
> > Anyway, I peeked in intel_error_decode source and couldn't spot anything
> > that looked it would break. You checked it by running it? Assuming you did:
> >
>
> Thanks. Yes, I did and also compared the error state output
> (intel_error_decode also still works). The only change is that the batch
> / ring / HWSP sections now use the engine->name (e.g. they will use rcs0
> instead of render, vcs0 instead of bsd, etc.).
>
> I didn't see anything in IGT complaining (probably because IGT usually
> reads dmesg and there we already use engine->name), but there must be
> something else out there that will complain.
Hmm, we will need to fixup mesa/aubinator_error_decode to expect the new
name, as it is using them to map various registers from the error state.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] drm/i915: Use the engine name directly in the error_state file
2018-01-17 15:15 ` Chris Wilson
@ 2018-01-18 0:39 ` Michel Thierry
2018-01-18 11:26 ` Chris Wilson
0 siblings, 1 reply; 10+ messages in thread
From: Michel Thierry @ 2018-01-18 0:39 UTC (permalink / raw)
To: Chris Wilson, Tvrtko Ursulin, intel-gfx
On 17/01/18 07:15, Chris Wilson wrote:
> Quoting Michel Thierry (2018-01-16 18:33:32)
>>
>>
>> On 1/15/2018 9:15 AM, Tvrtko Ursulin wrote:
>>>
>>> On 10/01/2018 01:21, Michel Thierry wrote:
>>>> Instead of using local string names that we will have to keep
>>>> maintaining, use the engine->name directly.
>>>>
>>>> v2: Better invalid engine_id handling, capture_bo will not be able know
>>>> the engine_id and end up with -1 (Michal).
>>>>
>>>> Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
>>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>> ---
>>>> drivers/gpu/drm/i915/i915_gpu_error.c | 33
>>>> ++++++++++++++++++++-------------
>>>> 1 file changed, 20 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
>>>> b/drivers/gpu/drm/i915/i915_gpu_error.c
>>>> index 94499c24f279..422e302161e5 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
>>>> @@ -34,16 +34,22 @@
>>>> #include "i915_drv.h"
>>>> -static const char *engine_str(int engine)
>>>> -{
>>>> - switch (engine) {
>>>> - case RCS: return "render";
>>>> - case VCS: return "bsd";
>>>> - case BCS: return "blt";
>>>> - case VECS: return "vebox";
>>>> - case VCS2: return "bsd2";
>>>> - default: return "";
>>>> - }
>>>> +static inline const char *intel_engine_name(struct intel_engine_cs
>>>> *engine)
>>>> +{
>>>> + return engine ? engine->name : "";
>>>> +}
>>>> +
>>>> +static inline struct intel_engine_cs *
>>>> +intel_engine_lookup(struct drm_i915_private *i915, int engine_id)
>>>> +{
>>>> + if (engine_id < 0 || engine_id >= I915_NUM_ENGINES)
>>>> + return NULL;
>>>> + return i915->engine[engine_id];
>>>> +}
>>>> +
>>>> +static const char *engine_str(struct drm_i915_private *i915, int
>>>> engine_id)
>>>> +{
>>>> + return intel_engine_name(intel_engine_lookup(i915, engine_id));
>>>> }
>>>
>>> Feels like a bit of an overkill to have three functions to this trivial
>>> thing but meh. Could also maybe cheat and have engine_id as unsigned int
>>> and so would only need to check for >= I915_NUM_ENGINES.
>
> Overkill? Yes! If you are concerned about the back mapping, just put a
> pointer back to the engine (ee->engine). We are not going to reallocate
> it after module load, right? RIGHT?!!! :)
>
>>> Anyway, I peeked in intel_error_decode source and couldn't spot anything
>>> that looked it would break. You checked it by running it? Assuming you did:
>>>
>>
>> Thanks. Yes, I did and also compared the error state output
>> (intel_error_decode also still works). The only change is that the batch
>> / ring / HWSP sections now use the engine->name (e.g. they will use rcs0
>> instead of render, vcs0 instead of bsd, etc.).
>>
>> I didn't see anything in IGT complaining (probably because IGT usually
>> reads dmesg and there we already use engine->name), but there must be
>> something else out there that will complain.
>
> Hmm, we will need to fixup mesa/aubinator_error_decode to expect the new
> name, as it is using them to map various registers from the error state.
Thanks for taking care of aubinator, hopefully next time I won't forget
about mesa.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] drm/i915: Use the engine name directly in the error_state file
2018-01-18 0:39 ` Michel Thierry
@ 2018-01-18 11:26 ` Chris Wilson
0 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2018-01-18 11:26 UTC (permalink / raw)
To: Michel Thierry, Tvrtko Ursulin, intel-gfx
Quoting Michel Thierry (2018-01-18 00:39:55)
> On 17/01/18 07:15, Chris Wilson wrote:
> > Hmm, we will need to fixup mesa/aubinator_error_decode to expect the new
> > name, as it is using them to map various registers from the error state.
>
> Thanks for taking care of aubinator, hopefully next time I won't forget
> about mesa.
Would be good if you could review my string handling there to make sure
it does match. :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] drm/i915: Use the engine name directly in the error_state file
2018-01-10 1:21 [PATCH v2] drm/i915: Use the engine name directly in the error_state file Michel Thierry
` (3 preceding siblings ...)
2018-01-15 17:15 ` Tvrtko Ursulin
@ 2018-01-18 18:17 ` Chris Wilson
4 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2018-01-18 18:17 UTC (permalink / raw)
To: Michel Thierry, intel-gfx
Quoting Michel Thierry (2018-01-10 01:21:51)
> Instead of using local string names that we will have to keep
> maintaining, use the engine->name directly.
>
> v2: Better invalid engine_id handling, capture_bo will not be able know
> the engine_id and end up with -1 (Michal).
>
> Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
Massaged the patch just ever so slightly and applied. Thanks for the
patch and review,
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-01-18 18:17 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-10 1:21 [PATCH v2] drm/i915: Use the engine name directly in the error_state file Michel Thierry
2018-01-10 1:42 ` ✓ Fi.CI.BAT: success for drm/i915: Use the engine name directly in the error_state file (rev2) Patchwork
2018-01-10 2:33 ` ✓ Fi.CI.IGT: " Patchwork
2018-01-12 20:47 ` [PATCH v2] drm/i915: Use the engine name directly in the error_state file Michel Thierry
2018-01-15 17:15 ` Tvrtko Ursulin
2018-01-16 18:33 ` Michel Thierry
2018-01-17 15:15 ` Chris Wilson
2018-01-18 0:39 ` Michel Thierry
2018-01-18 11:26 ` Chris Wilson
2018-01-18 18:17 ` Chris Wilson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.