Re: [PATCH 2/4] drm/i915: Consolidate get_fence with pin_fence

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 03/07/2017 11:14, Chris Wilson wrote:
> Following the pattern now used for obj->mm.pages, use just pin_fence and
> unpin_fence to control access to the fence registers. I.e. instead of
> calling get_fence(); pin_fence(), we now just need to call pin_fence().
> This will make it easier to reduce the locking requirements around
> fence registers.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_drv.h            |  4 ----
>   drivers/gpu/drm/i915/i915_gem.c            |  3 ++-
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c | 10 +++++-----
>   drivers/gpu/drm/i915/i915_gem_fence_reg.c  | 27 ++++++++++++++++++++++-----
>   drivers/gpu/drm/i915/i915_vma.c            |  3 +--
>   drivers/gpu/drm/i915/i915_vma.h            | 20 ++++++++------------
>   drivers/gpu/drm/i915/intel_display.c       |  3 +--
>   7 files changed, 39 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0029bb949e90..b08b56a2e680 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3545,10 +3545,6 @@ i915_vm_to_ppgtt(struct i915_address_space *vm)
>   	return container_of(vm, struct i915_hw_ppgtt, base);
>   }
>   
> -/* i915_gem_fence_reg.c */
> -int __must_check i915_vma_get_fence(struct i915_vma *vma);
> -int __must_check i915_vma_put_fence(struct i915_vma *vma);
> -
>   void i915_gem_revoke_fences(struct drm_i915_private *dev_priv);
>   void i915_gem_restore_fences(struct drm_i915_private *dev_priv);
>   
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 1b2dfa8bdeef..939a299260e9 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1941,7 +1941,7 @@ int i915_gem_fault(struct vm_fault *vmf)
>   	if (ret)
>   		goto err_unpin;
>   
> -	ret = i915_vma_get_fence(vma);
> +	ret = i915_vma_pin_fence(vma);

Hm, I think fence stealing stops working with this change?

Regards,

Tvrtko

