Re: [Intel-gfx] [PATCH v8 2/6] drm/i915/guc: Rename guc_ggtt_offset to intel_guc_ggtt_offset

2018-02-06 Thread Chris Wilson 
Quoting Jackie Li (2018-02-06 00:02:38)
> diff --git a/drivers/gpu/drm/i915/intel_guc_ads.c 
> b/drivers/gpu/drm/i915/intel_guc_ads.c
> index ac62753..7215594 100644
> --- a/drivers/gpu/drm/i915/intel_guc_ads.c
> +++ b/drivers/gpu/drm/i915/intel_guc_ads.c
> @@ -113,17 +113,6 @@ int intel_guc_ads_create(struct intel_guc *guc)
> blob->reg_state.white_list[engine->guc_id].count = 0;
> }
>  
> -   /*
> -* The GuC requires a "Golden Context" when it reinitialises
> -* engines after a reset. Here we use the Render ring default
> -* context, which must already exist and be pinned in the GGTT,
> -* so its address won't change after we've told the GuC where
> -* to find it. Note that we have to skip our header (1 page),
> -* because our GuC shared data is there.
> -*/
> -   blob->ads.golden_context_lrca =
> -   guc_ggtt_offset(dev_priv->kernel_context->engine[RCS].state) +
> -   skipped_offset;

> +   vma = dev_priv->kernel_context->engine[RCS].state;
> +   blob->ads.golden_context_lrca = intel_guc_ggtt_offset(guc, vma) +
> +   skipped_offset;
> +

Please consider the visual grouping in line breaks carefully. The
alignment in the first stanza couples the ggtt_offset with the addition
of the skipped_offset. The lack of alignment in the second, makes the
skipped_offset appear an unrelated after thought.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v8 2/6] drm/i915/guc: Rename guc_ggtt_offset to intel_guc_ggtt_offset

2018-02-05 Thread Sagar Arun Kamble 



On 2/6/2018 5:32 AM, Jackie Li wrote:

GuC related exported functions should start with "intel_guc_" prefix and
pass intel_guc as the first parameter since its guc related. Current
guc_ggtt_offset() failed to follow this code convention and this is a
problem for future patches that needs to access intel_guc data to verify
the GGTT offset against the GuC WOPCM top.

This patch renames the guc_ggtt_offset to intel_guc_ggtt_offset and updates
the related code to pass intel_guc pointer to this function call, so that
we can have a unified coding style for GuC code and also enable the future
patches to get GuC related data from intel_guc to do the offset
verification. Meanwhile, this patch also moves the GUC_GGTT_TOP from
intel_guc_regs.h to intel_guc.h since it is not GuC register related
definition.

v8:
  - Fixed coding style issues and moved GUC_GGTT_TOP to intel_guc.h (Sagar)
  - Updated commit message to explain to reason and motivation to add
intel_guc as the first parameter of intel_guc_ggtt_offset (Chris)

Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Chris Wilson 
Cc: Joonas Lahtinen 
Signed-off-by: Jackie Li 

Reviewed-by: Sagar Arun Kamble 

---
  drivers/gpu/drm/i915/intel_guc.c| 12 +++-
  drivers/gpu/drm/i915/intel_guc.h| 14 --
  drivers/gpu/drm/i915/intel_guc_ads.c| 25 +
  drivers/gpu/drm/i915/intel_guc_ct.c |  5 +++--
  drivers/gpu/drm/i915/intel_guc_fw.c |  2 +-
  drivers/gpu/drm/i915/intel_guc_log.c|  2 +-
  drivers/gpu/drm/i915/intel_guc_reg.h|  3 ---
  drivers/gpu/drm/i915/intel_guc_submission.c | 10 +-
  drivers/gpu/drm/i915/intel_huc.c|  6 --
  9 files changed, 46 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 9f45e6d..d9bc2a9 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -269,8 +269,10 @@ void intel_guc_init_params(struct intel_guc *guc)
  
  	/* If GuC submission is enabled, set up additional parameters here */

if (USES_GUC_SUBMISSION(dev_priv)) {
-   u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
-   u32 pgs = guc_ggtt_offset(dev_priv->guc.stage_desc_pool);
+   u32 ads = intel_guc_ggtt_offset(guc,
+   guc->ads_vma) >> PAGE_SHIFT;
+   u32 pgs = intel_guc_ggtt_offset(guc,
+   dev_priv->guc.stage_desc_pool);
u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16;
  
  		params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT;

@@ -418,7 +420,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
/* any value greater than GUC_POWER_D0 */
data[1] = GUC_POWER_D1;
-   data[2] = guc_ggtt_offset(guc->shared_data);
+   data[2] = intel_guc_ggtt_offset(guc, guc->shared_data);
  
  	return intel_guc_send(guc, data, ARRAY_SIZE(data));

  }
@@ -441,7 +443,7 @@ int intel_guc_reset_engine(struct intel_guc *guc,
data[3] = 0;
data[4] = 0;
data[5] = guc->execbuf_client->stage_id;
-   data[6] = guc_ggtt_offset(guc->shared_data);
+   data[6] = intel_guc_ggtt_offset(guc, guc->shared_data);
  
  	return intel_guc_send(guc, data, ARRAY_SIZE(data));

  }
@@ -463,7 +465,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
  
  	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;

data[1] = GUC_POWER_D0;
-   data[2] = guc_ggtt_offset(guc->shared_data);
+   data[2] = intel_guc_ggtt_offset(guc, guc->shared_data);
  
  	return intel_guc_send(guc, data, ARRAY_SIZE(data));

  }
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 9e0a97e..50be6de 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -101,13 +101,23 @@ static inline void intel_guc_notify(struct intel_guc *guc)
guc->notify(guc);
  }
  
-/*

+/* GuC addresses above GUC_GGTT_TOP also don't map through the GTT */
+#define GUC_GGTT_TOP   0xFEE0
+
+/**
+ * intel_guc_ggtt_offset() - Get and validate the GGTT offset of @vma
+ * @guc: intel guc.
+ * @vma: i915 graphics virtual memory area.
+ *
   * GuC does not allow any gfx GGTT address that falls into range [0, 
WOPCM_TOP),
   * which is reserved for Boot ROM, SRAM and WOPCM. Currently this top address 
is
   * 512K. In order to exclude 0-512K address space from GGTT, all gfx objects
   * used by GuC is pinned with PIN_OFFSET_BIAS along with size of WOPCM.
+ *
+ * Return: GGTT offset that meets the GuC gfx address requirement.
   */
-static inline u32 guc_ggtt_offset(struct i915_vma *vma)
+static inline u32 intel_guc_ggtt_offset(struct intel_guc

[Intel-gfx] [PATCH v8 2/6] drm/i915/guc: Rename guc_ggtt_offset to intel_guc_ggtt_offset

2018-02-05 Thread Jackie Li 
GuC related exported functions should start with "intel_guc_" prefix and
pass intel_guc as the first parameter since its guc related. Current
guc_ggtt_offset() failed to follow this code convention and this is a
problem for future patches that needs to access intel_guc data to verify
the GGTT offset against the GuC WOPCM top.

This patch renames the guc_ggtt_offset to intel_guc_ggtt_offset and updates
the related code to pass intel_guc pointer to this function call, so that
we can have a unified coding style for GuC code and also enable the future
patches to get GuC related data from intel_guc to do the offset
verification. Meanwhile, this patch also moves the GUC_GGTT_TOP from
intel_guc_regs.h to intel_guc.h since it is not GuC register related
definition.

v8:
 - Fixed coding style issues and moved GUC_GGTT_TOP to intel_guc.h (Sagar)
 - Updated commit message to explain to reason and motivation to add
   intel_guc as the first parameter of intel_guc_ggtt_offset (Chris)

Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Chris Wilson 
Cc: Joonas Lahtinen 
Signed-off-by: Jackie Li 
---
 drivers/gpu/drm/i915/intel_guc.c| 12 +++-
 drivers/gpu/drm/i915/intel_guc.h| 14 --
 drivers/gpu/drm/i915/intel_guc_ads.c| 25 +
 drivers/gpu/drm/i915/intel_guc_ct.c |  5 +++--
 drivers/gpu/drm/i915/intel_guc_fw.c |  2 +-
 drivers/gpu/drm/i915/intel_guc_log.c|  2 +-
 drivers/gpu/drm/i915/intel_guc_reg.h|  3 ---
 drivers/gpu/drm/i915/intel_guc_submission.c | 10 +-
 drivers/gpu/drm/i915/intel_huc.c|  6 --
 9 files changed, 46 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 9f45e6d..d9bc2a9 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -269,8 +269,10 @@ void intel_guc_init_params(struct intel_guc *guc)
 
/* If GuC submission is enabled, set up additional parameters here */
if (USES_GUC_SUBMISSION(dev_priv)) {
-   u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
-   u32 pgs = guc_ggtt_offset(dev_priv->guc.stage_desc_pool);
+   u32 ads = intel_guc_ggtt_offset(guc,
+   guc->ads_vma) >> PAGE_SHIFT;
+   u32 pgs = intel_guc_ggtt_offset(guc,
+   dev_priv->guc.stage_desc_pool);
u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16;
 
params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT;
@@ -418,7 +420,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
/* any value greater than GUC_POWER_D0 */
data[1] = GUC_POWER_D1;
-   data[2] = guc_ggtt_offset(guc->shared_data);
+   data[2] = intel_guc_ggtt_offset(guc, guc->shared_data);
 
return intel_guc_send(guc, data, ARRAY_SIZE(data));
 }
@@ -441,7 +443,7 @@ int intel_guc_reset_engine(struct intel_guc *guc,
data[3] = 0;
data[4] = 0;
data[5] = guc->execbuf_client->stage_id;
-   data[6] = guc_ggtt_offset(guc->shared_data);
+   data[6] = intel_guc_ggtt_offset(guc, guc->shared_data);
 
return intel_guc_send(guc, data, ARRAY_SIZE(data));
 }
@@ -463,7 +465,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
 
data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
data[1] = GUC_POWER_D0;
-   data[2] = guc_ggtt_offset(guc->shared_data);
+   data[2] = intel_guc_ggtt_offset(guc, guc->shared_data);
 
return intel_guc_send(guc, data, ARRAY_SIZE(data));
 }
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 9e0a97e..50be6de 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -101,13 +101,23 @@ static inline void intel_guc_notify(struct intel_guc *guc)
guc->notify(guc);
 }
 
-/*
+/* GuC addresses above GUC_GGTT_TOP also don't map through the GTT */
+#define GUC_GGTT_TOP   0xFEE0
+
+/**
+ * intel_guc_ggtt_offset() - Get and validate the GGTT offset of @vma
+ * @guc: intel guc.
+ * @vma: i915 graphics virtual memory area.
+ *
  * GuC does not allow any gfx GGTT address that falls into range [0, 
WOPCM_TOP),
  * which is reserved for Boot ROM, SRAM and WOPCM. Currently this top address 
is
  * 512K. In order to exclude 0-512K address space from GGTT, all gfx objects
  * used by GuC is pinned with PIN_OFFSET_BIAS along with size of WOPCM.
+ *
+ * Return: GGTT offset that meets the GuC gfx address requirement.
  */
-static inline u32 guc_ggtt_offset(struct i915_vma *vma)
+static inline u32 intel_guc_ggtt_offset(struct intel_guc *guc,
+   struct i915_vma *vma)
 {
u32 offset =