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

[Lucas De Marchi <lucas.demarchi@intel.com>]

On Fri, Mar 31, 2023 at 01:47:20PM -0700, Matt Roper wrote:
>On Fri, Mar 31, 2023 at 07:22:06AM -0600, Lucas De Marchi wrote:
>> On Mon, Mar 27, 2023 at 10:02:38AM -0700, Matt Roper wrote:
>> > On Thu, Mar 23, 2023 at 10:17:53PM -0700, Lucas De Marchi wrote:
>> > > Platform order is important when looping through the list of guc
>> > > firmware blobs since we use it to prevent loading a blob for a newer
>> > > platform onto an older one. Move PVC after ADL.
>> >
>> > Shouldn't we be moving the ADL platforms (graphics versions 12.0) higher
>> > than DG1 (12.10) and DG2 (12.50) too?
>>
>> question then would be:  would we be ordering them by gt
>> version?  Or by when they were introduced?
>
>Since all of the platforms here have the GuC inside the
>graphics IP[*], then the graphics IP version seems natural to me.

The order in drivers/gpu/drm/xe/xe_platform_types.h is unrelated to
anything GuC is doing though. It's the firmware loading code that
decided to use the platform enum value to stop early the iteration on
the table.

>
>"When they were introduced" would be identical for all of these
>platforms for the Xe driver (since we just dumped a big megapatch that
>contained all of these platforms at once).  But if you want to match
>when they were introduced *in i915* that would be reasonable too,

I was meaning more in the sense of "the HW being introduced", not the
support in i915 or xe. My main goal was actually to have the order
in XE_GUC_FIRMWARE_DEFS on the third patch be so that the platforms
using the full version are the top ones.

As you also mention, whatever we do it's sufficient to keep the same
order (for now) in the both the enum and XE_GUC_FIRMWARE_DEFS. I will
send a new version just using graphics version (and updating the comment
on xe_platform, so it's easier to see from the xe driver alone what
order to use.

thanks
Lucas De Marchi


>although the ADLs would still need to come before DG2 in that case.
>
>
>Matt
>
>[*] MTL has a GuC in both the graphics IP and the media IP.  One of our
>questions early on was whether the GuC IP itself would differ between
>the two GTs (requiring different firmwares for each).  The response that
>came back from the hardware team was that that's technically possible
>with standalone media, but at least for MTL they'd keep them identical.
>So for now, just basing 100% on the graphics IP version seems fine.  In
>the future we may need to stop tying GuC to platform at all and instead
>match on the appropriate IP version for whichever GT we're loading on.
>But that's a problem for the future...
>
>
>>
>> I think it makes more sense to be by when they were introduced as a
>> platform in the driver.
>>
>> 	1) what about media/display?
>> 	2) allow us to always be appending in the enum and elsewhere in
>> 	the driver.
>>
>> Lucas De Marchi
>>
>> >
>> >
>> > Matt
>> >
>> > >
>> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> > > ---
>> > >  drivers/gpu/drm/xe/xe_platform_types.h | 3 +--
>> > >  drivers/gpu/drm/xe/xe_uc_fw.c          | 2 +-
>> > >  2 files changed, 2 insertions(+), 3 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/xe/xe_platform_types.h b/drivers/gpu/drm/xe/xe_platform_types.h
>> > > index 72612c832e88..10367f6cc75a 100644
>> > > --- a/drivers/gpu/drm/xe/xe_platform_types.h
>> > > +++ b/drivers/gpu/drm/xe/xe_platform_types.h
>> > > @@ -9,14 +9,13 @@
>> > >  /* Keep in gen based order, and chronological order within a gen */
>> > >  enum xe_platform {
>> > >  	XE_PLATFORM_UNINITIALIZED = 0,
>> > > -	/* gen12 */
>> > >  	XE_TIGERLAKE,
>> > >  	XE_ROCKETLAKE,
>> > >  	XE_DG1,
>> > >  	XE_DG2,
>> > > -	XE_PVC,
>> > >  	XE_ALDERLAKE_S,
>> > >  	XE_ALDERLAKE_P,
>> > > +	XE_PVC,
>> > >  	XE_METEORLAKE,
>> > >  };
>> > >
>> > > diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c b/drivers/gpu/drm/xe/xe_uc_fw.c
>> > > index e2c982b37e87..174c42873ebb 100644
>> > > --- a/drivers/gpu/drm/xe/xe_uc_fw.c
>> > > +++ b/drivers/gpu/drm/xe/xe_uc_fw.c
>> > > @@ -43,9 +43,9 @@ 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(PVC,          guc_def(pvc,  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(TIGERLAKE,    guc_def(tgl,  70, 5, 2))
>> > > --
>> > > 2.39.0
>> > >
>> >
>> > --
>> > Matt Roper
>> > Graphics Software Engineer
>> > Linux GPU Platform Enablement
>> > Intel Corporation
>
>-- 
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation