Re: [Intel-gfx] [PATCH v7 3/6] drm/i915/guc: Implement dynamic GuC WOPCM offset and size

2018-02-01 Thread Sagar Arun Kamble 



On 2/2/2018 3:21 AM, Yaodong Li wrote:


On 02/01/2018 12:38 AM, Sagar Arun Kamble wrote:



On 1/19/2018 6:59 AM, Jackie Li wrote:

Hardware may have specific restrictions on GuC WOPCM size
It would be good if you can tell about Gen9/CNL restriction briefly 
here.

versus HuC firmware size. With static GuC WOPCM size,
there's no way to adjust the GuC WOPCM partition size based on
the actual HuC firmware size, so that GuC/HuC loading failure
would occur even if there was enough WOPCM space for both
GuC and HuC firmware.

This patch enables the dynamic calculation of the GuC WOPCM
aperture size used by GuC and HuC firmware.

we are also calculating for HuC?

Strictly speaking, we are calculating the GuC WOPCM offset based on
GuC & HuC firmware sizes. Will make it clearer. Thanks.


+    offset = huc_fw_size + WOPCM_RESERVED_SIZE;
+    if (offset >= WOPCM_DEFAULT_SIZE)
+    return -E2BIG;
+
+    /* Hardware requires GuC WOPCM offset needs to be 16K aligned. */
+    offset = ALIGN(offset, WOPCM_OFFSET_ALIGNMENT);
+    if ((offset + reserved) >= WOPCM_DEFAULT_SIZE)
+    return -E2BIG;
+
+    top = WOPCM_DEFAULT_SIZE - offset;
+    size = top - reserved;
+
+    /* GuC WOPCM size must be 4K aligned. */
+    size &= GUC_WOPCM_SIZE_MASK;
+

ALIGN(size, PAGE_SIZE)? Can avoid this new macro.
If you want to stay with GUC_WOPCM_SIZE_MASK, then that macro needs 
to be in guc_wopcm.h
I'd like to go with GUC_WOPCM_SIZE_MASK here since there's no sign 
that it should be related to
page size. *Align* seems not accurate here since I actually wanted to 
trim the size to 4k boundary,

will update the comments. Thanks!
Need to update comment though. /* GuC WOPCM size must be multiple of 4K 
pages */


Align does not suit here. We could have used "& ~PAGE_MASK" but okay 
with G_W_S_M too.
size being absolute here helps. had it been offset starting from 0 we 
would have needed extra logic.





--
Thanks,
Sagar

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

Re: [Intel-gfx] [PATCH v7 3/6] drm/i915/guc: Implement dynamic GuC WOPCM offset and size

2018-02-01 Thread Yaodong Li 


On 02/01/2018 12:38 AM, Sagar Arun Kamble wrote:



On 1/19/2018 6:59 AM, Jackie Li wrote:

Hardware may have specific restrictions on GuC WOPCM size

It would be good if you can tell about Gen9/CNL restriction briefly here.

versus HuC firmware size. With static GuC WOPCM size,
there's no way to adjust the GuC WOPCM partition size based on
the actual HuC firmware size, so that GuC/HuC loading failure
would occur even if there was enough WOPCM space for both
GuC and HuC firmware.

This patch enables the dynamic calculation of the GuC WOPCM
aperture size used by GuC and HuC firmware.

we are also calculating for HuC?

Strictly speaking, we are calculating the GuC WOPCM offset based on
GuC & HuC firmware sizes. Will make it clearer. Thanks.


+    offset = huc_fw_size + WOPCM_RESERVED_SIZE;
+    if (offset >= WOPCM_DEFAULT_SIZE)
+    return -E2BIG;
+
+    /* Hardware requires GuC WOPCM offset needs to be 16K aligned. */
+    offset = ALIGN(offset, WOPCM_OFFSET_ALIGNMENT);
+    if ((offset + reserved) >= WOPCM_DEFAULT_SIZE)
+    return -E2BIG;
+
+    top = WOPCM_DEFAULT_SIZE - offset;
+    size = top - reserved;
+
+    /* GuC WOPCM size must be 4K aligned. */
+    size &= GUC_WOPCM_SIZE_MASK;
+

ALIGN(size, PAGE_SIZE)? Can avoid this new macro.
If you want to stay with GUC_WOPCM_SIZE_MASK, then that macro needs to 
be in guc_wopcm.h
I'd like to go with GUC_WOPCM_SIZE_MASK here since there's no sign that 
it should be related to
page size. *Align* seems not accurate here since I actually wanted to 
trim the size to 4k boundary,

will update the comments. Thanks!


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

Re: [Intel-gfx] [PATCH v7 3/6] drm/i915/guc: Implement dynamic GuC WOPCM offset and size

2018-02-01 Thread Sagar Arun Kamble 



On 1/19/2018 6:59 AM, Jackie Li wrote:

