[Intel-gfx] [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 at 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 at ffwll.ch] On Behalf Of Daniel
>> > Vetter
>> > Sent: Tuesday, November 25, 2014 4:00 PM
>> > To: Chris Wilson; Daniel, Thomas; Daniel Vetter; intel-
>> > gfx at lists.freedesktop.org; akash goel (akash.goels at 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 at ffwll.ch [mailto:daniel.vetter at ffwll.ch] On
>> > > > > Behalf Of Daniel Vetter
>> > > > > Sent: Thursday, November 20, 2014 12:51 PM
>> > > > > To: Chris Wilson; Daniel, Thomas; intel-gfx at lists.freedesktop.org;
>> > > > > akash goel
>> > > > > (akash.goels at 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 at 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