| Message ID | 20211005113742.1101-13-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/28] dma-buf: add dma_resv_for_each_fence_unlocked v8 | expand |
On Tue, Oct 05, 2021 at 01:37:26PM +0200, Christian König wrote: > Simplifying the code a bit. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 14 ++++---------- > 1 file changed, 4 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index e8d70b6e6737..722e3c9e8882 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > @@ -1345,10 +1345,9 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo, > const struct ttm_place *place) > { > unsigned long num_pages = bo->resource->num_pages; > + struct dma_resv_iter resv_cursor; > struct amdgpu_res_cursor cursor; > - struct dma_resv_list *flist; > struct dma_fence *f; > - int i; > > /* Swapout? */ > if (bo->resource->mem_type == TTM_PL_SYSTEM) > @@ -1362,14 +1361,9 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo, > * If true, then return false as any KFD process needs all its BOs to > * be resident to run successfully > */ > - flist = dma_resv_shared_list(bo->base.resv); > - if (flist) { > - for (i = 0; i < flist->shared_count; ++i) { > - f = rcu_dereference_protected(flist->shared[i], > - dma_resv_held(bo->base.resv)); > - if (amdkfd_fence_check_mm(f, current->mm)) > - return false; > - } > + dma_resv_for_each_fence(&resv_cursor, bo->base.resv, true, f) { ^false? At least I'm not seeing the code look at the exclusive fence here. -Daniel > + if (amdkfd_fence_check_mm(f, current->mm)) > + return false; > } > > switch (bo->resource->mem_type) { > -- > 2.25.1 >
Am 13.10.21 um 16:07 schrieb Daniel Vetter: > On Tue, Oct 05, 2021 at 01:37:26PM +0200, Christian König wrote: >> Simplifying the code a bit. >> >> Signed-off-by: Christian König <christian.koenig@amd.com> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 14 ++++---------- >> 1 file changed, 4 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> index e8d70b6e6737..722e3c9e8882 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> @@ -1345,10 +1345,9 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo, >> const struct ttm_place *place) >> { >> unsigned long num_pages = bo->resource->num_pages; >> + struct dma_resv_iter resv_cursor; >> struct amdgpu_res_cursor cursor; >> - struct dma_resv_list *flist; >> struct dma_fence *f; >> - int i; >> >> /* Swapout? */ >> if (bo->resource->mem_type == TTM_PL_SYSTEM) >> @@ -1362,14 +1361,9 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo, >> * If true, then return false as any KFD process needs all its BOs to >> * be resident to run successfully >> */ >> - flist = dma_resv_shared_list(bo->base.resv); >> - if (flist) { >> - for (i = 0; i < flist->shared_count; ++i) { >> - f = rcu_dereference_protected(flist->shared[i], >> - dma_resv_held(bo->base.resv)); >> - if (amdkfd_fence_check_mm(f, current->mm)) >> - return false; >> - } >> + dma_resv_for_each_fence(&resv_cursor, bo->base.resv, true, f) { > ^false? > > At least I'm not seeing the code look at the exclusive fence here. Yes, but that's correct. We need to look at all potential fences. It's a design problem in KFD if you ask me, but that is a completely different topic. Christian. > -Daniel > >> + if (amdkfd_fence_check_mm(f, current->mm)) >> + return false; >> } >> >> switch (bo->resource->mem_type) { >> -- >> 2.25.1 >>
Am 2021-10-19 um 7:36 a.m. schrieb Christian König: > Am 13.10.21 um 16:07 schrieb Daniel Vetter: >> On Tue, Oct 05, 2021 at 01:37:26PM +0200, Christian König wrote: >>> Simplifying the code a bit. >>> >>> Signed-off-by: Christian König <christian.koenig@amd.com> >>> --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 14 ++++---------- >>> 1 file changed, 4 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >>> index e8d70b6e6737..722e3c9e8882 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >>> @@ -1345,10 +1345,9 @@ static bool >>> amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo, >>> const struct ttm_place *place) >>> { >>> unsigned long num_pages = bo->resource->num_pages; >>> + struct dma_resv_iter resv_cursor; >>> struct amdgpu_res_cursor cursor; >>> - struct dma_resv_list *flist; >>> struct dma_fence *f; >>> - int i; >>> /* Swapout? */ >>> if (bo->resource->mem_type == TTM_PL_SYSTEM) >>> @@ -1362,14 +1361,9 @@ static bool >>> amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo, >>> * If true, then return false as any KFD process needs all its >>> BOs to >>> * be resident to run successfully >>> */ >>> - flist = dma_resv_shared_list(bo->base.resv); >>> - if (flist) { >>> - for (i = 0; i < flist->shared_count; ++i) { >>> - f = rcu_dereference_protected(flist->shared[i], >>> - dma_resv_held(bo->base.resv)); >>> - if (amdkfd_fence_check_mm(f, current->mm)) >>> - return false; >>> - } >>> + dma_resv_for_each_fence(&resv_cursor, bo->base.resv, true, f) { >> ^false? >> >> At least I'm not seeing the code look at the exclusive fence here. > > Yes, but that's correct. We need to look at all potential fences. amdkfd_fence_check_mm is only meaningful for KFD eviction fences, and they are always added as shared fences. I think setting all_fences = false would return only the exclusive fence. Regards, Felix > > It's a design problem in KFD if you ask me, but that is a completely > different topic. > > Christian. > >> -Daniel >> >>> + if (amdkfd_fence_check_mm(f, current->mm)) >>> + return false; >>> } >>> switch (bo->resource->mem_type) { >>> -- >>> 2.25.1 >>> >
On Tue, Oct 19, 2021 at 12:30:40PM -0400, Felix Kuehling wrote: > Am 2021-10-19 um 7:36 a.m. schrieb Christian König: > > Am 13.10.21 um 16:07 schrieb Daniel Vetter: > >> On Tue, Oct 05, 2021 at 01:37:26PM +0200, Christian König wrote: > >>> Simplifying the code a bit. > >>> > >>> Signed-off-by: Christian König <christian.koenig@amd.com> > >>> --- > >>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 14 ++++---------- > >>> 1 file changed, 4 insertions(+), 10 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > >>> index e8d70b6e6737..722e3c9e8882 100644 > >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > >>> @@ -1345,10 +1345,9 @@ static bool > >>> amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo, > >>> const struct ttm_place *place) > >>> { > >>> unsigned long num_pages = bo->resource->num_pages; > >>> + struct dma_resv_iter resv_cursor; > >>> struct amdgpu_res_cursor cursor; > >>> - struct dma_resv_list *flist; > >>> struct dma_fence *f; > >>> - int i; > >>> /* Swapout? */ > >>> if (bo->resource->mem_type == TTM_PL_SYSTEM) > >>> @@ -1362,14 +1361,9 @@ static bool > >>> amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo, > >>> * If true, then return false as any KFD process needs all its > >>> BOs to > >>> * be resident to run successfully > >>> */ > >>> - flist = dma_resv_shared_list(bo->base.resv); > >>> - if (flist) { > >>> - for (i = 0; i < flist->shared_count; ++i) { > >>> - f = rcu_dereference_protected(flist->shared[i], > >>> - dma_resv_held(bo->base.resv)); > >>> - if (amdkfd_fence_check_mm(f, current->mm)) > >>> - return false; > >>> - } > >>> + dma_resv_for_each_fence(&resv_cursor, bo->base.resv, true, f) { > >> ^false? > >> > >> At least I'm not seeing the code look at the exclusive fence here. > > > > Yes, but that's correct. We need to look at all potential fences. > > amdkfd_fence_check_mm is only meaningful for KFD eviction fences, and > they are always added as shared fences. I think setting all_fences = > false would return only the exclusive fence. Hm yeah I got that wrong, which puts my entire review a bit in question :-) Anyway on the patch: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Regards, > Felix > > > > > > It's a design problem in KFD if you ask me, but that is a completely > > different topic. > > > > Christian. > > > >> -Daniel > >> > >>> + if (amdkfd_fence_check_mm(f, current->mm)) > >>> + return false; > >>> } > >>> switch (bo->resource->mem_type) { > >>> -- > >>> 2.25.1 > >>> > >
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index e8d70b6e6737..722e3c9e8882 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -1345,10 +1345,9 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo, const struct ttm_place *place) { unsigned long num_pages = bo->resource->num_pages; + struct dma_resv_iter resv_cursor; struct amdgpu_res_cursor cursor; - struct dma_resv_list *flist; struct dma_fence *f; - int i; /* Swapout? */ if (bo->resource->mem_type == TTM_PL_SYSTEM) @@ -1362,14 +1361,9 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo, * If true, then return false as any KFD process needs all its BOs to * be resident to run successfully */ - flist = dma_resv_shared_list(bo->base.resv); - if (flist) { - for (i = 0; i < flist->shared_count; ++i) { - f = rcu_dereference_protected(flist->shared[i], - dma_resv_held(bo->base.resv)); - if (amdkfd_fence_check_mm(f, current->mm)) - return false; - } + dma_resv_for_each_fence(&resv_cursor, bo->base.resv, true, f) { + if (amdkfd_fence_check_mm(f, current->mm)) + return false; } switch (bo->resource->mem_type) {
Simplifying the code a bit. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)