[Intel-gfx] [PATCH v11 4/6] drm/i915: Add HuC firmware size related restriction for Gen9 and CNL A0

[Yaodong Li]

On 03/01/2018 05:14 AM, Michal Wajdeczko wrote:
> On Thu, 01 Mar 2018 02:01:38 +0100, Jackie Li <yaodong.li at intel.com> 
> wrote:
>
>> On CNL A0 and Gen9, there's a hardware restriction that requires the
>> available GuC WOPCM size to be larger than or equal to HuC firmware 
>> size.
>>
>> This patch adds new verification code to ensure the available GuC WOPCM
>> size to be larger than or equal to HuC firmware size on both Gen9 and 
>> CNL
>> A0.
>>
>> v6:
>>  - Extended HuC FW size check against GuC WOPCM size to all
>>    Gen9 and CNL A0 platforms
>>
>> v7:
>>  - Fixed patch format issues
>>
>> v8:
>>  - Renamed variables and functions to avoid ambiguity (Joonas)
>>  - Updated commit message and comments to be more comprehensive (Sagar)
>>
>> v9:
>>  - Moved code that is not related to restriction check into a separate
>>    patch and updated the commit message accordingly (Sagar/Michal)
>>  - Avoided to call uc_get_fw_size for better layer isolation (Michal)
>>
>> v10:
>>  - Shorten function names and reorganized size_check code to have clear
>>    isolation (Joonas)
>>  - Removed unnecessary comments (Joonas)
>>
>> v11:
>>  - Fixed logic error in size check (Michal)
>>
>> BSpec: 10875
>>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> Cc: John Spotswood <john.a.spotswood at intel.com>
>> Cc: Jeff McGee <jeff.mcgee at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble at intel.com> (v8)
>> Signed-off-by: Jackie Li <yaodong.li at intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_wopcm.c | 28 +++++++++++++++++++++++++---
>>  1 file changed, 25 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c 
>> b/drivers/gpu/drm/i915/intel_wopcm.c
>> index bb78043..b30d7ff 100644
>> --- a/drivers/gpu/drm/i915/intel_wopcm.c
>> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
>> @@ -107,8 +107,26 @@ static inline int gen9_check_dword_gap(u32 
>> guc_wopcm_base, u32 guc_wopcm_size)
>>      return 0;
>>  }
>> +static inline int gen9_check_huc_fw_fits(u32 guc_wopcm_size, u32 
>> huc_fw_size)
>> +{
>> +    /*
>> +     * On Gen9 & CNL A0, hardware requires the total available GuC 
>> WOPCM
>> +     * size to be larger than or equal to HuC firmware size. Otherwise,
>> +     * firmware uploading would fail.
>> +     */
>> +    if (huc_fw_size > guc_wopcm_size - GUC_WOPCM_RESERVED) {
>> +        DRM_ERROR("HuC fw(%uKiB) won't fit in GuC WOPCM(%uKiB).\n",
>> +              huc_fw_size / 1024,
>> +              (guc_wopcm_size - GUC_WOPCM_RESERVED) / 1024);
>
> bikeshed: in earlier patches in similar error messages, you used
> "HuC FW (%KiB)" and didn't provide available space. Maybe simplest
> way to unify and minimize the code is to add one "failed" tag in
> wopcm_init function where you can print all values used for partitioning:
>
> failed:
>     DRM_ERROR("Failed to partition %uKiB WOPCM (%d)\n", 
> wopcm->size/1024, err);
>     DRM_ERROR("HuC FW size=%uKiB\n", ...);
>     DRM_ERROR("GuC FW size=%uKiB\n", ...);
>     return err;
>
I will keep it as it is now to save some time since I was told to keep 
these message
at the point where the error happened.:-)
>> +        return -E2BIG;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  static inline int check_hw_restriction(struct drm_i915_private *i915,
>> -                       u32 guc_wopcm_base, u32 guc_wopcm_size)
>> +                       u32 guc_wopcm_base, u32 guc_wopcm_size,
>> +                       u32 huc_fw_size)
>>  {
>>      int err = 0;
>> @@ -117,7 +135,10 @@ static inline int check_hw_restriction(struct 
>> drm_i915_private *i915,
>>      if (err)
>>          return err;
>> -    return 0;
>> +    if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, 
>> CNL_REVID_A0))
>> +        err = gen9_check_huc_fw_fits(guc_wopcm_size, huc_fw_size);
>> +
>> +    return err;
>>  }
>> /**
>> @@ -186,7 +207,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
>>          return -E2BIG;
>>      }
>> -    err = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size);
>> +    err = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size,
>> +                   huc_fw_size);
>>      if (err) {
>>          DRM_ERROR("Failed to meet HW restriction.\n");
>>          return err;
>
> but bikeshed is not critical, so
>
> Reviewed-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
Thanks,
-Jackie