>   	if (ret)
>   		goto err_unpin;
>   
> @@ -1957,6 +1957,7 @@ int i915_gem_fault(struct vm_fault *vmf)
>   			       min_t(u64, vma->size, area->vm_end - area->vm_start),
>   			       &ggtt->mappable);
>   
> +	i915_vma_unpin_fence(vma);
>   err_unpin:
>   	__i915_vma_unpin(vma);
>   err_unlock:
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 929f275e67aa..22a9f5358322 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -365,12 +365,12 @@ eb_pin_vma(struct i915_execbuffer *eb,
>   		return;
>   
>   	if (unlikely(entry->flags & EXEC_OBJECT_NEEDS_FENCE)) {
> -		if (unlikely(i915_vma_get_fence(vma))) {
> +		if (unlikely(i915_vma_pin_fence(vma))) {
>   			i915_vma_unpin(vma);
>   			return;
>   		}
>   
> -		if (i915_vma_pin_fence(vma))
> +		if (vma->fence)
>   			entry->flags |= __EXEC_OBJECT_HAS_FENCE;
>   	}
>   
> @@ -384,7 +384,7 @@ __eb_unreserve_vma(struct i915_vma *vma,
>   	GEM_BUG_ON(!(entry->flags & __EXEC_OBJECT_HAS_PIN));
>   
>   	if (unlikely(entry->flags & __EXEC_OBJECT_HAS_FENCE))
> -		i915_vma_unpin_fence(vma);
> +		__i915_vma_unpin_fence(vma);
>   
>   	__i915_vma_unpin(vma);
>   }
> @@ -564,13 +564,13 @@ static int eb_reserve_vma(const struct i915_execbuffer *eb,
>   	GEM_BUG_ON(eb_vma_misplaced(entry, vma));
>   
>   	if (unlikely(entry->flags & EXEC_OBJECT_NEEDS_FENCE)) {
> -		err = i915_vma_get_fence(vma);
> +		err = i915_vma_pin_fence(vma);
>   		if (unlikely(err)) {
>   			i915_vma_unpin(vma);
>   			return err;
>   		}
>   
> -		if (i915_vma_pin_fence(vma))
> +		if (vma->fence)
>   			entry->flags |= __EXEC_OBJECT_HAS_FENCE;
>   	}
>   
> diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> index 5fe2cd8c8f28..55ac7bc14fce 100644
> --- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> +++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> @@ -280,8 +280,7 @@ static int fence_update(struct drm_i915_fence_reg *fence,
>    *
>    * 0 on success, negative error code on failure.
>    */
> -int
> -i915_vma_put_fence(struct i915_vma *vma)
> +int i915_vma_put_fence(struct i915_vma *vma)
>   {
>   	struct drm_i915_fence_reg *fence = vma->fence;
>   
> @@ -299,6 +298,8 @@ static struct drm_i915_fence_reg *fence_find(struct drm_i915_private *dev_priv)
>   	struct drm_i915_fence_reg *fence;
>   
>   	list_for_each_entry(fence, &dev_priv->mm.fence_list, link) {
> +		GEM_BUG_ON(fence->vma && fence->vma->fence != fence);
> +
>   		if (fence->pin_count)
>   			continue;
>   
> @@ -313,7 +314,7 @@ static struct drm_i915_fence_reg *fence_find(struct drm_i915_private *dev_priv)
>   }
>   
>   /**
> - * i915_vma_get_fence - set up fencing for a vma
> + * i915_vma_pin_fence - set up fencing for a vma
>    * @vma: vma to map through a fence reg
>    *
>    * When mapping objects through the GTT, userspace wants to be able to write
> @@ -331,10 +332,11 @@ static struct drm_i915_fence_reg *fence_find(struct drm_i915_private *dev_priv)
>    * 0 on success, negative error code on failure.
>    */
>   int
> -i915_vma_get_fence(struct i915_vma *vma)
> +i915_vma_pin_fence(struct i915_vma *vma)
>   {
>   	struct drm_i915_fence_reg *fence;
>   	struct i915_vma *set = i915_gem_object_is_tiled(vma->obj) ? vma : NULL;
> +	int err;
>   
>   	/* Note that we revoke fences on runtime suspend. Therefore the user
>   	 * must keep the device awake whilst using the fence.
> @@ -344,6 +346,8 @@ i915_vma_get_fence(struct i915_vma *vma)
>   	/* Just update our place in the LRU if our fence is getting reused. */
>   	if (vma->fence) {
>   		fence = vma->fence;
> +		GEM_BUG_ON(fence->vma != vma);
> +		fence->pin_count++;
>   		if (!fence->dirty) {
>   			list_move_tail(&fence->link,
>   				       &fence->i915->mm.fence_list);
> @@ -353,10 +357,19 @@ i915_vma_get_fence(struct i915_vma *vma)
>   		fence = fence_find(vma->vm->i915);
>   		if (IS_ERR(fence))
>   			return PTR_ERR(fence);
> +		fence->pin_count++;
>   	} else
>   		return 0;
>   
> -	return fence_update(fence, set);
> +	err = fence_update(fence, set);
> +	if (err)
> +		goto err_unpin;
> +
> +	return 0;
> +
> +err_unpin:
> +	fence->pin_count--;
> +	return err;
>   }
>   
>   /**
> @@ -378,6 +391,8 @@ void i915_gem_revoke_fences(struct drm_i915_private *dev_priv)
>   	for (i = 0; i < dev_priv->num_fence_regs; i++) {
>   		struct drm_i915_fence_reg *fence = &dev_priv->fence_regs[i];
>   
> +		GEM_BUG_ON(fence->vma && fence->vma->fence != fence);
> +
>   		if (fence->vma)
>   			i915_gem_release_mmap(fence->vma->obj);
>   	}
> @@ -399,6 +414,8 @@ void i915_gem_restore_fences(struct drm_i915_private *dev_priv)
>   		struct drm_i915_fence_reg *reg = &dev_priv->fence_regs[i];
>   		struct i915_vma *vma = reg->vma;
>   
> +		GEM_BUG_ON(vma && vma->fence != reg);
> +
>   		/*
>   		 * Commit delayed tiling changes if we have an object still
>   		 * attached to the fence, otherwise just clear the fence.
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index d7330d3eeab6..efbfee8eac99 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -303,12 +303,11 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma *vma)
>   
>   	__i915_vma_pin(vma);
>   
> -	ret = i915_vma_get_fence(vma);
> +	ret = i915_vma_pin_fence(vma);
>   	if (ret) {
>   		__i915_vma_unpin(vma);
>   		return IO_ERR_PTR(ret);
>   	}
> -	i915_vma_pin_fence(vma);
>   
>   	return ptr;
>   }
> diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
> index 15e86e50a825..19f58af4f1bf 100644
> --- a/drivers/gpu/drm/i915/i915_vma.h
> +++ b/drivers/gpu/drm/i915/i915_vma.h
> @@ -342,15 +342,13 @@ static inline struct page *i915_vma_first_page(struct i915_vma *vma)
>    *
>    * True if the vma has a fence, false otherwise.
>    */
> -static inline bool
> -i915_vma_pin_fence(struct i915_vma *vma)
> +int i915_vma_pin_fence(struct i915_vma *vma);
> +int __must_check i915_vma_put_fence(struct i915_vma *vma);
> +
> +static inline void __i915_vma_unpin_fence(struct i915_vma *vma)
>   {
> -	lockdep_assert_held(&vma->obj->base.dev->struct_mutex);
> -	if (vma->fence) {
> -		vma->fence->pin_count++;
> -		return true;
> -	} else
> -		return false;
> +	GEM_BUG_ON(vma->fence->pin_count <= 0);
> +	vma->fence->pin_count--;
>   }
>   
>   /**
> @@ -365,10 +363,8 @@ static inline void
>   i915_vma_unpin_fence(struct i915_vma *vma)
>   {
>   	lockdep_assert_held(&vma->obj->base.dev->struct_mutex);
> -	if (vma->fence) {
> -		GEM_BUG_ON(vma->fence->pin_count <= 0);
> -		vma->fence->pin_count--;
> -	}
> +	if (vma->fence)
> +		__i915_vma_unpin_fence(vma);
>   }
>   
>   #endif
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4e03ca6c946f..432bbaf460b4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2183,8 +2183,7 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
>   		 * something and try to run the system in a "less than optimal"
>   		 * mode that matches the user configuration.
>   		 */
> -		if (i915_vma_get_fence(vma) == 0)
> -			i915_vma_pin_fence(vma);
> +		i915_vma_pin_fence(vma);
>   	}
>   
>   	i915_vma_get(vma);
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx