On Tue, Sep 12, 2017 at 2:35 PM, Christian König <deathsimple at vodafone.de> wrote: > 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. Ah, yes, I see now. Can you add that to the commit message? > > >> >>> 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. Sorry, I was mixing this up with the pinning code in my head. Objection withdrawn. With the updated description, patch is: Reviewed-by: Alex Deucher <alexander.deucher at amd.com> Alex > > 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 > > > > _______________________________________________ > amd-gfx mailing list > amd-gfx at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
