[PATCH] drm/amdgpu: Enable doorbell selfring if resize BAR successfully
Christian König
ckoenig.leichtzumerken at gmail.com
Tue Apr 25 08:53:52 UTC 2023
Am 25.04.23 um 05:29 schrieb Xiao, Shane:
> [AMD Official Use Only - General]
>
>
>
>> -----Original Message-----
>> From: Liu, Aaron <Aaron.Liu at amd.com>
>> Sent: Tuesday, April 25, 2023 9:14 AM
>> To: Alex Deucher <alexdeucher at gmail.com>; Koenig, Christian
>> <Christian.Koenig at amd.com>
>> Cc: Xiao, Shane <shane.xiao at amd.com>; Christian König
>> <ckoenig.leichtzumerken at gmail.com>; amd-gfx at lists.freedesktop.org;
>> Deucher, Alexander <Alexander.Deucher at amd.com>; Zhang, Hawking
>> <Hawking.Zhang at amd.com>; Kuehling, Felix <Felix.Kuehling at amd.com>; Hou,
>> Xiaomeng (Matthew) <Xiaomeng.Hou at amd.com>
>> Subject: RE: [PATCH] drm/amdgpu: Enable doorbell selfring if resize BAR
>> successfully
>>
>> [AMD Official Use Only - General]
>>
>>> -----Original Message-----
>>> From: Alex Deucher <alexdeucher at gmail.com>
>>> Sent: Tuesday, April 25, 2023 4:20 AM
>>> To: Koenig, Christian <Christian.Koenig at amd.com>
>>> Cc: Xiao, Shane <shane.xiao at amd.com>; Christian König
>>> <ckoenig.leichtzumerken at gmail.com>; amd-gfx at lists.freedesktop.org;
>>> Deucher, Alexander <Alexander.Deucher at amd.com>; Zhang, Hawking
>>> <Hawking.Zhang at amd.com>; Kuehling, Felix <Felix.Kuehling at amd.com>;
>>> Hou, Xiaomeng (Matthew) <Xiaomeng.Hou at amd.com>; Liu, Aaron
>>> <Aaron.Liu at 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.koenig at 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.leichtzumerken at gmail.com>; amd-
>>>>>> gfx at lists.freedesktop.org; Deucher, Alexander
>>>>>> <Alexander.Deucher at amd.com>; Zhang, Hawking
>>>>>> <Hawking.Zhang at amd.com>; Kuehling, Felix <Felix.Kuehling at amd.com>
>>>>>> Cc: Hou, Xiaomeng (Matthew) <Xiaomeng.Hou at amd.com>; Liu, Aaron
>>>>>> <Aaron.Liu at 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.leichtzumerken at gmail.com>
>>>>>>> Sent: Monday, April 24, 2023 5:07 PM
>>>>>>> To: Xiao, Shane <shane.xiao at amd.com>;
>>>>>>> amd-gfx at lists.freedesktop.org; Deucher, Alexander
>>>>>>> <Alexander.Deucher at amd.com>; Zhang, Hawking
>>>>>>> <Hawking.Zhang at amd.com>; Kuehling, Felix
>>>>>>> <Felix.Kuehling at amd.com>
>>>>>>> Cc: Hou, Xiaomeng (Matthew) <Xiaomeng.Hou at amd.com>; Liu, Aaron
>>>>>>> <Aaron.Liu at 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_enable_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.
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.xiao at amd.com>
>>>>>>>> Signed-off-by: Aaron Liu <aaron.liu at amd.com>
>>>>>>>> Tested-by: Xiaomeng Hou <Xiaomeng.Hou at 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;
>>>>>>>> }
>>>>>>>>
More information about the amd-gfx
mailing list