[Intel-xe] [PATCH 1/2] drm/xe: Add checks to load huc from a previous platform

[Matt Roper]

On Tue, Mar 21, 2023 at 08:34:47AM -0700, Matt Roper wrote:
> On Mon, Mar 20, 2023 at 04:52:53PM -0700, Anusha Srivatsa wrote:
> > Platforms like ADL-S use TGL HuC. Add suitable condition
> > so driver does not disable HuC.
> > 
> > Cc: Matt Atwood <matthew.s.atwood at intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa at intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_uc_fw.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c b/drivers/gpu/drm/xe/xe_uc_fw.c
> > index 00408f1b73e1..e2e5f7fbe9db 100644
> > --- a/drivers/gpu/drm/xe/xe_uc_fw.c
> > +++ b/drivers/gpu/drm/xe/xe_uc_fw.c
> > @@ -139,7 +139,7 @@ uc_fw_auto_select(struct xe_device *xe, struct xe_uc_fw *uc_fw)
> >  	fw_count = blobs_all[uc_fw->type].count;
> >  
> >  	for (i = 0; i < fw_count && p <= fw_blobs[i].p; i++) {
> > -		if (p == fw_blobs[i].p && rev >= fw_blobs[i].rev) {
> > +		if (p >= fw_blobs[i].p && rev >= fw_blobs[i].rev) {
> 
> Isn't this going to make various platforms pick up firmwares that we
> don't want it to?  I.e., how do you deal with platforms that don't
> want/need a HuC now if they automatically just start picking up a HuC
> for the next older platform?
> 
> It's not clear to me why this is needed either; once you pass the code
> through the preprocessor ("make drivers/gpu/drm/xe/xe_uc_fw.i"), the GuC
> firmware table looks like:
> 
> static const struct uc_fw_platform_requirement blobs_guc[] = {
>   { .p = XE_METEORLAKE, .rev = 0, .blob = { .major = 70, .minor = 5, .path = "i915/" "mtl" "_" "guc" "_" "70" ".bin" }, },
>   { .p = XE_ALDERLAKE_P, .rev = 0, .blob = { .major = 70, .minor = 5, .path = "i915/" "adlp" "_" "guc" "_" "70" ".bin" }, },
>   { .p = XE_ALDERLAKE_S, .rev = 0, .blob = { .major = 70, .minor = 5, .path = "i915/" "tgl" "_" "guc" "_" "70" ".bin" }, },
>   { .p = XE_PVC, .rev = 0, .blob = { .major = 70, .minor = 5, .path = "i915/" "pvc" "_" "guc" "_" "70" ".bin" }, },
>   { .p = XE_DG2, .rev = 0, .blob = { .major = 70, .minor = 5, .path = "i915/" "dg2" "_" "guc" "_" "70" ".bin" }, },
>   { .p = XE_DG1, .rev = 0, .blob = { .major = 70, .minor = 5, .path = "i915/" "dg1" "_" "guc" "_" "70" ".bin" }, },
>   { .p = XE_TIGERLAKE, .rev = 0, .blob = { .major = 70, .minor = 5, .path = "i915/" "tgl" "_" "guc" "_" "70" ".bin" }, },
> };

Sorry, that's the GuC table.  The HuC table is

 static const struct uc_fw_platform_requirement blobs_huc[] = {
  { .p = XE_DG1, .rev = 0, .blob = { .major = 7, .minor = 9, .path = "i915/" "dg1" "_huc_" "7" "." "9" "." "3" ".bin" }, }, 
  { .p = XE_TIGERLAKE, .rev = 0, .blob = { .major = 7, .minor = 9, .path = "i915/" "tgl" "_huc_" "7" "." "9" "." "3" ".bin" }, },
 };

But it should still just be a matter of adding table entries for the
other platforms to specify which firmware we want them to load now,
right?  Otherwise, we're going to wind up with stuff like all of DG2,
PVC, ADL-S, ADL-P, and MTL automatically falling back to use the DG1
firmware, which isn't what we want.


Matt

> 
> so it seems like the direct == test would still be what we want?  The
> only platform missing from the table today is RKL, but we can add that.
> 
> As an aside, the platform ordering here and in the definition of 'enum
> xe_platform' seems incorrect.  Since they're both incorrect in the same
> way, I don't think it causes a problem, but we should probably clean
> that up at some point too.
> 
> 
> 
> Matt
> 
> >  			const struct uc_fw_blob *blob = &fw_blobs[i].blob;
> >  
> >  			uc_fw->path = blob->path;
> > -- 
> > 2.25.1
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation

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