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

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

[Michal Wajdeczko]

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)
v3: restyle offset assignments (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>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 50 ++++++++++++++----------------
 1 file changed, 24 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index b4d24cd..ad4d6f0 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);
 }
-- 
2.7.4

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

✓ Fi.CI.BAT: success for drm/i915/guc: Use formalized struct definition for ads object (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915/guc: Use formalized struct definition for ads object (rev3)
URL   : https://patchwork.freedesktop.org/series/20826/
State : success

== Summary ==

Series 20826v3 drm/i915/guc: Use formalized struct definition for ads object
https://patchwork.freedesktop.org/api/1.0/series/20826/revisions/3/mbox/

Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                fail       -> PASS       (fi-snb-2520m)

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 453s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 581s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 528s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 548s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 501s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 504s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 436s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 427s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 438s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 509s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 499s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 480s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 480s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 592s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 480s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 514s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 541s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 422s

c641417b70c6b78efca29ae732d7cbf5716ac6d5 drm-tip: 2017y-03m-14d-16h-04m-56s UTC integration manifest
80ddf08 drm/i915/guc: Use formalized struct definition for ads object

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4164/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

From: Michal Wajdeczko <michal.wajdeczko@intel.com>

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)
v3: restyle offset assignments (Chris)
v4: stylistic reductions

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>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 51 ++++++++++++++----------------
 1 file changed, 23 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index b4d24cd7639a..3651a1b0d13c 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -806,26 +806,27 @@ static void guc_policies_init(struct guc_policies *policies)
 	policies->is_valid = 1;
 }
 
+#define p_offset(ptr, member) offsetof(typeof(*ptr), member)
+
 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 {
+		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 +834,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 +845,27 @@ 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);
-
-	ads->scheduler_policies =
-		guc_ggtt_offset(vma) + sizeof(struct guc_ads);
+	guc_policies_init(&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);
-
-	ads->reg_state_buffer = ads->reg_state_addr +
-			sizeof(struct guc_mmio_reg_state);
+	blob->ads.scheduler_policies = offset + p_offset(blob, policies);
+	blob->ads.reg_state_addr = offset + p_offset(blob, reg_state);
+	blob->ads.reg_state_buffer = offset + p_offset(blob, reg_state_buffer);
 
 	kunmap(page);
 }
-- 
2.11.0

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

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

[Michal Wajdeczko]

On Tue, Mar 14, 2017 at 08:53:02PM +0000, Chris Wilson wrote:
> From: Michal Wajdeczko <michal.wajdeczko@intel.com>
> 
> 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)
> v3: restyle offset assignments (Chris)
> v4: stylistic reductions
> 
> 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>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 51 ++++++++++++++----------------
>  1 file changed, 23 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index b4d24cd7639a..3651a1b0d13c 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -806,26 +806,27 @@ static void guc_policies_init(struct guc_policies *policies)
>  	policies->is_valid = 1;
>  }
>  
> +#define p_offset(ptr, member) offsetof(typeof(*ptr), member)
> +

I would place this helper macro in i915_utils.h ;)

-Michal

>  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 {
> +		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 +834,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 +845,27 @@ 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);
> -
> -	ads->scheduler_policies =
> -		guc_ggtt_offset(vma) + sizeof(struct guc_ads);
> +	guc_policies_init(&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);
> -
> -	ads->reg_state_buffer = ads->reg_state_addr +
> -			sizeof(struct guc_mmio_reg_state);
> +	blob->ads.scheduler_policies = offset + p_offset(blob, policies);
> +	blob->ads.reg_state_addr = offset + p_offset(blob, reg_state);
> +	blob->ads.reg_state_buffer = offset + p_offset(blob, reg_state_buffer);
>  
>  	kunmap(page);
>  }
> -- 
> 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Wed, Mar 15, 2017 at 12:02:21PM +0100, Michal Wajdeczko wrote:
> On Tue, Mar 14, 2017 at 08:53:02PM +0000, Chris Wilson wrote:
> > From: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > 
> > 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)
> > v3: restyle offset assignments (Chris)
> > v4: stylistic reductions
> > 
> > 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>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_guc_submission.c | 51 ++++++++++++++----------------
> >  1 file changed, 23 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> > index b4d24cd7639a..3651a1b0d13c 100644
> > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > @@ -806,26 +806,27 @@ static void guc_policies_init(struct guc_policies *policies)
> >  	policies->is_valid = 1;
> >  }
> >  
> > +#define p_offset(ptr, member) offsetof(typeof(*ptr), member)
> > +
> 
> I would place this helper macro in i915_utils.h ;)

