diff mbox

[v2] drm/i915/guc: Use formalized struct definition for ads object

Message ID 20170307110936.118816-1-michal.wajdeczko@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Michal Wajdeczko March 7, 2017, 11:09 a.m. UTC 
Manual pointer manipulation is error prone. Let compiler calculate
right offsets for us in case we need to change ads layout.

v2: don't call it object (Chris)

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 50 ++++++++++++++----------------
 1 file changed, 24 insertions(+), 26 deletions(-)

Comments

Chris Wilson March 7, 2017, 12:36 p.m. UTC | #1 
On Tue, Mar 07, 2017 at 11:09:36AM +0000, Michal Wajdeczko wrote:
> Manual pointer manipulation is error prone. Let compiler calculate
> right offsets for us in case we need to change ads layout.
> 
> v2: don't call it object (Chris)
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>

Looks good. I would have done mine line splitting slightly differently.
For

> -	ads->reg_state_buffer = ads->reg_state_addr +
> -			sizeof(struct guc_mmio_reg_state);
> +	blob->ads.reg_state_buffer = offset +
> +		offsetof(struct __guc_ads_blob, reg_state_buffer);

I would have used

	blob->ads.reg_state_buffer =
		offset + offsetof(struct __guc_ads_blob, reg_state_buffer);

I didn't see any mistakes in the conversion, and the packing is
certainly more obvious now.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index beb38e3..c95616e 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -810,22 +810,21 @@  static void guc_addon_create(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	struct i915_vma *vma;
-	struct guc_ads *ads;
-	struct guc_policies *policies;
-	struct guc_mmio_reg_state *reg_state;
-	struct intel_engine_cs *engine;
-	enum intel_engine_id id;
 	struct page *page;
-	u32 size;
-
 	/* The ads obj includes the struct itself and buffers passed to GuC */
-	size = sizeof(struct guc_ads) + sizeof(struct guc_policies) +
-			sizeof(struct guc_mmio_reg_state) +
-			GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE;
+	struct __guc_ads_blob {
+		struct guc_ads ads;
+		struct guc_policies policies;
+		struct guc_mmio_reg_state reg_state;
+		u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
+	} __packed *blob;
+	u32 offset;
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
 
 	vma = guc->ads_vma;
 	if (!vma) {
-		vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(size));
+		vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(sizeof(*blob)));
 		if (IS_ERR(vma))
 			return;
 
@@ -833,7 +832,8 @@  static void guc_addon_create(struct intel_guc *guc)
 	}
 
 	page = i915_vma_first_page(vma);
-	ads = kmap(page);
+	blob = kmap(page);
+	offset = i915_ggtt_offset(vma);
 
 	/*
 	 * The GuC requires a "Golden Context" when it reinitialises
@@ -843,34 +843,32 @@  static void guc_addon_create(struct intel_guc *guc)
 	 * to find it.
 	 */
 	engine = dev_priv->engine[RCS];
-	ads->golden_context_lrca = engine->status_page.ggtt_offset;
+	blob->ads.golden_context_lrca = engine->status_page.ggtt_offset;
 
 	for_each_engine(engine, dev_priv, id)
-		ads->eng_state_size[engine->guc_id] = intel_lr_context_size(engine);
+		blob->ads.eng_state_size[engine->guc_id] =
+			intel_lr_context_size(engine);
 
 	/* GuC scheduling policies */
-	policies = (void *)ads + sizeof(struct guc_ads);
-	guc_policies_init(policies);
+	guc_policies_init(&blob->policies);
 
-	ads->scheduler_policies =
-		guc_ggtt_offset(vma) + sizeof(struct guc_ads);
+	blob->ads.scheduler_policies = offset +
+		offsetof(struct __guc_ads_blob, policies);
 
 	/* MMIO reg state */
-	reg_state = (void *)policies + sizeof(struct guc_policies);
-
 	for_each_engine(engine, dev_priv, id) {
-		reg_state->mmio_white_list[engine->guc_id].mmio_start =
+		blob->reg_state.mmio_white_list[engine->guc_id].mmio_start =
 			engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
 
 		/* Nothing to be saved or restored for now. */
-		reg_state->mmio_white_list[engine->guc_id].count = 0;
+		blob->reg_state.mmio_white_list[engine->guc_id].count = 0;
 	}
 
-	ads->reg_state_addr = ads->scheduler_policies +
-			sizeof(struct guc_policies);
+	blob->ads.reg_state_addr = offset +
+		offsetof(struct __guc_ads_blob, reg_state);
 
-	ads->reg_state_buffer = ads->reg_state_addr +
-			sizeof(struct guc_mmio_reg_state);
+	blob->ads.reg_state_buffer = offset +
+		offsetof(struct __guc_ads_blob, reg_state_buffer);
 
 	kunmap(page);
 }