| Message ID | 20180717115600.5938-1-jakub.bartminski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Jakub Bartmiński (2018-07-17 12:55:58) > 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. Oops, do you have a Fixes? Any idea as to how this didn't reveal itself under our superficial testing? -Chris
On Tue, 17 Jul 2018 13:55:58 +0200, Jakub Bartmiński <jakub.bartminski@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@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michał Winiarski <michal.winiarski@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@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; > }
On Tue, 17 Jul 2018 14:09:16 +0200, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Jakub Bartmiński (2018-07-17 12:55:58) >> 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. > > Oops, do you have a Fixes? Ha, it could be my fault: intel_wopcm_init and intel_uc_init_misc were swapped in ("drm/i915/uc: Fetch GuC/HuC firmwares from guc/huc specific init") Michal
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@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@intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Michał Winiarski <michal.winiarski@intel.com> > > Cc: Michal Wajdeczko <michal.wajdeczko@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
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; +} + /** * 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; }
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@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Michał Winiarski <michal.winiarski@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@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(-)