Re: [PATCH 2/2] drm/i915: Unpin stolen pages

From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Daniel Vetter <daniel@ffwll.ch>, Ben Widawsky <ben@bwidawsk.net>,
	Intel GFX <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 2/2] drm/i915: Unpin stolen pages
Date: Mon, 3 Jun 2013 10:52:12 +0200	[thread overview]
Message-ID: <20130603085212.GK15743@phenom.ffwll.local> (raw)
In-Reply-To: <20130603083648.GC17485@cantiga.alporthouse.com>

On Mon, Jun 03, 2013 at 09:36:48AM +0100, Chris Wilson wrote:
> On Sat, Jun 01, 2013 at 03:13:04PM +0200, Daniel Vetter wrote:
> > On Sat, Jun 1, 2013 at 1:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > On Sat, Jun 01, 2013 at 11:17:10AM +0200, Daniel Vetter wrote:
> > >> On Sat, Jun 1, 2013 at 1:51 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > >> > That neatly explains the WARN. Not too happy about accumulating lots of
> > >> > backing storage specific processing into free_object, but that can be
> > >> > fixed up later (there is an obj->ops->release() pending).
> > >>
> > >> I'm more irked with the semantic overloading of object pinning. Might
> > >> be cleaner to otherwise mark stolen obejcts as not shrinkable instead
> > >> of pinning them for their entire lifetime. But we can bikeshed that
> > >> later on ;-)
> > >
> > > Some merit to that argument, but it still feels correct to say that the
> > > stolen pages are pinned for their lifetime. Given obj->ops->release(),
> > > it does actually become simpler to not mess around with pin_count. So
> > > later it is.
> > 
> > I was more unhappy that pin_count has different meanings, until I've
> > noticed that we've fixed that up already with the introduction of
> > ->pages_pin_count. Shouldn't stolen mem just hold a reference on that
> > one? After all unbinding from the gtt is ok with stolen memory, but
> > dropping the backing storage in the shrinker won't work. Not that we
> > currently use stolen for anything else than permanently pinned bos.
> 
> As mentioned on irc, stolen does use the pages_pin_count for its
> purposes. The purpose of this patch is purely to allow sanity checking
> the pages_pin_count with a WARN_ON during free which seems sensible but
> not strictly required.

Yeah, silly me didn't read the code before sending a knee-jerk mail ;-)

Series merged (hopefully in the right order and all, please check), thanks
for the patches and review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch