| Message ID | 20220621233005.3952293-1-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/guc: ADL-N should use the same GuC FW as ADL-S | expand |
On Tue, Jun 21, 2022 at 04:30:05PM -0700, Daniele Ceraolo Spurio wrote: > The only difference between the ADL S and P GuC FWs is the HWConfig > support. ADL-N does not support HWConfig, so we should use the same > binary as ADL-S, otherwise the GuC might attempt to fetch a config > table that does not exist. ADL-N is internally identified as an ADL-P, > so we need to special-case it in the FW selection code. > > Fixes: 7e28d0b26759 ("drm/i915/adl-n: Enable ADL-N platform") > Cc: John Harrison <John.C.Harrison@Intel.com> > Cc: Tejas Upadhyay <tejas.upadhyay@intel.com> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Would the config table still get used somehow even though we return false for ADL-N in has_table()? Even if it couldn't be used, this change makes the behavior more clear and explicit. Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c > index d2c5c9367cc4..ef2d10184ee2 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c > @@ -162,6 +162,15 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw) > u8 rev = INTEL_REVID(i915); > int i; > > + /* > + * The only difference between the ADL GuC FWs is the HWConfig support. > + * ADL-N does not support HWConfig, so we should use the same binary as > + * ADL-S, otherwise the GuC might attempt to fetch a config table that > + * does not exist. > + */ > + if (IS_ADLP_N(i915)) > + p = INTEL_ALDERLAKE_S; > + > GEM_BUG_ON(uc_fw->type >= ARRAY_SIZE(blobs_all)); > fw_blobs = blobs_all[uc_fw->type].blobs; > fw_count = blobs_all[uc_fw->type].count; > -- > 2.25.1 >
On 6/21/2022 5:34 PM, Matt Roper wrote: > On Tue, Jun 21, 2022 at 04:30:05PM -0700, Daniele Ceraolo Spurio wrote: >> The only difference between the ADL S and P GuC FWs is the HWConfig >> support. ADL-N does not support HWConfig, so we should use the same >> binary as ADL-S, otherwise the GuC might attempt to fetch a config >> table that does not exist. ADL-N is internally identified as an ADL-P, >> so we need to special-case it in the FW selection code. >> >> Fixes: 7e28d0b26759 ("drm/i915/adl-n: Enable ADL-N platform") >> Cc: John Harrison <John.C.Harrison@Intel.com> >> Cc: Tejas Upadhyay <tejas.upadhyay@intel.com> >> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Would the config table still get used somehow even though we return > false for ADL-N in has_table()? On platforms that support the config table, the GuC does always try to fetch and decode the table during the firmware init stage, even if we don't query it later due to has_table() being false. Daniele > > Even if it couldn't be used, this change makes the behavior more clear > and explicit. > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > >> --- >> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >> index d2c5c9367cc4..ef2d10184ee2 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >> @@ -162,6 +162,15 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw) >> u8 rev = INTEL_REVID(i915); >> int i; >> >> + /* >> + * The only difference between the ADL GuC FWs is the HWConfig support. >> + * ADL-N does not support HWConfig, so we should use the same binary as >> + * ADL-S, otherwise the GuC might attempt to fetch a config table that >> + * does not exist. >> + */ >> + if (IS_ADLP_N(i915)) >> + p = INTEL_ALDERLAKE_S; >> + >> GEM_BUG_ON(uc_fw->type >= ARRAY_SIZE(blobs_all)); >> fw_blobs = blobs_all[uc_fw->type].blobs; >> fw_count = blobs_all[uc_fw->type].count; >> -- >> 2.25.1 >>
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c index d2c5c9367cc4..ef2d10184ee2 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c @@ -162,6 +162,15 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw) u8 rev = INTEL_REVID(i915); int i; + /* + * The only difference between the ADL GuC FWs is the HWConfig support. + * ADL-N does not support HWConfig, so we should use the same binary as + * ADL-S, otherwise the GuC might attempt to fetch a config table that + * does not exist. + */ + if (IS_ADLP_N(i915)) + p = INTEL_ALDERLAKE_S; + GEM_BUG_ON(uc_fw->type >= ARRAY_SIZE(blobs_all)); fw_blobs = blobs_all[uc_fw->type].blobs; fw_count = blobs_all[uc_fw->type].count;
The only difference between the ADL S and P GuC FWs is the HWConfig support. ADL-N does not support HWConfig, so we should use the same binary as ADL-S, otherwise the GuC might attempt to fetch a config table that does not exist. ADL-N is internally identified as an ADL-P, so we need to special-case it in the FW selection code. Fixes: 7e28d0b26759 ("drm/i915/adl-n: Enable ADL-N platform") Cc: John Harrison <John.C.Harrison@Intel.com> Cc: Tejas Upadhyay <tejas.upadhyay@intel.com> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> --- drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 9 +++++++++ 1 file changed, 9 insertions(+)