Re: [Intel-gfx] [PATCH v3 3/4] drm/i915: Add code to accept valid locked WOPCM register values

2018-04-19 Thread Yaodong Li 

On 04/19/2018 09:37 AM, Michal Wajdeczko wrote:
On Mon, 16 Apr 2018 20:43:39 +0200, Yaodong Li  
wrote:



On 04/13/2018 09:20 PM, Michal Wajdeczko wrote:
On Tue, 10 Apr 2018 02:42:19 +0200, Jackie Li  
wrote:


In current code, we only compare the locked WOPCM register values 
with the
calculated values. However, we can continue loading GuC/HuC 
firmware if the
locked (or partially locked) values were valid for current GuC/HuC 
firmware

sizes.

This patch added a new code path to verify whether the locked register
values can be used for GuC/HuC firmware loading, it will 
recalculate the
verify the new values if these registers were partially locked, so 
that we
won't fail the GuC/HuC firmware loading even if the locked register 
values

are different from the calculated ones.

v2:
 - Update WOPCM register only if it's not locked

Signed-off-by: Jackie Li 
Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Michal Winiarski 
Cc: John Spotswood 
Cc: Joonas Lahtinen 
---
 drivers/gpu/drm/i915/intel_wopcm.c | 217 
+++--

 1 file changed, 185 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_wopcm.c 
b/drivers/gpu/drm/i915/intel_wopcm.c

index b1c08ca..fa8d2be 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -140,6 +140,53 @@ static inline int check_hw_restriction(struct 
drm_i915_private *i915,

 return err;
 }
+static inline u32 calculate_min_guc_wopcm_base(u32 huc_fw_size)
+{
+    return ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
+ GUC_WOPCM_OFFSET_ALIGNMENT);
+}
+
+static inline u32 calculate_min_guc_wopcm_size(u32 guc_fw_size)
+{
+    return guc_fw_size + GUC_WOPCM_RESERVED + 
GUC_WOPCM_STACK_RESERVED;

+}
+
+static inline int calculate_max_guc_wopcm_size(struct intel_wopcm 
*wopcm,

+   u32 guc_wopcm_base,
+   u32 *guc_wopcm_size)


Can't we just return this size as positive value?
I guess wopcm size will never be larger than MAX_INT.
We can add GEM_BUG_ON to be sure.


+{
+    struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
+    u32 ctx_rsvd = context_reserved_size(i915);
+
+    if (guc_wopcm_base >= wopcm->size ||
+    (guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
+    DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
+  guc_wopcm_base / 1024);
+    return -E2BIG;


You are mixing calculations with verifications here.
Focus on calculations as you have separate function that verifies 
values.



+    }
+
+    *guc_wopcm_size =
+    (wopcm->size - guc_wopcm_base - ctx_rsvd) & 
GUC_WOPCM_SIZE_MASK;

+
+    return 0;
+}
+
+static inline int verify_calculated_values(struct drm_i915_private


hmm, maybe we can unify somehow this verification to be able to work 
with

both calculated and locked values?
I actually thought about that. However, since the verification based 
on the assumption
that the unlocked reg value could be any numbers so it puts more 
checks on these values

which are unnecessary during the calculation.


maybe some checks would be "unnecessary" but they still should be valid.
and code reuse is nice benefit that sacrifice that few extra checks.
Hmm, actually there's no duplicated code here. But I agree that it will 
be clearer to
have single place for the verification - it makes sense too:). So will 
choose to

sacrifice a little bit time here for unnecessary checks.



I've made the responding check common
enough to be reused by this verification code and the calculation code.



