Re: [Intel-gfx] [PATCH v4 02/61] drm/i915: Add missing -EDEADLK handling to execbuf pinning

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 20-10-2020 om 22:18 schreef Matthew Brost:
> On Fri, Oct 16, 2020 at 12:43:45PM +0200, Maarten Lankhorst wrote:
>> i915_vma_pin may fail with -EDEADLK when we start locking page tables,
>> so ensure we handle this correctly.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  .../gpu/drm/i915/gem/i915_gem_execbuffer.c    | 23 +++++++++++++++----
>>  1 file changed, 18 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> index a199336792fb..0f5efced0b87 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> @@ -419,13 +419,14 @@ static u64 eb_pin_flags(const struct drm_i915_gem_exec_object2 *entry,
>>  	return pin_flags;
>>  }
>>  
>> -static inline bool
>> +static inline int
>>  eb_pin_vma(struct i915_execbuffer *eb,
>>  	   const struct drm_i915_gem_exec_object2 *entry,
>>  	   struct eb_vma *ev)
>>  {
>>  	struct i915_vma *vma = ev->vma;
>>  	u64 pin_flags;
>> +	int err;
>>  
>>  	if (vma->node.size)
>>  		pin_flags = vma->node.start;
>> @@ -438,16 +439,24 @@ eb_pin_vma(struct i915_execbuffer *eb,
>>  
>>  	/* Attempt to reuse the current location if available */
>>  	/* TODO: Add -EDEADLK handling here */
> Drop the TODO?
>
>> -	if (unlikely(i915_vma_pin_ww(vma, &eb->ww, 0, 0, pin_flags))) {
>> +	err = i915_vma_pin_ww(vma, &eb->ww, 0, 0, pin_flags);
>> +	if (err == -EDEADLK)
>> +		return err;
>> +
>> +	if (unlikely(err)) {
>>  		if (entry->flags & EXEC_OBJECT_PINNED)
>>  			return false;
>>  
>>  		/* Failing that pick any _free_ space if suitable */
>> -		if (unlikely(i915_vma_pin_ww(vma, &eb->ww,
>> +		err = i915_vma_pin_ww(vma, &eb->ww,
>>  					     entry->pad_to_size,
>>  					     entry->alignment,
>>  					     eb_pin_flags(entry, ev->flags) |
>> -					     PIN_USER | PIN_NOEVICT)))
>> +					     PIN_USER | PIN_NOEVICT);
>> +		if (err == -EDEADLK)
>> +			return err;
>> +
>> +		if (unlikely(err))
>>  			return false;
> Confusing to return a boolean 'false' while the return value of this
> function is an int. Return '0' if that is the intent, which I believe it
> based on how the caller handles the return. 

Yeah, I think it makes more sense to change eb_pin_vma to a proper int, instead of a special one.

In most places we can just propagate the error. I will respin this patch. :)

>>  	}
>>  
>> @@ -900,7 +909,11 @@ static int eb_validate_vmas(struct i915_execbuffer *eb)
>>  		if (err)
>>  			return err;
>>  
> Can't say I love the triple comparison of the return values, but if you
> need to do this I'd put all of comparison in the same clause. Just my
> opinion.
Neither. I think I will just special case -EDEADLK, which should be easy with the fix to eb_pin_vma I suggested above.
>
> Matt
>
>> -		if (eb_pin_vma(eb, entry, ev)) {
>> +		err = eb_pin_vma(eb, entry, ev);
>> +		if (err < 0)
>> +			return err;
>> +
>> +		if (err > 0) {
>>  			if (entry->offset != vma->node.start) {
>>  				entry->offset = vma->node.start | UPDATE;
>>  				eb->args->flags |= __EXEC_HAS_RELOC;
>> -- 
>> 2.28.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx