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