Re: [PATCH] drm/amd/display: Implement functions to let DC allocate GPU memory

["Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com>]

On 2021-01-20 5:26 a.m., Christian König wrote:
> Am 19.01.21 um 21:40 schrieb Bhawanpreet Lakha:
>> From: Harry Wentland <harry.wentland@amd.com>
>>
>> [Why]
>> DC needs to communicate with PM FW through GPU memory. In order
>> to do so we need to be able to allocate memory from within DC.
>>
>> [How]
>> Call amdgpu_bo_create_kernel to allocate GPU memory and use a
>> list in amdgpu_display_manager to track our allocations so we
>> can clean them up later.
> 
> Well that looks like classic mid-layering to me with a horrible 
> implementation of the free function.
> 
> Christian.

FWIW this is only really used during device creation and destruction so 
the overhead of the free function isn't a considerable concern.

Does AMDGPU always need to know the GPU address for the allocation to 
free or should we work on fixing the callsites for this to pass it down?

We generally separate the CPU/GPU pointer but maybe it'd be best to have 
some sort of allocation object here that has both for DC's purposes.

Regards,
Nicholas Kazlauskas

> 
>>
>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
>> ---
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 +
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  9 +++++
>>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 40 +++++++++++++++++--
>>   3 files changed, 48 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index e490fc2486f7..83ec92a69cba 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -1017,6 +1017,8 @@ static int amdgpu_dm_init(struct amdgpu_device 
>> *adev)
>>       init_data.soc_bounding_box = adev->dm.soc_bounding_box;
>> +    INIT_LIST_HEAD(&adev->dm.da_list);
>> +
>>       /* Display Core create. */
>>       adev->dm.dc = dc_create(&init_data);
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h 
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> index 38bc0f88b29c..49137924a855 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> @@ -130,6 +130,13 @@ struct amdgpu_dm_backlight_caps {
>>       bool aux_support;
>>   };
>> +struct dal_allocation {
>> +    struct list_head list;
>> +    struct amdgpu_bo *bo;
>> +    void *cpu_ptr;
>> +    u64 gpu_addr;
>> +};
>> +
>>   /**
>>    * struct amdgpu_display_manager - Central amdgpu display manager 
>> device
>>    *
>> @@ -350,6 +357,8 @@ struct amdgpu_display_manager {
>>        */
>>       struct amdgpu_encoder mst_encoders[AMDGPU_DM_MAX_CRTC];
>>       bool force_timing_sync;
>> +
>> +    struct list_head da_list;
>>   };
>>   enum dsc_clock_force_state {
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c 
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> index 3244a6ea7a65..5dc426e6e785 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> @@ -652,8 +652,31 @@ void *dm_helpers_allocate_gpu_mem(
>>           size_t size,
>>           long long *addr)
>>   {
>> -    // TODO
>> -    return NULL;
>> +    struct amdgpu_device *adev = ctx->driver_context;
>> +    struct dal_allocation *da;
>> +    u32 domain = (type == DC_MEM_ALLOC_TYPE_GART) ?
>> +        AMDGPU_GEM_DOMAIN_GTT : AMDGPU_GEM_DOMAIN_VRAM;
>> +    int ret;
>> +
>> +    da = kzalloc(sizeof(struct dal_allocation), GFP_KERNEL);
>> +    if (!da)
>> +        return NULL;
>> +
>> +    ret = amdgpu_bo_create_kernel(adev, size, PAGE_SIZE,
>> +                      domain, &da->bo,
>> +                      &da->gpu_addr, &da->cpu_ptr);
>> +
>> +    *addr = da->gpu_addr;
>> +
>> +    if (ret) {
>> +        kfree(da);
>> +        return NULL;
>> +    }
>> +
>> +    /* add da to list in dm */
>> +    list_add(&da->list, &adev->dm.da_list);
>> +
>> +    return da->cpu_ptr;
>>   }
>>   void dm_helpers_free_gpu_mem(
>> @@ -661,5 +684,16 @@ void dm_helpers_free_gpu_mem(
>>           enum dc_gpu_mem_alloc_type type,
>>           void *pvMem)
>>   {
>> -    // TODO
>> +    struct amdgpu_device *adev = ctx->driver_context;
>> +    struct dal_allocation *da;
>> +
>> +    /* walk the da list in DM */
>> +    list_for_each_entry(da, &adev->dm.da_list, list) {
>> +        if (pvMem == da->cpu_ptr) {
>> +            amdgpu_bo_free_kernel(&da->bo, &da->gpu_addr, &da->cpu_ptr);
>> +            list_del(&da->list);
>> +            kfree(da);
>> +            break;
>> +        }
>> +    }
>>   }
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cnicholas.kazlauskas%40amd.com%7C65096f1a05bf4379c1cd08d8bd2dd5a1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637467351862623818%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=Krf3epZ%2FsAWOmHRhUSukqEjKJJBLwtKNEe7GWYKBG1w%3D&amp;reserved=0 
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx