Was the change to true from the only use of intr in amdgpu_bo_sync_wait_resv intentional? If so, would it not make sense to remove the argument from the function signature while the API is changing?
On 1/23/20 8:21 AM, Christian König wrote: > If provided we only sync to the BOs reservation > object and no longer to the root PD. > > v2: update comment, cleanup amdgpu_bo_sync_wait_resv > v3: use correct reservation object while clearing > > Signed-off-by: Christian König <[email protected]> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 37 ++++++++++++++++++++------ > drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 3 +++ > drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 7 ----- > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 41 > +++++++++++++++++------------ > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 4 +-- > drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c | 22 ++++------------ > drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 15 +++-------- > 7 files changed, 67 insertions(+), 62 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c > index 46c76e2e1281..c70bbdda078c 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c > @@ -1403,30 +1403,51 @@ void amdgpu_bo_fence(struct amdgpu_bo *bo, struct > dma_fence *fence, > } > > /** > - * amdgpu_sync_wait_resv - Wait for BO reservation fences > + * amdgpu_bo_sync_wait_resv - Wait for BO reservation fences > * > - * @bo: buffer object > + * @adev: amdgpu device pointer > + * @resv: reservation object to sync to > + * @sync_mode: synchronization mode > * @owner: fence owner > * @intr: Whether the wait is interruptible > * > + * Extract the fences from the reservation object and waits for them to > finish. > + * > * Returns: > * 0 on success, errno otherwise. > */ > -int amdgpu_bo_sync_wait(struct amdgpu_bo *bo, void *owner, bool intr) > +int amdgpu_bo_sync_wait_resv(struct amdgpu_device *adev, struct dma_resv > *resv, > + enum amdgpu_sync_mode sync_mode, void *owner, > + bool intr) > { > - struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); > struct amdgpu_sync sync; > int r; > > amdgpu_sync_create(&sync); > - amdgpu_sync_resv(adev, &sync, bo->tbo.base.resv, > - AMDGPU_SYNC_NE_OWNER, owner); > - r = amdgpu_sync_wait(&sync, intr); > + amdgpu_sync_resv(adev, &sync, resv, sync_mode, owner); > + r = amdgpu_sync_wait(&sync, true); > amdgpu_sync_free(&sync); > - > return r; > } > > +/** > + * amdgpu_bo_sync_wait - Wrapper for amdgpu_bo_sync_wait_resv > + * @bo: buffer object to wait for > + * @owner: fence owner > + * @intr: Whether the wait is interruptible > + * > + * Wrapper to wait for fences in a BO. > + * Returns: > + * 0 on success, errno otherwise. > + */ > +int amdgpu_bo_sync_wait(struct amdgpu_bo *bo, void *owner, bool intr) > +{ > + struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); > + > + return amdgpu_bo_sync_wait_resv(adev, bo->tbo.base.resv, > + AMDGPU_SYNC_NE_OWNER, owner, intr); > +} > + > /** > * amdgpu_bo_gpu_offset - return GPU offset of bo > * @bo: amdgpu object for which we query the offset > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h > b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h > index 2eeafc77c9c1..96d805889e8d 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h > @@ -283,6 +283,9 @@ void amdgpu_bo_release_notify(struct ttm_buffer_object > *bo); > int amdgpu_bo_fault_reserve_notify(struct ttm_buffer_object *bo); > void amdgpu_bo_fence(struct amdgpu_bo *bo, struct dma_fence *fence, > bool shared); > +int amdgpu_bo_sync_wait_resv(struct amdgpu_device *adev, struct dma_resv > *resv, > + enum amdgpu_sync_mode sync_mode, void *owner, > + bool intr); > int amdgpu_bo_sync_wait(struct amdgpu_bo *bo, void *owner, bool intr); > u64 amdgpu_bo_gpu_offset(struct amdgpu_bo *bo); > int amdgpu_bo_validate(struct amdgpu_bo *bo); > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c > index 9f42032676da..b86392253696 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c > @@ -249,13 +249,6 @@ int amdgpu_sync_resv(struct amdgpu_device *adev, struct > amdgpu_sync *sync, > owner != AMDGPU_FENCE_OWNER_UNDEFINED) > continue; > > - /* VM updates only sync with moves but not with user > - * command submissions or KFD evictions fences > - */ > - if (fence_owner != AMDGPU_FENCE_OWNER_UNDEFINED && > - owner == AMDGPU_FENCE_OWNER_VM) > - continue; > - > /* Ignore fences depending on the sync mode */ > switch (mode) { > case AMDGPU_SYNC_ALWAYS: > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > index 0f79c17118bf..c268aa14381e 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > @@ -797,7 +797,7 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev, > params.vm = vm; > params.direct = direct; > > - r = vm->update_funcs->prepare(¶ms, AMDGPU_FENCE_OWNER_KFD, NULL); > + r = vm->update_funcs->prepare(¶ms, NULL, AMDGPU_SYNC_EXPLICIT); > if (r) > return r; > > @@ -1293,7 +1293,7 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev, > params.vm = vm; > params.direct = direct; > > - r = vm->update_funcs->prepare(¶ms, AMDGPU_FENCE_OWNER_VM, NULL); > + r = vm->update_funcs->prepare(¶ms, NULL, AMDGPU_SYNC_EXPLICIT); > if (r) > return r; > > @@ -1552,7 +1552,7 @@ static int amdgpu_vm_update_ptes(struct > amdgpu_vm_update_params *params, > * @adev: amdgpu_device pointer > * @vm: requested vm > * @direct: direct submission in a page fault > - * @exclusive: fence we need to sync to > + * @resv: fences we need to sync to > * @start: start of mapped range > * @last: last mapped entry > * @flags: flags for the entries > @@ -1567,14 +1567,14 @@ static int amdgpu_vm_update_ptes(struct > amdgpu_vm_update_params *params, > */ > static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev, > struct amdgpu_vm *vm, bool direct, > - struct dma_fence *exclusive, > + struct dma_resv *resv, > uint64_t start, uint64_t last, > uint64_t flags, uint64_t addr, > dma_addr_t *pages_addr, > struct dma_fence **fence) > { > struct amdgpu_vm_update_params params; > - void *owner = AMDGPU_FENCE_OWNER_VM; > + enum amdgpu_sync_mode sync_mode; > int r; > > memset(¶ms, 0, sizeof(params)); > @@ -1583,9 +1583,13 @@ static int amdgpu_vm_bo_update_mapping(struct > amdgpu_device *adev, > params.direct = direct; > params.pages_addr = pages_addr; > > - /* sync to everything except eviction fences on unmapping */ > + /* Implicitly sync to command submissions in the same VM before > + * unmapping. Sync to moving fences before mapping. > + */ > if (!(flags & AMDGPU_PTE_VALID)) > - owner = AMDGPU_FENCE_OWNER_KFD; > + sync_mode = AMDGPU_SYNC_EQ_OWNER; > + else > + sync_mode = AMDGPU_SYNC_EXPLICIT; > > amdgpu_vm_eviction_lock(vm); > if (vm->evicting) { > @@ -1593,7 +1597,7 @@ static int amdgpu_vm_bo_update_mapping(struct > amdgpu_device *adev, > goto error_unlock; > } > > - r = vm->update_funcs->prepare(¶ms, owner, exclusive); > + r = vm->update_funcs->prepare(¶ms, resv, sync_mode); > if (r) > goto error_unlock; > > @@ -1612,7 +1616,7 @@ static int amdgpu_vm_bo_update_mapping(struct > amdgpu_device *adev, > * amdgpu_vm_bo_split_mapping - split a mapping into smaller chunks > * > * @adev: amdgpu_device pointer > - * @exclusive: fence we need to sync to > + * @resv: fences we need to sync to > * @pages_addr: DMA addresses to use for mapping > * @vm: requested vm > * @mapping: mapped range and flags to use for the update > @@ -1628,7 +1632,7 @@ static int amdgpu_vm_bo_update_mapping(struct > amdgpu_device *adev, > * 0 for success, -EINVAL for failure. > */ > static int amdgpu_vm_bo_split_mapping(struct amdgpu_device *adev, > - struct dma_fence *exclusive, > + struct dma_resv *resv, > dma_addr_t *pages_addr, > struct amdgpu_vm *vm, > struct amdgpu_bo_va_mapping *mapping, > @@ -1704,7 +1708,7 @@ static int amdgpu_vm_bo_split_mapping(struct > amdgpu_device *adev, > } > > last = min((uint64_t)mapping->last, start + max_entries - 1); > - r = amdgpu_vm_bo_update_mapping(adev, vm, false, exclusive, > + r = amdgpu_vm_bo_update_mapping(adev, vm, false, resv, > start, last, flags, addr, > dma_addr, fence); > if (r) > @@ -1743,7 +1747,8 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, > struct amdgpu_bo_va *bo_va, > dma_addr_t *pages_addr = NULL; > struct ttm_mem_reg *mem; > struct drm_mm_node *nodes; > - struct dma_fence *exclusive, **last_update; > + struct dma_fence **last_update; > + struct dma_resv *resv; > uint64_t flags; > struct amdgpu_device *bo_adev = adev; > int r; > @@ -1751,7 +1756,7 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, > struct amdgpu_bo_va *bo_va, > if (clear || !bo) { > mem = NULL; > nodes = NULL; > - exclusive = NULL; > + resv = vm->root.base.bo->tbo.base.resv; > } else { > struct ttm_dma_tt *ttm; > > @@ -1761,7 +1766,7 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, > struct amdgpu_bo_va *bo_va, > ttm = container_of(bo->tbo.ttm, struct ttm_dma_tt, ttm); > pages_addr = ttm->dma_address; > } > - exclusive = bo->tbo.moving; > + resv = bo->tbo.base.resv; > } > > if (bo) { > @@ -1775,7 +1780,8 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, > struct amdgpu_bo_va *bo_va, > flags = 0x0; > } > > - if (clear || (bo && bo->tbo.base.resv == > vm->root.base.bo->tbo.base.resv)) > + if (clear || (bo && bo->tbo.base.resv == > + vm->root.base.bo->tbo.base.resv)) > last_update = &vm->last_update; > else > last_update = &bo_va->last_pt_update; > @@ -1789,7 +1795,7 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, > struct amdgpu_bo_va *bo_va, > } > > list_for_each_entry(mapping, &bo_va->invalids, list) { > - r = amdgpu_vm_bo_split_mapping(adev, exclusive, pages_addr, vm, > + r = amdgpu_vm_bo_split_mapping(adev, resv, pages_addr, vm, > mapping, flags, bo_adev, nodes, > last_update); > if (r) > @@ -1984,6 +1990,7 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev, > struct amdgpu_vm *vm, > struct dma_fence **fence) > { > + struct dma_resv *resv = vm->root.base.bo->tbo.base.resv; > struct amdgpu_bo_va_mapping *mapping; > uint64_t init_pte_value = 0; > struct dma_fence *f = NULL; > @@ -1998,7 +2005,7 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev, > mapping->start < AMDGPU_GMC_HOLE_START) > init_pte_value = AMDGPU_PTE_DEFAULT_ATC; > > - r = amdgpu_vm_bo_update_mapping(adev, vm, false, NULL, > + r = amdgpu_vm_bo_update_mapping(adev, vm, false, resv, > mapping->start, mapping->last, > init_pte_value, 0, NULL, &f); > amdgpu_vm_free_mapping(adev, vm, mapping, f); > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h > index c21a36bebc0c..b5705fcfc935 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h > @@ -230,8 +230,8 @@ struct amdgpu_vm_update_params { > > struct amdgpu_vm_update_funcs { > int (*map_table)(struct amdgpu_bo *bo); > - int (*prepare)(struct amdgpu_vm_update_params *p, void * owner, > - struct dma_fence *exclusive); > + int (*prepare)(struct amdgpu_vm_update_params *p, struct dma_resv *resv, > + enum amdgpu_sync_mode sync_mode); > int (*update)(struct amdgpu_vm_update_params *p, > struct amdgpu_bo *bo, uint64_t pe, uint64_t addr, > unsigned count, uint32_t incr, uint64_t flags); > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c > index 68b013be3837..e38516304070 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c > @@ -44,26 +44,14 @@ static int amdgpu_vm_cpu_map_table(struct amdgpu_bo > *table) > * Returns: > * Negativ errno, 0 for success. > */ > -static int amdgpu_vm_cpu_prepare(struct amdgpu_vm_update_params *p, void > *owner, > - struct dma_fence *exclusive) > +static int amdgpu_vm_cpu_prepare(struct amdgpu_vm_update_params *p, > + struct dma_resv *resv, > + enum amdgpu_sync_mode sync_mode) > { > - int r; > - > - /* Wait for any BO move to be completed */ > - if (exclusive) { > - r = dma_fence_wait(exclusive, true); > - if (unlikely(r)) > - return r; > - } > - > - /* Don't wait for submissions during page fault */ > - if (p->direct) > + if (!resv) > return 0; > > - /* Wait for PT BOs to be idle. PTs share the same resv. object > - * as the root PD BO > - */ > - return amdgpu_bo_sync_wait(p->vm->root.base.bo, owner, true); > + return amdgpu_bo_sync_wait_resv(p->adev, resv, sync_mode, p->vm, true); > } > > /** > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c > index ab6481751763..4cc7881f438c 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c > @@ -58,9 +58,9 @@ static int amdgpu_vm_sdma_map_table(struct amdgpu_bo *table) > * Negativ errno, 0 for success. > */ > static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p, > - void *owner, struct dma_fence *exclusive) > + struct dma_resv *resv, > + enum amdgpu_sync_mode sync_mode) > { > - struct amdgpu_bo *root = p->vm->root.base.bo; > unsigned int ndw = AMDGPU_VM_SDMA_MIN_NUM_DW; > int r; > > @@ -70,17 +70,10 @@ static int amdgpu_vm_sdma_prepare(struct > amdgpu_vm_update_params *p, > > p->num_dw_left = ndw; > > - /* Wait for moves to be completed */ > - r = amdgpu_sync_fence(&p->job->sync, exclusive, false); > - if (r) > - return r; > - > - /* Don't wait for any submissions during page fault handling */ > - if (p->direct) > + if (!resv) > return 0; > > - return amdgpu_sync_resv(p->adev, &p->job->sync, root->tbo.base.resv, > - AMDGPU_SYNC_NE_OWNER, owner); > + return amdgpu_sync_resv(p->adev, &p->job->sync, resv, sync_mode, p->vm); > } > > /** _______________________________________________ amd-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
