[Intel-gfx] [PATCH 1/2] drm/i915/lrc: Clarify the format of the context image

[Chris Wilson]

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 at chris-wilson.co.uk>
> Signed-off-by: Michel Thierry <michel.thierry at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Oscar Mateo <oscar.mateo at intel.com>
> Cc: intel-gvt-dev at lists.freedesktop.org
> ---
>  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 at chris-wilson.co.uk>
-Chris