[Intel-gfx] [PATCH 1/3] drm/i915: Retire the VMA's fence tracker before unbinding

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-06-21 10:17:49)
> 
> On 20/06/2017 17:05, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2017-06-20 16:55:12)
> >>
> >> On 20/06/2017 13:43, Chris Wilson wrote:
> >>> Since we may track unfenced access (GPU access to the vma that
> >>> explicitly requires no fence), vma->last_fence may be set without any
> >>
> >> Is this the gen < 4 code path? There is no actual fence in this case?
> > 
> > Yes, this is for old hw that used a fence for GPU access, and in this
> > case indeed there is no fence and we are tracking the access for when
> > the fence is not wanted to ensure that don't install a fence before the
> > GPU finished accessing the region.
> > 
> >>> attached fence (vma->fence) and so will not be flushed when we call
> >>> i915_vma_put_fence(). Since we stopped doing a full retire of the
> >>> activity trackers for unbind, we need to explicitly retire each tracker.
> >>>
> >>> Fixes: b0decaf75bd9 ("drm/i915: Track active vma requests")
> >>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/i915_vma.c | 5 +++++
> >>>    1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> >>> index 532c709febbd..1cfe137cdc32 100644
> >>> --- a/drivers/gpu/drm/i915/i915_vma.c
> >>> +++ b/drivers/gpu/drm/i915/i915_vma.c
> >>> @@ -672,6 +672,11 @@ int i915_vma_unbind(struct i915_vma *vma)
> >>>                                break;
> >>>                }
> >>>    
> >>> +             if (!ret) {
> >>> +                     ret = i915_gem_active_retire(&vma->last_fence,
> >>> +                                                  &vma->vm->i915->drm.struct_mutex);
> >>> +             }
> >>> +
> >>>                __i915_vma_unpin(vma);
> >>>                if (ret)
> >>>                        return ret;
> >>>
> >>
> >> Looks safe anyway, but I'd like to understand how exactly it happens and
> >> if it is still possible after patch 2/3.
> > 
> > For gen2/3, we still use last_fence. For others it was accidental and
> > should be no more after patch 2. I considered add a GEM_BUG_ON to
> > i915_move_to_active but didn't have a convenient i915 pointer, and I
> > have grander plans :)
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Thanks for accepting my poor grammar :)

Pushed the remaining pair,
-Chris