[Intel-gfx] [PATCH 2/4] drm/i915: Improve dynamic management/eviction of lrc backing objects

[Daniel Vetter]

On Fri, Oct 16, 2015 at 03:42:53PM +0100, Nick Hoath wrote:
> On 08/10/2015 14:35, Chris Wilson wrote:
> >On Wed, Oct 07, 2015 at 06:05:46PM +0200, Daniel Vetter wrote:
> >>On Tue, Oct 06, 2015 at 03:52:02PM +0100, Nick Hoath wrote:
> >>>Shovel all context related objects through the active queue and obj
> >>>management.
> >>>
> >>>- Added callback in vma_(un)bind to add CPU (un)mapping at same time
> >>>   if desired
> >>>- Inserted LRC hw context & ringbuf to vma active list
> >>>
> >>>Issue: VIZ-4277
> >>>Signed-off-by: Nick Hoath <nicholas.hoath at intel.com>
> >>>---
> >>>  drivers/gpu/drm/i915/i915_drv.h         |  4 ++
> >>>  drivers/gpu/drm/i915/i915_gem.c         |  3 ++
> >>>  drivers/gpu/drm/i915/i915_gem_gtt.c     |  8 ++++
> >>>  drivers/gpu/drm/i915/intel_lrc.c        | 28 +++++++++++--
> >>>  drivers/gpu/drm/i915/intel_ringbuffer.c | 71 ++++++++++++++++++---------------
> >>>  drivers/gpu/drm/i915/intel_ringbuffer.h |  3 --
> >>>  6 files changed, 79 insertions(+), 38 deletions(-)
> >>>
> >>>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>>index 3d217f9..d660ee3 100644
> >>>--- a/drivers/gpu/drm/i915/i915_drv.h
> >>>+++ b/drivers/gpu/drm/i915/i915_drv.h
> >>>@@ -2169,6 +2169,10 @@ struct drm_i915_gem_object {
> >>>  			struct work_struct *work;
> >>>  		} userptr;
> >>>  	};
> >>>+
> >>>+	/** Support for automatic CPU side mapping of object */
> >>>+	int (*mmap)(struct drm_i915_gem_object *obj, bool unmap);
> >>
> >>I don't think we need a map hook, that can still be done (if not done so
> I disagree - this keeps the interface symmetrical. Searching for the do/undo
> code paths and finding they are in difference places, called via different
> routes makes code harder to follow.
> >>already) by the callers. Also it's better to rename this to vma_unbind
> >>(and it should be at the vma level I think) since there's other potential
> Nope - the obj is created first, at a point where the map/unamp function can
> be known. Moving the map/unmap to the vma would mean having a callback path
> to the object just to set up the callback path when the vma is created
> anonymously at some later point.

One of the plans for this is to also use it for to-be-unpinned
framebuffers (4k buffers are huge ...). And in that case the unmap hook
only, and on the vma is the design we want. And since it also seems to
accomodate all the other users I do think it's the right choice.

Like I said, explicit setup and lazy, implicit cleanup is kinda how a lot
of things in gem work.
-Daniel

> >>users. So explicit maping, lazy unmapping for the kmaps we need. That's
> >>the same design we're using for binding objects into gpu address spaces.
> >>
> >>Also Chris Wilson has something similar, please align with him on the
> >>precise design of this callback.
> >
> >We need the unbind hook because of the movement in the first patch (it
> >is a separate issue, the code should work without it albeit having to
> >remap the ring/context state more often). The changelog in this patch
> >simply explains the i915_vma_move_to_active() additions. But to get the
> >shrink accurate we do need the context unpin on retirement and to do the
> >pin_count check in i915_vma_unbind() after waiting (rather than before,
> >as we currently do). However, the eviction code will not inspect the
> >active contexts objects yet (as it will continue to skip over the
> >ggtt->pin_count on them). The way I allowed ctx objects to be evicted was
> >to only keep the ctx->state pinned for the duration of the request
> >construction.
> >
> >Note that I think it should be a vma->unbind hook not an object level
> >one (it is i915_vma_unbind, without only a modicum of object level state
> >being modified in that function).
> >-Chris
> >
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch