Re: [PATCH 15/31] drm/i915/slpc: Sanitize GuC version

[Sagar Arun Kamble <sagar.a.kamble@intel.com>]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx