[v2,1/2] drm/i915: Impletments dynamic WOPCM partitioning.

Commit Message

[e-mail] Nov. 29, 2017, 10:47 a.m. UTC

On 11/29/2017 6:31 AM, Yaodong Li wrote:
> On 11/16/2017 08:00 PM, Sagar Arun Kamble wrote:
>>
>>
>> On 11/17/2017 3:17 AM, Michal Wajdeczko wrote:
>>> On Thu, 16 Nov 2017 08:34:01 +0100, Sagar Arun Kamble 
>>> <[e-mail]> wrote:
>>>
>>>> Typo in the subject.
>>>> GLK showing failure to load GuC with this approach on CI.
>>>>
>>>> On 11/15/2017 10:47 PM, Jackie Li wrote:
>>>>> Static WOPCM partitioning will lead to GuC loading failure
>>>>> if the GuC/HuC firmware size exceeded current static 512KB
>>>>> partition boundary.
>>>>>
>>>>> This patch enables the dynamical calculation of the WOPCM aperture
>>>>> used by GuC/HuC firmware. GuC WOPCM offset is  set to
>>>>> HuC size + reserved WOPCM size. GuC WOPCM size is set to
>>>>> total WOPCM size - GuC WOPCM offset - RC6CTX size. In this case,
>>>>> GuC WOPCM offset will be updated based on the size of HuC firmware
>>>>> while GuC WOPCM size will be set to use all the remaining WOPCM 
>>>>> space.
>>>>>
>>>>> v2:
>>>>>   - Removed intel_wopcm_init (Ville/Sagar/Joonas)
>>>>>   - Renamed and Moved the intel_wopcm_partition into intel_guc 
>>>>> (Sagar)
>>>>>   - Removed unnecessary function calls (Joonas)
>>>>>   - Init GuC WOPCM partition as soon as firmware fetching is 
>>>>> completed
>>>>>
>>>>> Signed-off-by: Jackie Li <[e-mail]>
<snip>
>>>>> +    err = do_wopcm_partition(i915, offset + huc_size, reserved);
>>>>> +    if (err) {
>>>>> +        if (!huc_size)
>>>>> +            goto fail;
>>>>> +
>>>>> +        /* Partition without loading HuC FW. */
>>>>> +        err = do_wopcm_partition(i915, offset, reserved);
>>>>> +        if (err)
>>>>> +            goto fail;
>>>>> +    }
>>>>> +
>>>>> +    /*
>>>>> +     * Check hardware restriction on Gen9
>>>>> +     * GuC WOPCM size is at least 4 bytes larger than GuC WOPCM 
>>>>> base due
>>>>> +     * to hardware limitation on Gen9.
>>>>> +     */
>>>>> +    if (IS_GEN9(i915)) {
>>>>> +        wopcm_base = wp->offset + GEN9_GUC_WOPCM_OFFSET;
>>>>> +        if (unlikely(wopcm_base > wp->size))
>>>>> +            goto fail;
>>>>> +
>>>>> +        delta = wp->size - wopcm_base;
>>>>> +        if (unlikely(delta < GEN9_GUC_WOPCM_DELTA))
>>>>> +            goto fail;
>>>>> +    }
>>>>> +
>>>>> +    if (guc_fw->fetch_status == INTEL_UC_FIRMWARE_SUCCESS) {
>>>>> +        guc_size = guc_fw->header_size + guc_fw->ucode_size;
>>>>> +        /* Need 8K stack for GuC */
>>>>> +        guc_size += GUC_WOPCM_STACK_RESERVED;
>>>>> +    }
>>>>> +
>>>>> +    if (guc_size > wp->size)
>>>>> +        goto fail;
>>>>> +
>>>>> +    DRM_DEBUG_DRIVER("GuC WOPCM offset %dKB, size %dKB, top %dKB\n",
>>>>> +        wp->offset >> 10, wp->size >> 10, wp->top >> 10);
>>>>> +
>>>>> +    return;
>>>>> +
>>>>> +fail:
>>>>> +    DRM_ERROR("WOPCM partitioning failed. Falling back to 
>>>>> execlist mode\n");
>>>>> +
>>>>> +    intel_uc_fini_fw(i915);
>> This is correct but should be handled from intel_uc_init_fw with this 
>> function returning status.
> it's a good idea. I will do it.
Something like this will be good

>>>>> +    if (i915_modparams.enable_guc_submission)
>>>>> +        i915_modparams.enable_guc_submission = 0;
>>>>> +
>>>>> +    i915_modparams.enable_guc_loading = 0;
>>>>> +}
>> This sanitization will be handled by intel_guc_fw_upload during 
>> intel_uc_init_hw so not needed.
> It's too late to do it until intel_uc_init_hw.
Yes. Let us not do wopcm_init in uc_init_hw as wopcm_init is one time task.
This modparam update is correct.
> what I wanted to guarantee here was guc submission
> would be disabled as long as the partitioning failed, so that we won't 
> need to allocate the kernel
> context above guc wopcm top. we sure can ignore the partitioning 
> failure can continue allocating
> the context above guc wopcm top, but the problem is we don't have a 
> valid guc wopcm top value
> if the partitioning was failed. another benefit to disable the guc 
> here is we won't even bother to call
> down into the logics in intel_uc_init_hw since we've already known for 
> sure. I cannot enable guc
> submission on the partitioning failure.
Agree.
>>>>> +
>>>>>   void intel_uc_init_fw(struct drm_i915_private *dev_priv)
>>>>>   {
>>>>>       intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw);
>>>>>       intel_uc_fw_fetch(dev_priv, &dev_priv->guc.fw);
>>>>> +    guc_init_wopcm_partition(dev_priv);
>>>> Create intel_uc_init_wopcm_partition(dev_priv) and call 
>>>> intel_guc_init_wopcm_partition(guc) from there.
>>>
>>> hmm, I'm not sure what benefit you expect from this call chain ?
>>>
>> wanted to avoid firmware specific calls from here but I was wrong as 
>> we are not expecting this function to be called from
>> outside of intel_uc. sorry.
>>>>>   }
>>>>>     void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
>>>>> @@ -174,9 +286,9 @@ int intel_uc_init_hw(struct drm_i915_private 
>>>>> *dev_priv)
>>>>>       }
>>>>>         /* init WOPCM */
>>>>> -    I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(dev_priv));
>>>>> +    I915_WRITE(GUC_WOPCM_SIZE, guc->wopcm.size);
>>>>>       I915_WRITE(DMA_GUC_WOPCM_OFFSET,
>>>>> -           GUC_WOPCM_OFFSET_VALUE | HUC_LOADING_AGENT_GUC);
>>>>> +           guc->wopcm.offset | HUC_LOADING_AGENT_GUC);
>>>>>         /* WaEnableuKernelHeaderValidFix:skl */
>>>>>       /* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
>>>>> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c 
>>>>> b/drivers/gpu/drm/i915/intel_uc_fw.c
>>>>> index 4bc82d3..34db2b1 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_uc_fw.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c
>>>>> @@ -95,9 +95,13 @@ void intel_uc_fw_fetch(struct drm_i915_private 
>>>>> *dev_priv,
>>>>>       uc_fw->ucode_offset = uc_fw->header_offset + 
>>>>> uc_fw->header_size;
>>>>>       uc_fw->ucode_size = (css->size_dw - css->header_size_dw) * 
>>>>> sizeof(u32);
>>>>>   -    /* Header and uCode will be loaded to WOPCM */
>>>>> +    /*
>>>>> +     * Header and uCode will be loaded to WOPCM
>>>>> +     * Only check the size against the overall available WOPCM 
>>>>> here. Will
>>>>> +     * continue to check the size during WOPCM partition 
>>>>> calculation.
>>>>> +     */
>>>>>       size = uc_fw->header_size + uc_fw->ucode_size;
>>>>> -    if (size > intel_guc_wopcm_size(dev_priv)) {
>>>>> +    if (size > WOPCM_DEFAULT_SIZE) {
>>>>>           DRM_WARN("%s: Firmware is too large to fit in WOPCM\n",
>>>>>                intel_uc_fw_type_repr(uc_fw->type));
>>>>>           err = -E2BIG;
>>>>> @@ -207,6 +211,7 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
>>>>>                  int (*xfer)(struct intel_uc_fw *uc_fw,
>>>>>                      struct i915_vma *vma))
>>>>>   {
>>>>> +    struct drm_i915_private *i915 = to_i915(uc_fw->obj->base.dev);
>>>>>       struct i915_vma *vma;
>>>>>       int err;
>>>>>   @@ -230,7 +235,7 @@ int intel_uc_fw_upload(struct intel_uc_fw 
>>>>> *uc_fw,
>>>>>       }
>>>>>         vma = i915_gem_object_ggtt_pin(uc_fw->obj, NULL, 0, 0,
>>>>> -                       PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
>>>>> +                       PIN_OFFSET_BIAS | i915->guc.wopcm.top);
>>>>>       if (IS_ERR(vma)) {
>>>>>           err = PTR_ERR(vma);
>>>>>           DRM_DEBUG_DRIVER("%s fw ggtt-pin err=%d\n",
>>
>

Patch

diff --git a/drivers/gpu/drm/i915/intel_uc.c 
b/drivers/gpu/drm/i915/intel_uc.c
index 1e2a30a..04c45d0 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -90,6 +90,15 @@  void intel_uc_init_fw(struct drm_i915_private *dev_priv)
  {
         intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw);
         intel_uc_fw_fetch(dev_priv, &dev_priv->guc.fw);
+
+       ret = intel_uc_init_wopcm_partition(dev_priv);
+       if (ret) {
+               intel_uc_fw_fini(&dev_priv->guc.fw);
+               intel_uc_fw_fini(&dev_priv->huc.fw);
+               i915_modparams.enable_guc_loading = 0;
+               i915_modparams.enable_guc_submission = 0;
+               i915_modparams.guc_log_level = -1;
+       }
  }
>>>>> +    /* GuC submission won't work without valid GuC WOPCM 
>>>>> partition */