drm_exec changes for eviction

drm_exec changes for eviction

[Thomas Hellström]

Hi Christian, Others.

In order to support exhaustive eviction there are some changes that I
think needs to be made to drm_exec:

1) Trylock support 
(This is for ttm_bo_vm, ttm_buffer_object_init_reserved, and also for
the eviction path where I think we want to make a trylock pass before a
sleeping lock pass).
In essence this means we can't leave any contending lock unlocked until
the next sleeping lock, but rather need to relock it on
drm_exec_retry_on_contention(), meaning that that macro also gets
passed and returns an error code to handle -EINTR.

2) Snapshot unlock
After successfully obtaining backing store, we want to unlock all
evicted objects. So snapshot the drm_exec state when eviction begins,
and unlock everything down to the snapshot after successful backing
store allocation. Essentially the snapshot contains the number of
locked objects and a pointer to the prelocked object.

Any concerns?
Thanks,
Thomas

Re: drm_exec changes for eviction

[Christian König]

Am 06.05.24 um 11:46 schrieb Thomas Hellström:
> Hi Christian, Others.
>
> In order to support exhaustive eviction there are some changes that I
> think needs to be made to drm_exec:
>
> 1) Trylock support
> (This is for ttm_bo_vm, ttm_buffer_object_init_reserved, and also for
> the eviction path where I think we want to make a trylock pass before a
> sleeping lock pass).

Not sure why we need this for ttm_bo_vm, but in general that sounds like 
the right approach to me as well.

> In essence this means we can't leave any contending lock unlocked until
> the next sleeping lock, but rather need to relock it on
> drm_exec_retry_on_contention(), meaning that that macro also gets
> passed and returns an error code to handle -EINTR.

Hui what? I can't really follow what you mean here.

>
> 2) Snapshot unlock
> After successfully obtaining backing store, we want to unlock all
> evicted objects. So snapshot the drm_exec state when eviction begins,
> and unlock everything down to the snapshot after successful backing
> store allocation. Essentially the snapshot contains the number of
> locked objects and a pointer to the prelocked object.

Interesting idea, never though about that. Not sure if that makes the 
situation better or worse.

One goal of drm_exec was to be able to keep evicted BOs locked until the 
whole submission is completed and that obviously contradicts that.

Regards,
Christian.

>
> Any concerns?
> Thanks,
> Thomas
>

Re: drm_exec changes for eviction

[Thomas Hellström]

On Tue, 2024-05-07 at 15:36 +0200, Christian König wrote:
> Am 06.05.24 um 11:46 schrieb Thomas Hellström:
> > Hi Christian, Others.
> > 
> > In order to support exhaustive eviction there are some changes that
> > I
> > think needs to be made to drm_exec:
> > 
> > 1) Trylock support
> > (This is for ttm_bo_vm, ttm_buffer_object_init_reserved, and also
> > for
> > the eviction path where I think we want to make a trylock pass
> > before a
> > sleeping lock pass).
> 
> Not sure why we need this for ttm_bo_vm, but in general that sounds
> like 
> the right approach to me as well.

In current ttm_bo_vm, if the trylock fails, the mmap lock is typically
dropped while we wait for the lock to become available, and then the
fault is retried, using the fault retry mechanism. (This is only to
avoid blocking the mmap lock unnecessarily- the comment around that
code is obsolete).

> 
> > In essence this means we can't leave any contending lock unlocked
> > until
> > the next sleeping lock, but rather need to relock it on
> > drm_exec_retry_on_contention(), meaning that that macro also gets
> > passed and returns an error code to handle -EINTR.
> 
> Hui what? I can't really follow what you mean here.

Currently drm_exec keeps a reference to the contended lock, which is
not locked. And it is locked on the *next* drm_exec_lock_obj() after a
restart. So the question arises what to do if the *next* lock is a
trylock. We shouldn't really do a sleeping lock of the contended object
at that point. So the suggestion here was to do it during
drm_exec_retry_on_contention() instead. But then we face the problem
that the locking of the contended object might fail with -EINTR.

> 
> > 
> > 2) Snapshot unlock
> > After successfully obtaining backing store, we want to unlock all
> > evicted objects. So snapshot the drm_exec state when eviction
> > begins,
> > and unlock everything down to the snapshot after successful backing
> > store allocation. Essentially the snapshot contains the number of
> > locked objects and a pointer to the prelocked object.
> 
> Interesting idea, never though about that. Not sure if that makes the
> situation better or worse.
> 
> One goal of drm_exec was to be able to keep evicted BOs locked until
> the 
> whole submission is completed and that obviously contradicts that.

We've tried both approaches on i915 downstream (non-ttm). IIRC the
snapshot idea was originally brought up by Sima. Couldn't see any real
difference, but I think snapshotting should be sufficient to avoid
starvation.

/Thomas


> 
> Regards,
> Christian.
> 
> > 
> > Any concerns?
> > Thanks,
> > Thomas
> > 
>