[AMD Official Use Only - General]
> -----Original Message-----
> From: Liu, Aaron <Aaron.Liu@xxxxxxx>
> Sent: Tuesday, April 25, 2023 9:14 AM
> To: Alex Deucher <alexdeucher@xxxxxxxxx>; Koenig, Christian
> <Christian.Koenig@xxxxxxx>
> Cc: Xiao, Shane <shane.xiao@xxxxxxx>; Christian König
> <ckoenig.leichtzumerken@xxxxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx;
> Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Zhang, Hawking
> <Hawking.Zhang@xxxxxxx>; Kuehling, Felix <Felix.Kuehling@xxxxxxx>; Hou,
> Xiaomeng (Matthew) <Xiaomeng.Hou@xxxxxxx>
> Subject: RE: [PATCH] drm/amdgpu: Enable doorbell selfring if resize BAR
> successfully
>
> [AMD Official Use Only - General]
>
> > -----Original Message-----
> > From: Alex Deucher <alexdeucher@xxxxxxxxx>
> > Sent: Tuesday, April 25, 2023 4:20 AM
> > To: Koenig, Christian <Christian.Koenig@xxxxxxx>
> > Cc: Xiao, Shane <shane.xiao@xxxxxxx>; Christian König
> > <ckoenig.leichtzumerken@xxxxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx;
> > Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Zhang, Hawking
> > <Hawking.Zhang@xxxxxxx>; Kuehling, Felix <Felix.Kuehling@xxxxxxx>;
> > Hou, Xiaomeng (Matthew) <Xiaomeng.Hou@xxxxxxx>; Liu, Aaron
> > <Aaron.Liu@xxxxxxx>
> > Subject: Re: [PATCH] drm/amdgpu: Enable doorbell selfring if resize
> > BAR successfully
> >
> > On Mon, Apr 24, 2023 at 3:11 PM Christian König
> > <christian.koenig@xxxxxxx>
> > wrote:
> > >
> > > Am 24.04.23 um 16:06 schrieb Xiao, Shane:
> > > > [AMD Official Use Only - General]
> > > >> -----Original Message-----
> > > >> From: Xiao, Shane
> > > >> Sent: Monday, April 24, 2023 6:31 PM
> > > >> To: Christian König <ckoenig.leichtzumerken@xxxxxxxxx>; amd-
> > > >> gfx@xxxxxxxxxxxxxxxxxxxxx; Deucher, Alexander
> > > >> <Alexander.Deucher@xxxxxxx>; Zhang, Hawking
> > > >> <Hawking.Zhang@xxxxxxx>; Kuehling, Felix <Felix.Kuehling@xxxxxxx>
> > > >> Cc: Hou, Xiaomeng (Matthew) <Xiaomeng.Hou@xxxxxxx>; Liu, Aaron
> > > >> <Aaron.Liu@xxxxxxx>
> > > >> Subject: RE: [PATCH] drm/amdgpu: Enable doorbell selfring if
> > > >> resize BAR successfully
> > > >>
> > > >> [AMD Official Use Only - General]
> > > >>> -----Original Message-----
> > > >>> From: Christian König <ckoenig.leichtzumerken@xxxxxxxxx>
> > > >>> Sent: Monday, April 24, 2023 5:07 PM
> > > >>> To: Xiao, Shane <shane.xiao@xxxxxxx>;
> > > >>> amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Deucher, Alexander
> > > >>> <Alexander.Deucher@xxxxxxx>; Zhang, Hawking
> > > >>> <Hawking.Zhang@xxxxxxx>; Kuehling, Felix
> > > >>> <Felix.Kuehling@xxxxxxx>
> > > >>> Cc: Hou, Xiaomeng (Matthew) <Xiaomeng.Hou@xxxxxxx>; Liu, Aaron
> > > >>> <Aaron.Liu@xxxxxxx>
> > > >>> Subject: Re: [PATCH] drm/amdgpu: Enable doorbell selfring if
> > > >>> resize BAR successfully
> > > >>>
> > > >>> Am 18.04.23 um 08:54 schrieb Shane Xiao:
> > > >>>> [Why]
> > > >>>> The selfring doorbell aperture will change when we resize FB
> > > >>>> BAR successfully during gmc sw init, we should reorder the
> > > >>>> sequence of enabling doorbell selfring aperture.
> > > >>> That's a good catch.
> > > >>>
> > > >>>> [How]
> > > >>>> Move enable_doorbell_selfring_aperture from *_common_hw_init to
> > > >>>> *_common_late_init.
> > > >>> But that sounds like a bad idea. Instead the full call to
> > > >>> nv_enable_doorbell_aperture() should be moved around.
> > > >> Hi Christian,
> > > >>
> > > >> Yes, I get your idea. But as far as I can understand that, the
> > > >> gfx hw init will use doorbell.
> > > >> If so, we cannot enable doorbell after gfx hw init.
> > > > We have come up with two ways to resolve the issue.
> > > >
> > > > 1) Separate enable_doorbell_aperture and
> > > > enable_doorbell_selfring_aperture. However, the
> > enable_doorbell_selfring_aperture should be moved in
> > *_common_ip_funcs-
> > >late_init.
> > >
> > > I'm not an expert for this part of the driver, but of hand that
> > > sounds like the right way of doing it.
> > >
> > > Alex any objections?
> >
> > Yeah, seems reasonable.
> >
> > Alex
> >
> enable_doorbell_aperture and enable_doorbell_selfring_aperture should be in
> common_*_init instead of gmc_hw_init.
> The order of execution of Shane's 1st way is :
> 1) common_sw_init
> 2) common_hw_init -> enable_doorbell_aperture
> 3) gmc_sw_init -> amdgpu_device_resize_fb_bar ///This relies
> gmc.real_vram_size to determine resize_fb_bar, so moving
> amdgpu_device_resize_fb_bar to common_sw_init is not a good idea.
> 4) gmc_hw_init
> 5) common_late_init -> enable_doorbell_selfring_aperture
>
> The 1st way looks good to me and reviewed-by me.
Hi Alex & Christian,
Since this patch has already implemented it in this way, is there any other comments on this patch body?
Or can I add you R-B or Acked-by on this patch?
Best Regards,
Shane
>
> > >
> > > Regards,
> > > Christian.
> > >
> > > > 2) The full call can be moved to gmc hw init. But it seems
> > > > strange to move
> > nbio configuration into gmc hw init.
> > > >
> > > > If neither of the above methods is suitable, could you please give
> > > > us some
> > advice on this issue?
> > > >
> > > > Best Regards,
> > > > Shane
> > > >
> > > >> Best Regards,
> > > >> Shane
> > > >>
> > > >>> Regards,
> > > >>> Christian.
> > > >>>
> > > >>>> This fixes the potential issue that GPU ring its own doorbell
> > > >>>> when this device is in translated mode with iommu is on.
> > > >>>>
> > > >>>> Signed-off-by: Shane Xiao <shane.xiao@xxxxxxx>
> > > >>>> Signed-off-by: Aaron Liu <aaron.liu@xxxxxxx>
> > > >>>> Tested-by: Xiaomeng Hou <Xiaomeng.Hou@xxxxxxx>
> > > >>>> ---
> > > >>>> drivers/gpu/drm/amd/amdgpu/nv.c | 4 +++-
> > > >>>> drivers/gpu/drm/amd/amdgpu/soc15.c | 4 +++-
> > > >>>> drivers/gpu/drm/amd/amdgpu/soc21.c | 4 +++-
> > > >>>> 3 files changed, 9 insertions(+), 3 deletions(-)
> > > >>>>
> > > >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c
> > > >>>> b/drivers/gpu/drm/amd/amdgpu/nv.c index
> > > >>>> 47420b403871..f4c85634a4c8
> > > >>>> 100644
> > > >>>> --- a/drivers/gpu/drm/amd/amdgpu/nv.c
> > > >>>> +++ b/drivers/gpu/drm/amd/amdgpu/nv.c
> > > >>>> @@ -535,7 +535,8 @@ static void
> > > >>>> nv_enable_doorbell_aperture(struct
> > > >>> amdgpu_device *adev,
> > > >>>> bool enable)
> > > >>>> {
> > > >>>> adev->nbio.funcs->enable_doorbell_aperture(adev,
> > > >>>> enable);
> > > >>>> - adev->nbio.funcs->enable_doorbell_selfring_aperture(adev,
> > > >>>> enable);
> > > >>>> + if (!enable)
> > > >>>> +
> > > >>>> + adev->nbio.funcs->enable_doorbell_selfring_aperture(adev,
> > > >>> false);
> > > >>>> }
> > > >>>>
> > > >>>> const struct amdgpu_ip_block_version nv_common_ip_block = @@
> > > >>>> -999,6
> > > >>>> +1000,7 @@ static int nv_common_late_init(void *handle)
> > > >>>> }
> > > >>>> }
> > > >>>>
> > > >>>> + adev->nbio.funcs->enable_doorbell_selfring_aperture(adev,
> > > >>>> + true);
> > > >>>> return 0;
> > > >>>> }
> > > >>>>
> > > >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c
> > > >>>> b/drivers/gpu/drm/amd/amdgpu/soc15.c
> > > >>>> index bc5dd80f10c1..0202de79a389 100644
> > > >>>> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
> > > >>>> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
> > > >>>> @@ -623,7 +623,8 @@ static void
> > > >>>> soc15_enable_doorbell_aperture(struct
> > > >>> amdgpu_device *adev,
> > > >>>> bool enable)
> > > >>>> {
> > > >>>> adev->nbio.funcs->enable_doorbell_aperture(adev,
> > > >>>> enable);
> > > >>>> - adev->nbio.funcs->enable_doorbell_selfring_aperture(adev,
> > > >>>> enable);
> > > >>>> + if (!enable)
> > > >>>> +
> > > >>>> + adev->nbio.funcs->enable_doorbell_selfring_aperture(adev,
> > > >>> false);
> > > >>>> }
> > > >>>>
> > > >>>> const struct amdgpu_ip_block_version vega10_common_ip_block
> > > >>>> = @@
> > > >>>> -1125,6 +1126,7 @@ static int soc15_common_late_init(void *handle)
> > > >>>> if (amdgpu_sriov_vf(adev))
> > > >>>> xgpu_ai_mailbox_get_irq(adev);
> > > >>>>
> > > >>>> + adev->nbio.funcs->enable_doorbell_selfring_aperture(adev,
> > > >>>> + true);
> > > >>>> return 0;
> > > >>>> }
> > > >>>>
> > > >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/soc21.c
> > > >>>> b/drivers/gpu/drm/amd/amdgpu/soc21.c
> > > >>>> index 514bfc705d5a..cd4619085d67 100644
> > > >>>> --- a/drivers/gpu/drm/amd/amdgpu/soc21.c
> > > >>>> +++ b/drivers/gpu/drm/amd/amdgpu/soc21.c
> > > >>>> @@ -454,7 +454,8 @@ static void
> > > >>>> soc21_enable_doorbell_aperture(struct
> > > >>> amdgpu_device *adev,
> > > >>>> bool enable)
> > > >>>> {
> > > >>>> adev->nbio.funcs->enable_doorbell_aperture(adev,
> > > >>>> enable);
> > > >>>> - adev->nbio.funcs->enable_doorbell_selfring_aperture(adev,
> > > >>>> enable);
> > > >>>> + if (!enable)
> > > >>>> +
> > > >>>> + adev->nbio.funcs->enable_doorbell_selfring_aperture(adev,
> > > >>> false);
> > > >>>> }
> > > >>>>
> > > >>>> const struct amdgpu_ip_block_version soc21_common_ip_block =
> > > >>>> @@
> > > >>>> -764,6 +765,7 @@ static int soc21_common_late_init(void *handle)
> > > >>>> amdgpu_irq_get(adev, &adev-
> > > >>>> nbio.ras_err_event_athub_irq, 0);
> > > >>>> }
> > > >>>>
> > > >>>> + adev->nbio.funcs->enable_doorbell_selfring_aperture(adev,
> > > >>>> + true);
> > > >>>> return 0;
> > > >>>> }
> > > >>>>
> > >
