[Intel-gfx] [PATCH] drm/i915/ttm: Fix incorrect assumptions about ttm_bo_validate() semantics

[Thomas Hellström]

On 6/18/21 12:53 PM, Matthew Auld wrote:
> On Fri, 18 Jun 2021 at 09:31, Thomas Hellström
> <thomas.hellstrom at linux.intel.com> wrote:
>> We have assumed that if the current placement was not the requested
>> placement, but instead one of the busy placements, a TTM move would have
>> been triggered. That is not the case.
>>
>> So when we initially place LMEM objects in "Limbo", (that is system
>> placement without any pages allocated), to be able to defer clearing
>> objects until first get_pages(), the first get_pages() would happily keep
>> objects in system memory if that is one of the allowed placements. And
>> since we don't yet support i915 GEM system memory from TTM, everything
>> breaks apart.
>>
>> So make sure we try the requested placement first, if no eviction is
>> needed. If that fails, retry with all allowed placements also allowing
>> evictions. Also make sure we handle TTM failure codes correctly.
>>
>> Also temporarily (until we support i915 GEM system on TTM), restrict
>> allowed placements to the requested placement to avoid things falling
>> apart should LMEM be full.
>>
>> Fixes: 38f28c0695c0 ("drm/i915/ttm: Calculate the object placement at get_pages time)
>> Signed-off-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 61 +++++++++++++++++++++++--
>>   1 file changed, 58 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> index df46535cca47..4bb0440f693c 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> @@ -64,6 +64,30 @@ static struct ttm_placement i915_sys_placement = {
>>          .busy_placement = &sys_placement_flags,
>>   };
>>
>> +static int i915_ttm_err_to_gem(int err)
>> +{
>> +       /* Fastpath */
>> +       if (likely(!err))
>> +               return 0;
>> +
>> +       switch (err) {
>> +       case -EBUSY:
>> +               /*
>> +                * TTM likes to convert -EDEADLK to -EBUSY, and wants us to
>> +                * restart the operation, since we don't record the contending
>> +                * lock. We use -EAGAIN to restart.
>> +                */
>> +               return -EAGAIN;
>> +       case -ENOSPC:
>> +               /* Memory type / region is full, and we can't evict. */
>> +               return -ENXIO;
> ttm system will return -ENOMEM right?
>
> Reviewed-by: Matthew Auld <matthew.auld at intel.com>

Hmm, Yes, I suppose so. Will that need some mangling before handing over 
to GEM?

Thanks for reviewing!

Thomas