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 <[email protected]>
>> ---
>> .../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
>> [email protected]
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
