Re: [PATCH] drm/amdkfd: Use gpu_offset for user queue's wptr

[Felix Kuehling <felix.kuehling@xxxxxxx>]

On 2023-09-17 22:14, Wang, YuBiao wrote:
> [AMD Official Use Only - General]
>
> Hi Felix,
>
> Yeah, I believe that we were always set gart domain start as 0 before.

Thanks for confirming that. The patch is

Reviewed-by: Felix Kuehling <Felix.Kuehling@xxxxxxx>


> Regards,
> Yubiao
>
> -----Original Message-----
> From: Kuehling, Felix <Felix.Kuehling@xxxxxxx>
> Sent: Friday, September 15, 2023 10:54 PM
> To: Koenig, Christian <Christian.Koenig@xxxxxxx>; Wang, YuBiao <YuBiao.Wang@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Sharma, Shashank <Shashank.Sharma@xxxxxxx>
> Cc: Chen, Horace <Horace.Chen@xxxxxxx>; Tuikov, Luben <Luben.Tuikov@xxxxxxx>; Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Zhang, Hawking <Hawking.Zhang@xxxxxxx>; Liu, Monk <Monk.Liu@xxxxxxx>; Xu, Feifei <Feifei.Xu@xxxxxxx>; Kevin Wang <Kevin1.Wang@xxxxxxx>
> Subject: Re: [PATCH] drm/amdkfd: Use gpu_offset for user queue's wptr
>
> On 2023-09-15 2:50, Christian König wrote:
> > Am 15.09.23 um 04:52 schrieb YuBiao Wang:
> > > Directly use tbo's start address will miss the domain start offset.
> > > Need to use gpu_offset instead.
> > >
> > > Signed-off-by: YuBiao Wang <YuBiao.Wang@xxxxxxx>
> > Felix and/or Shashank should probably take a look as well, but of hand
> > that looks like the correct fix.
> Looks reasonable to me. Why did this not cause problems before? Are we just lucky that the domain start offset is 0 on the GPUs we've tested so far?
>
> Regards,
>     Felix
>
>
> > Reviewed-by: Christian König <christian.koenig@xxxxxxx>
> >
> > > ---
> > >    drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 2 +-
> > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> > > b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> > > index 77159b03a422..36e7171ad9a7 100644
> > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> > > @@ -216,7 +216,7 @@ static int add_queue_mes(struct
> > > device_queue_manager *dqm, struct queue *q,
> > >          if (q->wptr_bo) {
> > >            wptr_addr_off = (uint64_t)q->properties.write_ptr &
> > > (PAGE_SIZE - 1);
> > > -        queue_input.wptr_mc_addr =
> > > ((uint64_t)q->wptr_bo->tbo.resource->start << PAGE_SHIFT) +
> > > wptr_addr_off;
> > > +        queue_input.wptr_mc_addr = amdgpu_bo_gpu_offset(q->wptr_bo)
> > > +wptr_addr_off;
> > >        }
> > >          queue_input.is_kfd_process = 1;