[Intel-gfx] [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 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch