[2/3] drm/amdgpu: Fix a typo in vega10_ih_set_rptr()

Submitted by Zhao, Yong on Feb. 6, 2019, 5:01 p.m.

Details

Message ID SN1PR12MB23657F8FB9109F776FD56F46F06F0@SN1PR12MB2365.namprd12.prod.outlook.com
State New
Headers show
Series "Series without cover letter" ( rev: 2 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Zhao, Yong Feb. 6, 2019, 5:01 p.m.
Not too sure about this, but it looks strange in the code when all other similar code uses 64-bit.

"Maybe it makes more sense to think of 64-bit doorbells as using 2 doorbell indexes". You mean the alternative is to multiply all the current 64 doorbell index constants with 2, right? That might be easier and cleaner, and we need to make sure that the *2 and << 1 conversions from 64-bit index to dword index are all removed.

Regards,
Yong

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
index 796004896661..36f0e3cada30 100644
--- a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
+++ b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
@@ -377,7 +377,7 @@  static void vega10_ih_set_rptr(struct amdgpu_device *adev,
        if (ih->use_doorbell) {
                /* XXX check if swapping is necessary on BE */
                *ih->rptr_cpu = ih->rptr;
-               WDOORBELL32(ih->doorbell_idx_in_dw, ih->rptr);
+               WDOORBELL64(ih->doorbell_idx_in_dw, ih->rptr);
        } else if (ih == &adev->irq.ih) {
                WREG32_SOC15(OSSSYS, 0, mmIH_RB_RPTR, ih->rptr);
        } else if (ih == &adev->irq.ih1) {

Comments

On Wed, Feb 6, 2019 at 12:01 PM Zhao, Yong <Yong.Zhao@amd.com> wrote:
>
> Not too sure about this, but it looks strange in the code when all other similar code uses 64-bit.

It's worth double checking, but Felix is right.  A number of IPs still
used 32 bit doorbells on vega10.  E.g., multi-media for example.

Alex

>
> "Maybe it makes more sense to think of 64-bit doorbells as using 2 doorbell indexes". You mean the alternative is to multiply all the current 64 doorbell index constants with 2, right? That might be easier and cleaner, and we need to make sure that the *2 and << 1 conversions from 64-bit index to dword index are all removed.
>
> Regards,
> Yong
> ________________________________
> From: Kuehling, Felix
> Sent: Wednesday, February 6, 2019 11:26 AM
> To: Zhao, Yong; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 2/3] drm/amdgpu: Fix a typo in vega10_ih_set_rptr()
>
> Are you sure about this? Typically 64-bit doorbells don't wrap around. But this one does. If the IH doorbell wraps around, there is no reason why it needs to be 64-bit, so I suspect it may still be a 32-bit doorbell.
>
> AFAIK, not all doorbells on Vega10 are 64-bit. It depends on the IP block. Therefore, maybe some of your other renaming changes should be reconsidered if they assume that all doorbells are 64-bit. Maybe it makes more sense to think of 64-bit doorbells as using 2 doorbell indexes.
>
> Regards,
>   Felix
>
> ________________________________________
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Zhao, Yong <Yong.Zhao@amd.com>
> Sent: Wednesday, February 6, 2019 10:49 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Zhao, Yong
> Subject: [PATCH 2/3] drm/amdgpu: Fix a typo in vega10_ih_set_rptr()
>
> Clearly, it should be a 64-bit doorbell operation.
>
> Change-Id: I644a2ebcb18c2ede24ee15692a6189efad10a35c
> Signed-off-by: Yong Zhao <Yong.Zhao@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/vega10_ih.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
> index 796004896661..36f0e3cada30 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
> @@ -377,7 +377,7 @@ static void vega10_ih_set_rptr(struct amdgpu_device *adev,
>         if (ih->use_doorbell) {
>                 /* XXX check if swapping is necessary on BE */
>                 *ih->rptr_cpu = ih->rptr;
> -               WDOORBELL32(ih->doorbell_idx_in_dw, ih->rptr);
> +               WDOORBELL64(ih->doorbell_idx_in_dw, ih->rptr);
>         } else if (ih == &adev->irq.ih) {
>                 WREG32_SOC15(OSSSYS, 0, mmIH_RB_RPTR, ih->rptr);
>         } else if (ih == &adev->irq.ih1) {
> --
> 2.17.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Am 06.02.19 um 19:17 schrieb Alex Deucher:
> On Wed, Feb 6, 2019 at 12:01 PM Zhao, Yong <Yong.Zhao@amd.com> wrote:
>> Not too sure about this, but it looks strange in the code when all other similar code uses 64-bit.
> It's worth double checking, but Felix is right.  A number of IPs still
> used 32 bit doorbells on vega10.  E.g., multi-media for example.

I agree, the IH is certainly a 32bit doorbell on Vega10.

Christian.

>
> Alex
>
>> "Maybe it makes more sense to think of 64-bit doorbells as using 2 doorbell indexes". You mean the alternative is to multiply all the current 64 doorbell index constants with 2, right? That might be easier and cleaner, and we need to make sure that the *2 and << 1 conversions from 64-bit index to dword index are all removed.
>>
>> Regards,
>> Yong
>> ________________________________
>> From: Kuehling, Felix
>> Sent: Wednesday, February 6, 2019 11:26 AM
>> To: Zhao, Yong; amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH 2/3] drm/amdgpu: Fix a typo in vega10_ih_set_rptr()
>>
>> Are you sure about this? Typically 64-bit doorbells don't wrap around. But this one does. If the IH doorbell wraps around, there is no reason why it needs to be 64-bit, so I suspect it may still be a 32-bit doorbell.
>>
>> AFAIK, not all doorbells on Vega10 are 64-bit. It depends on the IP block. Therefore, maybe some of your other renaming changes should be reconsidered if they assume that all doorbells are 64-bit. Maybe it makes more sense to think of 64-bit doorbells as using 2 doorbell indexes.
>>
>> Regards,
>>    Felix
>>
>> ________________________________________
>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Zhao, Yong <Yong.Zhao@amd.com>
>> Sent: Wednesday, February 6, 2019 10:49 AM
>> To: amd-gfx@lists.freedesktop.org
>> Cc: Zhao, Yong
>> Subject: [PATCH 2/3] drm/amdgpu: Fix a typo in vega10_ih_set_rptr()
>>
>> Clearly, it should be a 64-bit doorbell operation.
>>
>> Change-Id: I644a2ebcb18c2ede24ee15692a6189efad10a35c
>> Signed-off-by: Yong Zhao <Yong.Zhao@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/vega10_ih.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
>> index 796004896661..36f0e3cada30 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
>> @@ -377,7 +377,7 @@ static void vega10_ih_set_rptr(struct amdgpu_device *adev,
>>          if (ih->use_doorbell) {
>>                  /* XXX check if swapping is necessary on BE */
>>                  *ih->rptr_cpu = ih->rptr;
>> -               WDOORBELL32(ih->doorbell_idx_in_dw, ih->rptr);
>> +               WDOORBELL64(ih->doorbell_idx_in_dw, ih->rptr);
>>          } else if (ih == &adev->irq.ih) {
>>                  WREG32_SOC15(OSSSYS, 0, mmIH_RB_RPTR, ih->rptr);
>>          } else if (ih == &adev->irq.ih1) {
>> --
>> 2.17.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
I see. I will drop this. How about the other two patches?

Regards,
Yong