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

["Winiarski, Michal" <michal.winiarski@intel.com>]

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.
> 
> 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;
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx