[Intel-gfx] [PATCH 1/3] drm/i915/guc: Fix GuC pin bias and WOPCM initialization order

[Michal Wajdeczko]

On Tue, 17 Jul 2018 13:55:58 +0200, Jakub Bartmiński  
<jakub.bartminski at intel.com> wrote:

> It would seem that we are using uninitialized WOPCM variables when
> setting the GuC pin bias. The pin bias has to be set after the WOPCM,
> but before the call to i915_gem_contexts_init where the first contexts
> are pinned so the safest place to set it seems to be right after
> initializing the relevant variables in intel_wopcm_init.
>
> 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/intel_guc.c   | 21 ---------------------
>  drivers/gpu/drm/i915/intel_wopcm.c | 19 +++++++++++++++++++
>  2 files changed, 19 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
> b/drivers/gpu/drm/i915/intel_guc.c
> index e12bd259df17..47cacd59ac32 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -27,8 +27,6 @@
>  #include "intel_guc_submission.h"
>  #include "i915_drv.h"
> -static void guc_init_ggtt_pin_bias(struct intel_guc *guc);
> -
>  static void gen8_guc_raise_irq(struct intel_guc *guc)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> @@ -142,8 +140,6 @@ int intel_guc_init_misc(struct intel_guc *guc)
>  	struct drm_i915_private *i915 = guc_to_i915(guc);
>  	int ret;
> -	guc_init_ggtt_pin_bias(guc);
> -
>  	ret = guc_init_wq(guc);
>  	if (ret)
>  		return ret;
> @@ -611,23 +607,6 @@ int intel_guc_resume(struct intel_guc *guc)
>   * actual GuC WOPCM size.
>   */
> -/**
> - * guc_init_ggtt_pin_bias() - Initialize the GuC ggtt_pin_bias value.
> - * @guc: intel_guc structure.
> - *
> - * This function will calculate and initialize the ggtt_pin_bias value  
> based on
> - * overall WOPCM size and GuC WOPCM size.
> - */
> -static void guc_init_ggtt_pin_bias(struct intel_guc *guc)
> -{
> -	struct drm_i915_private *i915 = guc_to_i915(guc);
> -
> -	GEM_BUG_ON(!i915->wopcm.size);
> -	GEM_BUG_ON(i915->wopcm.size < i915->wopcm.guc.base);
> -
> -	guc->ggtt_pin_bias = i915->wopcm.size - i915->wopcm.guc.base;
> -}
> -
>  /**
>   * intel_guc_allocate_vma() - Allocate a GGTT VMA for GuC usage
>   * @guc:	the guc
> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c  
> b/drivers/gpu/drm/i915/intel_wopcm.c
> index 74bf76f3fddc..02f602db9548 100644
> --- a/drivers/gpu/drm/i915/intel_wopcm.c
> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
> @@ -140,6 +140,23 @@ static inline int check_hw_restriction(struct  
> drm_i915_private *i915,
>  	return err;
>  }
> +/**
> + * wopcm_init_guc_ggtt_pin_bias() - Initialize the GuC ggtt_pin_bias  
> value.
> + * @wopcm: pointer to intel_wopcm.
> + *
> + * This function will calculate and initialize the GuC ggtt_pin_bias  
> value based
> + * on overall WOPCM size and GuC WOPCM size.
> + */
> +static void wopcm_init_guc_ggtt_pin_bias(struct intel_wopcm *wopcm)
> +{
> +	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
> +
> +	GEM_BUG_ON(!wopcm->size);
> +	GEM_BUG_ON(wopcm->size < wopcm->guc.base);
> +
> +	i915->guc.ggtt_pin_bias = wopcm->size - wopcm->guc.base;

Hmm, I don't like the idea to modify GuC (or GGTT later on) internals
directly from WOPCM private functions.

Maybe you can change your series to first move pin_bias from GuC to
GGTT and then immediately setup it inside i915_gem_init_ggtt() which
is called after intel_wopcm_init() and before i915_gem_contexts_init()

Michal


> +}
> +
>  /**
>   * intel_wopcm_init() - Initialize the WOPCM structure.
>   * @wopcm: pointer to intel_wopcm.
> @@ -207,6 +224,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
>  	wopcm->guc.base = guc_wopcm_base;
>  	wopcm->guc.size = guc_wopcm_size;
> +	wopcm_init_guc_ggtt_pin_bias(wopcm);
> +
>  	return 0;
>  }