[PATCH 2/2] drm/amdgpu: restructure amdgpu_fill_buffer

[Christian König]

Am 28.01.22 um 16:55 schrieb Felix Kuehling:
>
> Am 2022-01-28 um 10:16 schrieb Christian König:
>> [SNIP]
>> -    if (bo->tbo.resource->mem_type == AMDGPU_PL_PREEMPT) {
>
> As far as I can see, you didn't add this check back elsewhere. The 
> promise for preemptible BOs is, that we never have to wait for the GPU 
> to finish accessing the BOs because the context using it is 
> preemptible. This was a necessary condition to disable GTT usage 
> accounting for such BOs. If you allow filling such BOs with this 
> function, you're breaking that promise.

That's now part of amdgpu_ttm_map_buffer(), but unfortunately as 
BUG_ON(). I've added a patch to change that into a warning.

[SNIP]
>> +        cur_size = min_t(u64, dst.size, AMDGPU_GTT_MAX_TRANSFER_BYTES);
>
> For VRAM, the cur_size could be much bigger because we're not limited 
> by the GART transfer window. I'm pretty sure that 2MB is going to add 
> too much overhead. For the extreme 60GB BO example, it would require 
> 30-thousand IBs and IB frames to fill the entire buffer. That's a lot 
> of VMID-switching, IB execution, fence signalling, interrupts, etc.

I've restructured the mapping function so that we can now copy/fill in 
256MiB chunks when no GART window is involved.

>
>
>> +
>> +        r = amdgpu_ttm_map_buffer(&bo->tbo, bo->tbo.resource, &dst,
>> +                      PFN_UP(cur_size), 1, ring, false,
>> +                      &to);
>> +        if (r)
>> +            goto error;
>> +
>> +        r = amdgpu_ttm_fill_mem(ring, src_data, to, cur_size, resv,
>> +                    &next, false);
>
> If amdgpu_ttm_map_buffer updated the page tables, shouldn't the 
> vm_needs_flush parameter be true? This flag should probably be 
> returned by amdgpu_ttm_map_buffer.

Ah, yes. That's indeed a typo. For now I've didn't added the 
vm_needs_flush parameter, but that's something we could optimize.

Regards,
Christian.

>
> Regards,
>   Felix
>
>
>> +        if (r)
>> +            goto error;
>> +
>> +        dma_fence_put(fence);
>> +        fence = next;
>> +
>> +        amdgpu_res_next(&dst, cur_size);
>> +    }
>> +error:
>> +    mutex_unlock(&adev->mman.gtt_window_lock);
>> +    if (f)
>> +        *f = dma_fence_get(fence);
>> +    dma_fence_put(fence);
>> +    return r;
>> +}
>> +
>>   /**
>>    * amdgpu_ttm_evict_resources - evict memory buffers
>>    * @adev: amdgpu device object
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>> index d9691f262f16..bcd34592e45d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>> @@ -35,6 +35,8 @@
>>     #define AMDGPU_GTT_MAX_TRANSFER_SIZE    512
>>   #define AMDGPU_GTT_NUM_TRANSFER_WINDOWS    2
>> +#define AMDGPU_GTT_MAX_TRANSFER_BYTES (AMDGPU_GTT_MAX_TRANSFER_SIZE * \
>> +                     AMDGPU_GPU_PAGE_SIZE)
>>     #define AMDGPU_POISON    0xd0bed0be