Re: [Intel-gfx] [PATCH 1/2] drm/i915: add wait and lock to i915_vma_move_to_active

[Andrzej Hajda <andrzej.hajda@intel.com>]

Hi Andi,

Thx for looking at it.

On 21.10.2022 17:51, Andi Shyti wrote:
> Hi Andrzej,
> 
> (at first I r-b'ed this patch, but then I wanted to think on some
> more "simplification" (if it really simplifies things). Please
> read the review in patch 2 first )

This is v1, there is already v2.
I will reply here to your comments with v2 in mind.

> 
>> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
>> index 1cae24349a96fd..80e7fdd5d16427 100644
>> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
>> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
>> @@ -565,10 +565,8 @@ static int make_obj_busy(struct drm_i915_gem_object *obj)
>>   			goto err_unpin;
>>   		}
>>   
>> -		err = i915_request_await_object(rq, vma->obj, true);
>> -		if (err == 0)
>> -			err = i915_vma_move_to_active(vma, rq,
>> -						      EXEC_OBJECT_WRITE);
>> +		err = i915_vma_move_to_active(vma, rq,
>> +					      EXEC_OBJECT_WRITE);
> 
> nit: don't need to break the line here.

Corrected in v2.

> 
>>   
>>   		i915_request_add(rq);
>>   err_unpin:
> 
> [...]
> 
>> @@ -860,9 +854,7 @@ static int read_whitelisted_registers(struct intel_context *ce,
>>   		return PTR_ERR(rq);
>>   
>>   	i915_vma_lock(results);
>> -	err = i915_request_await_object(rq, results->obj, true);
>> -	if (err == 0)
>> -		err = i915_vma_move_to_active(results, rq, EXEC_OBJECT_WRITE);
>> +	err = i915_vma_move_to_active(results, rq, EXEC_OBJECT_WRITE);
>>   	i915_vma_unlock(results);
>>   	if (err)
>>   		goto err_req;
>> @@ -944,9 +936,7 @@ static int scrub_whitelisted_registers(struct intel_context *ce)
>>   	}
>>   
>>   	i915_vma_lock(batch);
>> -	err = i915_request_await_object(rq, batch->obj, false);
>> -	if (err == 0)
>> -		err = i915_vma_move_to_active(batch, rq, 0);
>> +	err = i915_vma_move_to_active(batch, rq, 0);
>>   	i915_vma_unlock(batch);
> 
> The final risult would be:
> 
> 	i915_vma_lock();
> 	i915_vma_move_to_active()
> 	i915_vma_unlock();
> 
> and it's a pattern... as I suggested in patch 2, how about having
> an:
> 
> 	i915_vma_move_to_active_unlocked()


There is igt_vma_move_to_active_unlocked in patch 2. Chris suggested to 
limit this helper to selftests, as this pattern is specific to selftests 
and should not be exposed as 'internal API'.


> 
> and...
> 
>>   	if (err)
>>   		goto err_request;
>> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
>> index d6fe94cd0fdb61..b49098f045005e 100644
>> --- a/drivers/gpu/drm/i915/gvt/scheduler.c
>> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
>> @@ -570,9 +570,8 @@ static int prepare_shadow_batch_buffer(struct intel_vgpu_workload *workload)
>>   			if (gmadr_bytes == 8)
>>   				bb->bb_start_cmd_va[2] = 0;
>>   
>> -			ret = i915_vma_move_to_active(bb->vma,
>> -						      workload->req,
>> -						      0);
>> +			ret = _i915_vma_move_to_active(bb->vma, workload->req,
>> +						       &workload->req->fence, 0);
>>   			if (ret)
>>   				goto err;
>>   
>> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>> index 15816df916c781..19138e99d2fd03 100644
>> --- a/drivers/gpu/drm/i915/i915_perf.c
>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>> @@ -2015,9 +2015,7 @@ emit_oa_config(struct i915_perf_stream *stream,
>>   			goto err_add_request;
>>   	}
>>   
>> -	err = i915_request_await_object(rq, vma->obj, 0);
>> -	if (!err)
>> -		err = i915_vma_move_to_active(vma, rq, 0);
>> +	err = i915_vma_move_to_active(vma, rq, 0);
>>   	if (err)
>>   		goto err_add_request;
>>   
>> diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
>> index aecd9c64486b27..47ac5bd1ffcce6 100644
>> --- a/drivers/gpu/drm/i915/i915_vma.h
>> +++ b/drivers/gpu/drm/i915/i915_vma.h
>> @@ -64,7 +64,11 @@ static inline int __must_check
>>   i915_vma_move_to_active(struct i915_vma *vma, struct i915_request *rq,
>>   			unsigned int flags)
>>   {
>> -	return _i915_vma_move_to_active(vma, rq, &rq->fence, flags);
>> +	int err = i915_request_await_object(rq, vma->obj, flags & EXEC_OBJECT_WRITE);
>> +
>> +	if (!err)
>> +		err = _i915_vma_move_to_active(vma, rq, &rq->fence, flags);
>> +	return err;
>>   }
> 
> ... this i915_vma_move_to_active() now it's doing more than just
> moving to active but it's also waiting on dma fences, shall we
> call it i915_vma_move_to_active_async() or silimar? (I'm not good
> at giving names).

I do not feel an expert in this area, but for example 
__i915_vma_move_to_active also calls __i915_request_await_bind and then 
moves to active (so awaits are there anyway).
In v2 this was handled by putting i915_request_await_object to 
_i915_vma_move_to_active and added no_await flag.

Regards
Andrzej

> 
> The above would be i915_vma_move_to_active_async_unlocked(). Too
> long? More complex?
> 
> We would have something like:
> 
> 	i915_vma_move_to_active() /* not used */
> 	i915_vma_move_to_active_unlocked()
> 	i915_vma_move_to_active_async()
> 	i915_vma_move_to_active_async_unlocked()
> 
> Anyway as it is looks good, I didn't spot any error in the
> conversion:
> 
> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
> 
> Andi
> 
> [...]