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

Joonas Lahtinen
2016-07-26T08:00:09Z

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