Re: [PATCH 53/55] drm/i915: Release vma when the handle is closed

Joonas Lahtinen
2016-07-27T10:01:03Z

On ma, 2016-07-25 at 18:32 +0100, Chris Wilson wrote:
>  /**
>   * i915_gem_wait_ioctl - implements DRM_IOCTL_I915_GEM_WAIT
>   * @dev: drm device pointer
> @@ -2810,26 +2823,32 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait)
>   if (active && wait) {
>   int idx;
>
> + /* When a closed VMA is retired, it is unbound - eek.
> +  * In order to prevent it from being recursively closed,
> +  * take a pin on the vma so that the second unbind is
> +  * aborted.
> +  */
> + vma->pin_count++;

Always smells fishy. But an extra variable probably not worthy.

> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index ef0dc7131808..bfac2448ba04 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3339,6 +3339,31 @@ i915_vma_retire(struct i915_gem_active *active,
>   return;
>
>   list_move_tail(&vma->vm_link, &vma->vm->inactive_list);
> + if (unlikely(vma->closed && !vma->pin_count))
> + WARN_ON(i915_vma_unbind(vma));

So this is just an optimization to get rid of the VMA ASAP if we're
lucky?

> +
> +void i915_vma_close(struct i915_vma *vma)
> +{
> + GEM_BUG_ON(vma->closed);
> + vma->closed = true;
> +
> + list_del_init(&vma->obj_link);
> + if (!i915_vma_is_active(vma) && !vma->pin_count)
> + WARN_ON(__i915_vma_unbind_no_wait(vma));

Same here, an optimization?

Somebody from the original CC list probably should give it a look too.

Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation