| Message ID | 20170712193032.27080-2-michel.thierry@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Michel Thierry (2017-07-12 20:30:32) > Using the HWSP ggtt_offset to get the lrca offset is only correct if the > HWSP happens to be before it (when we reuse the PPHWSP of the kernel > context as the engine HWSP). Instead of making this assumption, get the > lrca offset from the kernel_context engine state. > > And while looking at this part of the GuC interaction, it was also > noticed that the firmware expects the size of only the engine context > (context minus the execlist part, i.e. don't include the first 80 > dwords), so pass the right size. > > v2: Use the new macros to prevent abusive overuse of the old ones (Chris). > > Reported-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Signed-off-by: Michel Thierry <michel.thierry@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Oscar Mateo <oscar.mateo@intel.com> > --- > drivers/gpu/drm/i915/i915_guc_submission.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index b7ca13860677..8b96935cf96a 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -1018,6 +1018,12 @@ static void guc_policies_init(struct guc_policies *policies) > policies->is_valid = 1; > } > > +/* > + * The first 80 dwords of the register state context, containing the > + * execlists and ppgtt registers. > + */ > +#define LR_HW_CONTEXT_SIZE (80 * sizeof(u32)) > + > static int guc_ads_create(struct intel_guc *guc) > { > struct drm_i915_private *dev_priv = guc_to_i915(guc); > @@ -1032,6 +1038,8 @@ static int guc_ads_create(struct intel_guc *guc) > } __packed *blob; > struct intel_engine_cs *engine; > enum intel_engine_id id; > + const u32 skipped_offset = LRC_HEADER_PAGES * PAGE_SIZE; > + const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE + LR_HW_CONTEXT_SIZE; I think I preferred the constants rather than the variables, but that's a matter of taste. Since you confirmed the guc works the way you describe and now have a big warning in place, Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Though I would get Daniele to confirm the interpretation. -Chris
On 12/07/17 12:49, Chris Wilson wrote: > Quoting Michel Thierry (2017-07-12 20:30:32) >> Using the HWSP ggtt_offset to get the lrca offset is only correct if the >> HWSP happens to be before it (when we reuse the PPHWSP of the kernel >> context as the engine HWSP). Instead of making this assumption, get the >> lrca offset from the kernel_context engine state. >> >> And while looking at this part of the GuC interaction, it was also >> noticed that the firmware expects the size of only the engine context >> (context minus the execlist part, i.e. don't include the first 80 >> dwords), so pass the right size. >> >> v2: Use the new macros to prevent abusive overuse of the old ones (Chris). >> >> Reported-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Signed-off-by: Michel Thierry <michel.thierry@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Oscar Mateo <oscar.mateo@intel.com> >> --- >> drivers/gpu/drm/i915/i915_guc_submission.c | 21 ++++++++++++++++++--- >> 1 file changed, 18 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c >> index b7ca13860677..8b96935cf96a 100644 >> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c >> @@ -1018,6 +1018,12 @@ static void guc_policies_init(struct guc_policies *policies) >> policies->is_valid = 1; >> } >> >> +/* >> + * The first 80 dwords of the register state context, containing the >> + * execlists and ppgtt registers. >> + */ >> +#define LR_HW_CONTEXT_SIZE (80 * sizeof(u32)) >> + >> static int guc_ads_create(struct intel_guc *guc) >> { >> struct drm_i915_private *dev_priv = guc_to_i915(guc); >> @@ -1032,6 +1038,8 @@ static int guc_ads_create(struct intel_guc *guc) >> } __packed *blob; >> struct intel_engine_cs *engine; >> enum intel_engine_id id; >> + const u32 skipped_offset = LRC_HEADER_PAGES * PAGE_SIZE; >> + const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE + LR_HW_CONTEXT_SIZE; > > I think I preferred the constants rather than the variables, but that's > a matter of taste. Since you confirmed the guc works the way you > describe and now have a big warning in place, > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > Though I would get Daniele to confirm the interpretation. > -Chris > LGTM. Acked-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 b7ca13860677..8b96935cf96a 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -1018,6 +1018,12 @@ static void guc_policies_init(struct guc_policies *policies) policies->is_valid = 1; } +/* + * The first 80 dwords of the register state context, containing the + * execlists and ppgtt registers. + */ +#define LR_HW_CONTEXT_SIZE (80 * sizeof(u32)) + static int guc_ads_create(struct intel_guc *guc) { struct drm_i915_private *dev_priv = guc_to_i915(guc); @@ -1032,6 +1038,8 @@ static int guc_ads_create(struct intel_guc *guc) } __packed *blob; struct intel_engine_cs *engine; enum intel_engine_id id; + const u32 skipped_offset = LRC_HEADER_PAGES * PAGE_SIZE; + const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE + LR_HW_CONTEXT_SIZE; u32 base; GEM_BUG_ON(guc->ads_vma); @@ -1062,13 +1070,20 @@ static int guc_ads_create(struct intel_guc *guc) * 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. + * to find it. Note that we have to skip our header (1 page), + * because our GuC shared data is there. */ blob->ads.golden_context_lrca = - dev_priv->engine[RCS]->status_page.ggtt_offset; + guc_ggtt_offset(dev_priv->kernel_context->engine[RCS].state) + skipped_offset; + /* + * The GuC expects us to exclude the portion of the context image that + * it skips from the size it is to read. It starts reading from after + * the execlist context (so skipping the first page [PPHWSP] and 80 + * dwords). Weird guc is weird. + */ for_each_engine(engine, dev_priv, id) - blob->ads.eng_state_size[engine->guc_id] = engine->context_size; + blob->ads.eng_state_size[engine->guc_id] = engine->context_size - skipped_size; base = guc_ggtt_offset(vma); blob->ads.scheduler_policies = base + ptr_offset(blob, policies);
Using the HWSP ggtt_offset to get the lrca offset is only correct if the HWSP happens to be before it (when we reuse the PPHWSP of the kernel context as the engine HWSP). Instead of making this assumption, get the lrca offset from the kernel_context engine state. And while looking at this part of the GuC interaction, it was also noticed that the firmware expects the size of only the engine context (context minus the execlist part, i.e. don't include the first 80 dwords), so pass the right size. v2: Use the new macros to prevent abusive overuse of the old ones (Chris). Reported-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Signed-off-by: Michel Thierry <michel.thierry@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Oscar Mateo <oscar.mateo@intel.com> --- drivers/gpu/drm/i915/i915_guc_submission.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)