[Intel-gfx] [PATCH 15/31] drm/i915/slpc: Sanitize GuC version
Sagar Arun Kamble
sagar.a.kamble at intel.com
Thu Sep 28 09:20:21 UTC 2017
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 at intel.com> wrote:
>
>> From: Tom O'Rourke <Tom.O'Rourke at 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 at intel.com>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at 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
More information about the Intel-gfx
mailing list