diff mbox series

[v4,02/61] drm/i915: Add missing -EDEADLK handling to execbuf pinning

Message ID 20201016104444.1492028-3-maarten.lankhorst@linux.intel.com
State New, archived
Headers show
Series drm/i915: Remove obj->mm.lock! | expand

Commit Message

Maarten Lankhorst Oct. 16, 2020, 10:43 a.m. UTC
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(-)

Comments

Matthew Brost Oct. 20, 2020, 8:18 p.m. UTC | #1
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. 

>  	}
>  
> @@ -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.

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
Maarten Lankhorst Oct. 30, 2020, 8:43 a.m. UTC | #2
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
Thomas Hellström Oct. 30, 2020, 3:11 p.m. UTC | #3
On 10/16/20 12:43 PM, 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>

When previous review comments addressed,

Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
diff mbox series

Patch

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 */
-	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;
 	}
 
@@ -900,7 +909,11 @@  static int eb_validate_vmas(struct i915_execbuffer *eb)
 		if (err)
 			return err;
 
-		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;