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

[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 at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx