On Fri, 09 Feb 2018 00:03:54 +0100, Jackie Li <yaodong...@intel.com> wrote:

Few more comments in addition to Joonas/Chris

GuC WOPCM registers are write-once registers. Current driver code accesses these registers without checking the accessibility to these registers which
will lead to unpredictable driver behaviors if these registers were touch
by other components (such as faulty BIOS code).

This patch moves the GuC WOPCM registers updating code into
intel_guc_wopcm.c and adds check before and after the update to GuC WOPCM
registers so that we can make sure the driver is in a known state before
and after writing to these write-once registers.

v6:
 - Made sure module reloading won't bug the kernel while doing
   locking status checking

v7:
 - Fixed patch format issues

v8:
 - Fixed coding style issue on register lock bit macro definition (Sagar)

v9:
 - Avoided to use redundant !! to cast uint to bool (Chris)
- Return error code instead of GEM_BUG_ON for locked with invalid register
   values case (Sagar)
 - Updated guc_wopcm_hw_init to use guc_wopcm as first parameter (Michal)
 - Added code to set and validate the HuC_LOADING_AGENT_GUC bit in GuC
   WOPCM offset register based on the presence of HuC firmware (Michal)
 - Use bit fields instead of macros for GuC WOPCM flags (Michal)

Cc: Michal Wajdeczko <michal.wajdec...@intel.com>
Cc: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
Signed-off-by: Jackie Li <yaodong...@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_reg.h   |   2 +
drivers/gpu/drm/i915/intel_guc_wopcm.c | 117 ++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_guc_wopcm.h |  12 +++-
 drivers/gpu/drm/i915/intel_uc.c        |   9 ++-
 4 files changed, 130 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_reg.h b/drivers/gpu/drm/i915/intel_guc_reg.h
index de4f78b..170d9cd 100644
--- a/drivers/gpu/drm/i915/intel_guc_reg.h
+++ b/drivers/gpu/drm/i915/intel_guc_reg.h
@@ -66,6 +66,8 @@
 #define   UOS_MOVE                       (1<<4)
 #define   START_DMA                      (1<<0)
 #define DMA_GUC_WOPCM_OFFSET           _MMIO(0xc340)
+#define   GUC_WOPCM_OFFSET_SHIFT       14
+#define   GUC_WOPCM_OFFSET_MASK                  (0x3ffff << 
GUC_WOPCM_OFFSET_SHIFT)
 #define   HUC_LOADING_AGENT_VCR                  (0<<1)
 #define   HUC_LOADING_AGENT_GUC                  (1<<1)
 #define GUC_MAX_IDLE_COUNT             _MMIO(0xC3E4)
diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.c b/drivers/gpu/drm/i915/intel_guc_wopcm.c
index 2e8e9ec..0af435a 100644
--- a/drivers/gpu/drm/i915/intel_guc_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c
@@ -90,6 +90,69 @@ static inline int guc_wopcm_size_check(struct intel_guc *guc, u32 huc_fw_size)
        return 0;
 }
+static inline bool __reg_locked(struct drm_i915_private *dev_priv,
+                               i915_reg_t reg)
+{
+       /* Set of bit-0 means this write-once register is locked. */
+       return I915_READ(reg) & BIT(0);
+}

Hmm, I'm still not happy as not all registers supports write-once bit.
What about something more generic/robust

static inline bool
__reg_test(struct drm_i915_private *dev_priv, i915_reg_t reg, u32 mask, u32 value)
{
        return (I915_READ(reg) & mask) == value;
}

static inline bool
__reg_is_set(struct drm_i915_private *dev_priv, i915_reg_t reg, u32 value)
{
        return __reg_test(dev_priv, reg, value, value);
}
...
#define WOPCM_OFFSET_VALID  (1<<0)
...
#define WOPCM_LOCKED        (1<<0)
...
locked = __reg_is_set(i915, GUC_WOPCM_SIZE, WOPCM_LOCKED);
locked = __reg_is_set(i915, DMA_GUC_WOPCM_OFFSET, WOPCM_OFFSET_VALID);

+
+static inline bool guc_wopcm_locked(struct intel_guc *guc)
+{
+       struct drm_i915_private *i915 = guc_to_i915(guc);
+       bool size_reg_locked = __reg_locked(i915, GUC_WOPCM_SIZE);
+       bool offset_reg_locked = __reg_locked(i915, DMA_GUC_WOPCM_OFFSET);
+
+       return size_reg_locked && offset_reg_locked;
+}
+
+static inline void guc_wopcm_hw_update(struct intel_guc *guc)
+{
+       struct drm_i915_private *dev_priv = guc_to_i915(guc);
+       u32 offset_reg_val;
+
+       /* GuC WOPCM registers should be unlocked at this point. */
+       GEM_BUG_ON(__reg_locked(dev_priv, GUC_WOPCM_SIZE));
+       GEM_BUG_ON(__reg_locked(dev_priv, DMA_GUC_WOPCM_OFFSET));

I'm not sure that GEM_BUG_ON can be used on bits that we don't
fully control

+
+       offset_reg_val = guc->wopcm.offset;
+       if (guc->wopcm.need_load_huc_fw)
+               offset_reg_val |= HUC_LOADING_AGENT_GUC;
+
+       I915_WRITE(GUC_WOPCM_SIZE, guc->wopcm.size);
+       I915_WRITE(DMA_GUC_WOPCM_OFFSET, offset_reg_val);
+
+       GEM_BUG_ON(!__reg_locked(dev_priv, GUC_WOPCM_SIZE));
+       GEM_BUG_ON(!__reg_locked(dev_priv, DMA_GUC_WOPCM_OFFSET));
+}
+
+static inline bool guc_wopcm_regs_valid(struct intel_guc *guc)

can't we always have intel_guc_wopcm as first param ?

+{
+       struct drm_i915_private *dev_priv = guc_to_i915(guc);
+       u32 size, offset;
+       bool guc_loads_huc;
+       u32 reg_val;
+
+       reg_val = I915_READ(GUC_WOPCM_SIZE);
+       /* GuC WOPCM size should be always multiple of 4K pages. */
+       size = reg_val & PAGE_MASK;
+
+       reg_val = I915_READ(DMA_GUC_WOPCM_OFFSET);
+       guc_loads_huc = reg_val & HUC_LOADING_AGENT_GUC;
+       offset = reg_val & GUC_WOPCM_OFFSET_MASK;
+
+       if (guc->wopcm.need_load_huc_fw && !guc_loads_huc)
+               return false;
+
+       return (size == guc->wopcm.size) && (offset == guc->wopcm.offset);
+}
+
+static inline
+struct intel_guc *guc_wopcm_to_guc(struct intel_guc_wopcm *guc_wopcm)
+{
+       return container_of(guc_wopcm, struct intel_guc, wopcm);
+}
+
 /**
  * intel_guc_wopcm_init() - Initialize the GuC WOPCM.
  * @guc_wopcm: intel_guc_wopcm..
@@ -108,12 +171,13 @@ static inline int guc_wopcm_size_check(struct intel_guc *guc, u32 huc_fw_size) int intel_guc_wopcm_init(struct intel_guc_wopcm *guc_wopcm, u32 guc_fw_size,
                         u32 huc_fw_size)
 {
-       struct intel_guc *guc =
-               container_of(guc_wopcm, struct intel_guc, wopcm);
+       struct intel_guc *guc = guc_wopcm_to_guc(guc_wopcm);
        u32 reserved = guc_wopcm_context_reserved_size(guc);
        u32 offset, size, top;
        int err;
+       GEM_BUG_ON(guc->wopcm.valid);
+
        if (!guc_fw_size)
                return -EINVAL;
@@ -147,6 +211,8 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm *guc_wopcm, u32 guc_fw_size,
        guc->wopcm.offset = offset;
        guc->wopcm.size = size;
        guc->wopcm.top = top;
+       /* Use GuC to load HuC firmware if HuC firmware is present. */
+       guc->wopcm.need_load_huc_fw = huc_fw_size ? 1 : 0;
        /* Check platform specific restrictions */
        err = guc_wopcm_size_check(guc, huc_fw_size);
@@ -160,3 +226,50 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm *guc_wopcm, u32 guc_fw_size,
        return 0;
 }
