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

[Russell, Kent]

[AMD Public Use]



> -----Original Message-----
> From: Kuehling, Felix <Felix.Kuehling at amd.com>
> Sent: Tuesday, September 29, 2020 10:37 AM
> To: Russell, Kent <Kent.Russell at amd.com>; amd-gfx at 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 at 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,