[Intel-gfx] [PATCH v3 2/5] drm/i915/guc: Move the pin bias value from GuC to GGTT

[Chris Wilson]

Quoting Jakub Bartmiński (2018-07-19 09:35:39)
> Removing the pin bias from GuC allows us to not check for GuC every time
> we pin a context, which fixes the assertion error on unresolved GuC
> platform default in mock contexts selftest.
> 
> With this change the intel_guc_ggtt_offset function has to know the full
> declaration of the drm_i915_private structure so it can no longer be
> located in the intel_guc.h header file due to include order.
> 
> v2:
> This also makes it so that there's no need to set GuC variables from
> within the WOPCM init function or to move the WOPCM init, while keeping
> the correct initialization order. Also for mock tests the pin bias is
> left at 0 and we make sure that the pin bias with GuC will not be
> smaller than without GuC.
> 
> v3:
> Avoid unused i915 in intel_guc_ggtt_offset if debug is disabled.
> 
> Testcase: igt/drv_selftest/mock_contexts #GuC
> Signed-off-by: Jakub Bartmiński <jakub.bartminski at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c   | 10 +-------
>  drivers/gpu/drm/i915/i915_gem_gtt.c       | 22 ++++++++++++----
>  drivers/gpu/drm/i915/i915_gem_gtt.h       |  2 ++
>  drivers/gpu/drm/i915/intel_guc.c          | 31 ++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_guc.h          | 28 +-------------------
>  drivers/gpu/drm/i915/intel_huc.c          |  2 +-
>  drivers/gpu/drm/i915/intel_uc_fw.c        |  2 +-
>  drivers/gpu/drm/i915/intel_wopcm.c        | 19 --------------
>  drivers/gpu/drm/i915/intel_wopcm.h        |  8 ++++++
>  drivers/gpu/drm/i915/selftests/mock_gtt.c |  2 ++
>  10 files changed, 60 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index b10770cfccd2..32f96b8cd9c4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -329,15 +329,7 @@ __create_hw_context(struct drm_i915_private *dev_priv,
>         ctx->desc_template =
>                 default_desc_template(dev_priv, dev_priv->mm.aliasing_ppgtt);
>  
> -       /*
> -        * GuC requires the ring to be placed in Non-WOPCM memory. If GuC is not
> -        * present or not in use we still need a small bias as ring wraparound
> -        * at offset 0 sometimes hangs. No idea why.
> -        */
> -       if (USES_GUC(dev_priv))
> -               ctx->ggtt_offset_bias = dev_priv->guc.ggtt_pin_bias;
> -       else
> -               ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
> +       ctx->ggtt_offset_bias = dev_priv->ggtt.pin_bias;
>  
>         return ctx;
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index d0acef299b9c..a61344379832 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2901,7 +2901,7 @@ void i915_gem_fini_aliasing_ppgtt(struct drm_i915_private *i915)
>         ggtt->vm.vma_ops.unbind_vma = ggtt_unbind_vma;
>  }
>  
> -int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
> +int i915_gem_init_ggtt(struct drm_i915_private *i915)
>  {
>         /* Let GEM Manage all of the aperture.
>          *
> @@ -2912,12 +2912,24 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
>          * aperture.  One page should be enough to keep any prefetching inside
>          * of the aperture.
>          */
> -       struct i915_ggtt *ggtt = &dev_priv->ggtt;
> +       struct i915_ggtt *ggtt = &i915->ggtt;
>         unsigned long hole_start, hole_end;
>         struct drm_mm_node *entry;
>         int ret;
>  
> -       ret = intel_vgt_balloon(dev_priv);
> +       /*
> +        * We need a small bias as ring wraparound at offset 0
> +        * sometimes hangs. No idea why.
> +        */
> +       ggtt->pin_bias = I915_GTT_PAGE_SIZE;
> +
> +       /* Any objects shared with GuC can't overlap with WOPCM. */
> +       if (USES_GUC(i915)) {
> +               ggtt->pin_bias = max(ggtt->pin_bias,
> +                                    intel_wopcm_get_pin_bias(&i915->wopcm));
> +       }

That is defeating the purpose of the series. We don't want to make these
decisions here, we just want to ask if intel_wopcm requires it. i.e., at
this point we only know that wopcm may reserve some of the Global GTT
for itself, we don't need to complicate that any further here.

USES_GUC() == modparam, ergo it should never be called in live code.
-Chris