Re: [Intel-gfx] [RFC v4 02/14] drm/i915/vm_bind: Add __i915_sw_fence_await_reservation()

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

On 21/09/2022 08:09, Niranjana Vishwanathapura wrote:
> Add function __i915_sw_fence_await_reservation() for
> asynchronous wait on a dma-resv object with specified
> dma_resv_usage. This is required for async vma unbind
> with vm_bind.
> 
> Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_sw_fence.c | 25 ++++++++++++++++++-------
>   drivers/gpu/drm/i915/i915_sw_fence.h |  7 ++++++-
>   2 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
> index 6fc0d1b89690..0ce8f4efc1ed 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence.c
> +++ b/drivers/gpu/drm/i915/i915_sw_fence.c
> @@ -569,12 +569,11 @@ int __i915_sw_fence_await_dma_fence(struct i915_sw_fence *fence,
>   	return ret;
>   }
>   
> -int i915_sw_fence_await_reservation(struct i915_sw_fence *fence,
> -				    struct dma_resv *resv,
> -				    const struct dma_fence_ops *exclude,
> -				    bool write,
> -				    unsigned long timeout,
> -				    gfp_t gfp)
> +int __i915_sw_fence_await_reservation(struct i915_sw_fence *fence,
> +				      struct dma_resv *resv,
> +				      enum dma_resv_usage usage,
> +				      unsigned long timeout,
> +				      gfp_t gfp)
>   {
>   	struct dma_resv_iter cursor;
>   	struct dma_fence *f;
> @@ -583,7 +582,7 @@ int i915_sw_fence_await_reservation(struct i915_sw_fence *fence,
>   	debug_fence_assert(fence);
>   	might_sleep_if(gfpflags_allow_blocking(gfp));
>   
> -	dma_resv_iter_begin(&cursor, resv, dma_resv_usage_rw(write));
> +	dma_resv_iter_begin(&cursor, resv, usage);
>   	dma_resv_for_each_fence_unlocked(&cursor, f) {
>   		pending = i915_sw_fence_await_dma_fence(fence, f, timeout,
>   							gfp);
> @@ -598,6 +597,18 @@ int i915_sw_fence_await_reservation(struct i915_sw_fence *fence,
>   	return ret;
>   }
>   
> +int i915_sw_fence_await_reservation(struct i915_sw_fence *fence,
> +				    struct dma_resv *resv,
> +				    const struct dma_fence_ops *exclude,
> +				    bool write,
> +				    unsigned long timeout,
> +				    gfp_t gfp)
> +{
> +	return __i915_sw_fence_await_reservation(fence, resv,
> +						 dma_resv_usage_rw(write),
> +						 timeout, gfp);
> +}

Drive by observation - it looked dodgy that you create a wrapper here 
which ignores one function parameter.

On a more detailed look it seems no callers actually use exclude and 
it's even unused inside this function since 1b5bdf071e62 ("drm/i915: use 
the new iterator in i915_sw_fence_await_reservation v3").

So a cleanup patch before this one?

Regards,

Tvrtko


> +
>   #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
>   #include "selftests/lib_sw_fence.c"
>   #include "selftests/i915_sw_fence.c"
> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h
> index 619fc5a22f0c..3cf4b6e16f35 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence.h
> +++ b/drivers/gpu/drm/i915/i915_sw_fence.h
> @@ -10,13 +10,13 @@
>   #define _I915_SW_FENCE_H_
>   
>   #include <linux/dma-fence.h>
> +#include <linux/dma-resv.h>
>   #include <linux/gfp.h>
>   #include <linux/kref.h>
>   #include <linux/notifier.h> /* for NOTIFY_DONE */
>   #include <linux/wait.h>
>   
>   struct completion;
> -struct dma_resv;
>   struct i915_sw_fence;
>   
>   enum i915_sw_fence_notify {
> @@ -89,6 +89,11 @@ int i915_sw_fence_await_dma_fence(struct i915_sw_fence *fence,
>   				  unsigned long timeout,
>   				  gfp_t gfp);
>   
> +int __i915_sw_fence_await_reservation(struct i915_sw_fence *fence,
> +				      struct dma_resv *resv,
> +				      enum dma_resv_usage usage,
> +				      unsigned long timeout,
> +				      gfp_t gfp);
>   int i915_sw_fence_await_reservation(struct i915_sw_fence *fence,
>   				    struct dma_resv *resv,
>   				    const struct dma_fence_ops *exclude,