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

[Daniel, Thomas]

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

Thomas.