| Message ID | 1487944897-31558-4-git-send-email-oscar.mateo@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On pe, 2017-02-24 at 06:01 -0800, Oscar Mateo wrote: > This vma contains much more than just the additional data struct (ads) > and since we were already using the word "addon" as an object in > guc_addon_create, make it the preffered one. No need for the vma suffix > either, as that dependency is given by the type. > > while at it, add an explanation of what things go inside the addon object. > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> <SNIP> > @@ -834,9 +834,13 @@ static int guc_addon_create(struct intel_guc *guc) > struct page *page; > u32 size; > > - GEM_BUG_ON(guc->ads_vma); > + GEM_BUG_ON(guc->addon); > > - /* The ads obj includes the struct itself and buffers passed to GuC */ > + /* The additional data struct (ADS) has pointers for different buffers > + * used by the GuC. The addon object contains the ADS itself (guc_ads), > + * the scheduling policies (guc_policies), a structure describing > + * a collection of register sets (guc_mmio_reg_state) and some extra > + * pages for the GuC to save its internal state for sleep */ Put this comment in the header and this is; Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > size = sizeof(struct guc_ads) + sizeof(struct guc_policies) + > sizeof(struct guc_mmio_reg_state) + > GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE;
On Fri, Feb 24, 2017 at 06:01:36AM -0800, Oscar Mateo wrote: > This vma contains much more than just the additional data struct (ads) > and since we were already using the word "addon" as an object in > guc_addon_create, make it the preffered one. No need for the vma suffix > either, as that dependency is given by the type. Hmm, but almost all over the driver we're using "vma" as a name or suffix. Why do you want to change this here ? Also, GUC still is using term "ADS" for this blob so I'm not convinced that we need new name here. -Michal > > while at it, add an explanation of what things go inside the addon object. > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > --- > drivers/gpu/drm/i915/i915_guc_submission.c | 12 ++++++++---- > drivers/gpu/drm/i915/intel_guc_loader.c | 4 ++-- > drivers/gpu/drm/i915/intel_uc.h | 2 +- > 3 files changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index 7562343c..e1922fe 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -834,9 +834,13 @@ static int guc_addon_create(struct intel_guc *guc) > struct page *page; > u32 size; > > - GEM_BUG_ON(guc->ads_vma); > + GEM_BUG_ON(guc->addon); > > - /* The ads obj includes the struct itself and buffers passed to GuC */ > + /* The additional data struct (ADS) has pointers for different buffers > + * used by the GuC. The addon object contains the ADS itself (guc_ads), > + * the scheduling policies (guc_policies), a structure describing > + * a collection of register sets (guc_mmio_reg_state) and some extra > + * pages for the GuC to save its internal state for sleep */ > size = sizeof(struct guc_ads) + sizeof(struct guc_policies) + > sizeof(struct guc_mmio_reg_state) + > GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE; > @@ -845,7 +849,7 @@ static int guc_addon_create(struct intel_guc *guc) > if (IS_ERR(vma)) > return PTR_ERR(vma); > > - guc->ads_vma = vma; > + guc->addon = vma; > > page = i915_vma_first_page(vma); > ads = kmap(page); > @@ -894,7 +898,7 @@ static int guc_addon_create(struct intel_guc *guc) > > static void guc_addon_destroy(struct intel_guc *guc) > { > - i915_vma_unpin_and_release(&guc->ads_vma); > + i915_vma_unpin_and_release(&guc->addon); > } > > /* > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index 1f9ec54..1eb0c51 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -213,8 +213,8 @@ static void guc_params_init(struct drm_i915_private *dev_priv) > } else > params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED; > > - if (guc->ads_vma) { > - u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT; > + if (guc->addon) { > + u32 ads = guc_ggtt_offset(guc->addon) >> PAGE_SHIFT; > params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT; > params[GUC_CTL_DEBUG] |= GUC_ADS_ENABLED; > } > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h > index 330d08f..d8897b5 100644 > --- a/drivers/gpu/drm/i915/intel_uc.h > +++ b/drivers/gpu/drm/i915/intel_uc.h > @@ -152,7 +152,7 @@ struct intel_guc { > /* intel_guc_recv interrupt related state */ > bool interrupts_enabled; > > - struct i915_vma *ads_vma; > + struct i915_vma *addon; > struct i915_vma *ctx_pool; > void *ctx_pool_vaddr; > struct ida ctx_ids; > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 03/07/2017 07:07 AM, Michal Wajdeczko wrote: > On Fri, Feb 24, 2017 at 06:01:36AM -0800, Oscar Mateo wrote: >> This vma contains much more than just the additional data struct (ads) >> and since we were already using the word "addon" as an object in >> guc_addon_create, make it the preffered one. No need for the vma suffix >> either, as that dependency is given by the type. > Hmm, but almost all over the driver we're using "vma" as a name or suffix. > Why do you want to change this here ? Same reason why I changed ctx_pool_vma to ctx_pool: because the type (i915_vma) already tells you everything you need to know (suggestion by Joonas). > Also, GUC still is using term "ADS" for this blob so I'm not convinced > that we need new name here. > > -Michal > We still keep the name "ads" in this patch, but we use it to refer to the struct the GuC expects, while "addon" is the GEM object that contains both the ads struct and all the additional stuff the GuC needs. But I could equally keep the ads name and s/guc_addon_create/guc_ads_create (after all, blue or red, a shed is a shed). >> while at it, add an explanation of what things go inside the addon object. >> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> >> --- >> drivers/gpu/drm/i915/i915_guc_submission.c | 12 ++++++++---- >> drivers/gpu/drm/i915/intel_guc_loader.c | 4 ++-- >> drivers/gpu/drm/i915/intel_uc.h | 2 +- >> 3 files changed, 11 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c >> index 7562343c..e1922fe 100644 >> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c >> @@ -834,9 +834,13 @@ static int guc_addon_create(struct intel_guc *guc) >> struct page *page; >> u32 size; >> >> - GEM_BUG_ON(guc->ads_vma); >> + GEM_BUG_ON(guc->addon); >> >> - /* The ads obj includes the struct itself and buffers passed to GuC */ >> + /* The additional data struct (ADS) has pointers for different buffers >> + * used by the GuC. The addon object contains the ADS itself (guc_ads), >> + * the scheduling policies (guc_policies), a structure describing >> + * a collection of register sets (guc_mmio_reg_state) and some extra >> + * pages for the GuC to save its internal state for sleep */ >> size = sizeof(struct guc_ads) + sizeof(struct guc_policies) + >> sizeof(struct guc_mmio_reg_state) + >> GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE; >> @@ -845,7 +849,7 @@ static int guc_addon_create(struct intel_guc *guc) >> if (IS_ERR(vma)) >> return PTR_ERR(vma); >> >> - guc->ads_vma = vma; >> + guc->addon = vma; >> >> page = i915_vma_first_page(vma); >> ads = kmap(page); >> @@ -894,7 +898,7 @@ static int guc_addon_create(struct intel_guc *guc) >> >> static void guc_addon_destroy(struct intel_guc *guc) >> { >> - i915_vma_unpin_and_release(&guc->ads_vma); >> + i915_vma_unpin_and_release(&guc->addon); >> } >> >> /* >> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c >> index 1f9ec54..1eb0c51 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_loader.c >> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c >> @@ -213,8 +213,8 @@ static void guc_params_init(struct drm_i915_private *dev_priv) >> } else >> params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED; >> >> - if (guc->ads_vma) { >> - u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT; >> + if (guc->addon) { >> + u32 ads = guc_ggtt_offset(guc->addon) >> PAGE_SHIFT; >> params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT; >> params[GUC_CTL_DEBUG] |= GUC_ADS_ENABLED; >> } >> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h >> index 330d08f..d8897b5 100644 >> --- a/drivers/gpu/drm/i915/intel_uc.h >> +++ b/drivers/gpu/drm/i915/intel_uc.h >> @@ -152,7 +152,7 @@ struct intel_guc { >> /* intel_guc_recv interrupt related state */ >> bool interrupts_enabled; >> >> - struct i915_vma *ads_vma; >> + struct i915_vma *addon; >> struct i915_vma *ctx_pool; >> void *ctx_pool_vaddr; >> struct ida ctx_ids; >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 7562343c..e1922fe 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -834,9 +834,13 @@ static int guc_addon_create(struct intel_guc *guc) struct page *page; u32 size; - GEM_BUG_ON(guc->ads_vma); + GEM_BUG_ON(guc->addon); - /* The ads obj includes the struct itself and buffers passed to GuC */ + /* The additional data struct (ADS) has pointers for different buffers + * used by the GuC. The addon object contains the ADS itself (guc_ads), + * the scheduling policies (guc_policies), a structure describing + * a collection of register sets (guc_mmio_reg_state) and some extra + * pages for the GuC to save its internal state for sleep */ size = sizeof(struct guc_ads) + sizeof(struct guc_policies) + sizeof(struct guc_mmio_reg_state) + GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE; @@ -845,7 +849,7 @@ static int guc_addon_create(struct intel_guc *guc) if (IS_ERR(vma)) return PTR_ERR(vma); - guc->ads_vma = vma; + guc->addon = vma; page = i915_vma_first_page(vma); ads = kmap(page); @@ -894,7 +898,7 @@ static int guc_addon_create(struct intel_guc *guc) static void guc_addon_destroy(struct intel_guc *guc) { - i915_vma_unpin_and_release(&guc->ads_vma); + i915_vma_unpin_and_release(&guc->addon); } /* diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 1f9ec54..1eb0c51 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -213,8 +213,8 @@ static void guc_params_init(struct drm_i915_private *dev_priv) } else params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED; - if (guc->ads_vma) { - u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT; + if (guc->addon) { + u32 ads = guc_ggtt_offset(guc->addon) >> PAGE_SHIFT; params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT; params[GUC_CTL_DEBUG] |= GUC_ADS_ENABLED; } diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h index 330d08f..d8897b5 100644 --- a/drivers/gpu/drm/i915/intel_uc.h +++ b/drivers/gpu/drm/i915/intel_uc.h @@ -152,7 +152,7 @@ struct intel_guc { /* intel_guc_recv interrupt related state */ bool interrupts_enabled; - struct i915_vma *ads_vma; + struct i915_vma *addon; struct i915_vma *ctx_pool; void *ctx_pool_vaddr; struct ida ctx_ids;
This vma contains much more than just the additional data struct (ads) and since we were already using the word "addon" as an object in guc_addon_create, make it the preffered one. No need for the vma suffix either, as that dependency is given by the type. while at it, add an explanation of what things go inside the addon object. Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> --- drivers/gpu/drm/i915/i915_guc_submission.c | 12 ++++++++---- drivers/gpu/drm/i915/intel_guc_loader.c | 4 ++-- drivers/gpu/drm/i915/intel_uc.h | 2 +- 3 files changed, 11 insertions(+), 7 deletions(-)