Re: [Intel-xe] [PATCH v2] drm/xe: Fix platform order

[Matt Roper <matthew.d.roper@intel.com>]

On Mon, Apr 03, 2023 at 05:44:33PM +0000, Srivatsa, Anusha wrote:
> 
> 
> > -----Original Message-----
> > From: De Marchi, Lucas <lucas.demarchi@intel.com>
> > Sent: Friday, March 31, 2023 4:09 PM
> > To: intel-xe@lists.freedesktop.org
> > Cc: Roper, Matthew D <matthew.d.roper@intel.com>; Srivatsa, Anusha
> > <anusha.srivatsa@intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>
> > Subject: [PATCH v2] drm/xe: Fix platform order
> > 
> > Platform order in enum xe_platform started to be used by some parts of the
> > code, like the GuC/HuC firmware loading logic. The order itself is not very
> > important, but it's better to follow a convention: as was documented in the
> > comment above the enum, reorder the platforms by graphics version. While at
> > it, remove the gen terminology.
> > 
> > v2:
> >   - Use "graphics version" instead of chronological order (Matt Roper)
> >   - Also change pciidlist to follow the same order
> >   - Remove "gen" from comments around enum xe_platform
> > 
> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_pci.c            |  4 ++--
> >  drivers/gpu/drm/xe/xe_platform_types.h | 12 +++++++-----
> >  drivers/gpu/drm/xe/xe_uc_fw.c          |  4 ++--
> >  3 files changed, 11 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c index
> > 91fed9d3105e..1844cff8fba8 100644
> > --- a/drivers/gpu/drm/xe/xe_pci.c
> > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > @@ -286,11 +286,11 @@ __diag_pop();
> >   */
> >  static const struct pci_device_id pciidlist[] = {
> >  	XE_TGL_IDS(INTEL_VGA_DEVICE, &tgl_desc),
> > +	XE_ADLS_IDS(INTEL_VGA_DEVICE, &adl_s_desc),
> > +	XE_ADLP_IDS(INTEL_VGA_DEVICE, &adl_p_desc),
> >  	XE_DG1_IDS(INTEL_VGA_DEVICE, &dg1_desc),
> >  	XE_ATS_M_IDS(INTEL_VGA_DEVICE, &ats_m_desc),
> >  	XE_DG2_IDS(INTEL_VGA_DEVICE, &dg2_desc),
> > -	XE_ADLS_IDS(INTEL_VGA_DEVICE, &adl_s_desc),
> > -	XE_ADLP_IDS(INTEL_VGA_DEVICE, &adl_p_desc),
> >  	XE_PVC_IDS(INTEL_VGA_DEVICE, &pvc_desc),
> >  	XE_MTL_IDS(INTEL_VGA_DEVICE, &mtl_desc),
> >  	{ }
> > diff --git a/drivers/gpu/drm/xe/xe_platform_types.h
> > b/drivers/gpu/drm/xe/xe_platform_types.h
> > index 72612c832e88..80c19bffe79c 100644
> > --- a/drivers/gpu/drm/xe/xe_platform_types.h
> > +++ b/drivers/gpu/drm/xe/xe_platform_types.h
> > @@ -6,27 +6,29 @@
> >  #ifndef _XE_PLATFORM_INFO_TYPES_H_
> >  #define _XE_PLATFORM_INFO_TYPES_H_
> > 
> > -/* Keep in gen based order, and chronological order within a gen */
> > +/*
> > + * Keep this in graphics version based order and chronological order
> > +within a
> > + * version
> > + */
> >  enum xe_platform {
> >  	XE_PLATFORM_UNINITIALIZED = 0,
> > -	/* gen12 */
> >  	XE_TIGERLAKE,
> >  	XE_ROCKETLAKE,
> > +	XE_ALDERLAKE_S,
> > +	XE_ALDERLAKE_P,
> >  	XE_DG1,
> >  	XE_DG2,
> >  	XE_PVC,
> > -	XE_ALDERLAKE_S,
> > -	XE_ALDERLAKE_P,
> >  	XE_METEORLAKE,
> >  };
> 
> For chronological order, shouldn't it be TGL,RKL, DG1, ADLS and ADLP?

DG1 is 12.10 whereas the others are 12.0, so this matches the guidelines
given in the comment.


Matt

> 
> Anusha 
> >  enum xe_subplatform {
> >  	XE_SUBPLATFORM_UNINITIALIZED = 0,
> >  	XE_SUBPLATFORM_NONE,
> > +	XE_SUBPLATFORM_ADLP_RPLU,
> >  	XE_SUBPLATFORM_DG2_G10,
> >  	XE_SUBPLATFORM_DG2_G11,
> >  	XE_SUBPLATFORM_DG2_G12,
> > -	XE_SUBPLATFORM_ADLP_RPLU,
> >  };
> > 
> >  #endif
> > diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c b/drivers/gpu/drm/xe/xe_uc_fw.c
> > index e2c982b37e87..d1d2e6f0820d 100644
> > --- a/drivers/gpu/drm/xe/xe_uc_fw.c
> > +++ b/drivers/gpu/drm/xe/xe_uc_fw.c
> > @@ -43,11 +43,11 @@ static struct xe_device *uc_fw_to_xe(struct xe_uc_fw
> > *uc_fw)
> >   */
> >  #define XE_GUC_FIRMWARE_DEFS(fw_def, guc_def) \
> >  	fw_def(METEORLAKE,   guc_def(mtl,  70, 5, 2)) \
> > -	fw_def(ALDERLAKE_P,  guc_def(adlp,  70, 5, 2)) \
> > -	fw_def(ALDERLAKE_S,  guc_def(tgl,  70, 5, 2)) \
> >  	fw_def(PVC,          guc_def(pvc,  70, 5, 2)) \
> >  	fw_def(DG2,          guc_def(dg2,  70, 5, 2)) \
> >  	fw_def(DG1,          guc_def(dg1,  70, 5, 2)) \
> > +	fw_def(ALDERLAKE_P,  guc_def(adlp,  70, 5, 2)) \
> > +	fw_def(ALDERLAKE_S,  guc_def(tgl,  70, 5, 2)) \
> >  	fw_def(TIGERLAKE,    guc_def(tgl,  70, 5, 2))
> > 
> >  #define XE_HUC_FIRMWARE_DEFS(fw_def, huc_def, huc_ver) \
> > --
> > 2.39.0
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation