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@xxxxxxxxxxxxxxxxxx>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
Now I notice Tvrtko already reviewed this, well.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
