| Message ID | 20171009145258.23303-3-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/10/17 07:52, Michał Winiarski wrote: > We were using first page of kernel context render state for sharing data > with GuC. While it's justified by the fact that those pages are not used > (note, GuC still enforces this layout and refuses to work if we remove > the extra page in front), it's also confusing (why are we using this > particular page?). Let's allocate a separate object instead. > > Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Jeff McGee <jeff.mcgee@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Oscar Mateo <oscar.mateo@intel.com> +Michel (engine and watchdog reset with GuC use the shared page) > --- > drivers/gpu/drm/i915/i915_guc_submission.c | 36 +++++++++++++++++++++++++++++- > drivers/gpu/drm/i915/intel_guc.c | 8 ++----- > drivers/gpu/drm/i915/intel_guc.h | 2 ++ > 3 files changed, 39 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index 8983d53af229..30f026566001 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -437,6 +437,33 @@ static void guc_stage_desc_fini(struct intel_guc *guc, > memset(desc, 0, sizeof(*desc)); > } > > +static int guc_shared_data_create(struct intel_guc *guc) > +{ > + struct i915_vma *vma; > + void *vaddr; > + > + vma = intel_guc_allocate_vma(guc, PAGE_SIZE); > + if (IS_ERR(vma)) > + return PTR_ERR(vma); > + > + vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB); > + if (IS_ERR(vaddr)) { > + i915_vma_unpin_and_release(&vma); > + return PTR_ERR(vaddr); > + } > + > + guc->shared_data = vma; > + guc->shared_data_vaddr = vaddr; I've noticed that this is now the 3rd place where we allocate and immediately pin a vma for guc (the other 2 being guc_stage_desc_pool_create and ctch_init). Maybe we can move the 2 operations to a more common helper (and do the same for the cleanup), but it's probably better to do it after all the ongoing GuC re-org has settled down. In the meantime, this patch is: Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Daniele
On 10/9/2017 11:41 AM, Daniele Ceraolo Spurio wrote: > > > On 09/10/17 07:52, Michał Winiarski wrote: >> We were using first page of kernel context render state for sharing data >> with GuC. While it's justified by the fact that those pages are not used >> (note, GuC still enforces this layout and refuses to work if we remove >> the extra page in front), it's also confusing (why are we using this >> particular page?). Let's allocate a separate object instead. >> >> Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Jeff McGee <jeff.mcgee@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Oscar Mateo <oscar.mateo@intel.com> > > +Michel (engine and watchdog reset with GuC use the shared page) > >> --- >> drivers/gpu/drm/i915/i915_guc_submission.c | 36 >> +++++++++++++++++++++++++++++- >> drivers/gpu/drm/i915/intel_guc.c | 8 ++----- >> drivers/gpu/drm/i915/intel_guc.h | 2 ++ >> 3 files changed, 39 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c >> b/drivers/gpu/drm/i915/i915_guc_submission.c >> index 8983d53af229..30f026566001 100644 >> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c >> @@ -437,6 +437,33 @@ static void guc_stage_desc_fini(struct intel_guc >> *guc, >> memset(desc, 0, sizeof(*desc)); >> } >> +static int guc_shared_data_create(struct intel_guc *guc) >> +{ >> + struct i915_vma *vma; >> + void *vaddr; >> + >> + vma = intel_guc_allocate_vma(guc, PAGE_SIZE); >> + if (IS_ERR(vma)) >> + return PTR_ERR(vma); >> + >> + vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB); >> + if (IS_ERR(vaddr)) { >> + i915_vma_unpin_and_release(&vma); >> + return PTR_ERR(vaddr); >> + } >> + >> + guc->shared_data = vma; >> + guc->shared_data_vaddr = vaddr; Hi, Allocating the shared_data until this point (i915_guc_submission_init) will be too late for GuC's watchdog. GuC watchdog happens without i915 knowledge, so we have to pass this shared_data_offset during guc_params_init (in params[9] for the curious) instead of a h2g command; and the GuC parameters block has this note: "These parameters are read by the firmware on startup and cannot be changed thereafter". Michał, if you plan to send another version of this, could you move it to guc_params_init? It isn't a big issue, I can just move it when we have an open source user and can upstream GuC watchdog. Thanks, -Michel > > I've noticed that this is now the 3rd place where we allocate and > immediately pin a vma for guc (the other 2 being > guc_stage_desc_pool_create and ctch_init). Maybe we can move the 2 > operations to a more common helper (and do the same for the cleanup), > but it's probably better to do it after all the ongoing GuC re-org has > settled down. In the meantime, this patch is: > > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > Daniele
On 09/10/17 15:35, Michel Thierry wrote: > On 10/9/2017 11:41 AM, Daniele Ceraolo Spurio wrote: >> >> >> On 09/10/17 07:52, Michał Winiarski wrote: >>> We were using first page of kernel context render state for sharing data >>> with GuC. While it's justified by the fact that those pages are not used >>> (note, GuC still enforces this layout and refuses to work if we remove >>> the extra page in front), it's also confusing (why are we using this >>> particular page?). Let's allocate a separate object instead. >>> >>> Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>> Cc: Jeff McGee <jeff.mcgee@intel.com> >>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Cc: Oscar Mateo <oscar.mateo@intel.com> >> >> +Michel (engine and watchdog reset with GuC use the shared page) >> >>> --- >>> drivers/gpu/drm/i915/i915_guc_submission.c | 36 >>> +++++++++++++++++++++++++++++- >>> drivers/gpu/drm/i915/intel_guc.c | 8 ++----- >>> drivers/gpu/drm/i915/intel_guc.h | 2 ++ >>> 3 files changed, 39 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c >>> b/drivers/gpu/drm/i915/i915_guc_submission.c >>> index 8983d53af229..30f026566001 100644 >>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c >>> @@ -437,6 +437,33 @@ static void guc_stage_desc_fini(struct intel_guc >>> *guc, >>> memset(desc, 0, sizeof(*desc)); >>> } >>> +static int guc_shared_data_create(struct intel_guc *guc) >>> +{ >>> + struct i915_vma *vma; >>> + void *vaddr; >>> + >>> + vma = intel_guc_allocate_vma(guc, PAGE_SIZE); >>> + if (IS_ERR(vma)) >>> + return PTR_ERR(vma); >>> + >>> + vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB); >>> + if (IS_ERR(vaddr)) { >>> + i915_vma_unpin_and_release(&vma); >>> + return PTR_ERR(vaddr); >>> + } >>> + >>> + guc->shared_data = vma; >>> + guc->shared_data_vaddr = vaddr; > > Hi, > > Allocating the shared_data until this point (i915_guc_submission_init) > will be too late for GuC's watchdog. > > GuC watchdog happens without i915 knowledge, so we have to pass this > shared_data_offset during guc_params_init (in params[9] for the curious) > instead of a h2g command; and the GuC parameters block has this note: > "These parameters are read by the firmware on startup and cannot be > changed thereafter". > > Michał, if you plan to send another version of this, could you move it > to guc_params_init? It isn't a big issue, I can just move it when we > have an open source user and can upstream GuC watchdog. > > Thanks, > > -Michel > Ignore my previous reply, this is already being allocated before guc_params_init as it is. Reviewed-by: Michel Thierry <michel.thierry@intel.com> >> >> I've noticed that this is now the 3rd place where we allocate and >> immediately pin a vma for guc (the other 2 being >> guc_stage_desc_pool_create and ctch_init). Maybe we can move the 2 >> operations to a more common helper (and do the same for the cleanup), >> but it's probably better to do it after all the ongoing GuC re-org has >> settled down. In the meantime, this patch is: >> >> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> >> Daniele
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 8983d53af229..30f026566001 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -437,6 +437,33 @@ static void guc_stage_desc_fini(struct intel_guc *guc, memset(desc, 0, sizeof(*desc)); } +static int guc_shared_data_create(struct intel_guc *guc) +{ + struct i915_vma *vma; + void *vaddr; + + vma = intel_guc_allocate_vma(guc, PAGE_SIZE); + if (IS_ERR(vma)) + return PTR_ERR(vma); + + vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB); + if (IS_ERR(vaddr)) { + i915_vma_unpin_and_release(&vma); + return PTR_ERR(vaddr); + } + + guc->shared_data = vma; + guc->shared_data_vaddr = vaddr; + + return 0; +} + +static void guc_shared_data_destroy(struct intel_guc *guc) +{ + i915_gem_object_unpin_map(guc->shared_data->obj); + i915_vma_unpin_and_release(&guc->shared_data); +} + /* Construct a Work Item and append it to the GuC's Work Queue */ static void guc_wq_item_append(struct i915_guc_client *client, struct drm_i915_gem_request *rq) @@ -1011,9 +1038,13 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv) if (ret) return ret; + ret = guc_shared_data_create(guc); + if (ret) + goto err_stage_desc_pool; + ret = intel_guc_log_create(guc); if (ret < 0) - goto err_stage_desc_pool; + goto err_shared_data; ret = guc_ads_create(guc); if (ret < 0) @@ -1023,6 +1054,8 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv) err_log: intel_guc_log_destroy(guc); +err_shared_data: + guc_shared_data_destroy(guc); err_stage_desc_pool: guc_stage_desc_pool_destroy(guc); return ret; @@ -1034,6 +1067,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv) guc_ads_destroy(guc); intel_guc_log_destroy(guc); + guc_shared_data_destroy(guc); guc_stage_desc_pool_destroy(guc); } diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c index bbe4c328e9fd..93b0bdec5882 100644 --- a/drivers/gpu/drm/i915/intel_guc.c +++ b/drivers/gpu/drm/i915/intel_guc.c @@ -187,9 +187,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; - /* first page is shared data with GuC */ - data[2] = guc_ggtt_offset(ctx->engine[RCS].state) + - LRC_GUCSHR_PN * PAGE_SIZE; + data[2] = guc_ggtt_offset(guc->shared_data); return intel_guc_send(guc, data, ARRAY_SIZE(data)); } @@ -214,9 +212,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv) data[0] = INTEL_GUC_ACTION_EXIT_S_STATE; data[1] = GUC_POWER_D0; - /* first page is shared data with GuC */ - data[2] = guc_ggtt_offset(ctx->engine[RCS].state) + - LRC_GUCSHR_PN * PAGE_SIZE; + data[2] = guc_ggtt_offset(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 aa9a7b55be6e..fdbb4428b88c 100644 --- a/drivers/gpu/drm/i915/intel_guc.h +++ b/drivers/gpu/drm/i915/intel_guc.h @@ -48,6 +48,8 @@ struct intel_guc { struct i915_vma *stage_desc_pool; void *stage_desc_pool_vaddr; struct ida stage_ids; + struct i915_vma *shared_data; + void *shared_data_vaddr; struct i915_guc_client *execbuf_client;
We were using first page of kernel context render state for sharing data with GuC. While it's justified by the fact that those pages are not used (note, GuC still enforces this layout and refuses to work if we remove the extra page in front), it's also confusing (why are we using this particular page?). Let's allocate a separate object instead. Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Jeff McGee <jeff.mcgee@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Oscar Mateo <oscar.mateo@intel.com> --- drivers/gpu/drm/i915/i915_guc_submission.c | 36 +++++++++++++++++++++++++++++- drivers/gpu/drm/i915/intel_guc.c | 8 ++----- drivers/gpu/drm/i915/intel_guc.h | 2 ++ 3 files changed, 39 insertions(+), 7 deletions(-)