Re: [Intel-gfx] [PATCH v8 5/6] drm/i915/guc: Check the locking status of GuC WOPCM registers

2018-02-07 Thread Yaodong Li 

On 02/07/2018 09:24 AM, Michal Wajdeczko wrote:
On Wed, 07 Feb 2018 05:02:00 +0100, Yaodong Li  
wrote:





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


+    /* 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


it's a common bit. use BIT(0) instead?


How common?
Note that your function accepts all registers.
Btw, even for these two registers used below bit0 has different
name definitions (Locked vs Offset Valid) which we should use.


this bit suggests the w-o registers were locked, right? (I mean
no matter how we call this bit).




+    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;

hmm, this will clear the HUC_LOADING_AGENT_GUC.


that's expected

the values here are expected to be the same as the values
set to registers. otherwise, we need update the registers.
in the case of !USES_HUC() if we would add the change
to set HUC_LOADING_AGENT bit accordingly. we still
need such a check, right?



guc_loads_huc = reg & HUC_LOADING_AGENT_GUC;


+
+    return guc_loads_huc && (size == guc->wopcm.size) &&


what if we run in !USES_HUC() mode?


Do you mean the HUC_LOADING_AGENT bit?

this patch series is trying to align with the current driver logic. 
this bit

is current set regardless USES_HUC()

Are you suggest we should not set this bit for !USE_HUC() mode?


We can set it as before, but when we compare already initialized
registers we can ignore this bit if we're running without HuC.
It must match only if we use HuC.


I can add these changes.

+    (offset == guc->wopcm.offset);
  #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;

I was trying to avoid bool in struct (really struggling with it 
actual size), maybe

u32 valid:1;
u32 hw_updated:1


bool:1 will work the same, try it !


I sure it's going to work:-) Just obsessed with the ideas that:
0) bool (_Bool) could be any length, but it's fine for us since we know 
the compiler and hw.
1) I cannot see any benefits for using bits definition for each flag for 
two reasons:

 a) it's won't provide any performance/code readability improvement.
 b) the struct definition would grow bigger visually and we need 
explain these are flags and
 come back to modify the struct every time we need to add more 
flags.
 One more reason:-) which may not apply here, but the beauty of use 
of flags is
     we can do flags |= (flag 1 | flag 2) once instead of flags.flag1 = 
1; flags.flag2 = 1;


So let keep it;-)? (please do let me know if I've made any nonsense 
assumptions again just like we

need explicitly cast int to bool with !! even the compiler already did so)


Regards,
-Jackie



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

Re: [Intel-gfx] [PATCH v8 5/6] drm/i915/guc: Check the locking status of GuC WOPCM registers

2018-02-07 Thread Michal Wajdeczko 
On Wed, 07 Feb 2018 05:02:00 +0100, Yaodong Li   
wrote:





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


+/* 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


it's a common bit. use BIT(0) instead?


How common?
Note that your function accepts all registers.
Btw, even for these two registers used below bit0 has different
name definitions (Locked vs Offset Valid) which we should use.





+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;

hmm, this will clear the HUC_LOADING_AGENT_GUC.


that's expected


guc_loads_huc = reg & HUC_LOADING_AGENT_GUC;


+
+return guc_loads_huc && (size == guc->wopcm.size) &&


what if we run in !USES_HUC() mode?


Do you mean the HUC_LOADING_AGENT bit?

this patch series is trying to align with the current driver logic. this  
bit

is current set regardless USES_HUC()

Are you suggest we should not set this bit for !USE_HUC() mode?


We can set it as before, but when we compare already initialized
registers we can ignore this bit if we're running without HuC.
It must match only if we use HuC.


+(offset == guc->wopcm.offset);
  #define GEN9_GUC_WOPCM_OFFSET(0x24000)
  +/* GuC WOPCM flags */
+#define INTEL_GUC_WOPCM_VALIDBIT(0)
+#define INTEL_GUC_WOPCM_HW_UPDATEDBIT(1)


maybe just

bool valid:1;
bool hw_updated:1;

I was trying to avoid bool in struct (really struggling with it actual  
size), maybe

u32 valid:1;
u32 hw_updated:1


bool:1 will work the same, try it !



Regards,
-Jackie


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

Re: [Intel-gfx] [PATCH v8 5/6] drm/i915/guc: Check the locking status of GuC WOPCM registers

2018-02-06 Thread Yaodong Li 



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


+    /* 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


it's a common bit. use BIT(0) instead?




+    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;

hmm, this will clear the HUC_LOADING_AGENT_GUC.

guc_loads_huc = reg & HUC_LOADING_AGENT_GUC;


+
+    return guc_loads_huc && (size == guc->wopcm.size) &&


what if we run in !USES_HUC() mode?


Do you mean the HUC_LOADING_AGENT bit?

this patch series is trying to align with the current driver logic. this bit
is current set regardless USES_HUC()

Are you suggest we should not set this bit for !USE_HUC() mode?

+    (offset == guc->wopcm.offset);
  #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;

I was trying to avoid bool in struct (really struggling with it actual 
size), maybe

u32 valid:1;
u32 hw_updated:1

Regards,
-Jackie


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

Re: [Intel-gfx] [PATCH v8 5/6] drm/i915/guc: Check the locking status of GuC WOPCM registers

2018-02-06 Thread Yaodong Li 



On 02/05/2018 10:39 PM, Sagar Arun Kamble wrote:

+/**
+ * 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.

In a second thought, I think we need do more work here to check whether the
locked values are sufficient for current fw sizes. if yes then return 
success else

fail the wopcm init.
I agree we only check the status and return the control to uc_init_hw.

Thanks & Regards,
-Jackie

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

Re: [Intel-gfx] [PATCH v8 5/6] drm/i915/guc: Check the locking status of GuC WOPCM registers

2018-02-06 Thread Michal Wajdeczko 
On Tue, 06 Feb 2018 07:39:27 +0100, Sagar Arun Kamble  
 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 
Cc: Chris Wilson 
Cc: Joonas Lahtinen 
Signed-off-by: Jackie Li 
---
  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  (0xc000)


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

Re: [Intel-gfx] [PATCH v8 5/6] drm/i915/guc: Check the locking status of GuC WOPCM registers

2018-02-06 Thread Chris Wilson 
Quoting Jackie Li (2018-02-06 00:02:41)
> +static inline bool __reg_locked(struct drm_i915_private *dev_priv,
> +   i915_reg_t reg)
> +{
> +   /* Explicitly cast the return value to bool. */

You already did by using bool.

> +   return !!(I915_READ(reg) & GUC_WOPCM_REG_LOCKED);

i.e. this is equivalent as defined by the C spec to
return I915_READ(reg) & GUC_WOPCM_REG_LOCKED;
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v8 5/6] drm/i915/guc: Check the locking status of GuC WOPCM registers

2018-02-05 Thread Sagar Arun Kamble 



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 
Cc: Chris Wilson 
Cc: Joonas Lahtinen 
Signed-off-by: Jackie Li 
---
  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  (0xc000)
  #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)
  
  #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);
+}
+
+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);
+   offset &= DMA_GUC_WOPCM_OFFSET_MASK;
+
+   return guc_loads_huc && (size == guc->wopcm.size) &&
+   (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