[AMD Official Use Only - General]
> -----Original Message-----
> From: Christian König <ckoenig.leichtzumer...@gmail.com>
> Sent: Tuesday, April 25, 2023 4:54 PM
> To: Xiao, Shane <shane.x...@amd.com>; Alex Deucher
> <alexdeuc...@gmail.com>; Koenig, Christian <christian.koe...@amd.com>
> Cc: amd-gfx@lists.freedesktop.org; Deucher, Alexander
> <alexander.deuc...@amd.com>; Zhang, Hawking
> <hawking.zh...@amd.com>; Kuehling, Felix <felix.kuehl...@amd.com>; Hou,
> Xiaomeng (Matthew) <xiaomeng....@amd.com>; Liu, Aaron
> <aaron....@amd.com>
> Subject: Re: [PATCH] drm/amdgpu: Enable doorbell selfring if resize BAR
> successfully
>
>
>
> Am 25.04.23 um 05:29 schrieb Xiao, Shane:
> > [AMD Official Use Only - General]
> >
> >
> >
> >> -----Original Message-----
> >> From: Liu, Aaron <aaron....@amd.com>
> >> Sent: Tuesday, April 25, 2023 9:14 AM
> >> To: Alex Deucher <alexdeuc...@gmail.com>; Koenig, Christian
> >> <christian.koe...@amd.com>
> >> Cc: Xiao, Shane <shane.x...@amd.com>; Christian König
> >> <ckoenig.leichtzumer...@gmail.com>; amd-gfx@lists.freedesktop.org;
> >> Deucher, Alexander <alexander.deuc...@amd.com>; Zhang, Hawking
> >> <hawking.zh...@amd.com>; Kuehling, Felix <felix.kuehl...@amd.com>;
> >> Hou, Xiaomeng (Matthew) <xiaomeng....@amd.com>
> >> Subject: RE: [PATCH] drm/amdgpu: Enable doorbell selfring if resize
> >> BAR successfully
> >>
> >> [AMD Official Use Only - General]
> >>
> >>> -----Original Message-----
> >>> From: Alex Deucher <alexdeuc...@gmail.com>
> >>> Sent: Tuesday, April 25, 2023 4:20 AM
> >>> To: Koenig, Christian <christian.koe...@amd.com>
> >>> Cc: Xiao, Shane <shane.x...@amd.com>; Christian König
> >>> <ckoenig.leichtzumer...@gmail.com>; amd-gfx@lists.freedesktop.org;
> >>> Deucher, Alexander <alexander.deuc...@amd.com>; Zhang, Hawking
> >>> <hawking.zh...@amd.com>; Kuehling, Felix <felix.kuehl...@amd.com>;
> >>> Hou, Xiaomeng (Matthew) <xiaomeng....@amd.com>; Liu, Aaron
> >>> <aaron....@amd.com>
> >>> 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.koe...@amd.com>
> >>> 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.leichtzumer...@gmail.com>; amd-
> >>>>>> g...@lists.freedesktop.org; Deucher, Alexander
> >>>>>> <alexander.deuc...@amd.com>; Zhang, Hawking
> >>>>>> <hawking.zh...@amd.com>; Kuehling, Felix
> <felix.kuehl...@amd.com>
> >>>>>> Cc: Hou, Xiaomeng (Matthew) <xiaomeng....@amd.com>; Liu, Aaron
> >>>>>> <aaron....@amd.com>
> >>>>>> Subject: RE: [PATCH] drm/amdgpu: Enable doorbell selfring if
> >>>>>> resize BAR successfully
> >>>>>>
> >>>>>> [AMD Official Use Only - General]
> >>>>>>> -----Original Message-----
> >>>>>>> From: Christian König <ckoenig.leichtzumer...@gmail.com>
> >>>>>>> Sent: Monday, April 24, 2023 5:07 PM
> >>>>>>> To: Xiao, Shane <shane.x...@amd.com>;
> >>>>>>> amd-gfx@lists.freedesktop.org; Deucher, Alexander
> >>>>>>> <alexander.deuc...@amd.com>; Zhang, Hawking
> >>>>>>> <hawking.zh...@amd.com>; Kuehling, Felix
> >>>>>>> <felix.kuehl...@amd.com>
> >>>>>>> Cc: Hou, Xiaomeng (Matthew) <xiaomeng....@amd.com>; Liu,
> Aaron
> >>>>>>> <aaron....@amd.com>
> >>>>>>> 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?
>
> At least remove the functions
> soc15_enable_doorbell_aperture()/nv_enable_doorbell_aperture()/soc21_enab
> le_doorbell_aperture()
> and open code the respective calls.
>
> Those don't make sense any more since we don't have a central point when the
> apertures are enabled/disabled.
Sure, I will update this in v2.
Best Regards,
Shane
>
> Regards,
> Christian.
>
> >
> > 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.x...@amd.com>
> >>>>>>>> Signed-off-by: Aaron Liu <aaron....@amd.com>
> >>>>>>>> Tested-by: Xiaomeng Hou <xiaomeng....@amd.com>
> >>>>>>>> ---
> >>>>>>>> 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;
> >>>>>>>> }
> >>>>>>>>