Hardware may have specific restrictions on GuC WOPCM size

It would be good if you can tell about Gen9/CNL restriction briefly here.

versus HuC firmware size. With static GuC WOPCM size,
there's no way to adjust the GuC WOPCM partition size based on
the actual HuC firmware size, so that GuC/HuC loading failure
would occur even if there was enough WOPCM space for both
GuC and HuC firmware.

This patch enables the dynamic calculation of the GuC WOPCM
aperture size used by GuC and HuC firmware.

we are also calculating for HuC?

  GuC WOPCM offset is
set to HuC size + reserved WOPCM size. GuC WOPCM size is set to
total WOPCM size - GuC WOPCM offset - RC6CTX size. In this case,
GuC WOPCM offset will be updated based on the size of HuC firmware
while GuC WOPCM size will be set to use all the remaining WOPCM space.

v2:
  - Removed intel_wopcm_init (Ville/Sagar/Joonas)
  - Renamed and Moved the intel_wopcm_partition into intel_guc (Sagar)
  - Removed unnecessary function calls (Joonas)
  - Init GuC WOPCM partition as soon as firmware fetching is completed

v3:
  - Fixed indentation issues (Chris)
  - Removed layering violation code (Chris/Michal)
  - Created separat files for GuC wopcm code  (Michal)
  - Used inline function to avoid code duplication (Michal)

v4:
  - Preset the GuC WOPCM top during early GuC init (Chris)
  - Fail intel_uc_init_hw() as soon as GuC WOPCM partitioning failed

v5:
  - Moved GuC DMA WOPCM register updating code into intel_guc_wopcm.c
  - Took care of the locking status before writing to GuC DMA
Write-Once registers. (Joonas)

v6:
  - Made sure the GuC WOPCM size to be multiple of 4K (4K aligned)

Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Sujaritha Sundaresan 
Cc: Daniele Ceraolo Spurio 
Cc: John Spotswood 
Cc: Oscar Mateo 
Cc: Chris Wilson 
Cc: Joonas Lahtinen 
Signed-off-by: Jackie Li 
---
  drivers/gpu/drm/i915/i915_gem_context.c |   9 +--
  drivers/gpu/drm/i915/intel_guc.c|   5 +-
  drivers/gpu/drm/i915/intel_guc.h|  12 ++--
  drivers/gpu/drm/i915/intel_guc_reg.h|   1 +
  drivers/gpu/drm/i915/intel_guc_wopcm.c  | 108 +---
  drivers/gpu/drm/i915/intel_guc_wopcm.h  |  40 ++--
  drivers/gpu/drm/i915/intel_huc.c|   2 +-
  drivers/gpu/drm/i915/intel_uc.c |  11 +++-
  drivers/gpu/drm/i915/intel_uc_fw.c  |  11 +++-
  drivers/gpu/drm/i915/intel_uc_fw.h  |  16 +
  10 files changed, 183 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
b/drivers/gpu/drm/i915/i915_gem_context.c
index 648e753..b485794 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -312,12 +312,13 @@ __create_hw_context(struct drm_i915_private *dev_priv,
ctx->desc_template =
default_desc_template(dev_priv, dev_priv->mm.aliasing_ppgtt);
  
-	/* GuC requires the ring to be placed above GUC_WOPCM_TOP. If GuC is not

-* present or not in use we still need a small bias as ring wraparound
-* at offset 0 sometimes hangs. No idea why.
+   /*
+* GuC requires the ring to be placed above GuC WOPCM top. If GuC is not
+-   * present or not in use we still need a small bias as ring wraparound
+-   * at offset 0 sometimes hangs. No idea why.

+-?

 */
if (USES_GUC(dev_priv))
-   ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
+   ctx->ggtt_offset_bias = dev_priv->guc.wopcm.top;
else
ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
  
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c

index e70885b..3521beb 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -64,6 +64,7 @@ void intel_guc_init_early(struct intel_guc *guc)
  {
intel_guc_fw_init_early(guc);
intel_guc_ct_init_early(>ct);
+   intel_guc_wopcm_init_early(>wopcm);
  
  	mutex_init(>send_mutex);

guc->send = intel_guc_send_nop;
@@ -473,7 +474,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
   * This is a wrapper to create an object for use with the GuC. In order to
   * use it inside the GuC, an object needs to be pinned lifetime, so we 
allocate
   * both some backing storage and a range inside the Global GTT. We must pin
- * it in the GGTT somewhere other than than [0, GUC_WOPCM_TOP) because that
+ * it in the GGTT somewhere other than than [0, GuC WOPCM top) because that
   * range is reserved inside GuC.
   *
   * Return:A i915_vma if successful, otherwise an ERR_PTR.
@@ -494,7 +495,7 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc 
*guc, u32 size)
goto err;

[Intel-gfx] [PATCH v7 3/6] drm/i915/guc: Implement dynamic GuC WOPCM offset and size

2018-01-18 Thread Jackie Li 
Hardware may have specific restrictions on GuC WOPCM size
versus HuC firmware size. With static GuC WOPCM size,
there's no way to adjust the GuC WOPCM partition size based on
the actual HuC firmware size, so that GuC/HuC loading failure
would occur even if there was enough WOPCM space for both
GuC and HuC firmware.

This patch enables the dynamic calculation of the GuC WOPCM
aperture size used by GuC and HuC firmware. GuC WOPCM offset is
set to HuC size + reserved WOPCM size. GuC WOPCM size is set to
total WOPCM size - GuC WOPCM offset - RC6CTX size. In this case,
GuC WOPCM offset will be updated based on the size of HuC firmware
while GuC WOPCM size will be set to use all the remaining WOPCM space.

v2:
 - Removed intel_wopcm_init (Ville/Sagar/Joonas)
 - Renamed and Moved the intel_wopcm_partition into intel_guc (Sagar)
 - Removed unnecessary function calls (Joonas)
 - Init GuC WOPCM partition as soon as firmware fetching is completed

v3:
 - Fixed indentation issues (Chris)
 - Removed layering violation code (Chris/Michal)
 - Created separat files for GuC wopcm code  (Michal)
 - Used inline function to avoid code duplication (Michal)

v4:
 - Preset the GuC WOPCM top during early GuC init (Chris)
 - Fail intel_uc_init_hw() as soon as GuC WOPCM partitioning failed

v5:
 - Moved GuC DMA WOPCM register updating code into intel_guc_wopcm.c
 - Took care of the locking status before writing to GuC DMA
   Write-Once registers. (Joonas)

v6:
 - Made sure the GuC WOPCM size to be multiple of 4K (4K aligned)

Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Sujaritha Sundaresan 
Cc: Daniele Ceraolo Spurio 
Cc: John Spotswood 
Cc: Oscar Mateo 
Cc: Chris Wilson 
Cc: Joonas Lahtinen 
Signed-off-by: Jackie Li 
---
 drivers/gpu/drm/i915/i915_gem_context.c |   9 +--
 drivers/gpu/drm/i915/intel_guc.c|   5 +-
 drivers/gpu/drm/i915/intel_guc.h|  12 ++--
 drivers/gpu/drm/i915/intel_guc_reg.h|   1 +
 drivers/gpu/drm/i915/intel_guc_wopcm.c  | 108 +---
 drivers/gpu/drm/i915/intel_guc_wopcm.h  |  40 ++--
 drivers/gpu/drm/i915/intel_huc.c|   2 +-
 drivers/gpu/drm/i915/intel_uc.c |  11 +++-
 drivers/gpu/drm/i915/intel_uc_fw.c  |  11 +++-
 drivers/gpu/drm/i915/intel_uc_fw.h  |  16 +
 10 files changed, 183 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
b/drivers/gpu/drm/i915/i915_gem_context.c
index 648e753..b485794 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -312,12 +312,13 @@ __create_hw_context(struct drm_i915_private *dev_priv,
ctx->desc_template =
default_desc_template(dev_priv, dev_priv->mm.aliasing_ppgtt);
 
-   /* GuC requires the ring to be placed above GUC_WOPCM_TOP. If GuC is not
-* present or not in use we still need a small bias as ring wraparound
-* at offset 0 sometimes hangs. No idea why.
+   /*
+* GuC requires the ring to be placed above GuC WOPCM top. If GuC is not
+-   * present or not in use we still need a small bias as ring wraparound
+-   * at offset 0 sometimes hangs. No idea why.
 */
if (USES_GUC(dev_priv))
-   ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
+   ctx->ggtt_offset_bias = dev_priv->guc.wopcm.top;
else
ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
 
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index e70885b..3521beb 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -64,6 +64,7 @@ void intel_guc_init_early(struct intel_guc *guc)
 {
intel_guc_fw_init_early(guc);
intel_guc_ct_init_early(>ct);
+   intel_guc_wopcm_init_early(>wopcm);
 
mutex_init(>send_mutex);
guc->send = intel_guc_send_nop;
@@ -473,7 +474,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
  * This is a wrapper to create an object for use with the GuC. In order to
  * use it inside the GuC, an object needs to be pinned lifetime, so we allocate
  * both some backing storage and a range inside the Global GTT. We must pin
- * it in the GGTT somewhere other than than [0, GUC_WOPCM_TOP) because that
+ * it in the GGTT somewhere other than than [0, GuC WOPCM top) because that
  * range is reserved inside GuC.
  *
  * Return: A i915_vma if successful, otherwise an ERR_PTR.
@@ -494,7 +495,7 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc 
*guc, u32 size)
goto err;
 
ret = i915_vma_pin(vma, 0, PAGE_SIZE,
-  PIN_GLOBAL | PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
+  PIN_GLOBAL | PIN_OFFSET_BIAS |