[PATCH 2/3] amd/amdgpu: Inherit coherence flags base on original BO flags

[Xiao, Shane]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Kuehling, Felix <Felix.Kuehling at amd.com>
> Sent: Tuesday, April 4, 2023 9:52 PM
> To: Xiao, Shane <shane.xiao at amd.com>; amd-gfx at lists.freedesktop.org;
> Koenig, Christian <Christian.Koenig at amd.com>
> Cc: Liu, Aaron <Aaron.Liu at amd.com>; Guo, Shikai <Shikai.Guo at amd.com>
> Subject: Re: [PATCH 2/3] amd/amdgpu: Inherit coherence flags base on original
> BO flags
> 
> Am 2023-04-04 um 05:56 schrieb Shane Xiao:
> > For SG BO to DMA-map userptrs on other GPUs, the SG BO need inherit
> > MTYPEs in PTEs from original BO.
> 
> Good catch. See two comments inline.
> 
> 
> >
> > If we set the flags, the device can be coherent with the CPUs and other GPUs.
> >
> > Signed-off-by: Shane Xiao <shane.xiao at amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 10 +++++++++-
> >   1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > index 33cda358cb9e..bcb0a7b32703 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > @@ -253,14 +253,22 @@ create_dmamap_sg_bo(struct amdgpu_device
> *adev,
> >   {
> >   	struct drm_gem_object *gem_obj;
> >   	int ret, align;
> > +	uint64_t flags = 0;
> >
> >   	ret = amdgpu_bo_reserve(mem->bo, false);
> >   	if (ret)
> >   		return ret;
> >
> >   	align = 1;
> > +	if(mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_USERPTR)
> > +	{
> > +		flags |= mem->bo->flags
> &(AMDGPU_GEM_CREATE_CPU_GTT_USWC |
> 
> I think userptrs never use USWC because the pages are not allocated by the
> driver. You can drop this flag.

OK. I will do it.

> 
> 
> > +				AMDGPU_GEM_CREATE_COHERENT |
> AMDGPU_GEM_CREATE_UNCACHED);
> > +		align = PAGE_SIZE;
> 
> Isn't a page alignment implicit anyway? I don't see why we need to use a
> different alignment for userptrs. If PAGE_SIZE is needed for this case,
> we can use the same for all cases We don't even need a local variable
> for this.

Yes,  a page alignment is implicit, and the local variable will be removed.

Best Regards,
Shane

> 
> Regards,
>    Felix
> 
> 
> > +	}
> > +
> >   	ret = amdgpu_gem_object_create(adev, mem->bo->tbo.base.size, align,
> > -			AMDGPU_GEM_DOMAIN_CPU,
> AMDGPU_GEM_CREATE_PREEMPTIBLE,
> > +			AMDGPU_GEM_DOMAIN_CPU,
> AMDGPU_GEM_CREATE_PREEMPTIBLE | flags,
> >   			ttm_bo_type_sg, mem->bo->tbo.base.resv, &gem_obj);
> >
> >   	amdgpu_bo_unreserve(mem->bo);