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

[Bartminski, Jakub]

On Tue, 2018-07-17 at 14:19 +0200, Michal Wajdeczko wrote:
> 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;
> >  }

Indeed, since I don't think that the mock tests use this initialization
path, we should be able to decide on how to set the new ggtt pin_bias
value with the USES_GUC macro inside i915_gem_init_ggtt without raising
any assertion failures.
I will also take into account Chris's suggestions and resend.

-Jakub
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3278 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20180717/20cedc51/attachment.bin>