[Intel-gfx] [PATCH v2 1/2] drm/i915: Impletments dynamic WOPCM partitioning.
Sagar Arun Kamble
sagar.a.kamble at intel.com
Wed Nov 29 10:47:57 UTC 2017
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
>>> <sagar.a.kamble at intel.com> 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 <yaodong.li at intel.com>
<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
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 */
>>>>> + 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",
>>
>
More information about the Intel-gfx
mailing list