[Intel-gfx] [PATCH] drm/i915/guc: Don't make assumptions while getting the lrca offset

[Chris Wilson]

Quoting Michel Thierry (2017-07-11 22:29:39)
> 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.
> 
> Reported-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Signed-off-by: Michel Thierry <michel.thierry at intel.com>
> 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>
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 48a1e9349a2c..97ec11a7cc9a 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);
> @@ -1033,6 +1039,7 @@ static int guc_ads_create(struct intel_guc *guc)
>         struct intel_engine_cs *engine;
>         enum intel_engine_id id;
>         u32 base;
> +       u32 lrca_offset = LRC_PPHWSP_PN * PAGE_SIZE;

Make it const and try to avoid the visual breaks in line length? (Whilst
also trying not to gratuitously mix types.)
>  
>         GEM_BUG_ON(guc->ads_vma);
>  
> @@ -1062,13 +1069,15 @@ 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. GuC will skip the PPHWSP and 'Execlist Context',
> +        * thus reduce the size by 1 page (PPHWSP) and 80 dwords.
>          */
>         blob->ads.golden_context_lrca =
> -               dev_priv->engine[RCS]->status_page.ggtt_offset;
> +               guc_ggtt_offset(dev_priv->kernel_context->engine[RCS].state) + lrca_offset;
>  
> +       /* context_size - PPHWSP - first 80 dwords */
>         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 - lrca_offset - LR_HW_CONTEXT_SIZE;

The first LRC_PPHWSP_PN pages are not included in engine->context_size,
they are added on by execlist_context_deferred_alloc().

I'm finding it odd that we pass in a pointer to the start of the context
image, but that the guc wants to be told the size of the context minus
the bytes that *it* is choosing to skip at the start. (May be correct,
but looks weird and merits a warning.) The alternative explanation is
that it is not skipping the first 80 dwords, but the last.
-Chris