Re: [PATCH 04/14] drm/amdgpu/gart: Add helper to bind VRAM BO

["Timur Kristóf" <timur.kristof@gmail.com>]

On Wed, 2025-10-29 at 11:16 +0100, Christian König wrote:
> 
> 
> On 10/28/25 23:06, Timur Kristóf wrote:
> > Binds a BO that is allocated in VRAM to the GART page table.
> > 
> > Useful when a kernel BO is located in VRAM but
> > needs to be accessed from the GART address space,
> > for example to give a kernel BO a 32-bit address
> > when GART is placed in LOW address space.
> > 
> > Co-developed-by: Christian König <christian.koenig@amd.com>
> > Signed-off-by: Christian König <christian.koenig@amd.com>
> > Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c | 41
> > ++++++++++++++++++++++++
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_gart.h |  2 ++
> >  2 files changed, 43 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> > index 83f3b94ed975..19b5e72a6a26 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> > @@ -390,6 +390,47 @@ void amdgpu_gart_bind(struct amdgpu_device
> > *adev, uint64_t offset,
> >  	amdgpu_gart_map(adev, offset, pages, dma_addr, flags,
> > adev->gart.ptr);
> >  }
> >  
> > +/**
> > + * amdgpu_gart_bind - bind VRAM BO into the GART page table
> 
> That should be the function name or otherwise you get automated
> warnings.

That's a copy paste mistake on my part. Thanks for catching that.

> 
> > + *
> > + * @adev: amdgpu_device pointer
> > + * @offset: offset into the GPU's gart aperture
> > + * @bo: the BO whose pages should be mapped
> > + * @flags: page table entry flags
> > + *
> > + * Binds a BO that is allocated in VRAM to the GART page table
> > + * (all ASICs).
> > + * Useful when a kernel BO is located in VRAM but
> > + * needs to be accessed from the GART address space,
> > + * for example to give a kernel BO a 32-bit address
> > + * when GART is placed in LOW address space.
> > + */
> > +void amdgpu_gart_bind_vram_bo(struct amdgpu_device *adev, uint64_t
> > offset,
> > +		     struct amdgpu_bo *bo, uint64_t flags)
> 
> Please not the BO but just the VRAM pa.

Sure, will do

> 
> > +{
> > +	u64 pa, bo_size;
> > +	u32 num_pages, start_page, i, idx;
> > +
> > +	if (!adev->gart.ptr)
> > +		return;
> > +
> > +	if (!drm_dev_enter(adev_to_drm(adev), &idx))
> > +		return;
> > +
> > +	pa = amdgpu_gmc_vram_pa(adev, bo);
> > +	bo_size = amdgpu_bo_size(bo);
> > +	num_pages = ALIGN(bo_size, AMDGPU_GPU_PAGE_SIZE) /
> > AMDGPU_GPU_PAGE_SIZE;
> > +	start_page = offset / AMDGPU_GPU_PAGE_SIZE;
> > +
> > +	for (i = 0; i < num_pages; ++i) {
> > +		amdgpu_gmc_set_pte_pde(adev, adev->gart.ptr,
> > +			start_page + i, pa + AMDGPU_GPU_PAGE_SIZE
> > * i, flags);
> > +	}
> > +
> 
> Ideally amdgpu_gart_map() should be able to take both dma_addr array
> or VRAM pa (or have two map functions).
> 
> This way we could cleanup the code in amdgpu_ttm_map_buffer as well.

Alright, I'll rework this new function so that it can be reused also by
amdgpu_ttm_map_buffer, does that sound alright to you?

> 
> 
> > +	amdgpu_gart_invalidate_tlb(adev);
> 
> IIRC we moved that out of amdgpu_gart_bind(), probably best to do so
> here as well.

Sure, will do

> 
> Regards,
> Christian.
> 
> > +	drm_dev_exit(idx);
> > +}
> > +
> >  /**
> >   * amdgpu_gart_invalidate_tlb - invalidate gart TLB
> >   *
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.h
> > index 7cc980bf4725..756548d0b520 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.h
> > @@ -64,5 +64,7 @@ void amdgpu_gart_map(struct amdgpu_device *adev,
> > uint64_t offset,
> >  		     void *dst);
> >  void amdgpu_gart_bind(struct amdgpu_device *adev, uint64_t offset,
> >  		      int pages, dma_addr_t *dma_addr, uint64_t
> > flags);
> > +void amdgpu_gart_bind_vram_bo(struct amdgpu_device *adev, uint64_t
> > offset,
> > +		     struct amdgpu_bo *bo, uint64_t flags);
> >  void amdgpu_gart_invalidate_tlb(struct amdgpu_device *adev);
> >  #endif