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@xxxxxxxxx> 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@xxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Michał Winiarski <michal.winiarski@xxxxxxxxx>
> > Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > ---
> > 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.
-JakubAttachment:
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
