[PATCH 3/5] drm/amdgpu: fix and cleanup amdgpu_bo_create
Christian König
deathsimple at vodafone.de
Tue Sep 12 18:35:49 UTC 2017
Am 12.09.2017 um 17:55 schrieb Deucher, Alexander:
>> -----Original Message-----
>> From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On Behalf
>> Of Christian König
>> Sent: Tuesday, September 12, 2017 5:09 AM
>> To: amd-gfx at lists.freedesktop.org
>> Subject: [PATCH 3/5] drm/amdgpu: fix and cleanup amdgpu_bo_create
>>
>> From: Christian König <christian.koenig at amd.com>
>>
>> Fix USWC handling by cleaning up the function and removing
>> quite a bit of unused code.
> Can you clarify what was broken?
We adjusted the BO flags for USWC handling, but those never took effect
because the placement was passed in instead of generated inside this
function.
>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 83 +++++++++------------
>> ---------
>> drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 8 ---
>> 2 files changed, 23 insertions(+), 68 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index 52d0109..726a662 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -64,11 +64,12 @@ bool amdgpu_ttm_bo_is_amdgpu_bo(struct
>> ttm_buffer_object *bo)
>> return false;
>> }
>>
>> -static void amdgpu_ttm_placement_init(struct amdgpu_device *adev,
>> - struct ttm_placement *placement,
>> - struct ttm_place *places,
>> - u32 domain, u64 flags)
>> +void amdgpu_ttm_placement_from_domain(struct amdgpu_bo *abo, u32
>> domain)
>> {
>> + struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev);
>> + struct ttm_placement *placement = &abo->placement;
>> + struct ttm_place *places = abo->placements;
>> + u64 flags = abo->flags;
>> u32 c = 0;
>>
>> if (domain & AMDGPU_GEM_DOMAIN_VRAM) {
>> @@ -151,27 +152,6 @@ static void amdgpu_ttm_placement_init(struct
>> amdgpu_device *adev,
>> placement->busy_placement = places;
>> }
>>
>> -void amdgpu_ttm_placement_from_domain(struct amdgpu_bo *abo, u32
>> domain)
>> -{
>> - struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev);
>> -
>> - amdgpu_ttm_placement_init(adev, &abo->placement, abo-
>>> placements,
>> - domain, abo->flags);
>> -}
>> -
>> -static void amdgpu_fill_placement_to_bo(struct amdgpu_bo *bo,
>> - struct ttm_placement *placement)
>> -{
>> - BUG_ON(placement->num_placement >
>> (AMDGPU_GEM_DOMAIN_MAX + 1));
>> -
>> - memcpy(bo->placements, placement->placement,
>> - placement->num_placement * sizeof(struct ttm_place));
>> - bo->placement.num_placement = placement->num_placement;
>> - bo->placement.num_busy_placement = placement-
>>> num_busy_placement;
>> - bo->placement.placement = bo->placements;
>> - bo->placement.busy_placement = bo->placements;
>> -}
>> -
>> /**
>> * amdgpu_bo_create_reserved - create reserved BO for kernel use
>> *
>> @@ -303,14 +283,13 @@ void amdgpu_bo_free_kernel(struct amdgpu_bo
>> **bo, u64 *gpu_addr,
>> *cpu_addr = NULL;
>> }
>>
>> -int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
>> - unsigned long size, int byte_align,
>> - bool kernel, u32 domain, u64 flags,
>> - struct sg_table *sg,
>> - struct ttm_placement *placement,
>> - struct reservation_object *resv,
>> - uint64_t init_value,
>> - struct amdgpu_bo **bo_ptr)
>> +static int amdgpu_bo_do_create(struct amdgpu_device *adev,
>> + unsigned long size, int byte_align,
>> + bool kernel, u32 domain, u64 flags,
>> + struct sg_table *sg,
>> + struct reservation_object *resv,
>> + uint64_t init_value,
>> + struct amdgpu_bo **bo_ptr)
> Still seems like amdgpu_bo_create_restricted is a better name than do_create.
How about amdgpu_bo_create_impl ?
Point is the function isn't restricted in any way any more.
Christian.
>
>> {
>> struct amdgpu_bo *bo;
>> enum ttm_bo_type type;
>> @@ -384,10 +363,11 @@ int amdgpu_bo_create_restricted(struct
>> amdgpu_device *adev,
>> bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
>> #endif
>>
>> - amdgpu_fill_placement_to_bo(bo, placement);
>> - /* Kernel allocation are uninterruptible */
>> + bo->tbo.bdev = &adev->mman.bdev;
>> + amdgpu_ttm_placement_from_domain(bo, domain);
>>
>> initial_bytes_moved = atomic64_read(&adev->num_bytes_moved);
>> + /* Kernel allocation are uninterruptible */
>> r = ttm_bo_init_reserved(&adev->mman.bdev, &bo->tbo, size,
>> type,
>> &bo->placement, page_align, !kernel, NULL,
>> acc_size, sg, resv,
>> &amdgpu_ttm_bo_destroy);
>> @@ -442,27 +422,17 @@ static int amdgpu_bo_create_shadow(struct
>> amdgpu_device *adev,
>> unsigned long size, int byte_align,
>> struct amdgpu_bo *bo)
>> {
>> - struct ttm_placement placement = {0};
>> - struct ttm_place placements[AMDGPU_GEM_DOMAIN_MAX + 1];
>> int r;
>>
>> if (bo->shadow)
>> return 0;
>>
>> - memset(&placements, 0, sizeof(placements));
>> - amdgpu_ttm_placement_init(adev, &placement, placements,
>> - AMDGPU_GEM_DOMAIN_GTT,
>> - AMDGPU_GEM_CREATE_CPU_GTT_USWC
>> |
>> - AMDGPU_GEM_CREATE_SHADOW);
>> -
>> - r = amdgpu_bo_create_restricted(adev, size, byte_align, true,
>> - AMDGPU_GEM_DOMAIN_GTT,
>> -
>> AMDGPU_GEM_CREATE_CPU_GTT_USWC |
>> - AMDGPU_GEM_CREATE_SHADOW,
>> - NULL, &placement,
>> - bo->tbo.resv,
>> - 0,
>> - &bo->shadow);
>> + r = amdgpu_bo_do_create(adev, size, byte_align, true,
>> + AMDGPU_GEM_DOMAIN_GTT,
>> + AMDGPU_GEM_CREATE_CPU_GTT_USWC |
>> + AMDGPU_GEM_CREATE_SHADOW,
>> + NULL, bo->tbo.resv, 0,
>> + &bo->shadow);
>> if (!r) {
>> bo->shadow->parent = amdgpu_bo_ref(bo);
>> mutex_lock(&adev->shadow_list_lock);
>> @@ -484,18 +454,11 @@ int amdgpu_bo_create(struct amdgpu_device
>> *adev,
>> uint64_t init_value,
>> struct amdgpu_bo **bo_ptr)
>> {
>> - struct ttm_placement placement = {0};
>> - struct ttm_place placements[AMDGPU_GEM_DOMAIN_MAX + 1];
>> uint64_t parent_flags = flags & ~AMDGPU_GEM_CREATE_SHADOW;
>> int r;
>>
>> - memset(&placements, 0, sizeof(placements));
>> - amdgpu_ttm_placement_init(adev, &placement, placements,
>> - domain, parent_flags);
>> -
>> - r = amdgpu_bo_create_restricted(adev, size, byte_align, kernel,
>> domain,
>> - parent_flags, sg, &placement, resv,
>> - init_value, bo_ptr);
>> + r = amdgpu_bo_do_create(adev, size, byte_align, kernel, domain,
>> + parent_flags, sg, resv, init_value, bo_ptr);
>> if (r)
>> return r;
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> index a4891be..39b6bf6 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> @@ -195,14 +195,6 @@ int amdgpu_bo_create(struct amdgpu_device
>> *adev,
>> struct reservation_object *resv,
>> uint64_t init_value,
>> struct amdgpu_bo **bo_ptr);
>> -int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
>> - unsigned long size, int byte_align,
>> - bool kernel, u32 domain, u64 flags,
>> - struct sg_table *sg,
>> - struct ttm_placement *placement,
>> - struct reservation_object *resv,
>> - uint64_t init_value,
>> - struct amdgpu_bo **bo_ptr);
>> int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
>> unsigned long size, int align,
>> u32 domain, struct amdgpu_bo **bo_ptr,
>> --
>> 2.7.4
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
More information about the amd-gfx
mailing list