Re: [Intel-gfx] [CI 2/2] drm/i915: limit the async bind to bind_async_flags

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

On 04/03/2022 16:41, Thomas Hellström wrote:
> On Fri, 2022-03-04 at 09:59 +0000, Matthew Auld wrote:
>> If the vm doesn't request async binding, like for example with the
>> dpt,
>> then we should be able to skip the async path and avoid calling
>> i915_vm_lock_objects() altogether. Currently if we have a moving
>> fence
>> set for the BO(even though it might have signalled), we still take
>> the
>> async patch regardless of the bind_async setting, and then later
>> still
>> end up just doing i915_gem_object_wait_moving_fence() anyway.
>>
>> Alternatively we would need to add dummy scratch object which can be
>> locked, just for the dpt.
>>
>> Suggested-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_vma.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_vma.c
>> b/drivers/gpu/drm/i915/i915_vma.c
>> index 94fcdb7bd21d..4d4d3659c938 100644
>> --- a/drivers/gpu/drm/i915/i915_vma.c
>> +++ b/drivers/gpu/drm/i915/i915_vma.c
>> @@ -1360,8 +1360,7 @@ int i915_vma_pin_ww(struct i915_vma *vma,
>> struct i915_gem_ww_ctx *ww,
>>          if (flags & PIN_GLOBAL)
>>                  wakeref = intel_runtime_pm_get(&vma->vm->i915-
>>> runtime_pm);
>>   
>> -       moving = vma->obj ?
> 
> Is there a chance that "moving" will be used uninitialized later?

It looks to be initialised with NULL further up.

> 
> 
>> i915_gem_object_get_moving_fence(vma->obj) : NULL;
>> -       if (flags & vma->vm->bind_async_flags || moving) {
>> +       if (flags & vma->vm->bind_async_flags) {
>>                  /* lock VM */
>>                  err = i915_vm_lock_objects(vma->vm, ww);
>>                  if (err)
>> @@ -1375,6 +1374,7 @@ int i915_vma_pin_ww(struct i915_vma *vma,
>> struct i915_gem_ww_ctx *ww,
>>   
>>                  work->vm = i915_vm_get(vma->vm);
>>   
>> +               moving = vma->obj ?
>> i915_gem_object_get_moving_fence(vma->obj) : NULL;
> 
> IIRC, with Maarten's recent changes, vma->obj is always non-NULL.

Yup, a number of these seem to have crept back in. I was going to send a 
follow up patch to fix all of them at once.

> 
> Otherwise LGTM.
> Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>

Thanks.

> 
> 
>>                  dma_fence_work_chain(&work->base, moving);
>>   
>>                  /* Allocate enough page directories to used PTE */
> 
> /Thomas
> 
>