I wanted to call it ptr_offset, more suitale for i915_utils.h, but
didn't fit into 80cols :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Michal Wajdeczko]

On Wed, Mar 15, 2017 at 11:19:01AM +0000, Chris Wilson wrote:
> On Wed, Mar 15, 2017 at 12:02:21PM +0100, Michal Wajdeczko wrote:
> > On Tue, Mar 14, 2017 at 08:53:02PM +0000, Chris Wilson wrote:
> > > From: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > 
> > > 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)
> > > v3: restyle offset assignments (Chris)
> > > v4: stylistic reductions
> > > 
> > > 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>
> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/i915_guc_submission.c | 51 ++++++++++++++----------------
> > >  1 file changed, 23 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> > > index b4d24cd7639a..3651a1b0d13c 100644
> > > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > > @@ -806,26 +806,27 @@ static void guc_policies_init(struct guc_policies *policies)
> > >  	policies->is_valid = 1;
> > >  }
> > >  
> > > +#define p_offset(ptr, member) offsetof(typeof(*ptr), member)
> > > +
> > 
> > I would place this helper macro in i915_utils.h ;)
> 
> I wanted to call it ptr_offset, more suitale for i915_utils.h, but
> didn't fit into 80cols :)

You can save few chars by renaming 'offset' into 'base'

	u32 offset;
	...
	base = i915_ggtt_offset(vma);
	...
	blob->ads.reg_state_buffer = base + ptr_offset(blob, reg_state_buffer);

then we are exactly at 80 column.

This will also make the code more cleaner as offset + offset didn't look good.

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

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

[Chris Wilson]

On Wed, Mar 15, 2017 at 12:44:11PM +0100, Michal Wajdeczko wrote:
> On Wed, Mar 15, 2017 at 11:19:01AM +0000, Chris Wilson wrote:
> > On Wed, Mar 15, 2017 at 12:02:21PM +0100, Michal Wajdeczko wrote:
> > > On Tue, Mar 14, 2017 at 08:53:02PM +0000, Chris Wilson wrote:
> > > > From: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > > 
> > > > 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)
> > > > v3: restyle offset assignments (Chris)
> > > > v4: stylistic reductions
> > > > 
> > > > 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>
> > > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_guc_submission.c | 51 ++++++++++++++----------------
> > > >  1 file changed, 23 insertions(+), 28 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> > > > index b4d24cd7639a..3651a1b0d13c 100644
> > > > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > > > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > > > @@ -806,26 +806,27 @@ static void guc_policies_init(struct guc_policies *policies)
> > > >  	policies->is_valid = 1;
> > > >  }
> > > >  
> > > > +#define p_offset(ptr, member) offsetof(typeof(*ptr), member)
> > > > +
> > > 
> > > I would place this helper macro in i915_utils.h ;)
> > 
> > I wanted to call it ptr_offset, more suitale for i915_utils.h, but
> > didn't fit into 80cols :)
> 
> You can save few chars by renaming 'offset' into 'base'
> 
> 	u32 offset;
> 	...
> 	base = i915_ggtt_offset(vma);
> 	...
> 	blob->ads.reg_state_buffer = base + ptr_offset(blob, reg_state_buffer);
> 
> then we are exactly at 80 column.
> 
> This will also make the code more cleaner as offset + offset didn't look good.

base + offset is better. Hmm, went with ptr_offsetof() since I had to
introduce a newline and I thought similarity with the underlying macro
would be sensible for i915_utils.h
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

From: Michal Wajdeczko <michal.wajdeczko@intel.com>

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)
v3: restyle offset assignments (Chris)
v4: stylistic reductions

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>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 69 ++++++++++++++----------------
 drivers/gpu/drm/i915/i915_utils.h          |  2 +
 2 files changed, 33 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index b4d24cd7639a..5ec2cbda2ba9 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 {
+		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;
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+	u32 base;
 
 	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,44 +832,38 @@ static void guc_addon_create(struct intel_guc *guc)
 	}
 
 	page = i915_vma_first_page(vma);
-	ads = kmap(page);
-
-	/*
-	 * 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.
-	 */
-	engine = dev_priv->engine[RCS];
-	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 = kmap(page);
 
 	/* GuC scheduling policies */
-	policies = (void *)ads + sizeof(struct guc_ads);
-	guc_policies_init(policies);
-
-	ads->scheduler_policies =
-		guc_ggtt_offset(vma) + sizeof(struct guc_ads);
+	guc_policies_init(&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);
+	/*
+	 * 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.
+	 */
+	blob->ads.golden_context_lrca =
+		dev_priv->engine[RCS]->status_page.ggtt_offset;
+
+	for_each_engine(engine, dev_priv, id)
+		blob->ads.eng_state_size[engine->guc_id] =
+			intel_lr_context_size(engine);
 
-	ads->reg_state_buffer = ads->reg_state_addr +
-			sizeof(struct guc_mmio_reg_state);
+	base = guc_ggtt_offset(vma);
+	blob->ads.scheduler_policies = base + ptr_offset(blob, policies);
+	blob->ads.reg_state_buffer = base + ptr_offset(blob, reg_state_buffer);
+	blob->ads.reg_state_addr = base + ptr_offset(blob, reg_state);
 
 	kunmap(page);
 }
diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
index b8ba0f2f92af..94a3a3299910 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -66,6 +66,8 @@
 #define ptr_pack_bits(ptr, bits)					\
 	((typeof(ptr))((unsigned long)(ptr) | (bits)))
 
+#define ptr_offset(ptr, member) offsetof(typeof(*(ptr)), member)
+
 #define fetch_and_zero(ptr) ({						\
 	typeof(*ptr) __T = *(ptr);					\
 	*(ptr) = (typeof(*ptr))0;					\
-- 
2.11.0

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

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

[Joonas Lahtinen]

On ke, 2017-03-15 at 11:49 +0000, Chris Wilson wrote:
> > From: Michal Wajdeczko <michal.wajdeczko@intel.com>
> 
> 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)
> v3: restyle offset assignments (Chris)
> v4: stylistic reductions
> 
> 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>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

This code definitely makes the computations better.

<SNIP>

> @@ -833,44 +832,38 @@ static void guc_addon_create(struct intel_guc *guc)
>  	}
>  
>  	page = i915_vma_first_page(vma);
> -	ads = kmap(page);

<SNIP>

> +	blob = kmap(page);

I would still like to see a BUILD_BUG_ON to verify that everything
addressed through blob truly fits within the first page.

Other than that;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915/guc: Use formalized struct definition for ads object (rev4)

[Patchwork]

== Series Details ==

Series: drm/i915/guc: Use formalized struct definition for ads object (rev4)
URL   : https://patchwork.freedesktop.org/series/20826/
State : failure

== Summary ==

Series 20826v4 drm/i915/guc: Use formalized struct definition for ads object
https://patchwork.freedesktop.org/api/1.0/series/20826/revisions/4/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007
        Subgroup basic-uc-pro-default:
                pass       -> INCOMPLETE (fi-skl-6700hq) fdo#100130
        Subgroup basic-uc-ro-default:
                pass       -> INCOMPLETE (fi-skl-6260u)
                pass       -> INCOMPLETE (fi-skl-6770hq) fdo#100130
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                fail       -> PASS       (fi-snb-2520m)

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100130 https://bugs.freedesktop.org/show_bug.cgi?id=100130

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 457s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 577s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 531s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 548s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 502s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 500s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 441s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 499s
fi-skl-6260u     total:55   pass:53   dwarn:0   dfail:0   fail:0   skip:1   time: 0s
fi-skl-6700hq    total:53   pass:45   dwarn:0   dfail:0   fail:0   skip:7   time: 0s
fi-skl-6770hq    total:55   pass:53   dwarn:0   dfail:0   fail:0   skip:1   time: 0s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 546s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time: 412s

c641417b70c6b78efca29ae732d7cbf5716ac6d5 drm-tip: 2017y-03m-14d-16h-04m-56s UTC integration manifest
0b960a5 drm/i915/guc: Use formalized struct definition for ads object

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4172/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915/guc: Use formalized struct definition for ads object (rev4)

[Patchwork]

== Series Details ==

Series: drm/i915/guc: Use formalized struct definition for ads object (rev4)
URL   : https://patchwork.freedesktop.org/series/20826/
State : success

== Summary ==

Series 20826v4 drm/i915/guc: Use formalized struct definition for ads object
https://patchwork.freedesktop.org/api/1.0/series/20826/revisions/4/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007
        Subgroup basic-uc-pro-default:
                pass       -> INCOMPLETE (fi-skl-6700hq) fdo#100130
        Subgroup basic-uc-ro-default:
                pass       -> INCOMPLETE (fi-skl-6770hq) fdo#100130
                pass       -> INCOMPLETE (fi-skl-6260u) fdo#100130
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                fail       -> PASS       (fi-snb-2520m)

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100130 https://bugs.freedesktop.org/show_bug.cgi?id=100130

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 457s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 577s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 531s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 548s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 502s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 500s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 441s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 499s
fi-skl-6260u     total:55   pass:53   dwarn:0   dfail:0   fail:0   skip:1   time: 0s
fi-skl-6700hq    total:53   pass:45   dwarn:0   dfail:0   fail:0   skip:7   time: 0s
fi-skl-6770hq    total:55   pass:53   dwarn:0   dfail:0   fail:0   skip:1   time: 0s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 546s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time: 412s

c641417b70c6b78efca29ae732d7cbf5716ac6d5 drm-tip: 2017y-03m-14d-16h-04m-56s UTC integration manifest
0b960a5 drm/i915/guc: Use formalized struct definition for ads object

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4172/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915/guc: Use formalized struct definition for ads object (rev5)

[Patchwork]

== Series Details ==

Series: drm/i915/guc: Use formalized struct definition for ads object (rev5)
URL   : https://patchwork.freedesktop.org/series/20826/
State : success

== Summary ==

Series 20826v5 drm/i915/guc: Use formalized struct definition for ads object
https://patchwork.freedesktop.org/api/1.0/series/20826/revisions/5/mbox/

Test gem_ctx_create:
        Subgroup basic-files:
                incomplete -> PASS       (fi-skl-6260u) fdo#100130
Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                incomplete -> PASS       (fi-skl-6700k) fdo#100130
        Subgroup basic-uc-pro-default:
                incomplete -> PASS       (fi-skl-6770hq) fdo#100130
        Subgroup basic-wb-ro-before-default:
                incomplete -> PASS       (fi-kbl-7500u) fdo#100130

fdo#100130 https://bugs.freedesktop.org/show_bug.cgi?id=100130

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 457s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 576s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 534s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 500s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 499s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 431s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 510s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 495s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 487s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 481s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 489s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 522s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 549s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 421s

