[Intel-gfx] [PATCH v4 1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes

[Yaodong Li]

On 04/19/2018 08:45 AM, Michal Wajdeczko wrote:
> On Wed, 18 Apr 2018 19:01:31 +0200, Jackie Li <yaodong.li at intel.com> 
> wrote:
>
>> After enabled the WOPCM write-once registers locking status checking,
>> reloading of the i915 module will fail with modparam enable_guc set to 3
>> (enable GuC and HuC firmware loading) if the module was originally 
>> loaded
>> with enable_guc set to 1 (only enable GuC firmware loading). This is
>> because WOPCM registers were updated and locked without considering 
>> the HuC
>> FW size. Since we need both GuC and HuC FW sizes to determine the final
>> layout of WOPCM, we should always calculate the WOPCM layout based on 
>> the
>> actual sizes of the GuC and HuC firmware available for a specific 
>> platform
>> if we need continue to support enable/disable HuC FW loading dynamically
>> with enable_guc modparam.
>>
>> This patch splits uC firmware fetching into two stages. First stage 
>> is to
>> fetch the firmware image and verify the firmware header. uC firmware 
>> will
>> be marked as verified and this will make FW info available for following
>> WOPCM layout calculation. The second stage is to create a GEM object and
>> copy the FW data into the created GEM object which will only be 
>> available
>> when GuC/HuC loading is enabled by enable_guc modparam. This will 
>> guarantee
>> that the WOPCM layout will be always be calculated correctly without 
>> making
>> any assumptions to the GuC and HuC firmware sizes.
>>
>> v3:
>>  - Rebase
>>
>> v4:
>>  - Renamed the new parameter add to intel_uc_fw_fetch (Michal)
>>
>> Signed-off-by: Jackie Li <yaodong.li at intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> Cc: Michal Winiarski <michal.winiarski at intel.com>
>> Cc: John Spotswood <john.a.spotswood at intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Reviewed-by: John Spotswood <john.a.spotswood at intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_uc.c    | 14 ++++----------
>>  drivers/gpu/drm/i915/intel_uc_fw.c | 31 ++++++++++++++++++++-----------
>>  drivers/gpu/drm/i915/intel_uc_fw.h |  7 +++++--
>>  3 files changed, 29 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 1cffaf7..73b8f6c 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -172,11 +172,8 @@ void intel_uc_init_early(struct drm_i915_private 
>> *i915)
>>     sanitize_options_early(i915);
>> -    if (USES_GUC(i915))
>> -        intel_uc_fw_fetch(i915, &guc->fw);
>> -
>> -    if (USES_HUC(i915))
>> -        intel_uc_fw_fetch(i915, &huc->fw);
>> +    intel_uc_fw_fetch(i915, &guc->fw, USES_GUC(i915));
>> +    intel_uc_fw_fetch(i915, &huc->fw, USES_HUC(i915));
>
> Again: I'm don't think that unconditional fetch of HuC fw is a right 
> choice.
> We should look for other options how to support enable_guc 1-->3 
> scenario.
>
This is the real fix I can think of to support such a scenario. I think 
the performance
impact here is minimal since it's only one time operation. I will think 
about the
use case of HAS_HUC = 1 and only enable guc submission.

But first I suggest we need to define the expected use case (like I 
mentioned in my last reply):
how to define "support enable_guc 1->3" (whether we should expect some 
system reboot, or
we need guarantee 100% work with no system reboot required)? whether a 
system reboot for
FW changes should be treated as code defects, etc.
>>  }
>> void intel_uc_cleanup_early(struct drm_i915_private *i915)
>> @@ -184,11 +181,8 @@ void intel_uc_cleanup_early(struct 
>> drm_i915_private *i915)
>>      struct intel_guc *guc = &i915->guc;
>>      struct intel_huc *huc = &i915->huc;
>> -    if (USES_HUC(i915))
>> -        intel_uc_fw_fini(&huc->fw);
>> -
>> -    if (USES_GUC(i915))
>> -        intel_uc_fw_fini(&guc->fw);
>> +    intel_uc_fw_fini(&huc->fw);
>> +    intel_uc_fw_fini(&guc->fw);
>>     guc_free_load_err_log(guc);
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c 
>> b/drivers/gpu/drm/i915/intel_uc_fw.c
>> index 6e8e0b5..c1fed06 100644
>> --- a/drivers/gpu/drm/i915/intel_uc_fw.c
>> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c
>> @@ -33,11 +33,13 @@
>>   *
>>   * @dev_priv: device private
>>   * @uc_fw: uC firmware
>> + * @fetch: whether fetch uC firmware into GEM object or not
>>   *
>> - * Fetch uC firmware into GEM obj.
>> + * Fetch and verify uC firmware and copy firmware data into GEM 
>> object if
>> + * @fetch is true.
>>   */
>>  void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
>> -               struct intel_uc_fw *uc_fw)
>> +               struct intel_uc_fw *uc_fw, bool fetch)
>>  {
>>      struct pci_dev *pdev = dev_priv->drm.pdev;
>>      struct drm_i915_gem_object *obj;
>> @@ -154,17 +156,24 @@ void intel_uc_fw_fetch(struct drm_i915_private 
>> *dev_priv,
>>          goto fail;
>>      }
>> -    obj = i915_gem_object_create_from_data(dev_priv, fw->data, 
>> fw->size);
>> -    if (IS_ERR(obj)) {
>> -        err = PTR_ERR(obj);
>> -        DRM_DEBUG_DRIVER("%s fw object_create err=%d\n",
>> -                 intel_uc_fw_type_repr(uc_fw->type), err);
>> -        goto fail;
>> +    uc_fw->size = fw->size;
>> +    uc_fw->fetch_status = INTEL_UC_FIRMWARE_VERIFIED;
>
> btw, I'm not sure that this new state is needed at all, as you don't
> plan to use that fw obj if you only loaded fw blob to read header...
>
we will have the header info stored along with the intel_guc/intel_fw 
anyway.
this state only suggests that valid FW images are available but wasn't 
loaded
to the memory.

Regards,
-Jackie