Based on my experiments I am able conclude that I can avoid validating the BO prior to pinning it. I don’t have the code history that led me to validating the BO in the first place. In any case I
posted an updated patch to the DRM-NEXT branch in addition to a standalone patch for the DKMS branch as well.
Thanks for pointing this out.
Regards,
Ramesh
From: Errabolu, Ramesh <Ramesh.Errabolu@xxxxxxx>
Sent: Tuesday, November 9, 2021 1:32 AM
To: Yu, Lang <Lang.Yu@xxxxxxx>
Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Subject: Re: [PATCH] drm/amdgpu: Pin MMIO/DOORBELL BO's in GTT domain
I will experiment to see if it is not needed. Will update patch based on the results.
On Tue, Nov 09, 2021 at 02:12:00PM +0800, Errabolu, Ramesh wrote:
> [AMD Official Use Only]
>
> Responses in line
>
> -----Original Message-----
> From: Yu, Lang <Lang.Yu@xxxxxxx>
> Sent: Monday, November 8, 2021 11:27 PM
> To: Errabolu, Ramesh <Ramesh.Errabolu@xxxxxxx>
> Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH] drm/amdgpu: Pin MMIO/DOORBELL BO's in GTT domain
>
> On Mon, Nov 08, 2021 at 07:37:44PM -0600, Ramesh Errabolu wrote:
> > MMIO/DOORBELL BOs encode control data and should be pinned in GTT
> > domain before enabling PCIe connected peer devices in accessing it
> >
> > Signed-off-by: Ramesh Errabolu <Ramesh.Errabolu@xxxxxxx>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 25 +++++++++
> > .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 55
> > +++++++++++++++++++
> > 2 files changed, 80 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> > index 4c1d6536a7a5..d9a1cfd7876f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> > @@ -300,6 +300,31 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
> > uint64_t va, void *drm_priv,
> > struct kgd_mem **mem, uint64_t *size,
> > uint64_t *mmap_offset);
> > +
> > +/**
> > + * amdgpu_amdkfd_gpuvm_pin_bo() - Pins a BO using following criteria
> > + * @bo: Handle of buffer object being pinned
> > + * @domain: Domain into which BO should be pinned
> > + *
> > + * - USERPTR BOs are UNPINNABLE and will return error
> > + * - All other BO types (GTT, VRAM, MMIO and DOORBELL) will have their
> > + * PIN count incremented. It is valid to PIN a BO multiple times
> > + *
> > + * Return: ZERO if successful in pinning, Non-Zero in case of error.
> > + * Will return -EINVAL if input BO parameter is a USERPTR type.
> > + */
> > +int amdgpu_amdkfd_gpuvm_pin_bo(struct amdgpu_bo *bo, u32 domain);
> > +
> > +/**
> > + * amdgpu_amdkfd_gpuvm_unpin_bo() - Unpins BO using following
> > +criteria
> > + * @bo: Handle of buffer object being unpinned
> > + *
> > + * - Is a illegal request for USERPTR BOs and is ignored
> > + * - All other BO types (GTT, VRAM, MMIO and DOORBELL) will have their
> > + * PIN count decremented. Calls to UNPIN must balance calls to PIN
> > + */
> > +void amdgpu_amdkfd_gpuvm_unpin_bo(struct amdgpu_bo *bo);
> > +
> > int amdgpu_amdkfd_get_tile_config(struct amdgpu_device *adev,
> > struct tile_config *config);
> > void amdgpu_amdkfd_ras_poison_consumption_handler(struct
> > amdgpu_device *adev); diff --git
> > a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > index 4fa814358552..f4ffc41873dd 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > @@ -1299,6 +1299,36 @@ static int init_kfd_vm(struct amdgpu_vm *vm, void **process_info,
> > return ret;
> > }
> >
> > +int amdgpu_amdkfd_gpuvm_pin_bo(struct amdgpu_bo *bo, u32 domain) {
> > + int ret = 0;
> > +
> > + ret = amdgpu_bo_reserve(bo, false);
> > + if (unlikely(ret))
> > + return ret;
> > +
> > + ret = amdgpu_bo_pin_restricted(bo, domain, 0, 0);
> > + if (ret)
> > + pr_err("Error in Pinning BO to domain: %d\n", domain);
> > +
> > + amdgpu_bo_sync_wait(bo, AMDGPU_FENCE_OWNER_KFD, false);
> > + amdgpu_bo_unreserve(bo);
> > +
> > + return ret;
> > +}
> > +
> > +void amdgpu_amdkfd_gpuvm_unpin_bo(struct amdgpu_bo *bo) {
> > + int ret = 0;
> > +
> > + ret = amdgpu_bo_reserve(bo, false);
> > + if (unlikely(ret))
> > + return;
> > +
> > + amdgpu_bo_unpin(bo);
> > + amdgpu_bo_unreserve(bo);
> > +}
> > +
> > int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct amdgpu_device *adev,
> > struct file *filp, u32 pasid,
> > void **process_info,
> > @@ -1525,6 +1555,23 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
> > if (offset)
> > *offset = amdgpu_bo_mmap_offset(bo);
> >
> > + if (flags & (KFD_IOC_ALLOC_MEM_FLAGS_DOORBELL |
> > + KFD_IOC_ALLOC_MEM_FLAGS_MMIO_REMAP)) {
> > + ret = amdgpu_amdkfd_bo_validate(bo, AMDGPU_GEM_DOMAIN_GTT, false);
> > + if (ret) {
> > + pr_err("Validating MMIO/DOORBELL BO during ALLOC FAILED\n");
> > + goto err_node_allow;
> > + }
>
> amdgpu_amdkfd_gpuvm_pin_bo() will do ttm_bo_validate(),
> amdgpu_amdkfd_bo_validate() seems redundant here.
>
> Ramesh: In my experiments I recall seeing an issue if BO was not validated in GTT domain prior to pinning. If my memory serves me correctly, the call to PIN will fail
>From amdgpu_bo_pin_restricted() we can see, pin operation will
If not pinned,
1, validate the BO with requested domain
2, increase pin count and update stats
If already pinned,
1, increase pin count
So seems not necessarily to validate it twice.
Regards,
Lang
> > +
> > + ret = amdgpu_amdkfd_gpuvm_pin_bo(bo, AMDGPU_GEM_DOMAIN_GTT);
> > + if (ret) {
> > + pr_err("Pinning MMIO/DOORBELL BO during ALLOC FAILED\n");
> > + goto err_node_allow;
> > + }
> > + bo->allowed_domains = AMDGPU_GEM_DOMAIN_GTT;
> > + bo->preferred_domains = AMDGPU_GEM_DOMAIN_GTT;
> > + }
> > +
> > return 0;
> >
> > allocate_init_user_pages_failed:
> > @@ -1561,6 +1608,14 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
> > bool is_imported = false;
> >
> > mutex_lock(&mem->lock);
> > +
> > + /* Unpin MMIO/DOORBELL BO's that were pinnned during allocation */
> > + if (mem->alloc_flags &
> > + (KFD_IOC_ALLOC_MEM_FLAGS_DOORBELL |
> > + KFD_IOC_ALLOC_MEM_FLAGS_MMIO_REMAP)) {
> > + amdgpu_amdkfd_gpuvm_unpin_bo(mem->bo);
> > + }
> > +
> > mapped_to_gpu_memory = mem->mapped_to_gpu_memory;
> > is_imported = mem->is_imported;
> > mutex_unlock(&mem->lock);
> > --
> > 2.31.1
> >
|