15bd1396bee591d573aad0f305dd0d9915d4a884 drm-tip: 2017y-03m-15d-10h-42m-31s UTC integration manifest
b3ad3bb drm/i915/guc: Use formalized struct definition for ads object

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4179/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✓ Fi.CI.BAT: success for drm/i915/guc: Use formalized struct definition for ads object (rev5)

[Chris Wilson]

On Wed, Mar 15, 2017 at 12:46:57PM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915/guc: Use formalized struct definition for ads object (rev5)
> URL   : https://patchwork.freedesktop.org/series/20826/
> State : success
> 
> == Summary ==
> 
> Series 20826v5 drm/i915/guc: Use formalized struct definition for ads object
> https://patchwork.freedesktop.org/api/1.0/series/20826/revisions/5/mbox/
> 
> Test gem_ctx_create:
>         Subgroup basic-files:
>                 incomplete -> PASS       (fi-skl-6260u) fdo#100130
> Test gem_exec_flush:
>         Subgroup basic-batch-kernel-default-uc:
>                 incomplete -> PASS       (fi-skl-6700k) fdo#100130
>         Subgroup basic-uc-pro-default:
>                 incomplete -> PASS       (fi-skl-6770hq) fdo#100130
>         Subgroup basic-wb-ro-before-default:
>                 incomplete -> PASS       (fi-kbl-7500u) fdo#100130
> 
> fdo#100130 https://bugs.freedesktop.org/show_bug.cgi?id=100130

Pushed. Didn't require the major rebase surgery as I expected. The ads
patch must be in the other guc cleanup series!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Michal Wajdeczko]

Manual pointer manipulation is error prone. Let compiler calculate
right offsets for us in case we need to change ads layout.

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 | 49 ++++++++++++++----------------
 1 file changed, 23 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index beb38e3..f87649b 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_object {
+		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 *obj;
+	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(*obj)));
 		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);
+	obj = kmap(page);
+	offset = i915_ggtt_offset(vma);
 
 	/*
 	 * The GuC requires a "Golden Context" when it reinitialises
@@ -843,34 +843,31 @@ 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;
+	obj->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);
+		obj->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(&obj->policies);
 
-	ads->scheduler_policies =
-		guc_ggtt_offset(vma) + sizeof(struct guc_ads);
+	obj->ads.scheduler_policies = offset +
+		offsetof(struct __guc_ads_object, 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 =
+		obj->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;
+		obj->reg_state.mmio_white_list[engine->guc_id].count = 0;
 	}
 
-	ads->reg_state_addr = ads->scheduler_policies +
-			sizeof(struct guc_policies);
+	obj->ads.reg_state_addr = offset +
+		offsetof(struct __guc_ads_object, reg_state);
 
-	ads->reg_state_buffer = ads->reg_state_addr +
-			sizeof(struct guc_mmio_reg_state);
+	obj->ads.reg_state_buffer = offset +
+		offsetof(struct __guc_ads_object, reg_state_buffer);
 
 	kunmap(page);
 }
-- 
2.7.4

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

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

[Chris Wilson]

On Tue, Mar 07, 2017 at 10:29:35AM +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.
> 
> 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 | 49 ++++++++++++++----------------
>  1 file changed, 23 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index beb38e3..f87649b 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_object {
> +		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 *obj;

A humble request not to call this obj. I was confused later and worrying
about what you were adding to drm_i915_gem_object.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Michal Wajdeczko]

On Tue, Mar 07, 2017 at 10:39:46AM +0000, Chris Wilson wrote:
> On Tue, Mar 07, 2017 at 10:29:35AM +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.
> > 
> > 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 | 49 ++++++++++++++----------------
> >  1 file changed, 23 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> > index beb38e3..f87649b 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_object {
> > +		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 *obj;
> 
> A humble request not to call this obj. I was confused later and worrying
> about what you were adding to drm_i915_gem_object.

Agree, but I just followed the name used in existing comment.
What about:

	struct __guc_ads_blob {
	...
	} __packed *blob;

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