| Message ID | 20230505160415.889525-6-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: GSC FW support for MTL | expand |
Considering the only request i have below is touching up of existing comments (as far as this patch is concerned), and since the rest of the code looks good, here is my R-b - but i hope you can anwser my newbie question at the bottom: Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com> On Fri, 2023-05-05 at 09:04 -0700, Ceraolo Spurio, Daniele wrote: > Add FW definition and the matching override modparam. > > The GSC FW has both a release version, based on platform and a rolling > counter, and a compatibility version, which is the one tracking > interface changes. Since what we care about is the interface, we use > the compatibility version in the buinary names. alan :s/buinary/binary > > Same as with the GuC, a major version bump indicate a > backward-incompatible change, while a minor version bump indicates a > backward-compatible one, so we use only the former in the file name. > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Alan Previn <alan.previn.teres.alexis@intel.com> > Cc: John Harrison <John.C.Harrison@Intel.com> > --- > drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 32 ++++++++++++++++++------ > 1 file changed, 24 insertions(+), 8 deletions(-) > > 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 36ee96c02d74..531cd172151d 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c > @@ -124,6 +124,18 @@ void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw, > fw_def(BROXTON, 0, huc_mmp(bxt, 2, 0, 0)) \ > fw_def(SKYLAKE, 0, huc_mmp(skl, 2, 0, 0)) > > +/* > + * The GSC FW has both a release version, based on platform and a rolling > + * counter, and a compatibility version, which is the one tracking > + * interface changes. Since what we care about is the interface, we use > + * the compatibility version in the buinary names. alan:s/buinary/binary also, since we will (i hope) be adding several comments (alongside the new version objects under intel_gsc_uc structure) in the patch #3 about what their differences are and which one we care about and when they get populated, perhaps we can minimize the information here and redirect to that other comment... OR ... we can minimize the comments in patch #3 and redirect here (will be good to have a single location with detailed explaination in the comments and a redirect-ptr from the other location since a reader would most likely stumble onto those questions from either of these locations). > + * Same as with the GuC, a major version bump indicate a > + * backward-incompatible change, while a minor version bump indicates a > + * backward-compatible one, so we use only the former in the file name. > + */ > +#define INTEL_GSC_FIRMWARE_DEFS(fw_def, gsc_def) \ > + fw_def(METEORLAKE, 0, gsc_def(mtl, 1, 0)) > + > /* > > alan:snip > @@ -257,14 +281,6 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw) > int i; > bool found; > > - /* > - * GSC FW support is still not fully in place, so we're not defining > - * the FW blob yet because we don't want the driver to attempt to load > - * it until we're ready for it. > - */ > - if (uc_fw->type == INTEL_UC_FW_TYPE_GSC) > - return; > - alan: more of a newbie question from myself: considering this is a new firmware we are adding, is there some kind of requirement to provide a link to the patch targetting the linux firmware repo that is a dependency of this series? or perhaps we should mention in the series that merge will only happen after that patch gets merged (with a final rev that includes the patch on the fw-repo side?). Just trying to understand the process. > /* > * 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
On 5/25/2023 4:48 PM, Teres Alexis, Alan Previn wrote: > Considering the only request i have below is touching up of existing comments (as > far as this patch is concerned), and since the rest of the code looks good, here is > my R-b - but i hope you can anwser my newbie question at the bottom: > > Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com> > > On Fri, 2023-05-05 at 09:04 -0700, Ceraolo Spurio, Daniele wrote: >> Add FW definition and the matching override modparam. >> >> The GSC FW has both a release version, based on platform and a rolling >> counter, and a compatibility version, which is the one tracking >> interface changes. Since what we care about is the interface, we use >> the compatibility version in the buinary names. > alan :s/buinary/binary > >> Same as with the GuC, a major version bump indicate a >> backward-incompatible change, while a minor version bump indicates a >> backward-compatible one, so we use only the former in the file name. >> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Alan Previn <alan.previn.teres.alexis@intel.com> >> Cc: John Harrison <John.C.Harrison@Intel.com> >> --- >> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 32 ++++++++++++++++++------ >> 1 file changed, 24 insertions(+), 8 deletions(-) >> >> 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 36ee96c02d74..531cd172151d 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >> @@ -124,6 +124,18 @@ void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw, >> fw_def(BROXTON, 0, huc_mmp(bxt, 2, 0, 0)) \ >> fw_def(SKYLAKE, 0, huc_mmp(skl, 2, 0, 0)) >> >> +/* >> + * The GSC FW has both a release version, based on platform and a rolling >> + * counter, and a compatibility version, which is the one tracking >> + * interface changes. Since what we care about is the interface, we use >> + * the compatibility version in the buinary names. > alan:s/buinary/binary > also, since we will (i hope) be adding several comments (alongside the new > version objects under intel_gsc_uc structure) in the patch #3 about what > their differences are and which one we care about and when they get populated, > perhaps we can minimize the information here and redirect to that other > comment... OR ... we can minimize the comments in patch #3 and redirect here > (will be good to have a single location with detailed explaination in the > comments and a redirect-ptr from the other location since a reader would > most likely stumble onto those questions from either of these locations). I'll redirect, that makes more sense > >> + * Same as with the GuC, a major version bump indicate a >> + * backward-incompatible change, while a minor version bump indicates a >> + * backward-compatible one, so we use only the former in the file name. >> + */ >> +#define INTEL_GSC_FIRMWARE_DEFS(fw_def, gsc_def) \ >> + fw_def(METEORLAKE, 0, gsc_def(mtl, 1, 0)) >> + >> /* >> >> > alan:snip > >> @@ -257,14 +281,6 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw) >> int i; >> bool found; >> >> - /* >> - * GSC FW support is still not fully in place, so we're not defining >> - * the FW blob yet because we don't want the driver to attempt to load >> - * it until we're ready for it. >> - */ >> - if (uc_fw->type == INTEL_UC_FW_TYPE_GSC) >> - return; >> - > alan: more of a newbie question from myself: considering this is a new firmware > we are adding, is there some kind of requirement to provide a link to the patch > targetting the linux firmware repo that is a dependency of this series? > or perhaps we should mention in the series that merge will only happen after > that patch gets merged (with a final rev that includes the patch on > the fw-repo side?). Just trying to understand the process. We usually merge the kernel patch first and do the pull-request to linux-firmware immediately after. We did have cases where we change firmware version between the first rev and the one that gets merged, so we only go to linux-firmware once we're sure it's not going to change anymore. Case in point, the GSC team has asked me to hold off in sending the current blob to linux-firmware because they have some pending fixes they want to include in the first "official" release. Daniele > > >> /* >> * 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
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 36ee96c02d74..531cd172151d 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c @@ -124,6 +124,18 @@ void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw, fw_def(BROXTON, 0, huc_mmp(bxt, 2, 0, 0)) \ fw_def(SKYLAKE, 0, huc_mmp(skl, 2, 0, 0)) +/* + * The GSC FW has both a release version, based on platform and a rolling + * counter, and a compatibility version, which is the one tracking + * interface changes. Since what we care about is the interface, we use + * the compatibility version in the buinary names. + * Same as with the GuC, a major version bump indicate a + * backward-incompatible change, while a minor version bump indicates a + * backward-compatible one, so we use only the former in the file name. + */ +#define INTEL_GSC_FIRMWARE_DEFS(fw_def, gsc_def) \ + fw_def(METEORLAKE, 0, gsc_def(mtl, 1, 0)) + /* * Set of macros for producing a list of filenames from the above table. */ @@ -159,6 +171,9 @@ void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw, #define MAKE_HUC_FW_PATH_MMP(prefix_, major_, minor_, patch_) \ __MAKE_UC_FW_PATH_MMP(prefix_, "huc", major_, minor_, patch_) +#define MAKE_GSC_FW_PATH(prefix_, major_, minor_) \ + __MAKE_UC_FW_PATH_MAJOR(prefix_, "gsc", major_) + /* * All blobs need to be declared via MODULE_FIRMWARE(). * This first expansion of the table macros is solely to provide @@ -169,6 +184,7 @@ void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw, INTEL_GUC_FIRMWARE_DEFS(INTEL_UC_MODULE_FW, MAKE_GUC_FW_PATH_MAJOR, MAKE_GUC_FW_PATH_MMP) INTEL_HUC_FIRMWARE_DEFS(INTEL_UC_MODULE_FW, MAKE_HUC_FW_PATH_BLANK, MAKE_HUC_FW_PATH_MMP, MAKE_HUC_FW_PATH_GSC) +INTEL_GSC_FIRMWARE_DEFS(INTEL_UC_MODULE_FW, MAKE_GSC_FW_PATH) /* * The next expansion of the table macros (in __uc_fw_auto_select below) provides @@ -218,6 +234,10 @@ struct __packed uc_fw_blob { #define HUC_FW_BLOB_GSC(prefix_) \ UC_FW_BLOB_NEW(0, 0, 0, true, MAKE_HUC_FW_PATH_GSC(prefix_)) +#define GSC_FW_BLOB(prefix_, major_, minor_) \ + UC_FW_BLOB_NEW(major_, minor_, 0, true, \ + MAKE_GSC_FW_PATH(prefix_, major_, minor_)) + struct __packed uc_fw_platform_requirement { enum intel_platform p; u8 rev; /* first platform rev using this FW */ @@ -245,9 +265,13 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw) static const struct uc_fw_platform_requirement blobs_huc[] = { INTEL_HUC_FIRMWARE_DEFS(MAKE_FW_LIST, HUC_FW_BLOB, HUC_FW_BLOB_MMP, HUC_FW_BLOB_GSC) }; + static const struct uc_fw_platform_requirement blobs_gsc[] = { + INTEL_GSC_FIRMWARE_DEFS(MAKE_FW_LIST, GSC_FW_BLOB) + }; static const struct fw_blobs_by_type blobs_all[INTEL_UC_FW_NUM_TYPES] = { [INTEL_UC_FW_TYPE_GUC] = { blobs_guc, ARRAY_SIZE(blobs_guc) }, [INTEL_UC_FW_TYPE_HUC] = { blobs_huc, ARRAY_SIZE(blobs_huc) }, + [INTEL_UC_FW_TYPE_GSC] = { blobs_gsc, ARRAY_SIZE(blobs_gsc) }, }; static bool verified[INTEL_UC_FW_NUM_TYPES]; const struct uc_fw_platform_requirement *fw_blobs; @@ -257,14 +281,6 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw) int i; bool found; - /* - * GSC FW support is still not fully in place, so we're not defining - * the FW blob yet because we don't want the driver to attempt to load - * it until we're ready for it. - */ - if (uc_fw->type == INTEL_UC_FW_TYPE_GSC) - return; - /* * 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
Add FW definition and the matching override modparam. The GSC FW has both a release version, based on platform and a rolling counter, and a compatibility version, which is the one tracking interface changes. Since what we care about is the interface, we use the compatibility version in the buinary names. Same as with the GuC, a major version bump indicate a backward-incompatible change, while a minor version bump indicates a backward-compatible one, so we use only the former in the file name. Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Alan Previn <alan.previn.teres.alexis@intel.com> Cc: John Harrison <John.C.Harrison@Intel.com> --- drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 32 ++++++++++++++++++------ 1 file changed, 24 insertions(+), 8 deletions(-)