Re: [PATCH] drm/i915: Defer adding preallocated stolen objects to the VM list

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Sep 24, 2015 at 02:24:36PM +0300, Jani Nikula wrote:
> On Thu, 24 Sep 2015, "Winiarski, Michal" <michal.winiarski@intel.com> wrote:
> > On Thu, 2015-09-24 at 11:57 +0100, Chris Wilson wrote:
> >> When preallocating a stolen object during early initialisation, we
> >> may
> >> be running before we have setup the the global GTT VM state, in
> >> particular before we have initialised the range manager and
> >> associated
> >> lists. As this is the case, we defer binding the stolen object until
> >> we
> >> call i915_gem_setup_global_gtt(). Not only should we defer the
> >> binding,
> >> but we should also defer the VM list manipulation.
> >> 
> >> Fixes regression uncovered by commit
> >> a2cad9dff4dd44d0244b966d980de9d602d87593
> >> Author: Michał Winiarski <michal.winiarski@intel.com>
> >> Date:   Wed Sep 16 11:49:00 2015 +0200
> >> 
> >>     drm/i915/gtt: Do not initialize drm_mm twice.
> 
> I confirm reverting the above works.
> 
> This patch is
> 
> Tested-by: Jani Nikula <jani.nikula@intel.com>
> 
> on a BWD GT2 ULT.

Queued for -next, thanks for the patch.
-Daniel

> 
> >> 
> >> Whilst I am here remove the duplicate work leaving dangling pointers
> >> from the error path...
> >> 
> >> v2: Typos galore before coffee.
> >> 
> >> Reported-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92099
> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> >> Cc: Michel Thierry <michel.thierry@intel.com>
> >> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> >> Cc: Michał Winiarski <michal.winiarski@intel.com>
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> >
> > Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
> >
> > -Michał
> >
> >> ---
> >>  drivers/gpu/drm/i915/i915_gem_gtt.c    |  2 +-
> >>  drivers/gpu/drm/i915/i915_gem_stolen.c | 16 ++++++----------
> >>  2 files changed, 7 insertions(+), 11 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> >> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >> index 01f3521e77d3..291305fb2f3c 100644
> >> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> >> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >> @@ -2533,7 +2533,6 @@ static int ggtt_bind_vma(struct i915_vma *vma,
> >>  		 * the bound flag ourselves.
> >>  		 */
> >>  		vma->bound |= GLOBAL_BIND;
> >> -
> >>  	}
> >>  
> >>  	if (dev_priv->mm.aliasing_ppgtt && flags & LOCAL_BIND) {
> >> @@ -2657,6 +2656,7 @@ static int i915_gem_setup_global_gtt(struct
> >> drm_device *dev,
> >>  			return ret;
> >>  		}
> >>  		vma->bound |= GLOBAL_BIND;
> >> +		list_add_tail(&vma->mm_list, &ggtt_vm
> >> ->inactive_list);
> >>  	}
> >>  
> >>  	/* Clear any non-preallocated blocks */
> >> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
> >> b/drivers/gpu/drm/i915/i915_gem_stolen.c
> >> index 55df6ce34751..15207796e1b3 100644
> >> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> >> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> >> @@ -584,7 +584,7 @@
> >> i915_gem_object_create_stolen_for_preallocated(struct drm_device
> >> *dev,
> >>  	vma = i915_gem_obj_lookup_or_create_vma(obj, ggtt);
> >>  	if (IS_ERR(vma)) {
> >>  		ret = PTR_ERR(vma);
> >> -		goto err_out;
> >> +		goto err;
> >>  	}
> >>  
> >>  	/* To simplify the initialisation sequence between KMS and
> >> GTT,
> >> @@ -598,23 +598,19 @@
> >> i915_gem_object_create_stolen_for_preallocated(struct drm_device
> >> *dev,
> >>  		ret = drm_mm_reserve_node(&ggtt->mm, &vma->node);
> >>  		if (ret) {
> >>  			DRM_DEBUG_KMS("failed to allocate stolen GTT
> >> space\n");
> >> -			goto err_vma;
> >> +			goto err;
> >>  		}
> >> -	}
> >>  
> >> -	vma->bound |= GLOBAL_BIND;
> >> +		vma->bound |= GLOBAL_BIND;
> >> +		list_add_tail(&vma->mm_list, &ggtt->inactive_list);
> >> +	}
> >>  
> >>  	list_add_tail(&obj->global_list, &dev_priv->mm.bound_list);
> >> -	list_add_tail(&vma->mm_list, &ggtt->inactive_list);
> >>  	i915_gem_object_pin_pages(obj);
> >>  
> >>  	return obj;
> >>  
> >> -err_vma:
> >> -	i915_gem_vma_destroy(vma);
> >> -err_out:
> >> -	i915_gem_stolen_remove_node(dev_priv, stolen);
> >> -	kfree(stolen);
> >> +err:
> >>  	drm_gem_object_unreference(&obj->base);
> >>  	return NULL;
> >>  }
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx