Re: [PATCH 3/4] drm/amdgpu: Add flush_tlb parameter to amdgpu_vm_bo_update

[Christian König <ckoenig.leichtzumerken@xxxxxxxxx>]

Am 02.06.21 um 04:54 schrieb Felix Kuehling:
> Am 2021-06-01 um 6:59 p.m. schrieb Eric Huang:
> > It is to pass the flag to KFD, and optimize table_freed in
> > amdgpu_vm_bo_update_mapping.
> >
> > Signed-off-by: Eric Huang <jinhuieric.huang@xxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  |  6 +++---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c |  2 +-
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 10 +++++-----
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  2 +-
> >   4 files changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > index e9f9f462a652..e3df132e53a5 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > @@ -916,7 +916,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
> >   	if (r)
> >   		return r;
> >   
> > -	r = amdgpu_vm_bo_update(adev, fpriv->prt_va, false);
> > +	r = amdgpu_vm_bo_update(adev, fpriv->prt_va, false, NULL);
> >   	if (r)
> >   		return r;
> >   
> > @@ -927,7 +927,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
> >   	if (amdgpu_mcbp || amdgpu_sriov_vf(adev)) {
> >   		bo_va = fpriv->csa_va;
> >   		BUG_ON(!bo_va);
> > -		r = amdgpu_vm_bo_update(adev, bo_va, false);
> > +		r = amdgpu_vm_bo_update(adev, bo_va, false, NULL);
> >   		if (r)
> >   			return r;
> >   
> > @@ -946,7 +946,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
> >   		if (bo_va == NULL)
> >   			continue;
> >   
> > -		r = amdgpu_vm_bo_update(adev, bo_va, false);
> > +		r = amdgpu_vm_bo_update(adev, bo_va, false, NULL);
> >   		if (r)
> >   			return r;
> >   
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> > index 2120a87a949f..eac2fd0048cc 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> > @@ -696,7 +696,7 @@ static void amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
> >   
> >   	if (operation == AMDGPU_VA_OP_MAP ||
> >   	    operation == AMDGPU_VA_OP_REPLACE) {
> > -		r = amdgpu_vm_bo_update(adev, bo_va, false);
> > +		r = amdgpu_vm_bo_update(adev, bo_va, false, NULL);
> >   		if (r)
> >   			goto error;
> >   	}
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > index 2c20bba7dc1a..fed3d44b5ded 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > @@ -1729,7 +1729,7 @@ int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
> >   	r = vm->update_funcs->commit(&params, fence);
> >   
> >   	if (table_freed)
> > -		*table_freed = params.table_freed;
> > +		*table_freed = *table_freed || params.table_freed;
> >   
> >   error_unlock:
> >   	amdgpu_vm_eviction_unlock(vm);
> > @@ -1793,7 +1793,7 @@ void amdgpu_vm_get_memory(struct amdgpu_vm *vm, uint64_t *vram_mem,
> >    * 0 for success, -EINVAL for failure.
> >    */
> >   int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
> > -			bool clear)
> > +			bool clear, bool *flush_tlb)
> To be consistent with amdgpu_vm_bo_update_mapping I'd name this
> parameter table_freed.
>
>
> >   {
> >   	struct amdgpu_bo *bo = bo_va->base.bo;
> >   	struct amdgpu_vm *vm = bo_va->base.vm;
> > @@ -1887,7 +1887,7 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
> >   						resv, mapping->start,
> >   						mapping->last, update_flags,
> >   						mapping->offset, mem,
> > -						pages_addr, last_update, NULL,
> > +						pages_addr, last_update, flush_tlb,
> >   						vram_base_offset);
> >   		if (r)
> >   			return r;
> > @@ -2141,7 +2141,7 @@ int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
> >   
> >   	list_for_each_entry_safe(bo_va, tmp, &vm->moved, base.vm_status) {
> >   		/* Per VM BOs never need to bo cleared in the page tables */
> > -		r = amdgpu_vm_bo_update(adev, bo_va, false);
> > +		r = amdgpu_vm_bo_update(adev, bo_va, false, NULL);
> >   		if (r)
> >   			return r;
> >   	}
> > @@ -2160,7 +2160,7 @@ int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
> >   		else
> >   			clear = true;
> >   
> > -		r = amdgpu_vm_bo_update(adev, bo_va, clear);
> > +		r = amdgpu_vm_bo_update(adev, bo_va, clear, NULL);
> >   		if (r)
> >   			return r;
> >   
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > index 67bba8462e7d..24a63e284a69 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > @@ -419,7 +419,7 @@ int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
> >   
> >   int amdgpu_vm_bo_update(struct amdgpu_device *adev,
> >   			struct amdgpu_bo_va *bo_va,
> > -			bool clear);
> > +			bool clear, bool *flush_tlb);
> Same as above. With that fixed, the patch and the series is
>
> Reviewed-by: Felix Kuehling <Felix.Kuehling@xxxxxxx>
>
> Please also give Christian a chance to review this patch in particular
> before you submit.

With the naming made consistent the patch is Reviewed-by: Christian 
König <christian.koenig@xxxxxxx> as well.

> Thanks,
>    Felix
>
>
> >   bool amdgpu_vm_evictable(struct amdgpu_bo *bo);
> >   void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
> >   			     struct amdgpu_bo *bo, bool evicted);
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx