Re: [PATCH 3/3] drm/i915: Preallocate mm node for GTT mmap offset

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Dec 18, 2012 at 10:11:14PM +0100, Daniel Vetter wrote:
> On Wed, Dec 12, 2012 at 10:55:17AM +0000, Chris Wilson wrote:
> > On Wed, 12 Dec 2012 12:48:43 +0200, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > > On Fri, 07 Dec 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > > As the shrinker may be invoked for the allocation, and it may reap
> > > > neighbouring objects in the offset range mm, we need to be careful in
> > > > the order in which we allocate the node, search for free space and then
> > > > insert the node into the mmap offset range manager.
> > > >
> > > > Fixes i-g-t/gem_tiled_swapping
> > > >
> > > > Reported-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_gem.c |   59 ++++++++++++++++++++++++++++++++-------
> > > >  1 file changed, 49 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > > index d17f52d..3ab97c6 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > > @@ -1512,14 +1512,29 @@ i915_gem_get_unfenced_gtt_alignment(struct drm_device *dev,
> > > >  static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> > > > +	struct drm_gem_mm *mm = obj->base.dev->mm_private;
> > > > +	struct drm_map_list *list;
> > > >  	int ret;
> > > >  
> > > > -	if (obj->base.map_list.map)
> > > > +	list = &obj->base.map_list;
> > > > +	if (list->map)
> > > >  		return 0;
> > > >  
> > > > -	ret = drm_gem_create_mmap_offset(&obj->base);
> > > > -	if (ret != -ENOSPC)
> > > > -		return ret;
> > > > +	/* Set the object up for mmap'ing */
> > > > +	list->map = kzalloc(sizeof(struct drm_map_list), GFP_KERNEL);
> > > 
> > > sizeof(struct drm_local_map) or sizeof(*list->map) instead?
> > > 
> > > Hmm, it's like this in drm_gem_create_mmap_offset too, either it's a bug
> > > or I'm being clueless.
> > 
> > You're right it's a a bug but fortunately in the safe direction, I'd
> > vote for sizeof(*list->map) as unequivocal.
> 
> All applied with bikeshed and pushed to -fixes, thanks.

Reconsidering just this patch, this smells too much like we leak our own
shrinker internals still. Can't we just set a dev_priv flag which tells
the shrinker not to attempt lock-stealing while we call down into the
drm mmap functions?

The issue is that people are talking about unifying the mmap lookup stuff
a bit since it's superflously duplicated between ttm and drm. So us
rolling our own special-case is a bit awkward. I've dropped the patch for
now.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch