Re: [Intel-gfx] [PATCH v2 00/16] drm/i915: Remove short term pins from execbuf.

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	intel-gfx@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v2 00/16] drm/i915: Remove short term pins from execbuf.
Date: Tue, 30 Nov 2021 18:38:43 +0000	[thread overview]
Message-ID: <ea3869e7-a4f7-52a6-0ef9-7531655d5647@linux.intel.com> (raw)
In-Reply-To: <b40c5455-7b60-7c97-9fcd-fba67ed71f6d@linux.intel.com>

On 30/11/2021 11:17, Maarten Lankhorst wrote:
> On 30-11-2021 09:54, Tvrtko Ursulin wrote:
>>
>> Hi,
>>
>> On 29/11/2021 13:47, Maarten Lankhorst wrote:
>>> New version of the series, with feedback from previous series added.
>>
>> If there was a cover letter sent for this work in the past could you please keep attaching it? Or if there wasn't, could you please write one?
>>
>> I am worried about two things. First is that we need to have a high level overview of the rules/design changes documented so third party people have any hope of getting code right after this lands. (Where we are, where we are going, how we will get there, how far did we get and when we will get to the end.)
>>
>> Second is that when parts of the series land piecemeal (Which they have in this right, right?), it gets very hard to write up a maintainer level changelog.
> 
> The preparation part is to ensure we always hold vma->obj->resv when unbinding.
> 
> The first preparation series ensured vma->obj always existed. This was not the case for mock gtt and gen6 aliasing gtt. This allowed us to remove all the special handling for those uncommon cases, and actually enforce we can always take that lock. This part is merged.

Sounds good. But also mention the high level motivation for why we 
always want to hold vma->obj->resv when unbinding in the introduction as 
well.

> 
> Patch 2-11 in this series adds the vma->obj->resv to eviction and shrinker. Those are the only parts where we don't take the lock yet.
> 
> After that, we always hold the lock when required, and we can start requiring the obj-> resv lock when unbinding. This is completed in patch 15.
> 
> With that fixed, removing short term pins can be done, because for unbind we now always take obj->resv, so holding obj->resv during execbuf submission is sufficient, and all short term pinning can be removed.

I'd also like the cover letter to contain a high level description on 
_why_ is removing short term pins needed or beneficial.

What was the flow and object lifetimes so far, and what it will be going 
forward etc.

> 
> We only pin temporarily when calling i915_gem_evict_vm in execbuf, which could also be handled in theory by just marking all objects as unpinned.
> 
> As a bonus, using TTM for delayed eviction on all objects becomes easy, just need to get rid of i915_active in i915_vma, as it keeps the object refcount alive.
> 
> Remainder is removing refcount to i915_vma, to make it a real

Sounds on the right track with maybe a bit more text so the readers can 
easily understand it on the higher level.

> 
>> But in any case, even on the mundane process level, we need to have cover letters for any non trivial work was the conclusion since some time ago.
> 
> Here you go! I hope it explains the reasoning.

It is on the right track. I think just needs to be expanded a bit with 
high level direction and plan, pointing out where in the grand scheme 
this series is. And then don't forget to add the improved text as cover 
letter when sending next time please.

Regards,

Tvrtko