Re: [Nouveau] [RFC PATCH] drm/nouveau: report channel owner in error messages
On Sun, Dec 09, 2012 at 12:04:54PM +0100, Marcin Slusarz wrote:
On Sun, Dec 09, 2012 at 02:48:49PM +1000, Ben Skeggs wrote:
diff --git a/drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c
b/drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c
index 487cb8c..d1120fc 100644
--- a/drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c
+++ b/drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c
@@ -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[] = {
{}
};
+static const struct nouveau_engine_map {
+ u32 value;
+ int engines[3];
+} nvdev_engine_for_vm_engine[] = {
+ { 0x, {NVDEV_ENGINE_GR, 0} },
+ { 0x0001, {NVDEV_ENGINE_VP, 0} },
+ { 0x0005, {NVDEV_ENGINE_FIFO, 0} },
+ { 0x0008, {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'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.
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.
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.
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...
I can handle the aliasing if you like, but feel free :)
+ { 0x0009, {NVDEV_ENGINE_BSP, 0} },
+ { 0x000a, {NVDEV_ENGINE_CRYPT, 0} },
+ { 0x000d, {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.
+};
+
static const struct nouveau_enum vm_fault[] = {
{ 0x, PT_NOT_PRESENT, NULL },
{ 0x0001, 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];
- 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
Re: [Nouveau] [RFC PATCH] drm/nouveau: report channel owner in error messages
On Mon, Dec 10, 2012 at 06:47:41PM +1000, Ben Skeggs wrote:
On Sun, Dec 09, 2012 at 12:04:54PM +0100, Marcin Slusarz wrote:
On Sun, Dec 09, 2012 at 02:48:49PM +1000, Ben Skeggs wrote:
diff --git a/drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c
b/drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c
index 487cb8c..d1120fc 100644
--- a/drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c
+++ b/drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c
@@ -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[] =
{
{}
};
+static const struct nouveau_engine_map {
+ u32 value;
+ int engines[3];
+} nvdev_engine_for_vm_engine[] = {
+ { 0x, {NVDEV_ENGINE_GR, 0} },
+ { 0x0001, {NVDEV_ENGINE_VP, 0} },
+ { 0x0005, {NVDEV_ENGINE_FIFO, 0} },
+ { 0x0008, {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'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.
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.
Thanks, I see your point now (and I agree it's valuable goal).
Really,
I should probably consider moving all the relevant structure definitions
to a priv.h within the engine modules themselves.
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.
Here, interrupts triggered by various engines are delivered to fb subdev, so
if we'll ever need to e.g. dump registers related to one of those engines
(or look up something in all contexts of said engine, or...), it will be much
harder. Sure, we can add another function pointer to nouveau_engine (or abuse
one of nouveau_ofuncs ;), but it feels a bit hacky...
For now it's only theoretical problem (we don't need engine specific data here),
but I think it's better to leave it as an option if possible. And I don't see
what can be improved by aliasing PPP to MPEG...
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...
I can handle the aliasing if you like, but feel free :)
+ { 0x0009, {NVDEV_ENGINE_BSP, 0} },
+ { 0x000a, {NVDEV_ENGINE_CRYPT, 0} },
+ { 0x000d, {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.
+};
+
static const struct nouveau_enum vm_fault[] = {
{ 0x, PT_NOT_PRESENT, NULL },
{ 0x0001, 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,
Re: [Nouveau] [RFC PATCH] drm/nouveau: report channel owner in error messages
On Sun, Dec 09, 2012 at 02:48:49PM +1000, Ben Skeggs wrote:
diff --git a/drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c
b/drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c
index 487cb8c..d1120fc 100644
--- a/drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c
+++ b/drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c
@@ -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[] = {
{}
};
+static const struct nouveau_engine_map {
+ u32 value;
+ int engines[3];
+} nvdev_engine_for_vm_engine[] = {
+ { 0x, {NVDEV_ENGINE_GR, 0} },
+ { 0x0001, {NVDEV_ENGINE_VP, 0} },
+ { 0x0005, {NVDEV_ENGINE_FIFO, 0} },
+ { 0x0008, {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'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.
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.
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...
I can handle the aliasing if you like, but feel free :)
+ { 0x0009, {NVDEV_ENGINE_BSP, 0} },
+ { 0x000a, {NVDEV_ENGINE_CRYPT, 0} },
+ { 0x000d, {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.
+};
+
static const struct nouveau_enum vm_fault[] = {
{ 0x, PT_NOT_PRESENT, NULL },
{ 0x0001, 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];
- 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.
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.
+ 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] ,
Re: [Nouveau] [RFC PATCH] drm/nouveau: report channel owner in error messages
On Fri, Dec 07, 2012 at 02:46:53PM +1000, Ben Skeggs wrote:
On Wed, Dec 05, 2012 at 11:56:22PM +0100, Marcin Slusarz wrote:
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 marcin.slus...@gmail.com
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.
(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(-)
diff --git a/drivers/gpu/drm/nouveau/core/engine/fifo/nv04.c
b/drivers/gpu/drm/nouveau/core/engine/fifo/nv04.c
index 76944c4..f5d4d28 100644
--- a/drivers/gpu/drm/nouveau/core/engine/fifo/nv04.c
+++ b/drivers/gpu/drm/nouveau/core/engine/fifo/nv04.c
@@ -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, 0x);
@@ -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,
Re: [Nouveau] [RFC PATCH] drm/nouveau: report channel owner in error messages
On Sun, Dec 09, 2012 at 12:26:42AM +0100, Marcin Slusarz wrote:
On Fri, Dec 07, 2012 at 02:46:53PM +1000, Ben Skeggs wrote:
On Wed, Dec 05, 2012 at 11:56:22PM +0100, Marcin Slusarz wrote:
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 marcin.slus...@gmail.com
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.
(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(-)
diff --git a/drivers/gpu/drm/nouveau/core/engine/fifo/nv04.c
b/drivers/gpu/drm/nouveau/core/engine/fifo/nv04.c
index 76944c4..f5d4d28 100644
--- a/drivers/gpu/drm/nouveau/core/engine/fifo/nv04.c
+++ b/drivers/gpu/drm/nouveau/core/engine/fifo/nv04.c
@@ -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, 0x);
@@ -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)
[Nouveau] [RFC PATCH] 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 marcin.slus...@gmail.com
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(-)
diff --git a/drivers/gpu/drm/nouveau/core/engine/fifo/nv04.c
b/drivers/gpu/drm/nouveau/core/engine/fifo/nv04.c
index 76944c4..f5d4d28 100644
--- a/drivers/gpu/drm/nouveau/core/engine/fifo/nv04.c
+++ b/drivers/gpu/drm/nouveau/core/engine/fifo/nv04.c
@@ -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, 0x);
@@ -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);
diff --git a/drivers/gpu/drm/nouveau/core/engine/graph/nv50.c
b/drivers/gpu/drm/nouveau/core/engine/graph/nv50.c
index
Re: [Nouveau] [RFC PATCH] drm/nouveau: report channel owner in error messages
Op 05-12-12 23:56, Marcin Slusarz schreef: 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 marcin.slus...@gmail.com Comments? Ideas? (This commit depends on this one: http://people.freedesktop.org/~mslusarz/0001-drm-nouveau-split-fifo-interrupt-handler.patch ) I love the idea, this has been something that nouveau lacked for a long time and would make error reporting a lot more useful. ~Maarten ___ Nouveau mailing list Nouveau@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/nouveau
