[Intel-gfx] [PATCH] drm/i915: Ensure associated VMAs are inactive when contexts are destroyed

[Chris Wilson]

On Mon, Oct 26, 2015 at 01:10:19PM +0000, Tvrtko Ursulin wrote:
> 
> On 26/10/15 12:10, Chris Wilson wrote:
> > On Mon, Oct 26, 2015 at 12:00:06PM +0000, Tvrtko Ursulin wrote:
> >>
> >> On 26/10/15 11:23, Chris Wilson wrote:
> >>> On Mon, Oct 26, 2015 at 11:05:03AM +0000, Tvrtko Ursulin wrote:
> >>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>>>
> >>>> In the following commit:
> >>>>
> >>>>      commit e9f24d5fb7cf3628b195b18ff3ac4e37937ceeae
> >>>>      Author: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>>>      Date:   Mon Oct 5 13:26:36 2015 +0100
> >>>>
> >>>>          drm/i915: Clean up associated VMAs on context destruction
> >>>>
> >>>> I added a WARN_ON assertion that VM's active list must be empty
> >>>> at the time of owning context is getting freed, but that turned
> >>>> out to be a wrong assumption.
> >>>>
> >>>> Due ordering of operations in i915_gem_object_retire__read, where
> >>>> contexts are unreferenced before VMAs are moved to the inactive
> >>>> list, the described situation can in fact happen.
> >>>
> >>> The context is being unreferenced indirectly. Adding a direct reference
> >>> here is even more bizarre.
> >>
> >> Perhaps is not the prettiest, but it sounds logical to me to ensure
> >> that order of destruction of involved object hierarchy goes from the
> >> bottom-up and is not interleaved.
> >>
> >> If you consider the active/inactive list position as part of the
> >> retire process, doing it at the very place in code, and the very
> >> object that looked to be destroyed out of sequence, to me sounded
> >> logical.
> >>
> >> How would you do it, can you think of a better way?
> > 
> > The reference is via the request. We are handling requests, it makes
> > more sense that you take the reference on the request.
> 
> Hm, so you would be happy with:

Go up another level. There is just one callsite where the reference
needs to be added across the call.

And no, I would not be happy as I see this as just futher increasing the
technical debt.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre