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

[Christian König]

Am 20.01.21 um 15:16 schrieb Kazlauskas, Nicholas:
> 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 at 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?

No, we only need to know the BO. The GPU address pointer is only passed 
in so that it can be set to NULL and the CPU address pointer is used to 
judge if we need to kunmap() or not.

> 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.

You need to pass around either the BO pointer directly or a structure 
containing it, but using the CPU mapping address to determine the BO is 
a really no-go.

Regards,
Christian.

>
> Regards,
> Nicholas Kazlauskas
>
>>
>>>
>>> Signed-off-by: Harry Wentland <harry.wentland at 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 at lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7Cnicholas.kazlauskas%40amd.com%7C65096f1a05bf4379c1cd08d8bd2dd5a1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637467351862623818%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Krf3epZ%2FsAWOmHRhUSukqEjKJJBLwtKNEe7GWYKBG1w%3D&reserved=0 
>>
>