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@xxxxxxxxxxxxxxx>
> ---
> .../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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
