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@xxxxxxxxx> 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@xxxxxxxxx>
<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",
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
