[PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfs

[PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfs

[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 of i915_gem_obj_ggtt_pin).

Signed-off-by: Thomas Daniel <thomas.daniel@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c |   19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index f91e7f7..7e1e7f7 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -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');
 			}
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfs

[Chris Wilson]

On Thu, Nov 20, 2014 at 11:12:05AM +0000, Thomas Daniel wrote:
> 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).

Please explain why you need to pin the pages.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfs

[Daniel, Thomas]

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Thursday, November 20, 2014 11:16 AM
> To: Daniel, Thomas
> Cc: intel-gfx@lists.freedesktop.org; akash-goels@gmail.com
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when
> dumping in debugfs
> 
> On Thu, Nov 20, 2014 at 11:12:05AM +0000, Thomas Daniel wrote:
> > 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).
> 
> 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.

> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfs

[Daniel Vetter]

On Thu, Nov 20, 2014 at 12:25 PM, Daniel, Thomas
<thomas.daniel@intel.com> wrote:
>> -----Original Message-----
>> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
>> Sent: Thursday, November 20, 2014 11:16 AM
>> To: Daniel, Thomas
>> Cc: intel-gfx@lists.freedesktop.org; akash-goels@gmail.com
>> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when
>> dumping in debugfs
>>
>> On Thu, Nov 20, 2014 at 11:12:05AM +0000, Thomas Daniel wrote:
>> > 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).
>>
>> 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfs

[Chris Wilson]

On Thu, Nov 20, 2014 at 11:25:57AM +0000, Daniel, Thomas wrote:
> > -----Original Message-----
> > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > Sent: Thursday, November 20, 2014 11:16 AM
> > To: Daniel, Thomas
> > Cc: intel-gfx@lists.freedesktop.org; akash-goels@gmail.com
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when
> > dumping in debugfs
> > 
> > On Thu, Nov 20, 2014 at 11:12:05AM +0000, Thomas Daniel wrote:
> > > 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).
> > 
> > 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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfs

[Daniel, Thomas]

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Thursday, November 20, 2014 11:41 AM
> To: Daniel, Thomas
> Cc: intel-gfx@lists.freedesktop.org; akash goel (akash.goels@gmail.com)
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when
> dumping in debugfs
> 
> On Thu, Nov 20, 2014 at 11:25:57AM +0000, Daniel, Thomas wrote:
> > > -----Original Message-----
> > > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > > Sent: Thursday, November 20, 2014 11:16 AM
> > > To: Daniel, Thomas
> > > Cc: intel-gfx@lists.freedesktop.org; akash-goels@gmail.com
> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT
> > > when dumping in debugfs
> > >
> > > On Thu, Nov 20, 2014 at 11:12:05AM +0000, Thomas Daniel wrote:
> > > > 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).
> > >
> > > 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.
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.

> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfs

[Chris Wilson]

On Thu, Nov 20, 2014 at 12:09:18PM +0000, Daniel, Thomas wrote:
> > -----Original Message-----
> > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > Sent: Thursday, November 20, 2014 11:41 AM
> > To: Daniel, Thomas
> > Cc: intel-gfx@lists.freedesktop.org; akash goel (akash.goels@gmail.com)
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when
> > dumping in debugfs
> > 
> > On Thu, Nov 20, 2014 at 11:25:57AM +0000, Daniel, Thomas wrote:
> > > > -----Original Message-----
> > > > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > > > Sent: Thursday, November 20, 2014 11:16 AM
> > > > To: Daniel, Thomas
> > > > Cc: intel-gfx@lists.freedesktop.org; akash-goels@gmail.com
> > > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT
> > > > when dumping in debugfs
> > > >
> > > > On Thu, Nov 20, 2014 at 11:12:05AM +0000, Thomas Daniel wrote:
> > > > > 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).
> > > >
> > > > 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.
> 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.

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.
 
> 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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfs

[Daniel Vetter]

On Thu, Nov 20, 2014 at 1:28 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> 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.

Hm actually I think better safe than sorry here. At least I have
(again) completely forgotten about our dear shrinker ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfs

[Daniel, Thomas]

> -----Original Message-----
> From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> Daniel Vetter
> Sent: Thursday, November 20, 2014 12:51 PM
> To: Chris Wilson; Daniel, Thomas; intel-gfx@lists.freedesktop.org; akash goel
> (akash.goels@gmail.com)
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when
> dumping in debugfs
> 
> On Thu, Nov 20, 2014 at 1:28 PM, Chris Wilson <chris@chris-wilson.co.uk>
> wrote:
> >> 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.
> 
> 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.


> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfs

[Chris Wilson]

On Tue, Nov 25, 2014 at 02:30:55PM +0000, Daniel, Thomas wrote:
> > -----Original Message-----
> > From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> > Daniel Vetter
> > Sent: Thursday, November 20, 2014 12:51 PM
> > To: Chris Wilson; Daniel, Thomas; intel-gfx@lists.freedesktop.org; akash goel
> > (akash.goels@gmail.com)
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when
> > dumping in debugfs
> > 
> > On Thu, Nov 20, 2014 at 1:28 PM, Chris Wilson <chris@chris-wilson.co.uk>
> > wrote:
> > >> 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.
> > 
> > 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:

>> 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).

> 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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfs

[Daniel Vetter]

