[PATCH] drm/amdgpu: Use SKU instead of DID for FRU check

[PATCH] drm/amdgpu: Use SKU instead of DID for FRU check

[Kent Russell]

The VG20 DIDs 66a0, 66a1 and 66a4 are used for various SKUs that may or may
not have the FRU EEPROM on it. Parse the VBIOS to check for server SKU
variants (D131 or D134) until a more general solution can be determined.

Signed-off-by: Kent Russell <kent.russell@amd.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c    | 37 +++++++++++++------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
index e811fecc540f..82207b758800 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
@@ -34,18 +34,33 @@
 
 static bool is_fru_eeprom_supported(struct amdgpu_device *adev)
 {
-	/* TODO: Gaming SKUs don't have the FRU EEPROM.
-	 * Use this hack to address hangs on modprobe on gaming SKUs
-	 * until a proper solution can be implemented by only supporting
-	 * the explicit chip IDs for VG20 Server cards
-	 *
-	 * TODO: Add list of supported Arcturus DIDs once confirmed
+	/* TODO: See if we can figure this out dynamically instead of
+	 * having to parse VBIOS versions.
 	 */
-	if ((adev->asic_type == CHIP_VEGA20 && adev->pdev->device == 0x66a0) ||
-	    (adev->asic_type == CHIP_VEGA20 && adev->pdev->device == 0x66a1) ||
-	    (adev->asic_type == CHIP_VEGA20 && adev->pdev->device == 0x66a4))
-		return true;
-	return false;
+	struct atom_context *atom_ctx = adev->mode_info.atom_context;
+	char model[3];
+	int modelnum;
+
+	/* VBIOS is of the format ###-DXXXXYY-XX. We only want the first
+	 * 3 digits after the D, and if we convert it to a hex integer,
+	 * we can use switch for ease/speed/readability and potential
+	 * expandability if required
+	 */
+	strncpy(model, atom_ctx->vbios_version + 5, 3);
+	modelnum = strtoul(model, NULL, 16);
+	switch (adev->asic_type) {
+	case CHIP_VEGA20:
+		switch (modelnum) {
+		/* These are the server VG20 SKUs */
+		case 0x161:
+		case 0x163:
+			return true;
+		default:
+			return false;
+		}
+	default:
+		return false;
+	}
 }
 
 static int amdgpu_fru_read_eeprom(struct amdgpu_device *adev, uint32_t addrptr,
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Use SKU instead of DID for FRU check

[Felix Kuehling]

Am 2020-09-29 um 7:31 a.m. schrieb Kent Russell:
> The VG20 DIDs 66a0, 66a1 and 66a4 are used for various SKUs that may or may
> not have the FRU EEPROM on it. Parse the VBIOS to check for server SKU
> variants (D131 or D134) until a more general solution can be determined.
>
> Signed-off-by: Kent Russell <kent.russell@amd.com>
> ---
>  .../gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c    | 37 +++++++++++++------
>  1 file changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> index e811fecc540f..82207b758800 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> @@ -34,18 +34,33 @@
>  
>  static bool is_fru_eeprom_supported(struct amdgpu_device *adev)
>  {
> -	/* TODO: Gaming SKUs don't have the FRU EEPROM.
> -	 * Use this hack to address hangs on modprobe on gaming SKUs
> -	 * until a proper solution can be implemented by only supporting
> -	 * the explicit chip IDs for VG20 Server cards
> -	 *
> -	 * TODO: Add list of supported Arcturus DIDs once confirmed
> +	/* TODO: See if we can figure this out dynamically instead of
> +	 * having to parse VBIOS versions.
>  	 */
> -	if ((adev->asic_type == CHIP_VEGA20 && adev->pdev->device == 0x66a0) ||
> -	    (adev->asic_type == CHIP_VEGA20 && adev->pdev->device == 0x66a1) ||
> -	    (adev->asic_type == CHIP_VEGA20 && adev->pdev->device == 0x66a4))
> -		return true;
> -	return false;
> +	struct atom_context *atom_ctx = adev->mode_info.atom_context;
> +	char model[3];
> +	int modelnum;
> +
> +	/* VBIOS is of the format ###-DXXXXYY-XX. We only want the first

I'm not sure I understand the format here. There are four X, but you
only want the first three of them?

What are the two X in the end? are they related to the first four X?


> +	 * 3 digits after the D, and if we convert it to a hex integer,
> +	 * we can use switch for ease/speed/readability and potential
> +	 * expandability if required
> +	 */
> +	strncpy(model, atom_ctx->vbios_version + 5, 3);

This produces a string in _model_ that's not 0-terminated.


> +	modelnum = strtoul(model, NULL, 16);

Using strtoul on a non-terminated string is not reliable.

Regards,
  Felix


> +	switch (adev->asic_type) {
> +	case CHIP_VEGA20:
> +		switch (modelnum) {
> +		/* These are the server VG20 SKUs */
> +		case 0x161:
> +		case 0x163:
> +			return true;
> +		default:
> +			return false;
> +		}
> +	default:
> +		return false;
> +	}
>  }
>  
>  static int amdgpu_fru_read_eeprom(struct amdgpu_device *adev, uint32_t addrptr,
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu: Use SKU instead of DID for FRU check

[Russell, Kent]

[AMD Public Use]



> -----Original Message-----
> From: Kuehling, Felix <Felix.Kuehling@amd.com>
> Sent: Tuesday, September 29, 2020 10:37 AM
> To: Russell, Kent <Kent.Russell@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: Use SKU instead of DID for FRU check
> 
> 
> Am 2020-09-29 um 7:31 a.m. schrieb Kent Russell:
> > The VG20 DIDs 66a0, 66a1 and 66a4 are used for various SKUs that may or may
> > not have the FRU EEPROM on it. Parse the VBIOS to check for server SKU
> > variants (D131 or D134) until a more general solution can be determined.
> >
> > Signed-off-by: Kent Russell <kent.russell@amd.com>
> > ---
> >  .../gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c    | 37 +++++++++++++------
> >  1 file changed, 26 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> > index e811fecc540f..82207b758800 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> > @@ -34,18 +34,33 @@
> >
> >  static bool is_fru_eeprom_supported(struct amdgpu_device *adev)
> >  {
> > -	/* TODO: Gaming SKUs don't have the FRU EEPROM.
> > -	 * Use this hack to address hangs on modprobe on gaming SKUs
> > -	 * until a proper solution can be implemented by only supporting
> > -	 * the explicit chip IDs for VG20 Server cards
> > -	 *
> > -	 * TODO: Add list of supported Arcturus DIDs once confirmed
> > +	/* TODO: See if we can figure this out dynamically instead of
> > +	 * having to parse VBIOS versions.
> >  	 */
> > -	if ((adev->asic_type == CHIP_VEGA20 && adev->pdev->device == 0x66a0) ||
> > -	    (adev->asic_type == CHIP_VEGA20 && adev->pdev->device == 0x66a1) ||
> > -	    (adev->asic_type == CHIP_VEGA20 && adev->pdev->device == 0x66a4))
> > -		return true;
> > -	return false;
> > +	struct atom_context *atom_ctx = adev->mode_info.atom_context;
> > +	char model[3];
> > +	int modelnum;
> > +
> > +	/* VBIOS is of the format ###-DXXXXYY-XX. We only want the first
> 
> I'm not sure I understand the format here. There are four X, but you
> only want the first three of them?
> 
> What are the two X in the end? are they related to the first four X?
I tried to edit it but made a mistake, it seems. It should be ###-DXXXYY-ZZ . And the last 2 are unrelated, they're for further differentiation.
> 
> 
> > +	 * 3 digits after the D, and if we convert it to a hex integer,
> > +	 * we can use switch for ease/speed/readability and potential
> > +	 * expandability if required
> > +	 */
> > +	strncpy(model, atom_ctx->vbios_version + 5, 3);
> 
> This produces a string in _model_ that's not 0-terminated.
> 
> 
> > +	modelnum = strtoul(model, NULL, 16);
> 
> Using strtoul on a non-terminated string is not reliable.
Right! Thanks, I'll clean that up.

> 
> Regards,
>   Felix
> 
> 
> > +	switch (adev->asic_type) {
> > +	case CHIP_VEGA20:
> > +		switch (modelnum) {
> > +		/* These are the server VG20 SKUs */
> > +		case 0x161:
> > +		case 0x163:
> > +			return true;
> > +		default:
> > +			return false;
> > +		}
> > +	default:
> > +		return false;
> > +	}
> >  }
> >
> >  static int amdgpu_fru_read_eeprom(struct amdgpu_device *adev, uint32_t addrptr,
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx