Am 12.05.21 um 15:02 schrieb Thomas Hellström:
On Wed, 2021-05-12 at 09:09 +0200, Christian König wrote:
Am 12.05.21 um 09:05 schrieb Thomas Hellström:
On Wed, 2021-05-12 at 08:57 +0200, Christian König wrote:
Am 11.05.21 um 16:28 schrieb Thomas Hellström:
On 5/11/21 4:09 PM, Christian König wrote:
Am 11.05.21 um 16:06 schrieb Thomas Hellström (Intel):
On 5/11/21 3:58 PM, Christian König wrote:
Am 11.05.21 um 15:25 schrieb Thomas Hellström:
Most logical place to introduce TTM buffer objects is
as an
i915
gem object backend. We need to add some ops to account
for
added
functionality like delayed delete and LRU list
manipulation.
Initially we support only LMEM and SYSTEM memory, but
SYSTEM
(which in this case means evicted LMEM objects) is not
visible to i915 GEM yet. The plan is to move the i915
gem
system
region
over to the TTM system memory type in upcoming patches.
We set up GPU bindings directly both from LMEM and from
the
system
region,
as there is no need to use the legacy TTM_TT memory
type.
We reserve
that for future porting of GGTT bindings to TTM.
There are some changes to TTM to allow for purging
system
memory
buffer
objects and to refuse swapping of some objects:
Unfortunately i915
gem
still relies heavily on short-term object pinning, and
we've
chosen to
keep short-term-pinned buffer objects on the TTM LRU
lists
for now,
meaning that we need some sort of mechanism to tell TTM
they are not
swappable. A longer term goal is to get rid of the
short-
term
pinning.
Well just use the eviction_valuable interface for this.
Yes, we do that for vram/lmem eviction, but we have nothing
similar
for system swapping. Do I understand you correctly that you
want me
to add a call to eviction_valuable() also for that instead
of
swap_possible()?
You should already have that. eviction_valuable is called in
both
cases.
Hmm. I can only see it called from ttm_mem_evict_first() which
is
not
in the swapping path? Or do I miss something?
Mhm, looks like my recollection was wrong. We should probably
move
the
call into the ttm_bo_evict_swapout_allowable() function.
Yes, I think we also need a convention whether it's called dma_resv
locked or not, since the helper accesses bo->mem, which should
really
only be done under reservation. At the same point, there is value
in
calling this function while holding the LRU lock.
You actually need to call it while holding the lock because eviction
otherwise ends up in an endless loop.
Trying to fix that for years, but so far no luck with that.
Also, I wonder whether implementations of this callback might
encounter
unexpected data when called from the swapout path, because at least
the
helper assumes it not in system memory, since it is accessing bo-
mem.start.
So unless we use a separate callback for swapout, there's some
auditing
to be done.
Please audit the existing callbacks and move the callback into the
function after doing that.
Thanks,
Christian.
Would it be OK if I also move the kref_get_unless_zero() to before
ttm_bo_evict_swapout_allowable() to make the code less sensitive to
surprises?
No, because then you need a kref_put while holding the spinlock which is
not allowed.
Christian.
/Thomas
Pls let me know what you think.
Thanks,
Thomas
Christian.
Thanks,
Thomas
