drm/i915: Don't pin LRC in GGTT when dumping in debugfs
Subsystems (2)
PATCH 1/1 drm/i915: Don't pin LRC in GGTT when dumping in debugfs
LRC object does not need to be mapped into the GGTT when dumping. Just use pin_pages. A side-effect of this patch is that a compiler warning goes away (not checking return value of i915_gem_obj_ggtt_pin).
--- drivers/gpu/drm/i915/i915_debugfs.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)
drivers/gpu/drm/i915/i915_debugfs.c +12 -7
@@ -1801,25 +1801,30 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)struct page *page;uint32_t *reg_state;int j; +unsigned long ggtt_offset =0 ; -i915_gem_obj_ggtt_pin (ctx_obj, -GEN8_LR_CONTEXT_ALIGN ,0 ); - - page =i915_gem_object_get_page (ctx_obj,1 ); - reg_state =kmap_atomic (page); +i915_gem_object_pin_pages (ctx_obj);seq_printf (m,"CONTEXT: %s %u\n" , ring->name,intel_execlists_ctx_id (ctx_obj)); +if (!i915_gem_obj_ggtt_bound (ctx_obj)) +seq_puts (m,"\tNot bound in GGTT\n" ); +else + ggtt_offset =i915_gem_obj_ggtt_offset (ctx_obj); + + page =i915_gem_object_get_page (ctx_obj,1 ); + reg_state =kmap_atomic (page); +for (j =0 ; j <0x600 /sizeof (u32) /4 ; j +=4 ) {seq_printf (m,"\t[0x%08lx] 0x%08x 0x%08x 0x%08x 0x%08x\n" , -i915_gem_obj_ggtt_offset (ctx_obj) +4096 + (j *4 ), + ggtt_offset +4096 + (j *4 ), reg_state[j], reg_state[j +1 ], reg_state[j +2 ], reg_state[j +3 ]); }kunmap_atomic (reg_state); -i915_gem_object_ggtt_unpin (ctx_obj); +i915_gem_object_unpin_pages (ctx_obj);seq_putc (m,'\n' ); }
Signature · 6 lines
1.7.9.5 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 3 lines · Thomas Daniel
LRC object does not need to be mapped into the GGTT when dumping. Just use
pin_pages. A side-effect of this patch is that a compiler warning goes away (not checking return value ofi915_gem_obj_ggtt_pin).
Please explain why you need to pin the pages.
-Chris
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 8 lines
-----Original Message-----
From: Chris Wilson [mailto:[email protected]]
Sent: Thursday, November 20, 2014 11:16 AM
To: Daniel, Thomas
Cc: [email protected]; [email protected]
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfs
Quoted 6 lines
Quoted 3 lines · Thomas Daniel
LRC object does not need to be mapped into the GGTT when dumping. Just use
pin_pages. A side-effect of this patch is that a compiler warning goes away (not checking return value ofi915_gem_obj_ggtt_pin).Please explain why you need to pin the pages.
I suppose I don't as this is protected by the struct mutex and unpin is called before returning.
Thomas.
Quoted 4 lines
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 8 lines · Thomas
Quoted 8 lines
-----Original Message-----
From: Chris Wilson [mailto:[email protected]]
Sent: Thursday, November 20, 2014 11:16 AM
To: Daniel, Thomas
Cc: [email protected]; [email protected]
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfs
Quoted 7 lines
Quoted 6 lines · Thomas Daniel
Quoted 3 lines
LRC object does not need to be mapped into the GGTT when dumping. Just use
pin_pages. A side-effect of this patch is that a compiler warning goes away (not checking return value ofi915_gem_obj_ggtt_pin).Please explain why you need to pin the pages.
I suppose I don't as this is protected by the struct mutex and unpin is called before returning.
Hm right a simple call to i915_gem_object_get_pages should be all that's needed to get at the pages list. Sorry for the misadvise on the internal mail.
-Daniel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 8 lines · Thomas
Quoted 8 lines
-----Original Message-----
From: Chris Wilson [mailto:[email protected]]
Sent: Thursday, November 20, 2014 11:16 AM
To: Daniel, Thomas
Cc: [email protected]; [email protected]
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfs
Quoted 7 lines
Quoted 6 lines · Thomas Daniel
Quoted 3 lines
LRC object does not need to be mapped into the GGTT when dumping. Just use
pin_pages. A side-effect of this patch is that a compiler warning goes away (not checking return value ofi915_gem_obj_ggtt_pin).Please explain why you need to pin the pages.
I suppose I don't as this is protected by the struct mutex and unpin is called before returning.
The question is: do we need protection against kmalloc and a potential call into the shrinker who may steal the pages from underneath us. Here, we only do a seq_printf() under the lock after get_pages() and that uses a preallocated buffer.
-Chris
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 23 lines
-----Original Message-----
From: Chris Wilson [mailto:[email protected]]
Sent: Thursday, November 20, 2014 11:41 AM
To: Daniel, Thomas
Cc: [email protected]; akash goel ([email protected])
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfsQuoted 8 lines · Thomas
Quoted 8 lines
-----Original Message-----
From: Chris Wilson [mailto:[email protected]]
Sent: Thursday, November 20, 2014 11:16 AM
To: Daniel, Thomas
Cc: [email protected]; [email protected]
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfsQuoted 4 lines
Quoted 4 lines · Thomas Daniel
Quoted 3 lines
LRC object does not need to be mapped into the GGTT when dumping. Just use
pin_pages. A side-effect of this patch is that a compiler warning goes away (not checking return value of
i915_gem_obj_ggtt_pin).Quoted 1 line
Quoted 1 line
Quoted 8 lines
Quoted 2 lines
Quoted 1 line
Please explain why you need to pin the pages.
I suppose I don't as this is protected by the struct mutex and unpin is called
before returning.
The question is: do we need protection against kmalloc and a potential call into the shrinker who may steal the pages from underneath us. Here, we only do a
seq_printf()under the lock afterget_pages()and that uses a preallocated buffer.
I don't think so... If a context is in the context_list then the ctx_obj will have a nonzero refcount. The struct mutex prevents the refcount from changing.
Can you identify any situation where the pages may go away?
Thomas.
Quoted 4 lines
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 24 lines · Thomas
Quoted 24 lines
-----Original Message-----
From: Chris Wilson [mailto:[email protected]]
Sent: Thursday, November 20, 2014 11:41 AM
To: Daniel, Thomas
Cc: [email protected]; akash goel ([email protected])
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfsQuoted 8 lines · Thomas
Quoted 8 lines
-----Original Message-----
From: Chris Wilson [mailto:[email protected]]
Sent: Thursday, November 20, 2014 11:16 AM
To: Daniel, Thomas
Cc: [email protected]; [email protected]
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfsQuoted 4 lines
Quoted 4 lines · Thomas Daniel
Quoted 3 lines
LRC object does not need to be mapped into the GGTT when dumping. Just use
pin_pages. A side-effect of this patch is that a compiler warning goes away (not checking return value of
i915_gem_obj_ggtt_pin).Quoted 2 lines
Quoted 2 lines
Please explain why you need to pin the pages.
Quoted 8 lines
Quoted 7 lines
Quoted 1 line
I suppose I don't as this is protected by the struct mutex and unpin is called
before returning.
The question is: do we need protection against kmalloc and a potential call into the shrinker who may steal the pages from underneath us. Here, we only do a
seq_printf()under the lock afterget_pages()and that uses a preallocated buffer.I don't think so... If a context is in the
context_listthen thectx_objwill have a nonzero refcount. The struct mutex prevents the refcount from changing.
The shrinker only steals pages. Well it may reap the active reference count as well, which must also be kept in mind, but the ctx_obj should have the reference from being inside the list and be safe.
Quoted 1 line
Can you identify any situation where the pages may go away?
Anytime you trigger an allocation, the system may reap any objects pages. It will even steal the dev->struct_mutex. To protect against the shrinker you have to call pin_pages(). Here, there are no allocations inside the loop and so you don't need to worry about the shrinker stealing your pages.
-Chris
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 7 lines · Chris Wilson
Quoted 1 line
Can you identify any situation where the pages may go away?
Anytime you trigger an allocation, the system may reap any objects pages. It will even steal the
dev->struct_mutex. To protect against the shrinker you have to callpin_pages(). Here, there are no allocations inside the loop and so you don't need to worry about the shrinker stealing your pages.
Hm actually I think better safe than sorry here. At least I have (again) completely forgotten about our dear shrinker ... -Daniel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 19 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Thursday, November 20, 2014 12:51 PM
To: Chris Wilson; Daniel, Thomas; [email protected]; akash goel ([email protected])
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfsQuoted 7 lines · Chris Wilson
Quoted 1 line
Can you identify any situation where the pages may go away?
Anytime you trigger an allocation, the system may reap any objects pages. It will even steal the
dev->struct_mutex. To protect against the shrinker you have to callpin_pages(). Here, there are no allocations inside the loop and so you don't need to worry about the shrinker stealing your pages.
Quoted 3 lines
Hm actually I think better safe than sorry here. At least I have (again) completely forgotten about our dear shrinker ... -Daniel
Does this discussion count as a review? What was the conclusion - do I need to make a version without pinning or is it better safe than sorry?
Cheers,
Thomas.
Quoted 4 lines
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 19 lines · Thomas
Quoted 19 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Thursday, November 20, 2014 12:51 PM
To: Chris Wilson; Daniel, Thomas; [email protected]; akash goel ([email protected])
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfsQuoted 7 lines · Chris Wilson
Quoted 1 line
Can you identify any situation where the pages may go away?
Anytime you trigger an allocation, the system may reap any objects pages. It will even steal the
dev->struct_mutex. To protect against the shrinker you have to callpin_pages(). Here, there are no allocations inside the loop and so you don't need to worry about the shrinker stealing your pages.
Quoted 5 lines
Quoted 3 lines
Hm actually I think better safe than sorry here. At least I have (again) completely forgotten about our dear shrinker ... -Daniel
Does this discussion count as a review? What was the conclusion - do I need to make a version without pinning or is it better safe than sorry?
To bring it full circle:
Quoted 5 lines
Quoted 3 lines
LRC object does not need to be mapped into the GGTT when dumping. Just use
pin_pages. A side-effect of this patch is that a compiler warning goes away (not checking return value ofi915_gem_obj_ggtt_pin).Please explain why you need to pin the pages.
In particular, explain why just calling pin_pages() doesn't do what you want. And then afterwards you can leave a note in the commitlog why you use pin_pages() as overkill.
-Chris
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 32 lines · Chris Wilson
Quoted 19 lines · Thomas
Quoted 19 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Thursday, November 20, 2014 12:51 PM
To: Chris Wilson; Daniel, Thomas; [email protected]; akash goel ([email protected])
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfsQuoted 7 lines · Chris Wilson
Quoted 1 line
Can you identify any situation where the pages may go away?
Anytime you trigger an allocation, the system may reap any objects pages. It will even steal the
dev->struct_mutex. To protect against the shrinker you have to callpin_pages(). Here, there are no allocations inside the loop and so you don't need to worry about the shrinker stealing your pages.Quoted 5 lines
Quoted 3 lines
Hm actually I think better safe than sorry here. At least I have (again) completely forgotten about our dear shrinker ... -Daniel
Does this discussion count as a review? What was the conclusion - do I need to make a version without pinning or is it better safe than sorry?
To bring it full circle:
Quoted 3 lines
Quoted 3 lines
LRC object does not need to be mapped into the GGTT when dumping. Just use
pin_pages. A side-effect of this patch is that a compiler warning goes away (not checking return value ofi915_gem_obj_ggtt_pin).
Quoted 5 lines
Quoted 1 line
Please explain why you need to pin the pages.
In particular, explain why just calling
pin_pages()doesn't do what you want. And then afterwards you can leave a note in the commitlog why you usepin_pages()as overkill.
Ok I think I see the confusion - we still don't have any error checking, because we don't call get_pages. pin_pages alone doesn't do a whole lot really. We might actually want to put the get_pages into the pin_pages to simplify the interface a bit. Well we need to keep the raw version as __ since some users really don't want a get_pages.
-Daniel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 52 lines
-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Tuesday, November 25, 2014 4:00 PM
To: Chris Wilson; Daniel, Thomas; Daniel Vetter; intel- [email protected]; akash goel ([email protected])
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfsQuoted 22 lines · Chris Wilson
Quoted 21 lines · Thomas
Quoted 21 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Thursday, November 20, 2014 12:51 PM
To: Chris Wilson; Daniel, Thomas; [email protected]; akash goel
([email protected])
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfsQuoted 7 lines · Chris Wilson
Quoted 1 line
Can you identify any situation where the pages may go away?
Anytime you trigger an allocation, the system may reap any objects pages. It will even steal the
dev->struct_mutex. To protect against the shrinker you have to callpin_pages(). Here, there are no allocations inside the loop and so you don't need to worry about the shrinker stealing your pages.Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
Hm actually I think better safe than sorry here. At least I have (again) completely forgotten about our dear shrinker ... -Daniel
Does this discussion count as a review? What was the conclusion - do I
need to make a version without pinning or is it better safe than sorry?
Quoted 6 lines
To bring it full circle:
Quoted 3 lines
Quoted 3 lines
LRC object does not need to be mapped into the GGTT when dumping. Just use
pin_pages. A side-effect of this patch is that a compiler warning goes away (not checking return value of
i915_gem_obj_ggtt_pin).Quoted 6 lines
Quoted 2 lines
Please explain why you need to pin the pages.
In particular, explain why just calling
pin_pages()doesn't do what you want. And then afterwards you can leave a note in the commitlog why you usepin_pages()as overkill.
Quoted 6 lines
Ok I think I see the confusion - we still don't have any error checking, because we don't call
get_pages.pin_pagesalone doesn't do a whole lot really. We might actually want to put theget_pagesinto thepin_pagesto simplify the interface a bit. Well we need to keep the raw version as __ since some users really don't want aget_pages.
-Daniel
Well I'm more confused now. Pin_pages() just increments a refcount. There should already be backing store allocated as the context is in the dev_priv->context_list and we have the struct_mutex. The code calls i915_gem_object_get_page to get a pointer to page 1 of the context object (the logical ring context is here), then kmaps it. What is the benefit of calling get_pages? Do you mean i915_gem_object_ops.get_pages? I don't care if the object is mapped into GTT at this point - I just want to dump it from the CPU. That's why I removed the ggtt_pin in the first place.
Thomas.
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 52 lines · Thomas
Quoted 52 lines
-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Tuesday, November 25, 2014 4:00 PM
To: Chris Wilson; Daniel, Thomas; Daniel Vetter; intel- [email protected]; akash goel ([email protected])
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfsQuoted 22 lines · Chris Wilson
Quoted 21 lines · Thomas
Quoted 21 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Thursday, November 20, 2014 12:51 PM
To: Chris Wilson; Daniel, Thomas; [email protected]; akash goel
([email protected])
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfsQuoted 7 lines · Chris Wilson
Quoted 1 line
Can you identify any situation where the pages may go away?
Anytime you trigger an allocation, the system may reap any objects pages. It will even steal the
dev->struct_mutex. To protect against the shrinker you have to callpin_pages(). Here, there are no allocations inside the loop and so you don't need to worry about the shrinker stealing your pages.Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
Hm actually I think better safe than sorry here. At least I have (again) completely forgotten about our dear shrinker ... -Daniel
Does this discussion count as a review? What was the conclusion - do I
need to make a version without pinning or is it better safe than sorry?
Quoted 6 lines
To bring it full circle:
Quoted 3 lines
Quoted 3 lines
LRC object does not need to be mapped into the GGTT when dumping. Just use
pin_pages. A side-effect of this patch is that a compiler warning goes away (not checking return value of
i915_gem_obj_ggtt_pin).Quoted 6 lines
Quoted 2 lines
Please explain why you need to pin the pages.
In particular, explain why just calling
pin_pages()doesn't do what you want. And then afterwards you can leave a note in the commitlog why you usepin_pages()as overkill.
Quoted 7 lines
Quoted 6 lines
Ok I think I see the confusion - we still don't have any error checking, because we don't call
get_pages.pin_pagesalone doesn't do a whole lot really. We might actually want to put theget_pagesinto thepin_pagesto simplify the interface a bit. Well we need to keep the raw version as __ since some users really don't want aget_pages.
-DanielWell I'm more confused now.
Pin_pages()just increments a refcount. There should already be backing store allocated as the context is in thedev_priv->context_listand we have thestruct_mutex. The code callsi915_gem_object_get_pageto get a pointer to page 1 of the context object (the logical ring context is here), then kmaps it. What is the benefit of callingget_pages? Do you mean i915_gem_object_ops.get_pages? I don't care if the object is mapped into GTT at this point - I just want to dump it from the CPU. That's why I removed theggtt_pinin the first place.
The object does not necessarily have any obj->pages at this point. i915_gem_object_get_pages() returns NULL and the code OOPSes. -Chris
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 59 lines · Chris Wilson
Quoted 58 lines · Thomas
Quoted 58 lines
-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Tuesday, November 25, 2014 4:00 PM
To: Chris Wilson; Daniel, Thomas; Daniel Vetter; intel- [email protected]; akash goel ([email protected])
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfsQuoted 22 lines · Chris Wilson
Quoted 21 lines · Thomas
Quoted 21 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Thursday, November 20, 2014 12:51 PM
To: Chris Wilson; Daniel, Thomas; [email protected]; akash goel
([email protected])
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfsQuoted 7 lines · Chris Wilson
Quoted 1 line
Can you identify any situation where the pages may go away?
Anytime you trigger an allocation, the system may reap any objects pages. It will even steal the
dev->struct_mutex. To protect against the shrinker you have to callpin_pages(). Here, there are no allocations inside the loop and so you don't need to worry about the shrinker stealing your pages.Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
Hm actually I think better safe than sorry here. At least I have (again) completely forgotten about our dear shrinker ... -Daniel
Does this discussion count as a review? What was the conclusion - do I
need to make a version without pinning or is it better safe than sorry?
Quoted 6 lines
To bring it full circle:
Quoted 3 lines
Quoted 3 lines
LRC object does not need to be mapped into the GGTT when dumping. Just use
pin_pages. A side-effect of this patch is that a compiler warning goes away (not checking return value of
i915_gem_obj_ggtt_pin).Quoted 6 lines
Quoted 2 lines
Please explain why you need to pin the pages.
In particular, explain why just calling
pin_pages()doesn't do what you want. And then afterwards you can leave a note in the commitlog why you usepin_pages()as overkill.Ok I think I see the confusion - we still don't have any error checking, because we don't call
get_pages.pin_pagesalone doesn't do a whole lot really. We might actually want to put theget_pagesinto thepin_pagesto simplify the interface a bit. Well we need to keep the raw version as __ since some users really don't want aget_pages.
-Daniel
Quoted 4 lines
Quoted 1 line
Well I'm more confused now.
Pin_pages()just increments a refcount. There should already be backing store allocated as the context is in thedev_priv->context_listand we have thestruct_mutex. The code callsi915_gem_object_get_pageto get a pointer to page 1 of the context object (the logical ring context is here), then kmaps it. What is the benefit of callingget_pages? Do you mean i915_gem_object_ops.get_pages? I don't care if the object is mapped into GTT at this point - I just want to dump it from the CPU. That's why I removed theggtt_pinin the first place.The object does not necessarily have any
obj->pagesat this point.i915_gem_object_get_pages()returns NULL and the code OOPSes.
The following sequence should be enough to reproduce this:
1. Allocate a few contexts, use them.
2. Thrash memory by allocating&using enough gem buffer objects to hit swap (allocate 1MB objects until you have enough to fill main memory, touch each through gtt mmaps).
3. Read debugfs.
Presuming Chris&I are not off the mark that should result in an oops with your patch. With the old code it'll only result in an oops if you also interrupt the process while doing 3 (which means ggtt pinning gets aborted with -EINTR). Can you please do a quick igt?
Thanks, Daniel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 1/1 drm/i915: Don't pin LRC in GGTT when dumping in debugfs
In reply to Thomas DanielLRC object does not need to be mapped into the GGTT when dumping. A side-effect of this patch is that a compiler warning goes away (not checking return value of i915_gem_obj_ggtt_pin).
v2: Broke out individual context dumping into a new function as the indentation was getting a bit crazy. Added notification of contexts with no gem object for debugging purposes. Removed unnecessary pin_pages and unpin_pages, replaced with explicit get_pages for the context object as there may be no backing store allocated at this time (Comment for get_pages says "Ensure that the associated pages are gathered from the backing storage and pinned into our object"). Improved error checking - get_pages and get_page are checked for failure.
--- drivers/gpu/drm/i915/i915_debugfs.c | 84 ++++++++++++++++++++--------------- 1 file changed, 49 insertions(+), 35 deletions(-)
drivers/gpu/drm/i915/i915_debugfs.c +49 -35
@@ -1773,6 +1773,50 @@ static int i915_context_status(struct seq_file *m, void *unused)return 0 ; } +static void i915_dump_lrc_obj (struct seq_file *m, +struct intel_engine_cs *ring, +struct drm_i915_gem_object *ctx_obj) +{ +struct page *page; +uint32_t *reg_state; +int j; +unsigned long ggtt_offset =0 ; + +if (ctx_obj ==NULL ) { +seq_printf (m,"Context on %s with no gem object\n" , + ring->name); +return ; + } + +seq_printf (m,"CONTEXT: %s %u\n" , ring->name, +intel_execlists_ctx_id (ctx_obj)); + +if (!i915_gem_obj_ggtt_bound (ctx_obj)) +seq_puts (m,"\tNot bound in GGTT\n" ); +else + ggtt_offset =i915_gem_obj_ggtt_offset (ctx_obj); + +if (i915_gem_object_get_pages (ctx_obj)) { +seq_puts (m,"\tFailed to get pages for context object\n" ); +return ; + } + + page =i915_gem_object_get_page (ctx_obj,1 ); +if (!WARN_ON (page ==NULL )) { + reg_state =kmap_atomic (page); + +for (j =0 ; j <0x600 /sizeof (u32) /4 ; j +=4 ) { +seq_printf (m,"\t[0x%08lx] 0x%08x 0x%08x 0x%08x 0x%08x\n" , + ggtt_offset +4096 + (j *4 ), + reg_state[j], reg_state[j +1 ], + reg_state[j +2 ], reg_state[j +3 ]); + } +kunmap_atomic (reg_state); + } + +seq_putc (m,'\n' ); +} +static int i915_dump_lrc (struct seq_file *m,void *unused) {struct drm_info_node *node = (struct drm_info_node *) m->private;
@@ -1791,41 +1835,11 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)if (ret)return ret; -list_for_each_entry (ctx, &dev_priv->context_list, link) { -for_each_ring (ring, dev_priv, i) { -struct drm_i915_gem_object *ctx_obj = ctx->engine[i].state; - -if (ring->default_context == ctx) -continue ; - -if (ctx_obj) { -struct page *page; -uint32_t *reg_state; -int j; - -i915_gem_obj_ggtt_pin (ctx_obj, -GEN8_LR_CONTEXT_ALIGN ,0 ); - - page =i915_gem_object_get_page (ctx_obj,1 ); - reg_state =kmap_atomic (page); - -seq_printf (m,"CONTEXT: %s %u\n" , ring->name, -intel_execlists_ctx_id (ctx_obj)); - -for (j =0 ; j <0x600 /sizeof (u32) /4 ; j +=4 ) { -seq_printf (m,"\t[0x%08lx] 0x%08x 0x%08x 0x%08x 0x%08x\n" , -i915_gem_obj_ggtt_offset (ctx_obj) +4096 + (j *4 ), - reg_state[j], reg_state[j +1 ], - reg_state[j +2 ], reg_state[j +3 ]); - } -kunmap_atomic (reg_state); - -i915_gem_object_ggtt_unpin (ctx_obj); - -seq_putc (m,'\n' ); - } - } - } +list_for_each_entry (ctx, &dev_priv->context_list, link) +for_each_ring (ring, dev_priv, i) +if (ring->default_context != ctx) +i915_dump_lrc_obj (m, ring, + ctx->engine[i].state);mutex_unlock (&dev->struct_mutex);
Signature · 6 lines
1.7.9.5 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter
AppliedQuoted 11 lines · Thomas Daniel
LRC object does not need to be mapped into the GGTT when dumping. A side-effect of this patch is that a compiler warning goes away (not checking return value of
i915_gem_obj_ggtt_pin).v2: Broke out individual context dumping into a new function as the indentation was getting a bit crazy. Added notification of contexts with no gem object for debugging purposes. Removed unnecessary
pin_pagesandunpin_pages, replaced with explicitget_pagesfor the context object as there may be no backing store allocated at this time (Comment forget_pagessays "Ensure that the associated pages are gathered from the backing storage and pinned into our object"). Improved error checking -get_pagesandget_pageare checked for failure.
The comment that get_pages pins the backing storage is outdated btw - at every point where the shrinker might kick in (any memory allocation really) you might loose the backing storage again. Hence why we have the pin/unpin_pages functions. But like Chris said this isn't actually required here. Care for a patch to update that comment?
Quoted 11 lines
Signed-off-by Thomas Daniel <[email protected]>--- drivers/gpu/drm/i915/i915_debugfs.c | 84 ++++++++++++++++++++--------------- 1 file changed, 49 insertions(+), 35 deletions(-)drivers/gpu/drm/i915/i915_debugfs.c +0
-0
drivers/gpu/drm/i915/i915_debugfs.c static int i915_context_status(struct seq_file *m, void *unused) · line 1778
Quoted 6 lines
drivers/gpu/drm/i915/i915_debugfs.c +3
-0@@ -1773,6 +1773,50 @@ static int i915_context_status(struct seq_file *m, void *unused)return 0 ; } +static void i915_dump_lrc_obj (struct seq_file *m, +struct intel_engine_cs *ring, +struct drm_i915_gem_object *ctx_obj)
Please align parameter continuation lines to the opening ( since that's the usual style we use in i915. Not doing that tends to look pretty jarring. checkpatch --strict will look for these (and a few other things I tend to ignore, so please use with caution).
Quoted 73 lines
drivers/gpu/drm/i915/i915_debugfs.c +41
-25+{ +struct page *page; +uint32_t *reg_state; +int j; +unsigned long ggtt_offset =0 ; + +if (ctx_obj ==NULL ) { +seq_printf (m,"Context on %s with no gem object\n" , + ring->name); +return ; + } + +seq_printf (m,"CONTEXT: %s %u\n" , ring->name, +intel_execlists_ctx_id (ctx_obj)); + +if (!i915_gem_obj_ggtt_bound (ctx_obj)) +seq_puts (m,"\tNot bound in GGTT\n" ); +else + ggtt_offset =i915_gem_obj_ggtt_offset (ctx_obj); + +if (i915_gem_object_get_pages (ctx_obj)) { +seq_puts (m,"\tFailed to get pages for context object\n" ); +return ; + } + + page =i915_gem_object_get_page (ctx_obj,1 ); +if (!WARN_ON (page ==NULL )) { + reg_state =kmap_atomic (page); + +for (j =0 ; j <0x600 /sizeof (u32) /4 ; j +=4 ) { +seq_printf (m,"\t[0x%08lx] 0x%08x 0x%08x 0x%08x 0x%08x\n" , + ggtt_offset +4096 + (j *4 ), + reg_state[j], reg_state[j +1 ], + reg_state[j +2 ], reg_state[j +3 ]); + } +kunmap_atomic (reg_state); + } + +seq_putc (m,'\n' ); +} +static int i915_dump_lrc (struct seq_file *m,void *unused) {struct drm_info_node *node = (struct drm_info_node *) m->private;@@ -1791,41 +1835,11 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)if (ret)return ret; -list_for_each_entry (ctx, &dev_priv->context_list, link) { -for_each_ring (ring, dev_priv, i) { -struct drm_i915_gem_object *ctx_obj = ctx->engine[i].state; - -if (ring->default_context == ctx) -continue ; - -if (ctx_obj) { -struct page *page; -uint32_t *reg_state; -int j; - -i915_gem_obj_ggtt_pin (ctx_obj, -GEN8_LR_CONTEXT_ALIGN ,0 ); - - page =i915_gem_object_get_page (ctx_obj,1 ); - reg_state =kmap_atomic (page); - -seq_printf (m,"CONTEXT: %s %u\n" , ring->name, -intel_execlists_ctx_id (ctx_obj)); - -for (j =0 ; j <0x600 /sizeof (u32) /4 ; j +=4 ) { - seq_printf(m, "\t[0x%08lx] 0x%08x 0x%08x 0x%08x 0x%08x\n", - i915_gem_obj_ggtt_offset(ctx_obj) + 4096 + (j * 4), - reg_state[j], reg_state[j + 1],
drivers/gpu/drm/i915/i915_debugfs.c static int i915_dump_lrc(struct seq_file *m, void *unused) · line 1828
Quoted 10 lines
drivers/gpu/drm/i915/i915_debugfs.c +0
-10- reg_state[j + 2], reg_state[j + 3]); - } -kunmap_atomic (reg_state); - -i915_gem_object_ggtt_unpin (ctx_obj); - -seq_putc (m,'\n' ); - } - } - }
I've added back some of the braces here for readability of this nested loops - I got a bit confused ;-)
Queued for -next, thanks for the patch.
-Daniel
Quoted 15 lines
drivers/gpu/drm/i915/i915_debugfs.c +5
-0+list_for_each_entry (ctx, &dev_priv->context_list, link) +for_each_ring (ring, dev_priv, i) +if (ring->default_context != ctx) +i915_dump_lrc_obj (m, ring, + ctx->engine[i].state);mutex_unlock (&dev->struct_mutex);Signature · 6 lines
1.7.9.5 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV -4 364/364 360/364
ILK +1-6 365/366 360/366
SNB 450/450 450/450 IVB 498/498 498/498 BYT 289/289 289/289 HSW 564/564 564/564 BDW 417/417 417/417 -------------------------------------Detailed------------------------------------- Platform Test drm-intel-nightly Series Applied *PNV igt_gem_concurrent_blit_gpuX-bcs-early-read PASS(2, M25) NO_RESULT(1, M25) *PNV igt_gem_concurrent_blit_prw-bcs-overwrite-source-interruptible PASS(2, M25) NO_RESULT(1, M25) *PNV igt_gem_userptr_blits_coherency-sync PASS(2, M25) NO_RESULT(1, M25) *PNV igt_kms_sysfs_edid_timing PASS(3, M25M7) FAIL(1, M25) *ILK igt_kms_flip_rcs-flip-vs-panning-interruptible PASS(3, M26) DMESG_WARN(1, M26) *ILK igt_kms_render_direct-render PASS(3, M26) DMESG_WARN(1, M26) ILK igt_kms_flip_bcs-flip-vs-modeset-interruptible DMESG_WARN(1, M26)PASS(6, M26) DMESG_WARN(1, M26) *ILK igt_kms_flip_blocking-absolute-wf_vblank-interruptible NSPT(1, M26)PASS(2, M26) DMESG_WARN(1, M26) ILK igt_kms_flip_busy-flip-interruptible DMESG_WARN(1, M26)PASS(5, M26) DMESG_WARN(1, M26) *ILK igt_kms_flip_rcs-flip-vs-dpms PASS(4, M26) DMESG_WARN(1, M26) ILK igt_kms_flip_wf_vblank-ts-check DMESG_WARN(2, M26)PASS(11, M26M37) PASS(1, M26)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list [email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx