Re: [PATCH v8 3/6] drm/i915/guc: Implement dynamic GuC WOPCM offset and size

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 02/06/2018 02:25 PM, Michal Wajdeczko wrote:

default_desc_template(dev_priv, dev_priv->mm.aliasing_ppgtt);
  -    /* GuC requires the ring to be placed above GUC_WOPCM_TOP. If GuC is not
+    /*
+     * GuC requires the ring to be placed above GuC WOPCM top. If GuC is not        * present or not in use we still need a small bias as ring wraparound
       * at offset 0 sometimes hangs. No idea why.
       */
      if (USES_GUC(dev_priv))
-        ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
+        ctx->ggtt_offset_bias = dev_priv->guc.wopcm.top;

I know that Joonas was against using extra inline function to read
value of "guc.wopcm.top" but maybe we should discuss it again as
now we maybe using invalid/unset value since now we are not verifying
"guc->wopcm.valid" flag.

It should be fine in this case since we have early inited the value to the worst case (using the size of all WOPCM). we could not use valid here since uc_init
has been called at this point.

+static inline int gen9_guc_wopcm_size_check(struct drm_i915_private *i915)

maybe it would be better to pass "wopcm" instead of "i915" ?

hmm, I think it's better to have an unified parameter list for all the check functions.
and it the same since we will have to get wopcm from i915 anyways.
+{
+    struct intel_guc_wopcm *wopcm = &i915->guc.wopcm;
use variable name as guc_wopcm?
+    u32 guc_wopcm_start;
+    u32 delta;
+
+    /*
+     * GuC WOPCM size is at least 4 bytes larger than the offset from WOPCM

Either use 4B directly below (as you give explanation here) or move that
comment near the definition of GEN9_GUC_WOPCM_DELTA.

+     * base (GuC WOPCM offset from WOPCM base + GEN9_GUC_WOPCM_OFFSET) due
+     * to hardware limitation on Gen9.
+     */
+    guc_wopcm_start = wopcm->offset + GEN9_GUC_WOPCM_OFFSET;
+    if (unlikely(guc_wopcm_start > wopcm->size))
+        return -E2BIG;
+
+    delta = wopcm->size - guc_wopcm_start;
+    if (unlikely(delta < GEN9_GUC_WOPCM_DELTA))
+        return -E2BIG;
+
+    return 0;
+}
+
+static inline int guc_wopcm_check_hw_restrictions(struct intel_guc *guc)
+{
+    struct drm_i915_private *i915 = guc_to_i915(guc);
+
+    if (IS_GEN9(i915))
+        return gen9_guc_wopcm_size_check(i915);

please use function name that matches dispatcher function

guc_wopcm_check_hw_restrictions()
gen9_guc_wopcm_check_hw_restrictions()

or

guc_wopcm_size_check()
gen9_guc_wopcm_size_check()

+
+    return 0;
+}
+
+/**
+ * intel_guc_wopcm_init() - Initialize the GuC WOPCM..
remove extra "."
   * @guc: intel guc.
+ * @guc_fw_size: size of GuC firmware.
+ * @huc_fw_size: size of HuC firmware.
   *
- * Get the platform specific GuC WOPCM size.
+ * Calculate the GuC WOPCM offset and size based on GuC and HuC firmware sizes. + * This function will to set the GuC WOPCM size to the size of maximum WOPCM
remove "to"
+ * available for GuC. This function will also enforce platform dependent + * hardware restrictions on GuC WOPCM offset and size. It will fail the GuC + * WOPCM init if any of these checks were failed, so that the following GuC
+ * firmware uploading would be aborted.
   *
- * Return: size of the GuC WOPCM.
+ * Return: 0 on success, non-zero error code on failure.
   */
-u32 intel_guc_wopcm_size(struct intel_guc *guc)

Hmm, it looks that all your changes for this function from patch 2/6
are now lost ...

patch 1/6 & 2/6 are only for some code movings. but have to make it clean.
but I do not need this func anymore:o)
+int intel_guc_wopcm_init(struct intel_guc *guc, u32 guc_fw_size,
+             u32 huc_fw_size)
  {
-    struct drm_i915_private *i915 = guc_to_i915(guc);
-    u32 size = GUC_WOPCM_TOP;
+    u32 reserved = guc_reserved_wopcm_size(guc);
+    u32 offset, size, top;
+    int err;
  -    /* On BXT, the top of WOPCM is reserved for RC6 context */
-    if (IS_GEN9_LP(i915))
-        size -= BXT_GUC_WOPCM_RC6_RESERVED;
+    if (guc->wopcm.valid)
+        return 0;

Is there a scenario when this function will be called more than one?
You're right. we need not need such a check anymore.

+
+    if (!guc_fw_size)
+        return -EINVAL;

GEM_BUG_ON ?

+
+    if (reserved >= WOPCM_DEFAULT_SIZE)
+        return -E2BIG;

GEM_BUG_ON ?
Will give the control back to uc_init.?

+
+    offset = huc_fw_size + WOPCM_RESERVED_SIZE;

What's the difference between guc_reserved_wopcm_size and WOPCM_RESERVED_SIZE

WOPCM_RESERVED_SIZE is the reserved size in Non-GuC WOPCM.
+    guc->wopcm.offset = offset;
+    guc->wopcm.size = size;
+    guc->wopcm.top = top;
+
+    /* Check platform specific restrictions */
+    err = guc_wopcm_check_hw_restrictions(guc);

maybe you should check HW restrictions *before* initializing the structure?

err = check_hw_restrictions(i915, offset, size, top);
Actually, I was struggling on this a bit, but it's clearer to have to valid bit in this struct instead of only checking offset/size/top. And now that we have valid bit it would be better to init the other fields and using less parameters for this function?

Regards,
-Jackie


_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux