On Tue, 06 Feb 2018 07:39:27 +0100, Sagar Arun Kamble
<sagar.a.kamble@xxxxxxxxx> wrote:
On 2/6/2018 5:32 AM, Jackie Li wrote:
GuC WOPCM registers are write-once registers. Current driver code
accesses
these registers without checking the accessibility to these registers,
this
will lead unpredictable driver behaviors if these registers were touch
by
other components (such as faulty BIOS code).
This patch moves the GuC WOPCM register updating operations into
intel_guc_wopcm.c and adds checks before and after the write to GuC
WOPCM
registers to 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)
Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Signed-off-by: Jackie Li <yaodong.li@xxxxxxxxx>
---
drivers/gpu/drm/i915/intel_guc.h | 2 +-
drivers/gpu/drm/i915/intel_guc_reg.h | 2 +
drivers/gpu/drm/i915/intel_guc_wopcm.c | 93
++++++++++++++++++++++++++++++++--
drivers/gpu/drm/i915/intel_guc_wopcm.h | 9 +++-
drivers/gpu/drm/i915/intel_uc.c | 5 +-
5 files changed, 101 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_guc.h
b/drivers/gpu/drm/i915/intel_guc.h
index 06f315e..cb1703b 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -122,7 +122,7 @@ static inline u32 intel_guc_ggtt_offset(struct
intel_guc *guc,
{
u32 offset = i915_ggtt_offset(vma);
- GEM_BUG_ON(!guc->wopcm.valid);
+ GEM_BUG_ON(!(guc->wopcm.flags & INTEL_GUC_WOPCM_VALID));
GEM_BUG_ON(offset < guc->wopcm.top);
GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
diff --git a/drivers/gpu/drm/i915/intel_guc_reg.h
b/drivers/gpu/drm/i915/intel_guc_reg.h
index de4f78b..18cb2ef 100644
--- a/drivers/gpu/drm/i915/intel_guc_reg.h
+++ b/drivers/gpu/drm/i915/intel_guc_reg.h
@@ -66,6 +66,7 @@
#define UOS_MOVE (1<<4)
#define START_DMA (1<<0)
#define DMA_GUC_WOPCM_OFFSET _MMIO(0xc340)
+#define DMA_GUC_WOPCM_OFFSET_MASK (0xffffc000)
please define _SHIFT and use it in _MASK definition
#define HUC_LOADING_AGENT_VCR (0<<1)
#define HUC_LOADING_AGENT_GUC (1<<1)
#define GUC_MAX_IDLE_COUNT _MMIO(0xC3E4)
@@ -75,6 +76,7 @@
/* Defines WOPCM space available to GuC firmware */
#define GUC_WOPCM_SIZE _MMIO(0xc050)
+#define GUC_WOPCM_REG_LOCKED BIT(0)
use (1 << 0) instead
#define GEN8_GT_PM_CONFIG _MMIO(0x138140)
#define GEN9LP_GT_PM_CONFIG _MMIO(0x138140)
diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.c
b/drivers/gpu/drm/i915/intel_guc_wopcm.c
index 3cba9ac..8317d9e 100644
--- a/drivers/gpu/drm/i915/intel_guc_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c
@@ -89,6 +89,55 @@ static inline int
guc_wopcm_check_hw_restrictions(struct intel_guc *guc)
return 0;
}
+static inline bool __reg_locked(struct drm_i915_private *dev_priv,
+ i915_reg_t reg)
+{
+ /* Explicitly cast the return value to bool. */
+ return !!(I915_READ(reg) & GUC_WOPCM_REG_LOCKED);
you should avoid reusing bits defined for one register in others
+}
+
+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);
+
+ /* 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));
+
+ I915_WRITE(GUC_WOPCM_SIZE, guc->wopcm.size);
+ I915_WRITE(DMA_GUC_WOPCM_OFFSET,
+ guc->wopcm.offset | HUC_LOADING_AGENT_GUC);
+
+ 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)
+{
+ struct drm_i915_private *dev_priv = guc_to_i915(guc);
+ u32 size, offset;
+ bool guc_loads_huc;
+
+ /* GuC WOPCM size should be always multiple of 4K pages. */
+ size = I915_READ(GUC_WOPCM_SIZE) & PAGE_MASK;
+ offset = I915_READ(DMA_GUC_WOPCM_OFFSET);
+
+ guc_loads_huc = !!(offset & HUC_LOADING_AGENT_GUC);
please follow Chris suggestions to avoid unnecessary explicit casts
+ offset &= DMA_GUC_WOPCM_OFFSET_MASK;
maybe like this for easier reading:
u32 reg;
reg = I915_READ(GUC_WOPCM_SIZE);
size = reg & PAGE_MASK;
reg = I915_READ(DMA_GUC_WOPCM_OFFSET);
offset = reg & DMA_GUC_WOPCM_OFFSET_MASK;
guc_loads_huc = reg & HUC_LOADING_AGENT_GUC;
+
+ return guc_loads_huc && (size == guc->wopcm.size) &&
what if we run in !USES_HUC() mode?
+ (offset == guc->wopcm.offset);
+}
+
/**
* intel_guc_wopcm_init() - Initialize the GuC WOPCM..
* @guc: intel guc.
@@ -111,8 +160,7 @@ int intel_guc_wopcm_init(struct intel_guc *guc, u32
guc_fw_size,
u32 offset, size, top;
int err;
- if (guc->wopcm.valid)
- return 0;
+ GEM_BUG_ON(guc->wopcm.flags & INTEL_GUC_WOPCM_VALID);
if (!guc_fw_size)
return -EINVAL;
@@ -153,10 +201,49 @@ int intel_guc_wopcm_init(struct intel_guc *guc,
u32 guc_fw_size,
if (err)
return err;
- guc->wopcm.valid = 1;
+ guc->wopcm.flags |= INTEL_GUC_WOPCM_VALID;
DRM_DEBUG_DRIVER("GuC WOPCM offset %dKB, size %dKB, top %dKB\n",
offset >> 10, size >> 10, top >> 10);
return 0;
}
+
+/**
+ * intel_guc_wopcm_init_hw() - Setup GuC WOPCM registers.
+ * @guc: intel guc.
+ *
+ * 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.
+ */
+void intel_guc_wopcm_init_hw(struct intel_guc *guc)
+{
+ bool locked = guc_wopcm_locked(guc);
+
+ GEM_BUG_ON(!(guc->wopcm.flags & INTEL_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) {
+ /*
+ * 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;
+
+ GEM_BUG_ON(!(guc->wopcm.flags & INTEL_GUC_WOPCM_HW_UPDATED));
We should not go ahead with uc_init_hw in this case so returning error
from here or
checking wopcm.flags to abort uc_init_hw is needed with debug message.
+ return;
+ }
+
+ /* Always update registers when GuC WOPCM is not locked. */
+ guc_wopcm_hw_update(guc);
+
+out:
+ guc->wopcm.flags |= INTEL_GUC_WOPCM_HW_UPDATED;
+}
diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.h
b/drivers/gpu/drm/i915/intel_guc_wopcm.h
index 8c71d20a..627f7fa 100644
--- a/drivers/gpu/drm/i915/intel_guc_wopcm.h
+++ b/drivers/gpu/drm/i915/intel_guc_wopcm.h
@@ -52,12 +52,16 @@ struct intel_guc;
*/
#define GEN9_GUC_WOPCM_OFFSET (0x24000)
+/* GuC WOPCM flags */
+#define INTEL_GUC_WOPCM_VALID BIT(0)
+#define INTEL_GUC_WOPCM_HW_UPDATED BIT(1)
maybe just
bool valid:1;
bool hw_updated:1;
+
/**
* intel_guc_wopcm - GuC WOPCM related settings.
* @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.
+ * @flags: bitmap of INTEL_GUC_WOPCM_<foo>.
*
* 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 as
@@ -75,7 +79,7 @@ struct intel_guc_wopcm {
u32 offset;
u32 size;
u32 top;
- u32 valid;
+ u32 flags;
};
/**
@@ -93,5 +97,6 @@ static inline void intel_guc_wopcm_init_early(struct
intel_guc_wopcm *wopcm)
}
int intel_guc_wopcm_init(struct intel_guc *guc, u32 guc_size, u32
huc_size);
+void intel_guc_wopcm_init_hw(struct intel_guc *guc);
#endif
diff --git a/drivers/gpu/drm/i915/intel_uc.c
b/drivers/gpu/drm/i915/intel_uc.c
index 2292f31..62e85b5 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -346,10 +346,7 @@ int intel_uc_init_hw(struct drm_i915_private
*dev_priv)
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);
+ intel_guc_wopcm_init_hw(guc);
/* WaEnableuKernelHeaderValidFix:skl */
/* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
