[PATCH] drm/amdgpu: Enable doorbell selfring if resize BAR successfully

[Christian König]

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?

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