[PATCH] drm/amdgpu/si: fix ASIC tests

[PATCH] drm/amdgpu/si: fix ASIC tests

[Jean Delvare]

Comparing adev->family with CHIP constants is not correct.
adev->family can only be compared with AMDGPU_FAMILY constants and
adev->asic_type is the struct member to compare with CHIP constants.
They are separate identification spaces.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Fixes: 62a37553414a ("drm/amdgpu: add si implementation v10")
Cc: Ken Wang <Qingqing.Wang@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: "David (ChunMing) Zhou" <David1.Zhou@amd.com>
---
I stumbled upon this while reading the code. The comparisons look
obviously incorrect, but on the other hand it's hard to believe that
the bug has been there for almost 4 years without any user reporting
any actual problem caused by it. So this probably requires a more
in-depth analysis by someone familiar with the code.

I tested these changes on my Oland card and did not notice any
difference, but I don't know what I should be looking at.

 drivers/gpu/drm/amd/amdgpu/si.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

--- linux-5.2.orig/drivers/gpu/drm/amd/amdgpu/si.c	2019-07-08 00:41:56.000000000 +0200
+++ linux-5.2/drivers/gpu/drm/amd/amdgpu/si.c	2019-08-28 10:29:43.544180131 +0200
@@ -1867,7 +1867,7 @@ static void si_program_aspm(struct amdgp
 			if (orig != data)
 				si_pif_phy1_wreg(adev,PB1_PIF_PWRDOWN_1, data);
 
-			if ((adev->family != CHIP_OLAND) && (adev->family != CHIP_HAINAN)) {
+			if ((adev->asic_type != CHIP_OLAND) && (adev->asic_type != CHIP_HAINAN)) {
 				orig = data = si_pif_phy0_rreg(adev,PB0_PIF_PWRDOWN_0);
 				data &= ~PLL_RAMP_UP_TIME_0_MASK;
 				if (orig != data)
@@ -1916,14 +1916,14 @@ static void si_program_aspm(struct amdgp
 
 			orig = data = si_pif_phy0_rreg(adev,PB0_PIF_CNTL);
 			data &= ~LS2_EXIT_TIME_MASK;
-			if ((adev->family == CHIP_OLAND) || (adev->family == CHIP_HAINAN))
+			if ((adev->asic_type == CHIP_OLAND) || (adev->asic_type == CHIP_HAINAN))
 				data |= LS2_EXIT_TIME(5);
 			if (orig != data)
 				si_pif_phy0_wreg(adev,PB0_PIF_CNTL, data);
 
 			orig = data = si_pif_phy1_rreg(adev,PB1_PIF_CNTL);
 			data &= ~LS2_EXIT_TIME_MASK;
-			if ((adev->family == CHIP_OLAND) || (adev->family == CHIP_HAINAN))
+			if ((adev->asic_type == CHIP_OLAND) || (adev->asic_type == CHIP_HAINAN))
 				data |= LS2_EXIT_TIME(5);
 			if (orig != data)
 				si_pif_phy1_wreg(adev,PB1_PIF_CNTL, data);


-- 
Jean Delvare
SUSE L3 Support
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu/si: fix ASIC tests

[Alex Deucher]

On Wed, Aug 28, 2019 at 11:28 AM Jean Delvare <jdelvare@suse.de> wrote:
>
> Comparing adev->family with CHIP constants is not correct.
> adev->family can only be compared with AMDGPU_FAMILY constants and
> adev->asic_type is the struct member to compare with CHIP constants.
> They are separate identification spaces.
>
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Fixes: 62a37553414a ("drm/amdgpu: add si implementation v10")
> Cc: Ken Wang <Qingqing.Wang@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: "David (ChunMing) Zhou" <David1.Zhou@amd.com>
> ---
> I stumbled upon this while reading the code. The comparisons look
> obviously incorrect, but on the other hand it's hard to believe that
> the bug has been there for almost 4 years without any user reporting
> any actual problem caused by it. So this probably requires a more
> in-depth analysis by someone familiar with the code.
>
> I tested these changes on my Oland card and did not notice any
> difference, but I don't know what I should be looking at.

It's leftover from when the code was ported to radeon.  radeon used
family for asic_type and amdgpu has both.  It probably got missed when
the code was ported.  The differences between those chips and others
are pretty minor (a couple of new fields and additional instances of
some registers), so it may not have had much effect.  Anyway, the
change is correct.  Applied.  Thanks!

Alex

>
>  drivers/gpu/drm/amd/amdgpu/si.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> --- linux-5.2.orig/drivers/gpu/drm/amd/amdgpu/si.c      2019-07-08 00:41:56.000000000 +0200
> +++ linux-5.2/drivers/gpu/drm/amd/amdgpu/si.c   2019-08-28 10:29:43.544180131 +0200
> @@ -1867,7 +1867,7 @@ static void si_program_aspm(struct amdgp
>                         if (orig != data)
>                                 si_pif_phy1_wreg(adev,PB1_PIF_PWRDOWN_1, data);
>
> -                       if ((adev->family != CHIP_OLAND) && (adev->family != CHIP_HAINAN)) {
> +                       if ((adev->asic_type != CHIP_OLAND) && (adev->asic_type != CHIP_HAINAN)) {
>                                 orig = data = si_pif_phy0_rreg(adev,PB0_PIF_PWRDOWN_0);
>                                 data &= ~PLL_RAMP_UP_TIME_0_MASK;
>                                 if (orig != data)
> @@ -1916,14 +1916,14 @@ static void si_program_aspm(struct amdgp
>
>                         orig = data = si_pif_phy0_rreg(adev,PB0_PIF_CNTL);
>                         data &= ~LS2_EXIT_TIME_MASK;
> -                       if ((adev->family == CHIP_OLAND) || (adev->family == CHIP_HAINAN))
> +                       if ((adev->asic_type == CHIP_OLAND) || (adev->asic_type == CHIP_HAINAN))
>                                 data |= LS2_EXIT_TIME(5);
>                         if (orig != data)
>                                 si_pif_phy0_wreg(adev,PB0_PIF_CNTL, data);
>
>                         orig = data = si_pif_phy1_rreg(adev,PB1_PIF_CNTL);
>                         data &= ~LS2_EXIT_TIME_MASK;
> -                       if ((adev->family == CHIP_OLAND) || (adev->family == CHIP_HAINAN))
> +                       if ((adev->asic_type == CHIP_OLAND) || (adev->asic_type == CHIP_HAINAN))
>                                 data |= LS2_EXIT_TIME(5);
>                         if (orig != data)
>                                 si_pif_phy1_wreg(adev,PB1_PIF_CNTL, data);
>
>
> --
> Jean Delvare
> SUSE L3 Support
> _______________________________________________
> 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