Re: [PATCH v3 2/5] drm/i915: Use batch pools with the command parser

From: Daniel Vetter <daniel@ffwll.ch>
To: "Volkin, Bradley D" <bradley.d.volkin@intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH v3 2/5] drm/i915: Use batch pools with the command parser
Date: Fri, 7 Nov 2014 10:45:03 +0100	[thread overview]
Message-ID: <20141107094503.GD6135@phenom.ffwll.local> (raw)
In-Reply-To: <20141106173800.GA2754@bdvolkin-ubuntu-desktop>

On Thu, Nov 06, 2014 at 09:38:00AM -0800, Volkin, Bradley D wrote:
> On Thu, Nov 06, 2014 at 05:56:36AM -0800, Daniel Vetter wrote:
> > On Thu, Nov 06, 2014 at 07:36:55AM +0000, Chris Wilson wrote:
> > > On Wed, Nov 05, 2014 at 02:42:00PM -0800, Volkin, Bradley D wrote:
> > > > For this part, I've got an implementation that works ok but one difference is
> > > > that if we stop submitting batches, and therefore stop calling batch_pool_get,
> > > > we stop moving buffers to the batch pool's inactive list. This means some buffers
> > > > don't get marked purgeable even when they are. The solution that I see is to
> > > > add a function to do the batch pool active -> inactive work and then call that
> > > > from the appropriate place(s), but that seems to defeat the purpose of the
> > > > proposed change. Suggestions?
> > > 
> > > Just mark them always as purgeable.
> > 
> > Yeah the trick with purgeable is that the shrinker will wait for the
> > buffers to retire if they're still active. So you can mark the purgeable
> > right after the move_to_active call. Then the only part that doesn't
> > happen automatically is the batch-pool internal accounting. But we also
> > don't really care about that until we want a new shadow batch.
> 
> Ok. I was concerned about leaving objects purgeable because there are various
> places where the driver checks that an object is not purgeable. Looking at it
> again, the only one I'm nervous about is i915_gem_object_get_pages(), but I'll
> put something together and see if it's a problem. I imagine we can avoid the
> issue by carefully setting madv during/after the parser flow.

Yeah, calling get_pages on a purgeable object is a bug. But you have the
bo already pinned, so the only thing we might call is put_pages. And
being able to free pages is the point of purgeable. Of course before you
reuse it you have to set the bo to willneed again.

Aside: Since you're digging around in all this, feel like doing a DOC:
comment about purgeable memeory and pulling it into the kerneldoc? I know
that GEM driver docs are really thin still so that comment will look
lonely in the docbook, but we need to start somewhere.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx