drm/nouveau: report channel owner in error messages

Full piglit run with this patch:
http://people.freedesktop.org/~mslusarz/chan_owners.txt

This patch covers only a small subset of all error messages, so:

Not-yet-signed-off-by Marcin Slusarz <[email protected]>

Comments? Ideas?

(This commit depends on this one:
http://people.freedesktop.org/~mslusarz/0001-drm-nouveau-split-fifo-interrupt-handler.patch )

Full piglit run with this patch:
http://people.freedesktop.org/~mslusarz/chan_owners.txt

This patch covers only a small subset of all error messages, so:

Not-yet-signed-off-by Marcin Slusarz <[email protected]>

Comments? Ideas?

(This commit depends on this one:
http://people.freedesktop.org/~mslusarz/0001-drm-nouveau-split-fifo-interrupt-handler.patch )

---
 core/engine/fifo/nv04.c    |   43 +++++++++++++++++++++++++++++-------
 core/engine/graph/nv50.c   |   14 +++++++++--
 core/include/core/client.h |    2 -
 core/subdev/fb/nv50.c      |   53 ++++++++++++++++++++++++++++++++++++++++++---
 nouveau_drm.c              |    5 ++--
 5 files changed, 100 insertions(+), 17 deletions(-)

drivers/gpu/drm/nouveau/core/engine/fifo/nv04.c +35 -8

index 76944c4..f5d4d28 100644
@@ -24,6 +24,7 @@
 
 #include <core/os.h>
 #include <core/class.h>
+#include <core/client.h>
 #include <core/engctx.h>
 #include <core/namedb.h>
 #include <core/handle.h>

@@ -398,10 +399,29 @@ out:
 	return handled;
 }
 
+static struct nouveau_client *
+nv04_fifo_client_for_chid(struct nv04_fifo_priv *priv, u32 chid)
+{
+	struct nouveau_fifo_chan *chan;
+	struct nouveau_client *client = NULL;
+	unsigned long flags;
+
+	spin_lock_irqsave(&priv->base.lock, flags);
+	if (chid >= priv->base.min &&
+			chid <= priv->base.max) {
+		chan = (void *)priv->base.channel[chid];
+		client = nouveau_client(chan);
+	}
+	spin_unlock_irqrestore(&priv->base.lock, flags);
+
+	return client;
+}
+
 static void
 nv04_fifo_cache_error(struct nouveau_device *device,
 		struct nv04_fifo_priv *priv, u32 chid, u32 get)
 {
+	struct nouveau_client *client;
 	u32 mthd, data;
 	int ptr;
 

@@ -421,9 +441,12 @@ nv04_fifo_cache_error(struct nouveau_device *device,
 	}
 
 	if (!nv04_fifo_swmthd(priv, chid, mthd, data)) {
+		client = nv04_fifo_client_for_chid(priv, chid);
+
 		nv_error(priv,
-			 "CACHE_ERROR - Ch %d/%d Mthd 0x%04x Data 0x%08x\n",
-			 chid, (mthd >> 13) & 7, mthd & 0x1ffc, data);
+			 "CACHE_ERROR - Ch %d/%d [%s] Mthd 0x%04x Data 0x%08x\n",
+			 chid, (mthd >> 13) & 7, client ? client->name : "unk",
+			 mthd & 0x1ffc, data);
 	}
 
 	nv_wr32(priv, NV04_PFIFO_CACHE1_DMA_PUSH, 0);

@@ -445,11 +468,14 @@ static void
 nv04_fifo_dma_pusher(struct nouveau_device *device, struct nv04_fifo_priv *priv,
 		u32 chid)
 {
+	struct nouveau_client *client;
 	u32 dma_get = nv_rd32(priv, 0x003244);
 	u32 dma_put = nv_rd32(priv, 0x003240);
 	u32 push = nv_rd32(priv, 0x003220);
 	u32 state = nv_rd32(priv, 0x003228);
 
+	client = nv04_fifo_client_for_chid(priv, chid);
+
 	if (device->card_type == NV_50) {
 		u32 ho_get = nv_rd32(priv, 0x003328);
 		u32 ho_put = nv_rd32(priv, 0x003320);

@@ -457,9 +483,10 @@ nv04_fifo_dma_pusher(struct nouveau_device *device, struct nv04_fifo_priv *priv,
 		u32 ib_put = nv_rd32(priv, 0x003330);
 
 		nv_error(priv,
-			 "DMA_PUSHER - Ch %d Get 0x%02x%08x Put 0x%02x%08x IbGet 0x%08x IbPut 0x%08x State 0x%08x (err: %s) Push 0x%08x\n",
-			 chid, ho_get, dma_get, ho_put, dma_put, ib_get, ib_put,
-			 state, nv_dma_state_err(state), push);
+			 "DMA_PUSHER - Ch %d [%s] Get 0x%02x%08x Put 0x%02x%08x IbGet 0x%08x IbPut 0x%08x State 0x%08x (err: %s) Push 0x%08x\n",
+			 chid, client ? client->name : "unk", ho_get, dma_get,
+			 ho_put, dma_put, ib_get, ib_put, state,
+			 nv_dma_state_err(state), push);
 
 		/* METHOD_COUNT, in DMA_STATE on earlier chipsets */
 		nv_wr32(priv, 0x003364, 0x00000000);

@@ -471,9 +498,9 @@ nv04_fifo_dma_pusher(struct nouveau_device *device, struct nv04_fifo_priv *priv,
 			nv_wr32(priv, 0x003334, ib_put);
 	} else {
 		nv_error(priv,
-			 "DMA_PUSHER - Ch %d Get 0x%08x Put 0x%08x State 0x%08x (err: %s) Push 0x%08x\n",
-			 chid, dma_get, dma_put, state, nv_dma_state_err(state),
-			 push);
+			 "DMA_PUSHER - Ch %d [%s] Get 0x%08x Put 0x%08x State 0x%08x (err: %s) Push 0x%08x\n",
+			 chid, client ? client->name : "unk", dma_get, dma_put,
+			 state, nv_dma_state_err(state), push);
 
 		if (dma_get != dma_put)
 			nv_wr32(priv, 0x003244, dma_put);

drivers/gpu/drm/nouveau/core/engine/graph/nv50.c +11 -3

index b1c3d83..f2935ed 100644
@@ -24,6 +24,7 @@
 
 #include <core/os.h>
 #include <core/class.h>
+#include <core/client.h>
 #include <core/handle.h>
 #include <core/engctx.h>
 #include <core/enum.h>

@@ -786,12 +787,19 @@ nv50_graph_intr(struct nouveau_subdev *subdev)
 	nv_wr32(priv, 0x400500, 0x00010001);
 
 	if (show) {
+		const char *client_name = "unk";
+		if (engctx) {
+			struct nouveau_client *client = nouveau_client(engctx);
+			if (client)
+				client_name = client->name;
+		}
 		nv_error(priv, "");
 		nouveau_bitfield_print(nv50_graph_intr_name, show);
 		printk("\n");
-		nv_error(priv, "ch %d [0x%010llx] subc %d class 0x%04x "
-			       "mthd 0x%04x data 0x%08x\n",
-			 chid, (u64)inst << 12, subc, class, mthd, data);
+		nv_error(priv,
+			 "ch %d [0x%010llx %s] subc %d class 0x%04x mthd 0x%04x data 0x%08x\n",
+			 chid, (u64)inst << 12, client_name, subc, class, mthd,
+			 data);
 	}
 
 	if (nv_rd32(priv, 0x400824) & (1 << 31))

drivers/gpu/drm/nouveau/core/include/core/client.h +1 -1

index 0193532..8feba2f 100644
@@ -7,7 +7,7 @@ struct nouveau_client {
 	struct nouveau_namedb base;
 	struct nouveau_handle *root;
 	struct nouveau_object *device;
-	char name[16];
+	char name[32];
 	u32 debug;
 	struct nouveau_vm *vm;
 };

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +6 -1

index 487cb8c..d1120fc 100644
@@ -22,8 +22,13 @@
  * Authors: Ben Skeggs
  */
 
-#include <core/object.h>
+#include <core/client.h>
 #include <core/enum.h>
+#include <core/engctx.h>
+#include <core/object.h>
+
+#include <engine/fifo.h>
+#include <engine/graph.h>
 
 #include <subdev/fb.h>
 #include <subdev/bios.h>

@@ -317,6 +322,19 @@ static const struct nouveau_enum vm_engine[] = {
 	{}

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +6 -0

+};
+
 static const struct nouveau_enum vm_fault[] = {
 	{ 0x00000000, "PT_NOT_PRESENT", NULL },
 	{ 0x00000001, "PT_TOO_SHORT", NULL },

@@ -334,8 +352,12 @@ static void
 nv50_fb_intr(struct nouveau_subdev *subdev)
 {
 	struct nouveau_device *device = nv_device(subdev);
+	struct nouveau_engine *engine = NULL;
 	struct nv50_fb_priv *priv = (void *)subdev;
 	const struct nouveau_enum *en, *cl;
+	struct nouveau_object *engctx = NULL;
+	const int *poss_engines = NULL;
+	const char *client_name = "unk";
 	u32 trap[6], idx, chan;
 	u8 st0, st1, st2, st3;
 	int i;

@@ -366,9 +388,34 @@ nv50_fb_intr(struct nouveau_subdev *subdev)
 	}
 	chan = (trap[2] << 16) | trap[1];
 

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +18 -1

+		if (engine) {
+			engctx = nouveau_engctx_get(engine, chan);
+			if (engctx)
+				break;
+		}
+	}
+
+	if (engctx) {
+		struct nouveau_client *client = nouveau_client(engctx);
+		if (client)
+			client_name = client->name;
+	}
+
+	nv_error(priv, "trapped %s at 0x%02x%04x%04x on channel 0x%08x [%s] ",
 		 (trap[5] & 0x00000100) ? "read" : "write",
-		 trap[5] & 0xff, trap[4] & 0xffff, trap[3] & 0xffff, chan);
+		 trap[5] & 0xff, trap[4] & 0xffff, trap[3] & 0xffff, chan,
+		 client_name);
+
+	nouveau_engctx_put(engctx);
 
 	en = nouveau_enum_find(vm_engine, st0);
 	if (en)

drivers/gpu/drm/nouveau/nouveau_drm.c +3 -2

index 38e9a08..a50362e 100644
@@ -539,10 +539,11 @@ nouveau_drm_open(struct drm_device *dev, struct drm_file *fpriv)
 	struct pci_dev *pdev = dev->pdev;
 	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nouveau_cli *cli;
-	char name[16];
+	char name[32], tmpname[TASK_COMM_LEN];
 	int ret;
 
-	snprintf(name, sizeof(name), "%d", pid_nr(fpriv->pid));
+	get_task_comm(tmpname, current);
+	snprintf(name, sizeof(name), "%s[%d]", tmpname, pid_nr(fpriv->pid));
 
 	ret = nouveau_cli_create(pdev, name, sizeof(*cli), (void **)&cli);
 	if (ret)

_______________________________________________
Nouveau mailing list [email protected]
http://lists.freedesktop.org/mailman/listinfo/nouveau

Quoted 7 lines · Marcin Slusarz

Full piglit run with this patch:
http://people.freedesktop.org/~mslusarz/chan_owners.txt

This patch covers only a small subset of all error messages, so:

Not-yet-signed-off-by Marcin Slusarz <[email protected]>

Comments? Ideas?

Very nice idea. I'll put a little bit of thought into it, but I think it's looking good. More comments where appropriate inline.

Quoted 179 lines

(This commit depends on this one:
http://people.freedesktop.org/~mslusarz/0001-drm-nouveau-split-fifo-interrupt-handler.patch )

---
 core/engine/fifo/nv04.c    |   43 +++++++++++++++++++++++++++++-------
 core/engine/graph/nv50.c   |   14 +++++++++--
 core/include/core/client.h |    2 -
 core/subdev/fb/nv50.c      |   53 ++++++++++++++++++++++++++++++++++++++++++---
 nouveau_drm.c              |    5 ++--
 5 files changed, 100 insertions(+), 17 deletions(-)

drivers/gpu/drm/nouveau/core/engine/fifo/nv04.c +35 -8

index 76944c4..f5d4d28 100644
@@ -24,6 +24,7 @@
 
 #include <core/os.h>
 #include <core/class.h>
+#include <core/client.h>
 #include <core/engctx.h>
 #include <core/namedb.h>
 #include <core/handle.h>

@@ -398,10 +399,29 @@ out:
 	return handled;
 }
 
+static struct nouveau_client *
+nv04_fifo_client_for_chid(struct nv04_fifo_priv *priv, u32 chid)
+{
+	struct nouveau_fifo_chan *chan;
+	struct nouveau_client *client = NULL;
+	unsigned long flags;
+
+	spin_lock_irqsave(&priv->base.lock, flags);
+	if (chid >= priv->base.min &&
+			chid <= priv->base.max) {
+		chan = (void *)priv->base.channel[chid];
+		client = nouveau_client(chan);
+	}
+	spin_unlock_irqrestore(&priv->base.lock, flags);
+
+	return client;
+}
+
 static void
 nv04_fifo_cache_error(struct nouveau_device *device,
 		struct nv04_fifo_priv *priv, u32 chid, u32 get)
 {
+	struct nouveau_client *client;
 	u32 mthd, data;
 	int ptr;
 

@@ -421,9 +441,12 @@ nv04_fifo_cache_error(struct nouveau_device *device,
 	}
 
 	if (!nv04_fifo_swmthd(priv, chid, mthd, data)) {
+		client = nv04_fifo_client_for_chid(priv, chid);
+
 		nv_error(priv,
-			 "CACHE_ERROR - Ch %d/%d Mthd 0x%04x Data 0x%08x\n",
-			 chid, (mthd >> 13) & 7, mthd & 0x1ffc, data);
+			 "CACHE_ERROR - Ch %d/%d [%s] Mthd 0x%04x Data 0x%08x\n",
+			 chid, (mthd >> 13) & 7, client ? client->name : "unk",
+			 mthd & 0x1ffc, data);
 	}
 
 	nv_wr32(priv, NV04_PFIFO_CACHE1_DMA_PUSH, 0);

@@ -445,11 +468,14 @@ static void
 nv04_fifo_dma_pusher(struct nouveau_device *device, struct nv04_fifo_priv *priv,
 		u32 chid)
 {
+	struct nouveau_client *client;
 	u32 dma_get = nv_rd32(priv, 0x003244);
 	u32 dma_put = nv_rd32(priv, 0x003240);
 	u32 push = nv_rd32(priv, 0x003220);
 	u32 state = nv_rd32(priv, 0x003228);
 
+	client = nv04_fifo_client_for_chid(priv, chid);
+
 	if (device->card_type == NV_50) {
 		u32 ho_get = nv_rd32(priv, 0x003328);
 		u32 ho_put = nv_rd32(priv, 0x003320);

@@ -457,9 +483,10 @@ nv04_fifo_dma_pusher(struct nouveau_device *device, struct nv04_fifo_priv *priv,
 		u32 ib_put = nv_rd32(priv, 0x003330);
 
 		nv_error(priv,
-			 "DMA_PUSHER - Ch %d Get 0x%02x%08x Put 0x%02x%08x IbGet 0x%08x IbPut 0x%08x State 0x%08x (err: %s) Push 0x%08x\n",
-			 chid, ho_get, dma_get, ho_put, dma_put, ib_get, ib_put,
-			 state, nv_dma_state_err(state), push);
+			 "DMA_PUSHER - Ch %d [%s] Get 0x%02x%08x Put 0x%02x%08x IbGet 0x%08x IbPut 0x%08x State 0x%08x (err: %s) Push 0x%08x\n",
+			 chid, client ? client->name : "unk", ho_get, dma_get,
+			 ho_put, dma_put, ib_get, ib_put, state,
+			 nv_dma_state_err(state), push);
 
 		/* METHOD_COUNT, in DMA_STATE on earlier chipsets */
 		nv_wr32(priv, 0x003364, 0x00000000);

@@ -471,9 +498,9 @@ nv04_fifo_dma_pusher(struct nouveau_device *device, struct nv04_fifo_priv *priv,
 			nv_wr32(priv, 0x003334, ib_put);
 	} else {
 		nv_error(priv,
-			 "DMA_PUSHER - Ch %d Get 0x%08x Put 0x%08x State 0x%08x (err: %s) Push 0x%08x\n",
-			 chid, dma_get, dma_put, state, nv_dma_state_err(state),
-			 push);
+			 "DMA_PUSHER - Ch %d [%s] Get 0x%08x Put 0x%08x State 0x%08x (err: %s) Push 0x%08x\n",
+			 chid, client ? client->name : "unk", dma_get, dma_put,
+			 state, nv_dma_state_err(state), push);
 
 		if (dma_get != dma_put)
 			nv_wr32(priv, 0x003244, dma_put);

drivers/gpu/drm/nouveau/core/engine/graph/nv50.c +11 -3

index b1c3d83..f2935ed 100644
@@ -24,6 +24,7 @@
 
 #include <core/os.h>
 #include <core/class.h>
+#include <core/client.h>
 #include <core/handle.h>
 #include <core/engctx.h>
 #include <core/enum.h>

@@ -786,12 +787,19 @@ nv50_graph_intr(struct nouveau_subdev *subdev)
 	nv_wr32(priv, 0x400500, 0x00010001);
 
 	if (show) {
+		const char *client_name = "unk";
+		if (engctx) {
+			struct nouveau_client *client = nouveau_client(engctx);
+			if (client)
+				client_name = client->name;
+		}
 		nv_error(priv, "");
 		nouveau_bitfield_print(nv50_graph_intr_name, show);
 		printk("\n");
-		nv_error(priv, "ch %d [0x%010llx] subc %d class 0x%04x "
-			       "mthd 0x%04x data 0x%08x\n",
-			 chid, (u64)inst << 12, subc, class, mthd, data);
+		nv_error(priv,
+			 "ch %d [0x%010llx %s] subc %d class 0x%04x mthd 0x%04x data 0x%08x\n",
+			 chid, (u64)inst << 12, client_name, subc, class, mthd,
+			 data);
 	}
 
 	if (nv_rd32(priv, 0x400824) & (1 << 31))

drivers/gpu/drm/nouveau/core/include/core/client.h +1 -1

index 0193532..8feba2f 100644
@@ -7,7 +7,7 @@ struct nouveau_client {
 	struct nouveau_namedb base;
 	struct nouveau_handle *root;
 	struct nouveau_object *device;
-	char name[16];
+	char name[32];
 	u32 debug;
 	struct nouveau_vm *vm;
 };

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +6 -1

index 487cb8c..d1120fc 100644
@@ -22,8 +22,13 @@
  * Authors: Ben Skeggs
  */
 
-#include <core/object.h>
+#include <core/client.h>
 #include <core/enum.h>
+#include <core/engctx.h>
+#include <core/object.h>
+
+#include <engine/fifo.h>
+#include <engine/graph.h>
 
 #include <subdev/fb.h>
 #include <subdev/bios.h>

@@ -317,6 +322,19 @@ static const struct nouveau_enum vm_engine[] = {
 	{}

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c static const struct nouveau_enum vm_engine[] = { · line 332

Quoted 10 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +8 -0

 };
 
+static const struct nouveau_engine_map {
+	u32 value;
+	int engines[3];
+} nvdev_engine_for_vm_engine[] = {
+	{ 0x00000000, {NVDEV_ENGINE_GR, 0} },
+	{ 0x00000001, {NVDEV_ENGINE_VP, 0} },
+	{ 0x00000005, {NVDEV_ENGINE_FIFO, 0} },
+	{ 0x00000008, {NVDEV_ENGINE_PPP, NVDEV_ENGINE_MPEG, 0} },

I think it may actually be a good idea to go (in core/device.h):

NVDEV_ENGINE_MPEG,
+ NVDEV_ENGINE_PPP = NVDEV_ENGINE_MPEG,
NVDEV_ENGINE_ME,

PPP got introduced when MPEG disappeared. There's likely a few other engines we can create as aliases for each other too. What do you think?

I can handle the aliasing if you like, but feel free :)

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c static const struct nouveau_enum vm_engine[] = { · line 335

Quoted 3 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +3 -0

+	{ 0x00000009, {NVDEV_ENGINE_BSP, 0} },
+	{ 0x0000000a, {NVDEV_ENGINE_CRYPT, 0} },
+	{ 0x0000000d, {NVDEV_ENGINE_COPY0, NVDEV_ENGINE_COPY1, 0} },

COPY1 doesn't exist on NV50. NVC0 has its own VM engine for the additional copy engines.

Quoted 22 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +6 -0

+};
+
 static const struct nouveau_enum vm_fault[] = {
 	{ 0x00000000, "PT_NOT_PRESENT", NULL },
 	{ 0x00000001, "PT_TOO_SHORT", NULL },

@@ -334,8 +352,12 @@ static void
 nv50_fb_intr(struct nouveau_subdev *subdev)
 {
 	struct nouveau_device *device = nv_device(subdev);
+	struct nouveau_engine *engine = NULL;
 	struct nv50_fb_priv *priv = (void *)subdev;
 	const struct nouveau_enum *en, *cl;
+	struct nouveau_object *engctx = NULL;
+	const int *poss_engines = NULL;
+	const char *client_name = "unk";
 	u32 trap[6], idx, chan;
 	u8 st0, st1, st2, st3;
 	int i;

@@ -366,9 +388,34 @@ nv50_fb_intr(struct nouveau_subdev *subdev)
 	}
 	chan = (trap[2] << 16) | trap[1];
 

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c nv50_fb_intr(struct nouveau_subdev *subdev) · line 399

Quoted 10 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +9 -1

-	nv_error(priv, "trapped %s at 0x%02x%04x%04x on channel 0x%08x ",
+	for (i = 0; i < ARRAY_SIZE(nvdev_engine_for_vm_engine); ++i) {
+		if (nvdev_engine_for_vm_engine[i].value == st0) {
+			poss_engines = nvdev_engine_for_vm_engine[i].engines;
+			break;
+		}
+	}
+
+	for (i = 0; poss_engines && poss_engines[i]; ++i) {
+		engine = nv_engine(device->subdev[poss_engines[i]]);

engine = nouveau_engine(device, poss_engines[i]);

Perhaps you can even append another field to nouveau_enum to store the subdev index in too, rather than having to look it up?

Quoted 41 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +18 -1

+		if (engine) {
+			engctx = nouveau_engctx_get(engine, chan);
+			if (engctx)
+				break;
+		}
+	}
+
+	if (engctx) {
+		struct nouveau_client *client = nouveau_client(engctx);
+		if (client)
+			client_name = client->name;
+	}
+
+	nv_error(priv, "trapped %s at 0x%02x%04x%04x on channel 0x%08x [%s] ",
 		 (trap[5] & 0x00000100) ? "read" : "write",
-		 trap[5] & 0xff, trap[4] & 0xffff, trap[3] & 0xffff, chan);
+		 trap[5] & 0xff, trap[4] & 0xffff, trap[3] & 0xffff, chan,
+		 client_name);
+
+	nouveau_engctx_put(engctx);
 
 	en = nouveau_enum_find(vm_engine, st0);
 	if (en)

drivers/gpu/drm/nouveau/nouveau_drm.c +3 -2

index 38e9a08..a50362e 100644
@@ -539,10 +539,11 @@ nouveau_drm_open(struct drm_device *dev, struct drm_file *fpriv)
 	struct pci_dev *pdev = dev->pdev;
 	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nouveau_cli *cli;
-	char name[16];
+	char name[32], tmpname[TASK_COMM_LEN];
 	int ret;
 
-	snprintf(name, sizeof(name), "%d", pid_nr(fpriv->pid));
+	get_task_comm(tmpname, current);
+	snprintf(name, sizeof(name), "%s[%d]", tmpname, pid_nr(fpriv->pid));
 
 	ret = nouveau_cli_create(pdev, name, sizeof(*cli), (void **)&cli);
 	if (ret)

Quoted 10 lines · Ben Skeggs

Quoted 7 lines · Marcin Slusarz

Full piglit run with this patch:
http://people.freedesktop.org/~mslusarz/chan_owners.txt

This patch covers only a small subset of all error messages, so:

Not-yet-signed-off-by Marcin Slusarz <[email protected]>

Comments? Ideas?

Very nice idea. I'll put a little bit of thought into it, but I think it's looking good. More comments where appropriate inline.

Thanks.

Quoted 195 lines

Quoted 179 lines

(This commit depends on this one:
http://people.freedesktop.org/~mslusarz/0001-drm-nouveau-split-fifo-interrupt-handler.patch )

---
 core/engine/fifo/nv04.c    |   43 +++++++++++++++++++++++++++++-------
 core/engine/graph/nv50.c   |   14 +++++++++--
 core/include/core/client.h |    2 -
 core/subdev/fb/nv50.c      |   53 ++++++++++++++++++++++++++++++++++++++++++---
 nouveau_drm.c              |    5 ++--
 5 files changed, 100 insertions(+), 17 deletions(-)

drivers/gpu/drm/nouveau/core/engine/fifo/nv04.c +35 -8

index 76944c4..f5d4d28 100644
@@ -24,6 +24,7 @@
 
 #include <core/os.h>
 #include <core/class.h>
+#include <core/client.h>
 #include <core/engctx.h>
 #include <core/namedb.h>
 #include <core/handle.h>

@@ -398,10 +399,29 @@ out:
 	return handled;
 }
 
+static struct nouveau_client *
+nv04_fifo_client_for_chid(struct nv04_fifo_priv *priv, u32 chid)
+{
+	struct nouveau_fifo_chan *chan;
+	struct nouveau_client *client = NULL;
+	unsigned long flags;
+
+	spin_lock_irqsave(&priv->base.lock, flags);
+	if (chid >= priv->base.min &&
+			chid <= priv->base.max) {
+		chan = (void *)priv->base.channel[chid];
+		client = nouveau_client(chan);
+	}
+	spin_unlock_irqrestore(&priv->base.lock, flags);
+
+	return client;
+}
+
 static void
 nv04_fifo_cache_error(struct nouveau_device *device,
 		struct nv04_fifo_priv *priv, u32 chid, u32 get)
 {
+	struct nouveau_client *client;
 	u32 mthd, data;
 	int ptr;
 

@@ -421,9 +441,12 @@ nv04_fifo_cache_error(struct nouveau_device *device,
 	}
 
 	if (!nv04_fifo_swmthd(priv, chid, mthd, data)) {
+		client = nv04_fifo_client_for_chid(priv, chid);
+
 		nv_error(priv,
-			 "CACHE_ERROR - Ch %d/%d Mthd 0x%04x Data 0x%08x\n",
-			 chid, (mthd >> 13) & 7, mthd & 0x1ffc, data);
+			 "CACHE_ERROR - Ch %d/%d [%s] Mthd 0x%04x Data 0x%08x\n",
+			 chid, (mthd >> 13) & 7, client ? client->name : "unk",
+			 mthd & 0x1ffc, data);
 	}
 
 	nv_wr32(priv, NV04_PFIFO_CACHE1_DMA_PUSH, 0);

@@ -445,11 +468,14 @@ static void
 nv04_fifo_dma_pusher(struct nouveau_device *device, struct nv04_fifo_priv *priv,
 		u32 chid)
 {
+	struct nouveau_client *client;
 	u32 dma_get = nv_rd32(priv, 0x003244);
 	u32 dma_put = nv_rd32(priv, 0x003240);
 	u32 push = nv_rd32(priv, 0x003220);
 	u32 state = nv_rd32(priv, 0x003228);
 
+	client = nv04_fifo_client_for_chid(priv, chid);
+
 	if (device->card_type == NV_50) {
 		u32 ho_get = nv_rd32(priv, 0x003328);
 		u32 ho_put = nv_rd32(priv, 0x003320);

@@ -457,9 +483,10 @@ nv04_fifo_dma_pusher(struct nouveau_device *device, struct nv04_fifo_priv *priv,
 		u32 ib_put = nv_rd32(priv, 0x003330);
 
 		nv_error(priv,
-			 "DMA_PUSHER - Ch %d Get 0x%02x%08x Put 0x%02x%08x IbGet 0x%08x IbPut 0x%08x State 0x%08x (err: %s) Push 0x%08x\n",
-			 chid, ho_get, dma_get, ho_put, dma_put, ib_get, ib_put,
-			 state, nv_dma_state_err(state), push);
+			 "DMA_PUSHER - Ch %d [%s] Get 0x%02x%08x Put 0x%02x%08x IbGet 0x%08x IbPut 0x%08x State 0x%08x (err: %s) Push 0x%08x\n",
+			 chid, client ? client->name : "unk", ho_get, dma_get,
+			 ho_put, dma_put, ib_get, ib_put, state,
+			 nv_dma_state_err(state), push);
 
 		/* METHOD_COUNT, in DMA_STATE on earlier chipsets */
 		nv_wr32(priv, 0x003364, 0x00000000);

@@ -471,9 +498,9 @@ nv04_fifo_dma_pusher(struct nouveau_device *device, struct nv04_fifo_priv *priv,
 			nv_wr32(priv, 0x003334, ib_put);
 	} else {
 		nv_error(priv,
-			 "DMA_PUSHER - Ch %d Get 0x%08x Put 0x%08x State 0x%08x (err: %s) Push 0x%08x\n",
-			 chid, dma_get, dma_put, state, nv_dma_state_err(state),
-			 push);
+			 "DMA_PUSHER - Ch %d [%s] Get 0x%08x Put 0x%08x State 0x%08x (err: %s) Push 0x%08x\n",
+			 chid, client ? client->name : "unk", dma_get, dma_put,
+			 state, nv_dma_state_err(state), push);
 
 		if (dma_get != dma_put)
 			nv_wr32(priv, 0x003244, dma_put);

drivers/gpu/drm/nouveau/core/engine/graph/nv50.c +11 -3

index b1c3d83..f2935ed 100644
@@ -24,6 +24,7 @@
 
 #include <core/os.h>
 #include <core/class.h>
+#include <core/client.h>
 #include <core/handle.h>
 #include <core/engctx.h>
 #include <core/enum.h>

@@ -786,12 +787,19 @@ nv50_graph_intr(struct nouveau_subdev *subdev)
 	nv_wr32(priv, 0x400500, 0x00010001);
 
 	if (show) {
+		const char *client_name = "unk";
+		if (engctx) {
+			struct nouveau_client *client = nouveau_client(engctx);
+			if (client)
+				client_name = client->name;
+		}
 		nv_error(priv, "");
 		nouveau_bitfield_print(nv50_graph_intr_name, show);
 		printk("\n");
-		nv_error(priv, "ch %d [0x%010llx] subc %d class 0x%04x "
-			       "mthd 0x%04x data 0x%08x\n",
-			 chid, (u64)inst << 12, subc, class, mthd, data);
+		nv_error(priv,
+			 "ch %d [0x%010llx %s] subc %d class 0x%04x mthd 0x%04x data 0x%08x\n",
+			 chid, (u64)inst << 12, client_name, subc, class, mthd,
+			 data);
 	}
 
 	if (nv_rd32(priv, 0x400824) & (1 << 31))

drivers/gpu/drm/nouveau/core/include/core/client.h +1 -1

index 0193532..8feba2f 100644
@@ -7,7 +7,7 @@ struct nouveau_client {
 	struct nouveau_namedb base;
 	struct nouveau_handle *root;
 	struct nouveau_object *device;
-	char name[16];
+	char name[32];
 	u32 debug;
 	struct nouveau_vm *vm;
 };

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +6 -1

index 487cb8c..d1120fc 100644
@@ -22,8 +22,13 @@
  * Authors: Ben Skeggs
  */
 
-#include <core/object.h>
+#include <core/client.h>
 #include <core/enum.h>
+#include <core/engctx.h>
+#include <core/object.h>
+
+#include <engine/fifo.h>
+#include <engine/graph.h>
 
 #include <subdev/fb.h>
 #include <subdev/bios.h>

@@ -317,6 +322,19 @@ static const struct nouveau_enum vm_engine[] = {
 	{}

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c static const struct nouveau_enum vm_engine[] = { · line 332

Quoted 10 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +8 -0

 };
 
+static const struct nouveau_engine_map {
+	u32 value;
+	int engines[3];
+} nvdev_engine_for_vm_engine[] = {
+	{ 0x00000000, {NVDEV_ENGINE_GR, 0} },
+	{ 0x00000001, {NVDEV_ENGINE_VP, 0} },
+	{ 0x00000005, {NVDEV_ENGINE_FIFO, 0} },
+	{ 0x00000008, {NVDEV_ENGINE_PPP, NVDEV_ENGINE_MPEG, 0} },

I think it may actually be a good idea to go (in core/device.h):

NVDEV_ENGINE_MPEG,
+ NVDEV_ENGINE_PPP = NVDEV_ENGINE_MPEG,
NVDEV_ENGINE_ME,

Quoted 2 lines

PPP got introduced when MPEG disappeared. There's likely a few other engines we can create as aliases for each other too. What do you think?

I'm not sure it's such a good idea. Suddenly nouveau_engctx_get(NVDEV_ENGINE_PPP, chan) can return nouveau_mpeg_chan and device->subdev[NVDEV_ENGINE_PPP] can point to nouveau_mpeg, etc. So in generic code you can't rely on device->subdev[X] being NULL on cards which don't have X engine...

Quoted 9 lines

I can handle the aliasing if you like, but feel free :)

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c static const struct nouveau_enum vm_engine[] = { · line 335

Quoted 3 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +3 -0

+	{ 0x00000009, {NVDEV_ENGINE_BSP, 0} },
+	{ 0x0000000a, {NVDEV_ENGINE_CRYPT, 0} },
+	{ 0x0000000d, {NVDEV_ENGINE_COPY0, NVDEV_ENGINE_COPY1, 0} },

COPY1 doesn't exist on NV50. NVC0 has its own VM engine for the additional copy engines.

OK.

Quoted 22 lines

Quoted 22 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +6 -0

+};
+
 static const struct nouveau_enum vm_fault[] = {
 	{ 0x00000000, "PT_NOT_PRESENT", NULL },
 	{ 0x00000001, "PT_TOO_SHORT", NULL },

@@ -334,8 +352,12 @@ static void
 nv50_fb_intr(struct nouveau_subdev *subdev)
 {
 	struct nouveau_device *device = nv_device(subdev);
+	struct nouveau_engine *engine = NULL;
 	struct nv50_fb_priv *priv = (void *)subdev;
 	const struct nouveau_enum *en, *cl;
+	struct nouveau_object *engctx = NULL;
+	const int *poss_engines = NULL;
+	const char *client_name = "unk";
 	u32 trap[6], idx, chan;
 	u8 st0, st1, st2, st3;
 	int i;

@@ -366,9 +388,34 @@ nv50_fb_intr(struct nouveau_subdev *subdev)
 	}
 	chan = (trap[2] << 16) | trap[1];
 

Quoted 11 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c nv50_fb_intr(struct nouveau_subdev *subdev) · line 399

Quoted 10 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +9 -1

-	nv_error(priv, "trapped %s at 0x%02x%04x%04x on channel 0x%08x ",
+	for (i = 0; i < ARRAY_SIZE(nvdev_engine_for_vm_engine); ++i) {
+		if (nvdev_engine_for_vm_engine[i].value == st0) {
+			poss_engines = nvdev_engine_for_vm_engine[i].engines;
+			break;
+		}
+	}
+
+	for (i = 0; poss_engines && poss_engines[i]; ++i) {
+		engine = nv_engine(device->subdev[poss_engines[i]]);

engine = nouveau_engine(device, poss_engines[i]);

OK.

Quoted 2 lines

Perhaps you can even append another field to nouveau_enum to store the subdev index in too, rather than having to look it up?

Good idea. Thanks.

Quoted 41 lines

Quoted 41 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +18 -1

+		if (engine) {
+			engctx = nouveau_engctx_get(engine, chan);
+			if (engctx)
+				break;
+		}
+	}
+
+	if (engctx) {
+		struct nouveau_client *client = nouveau_client(engctx);
+		if (client)
+			client_name = client->name;
+	}
+
+	nv_error(priv, "trapped %s at 0x%02x%04x%04x on channel 0x%08x [%s] ",
 		 (trap[5] & 0x00000100) ? "read" : "write",
-		 trap[5] & 0xff, trap[4] & 0xffff, trap[3] & 0xffff, chan);
+		 trap[5] & 0xff, trap[4] & 0xffff, trap[3] & 0xffff, chan,
+		 client_name);
+
+	nouveau_engctx_put(engctx);
 
 	en = nouveau_enum_find(vm_engine, st0);
 	if (en)

drivers/gpu/drm/nouveau/nouveau_drm.c +3 -2

index 38e9a08..a50362e 100644
@@ -539,10 +539,11 @@ nouveau_drm_open(struct drm_device *dev, struct drm_file *fpriv)
 	struct pci_dev *pdev = dev->pdev;
 	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nouveau_cli *cli;
-	char name[16];
+	char name[32], tmpname[TASK_COMM_LEN];
 	int ret;
 
-	snprintf(name, sizeof(name), "%d", pid_nr(fpriv->pid));
+	get_task_comm(tmpname, current);
+	snprintf(name, sizeof(name), "%s[%d]", tmpname, pid_nr(fpriv->pid));
 
 	ret = nouveau_cli_create(pdev, name, sizeof(*cli), (void **)&cli);
 	if (ret)

Quoted 40 lines

Quoted 39 lines

Quoted 21 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +6 -1

index 487cb8c..d1120fc 100644
@@ -22,8 +22,13 @@
  * Authors: Ben Skeggs
  */
 
-#include <core/object.h>
+#include <core/client.h>
 #include <core/enum.h>
+#include <core/engctx.h>
+#include <core/object.h>
+
+#include <engine/fifo.h>
+#include <engine/graph.h>
 
 #include <subdev/fb.h>
 #include <subdev/bios.h>

@@ -317,6 +322,19 @@ static const struct nouveau_enum vm_engine[] = {
 	{}

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c static const struct nouveau_enum vm_engine[] = { · line 332

Quoted 10 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +8 -0

 };
 
+static const struct nouveau_engine_map {
+	u32 value;
+	int engines[3];
+} nvdev_engine_for_vm_engine[] = {
+	{ 0x00000000, {NVDEV_ENGINE_GR, 0} },
+	{ 0x00000001, {NVDEV_ENGINE_VP, 0} },
+	{ 0x00000005, {NVDEV_ENGINE_FIFO, 0} },
+	{ 0x00000008, {NVDEV_ENGINE_PPP, NVDEV_ENGINE_MPEG, 0} },

I think it may actually be a good idea to go (in core/device.h):

NVDEV_ENGINE_MPEG,
+ NVDEV_ENGINE_PPP = NVDEV_ENGINE_MPEG,
NVDEV_ENGINE_ME,

PPP got introduced when MPEG disappeared. There's likely a few other engines we can create as aliases for each other too. What do you think?

Quoted 4 lines

I'm not sure it's such a good idea. Suddenly nouveau_engctx_get(NVDEV_ENGINE_PPP, chan) can return nouveau_mpeg_chan and device->subdev[NVDEV_ENGINE_PPP] can point to nouveau_mpeg, etc. So in generic code you can't rely on device->subdev[X] being NULL on cards which don't have X engine...

This is true for non-engine subdevs, yes. But, the engines themselves should *never* *ever* be accessed from anything other than the object interface, from which it's impossible for a mix-up to happen.

If we do the aliasing this point should probably be documented in the enum list, and the nouveau_whatever() accessors removed.

Quoted 94 lines

Quoted 9 lines

I can handle the aliasing if you like, but feel free :)

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c static const struct nouveau_enum vm_engine[] = { · line 335

Quoted 3 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +3 -0

+	{ 0x00000009, {NVDEV_ENGINE_BSP, 0} },
+	{ 0x0000000a, {NVDEV_ENGINE_CRYPT, 0} },
+	{ 0x0000000d, {NVDEV_ENGINE_COPY0, NVDEV_ENGINE_COPY1, 0} },

COPY1 doesn't exist on NV50. NVC0 has its own VM engine for the additional copy engines.

OK.

Quoted 22 lines

Quoted 22 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +6 -0

+};
+
 static const struct nouveau_enum vm_fault[] = {
 	{ 0x00000000, "PT_NOT_PRESENT", NULL },
 	{ 0x00000001, "PT_TOO_SHORT", NULL },

@@ -334,8 +352,12 @@ static void
 nv50_fb_intr(struct nouveau_subdev *subdev)
 {
 	struct nouveau_device *device = nv_device(subdev);
+	struct nouveau_engine *engine = NULL;
 	struct nv50_fb_priv *priv = (void *)subdev;
 	const struct nouveau_enum *en, *cl;
+	struct nouveau_object *engctx = NULL;
+	const int *poss_engines = NULL;
+	const char *client_name = "unk";
 	u32 trap[6], idx, chan;
 	u8 st0, st1, st2, st3;
 	int i;

@@ -366,9 +388,34 @@ nv50_fb_intr(struct nouveau_subdev *subdev)
 	}
 	chan = (trap[2] << 16) | trap[1];
 

Quoted 11 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c nv50_fb_intr(struct nouveau_subdev *subdev) · line 399

Quoted 10 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +9 -1

-	nv_error(priv, "trapped %s at 0x%02x%04x%04x on channel 0x%08x ",
+	for (i = 0; i < ARRAY_SIZE(nvdev_engine_for_vm_engine); ++i) {
+		if (nvdev_engine_for_vm_engine[i].value == st0) {
+			poss_engines = nvdev_engine_for_vm_engine[i].engines;
+			break;
+		}
+	}
+
+	for (i = 0; poss_engines && poss_engines[i]; ++i) {
+		engine = nv_engine(device->subdev[poss_engines[i]]);

engine = nouveau_engine(device, poss_engines[i]);

OK.

Quoted 2 lines

Perhaps you can even append another field to nouveau_enum to store the subdev index in too, rather than having to look it up?

Good idea. Thanks.

Quoted 41 lines

Quoted 41 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +18 -1

+		if (engine) {
+			engctx = nouveau_engctx_get(engine, chan);
+			if (engctx)
+				break;
+		}
+	}
+
+	if (engctx) {
+		struct nouveau_client *client = nouveau_client(engctx);
+		if (client)
+			client_name = client->name;
+	}
+
+	nv_error(priv, "trapped %s at 0x%02x%04x%04x on channel 0x%08x [%s] ",
 		 (trap[5] & 0x00000100) ? "read" : "write",
-		 trap[5] & 0xff, trap[4] & 0xffff, trap[3] & 0xffff, chan);
+		 trap[5] & 0xff, trap[4] & 0xffff, trap[3] & 0xffff, chan,
+		 client_name);
+
+	nouveau_engctx_put(engctx);
 
 	en = nouveau_enum_find(vm_engine, st0);
 	if (en)

drivers/gpu/drm/nouveau/nouveau_drm.c +3 -2

index 38e9a08..a50362e 100644
@@ -539,10 +539,11 @@ nouveau_drm_open(struct drm_device *dev, struct drm_file *fpriv)
 	struct pci_dev *pdev = dev->pdev;
 	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nouveau_cli *cli;
-	char name[16];
+	char name[32], tmpname[TASK_COMM_LEN];
 	int ret;
 
-	snprintf(name, sizeof(name), "%d", pid_nr(fpriv->pid));
+	get_task_comm(tmpname, current);
+	snprintf(name, sizeof(name), "%s[%d]", tmpname, pid_nr(fpriv->pid));
 
 	ret = nouveau_cli_create(pdev, name, sizeof(*cli), (void **)&cli);
 	if (ret)

Quoted 44 lines · Ben Skeggs

Quoted 44 lines

Quoted 33 lines

Quoted 21 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +6 -1

index 487cb8c..d1120fc 100644
@@ -22,8 +22,13 @@
  * Authors: Ben Skeggs
  */
 
-#include <core/object.h>
+#include <core/client.h>
 #include <core/enum.h>
+#include <core/engctx.h>
+#include <core/object.h>
+
+#include <engine/fifo.h>
+#include <engine/graph.h>
 
 #include <subdev/fb.h>
 #include <subdev/bios.h>

@@ -317,6 +322,19 @@ static const struct nouveau_enum vm_engine[] = {
 	{}

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c static const struct nouveau_enum vm_engine[] = { · line 332

Quoted 10 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +8 -0

 };
 
+static const struct nouveau_engine_map {
+	u32 value;
+	int engines[3];
+} nvdev_engine_for_vm_engine[] = {
+	{ 0x00000000, {NVDEV_ENGINE_GR, 0} },
+	{ 0x00000001, {NVDEV_ENGINE_VP, 0} },
+	{ 0x00000005, {NVDEV_ENGINE_FIFO, 0} },
+	{ 0x00000008, {NVDEV_ENGINE_PPP, NVDEV_ENGINE_MPEG, 0} },

I think it may actually be a good idea to go (in core/device.h):

Quoted 6 lines

NVDEV_ENGINE_MPEG,
+ NVDEV_ENGINE_PPP = NVDEV_ENGINE_MPEG,
NVDEV_ENGINE_ME,

PPP got introduced when MPEG disappeared. There's likely a few other engines we can create as aliases for each other too. What do you think?

I'm not sure it's such a good idea. Suddenly nouveau_engctx_get(NVDEV_ENGINE_PPP, chan) can return nouveau_mpeg_chan and device->subdev[NVDEV_ENGINE_PPP] can point to nouveau_mpeg, etc. So in generic code you can't rely on device->subdev[X] being NULL on cards which don't have X engine...

Quoted 3 lines

This is true for non-engine subdevs, yes. But, the engines themselves should *never* *ever* be accessed from anything other than the object interface, from which it's impossible for a mix-up to happen.

what does this code (in this patch) do then? For me, it does exactly what you want to be forbidden.

What are we going to do when we'll need to look up something in engine specific data (e.g. nouveau_mpeg) to improve error reporting? We'll be screwed if NVDEV_ENGINE_PPP == NVDEV_ENGINE_MPEG.

Quoted 3 lines

If we do the aliasing this point should probably be documented in the enum list, and the nouveau_whatever() accessors removed.

But what's the point of all of this? Removal of one line from above list is pretty weak upside when there are so many downsides. Maybe I'm missing something obvious...

Quoted 94 lines

Quoted 94 lines

Quoted 9 lines

I can handle the aliasing if you like, but feel free :)

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c static const struct nouveau_enum vm_engine[] = { · line 335

Quoted 3 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +3 -0

+	{ 0x00000009, {NVDEV_ENGINE_BSP, 0} },
+	{ 0x0000000a, {NVDEV_ENGINE_CRYPT, 0} },
+	{ 0x0000000d, {NVDEV_ENGINE_COPY0, NVDEV_ENGINE_COPY1, 0} },

COPY1 doesn't exist on NV50. NVC0 has its own VM engine for the additional copy engines.

OK.

Quoted 22 lines

Quoted 22 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +6 -0

+};
+
 static const struct nouveau_enum vm_fault[] = {
 	{ 0x00000000, "PT_NOT_PRESENT", NULL },
 	{ 0x00000001, "PT_TOO_SHORT", NULL },

@@ -334,8 +352,12 @@ static void
 nv50_fb_intr(struct nouveau_subdev *subdev)
 {
 	struct nouveau_device *device = nv_device(subdev);
+	struct nouveau_engine *engine = NULL;
 	struct nv50_fb_priv *priv = (void *)subdev;
 	const struct nouveau_enum *en, *cl;
+	struct nouveau_object *engctx = NULL;
+	const int *poss_engines = NULL;
+	const char *client_name = "unk";
 	u32 trap[6], idx, chan;
 	u8 st0, st1, st2, st3;
 	int i;

@@ -366,9 +388,34 @@ nv50_fb_intr(struct nouveau_subdev *subdev)
 	}
 	chan = (trap[2] << 16) | trap[1];
 

Quoted 11 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c nv50_fb_intr(struct nouveau_subdev *subdev) · line 399

Quoted 10 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +9 -1

-	nv_error(priv, "trapped %s at 0x%02x%04x%04x on channel 0x%08x ",
+	for (i = 0; i < ARRAY_SIZE(nvdev_engine_for_vm_engine); ++i) {
+		if (nvdev_engine_for_vm_engine[i].value == st0) {
+			poss_engines = nvdev_engine_for_vm_engine[i].engines;
+			break;
+		}
+	}
+
+	for (i = 0; poss_engines && poss_engines[i]; ++i) {
+		engine = nv_engine(device->subdev[poss_engines[i]]);

engine = nouveau_engine(device, poss_engines[i]);

OK.

Quoted 2 lines

Perhaps you can even append another field to nouveau_enum to store the subdev index in too, rather than having to look it up?

Good idea. Thanks.

Quoted 41 lines

Quoted 41 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +18 -1

+		if (engine) {
+			engctx = nouveau_engctx_get(engine, chan);
+			if (engctx)
+				break;
+		}
+	}
+
+	if (engctx) {
+		struct nouveau_client *client = nouveau_client(engctx);
+		if (client)
+			client_name = client->name;
+	}
+
+	nv_error(priv, "trapped %s at 0x%02x%04x%04x on channel 0x%08x [%s] ",
 		 (trap[5] & 0x00000100) ? "read" : "write",
-		 trap[5] & 0xff, trap[4] & 0xffff, trap[3] & 0xffff, chan);
+		 trap[5] & 0xff, trap[4] & 0xffff, trap[3] & 0xffff, chan,
+		 client_name);
+
+	nouveau_engctx_put(engctx);
 
 	en = nouveau_enum_find(vm_engine, st0);
 	if (en)

drivers/gpu/drm/nouveau/nouveau_drm.c +3 -2

index 38e9a08..a50362e 100644
@@ -539,10 +539,11 @@ nouveau_drm_open(struct drm_device *dev, struct drm_file *fpriv)
 	struct pci_dev *pdev = dev->pdev;
 	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nouveau_cli *cli;
-	char name[16];
+	char name[32], tmpname[TASK_COMM_LEN];
 	int ret;
 
-	snprintf(name, sizeof(name), "%d", pid_nr(fpriv->pid));
+	get_task_comm(tmpname, current);
+	snprintf(name, sizeof(name), "%s[%d]", tmpname, pid_nr(fpriv->pid));
 
 	ret = nouveau_cli_create(pdev, name, sizeof(*cli), (void **)&cli);
 	if (ret)

Quoted 45 lines · Marcin Slusarz

Quoted 44 lines · Ben Skeggs

Quoted 44 lines

Quoted 33 lines

Quoted 21 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +6 -1

index 487cb8c..d1120fc 100644
@@ -22,8 +22,13 @@
  * Authors: Ben Skeggs
  */
 
-#include <core/object.h>
+#include <core/client.h>
 #include <core/enum.h>
+#include <core/engctx.h>
+#include <core/object.h>
+
+#include <engine/fifo.h>
+#include <engine/graph.h>
 
 #include <subdev/fb.h>
 #include <subdev/bios.h>

@@ -317,6 +322,19 @@ static const struct nouveau_enum vm_engine[] = {
 	{}

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c static const struct nouveau_enum vm_engine[] = { · line 332

Quoted 10 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +8 -0

 };
 
+static const struct nouveau_engine_map {
+	u32 value;
+	int engines[3];
+} nvdev_engine_for_vm_engine[] = {
+	{ 0x00000000, {NVDEV_ENGINE_GR, 0} },
+	{ 0x00000001, {NVDEV_ENGINE_VP, 0} },
+	{ 0x00000005, {NVDEV_ENGINE_FIFO, 0} },
+	{ 0x00000008, {NVDEV_ENGINE_PPP, NVDEV_ENGINE_MPEG, 0} },

I think it may actually be a good idea to go (in core/device.h):

Quoted 6 lines

NVDEV_ENGINE_MPEG,
+ NVDEV_ENGINE_PPP = NVDEV_ENGINE_MPEG,
NVDEV_ENGINE_ME,

PPP got introduced when MPEG disappeared. There's likely a few other engines we can create as aliases for each other too. What do you think?

I'm not sure it's such a good idea. Suddenly nouveau_engctx_get(NVDEV_ENGINE_PPP, chan) can return nouveau_mpeg_chan and device->subdev[NVDEV_ENGINE_PPP] can point to nouveau_mpeg, etc. So in generic code you can't rely on device->subdev[X] being NULL on cards which don't have X engine...

Quoted 6 lines

Quoted 3 lines

This is true for non-engine subdevs, yes. But, the engines themselves should *never* *ever* be accessed from anything other than the object interface, from which it's impossible for a mix-up to happen.

what does this code (in this patch) do then? For me, it does exactly what you want to be forbidden.

I probably wasn't very clear on what I meant, sorry about that. What I meant is that there should never be any need to access anything specific to a certain engine type from code that doesn't belong to that engine, *except* for what's exposed by the object interface.

So, accessing the nouveau_object/subdev/engine data is OK, accessing the nouveau_graph/copy/whatever should only be done by that module.

Really,
I should probably consider moving all the relevant structure definitions to a priv.h within the engine modules themselves.

Quoted 4 lines

What are we going to do when we'll need to look up something in engine specific data (e.g. nouveau_mpeg) to improve error reporting? We'll be screwed if NVDEV_ENGINE_PPP == NVDEV_ENGINE_MPEG.

What will we possibly need to do here? The error data we get is signalled via an interrupt directly to the specific module, which will indeed be able to access its own private information.

Quoted 102 lines

Quoted 3 lines

If we do the aliasing this point should probably be documented in the enum list, and the nouveau_whatever() accessors removed.

But what's the point of all of this? Removal of one line from above list is pretty weak upside when there are so many downsides. Maybe I'm missing something obvious...

Quoted 94 lines

Quoted 94 lines

Quoted 9 lines

I can handle the aliasing if you like, but feel free :)

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c static const struct nouveau_enum vm_engine[] = { · line 335

Quoted 3 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +3 -0

+	{ 0x00000009, {NVDEV_ENGINE_BSP, 0} },
+	{ 0x0000000a, {NVDEV_ENGINE_CRYPT, 0} },
+	{ 0x0000000d, {NVDEV_ENGINE_COPY0, NVDEV_ENGINE_COPY1, 0} },

COPY1 doesn't exist on NV50. NVC0 has its own VM engine for the additional copy engines.

OK.

Quoted 22 lines

Quoted 22 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +6 -0

+};
+
 static const struct nouveau_enum vm_fault[] = {
 	{ 0x00000000, "PT_NOT_PRESENT", NULL },
 	{ 0x00000001, "PT_TOO_SHORT", NULL },

@@ -334,8 +352,12 @@ static void
 nv50_fb_intr(struct nouveau_subdev *subdev)
 {
 	struct nouveau_device *device = nv_device(subdev);
+	struct nouveau_engine *engine = NULL;
 	struct nv50_fb_priv *priv = (void *)subdev;
 	const struct nouveau_enum *en, *cl;
+	struct nouveau_object *engctx = NULL;
+	const int *poss_engines = NULL;
+	const char *client_name = "unk";
 	u32 trap[6], idx, chan;
 	u8 st0, st1, st2, st3;
 	int i;

@@ -366,9 +388,34 @@ nv50_fb_intr(struct nouveau_subdev *subdev)
 	}
 	chan = (trap[2] << 16) | trap[1];
 

Quoted 11 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c nv50_fb_intr(struct nouveau_subdev *subdev) · line 399

Quoted 10 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +9 -1

-	nv_error(priv, "trapped %s at 0x%02x%04x%04x on channel 0x%08x ",
+	for (i = 0; i < ARRAY_SIZE(nvdev_engine_for_vm_engine); ++i) {
+		if (nvdev_engine_for_vm_engine[i].value == st0) {
+			poss_engines = nvdev_engine_for_vm_engine[i].engines;
+			break;
+		}
+	}
+
+	for (i = 0; poss_engines && poss_engines[i]; ++i) {
+		engine = nv_engine(device->subdev[poss_engines[i]]);

engine = nouveau_engine(device, poss_engines[i]);

OK.

Quoted 2 lines

Perhaps you can even append another field to nouveau_enum to store the subdev index in too, rather than having to look it up?

Good idea. Thanks.

Quoted 41 lines

Quoted 41 lines

drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c +18 -1

+		if (engine) {
+			engctx = nouveau_engctx_get(engine, chan);
+			if (engctx)
+				break;
+		}
+	}
+
+	if (engctx) {
+		struct nouveau_client *client = nouveau_client(engctx);
+		if (client)
+			client_name = client->name;
+	}
+
+	nv_error(priv, "trapped %s at 0x%02x%04x%04x on channel 0x%08x [%s] ",
 		 (trap[5] & 0x00000100) ? "read" : "write",
-		 trap[5] & 0xff, trap[4] & 0xffff, trap[3] & 0xffff, chan);
+		 trap[5] & 0xff, trap[4] & 0xffff, trap[3] & 0xffff, chan,
+		 client_name);
+
+	nouveau_engctx_put(engctx);
 
 	en = nouveau_enum_find(vm_engine, st0);
 	if (en)

drivers/gpu/drm/nouveau/nouveau_drm.c +3 -2

index 38e9a08..a50362e 100644
@@ -539,10 +539,11 @@ nouveau_drm_open(struct drm_device *dev, struct drm_file *fpriv)
 	struct pci_dev *pdev = dev->pdev;
 	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nouveau_cli *cli;
-	char name[16];
+	char name[32], tmpname[TASK_COMM_LEN];
 	int ret;
 
-	snprintf(name, sizeof(name), "%d", pid_nr(fpriv->pid));
+	get_task_comm(tmpname, current);
+	snprintf(name, sizeof(name), "%s[%d]", tmpname, pid_nr(fpriv->pid));
 
 	ret = nouveau_cli_create(pdev, name, sizeof(*cli), (void **)&cli);
 	if (ret)