*i915,
+   u32 guc_fw_size, u32 huc_fw_size,
+   u32 guc_wopcm_base,
+   u32 guc_wopcm_size)
+{
+    if (guc_wopcm_size < calculate_min_guc_wopcm_size(guc_fw_size)) {
+    DRM_ERROR("Need %uKiB WOPCM for GuC FW, %uKiB available.\n",
+  calculate_min_guc_wopcm_size(guc_fw_size),


you are calling calculate_min_guc_wopcm_size() twice

Will update it.



+  guc_wopcm_size / 1024);
+    return -E2BIG;
+    }
+
+    return check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size,
+    huc_fw_size);
+}
+
 /**
  * intel_wopcm_init() - Initialize the WOPCM structure.
  * @wopcm: pointer to intel_wopcm.
@@ -157,10 +204,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
 struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
 u32 guc_fw_size = intel_uc_fw_get_upload_size(>guc.fw);
 u32 huc_fw_size = intel_uc_fw_get_upload_size(>huc.fw);
-    u32 ctx_rsvd = context_reserved_size(i915);
 u32 guc_wopcm_base;
 u32 guc_wopcm_size;
-    u32 guc_wopcm_rsvd;
 int err;
    GEM_BUG_ON(!wopcm->size);
@@ -177,35 +222,121 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
 return -E2BIG;
 }
-   

Re: [Intel-gfx] [PATCH v3 3/4] drm/i915: Add code to accept valid locked WOPCM register values

2018-04-19 Thread Michal Wajdeczko 
On Mon, 16 Apr 2018 20:43:39 +0200, Yaodong Li   
wrote:



On 04/13/2018 09:20 PM, Michal Wajdeczko wrote:
On Tue, 10 Apr 2018 02:42:19 +0200, Jackie Li   
wrote:


In current code, we only compare the locked WOPCM register values with  
the
calculated values. However, we can continue loading GuC/HuC firmware  
if the
locked (or partially locked) values were valid for current GuC/HuC  
firmware

sizes.

This patch added a new code path to verify whether the locked register
values can be used for GuC/HuC firmware loading, it will recalculate  
the
verify the new values if these registers were partially locked, so  
that we
won't fail the GuC/HuC firmware loading even if the locked register  
values

are different from the calculated ones.

v2:
 - Update WOPCM register only if it's not locked

Signed-off-by: Jackie Li 
Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Michal Winiarski 
Cc: John Spotswood 
Cc: Joonas Lahtinen 
---
 drivers/gpu/drm/i915/intel_wopcm.c | 217  
+++--

 1 file changed, 185 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_wopcm.c  
b/drivers/gpu/drm/i915/intel_wopcm.c

index b1c08ca..fa8d2be 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -140,6 +140,53 @@ static inline int check_hw_restriction(struct  
drm_i915_private *i915,

 return err;
 }
+static inline u32 calculate_min_guc_wopcm_base(u32 huc_fw_size)
+{
+return ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
+ GUC_WOPCM_OFFSET_ALIGNMENT);
+}
+
+static inline u32 calculate_min_guc_wopcm_size(u32 guc_fw_size)
+{
+return guc_fw_size + GUC_WOPCM_RESERVED +  
GUC_WOPCM_STACK_RESERVED;

+}
+
+static inline int calculate_max_guc_wopcm_size(struct intel_wopcm  
*wopcm,

+   u32 guc_wopcm_base,
+   u32 *guc_wopcm_size)


Can't we just return this size as positive value?
I guess wopcm size will never be larger than MAX_INT.
We can add GEM_BUG_ON to be sure.


+{
+struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
+u32 ctx_rsvd = context_reserved_size(i915);
+
+if (guc_wopcm_base >= wopcm->size ||
+(guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
+DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
+  guc_wopcm_base / 1024);
+return -E2BIG;


You are mixing calculations with verifications here.
Focus on calculations as you have separate function that verifies  
values.



+}
+
+*guc_wopcm_size =
+(wopcm->size - guc_wopcm_base - ctx_rsvd) &  
GUC_WOPCM_SIZE_MASK;

+
+return 0;
+}
+
+static inline int verify_calculated_values(struct drm_i915_private


hmm, maybe we can unify somehow this verification to be able to work  
with

both calculated and locked values?
I actually thought about that. However, since the verification based on  
the assumption
that the unlocked reg value could be any numbers so it puts more checks  
on these values

which are unnecessary during the calculation.


maybe some checks would be "unnecessary" but they still should be valid.
and code reuse is nice benefit that sacrifice that few extra checks.


I've made the responding check common
enough to be reused by this verification code and the calculation code.



*i915,
+   u32 guc_fw_size, u32 huc_fw_size,
+   u32 guc_wopcm_base,
+   u32 guc_wopcm_size)
+{
+if (guc_wopcm_size < calculate_min_guc_wopcm_size(guc_fw_size)) {
+DRM_ERROR("Need %uKiB WOPCM for GuC FW, %uKiB available.\n",
+  calculate_min_guc_wopcm_size(guc_fw_size),


you are calling calculate_min_guc_wopcm_size() twice

Will update it.



+  guc_wopcm_size / 1024);
+return -E2BIG;
+}
+
+return check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size,
+huc_fw_size);
+}
+
 /**
  * intel_wopcm_init() - Initialize the WOPCM structure.
  * @wopcm: pointer to intel_wopcm.
@@ -157,10 +204,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
 struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
 u32 guc_fw_size = intel_uc_fw_get_upload_size(>guc.fw);
 u32 huc_fw_size = intel_uc_fw_get_upload_size(>huc.fw);
-u32 ctx_rsvd = context_reserved_size(i915);
 u32 guc_wopcm_base;
 u32 guc_wopcm_size;
-u32 guc_wopcm_rsvd;
 int err;
GEM_BUG_ON(!wopcm->size);
@@ -177,35 +222,121 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
 return -E2BIG;
 }
-guc_wopcm_base = ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
-   GUC_WOPCM_OFFSET_ALIGNMENT);
-if ((guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
-DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
-  guc_wopcm_base / 1024);
+

Re: [Intel-gfx] [PATCH v3 3/4] drm/i915: Add code to accept valid locked WOPCM register values

2018-04-16 Thread Yaodong Li 

On 04/13/2018 09:20 PM, Michal Wajdeczko wrote:
On Tue, 10 Apr 2018 02:42:19 +0200, Jackie Li  
wrote:


In current code, we only compare the locked WOPCM register values 
with the
calculated values. However, we can continue loading GuC/HuC firmware 
if the
locked (or partially locked) values were valid for current GuC/HuC 
firmware

sizes.

This patch added a new code path to verify whether the locked register
values can be used for GuC/HuC firmware loading, it will recalculate the
verify the new values if these registers were partially locked, so 
that we
won't fail the GuC/HuC firmware loading even if the locked register 
values

are different from the calculated ones.

v2:
 - Update WOPCM register only if it's not locked

Signed-off-by: Jackie Li 
Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Michal Winiarski 
Cc: John Spotswood 
Cc: Joonas Lahtinen 
---
 drivers/gpu/drm/i915/intel_wopcm.c | 217 
+++--

 1 file changed, 185 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_wopcm.c 
b/drivers/gpu/drm/i915/intel_wopcm.c

index b1c08ca..fa8d2be 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -140,6 +140,53 @@ static inline int check_hw_restriction(struct 
drm_i915_private *i915,

 return err;
 }
+static inline u32 calculate_min_guc_wopcm_base(u32 huc_fw_size)
+{
+    return ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
+ GUC_WOPCM_OFFSET_ALIGNMENT);
+}
+
+static inline u32 calculate_min_guc_wopcm_size(u32 guc_fw_size)
+{
+    return guc_fw_size + GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED;
+}
+
+static inline int calculate_max_guc_wopcm_size(struct intel_wopcm 
*wopcm,

+   u32 guc_wopcm_base,
+   u32 *guc_wopcm_size)


Can't we just return this size as positive value?
I guess wopcm size will never be larger than MAX_INT.
We can add GEM_BUG_ON to be sure.


+{
+    struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
+    u32 ctx_rsvd = context_reserved_size(i915);
+
+    if (guc_wopcm_base >= wopcm->size ||
+    (guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
+    DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
+  guc_wopcm_base / 1024);
+    return -E2BIG;


You are mixing calculations with verifications here.
Focus on calculations as you have separate function that verifies values.


+    }
+
+    *guc_wopcm_size =
+    (wopcm->size - guc_wopcm_base - ctx_rsvd) & 
GUC_WOPCM_SIZE_MASK;

+
+    return 0;
+}
+
+static inline int verify_calculated_values(struct drm_i915_private


hmm, maybe we can unify somehow this verification to be able to work with
both calculated and locked values?
I actually thought about that. However, since the verification based on 
the assumption
that the unlocked reg value could be any numbers so it puts more checks 
on these values
which are unnecessary during the calculation. I've made the responding 
check common

enough to be reused by this verification code and the calculation code.



*i915,
+   u32 guc_fw_size, u32 huc_fw_size,
+   u32 guc_wopcm_base,
+   u32 guc_wopcm_size)
+{
+    if (guc_wopcm_size < calculate_min_guc_wopcm_size(guc_fw_size)) {
+    DRM_ERROR("Need %uKiB WOPCM for GuC FW, %uKiB available.\n",
+  calculate_min_guc_wopcm_size(guc_fw_size),


you are calling calculate_min_guc_wopcm_size() twice

Will update it.



+  guc_wopcm_size / 1024);
+    return -E2BIG;
+    }
+
+    return check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size,
+    huc_fw_size);
+}
+
 /**
  * intel_wopcm_init() - Initialize the WOPCM structure.
  * @wopcm: pointer to intel_wopcm.
@@ -157,10 +204,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
 struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
 u32 guc_fw_size = intel_uc_fw_get_upload_size(>guc.fw);
 u32 huc_fw_size = intel_uc_fw_get_upload_size(>huc.fw);
-    u32 ctx_rsvd = context_reserved_size(i915);
 u32 guc_wopcm_base;
 u32 guc_wopcm_size;
-    u32 guc_wopcm_rsvd;
 int err;
GEM_BUG_ON(!wopcm->size);
@@ -177,35 +222,121 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
 return -E2BIG;
 }
-    guc_wopcm_base = ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
-   GUC_WOPCM_OFFSET_ALIGNMENT);
-    if ((guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
-    DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
-  guc_wopcm_base / 1024);
+    guc_wopcm_base = calculate_min_guc_wopcm_base(huc_fw_size);
+    err = calculate_max_guc_wopcm_size(wopcm, guc_wopcm_base,
+   _wopcm_size);
+    if (err)
+    return err;
+
+    DRM_DEBUG_DRIVER("Calculated GuC WOPCM

Re: [Intel-gfx] [PATCH v3 3/4] drm/i915: Add code to accept valid locked WOPCM register values

2018-04-13 Thread Michal Wajdeczko 

On Tue, 10 Apr 2018 02:42:19 +0200, Jackie Li  wrote:

In current code, we only compare the locked WOPCM register values with  
the
calculated values. However, we can continue loading GuC/HuC firmware if  
the
locked (or partially locked) values were valid for current GuC/HuC  
firmware

sizes.

This patch added a new code path to verify whether the locked register
values can be used for GuC/HuC firmware loading, it will recalculate the
verify the new values if these registers were partially locked, so that  
we
won't fail the GuC/HuC firmware loading even if the locked register  
values

are different from the calculated ones.

v2:
 - Update WOPCM register only if it's not locked

Signed-off-by: Jackie Li 
Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Michal Winiarski 
Cc: John Spotswood 
Cc: Joonas Lahtinen 
---
 drivers/gpu/drm/i915/intel_wopcm.c | 217  
+++--

 1 file changed, 185 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_wopcm.c  
b/drivers/gpu/drm/i915/intel_wopcm.c

index b1c08ca..fa8d2be 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -140,6 +140,53 @@ static inline int check_hw_restriction(struct  
drm_i915_private *i915,

return err;
 }
+static inline u32 calculate_min_guc_wopcm_base(u32 huc_fw_size)
+{
+   return ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
+GUC_WOPCM_OFFSET_ALIGNMENT);
+}
+
+static inline u32 calculate_min_guc_wopcm_size(u32 guc_fw_size)
+{
+   return guc_fw_size + GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED;
+}
+
+static inline int calculate_max_guc_wopcm_size(struct intel_wopcm  
*wopcm,

+  u32 guc_wopcm_base,
+  u32 *guc_wopcm_size)


Can't we just return this size as positive value?
I guess wopcm size will never be larger than MAX_INT.
We can add GEM_BUG_ON to be sure.


+{
+   struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
+   u32 ctx_rsvd = context_reserved_size(i915);
+
+   if (guc_wopcm_base >= wopcm->size ||
+   (guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
+   DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
+ guc_wopcm_base / 1024);
+   return -E2BIG;


You are mixing calculations with verifications here.
Focus on calculations as you have separate function that verifies values.


+   }
+
+   *guc_wopcm_size =
+   (wopcm->size - guc_wopcm_base - ctx_rsvd) & GUC_WOPCM_SIZE_MASK;
+
+   return 0;
+}
+
+static inline int verify_calculated_values(struct drm_i915_private


hmm, maybe we can unify somehow this verification to be able to work with
both calculated and locked values?


*i915,
+  u32 guc_fw_size, u32 huc_fw_size,
+  u32 guc_wopcm_base,
+  u32 guc_wopcm_size)
+{
+   if (guc_wopcm_size < calculate_min_guc_wopcm_size(guc_fw_size)) {
+   DRM_ERROR("Need %uKiB WOPCM for GuC FW, %uKiB available.\n",
+ calculate_min_guc_wopcm_size(guc_fw_size),


you are calling calculate_min_guc_wopcm_size() twice


+ guc_wopcm_size / 1024);
+   return -E2BIG;
+   }
+
+   return check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size,
+   huc_fw_size);
+}
+
 /**
  * intel_wopcm_init() - Initialize the WOPCM structure.
  * @wopcm: pointer to intel_wopcm.
@@ -157,10 +204,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
u32 guc_fw_size = intel_uc_fw_get_upload_size(>guc.fw);
u32 huc_fw_size = intel_uc_fw_get_upload_size(>huc.fw);
-   u32 ctx_rsvd = context_reserved_size(i915);
u32 guc_wopcm_base;
u32 guc_wopcm_size;
-   u32 guc_wopcm_rsvd;
int err;
GEM_BUG_ON(!wopcm->size);
@@ -177,35 +222,121 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
return -E2BIG;
}
-   guc_wopcm_base = ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
-  GUC_WOPCM_OFFSET_ALIGNMENT);
-   if ((guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
-   DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
- guc_wopcm_base / 1024);
+   guc_wopcm_base = calculate_min_guc_wopcm_base(huc_fw_size);
+   err = calculate_max_guc_wopcm_size(wopcm, guc_wopcm_base,
+  _wopcm_size);
+   if (err)
+   return err;
+
+   DRM_DEBUG_DRIVER("Calculated GuC WOPCM Region: [%uKiB, %uKiB)\n",
+guc_wopcm_base

Re: [Intel-gfx] [PATCH v3 3/4] drm/i915: Add code to accept valid locked WOPCM register values

2018-04-13 Thread John Spotswood 
On Mon, 2018-04-09 at 17:42 -0700, Jackie Li wrote:
> In current code, we only compare the locked WOPCM register values
> with the
> calculated values. However, we can continue loading GuC/HuC firmware
> if the
> locked (or partially locked) values were valid for current GuC/HuC
> firmware
> sizes.
> 
> This patch added a new code path to verify whether the locked
> register
> values can be used for GuC/HuC firmware loading, it will recalculate
> the
> verify the new values if these registers were partially locked, so
> that we
> won't fail the GuC/HuC firmware loading even if the locked register
> values
> are different from the calculated ones.
> 
> v2:
>  - Update WOPCM register only if it's not locked
> 
> Signed-off-by: Jackie Li 
> Cc: Michal Wajdeczko 
> Cc: Sagar Arun Kamble 
> Cc: Michal Winiarski 
> Cc: John Spotswood 
> Cc: Joonas Lahtinen 

Reviewed-by: John Spotswood 

> ---
>  drivers/gpu/drm/i915/intel_wopcm.c | 217
> +++--
>  1 file changed, 185 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c
> b/drivers/gpu/drm/i915/intel_wopcm.c
> index b1c08ca..fa8d2be 100644
> --- a/drivers/gpu/drm/i915/intel_wopcm.c
> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
> @@ -140,6 +140,53 @@ static inline int check_hw_restriction(struct
> drm_i915_private *i915,
>   return err;
>  }
>  
> +static inline u32 calculate_min_guc_wopcm_base(u32 huc_fw_size)
> +{
> + return ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
> +  GUC_WOPCM_OFFSET_ALIGNMENT);
> +}
> +
> +static inline u32 calculate_min_guc_wopcm_size(u32 guc_fw_size)
> +{
> + return guc_fw_size + GUC_WOPCM_RESERVED +
> GUC_WOPCM_STACK_RESERVED;
> +}
> +
> +static inline int calculate_max_guc_wopcm_size(struct intel_wopcm
> *wopcm,
> +    u32 guc_wopcm_base,
> +    u32 *guc_wopcm_size)
> +{
> + struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
> + u32 ctx_rsvd = context_reserved_size(i915);
> +
> + if (guc_wopcm_base >= wopcm->size ||
> + (guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
> + DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
> +   guc_wopcm_base / 1024);
> + return -E2BIG;
> + }
> +
> + *guc_wopcm_size =
> + (wopcm->size - guc_wopcm_base - ctx_rsvd) &
> GUC_WOPCM_SIZE_MASK;
> +
> + return 0;
> +}
> +
> +static inline int verify_calculated_values(struct drm_i915_private
> *i915,
> +    u32 guc_fw_size, u32
> huc_fw_size,
> +    u32 guc_wopcm_base,
> +    u32 guc_wopcm_size)
> +{
> + if (guc_wopcm_size <
> calculate_min_guc_wopcm_size(guc_fw_size)) {
> + DRM_ERROR("Need %uKiB WOPCM for GuC FW, %uKiB
> available.\n",
> +   calculate_min_guc_wopcm_size(guc_fw_size),
> +   guc_wopcm_size / 1024);
> + return -E2BIG;
> + }
> +
> + return check_hw_restriction(i915, guc_wopcm_base,
> guc_wopcm_size,
> + huc_fw_size);
> +}
> +
>  /**
>   * intel_wopcm_init() - Initialize the WOPCM structure.
>   * @wopcm: pointer to intel_wopcm.
> @@ -157,10 +204,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
>   struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
>   u32 guc_fw_size = intel_uc_fw_get_upload_size(
> >guc.fw);
>   u32 huc_fw_size = intel_uc_fw_get_upload_size(
> >huc.fw);
> - u32 ctx_rsvd = context_reserved_size(i915);
>   u32 guc_wopcm_base;
>   u32 guc_wopcm_size;
> - u32 guc_wopcm_rsvd;
>   int err;
>  
>   GEM_BUG_ON(!wopcm->size);
> @@ -177,35 +222,121 @@ int intel_wopcm_init(struct intel_wopcm
> *wopcm)
>   return -E2BIG;
>   }
>  
> - guc_wopcm_base = ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
> -    GUC_WOPCM_OFFSET_ALIGNMENT);
> - if ((guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
> - DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
> -   guc_wopcm_base / 1024);
> + guc_wopcm_base = calculate_min_guc_wopcm_base(huc_fw_size);
> + err = calculate_max_guc_wopcm_size(wopcm, guc_wopcm_base,
> +    _wopcm_size);
> + if (err)
> + return err;
> +
> + DRM_DEBUG_DRIVER("Calculated GuC WOPCM Region: [%uKiB,
> %uKiB)\n",
> +  guc_wopcm_base / 1024,
> +  (guc_wopcm_base + guc_wopcm_size) / 1024);
> +
> + err = verify_calculated_values(i915, guc_fw_size,
> huc_fw_size,
> +    guc_wopcm_base,
> guc_wopcm_size);
> + if (err)
> +

[Intel-gfx] [PATCH v3 3/4] drm/i915: Add code to accept valid locked WOPCM register values

2018-04-09 Thread Jackie Li 
In current code, we only compare the locked WOPCM register values with the
calculated values. However, we can continue loading GuC/HuC firmware if the
locked (or partially locked) values were valid for current GuC/HuC firmware
sizes.

This patch added a new code path to verify whether the locked register
values can be used for GuC/HuC firmware loading, it will recalculate the
verify the new values if these registers were partially locked, so that we
won't fail the GuC/HuC firmware loading even if the locked register values
are different from the calculated ones.

v2:
 - Update WOPCM register only if it's not locked

Signed-off-by: Jackie Li 
Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Michal Winiarski 
Cc: John Spotswood 
Cc: Joonas Lahtinen 
---
 drivers/gpu/drm/i915/intel_wopcm.c | 217 +++--
 1 file changed, 185 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_wopcm.c 
b/drivers/gpu/drm/i915/intel_wopcm.c
index b1c08ca..fa8d2be 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -140,6 +140,53 @@ static inline int check_hw_restriction(struct 
drm_i915_private *i915,
return err;
 }
 
+static inline u32 calculate_min_guc_wopcm_base(u32 huc_fw_size)
+{
+   return ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
+GUC_WOPCM_OFFSET_ALIGNMENT);
+}
+
+static inline u32 calculate_min_guc_wopcm_size(u32 guc_fw_size)
+{
+   return guc_fw_size + GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED;
+}
+
+static inline int calculate_max_guc_wopcm_size(struct intel_wopcm *wopcm,
+  u32 guc_wopcm_base,
+  u32 *guc_wopcm_size)
+{
+   struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
+   u32 ctx_rsvd = context_reserved_size(i915);
+
+   if (guc_wopcm_base >= wopcm->size ||
+   (guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
+   DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
+ guc_wopcm_base / 1024);
+   return -E2BIG;
+   }
+
+   *guc_wopcm_size =
+   (wopcm->size - guc_wopcm_base - ctx_rsvd) & GUC_WOPCM_SIZE_MASK;
+
+   return 0;
+}
+
+static inline int verify_calculated_values(struct drm_i915_private *i915,
+  u32 guc_fw_size, u32 huc_fw_size,
+  u32 guc_wopcm_base,
+  u32 guc_wopcm_size)
+{
+   if (guc_wopcm_size < calculate_min_guc_wopcm_size(guc_fw_size)) {
+   DRM_ERROR("Need %uKiB WOPCM for GuC FW, %uKiB available.\n",
+ calculate_min_guc_wopcm_size(guc_fw_size),
+ guc_wopcm_size / 1024);
+   return -E2BIG;
+   }
+
+   return check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size,
+   huc_fw_size);
+}
+
 /**
  * intel_wopcm_init() - Initialize the WOPCM structure.
  * @wopcm: pointer to intel_wopcm.
@@ -157,10 +204,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
u32 guc_fw_size = intel_uc_fw_get_upload_size(>guc.fw);
u32 huc_fw_size = intel_uc_fw_get_upload_size(>huc.fw);
-   u32 ctx_rsvd = context_reserved_size(i915);
u32 guc_wopcm_base;
u32 guc_wopcm_size;
-   u32 guc_wopcm_rsvd;
int err;
 
GEM_BUG_ON(!wopcm->size);
@@ -177,35 +222,121 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
return -E2BIG;
}
 
-   guc_wopcm_base = ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
-  GUC_WOPCM_OFFSET_ALIGNMENT);
-   if ((guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
-   DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
- guc_wopcm_base / 1024);
+   guc_wopcm_base = calculate_min_guc_wopcm_base(huc_fw_size);
+   err = calculate_max_guc_wopcm_size(wopcm, guc_wopcm_base,
+  _wopcm_size);
+   if (err)
+   return err;
+
+   DRM_DEBUG_DRIVER("Calculated GuC WOPCM Region: [%uKiB, %uKiB)\n",
+guc_wopcm_base / 1024,
+(guc_wopcm_base + guc_wopcm_size) / 1024);
+
+   err = verify_calculated_values(i915, guc_fw_size, huc_fw_size,
+  guc_wopcm_base, guc_wopcm_size);
+   if (err)
+   return err;
+
+   wopcm->guc.base = guc_wopcm_base;
+   wopcm->guc.size = guc_wopcm_size;
+
+   return 0;
+}
+
+static inline int verify_locked_values(struct intel_wopcm *wopcm,
+  u32 guc_wopcm_base, u32 guc_wopcm_size)
+{
+