Quoting Michel Thierry (2017-07-12 20:30:31)
> Not only the context image consist of two parts (the PPHWSP, and the
> logical context state), but we also allocate a header at the start of
> for sharing data with GuC. Thus every lrc looks like this:
>
> | [guc] | [hwsp] [logical state] |
> |<- our header ->|<- context image ->|
>
> So far, we have oversimplified whenever we use each of these parts of the
> context, just because the GuC header happens to be in page 0, and the
> (PP)HWSP is in page 1. But this had led to using the same define for more
> than one meaning (as a page index in the lrc and as 1 page).
>
> This patch adds defines for the GuC shared page, the PPHWSP page and the
> start of the logical state. It also updated the places where the old
> define was being used. Since we are not changing the size (or format) of
> the context, there are no functional changes.
>
> Suggested-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Michel Thierry <michel.thierry@xxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> Cc: intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx
> ---
> drivers/gpu/drm/i915/gvt/scheduler.c | 4 ++--
> drivers/gpu/drm/i915/i915_guc_submission.c | 4 ++--
> drivers/gpu/drm/i915/intel_lrc.c | 11 +++++++----
> drivers/gpu/drm/i915/intel_lrc.h | 25 ++++++++++++++++++++++---
> 4 files changed, 33 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
> index 0e2e36ad6196..8e9eeff156f6 100644
> --- a/drivers/gpu/drm/i915/gvt/scheduler.c
> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
> @@ -87,7 +87,7 @@ static int populate_shadow_context(struct intel_vgpu_workload *workload)
> return -EINVAL;
> }
>
> - page = i915_gem_object_get_page(ctx_obj, LRC_PPHWSP_PN + i);
> + page = i915_gem_object_get_page(ctx_obj, LRC_HEADER_PAGES + i);
Ah, that makes much more sense.
> dst = kmap(page);
> intel_gvt_hypervisor_read_gpa(vgpu, context_gpa, dst,
> GTT_PAGE_SIZE);
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 48a1e9349a2c..b7ca13860677 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -1310,7 +1310,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
> /* 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);
> + data[2] = guc_ggtt_offset(ctx->engine[RCS].state) + LRC_GUCSHR_PN * PAGE_SIZE;
>
> return intel_guc_send(guc, data, ARRAY_SIZE(data));
> }
> @@ -1336,7 +1336,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);
> + data[2] = guc_ggtt_offset(ctx->engine[RCS].state) + LRC_GUCSHR_PN * PAGE_SIZE;
Could spot any others, so lgtm.
> return intel_guc_send(guc, data, ARRAY_SIZE(data));
> }
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 699868d81de8..67a1187b0afe 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -279,7 +279,7 @@ intel_lr_context_descriptor_update(struct i915_gem_context *ctx,
> BUILD_BUG_ON(MAX_CONTEXT_HW_ID > (1<<GEN8_CTX_ID_WIDTH));
>
> desc = ctx->desc_template; /* bits 0-11 */
> - desc |= i915_ggtt_offset(ce->state) + LRC_PPHWSP_PN * PAGE_SIZE;
Arguable that this could be PPHWSP since that is the start of the
context image for execlists. HEADER is also an apt description, so 50/50.
Actually favouring using HEADER, so keep the change.
> + desc |= i915_ggtt_offset(ce->state) + LRC_HEADER_PAGES * PAGE_SIZE;
> /* bits 12-31 */
> desc |= (u64)ctx->hw_id << GEN8_CTX_ID_SHIFT; /* bits 32-52 */
>
> @@ -1695,7 +1695,7 @@ logical_ring_default_irqs(struct intel_engine_cs *engine)
> static int
> lrc_setup_hws(struct intel_engine_cs *engine, struct i915_vma *vma)
> {
> - const int hws_offset = LRC_PPHWSP_PN * PAGE_SIZE;
> + const int hws_offset = LRC_HEADER_PAGES * PAGE_SIZE;
This should be PPHWSP_PN since we explicitly want the pointer tot he
pphwsp.
> void *hws;
>
> /* The HWSP is part of the default context object in LRC mode. */
> @@ -2015,8 +2015,11 @@ static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
>
> context_size = round_up(engine->context_size, I915_GTT_PAGE_SIZE);
>
> - /* One extra page as the sharing data between driver and GuC */
> - context_size += PAGE_SIZE * LRC_PPHWSP_PN;
> + /*
> + * Before the actual start of the context image, we insert a few pages
> + * for our own use and for sharing with the GuC.
> + */
> + context_size += LRC_HEADER_PAGES * PAGE_SIZE;
Good riddance, that LRC_PPHWSP_PN has left me quite confused for some
time.
With the change to lrc_setup_hws,
Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
