Re: [Intel-gfx] [PATCH 7/9] drm/i915: stop using ttm_bo_wait

[Matthew Auld <matthew.auld@intel.com>]

On 05/12/2022 19:58, Christian König wrote:
> Am 30.11.22 um 15:06 schrieb Daniel Vetter:
>> On Wed, 30 Nov 2022 at 14:03, Tvrtko Ursulin
>> <tvrtko.ursulin@linux.intel.com> wrote:
>>> On 29/11/2022 18:05, Matthew Auld wrote:
>>>> On Fri, 25 Nov 2022 at 11:14, Tvrtko Ursulin
>>>> <tvrtko.ursulin@linux.intel.com> wrote:
>>>>>
>>>>> + Matt
>>>>>
>>>>> On 25/11/2022 10:21, Christian König wrote:
>>>>>> TTM is just wrapping core DMA functionality here, remove the 
>>>>>> mid-layer.
>>>>>> No functional change.
>>>>>>
>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 9 ++++++---
>>>>>>     1 file changed, 6 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c 
>>>>>> b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>>>>>> index 5247d88b3c13..d409a77449a3 100644
>>>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>>>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>>>>>> @@ -599,13 +599,16 @@ i915_ttm_resource_get_st(struct 
>>>>>> drm_i915_gem_object *obj,
>>>>>>     static int i915_ttm_truncate(struct drm_i915_gem_object *obj)
>>>>>>     {
>>>>>>         struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
>>>>>> -     int err;
>>>>>> +     long err;
>>>>>>
>>>>>>         WARN_ON_ONCE(obj->mm.madv == I915_MADV_WILLNEED);
>>>>>>
>>>>>> -     err = ttm_bo_wait(bo, true, false);
>>>>>> -     if (err)
>>>>>> +     err = dma_resv_wait_timeout(bo->base.resv, 
>>>>>> DMA_RESV_USAGE_BOOKKEEP,
>>>>>> +                                 true, 15 * HZ);
>>>>> This 15 second stuck out a bit for me and then on a slightly deeper 
>>>>> look
>>>>> it seems this timeout will "leak" into a few of i915 code paths. If we
>>>>> look at the difference between the legacy shmem and ttm backend I 
>>>>> am not
>>>>> sure if the legacy one is blocking or not - but if it can block I 
>>>>> don't
>>>>> think it would have an arbitrary timeout like this. Matt your 
>>>>> thoughts?
>>>> Not sure what is meant by leak here, but the legacy shmem must also
>>>> wait/block when unbinding each VMA, before calling truncate. It's the
>>> By "leak" I meant if 15s timeout propagates into some code paths visible
>>> from userspace which with a legacy backend instead have an indefinite
>>> wait. If we have that it's probably not very good to have this
>>> inconsistency, or to apply an arbitrary timeout to those path to 
>>> start with.
>>>
>>>> same story for the ttm backend, except slightly more complicated in
>>>> that there might be no currently bound VMA, and yet the GPU could
>>>> still be accessing the pages due to async unbinds, kernel moves etc,
>>>> which the wait here (and in i915_ttm_shrink) is meant to protect
>>>> against. If the wait times out it should just fail gracefully. I guess
>>>> we could just use MAX_SCHEDULE_TIMEOUT here? Not sure if it really
>>>> matters though.
>>> Right, depends if it can leak or not to userspace and diverge between
>>> backends.
>> Generally lock_timeout() is a design bug. It's either
>> lock_interruptible (or maybe lock_killable) or try_lock, but
>> lock_timeout is just duct-tape. I haven't dug in to figure out what
>> should be here, but it smells fishy.
> 
> Independent of this discussion could I get an rb for removing 
> ttm_bo_wait() from i915?
> 
> Exactly hiding this timeout inside TTM is what always made me quite 
> nervous here.

There are also a few places in i915 calling bo_wait_ctx(), which appears 
to just wrap ttm_bo_wait(). I guess that should also be converted to 
dma_resv_wait_timeout()? Or what is the story with that?

Anyway,
Reviewed-by: Matthew Auld <matthew.auld@intel.com>

> 
> Regards,
> Christian.
> 
>> -Daniel
>