Re: [Intel-gfx] [PATCH] drm/i915: Use the engine name directly in the error_state file
On 09/01/18 13:37, Michel Thierry wrote:
On 09/01/18 12:46, Chris Wilson wrote:
Quoting Michal Wajdeczko (2018-01-09 20:39:09)
On Tue, 09 Jan 2018 20:33:55 +0100, Michel Thierry
wrote:
Instead of using local string names that we will have to keep
maintaining, use the engine->name directly.
Suggested-by: Michal Wajdeczko
Signed-off-by: Michel Thierry
Cc: Michal Wajdeczko
I'd a patch to do this, I just had to make sure the tooling was ready
for a name change. At the time, changing this string broke a few igt and
intel_error_decode.
I think your igt patch "lib: Fix up internal engine names (again)"
covered it.
Not sure if there are more tools looking at the engines' registers in
the error_state outside igt.
---
drivers/gpu/drm/i915/i915_gpu_error.c | 23 ++-
1 file changed, 10 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..db95ecacdace 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -34,16 +34,12 @@
#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 const char *engine_str(struct drm_i915_private *i915, int
engine_id)
+{
+ if (!i915->engine[engine_id])
+ return "";
While unlikely, empty string may be misleading, so maybe better we
return
"" or at least "?" here. Also maybe we can do as part of more
global helper function like
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_MAX_ENGINES)
return NULL;
return i915->engine[engine_id];
}
and then
static const char *engine_str(struct drm_i915_private *i915, int
engine_id)
{
return intel_engine_name(intel_engine_lookup(i915, engine_id));
}
No need, this is all internal, perma-allocated structs.
i915_gpu_info_open can pass engine_id=-1, better have some protection.
E.g. cat /sys/kernel/debug/dri/0/i915_gpu_info (while idle)
[ 84.448504] GEM_BUG_ON(engine_id == -1)
[ 84.448591] [ cut here ]
[ 84.448630] kernel BUG at drivers/gpu/drm/i915/i915_gpu_error.c:39!
[ 84.448673] invalid opcode: [#1] SMP
[ 84.448696] Modules linked in: rfcomm bnep nls_iso8859_1
...
[ 84.449425] Call Trace:
[ 84.449463] print_error_buffers+0x166/0x210 [i915]
[ 84.449510] i915_error_state_to_str+0x923/0x1210 [i915]
[ 84.449541] ? create_object+0x23f/0x2e0
[ 84.449584] ? i915_error_state_buf_init+0x59/0xc0 [i915]
[ 84.449613] ? rcu_read_lock_sched_held+0x49/0x90
[ 84.449644] ? kmalloc_order_trace+0x10d/0x170
[ 84.449685] gpu_state_read+0x56/0xb0 [i915]
[ 84.449712] full_proxy_read+0x4d/0x70
[ 84.449736] __vfs_read+0x23/0x120
[ 84.449759] vfs_read+0xa0/0x140
[ 84.449779] SyS_read+0x45/0xa0
[ 84.449801] entry_SYSCALL_64_fastpath+0x23/0x9a
[ 84.449834] RIP: 0033:0x7f2a2d3d4230
[ 84.449856] RSP: 002b:7ffda36960e8 EFLAGS: 0246
[ 84.449861] Code: 15 83 fe 04 7f 27 48 63 f6 48 8b 84 f7 08 46 00 00
48 83 c0 08 c3 48 c7 c6 e3 03 59 c0 48 c7 c7 f3 03 59 c0 31 c0 e8 66 75
b9 c6 <0f> 0b 48 c7 c6 05 04 59 c0 48 c7 c7 f3 03 59 c0 31 c0 e8 4f 75
[ 84.450028] RIP: engine_str+0x34/0x50 [i915] RSP: b7e142b53c98
[ 89.410577] ---[ end trace 5eacc9ca2b6d1e5d ]---
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Use the engine name directly in the error_state file
On 09/01/18 12:46, Chris Wilson wrote:
Quoting Michal Wajdeczko (2018-01-09 20:39:09)
On Tue, 09 Jan 2018 20:33:55 +0100, Michel Thierry
wrote:
Instead of using local string names that we will have to keep
maintaining, use the engine->name directly.
Suggested-by: Michal Wajdeczko
Signed-off-by: Michel Thierry
Cc: Michal Wajdeczko
I'd a patch to do this, I just had to make sure the tooling was ready
for a name change. At the time, changing this string broke a few igt and
intel_error_decode.
I think your igt patch "lib: Fix up internal engine names (again)"
covered it.
Not sure if there are more tools looking at the engines' registers in
the error_state outside igt.
---
drivers/gpu/drm/i915/i915_gpu_error.c | 23 ++-
1 file changed, 10 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..db95ecacdace 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -34,16 +34,12 @@
#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 const char *engine_str(struct drm_i915_private *i915, int
engine_id)
+{
+ if (!i915->engine[engine_id])
+ return "";
While unlikely, empty string may be misleading, so maybe better we return
"" or at least "?" here. Also maybe we can do as part of more
global helper function like
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_MAX_ENGINES)
return NULL;
return i915->engine[engine_id];
}
and then
static const char *engine_str(struct drm_i915_private *i915, int engine_id)
{
return intel_engine_name(intel_engine_lookup(i915, engine_id));
}
No need, this is all internal, perma-allocated structs.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Use the engine name directly in the error_state file
Quoting Michal Wajdeczko (2018-01-09 20:39:09)
> On Tue, 09 Jan 2018 20:33:55 +0100, Michel Thierry
> wrote:
>
> > Instead of using local string names that we will have to keep
> > maintaining, use the engine->name directly.
> >
> > Suggested-by: Michal Wajdeczko
> > Signed-off-by: Michel Thierry
> > Cc: Michal Wajdeczko
I'd a patch to do this, I just had to make sure the tooling was ready
for a name change. At the time, changing this string broke a few igt and
intel_error_decode.
> > ---
> > drivers/gpu/drm/i915/i915_gpu_error.c | 23 ++-
> > 1 file changed, 10 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..db95ecacdace 100644
> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > @@ -34,16 +34,12 @@
> > #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 const char *engine_str(struct drm_i915_private *i915, int
> > engine_id)
> > +{
> > + if (!i915->engine[engine_id])
> > + return "";
>
> While unlikely, empty string may be misleading, so maybe better we return
> "" or at least "?" here. Also maybe we can do as part of more
> global helper function like
>
> 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_MAX_ENGINES)
> return NULL;
> return i915->engine[engine_id];
> }
>
> and then
>
> static const char *engine_str(struct drm_i915_private *i915, int engine_id)
> {
> return intel_engine_name(intel_engine_lookup(i915, engine_id));
> }
No need, this is all internal, perma-allocated structs.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Use the engine name directly in the error_state file
On Tue, 09 Jan 2018 20:33:55 +0100, Michel Thierry
wrote:
Instead of using local string names that we will have to keep
maintaining, use the engine->name directly.
Suggested-by: Michal Wajdeczko
Signed-off-by: Michel Thierry
Cc: Michal Wajdeczko
---
drivers/gpu/drm/i915/i915_gpu_error.c | 23 ++-
1 file changed, 10 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..db95ecacdace 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -34,16 +34,12 @@
#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 const char *engine_str(struct drm_i915_private *i915, int
engine_id)
+{
+ if (!i915->engine[engine_id])
+ return "";
While unlikely, empty string may be misleading, so maybe better we return
"" or at least "?" here. Also maybe we can do as part of more
global helper function like
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_MAX_ENGINES)
return NULL;
return i915->engine[engine_id];
}
and then
static const char *engine_str(struct drm_i915_private *i915, int engine_id)
{
return intel_engine_name(intel_engine_lookup(i915, engine_id));
}
+ else
+ return i915->engine[engine_id]->name;
}
static const char *tiling_flag(int tiling)
@@ -345,7 +341,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 ? " " : "");
Hmm, it looks that -1 is allowed/expected as engine_id so proposed
above check in lookup function is mandatory
- 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 +413,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 +630,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
[Intel-gfx] [PATCH] drm/i915: Use the engine name directly in the error_state file
Instead of using local string names that we will have to keep
maintaining, use the engine->name directly.
Suggested-by: Michal Wajdeczko
Signed-off-by: Michel Thierry
Cc: Michal Wajdeczko
---
drivers/gpu/drm/i915/i915_gpu_error.c | 23 ++-
1 file changed, 10 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..db95ecacdace 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -34,16 +34,12 @@
#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 const char *engine_str(struct drm_i915_private *i915, int engine_id)
+{
+ if (!i915->engine[engine_id])
+ return "";
+ else
+ return i915->engine[engine_id]->name;
}
static const char *tiling_flag(int tiling)
@@ -345,7 +341,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 +413,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 +630,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
