| Message ID | 20170307102935.120596-1-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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); }
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(-)