Re: [PATCH v6 20/20] drm/i915/vm_bind: Async vm_unbind support

[Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>]

On Wed, Nov 09, 2022 at 10:13:36PM +0100, Andi Shyti wrote:
>Hi Niranjana,
>
>...
>
>> -static void force_unbind(struct i915_vma *vma)
>> +static void force_unbind(struct i915_vma *vma, bool async)
>>  {
>>  	if (!drm_mm_node_allocated(&vma->node))
>>  		return;
>> @@ -1725,7 +1727,21 @@ static void force_unbind(struct i915_vma *vma)
>>  		i915_vma_set_purged(vma);
>>
>>  	atomic_and(~I915_VMA_PIN_MASK, &vma->flags);
>> -	WARN_ON(__i915_vma_unbind(vma));
>> +	if (async) {
>> +		struct dma_fence *fence;
>> +
>> +		fence = __i915_vma_unbind_async(vma);
>> +		if (IS_ERR_OR_NULL(fence)) {
>> +			async = false;
>> +		} else {
>> +			dma_resv_add_fence(vma->obj->base.resv, fence,
>> +					   DMA_RESV_USAGE_READ);
>> +			dma_fence_put(fence);
>> +		}
>> +	}
>> +
>> +	if (!async)
>> +		WARN_ON(__i915_vma_unbind(vma));
>>  	GEM_BUG_ON(drm_mm_node_allocated(&vma->node));
>>  }
>>
>> @@ -1785,7 +1801,7 @@ void i915_vma_destroy_locked(struct i915_vma *vma)
>>  {
>>  	lockdep_assert_held(&vma->vm->mutex);
>>
>> -	force_unbind(vma);
>> +	force_unbind(vma, false);
>
>How about:
>
>#define force_unbind(v)		__force_unbind(v, false)
>#define force_unbind_async(v)	__force_unbind(v, true)
>
>The true/false parameters in a function is not immediately
>understandable.
>
>or
>
>#define force_unbind_sync(v)	force_unbind(v, false)
>#define force_unbind_async(v)	force_unbind(v, true)
>
>but I prefer the first version.

Andi, I get the point. But currently, force_unbind() is staic
function with only couple of invocations. These defines seems
an overkill (would be good to define these in header files
if the function is not static). Hope we can keep it as is for now.

Niranjana

>
>Andi