Re: [PATCH 35/55] drm/i915: Be more careful when unbinding vma

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On ma, 2016-07-25 at 18:32 +0100, Chris Wilson wrote:
> When we call i915_vma_unbind(), we will wait upon outstanding rendering.
> This will also trigger a retirement phase, which may update the object
> lists. If, we extend request tracking to the VMA itself (rather than
> keep it at the encompassing object), then there is a potential that the
> obj->vma_list be modified for other elements upon i915_vma_unbind(). As
> a result, if we walk over the object list and call i915_vma_unbind(), we
> need to be prepared for that list to change.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h          |  2 ++
>  drivers/gpu/drm/i915/i915_gem.c          | 57 +++++++++++++++++++++++---------
>  drivers/gpu/drm/i915/i915_gem_shrinker.c |  8 +----
>  drivers/gpu/drm/i915/i915_gem_userptr.c  |  4 +--
>  4 files changed, 46 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e28228c6f383..2abae63258a3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3052,6 +3052,8 @@ int __must_check i915_vma_unbind(struct i915_vma *vma);
>   * _guarantee_ VMA in question is _not in use_ anywhere.
>   */
>  int __must_check __i915_vma_unbind_no_wait(struct i915_vma *vma);
> +
> +int i915_gem_object_unbind(struct drm_i915_gem_object *obj);
>  int i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
>  void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv);
>  void i915_gem_release_mmap(struct drm_i915_gem_object *obj);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a3defd7b4046..9169f5f3d20c 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -283,18 +283,38 @@ static const struct drm_i915_gem_object_ops i915_gem_phys_ops = {
>  	.release = i915_gem_object_release_phys,
>  };
>  
> +int
> +i915_gem_object_unbind(struct drm_i915_gem_object *obj)
> +{
> +	struct i915_vma *vma;
> +	LIST_HEAD(still_in_list);
> +	int ret;
> +
> +	/* The vma will only be freed if it is marked as closed, and if we wait
> +	 * upon rendering to the vma, we may unbind anything in the list.
> +	 */
> +	while ((vma = list_first_entry_or_null(&obj->vma_list,
> +					       struct i915_vma,
> +					       obj_link))) {
> +		list_move_tail(&vma->obj_link, &still_in_list);
> +		ret = i915_vma_unbind(vma);
> +		if (ret)
> +			break;
> +	}
> +	list_splice(&still_in_list, &obj->vma_list);
> +
> +	return ret;
> +}
> +
>  static int
>  drop_pages(struct drm_i915_gem_object *obj)
>  {
> -	struct i915_vma *vma, *next;
>  	int ret;
>  
>  	i915_gem_object_get(obj);
> -	list_for_each_entry_safe(vma, next, &obj->vma_list, obj_link)
> -		if (i915_vma_unbind(vma))
> -			break;
> -
> -	ret = i915_gem_object_put_pages(obj);
> +	ret = i915_gem_object_unbind(obj);
> +	if (ret == 0)

(!ret)

Other than that, looks good.

The list_for_each loops are fancy to review because we have so many
levels of functions and you never know where the corresponding list_add
or list_del is, in this case two different files O_o

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Now I notice Tvrtko already reviewed this, well.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx