[AMD Official Use Only - AMD Internal Distribution Only]
Oops, lemme fix that real quick...
> -----Original Message-----
> From: Joshi, Mukul <Mukul.Joshi@xxxxxxx>
> Sent: Thursday, October 10, 2024 11:46
> To: Li, Yunxiang (Teddy) <Yunxiang.Li@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Koenig, Christian
> <Christian.Koenig@xxxxxxx>; Gui, Jack <Jack.Gui@xxxxxxx>; Li, Yunxiang
> (Teddy) <Yunxiang.Li@xxxxxxx>
> Subject: RE: [PATCH 1/2] drm/amdkfd: cleanup device pointer dereference chains
>
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> > -----Original Message-----
> > From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
> > Yunxiang Li
> > Sent: Thursday, October 10, 2024 11:19 AM
> > To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Koenig, Christian
> > <Christian.Koenig@xxxxxxx>; Gui, Jack <Jack.Gui@xxxxxxx>; Li, Yunxiang
> > (Teddy) <Yunxiang.Li@xxxxxxx>
> > Subject: [PATCH 1/2] drm/amdkfd: cleanup device pointer dereference
> > chains
> >
> > Pull out some duplicated dereference chains into variables, and in
> > some cases grab struct device pointer directly from amdgpu_device instead of via
> drm_device.
> >
> > Signed-off-by: Yunxiang Li <Yunxiang.Li@xxxxxxx>
> > ---
> > drivers/gpu/drm/amd/amdkfd/kfd_process.c | 32
> > +++++++++++++-----------
> > 1 file changed, 18 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> > b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> > index d665ecdcd12fc..c334432e55b14 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> > @@ -1051,6 +1051,7 @@ static void kfd_process_destroy_pdds(struct
> > kfd_process *p)
> >
> > for (i = 0; i < p->n_pdds; i++) {
> > struct kfd_process_device *pdd = p->pdds[i];
> > + struct amdgpu_device *adev = pdd->dev->adev;
> >
> > pr_debug("Releasing pdd (topology id %d) for process
> > (pasid 0x%x)\n",
> > pdd->dev->id, p->pasid); @@ -1059,8
> > +1060,8 @@ static void kfd_process_destroy_pdds(struct kfd_process *p)
> > kfd_process_device_destroy_ib_mem(pdd);
> >
> > if (pdd->drm_file) {
> > - amdgpu_amdkfd_gpuvm_release_process_vm(
> > - pdd->dev->adev, pdd->drm_priv);
> > + amdgpu_amdkfd_gpuvm_release_process_vm(adev,
> > +
> > + pdd->drm_priv);
> > fput(pdd->drm_file);
> > }
> >
> > @@ -1073,15 +1074,14 @@ static void kfd_process_destroy_pdds(struct
> > kfd_process *p)
> > kfd_free_process_doorbells(pdd->dev->kfd, pdd);
> >
> > if (pdd->dev->kfd->shared_resources.enable_mes)
> > - amdgpu_amdkfd_free_gtt_mem(pdd->dev->adev,
> > - &pdd->proc_ctx_bo);
> > + amdgpu_amdkfd_free_gtt_mem(adev,
> > + &pdd->proc_ctx_bo);
> > /*
> > * before destroying pdd, make sure to report availability
> > * for auto suspend
> > */
> > if (pdd->runtime_inuse) {
> > - pm_runtime_mark_last_busy(adev_to_drm(pdd->dev-
> > >adev)->dev);
> > - pm_runtime_put_autosuspend(adev_to_drm(pdd->dev-
> > >adev)->dev);
> > + pm_runtime_mark_last_busy(adev->dev);
> > + pm_runtime_put_autosuspend(adev->dev);
> > pdd->runtime_inuse = false;
> > }
> >
> > @@ -1606,6 +1606,8 @@ struct kfd_process_device
> > *kfd_create_process_device_data(struct kfd_node *dev,
> > struct
> > kfd_process *p) {
> > struct kfd_process_device *pdd = NULL;
> > + struct amdgpu_device *adev = dev->adev;
> > + struct device *bdev = adev->dev;
> > int retval = 0;
> >
> > if (WARN_ON_ONCE(p->n_pdds >= MAX_GPU_INSTANCE)) @@ -
> > 1631,14 +1633,17 @@ struct kfd_process_device
> > *kfd_create_process_device_data(struct kfd_node *dev,
> > atomic64_set(&pdd->evict_duration_counter, 0);
> >
> > if (dev->kfd->shared_resources.enable_mes) {
> > - retval = amdgpu_amdkfd_alloc_gtt_mem(dev->adev,
> > + retval = amdgpu_amdkfd_alloc_gtt_mem(adev,
> > AMDGPU_MES_PROC_CTX_SIZE,
> > &pdd->proc_ctx_bo,
> > &pdd->proc_ctx_gpu_addr,
> > &pdd->proc_ctx_cpu_ptr,
> > false);
> > + retval = amdgpu_amdkfd_alloc_gtt_mem(
> > + adev, AMDGPU_MES_PROC_CTX_SIZE, &pdd-
> > >proc_ctx_bo,
> > + &pdd->proc_ctx_gpu_addr, &pdd->proc_ctx_cpu_ptr,
> > + false);
>
> Looks like you are duplicating the amdgpu_amdkfd_alloc_gtt_mem call here.
>
> Regards,
> Mukul
>
> > if (retval) {
> > - dev_err(dev->adev->dev,
> > + dev_err(bdev,
> > "failed to allocate process context bo\n");
> > goto err_free_pdd;
> > }
> > @@ -1647,10 +1652,8 @@ struct kfd_process_device
> > *kfd_create_process_device_data(struct kfd_node *dev,
> >
> > p->pdds[p->n_pdds++] = pdd;
> > if (kfd_dbg_is_per_vmid_supported(pdd->dev))
> > - pdd->spi_dbg_override = pdd->dev->kfd2kgd->disable_debug_trap(
> > - pdd->dev->adev,
> > - false,
> > - 0);
> > + pdd->spi_dbg_override =
> > + pdd->dev->kfd2kgd->disable_debug_trap(adev,
> > + false, 0);
> >
> > /* Init idr used for memory handle translation */
> > idr_init(&pdd->alloc_idr);
> > @@ -1750,11 +1753,12 @@ struct kfd_process_device
> > *kfd_bind_process_to_device(struct kfd_node *dev,
> > struct
> > kfd_process *p) {
> > struct kfd_process_device *pdd;
> > + struct device *bdev = dev->adev->dev;
> > int err;
> >
> > pdd = kfd_get_process_device_data(dev, p);
> > if (!pdd) {
> > - dev_err(dev->adev->dev, "Process device data doesn't exist\n");
> > + dev_err(bdev, "Process device data doesn't exist\n");
> > return ERR_PTR(-ENOMEM);
> > }
> >
> > @@ -1767,7 +1771,7 @@ struct kfd_process_device
> > *kfd_bind_process_to_device(struct kfd_node *dev,
> > * pdd is destroyed.
> > */
> > if (!pdd->runtime_inuse) {
> > - err = pm_runtime_get_sync(adev_to_drm(dev->adev)->dev);
> > + err = pm_runtime_get_sync(bdev);
> > if (err < 0) {
> >
> > pm_runtime_put_autosuspend(adev_to_drm(dev->adev)-
> > >dev);
> > return ERR_PTR(err);
> > --
> > 2.34.1
>
