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
>
> 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;
> >>>> }
> >>>>
>
