| Message ID | 1505842927-13327-16-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 19 Sep 2017 19:41:51 +0200, Sagar Arun Kamble <sagar.a.kamble@intel.com> wrote: > From: Tom O'Rourke <Tom.O'Rourke@intel.com> > > The SLPC interface is dependent on GuC version. > Only GuC versions known to be compatible are supported here. > > SLPC with GuC firmware v9 is supported with this series. > > v1: Updated with modified sanitize_slpc_option in earlier patch. > > v2-v3: Rebase. > > v4: Updated support for GuC firmware v9. > > v5: Commit subject updated. > > v6: Commit subject and message update. Add support condition as >=v9. > > v7: Sanitizing GuC version in intel_uc_init_fw for SLPC compatibility. > Added info. print for needed version and pointer to 01.org. > > v8: s/FIRMWARE_URL/I915_FIRMWARE_URL, Macro added for SLPC required GuC > Major version and rearrangement for sanitization. (MichalW, Joonas) > > v9: Checking major_ver_found to sanitize SLPC option enable_slpc post > fetching the firmware as with Custom firmware loaded through > guc_firmware_path parameter, major_ver_wanted are cleared. (Lukasz) > > v10: Moved the I915_FIRMWARE_URL macro to intel_uc_common.h. > > Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/intel_csr.c | 15 ++++++--------- > drivers/gpu/drm/i915/intel_guc.h | 1 + > drivers/gpu/drm/i915/intel_guc_loader.c | 15 +++++++++++++++ > drivers/gpu/drm/i915/intel_uc.c | 1 + > drivers/gpu/drm/i915/intel_uc_common.h | 2 ++ > 5 files changed, 25 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_csr.c > b/drivers/gpu/drm/i915/intel_csr.c > index 965988f..56c56f5 100644 > --- a/drivers/gpu/drm/i915/intel_csr.c > +++ b/drivers/gpu/drm/i915/intel_csr.c > @@ -52,11 +52,6 @@ > MODULE_FIRMWARE(I915_CSR_BXT); > #define BXT_CSR_VERSION_REQUIRED CSR_VERSION(1, 7) > -#define FIRMWARE_URL "https://01.org/linuxgraphics/downloads/firmware" > - > - > - > - > #define CSR_MAX_FW_SIZE 0x2FFF > #define CSR_DEFAULT_FW_OFFSET 0xFFFFFFFF > @@ -309,11 +304,12 @@ static uint32_t *parse_csr_fw(struct > drm_i915_private *dev_priv, > if (csr->version != required_version) { > DRM_INFO("Refusing to load DMC firmware v%u.%u," > - " please use v%u.%u [" FIRMWARE_URL "].\n", > + " please use v%u.%u [%s].\n", > CSR_VERSION_MAJOR(csr->version), > CSR_VERSION_MINOR(csr->version), > CSR_VERSION_MAJOR(required_version), > - CSR_VERSION_MINOR(required_version)); > + CSR_VERSION_MINOR(required_version), > + I915_FIRMWARE_URL); Hmm, I'm not sure that including URL here is useful. URL will be repeated in csr_load_work_fn() where we can explain its purpose ;) > return NULL; > } > @@ -420,8 +416,9 @@ static void csr_load_work_fn(struct work_struct > *work) > } else { > dev_notice(dev_priv->drm.dev, > "Failed to load DMC firmware" > - " [" FIRMWARE_URL "]," > - " disabling runtime power management.\n"); > + " [%s]," > + " disabling runtime power management.\n", > + I915_FIRMWARE_URL); Maybe more user friendly message should looks like: "Failed to load DMC firmware, disabling runtime power management." "DMC firmware can be downloaded from https://01.org/linuxgraphics/downloads/firmware" > } > release_firmware(fw); > diff --git a/drivers/gpu/drm/i915/intel_guc.h > b/drivers/gpu/drm/i915/intel_guc.h > index a894991..3821bf2 100644 > --- a/drivers/gpu/drm/i915/intel_guc.h > +++ b/drivers/gpu/drm/i915/intel_guc.h > @@ -159,6 +159,7 @@ static inline void intel_guc_init_early(struct > intel_guc *guc) > int intel_guc_select_fw(struct intel_guc *guc); > int intel_guc_init_hw(struct intel_guc *guc); > u32 intel_guc_wopcm_size(struct intel_guc *guc); > +void intel_guc_fetch_sanitize_options(struct intel_guc *guc); > /* i915_guc_submission.c */ > int i915_guc_submission_init(struct drm_i915_private *dev_priv); > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c > b/drivers/gpu/drm/i915/intel_guc_loader.c > index 6ee7c16..4550620 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -64,6 +64,8 @@ > #define GLK_FW_MAJOR 10 > #define GLK_FW_MINOR 56 > +#define I915_SLPC_REQUIRED_GUC_MAJOR 9 > + > #define GUC_FW_PATH(platform, major, minor) \ > "i915/" __stringify(platform) "_guc_ver" __stringify(major) "_" > __stringify(minor) ".bin" > @@ -418,3 +420,16 @@ int intel_guc_select_fw(struct intel_guc *guc) > return 0; > } > + > +void intel_guc_fetch_sanitize_options(struct intel_guc *guc) > +{ > + if (guc->fw.major_ver_found < > + I915_SLPC_REQUIRED_GUC_MAJOR) { Generally we do not allow Guc fw major "found" to be different than "wanted" so this check could be also done against "wanted". > + DRM_INFO("SLPC not supported with GuC firmware" > + " v%u, please use v%u+ [%s].\n", > + guc->fw.major_ver_found, > + I915_SLPC_REQUIRED_GUC_MAJOR, > + I915_FIRMWARE_URL); Not sure that URL in this message is helpful. > + i915.enable_slpc = 0; > + } > +} > diff --git a/drivers/gpu/drm/i915/intel_uc.c > b/drivers/gpu/drm/i915/intel_uc.c > index eeec986..350027f 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -194,6 +194,7 @@ static void fetch_uc_fw(struct drm_i915_private > *dev_priv, > } > uc_fw->major_ver_found = css->guc.sw_version >> 16; > uc_fw->minor_ver_found = css->guc.sw_version & 0xFFFF; > + intel_guc_fetch_sanitize_options(&dev_priv->guc); This should be done as part of intel_uc_sanitize_options() > break; > case INTEL_UC_FW_TYPE_HUC: > diff --git a/drivers/gpu/drm/i915/intel_uc_common.h > b/drivers/gpu/drm/i915/intel_uc_common.h > index 3de6823..4726511 100644 > --- a/drivers/gpu/drm/i915/intel_uc_common.h > +++ b/drivers/gpu/drm/i915/intel_uc_common.h > @@ -27,6 +27,8 @@ > #include "intel_ringbuffer.h" > #include "i915_vma.h" Add separation line > +#define I915_FIRMWARE_URL > "https://01.org/linuxgraphics/intel-linux-graphics-firmwares" > + > enum intel_uc_fw_status { > INTEL_UC_FIRMWARE_FAIL = -1, > INTEL_UC_FIRMWARE_NONE = 0, Michal
On 9/21/2017 6:22 PM, Michal Wajdeczko wrote: > On Tue, 19 Sep 2017 19:41:51 +0200, Sagar Arun Kamble > <sagar.a.kamble@intel.com> wrote: > >> From: Tom O'Rourke <Tom.O'Rourke@intel.com> >> >> The SLPC interface is dependent on GuC version. >> Only GuC versions known to be compatible are supported here. >> >> SLPC with GuC firmware v9 is supported with this series. >> >> v1: Updated with modified sanitize_slpc_option in earlier patch. >> >> v2-v3: Rebase. >> >> v4: Updated support for GuC firmware v9. >> >> v5: Commit subject updated. >> >> v6: Commit subject and message update. Add support condition as >=v9. >> >> v7: Sanitizing GuC version in intel_uc_init_fw for SLPC compatibility. >> Added info. print for needed version and pointer to 01.org. >> >> v8: s/FIRMWARE_URL/I915_FIRMWARE_URL, Macro added for SLPC required GuC >> Major version and rearrangement for sanitization. (MichalW, Joonas) >> >> v9: Checking major_ver_found to sanitize SLPC option enable_slpc post >> fetching the firmware as with Custom firmware loaded through >> guc_firmware_path parameter, major_ver_wanted are cleared. (Lukasz) >> >> v10: Moved the I915_FIRMWARE_URL macro to intel_uc_common.h. >> >> Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> --- >> drivers/gpu/drm/i915/intel_csr.c | 15 ++++++--------- >> drivers/gpu/drm/i915/intel_guc.h | 1 + >> drivers/gpu/drm/i915/intel_guc_loader.c | 15 +++++++++++++++ >> drivers/gpu/drm/i915/intel_uc.c | 1 + >> drivers/gpu/drm/i915/intel_uc_common.h | 2 ++ >> 5 files changed, 25 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_csr.c >> b/drivers/gpu/drm/i915/intel_csr.c >> index 965988f..56c56f5 100644 >> --- a/drivers/gpu/drm/i915/intel_csr.c >> +++ b/drivers/gpu/drm/i915/intel_csr.c >> @@ -52,11 +52,6 @@ >> MODULE_FIRMWARE(I915_CSR_BXT); >> #define BXT_CSR_VERSION_REQUIRED CSR_VERSION(1, 7) >> -#define FIRMWARE_URL "https://01.org/linuxgraphics/downloads/firmware" >> - >> - >> - >> - >> #define CSR_MAX_FW_SIZE 0x2FFF >> #define CSR_DEFAULT_FW_OFFSET 0xFFFFFFFF >> @@ -309,11 +304,12 @@ static uint32_t *parse_csr_fw(struct >> drm_i915_private *dev_priv, >> if (csr->version != required_version) { >> DRM_INFO("Refusing to load DMC firmware v%u.%u," >> - " please use v%u.%u [" FIRMWARE_URL "].\n", >> + " please use v%u.%u [%s].\n", >> CSR_VERSION_MAJOR(csr->version), >> CSR_VERSION_MINOR(csr->version), >> CSR_VERSION_MAJOR(required_version), >> - CSR_VERSION_MINOR(required_version)); >> + CSR_VERSION_MINOR(required_version), >> + I915_FIRMWARE_URL); > > Hmm, I'm not sure that including URL here is useful. > URL will be repeated in csr_load_work_fn() where we can explain > its purpose ;) Sure. Will remove from here. > > >> return NULL; >> } >> @@ -420,8 +416,9 @@ static void csr_load_work_fn(struct work_struct >> *work) >> } else { >> dev_notice(dev_priv->drm.dev, >> "Failed to load DMC firmware" >> - " [" FIRMWARE_URL "]," >> - " disabling runtime power management.\n"); >> + " [%s]," >> + " disabling runtime power management.\n", >> + I915_FIRMWARE_URL); > > Maybe more user friendly message should looks like: > > "Failed to load DMC firmware, disabling runtime power management." > "DMC firmware can be downloaded from > https://01.org/linuxgraphics/downloads/firmware" Yes. Will update like this. > >> } >> release_firmware(fw); >> diff --git a/drivers/gpu/drm/i915/intel_guc.h >> b/drivers/gpu/drm/i915/intel_guc.h >> index a894991..3821bf2 100644 >> --- a/drivers/gpu/drm/i915/intel_guc.h >> +++ b/drivers/gpu/drm/i915/intel_guc.h >> @@ -159,6 +159,7 @@ static inline void intel_guc_init_early(struct >> intel_guc *guc) >> int intel_guc_select_fw(struct intel_guc *guc); >> int intel_guc_init_hw(struct intel_guc *guc); >> u32 intel_guc_wopcm_size(struct intel_guc *guc); >> +void intel_guc_fetch_sanitize_options(struct intel_guc *guc); >> /* i915_guc_submission.c */ >> int i915_guc_submission_init(struct drm_i915_private *dev_priv); >> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c >> b/drivers/gpu/drm/i915/intel_guc_loader.c >> index 6ee7c16..4550620 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_loader.c >> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c >> @@ -64,6 +64,8 @@ >> #define GLK_FW_MAJOR 10 >> #define GLK_FW_MINOR 56 >> +#define I915_SLPC_REQUIRED_GUC_MAJOR 9 >> + >> #define GUC_FW_PATH(platform, major, minor) \ >> "i915/" __stringify(platform) "_guc_ver" __stringify(major) >> "_" __stringify(minor) ".bin" >> @@ -418,3 +420,16 @@ int intel_guc_select_fw(struct intel_guc *guc) >> return 0; >> } >> + >> +void intel_guc_fetch_sanitize_options(struct intel_guc *guc) >> +{ >> + if (guc->fw.major_ver_found < >> + I915_SLPC_REQUIRED_GUC_MAJOR) { > > Generally we do not allow Guc fw major "found" to be different than > "wanted" > so this check could be also done against "wanted". With Custom firmware loaded through guc_firmware_path parameter, major_ver_wanted is cleared. And then check fails. Lukasz verified this. > >> + DRM_INFO("SLPC not supported with GuC firmware" >> + " v%u, please use v%u+ [%s].\n", >> + guc->fw.major_ver_found, >> + I915_SLPC_REQUIRED_GUC_MAJOR, >> + I915_FIRMWARE_URL); > > Not sure that URL in this message is helpful. Will reword the message like for DMC. > >> + i915.enable_slpc = 0; >> + } >> +} >> diff --git a/drivers/gpu/drm/i915/intel_uc.c >> b/drivers/gpu/drm/i915/intel_uc.c >> index eeec986..350027f 100644 >> --- a/drivers/gpu/drm/i915/intel_uc.c >> +++ b/drivers/gpu/drm/i915/intel_uc.c >> @@ -194,6 +194,7 @@ static void fetch_uc_fw(struct drm_i915_private >> *dev_priv, >> } >> uc_fw->major_ver_found = css->guc.sw_version >> 16; >> uc_fw->minor_ver_found = css->guc.sw_version & 0xFFFF; >> + intel_guc_fetch_sanitize_options(&dev_priv->guc); > > This should be done as part of intel_uc_sanitize_options() During fetch we know what is found hence doing check there helps for case when GuC firmware is supplied through guc_firmware_path parameter. > >> break; >> case INTEL_UC_FW_TYPE_HUC: >> diff --git a/drivers/gpu/drm/i915/intel_uc_common.h >> b/drivers/gpu/drm/i915/intel_uc_common.h >> index 3de6823..4726511 100644 >> --- a/drivers/gpu/drm/i915/intel_uc_common.h >> +++ b/drivers/gpu/drm/i915/intel_uc_common.h >> @@ -27,6 +27,8 @@ >> #include "intel_ringbuffer.h" >> #include "i915_vma.h" > > Add separation line Ok. Will update. > >> +#define I915_FIRMWARE_URL >> "https://01.org/linuxgraphics/intel-linux-graphics-firmwares" >> + >> enum intel_uc_fw_status { >> INTEL_UC_FIRMWARE_FAIL = -1, >> INTEL_UC_FIRMWARE_NONE = 0, > > Michal
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c index 965988f..56c56f5 100644 --- a/drivers/gpu/drm/i915/intel_csr.c +++ b/drivers/gpu/drm/i915/intel_csr.c @@ -52,11 +52,6 @@ MODULE_FIRMWARE(I915_CSR_BXT); #define BXT_CSR_VERSION_REQUIRED CSR_VERSION(1, 7) -#define FIRMWARE_URL "https://01.org/linuxgraphics/downloads/firmware" - - - - #define CSR_MAX_FW_SIZE 0x2FFF #define CSR_DEFAULT_FW_OFFSET 0xFFFFFFFF @@ -309,11 +304,12 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv, if (csr->version != required_version) { DRM_INFO("Refusing to load DMC firmware v%u.%u," - " please use v%u.%u [" FIRMWARE_URL "].\n", + " please use v%u.%u [%s].\n", CSR_VERSION_MAJOR(csr->version), CSR_VERSION_MINOR(csr->version), CSR_VERSION_MAJOR(required_version), - CSR_VERSION_MINOR(required_version)); + CSR_VERSION_MINOR(required_version), + I915_FIRMWARE_URL); return NULL; } @@ -420,8 +416,9 @@ static void csr_load_work_fn(struct work_struct *work) } else { dev_notice(dev_priv->drm.dev, "Failed to load DMC firmware" - " [" FIRMWARE_URL "]," - " disabling runtime power management.\n"); + " [%s]," + " disabling runtime power management.\n", + I915_FIRMWARE_URL); } release_firmware(fw); diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h index a894991..3821bf2 100644 --- a/drivers/gpu/drm/i915/intel_guc.h +++ b/drivers/gpu/drm/i915/intel_guc.h @@ -159,6 +159,7 @@ static inline void intel_guc_init_early(struct intel_guc *guc) int intel_guc_select_fw(struct intel_guc *guc); int intel_guc_init_hw(struct intel_guc *guc); u32 intel_guc_wopcm_size(struct intel_guc *guc); +void intel_guc_fetch_sanitize_options(struct intel_guc *guc); /* i915_guc_submission.c */ int i915_guc_submission_init(struct drm_i915_private *dev_priv); diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 6ee7c16..4550620 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -64,6 +64,8 @@ #define GLK_FW_MAJOR 10 #define GLK_FW_MINOR 56 +#define I915_SLPC_REQUIRED_GUC_MAJOR 9 + #define GUC_FW_PATH(platform, major, minor) \ "i915/" __stringify(platform) "_guc_ver" __stringify(major) "_" __stringify(minor) ".bin" @@ -418,3 +420,16 @@ int intel_guc_select_fw(struct intel_guc *guc) return 0; } + +void intel_guc_fetch_sanitize_options(struct intel_guc *guc) +{ + if (guc->fw.major_ver_found < + I915_SLPC_REQUIRED_GUC_MAJOR) { + DRM_INFO("SLPC not supported with GuC firmware" + " v%u, please use v%u+ [%s].\n", + guc->fw.major_ver_found, + I915_SLPC_REQUIRED_GUC_MAJOR, + I915_FIRMWARE_URL); + i915.enable_slpc = 0; + } +} diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index eeec986..350027f 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -194,6 +194,7 @@ static void fetch_uc_fw(struct drm_i915_private *dev_priv, } uc_fw->major_ver_found = css->guc.sw_version >> 16; uc_fw->minor_ver_found = css->guc.sw_version & 0xFFFF; + intel_guc_fetch_sanitize_options(&dev_priv->guc); break; case INTEL_UC_FW_TYPE_HUC: diff --git a/drivers/gpu/drm/i915/intel_uc_common.h b/drivers/gpu/drm/i915/intel_uc_common.h index 3de6823..4726511 100644 --- a/drivers/gpu/drm/i915/intel_uc_common.h +++ b/drivers/gpu/drm/i915/intel_uc_common.h @@ -27,6 +27,8 @@ #include "intel_ringbuffer.h" #include "i915_vma.h" +#define I915_FIRMWARE_URL "https://01.org/linuxgraphics/intel-linux-graphics-firmwares" + enum intel_uc_fw_status { INTEL_UC_FIRMWARE_FAIL = -1, INTEL_UC_FIRMWARE_NONE = 0,