On Tue, Nov 25, 2014 at 02:44:33PM +0000, Chris Wilson wrote:
> On Tue, Nov 25, 2014 at 02:30:55PM +0000, Daniel, Thomas wrote:
> > > -----Original Message-----
> > > From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> > > Daniel Vetter
> > > Sent: Thursday, November 20, 2014 12:51 PM
> > > To: Chris Wilson; Daniel, Thomas; intel-gfx@lists.freedesktop.org; akash goel
> > > (akash.goels@gmail.com)
> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when
> > > dumping in debugfs
> > > 
> > > On Thu, Nov 20, 2014 at 1:28 PM, Chris Wilson <chris@chris-wilson.co.uk>
> > > wrote:
> > > >> 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.
> > > 
> > > 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:
> 
> >> 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).
> 
> > 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.

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfs

[Daniel, Thomas]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Tuesday, November 25, 2014 4:00 PM
> To: Chris Wilson; Daniel, Thomas; Daniel Vetter; intel-
> gfx@lists.freedesktop.org; akash goel (akash.goels@gmail.com)
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when
> dumping in debugfs
> 
> On Tue, Nov 25, 2014 at 02:44:33PM +0000, Chris Wilson wrote:
> > On Tue, Nov 25, 2014 at 02:30:55PM +0000, Daniel, Thomas wrote:
> > > > -----Original Message-----
> > > > From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On
> > > > Behalf Of Daniel Vetter
> > > > Sent: Thursday, November 20, 2014 12:51 PM
> > > > To: Chris Wilson; Daniel, Thomas; intel-gfx@lists.freedesktop.org;
> > > > akash goel
> > > > (akash.goels@gmail.com)
> > > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT
> > > > when dumping in debugfs
> > > >
> > > > On Thu, Nov 20, 2014 at 1:28 PM, Chris Wilson
> > > > <chris@chris-wilson.co.uk>
> > > > wrote:
> > > > >> 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.
> > > >
> > > > 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:
> >
> > >> 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).
> >
> > > 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.
> 
> 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
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
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfs

[Chris Wilson]

On Tue, Nov 25, 2014 at 04:42:22PM +0000, Daniel, Thomas wrote:
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> > Vetter
> > Sent: Tuesday, November 25, 2014 4:00 PM
> > To: Chris Wilson; Daniel, Thomas; Daniel Vetter; intel-
> > gfx@lists.freedesktop.org; akash goel (akash.goels@gmail.com)
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when
> > dumping in debugfs
> > 
> > On Tue, Nov 25, 2014 at 02:44:33PM +0000, Chris Wilson wrote:
> > > On Tue, Nov 25, 2014 at 02:30:55PM +0000, Daniel, Thomas wrote:
> > > > > -----Original Message-----
> > > > > From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On
> > > > > Behalf Of Daniel Vetter
> > > > > Sent: Thursday, November 20, 2014 12:51 PM
> > > > > To: Chris Wilson; Daniel, Thomas; intel-gfx@lists.freedesktop.org;
> > > > > akash goel
> > > > > (akash.goels@gmail.com)
> > > > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT
> > > > > when dumping in debugfs
> > > > >
> > > > > On Thu, Nov 20, 2014 at 1:28 PM, Chris Wilson
> > > > > <chris@chris-wilson.co.uk>
> > > > > wrote:
> > > > > >> 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.
> > > > >
> > > > > 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:
> > >
> > > >> 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).
> > >
> > > > 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.
> > 
> > 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
> 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.

The object does not necessarily have any obj->pages at this point.
i915_gem_object_get_pages() returns NULL and the code OOPSes.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfs

[Daniel Vetter]

On Tue, Nov 25, 2014 at 9:51 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, Nov 25, 2014 at 04:42:22PM +0000, Daniel, Thomas wrote:
>> > -----Original Message-----
>> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
>> > Vetter
>> > Sent: Tuesday, November 25, 2014 4:00 PM
>> > To: Chris Wilson; Daniel, Thomas; Daniel Vetter; intel-
>> > gfx@lists.freedesktop.org; akash goel (akash.goels@gmail.com)
>> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT when
>> > dumping in debugfs
>> >
>> > On Tue, Nov 25, 2014 at 02:44:33PM +0000, Chris Wilson wrote:
>> > > On Tue, Nov 25, 2014 at 02:30:55PM +0000, Daniel, Thomas wrote:
>> > > > > -----Original Message-----
>> > > > > From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On
>> > > > > Behalf Of Daniel Vetter
>> > > > > Sent: Thursday, November 20, 2014 12:51 PM
>> > > > > To: Chris Wilson; Daniel, Thomas; intel-gfx@lists.freedesktop.org;
>> > > > > akash goel
>> > > > > (akash.goels@gmail.com)
>> > > > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Don't pin LRC in GGTT
>> > > > > when dumping in debugfs
>> > > > >
>> > > > > On Thu, Nov 20, 2014 at 1:28 PM, Chris Wilson
>> > > > > <chris@chris-wilson.co.uk>
>> > > > > wrote:
>> > > > > >> 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.
>> > > > >
>> > > > > 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:
>> > >
>> > > >> 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).
>> > >
>> > > > 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.
>> >
>> > 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
>> 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.
>
> The object does not necessarily have any obj->pages at 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfs

[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_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.

Signed-off-by: Thomas Daniel <thomas.daniel@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c |   84 ++++++++++++++++++++---------------
 1 file changed, 49 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 7ea3843..e1de646 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -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);
 
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfs

[Daniel Vetter]

On Tue, Dec 02, 2014 at 01:21:18PM +0000, Thomas Daniel wrote:
> 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_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.

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?

> 
> Signed-off-by: Thomas Daniel <thomas.daniel@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |   84 ++++++++++++++++++++---------------
>  1 file changed, 49 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 7ea3843..e1de646 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -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).

> +{
> +	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');
> -			}
> -		}
> -	}

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

> +	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);
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfs

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------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
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx