On Wed, 07 Feb 2018 05:51:56 +0100, Yaodong Li <yaodong...@intel.com>
wrote:
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.
but you can still access i915 from wopcm using container_of trick.
+{
+ 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)
so maybe part of these code movement was unnecessary, and affected
code can be replaced directly in this patch ?
+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.?
Why?
Note that we control what guc_reserved_wopcm_size() will return
so this check is just for us, we should never fail here.
+
+ 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
But if check_hw_restrictions() fails we will return error and we should
never use this struct, so this 'valid' bit now seems more redundant.
it would be better to init the other fields and using less parameters
for this function?
If I recall correctly x86-64 can pass 6 function parameters in registers.
Michal
Regards,
-Jackie
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