+
+/**
+ * intel_guc_wopcm_init_hw() - Setup GuC WOPCM registers.
+ * @guc_wopcm: intel_guc_wopcm.
+ *
+ * Setup the GuC WOPCM size and offset registers with the stored values. It will + * also check the registers locking status to determine whether these registers
+ * are unlocked and can be updated.
+ *
+ * Return: 0 on success. -EINVAL if registers were locked with incorrect values.
+ */
+int intel_guc_wopcm_init_hw(struct intel_guc_wopcm *guc_wopcm)
+{
+       struct intel_guc *guc = guc_wopcm_to_guc(guc_wopcm);
+       bool locked = guc_wopcm_locked(guc);
+
+       GEM_BUG_ON(!guc->wopcm.valid);
+
+       /*
+        * Bug if driver hasn't updated the HW Registers and GuC WOPCM has been
+        * locked. Return directly if WOPCM was locked and we have updated
+        * the registers.
+        */
+       if (locked) {
+               if (guc->wopcm.hw_updated)
+                       return 0;
+
+               /*
+                * Mark as updated if registers contained correct values.
+                * This will happen while reloading the driver module without
+                * rebooting the system.
+                */
+               if (guc_wopcm_regs_valid(guc))
+                       goto out;
+
+               /* Register locked without valid values. Abort HW init. */
+               return -EINVAL;

I'm not sure that EINVAL is correct error code here ... maybe EACCES ?

+       }
+
+       /* Always update registers when GuC WOPCM is not locked. */
+       guc_wopcm_hw_update(guc);
+
+out:
+       guc->wopcm.hw_updated = 1;
+
+       return 0;
+}
diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.h b/drivers/gpu/drm/i915/intel_guc_wopcm.h
index 1c5ffeb..471fb8e 100644
--- a/drivers/gpu/drm/i915/intel_guc_wopcm.h
+++ b/drivers/gpu/drm/i915/intel_guc_wopcm.h
@@ -86,7 +86,9 @@ struct intel_guc;
  * @offset: GuC WOPCM offset from the WOPCM base.
  * @size: size of GuC WOPCM for GuC firmware.
  * @top: start of the non-GuC WOPCM memory.
- * @valid: whether this structure contains valid (1-valid, 0-invalid) info.
+ * @valid: whether the values in this struct are valid.
+ * @hw_updated: GuC WOPCM registers has been updated with values in this struct. + * @need_load_huc_fw: whether need to configure GuC to load HuC firmware.
  *
* We simply use this structure to track the GuC use of WOPCM. The layout of * WOPCM would be defined by writing to GuC WOPCM offset and size registers.
@@ -95,7 +97,11 @@ struct intel_guc_wopcm {
        u32 offset;
        u32 size;
        u32 top;
-       u32 valid;
+
+       /* GuC WOPCM flags below. */
+       u32 valid:1;
+       u32 hw_updated:1;
+       u32 need_load_huc_fw:1;
 };
/**
@@ -114,5 +120,5 @@ static inline void intel_guc_wopcm_init_early(struct intel_guc_wopcm *guc_wopcm)
int intel_guc_wopcm_init(struct intel_guc_wopcm *guc_wopcm, u32 guc_size,
                         u32 huc_size);
-
+int intel_guc_wopcm_init_hw(struct intel_guc_wopcm *guc_wopcm);
 #endif
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index c842f36..8938096 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -343,14 +343,13 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
        GEM_BUG_ON(!HAS_GUC(dev_priv));
+       ret = intel_guc_wopcm_init_hw(&guc->wopcm);

Again, extra error path without single log message...

+       if (ret)
+               goto err_out;
+
        guc_disable_communication(guc);
        gen9_reset_guc_interrupts(dev_priv);
-       /* init WOPCM */
-       I915_WRITE(GUC_WOPCM_SIZE, guc->wopcm.size);
-       I915_WRITE(DMA_GUC_WOPCM_OFFSET,
-                  guc->wopcm.offset | HUC_LOADING_AGENT_GUC);
-
        /* WaEnableuKernelHeaderValidFix:skl */
        /* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
        if (IS_GEN9(dev_priv))

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

Reply via email to