[Intel-gfx] [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 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.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre