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

2018-02-14 Thread Yaodong Li



On 02/13/2018 05:18 PM, Yaodong Li wrote:

On 02/13/2018 09:39 AM, Michal Wajdeczko wrote:


+
+static inline u32 lockable_reg_read(struct lockable_reg *lreg)
+{
+    struct drm_i915_private *dev_priv = guc_to_i915(lreg->guc);
+
+    lreg->reg_val = I915_READ(lreg->reg);
+
+    return lreg->reg_val;
+}
+
+static inline bool lockable_reg_validate(struct lockable_reg *lreg, 
u32 new_val)

+{
+    GEM_BUG_ON(!lreg->validate);
+
+    return lreg->validate(lreg, lreg->reg_val, new_val);
+}
+
+static inline bool lockable_reg_locked(struct lockable_reg *lreg)
+{
+    u32 reg_val = lockable_reg_read(lreg);
+
+    return reg_val & lreg->lock_bit;
+}
+
+static inline bool lockable_reg_locked_and_valid(struct 
lockable_reg *lreg,

+ u32 new_val)
+{
+    if (lockable_reg_locked(lreg)) {
+    if (lockable_reg_validate(lreg, new_val))
+    return true;
+
+    return false;
+    }
+
+    return false;
+}
+
+static inline bool lockable_reg_write(struct lockable_reg *lreg, 
u32 val)

+{
+    struct drm_i915_private *dev_priv = guc_to_i915(lreg->guc);
+
+    /*
+ * Write-once register was locked which may happen with either 
a faulty
+ * BIOS code or driver module reloading. We should still return 
success

+ * for the write if the register was locked with a valid value.
+ */
+    if (lockable_reg_locked(lreg)) {
+    if (lockable_reg_validate(lreg, val))
+    goto out;
+
+    DRM_DEBUG_DRIVER("Register %s was locked with invalid 
value\n",

+ lreg->name);
+
+    return false;
+    }
+
+    I915_WRITE(lreg->reg, val);
+
+    if (!lockable_reg_locked_and_valid(lreg, val)) {
+    DRM_DEBUG_DRIVER("Failed to lock Register %s\n", lreg->name);
+    return false;
+    }


As we acknowledge that there are scenarios where registers can be 
already

locked, do we really need to make our code so complex ? Maybe

int write_and_verify(struct drm_i915_private *dev_priv,
 i915_reg_t reg, u32 value, u32 locked_bit)
{
I915_WRITE(reg, value);

return I915_READ(reg) != (value | locked_bit) ? -EIO : 0;
}


Yes, I agree it's too complex at least for the validation part. Thanks!

My intention was trying to avoid extra write once we found the reg
was locked and to distinguish between faulty SW behavior and
hardware locking error? but now I feel it's not worth it.:-(

Sorry. I regret:-). I think it makes since to use my complex way
to validate the values because of the rsvd bits these regs.
a comparison I915_READ(reg) != (value | locked_bit) cannot
rule out the possibility that these regs were updated by faulty
software code (e.g. BIOS) with these rsvd bit set to random
values. and it's aways better to not make any assumption to
these rsvd bits. so I will still keep using the complex
way (explicitly clear this rsvd bit before comparison) to do this.
As for the lock_bit_check->write->verify sequence v.s. write->verify
sequence I think I can prefer to first one since it would provide
comprehensive info when any error occurs. so let's keep it?:-)

+
+
+#define DEFINE_LOCKABLE_REG(var, rg, lb, func)    \
+    struct lockable_reg var = {    \
+    .name = #rg,    \
+    .guc = guc_wopcm_to_guc(guc_wopcm),    \


btw, implicit macro params are evil...

Agree. but seems we always use similar approach in
I915_READ/WRITE().O:-)

I will avoid this implicit macro.

+    .reg = rg,    \
+    .reg_val = 0,    \
+    .lock_bit = lb,    \
+    .validate = func,    \


...and macro names should be always wrapped into ()


Thanks!

Will Add them.

Regards,
-Jackie


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


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

2018-02-14 Thread Yaodong Li



On 02/14/2018 09:07 AM, Michal Wajdeczko wrote:
On Wed, 14 Feb 2018 02:18:29 +0100, Yaodong Li  
wrote:



On 02/13/2018 09:39 AM, Michal Wajdeczko wrote:






+
+#define DEFINE_LOCKABLE_REG(var, rg, lb, func)    \
+    struct lockable_reg var = {    \
+    .name = #rg,    \
+    .guc = guc_wopcm_to_guc(guc_wopcm),    \


btw, implicit macro params are evil...

Agree. but seems we always use similar approach in
I915_READ/WRITE().O:-)


True, but the only reason why it is still not fixed is that
it requires changes in too many places ...

Sure. will avoid to use implicit params then:-) Thanks.





+ * @load_huc_fw: whether need to configure GuC to load HuC firmware.


I'm not sure that we need to track this flag inside structure.
It is just a parameter for doing partitioning and final check.


I think it's related to actual reg configuration. Any suggestions since
we do need it in hw_init to setup offset reg?


You can do partitioning at intel_wopcm level, but program hw at intel_uc
level when you know for sure if HuC is in use. Note that there is no 
wrong

in using results from WOPCM partitioning outside of intel_wopmc.


As mentioned before, we can avoid this flag and "valid" flag if do
partitioning and validation *before* writing final results to the
struct.
In current code, we do verify the ggtt offset against wopcm top in 
our current code which means
current code won't trust the fact that ggtt offset would never be 
used after uc/guc init failed.


But after failure in WOPCM partitioning, which should be done sooner than
uc init, we should abort driver load, so there shall be no access to 
ggtt.


I would like to have more discussion on intel_wopcm over 
intel_guc_wopcm. :-)
This is the reason for this valid bit (which clearly suggests the 
struct is ready to use) - I won't


I'm not sure that "valid" flag approach is correct - you should trust 
your
code path, otherwise you will have to add "valid" flags to every 
structure

and this is not a scalable solution ;)

True. I need some courage here:-). I would doubt myself after seeing 
guc_ggtt_offset
validates every offset against the wopcm->guc.top which is the only 
reason I kept using

this flag:-(

Regards,
-Jackie

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


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

2018-02-14 Thread Michal Wajdeczko
On Wed, 14 Feb 2018 02:18:29 +0100, Yaodong Li   
wrote:



On 02/13/2018 09:39 AM, Michal Wajdeczko wrote:






+
+#define DEFINE_LOCKABLE_REG(var, rg, lb, func)\
+struct lockable_reg var = {\
+.name = #rg,\
+.guc = guc_wopcm_to_guc(guc_wopcm),\


btw, implicit macro params are evil...

Agree. but seems we always use similar approach in
I915_READ/WRITE().O:-)


True, but the only reason why it is still not fixed is that
it requires changes in too many places ...




+ * @load_huc_fw: whether need to configure GuC to load HuC firmware.


I'm not sure that we need to track this flag inside structure.
It is just a parameter for doing partitioning and final check.


I think it's related to actual reg configuration. Any suggestions since
we do need it in hw_init to setup offset reg?


You can do partitioning at intel_wopcm level, but program hw at intel_uc
level when you know for sure if HuC is in use. Note that there is no wrong
in using results from WOPCM partitioning outside of intel_wopmc.


As mentioned before, we can avoid this flag and "valid" flag if do
partitioning and validation *before* writing final results to the
struct.
In current code, we do verify the ggtt offset against wopcm top in our  
current code which means
current code won't trust the fact that ggtt offset would never be used  
after uc/guc init failed.


But after failure in WOPCM partitioning, which should be done sooner than
uc init, we should abort driver load, so there shall be no access to ggtt.

This is the reason for this valid bit (which clearly suggests the struct  
is ready to use) - I won't


I'm not sure that "valid" flag approach is correct - you should trust your
code path, otherwise you will have to add "valid" flags to every structure
and this is not a scalable solution ;)

assume the ggtt_offset would never be called even if the uc/guc_init  
returned failure.



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


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

2018-02-13 Thread Yaodong Li

On 02/13/2018 09:39 AM, Michal Wajdeczko wrote:


+
+static inline u32 lockable_reg_read(struct lockable_reg *lreg)
+{
+    struct drm_i915_private *dev_priv = guc_to_i915(lreg->guc);
+
+    lreg->reg_val = I915_READ(lreg->reg);
+
+    return lreg->reg_val;
+}
+
+static inline bool lockable_reg_validate(struct lockable_reg *lreg, 
u32 new_val)

+{
+    GEM_BUG_ON(!lreg->validate);
+
+    return lreg->validate(lreg, lreg->reg_val, new_val);
+}
+
+static inline bool lockable_reg_locked(struct lockable_reg *lreg)
+{
+    u32 reg_val = lockable_reg_read(lreg);
+
+    return reg_val & lreg->lock_bit;
+}
+
+static inline bool lockable_reg_locked_and_valid(struct lockable_reg 
*lreg,

+ u32 new_val)
+{
+    if (lockable_reg_locked(lreg)) {
+    if (lockable_reg_validate(lreg, new_val))
+    return true;
+
+    return false;
+    }
+
+    return false;
+}
+
+static inline bool lockable_reg_write(struct lockable_reg *lreg, u32 
val)

+{
+    struct drm_i915_private *dev_priv = guc_to_i915(lreg->guc);
+
+    /*
+ * Write-once register was locked which may happen with either a 
faulty
+ * BIOS code or driver module reloading. We should still return 
success

+ * for the write if the register was locked with a valid value.
+ */
+    if (lockable_reg_locked(lreg)) {
+    if (lockable_reg_validate(lreg, val))
+    goto out;
+
+    DRM_DEBUG_DRIVER("Register %s was locked with invalid value\n",
+ lreg->name);
+
+    return false;
+    }
+
+    I915_WRITE(lreg->reg, val);
+
+    if (!lockable_reg_locked_and_valid(lreg, val)) {
+    DRM_DEBUG_DRIVER("Failed to lock Register %s\n", lreg->name);
+    return false;
+    }


As we acknowledge that there are scenarios where registers can be already
locked, do we really need to make our code so complex ? Maybe

int write_and_verify(struct drm_i915_private *dev_priv,
 i915_reg_t reg, u32 value, u32 locked_bit)
{
I915_WRITE(reg, value);

return I915_READ(reg) != (value | locked_bit) ? -EIO : 0;
}


Yes, I agree it's too complex at least for the validation part. Thanks!

My intention was trying to avoid extra write once we found the reg
was locked and to distinguish between faulty SW behavior and
hardware locking error? but now I feel it's not worth it.:-(

+
+
+#define DEFINE_LOCKABLE_REG(var, rg, lb, func)    \
+    struct lockable_reg var = {    \
+    .name = #rg,    \
+    .guc = guc_wopcm_to_guc(guc_wopcm),    \


btw, implicit macro params are evil...

Agree. but seems we always use similar approach in
I915_READ/WRITE().O:-)

+    .reg = rg,    \
+    .reg_val = 0,    \
+    .lock_bit = lb,    \
+    .validate = func,    \


...and macro names should be always wrapped into ()


Thanks!
diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.h 
b/drivers/gpu/drm/i915/intel_guc_wopcm.h

index 13fcab6..89dd44c 100644
--- a/drivers/gpu/drm/i915/intel_guc_wopcm.h
+++ b/drivers/gpu/drm/i915/intel_guc_wopcm.h
@@ -66,7 +66,8 @@ 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.
+ * @load_huc_fw: whether need to configure GuC to load HuC firmware.


I'm not sure that we need to track this flag inside structure.
It is just a parameter for doing partitioning and final check.


I think it's related to actual reg configuration. Any suggestions since
we do need it in hw_init to setup offset reg?

As mentioned before, we can avoid this flag and "valid" flag if do
partitioning and validation *before* writing final results to the
struct.
In current code, we do verify the ggtt offset against wopcm top in our 
current code which means
current code won't trust the fact that ggtt offset would never be used 
after uc/guc init failed.
This is the reason for this valid bit (which clearly suggests the struct 
is ready to use) - I won't
assume the ggtt_offset would never be called even if the uc/guc_init 
returned failure.



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


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

2018-02-13 Thread Michal Wajdeczko

On Tue, 13 Feb 2018 00:45:52 +0100, Jackie Li  wrote:

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)

v10:
 - Refined variable names, removed reduntant comments (Joonas)
 - Introduced lockable_reg to handle the write once register write and
   propagate the write error to caller (Joonas)
 - Used lockable_reg abstraction to avoid locking bit check on generic
   i915_reg_t (Michal)
 - Added log message for error paths (Michal)
 - Removed hw_updated flag and only relies on real hardware status

Cc: Michal Wajdeczko 
Cc: Chris Wilson 
Cc: Joonas Lahtinen 
Signed-off-by: Jackie Li 
---
 drivers/gpu/drm/i915/intel_guc_reg.h   |   2 +
 drivers/gpu/drm/i915/intel_guc_wopcm.c | 151  
-

 drivers/gpu/drm/i915/intel_guc_wopcm.h |  10 ++-
 drivers/gpu/drm/i915/intel_uc.c|   9 +-
 4 files changed, 162 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_reg.h  
b/drivers/gpu/drm/i915/intel_guc_reg.h

index c482df5..a550078 100644
--- a/drivers/gpu/drm/i915/intel_guc_reg.h
+++ b/drivers/gpu/drm/i915/intel_guc_reg.h
@@ -68,6 +68,8 @@
 #define DMA_GUC_WOPCM_OFFSET   _MMIO(0xc340)
 #define   HUC_LOADING_AGENT_VCR  (0<<1)
 #define   HUC_LOADING_AGENT_GUC  (1<<1)
+#define   GUC_WOPCM_OFFSET_SHIFT   14
+#define   GUC_WOPCM_OFFSET_MASK  (0x3 << 
GUC_WOPCM_OFFSET_SHIFT)
 #define GUC_MAX_IDLE_COUNT _MMIO(0xC3E4)
#define HUC_STATUS2 _MMIO(0xD3B0)
diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.c  
b/drivers/gpu/drm/i915/intel_guc_wopcm.c

index 0194266..4043798 100644
--- a/drivers/gpu/drm/i915/intel_guc_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c
@@ -83,6 +83,12 @@ void intel_guc_wopcm_init_early(struct  
intel_guc_wopcm *guc_wopcm)

guc_wopcm->top = WOPCM_DEFAULT_SIZE;
 }
+static inline
+struct intel_guc *guc_wopcm_to_guc(struct intel_guc_wopcm *guc_wopcm)
+{
+   return container_of(guc_wopcm, struct intel_guc, wopcm);
+}
+


This can be moved to patch 3/7


 /**
  * intel_guc_wopcm_init() - Initialize the GuC WOPCM.
  * @guc_wopcm: pointer to intel_guc_wopcm.
@@ -101,13 +107,13 @@ 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_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 = context_reserved_size(guc);
u32 offset, size, top;
int err;
GEM_BUG_ON(!guc_fw_size);
+   GEM_BUG_ON(guc->wopcm.valid);
offset = huc_fw_size + WOPCM_RESERVED_SIZE;
@@ -138,6 +144,7 @@ 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;
+   guc->wopcm.load_huc_fw = huc_fw_size > 0;
err = guc_wopcm_size_check(guc, huc_fw_size);
if (err) {
@@ -152,3 +159,143 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm  
*guc_wopcm, u32 guc_fw_size,

return 0;
 }
+
+struct lockable_reg {
+   const char *name;
+   struct intel_guc *guc;
+   i915_reg_t reg;
+   u32 reg_val;
+   u32 lock_bit;
+   bool (*validate)(struct lockable_reg *lreg, u32 reg_val, u32 new_val);
+};



Feels like a bit over engineering in this way.



+
+static inline u32 lockable_reg_read(struct lockable_reg *lreg)
+{
+   struct drm_i915_private *dev_priv = guc_to_i915(lreg->guc);
+
+   lreg->reg_val = I915_READ(lreg->reg);
+
+   return lreg->reg_val;
+}
+
+static inline 

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

2018-02-12 Thread Jackie Li
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)

v10:
 - Refined variable names, removed reduntant comments (Joonas)
 - Introduced lockable_reg to handle the write once register write and
   propagate the write error to caller (Joonas)
 - Used lockable_reg abstraction to avoid locking bit check on generic
   i915_reg_t (Michal)
 - Added log message for error paths (Michal)
 - Removed hw_updated flag and only relies on real hardware status

Cc: Michal Wajdeczko 
Cc: Chris Wilson 
Cc: Joonas Lahtinen 
Signed-off-by: Jackie Li 
---
 drivers/gpu/drm/i915/intel_guc_reg.h   |   2 +
 drivers/gpu/drm/i915/intel_guc_wopcm.c | 151 -
 drivers/gpu/drm/i915/intel_guc_wopcm.h |  10 ++-
 drivers/gpu/drm/i915/intel_uc.c|   9 +-
 4 files changed, 162 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_reg.h 
b/drivers/gpu/drm/i915/intel_guc_reg.h
index c482df5..a550078 100644
--- a/drivers/gpu/drm/i915/intel_guc_reg.h
+++ b/drivers/gpu/drm/i915/intel_guc_reg.h
@@ -68,6 +68,8 @@
 #define DMA_GUC_WOPCM_OFFSET   _MMIO(0xc340)
 #define   HUC_LOADING_AGENT_VCR  (0<<1)
 #define   HUC_LOADING_AGENT_GUC  (1<<1)
+#define   GUC_WOPCM_OFFSET_SHIFT   14
+#define   GUC_WOPCM_OFFSET_MASK  (0x3 << 
GUC_WOPCM_OFFSET_SHIFT)
 #define GUC_MAX_IDLE_COUNT _MMIO(0xC3E4)
 
 #define HUC_STATUS2 _MMIO(0xD3B0)
diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.c 
b/drivers/gpu/drm/i915/intel_guc_wopcm.c
index 0194266..4043798 100644
--- a/drivers/gpu/drm/i915/intel_guc_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c
@@ -83,6 +83,12 @@ void intel_guc_wopcm_init_early(struct intel_guc_wopcm 
*guc_wopcm)
guc_wopcm->top = WOPCM_DEFAULT_SIZE;
 }
 
+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: pointer to intel_guc_wopcm.
@@ -101,13 +107,13 @@ 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_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 = context_reserved_size(guc);
u32 offset, size, top;
int err;
 
GEM_BUG_ON(!guc_fw_size);
+   GEM_BUG_ON(guc->wopcm.valid);
 
offset = huc_fw_size + WOPCM_RESERVED_SIZE;
 
@@ -138,6 +144,7 @@ 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;
+   guc->wopcm.load_huc_fw = huc_fw_size > 0;
 
err = guc_wopcm_size_check(guc, huc_fw_size);
if (err) {
@@ -152,3 +159,143 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm 
*guc_wopcm, u32 guc_fw_size,
 
return 0;
 }
+
+struct lockable_reg {
+   const char *name;
+   struct intel_guc *guc;
+   i915_reg_t reg;
+   u32 reg_val;
+   u32 lock_bit;
+   bool (*validate)(struct lockable_reg *lreg, u32 reg_val, u32 new_val);
+};
+
+static inline u32 lockable_reg_read(struct lockable_reg *lreg)
+{
+   struct drm_i915_private *dev_priv = guc_to_i915(lreg->guc);
+
+   lreg->reg_val = I915_READ(lreg->reg);
+
+   return lreg->reg_val;
+}
+
+static inline bool lockable_reg_validate(struct lockable_reg *lreg, u32 
new_val)
+{
+   GEM_BUG_ON(!lreg->validate);
+
+   return lreg->validate(lreg, lreg->reg_val, new_val);
+}
+
+static