| Message ID | 20180720133355.26180-1-jakub.bartminski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 20, 2018 at 03:33:51PM +0200, Jakub Bartmiński wrote: > It would appear that the calculated GuC pin bias was larger than it > should be, as the GuC address space does NOT contain the "HW contexts RSVD" > part of the WOPCM. Thus, the GuC pin bias is simply the GuC WOPCM size. > > 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> Cc: Jackie Li <yaodong.li@intel.com> Matches what I was able to read on GuC view of GGTT. Reviewed-by: Michał Winiarski <michal.winiarski@intel.com> -Michał > --- > drivers/gpu/drm/i915/intel_guc.c | 50 ++++++++++++++------------------ > 1 file changed, 22 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c > index e12bd259df17..17753952933e 100644 > --- a/drivers/gpu/drm/i915/intel_guc.c > +++ b/drivers/gpu/drm/i915/intel_guc.c > @@ -582,50 +582,44 @@ int intel_guc_resume(struct intel_guc *guc) > * > * :: > * > - * +==============> +====================+ <== GUC_GGTT_TOP > - * ^ | | > - * | | | > - * | | DRAM | > - * | | Memory | > - * | | | > - * GuC | | > - * Address +========> +====================+ <== WOPCM Top > - * Space ^ | HW contexts RSVD | > - * | | | WOPCM | > - * | | +==> +--------------------+ <== GuC WOPCM Top > - * | GuC ^ | | > - * | GGTT | | | > - * | Pin GuC | GuC | > - * | Bias WOPCM | WOPCM | > - * | | Size | | > - * | | | | | > - * v v v | | > - * +=====+=====+==> +====================+ <== GuC WOPCM Base > - * | Non-GuC WOPCM | > - * | (HuC/Reserved) | > - * +====================+ <== WOPCM Base > + * +============> +====================+ <== GUC_GGTT_TOP > + * ^ | | > + * | | | > + * | | DRAM | > + * | | Memory | > + * | | | > + * GuC | | > + * Address +====> +====================+ <== GuC WOPCM Top > + * Space ^ | | > + * | | | | > + * | GuC | GuC | > + * | WOPCM | WOPCM | > + * | Size | | > + * | | | | > + * v v | | > + * +=======+====> +====================+ <== GuC WOPCM Base > * > * The lower part of GuC Address Space [0, ggtt_pin_bias) is mapped to WOPCM > * while upper part of GuC Address Space [ggtt_pin_bias, GUC_GGTT_TOP) is mapped > - * to DRAM. The value of the GuC ggtt_pin_bias is determined by WOPCM size and > - * actual GuC WOPCM size. > + * to DRAM. The value of the GuC ggtt_pin_bias is the 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. > + * This function will calculate and initialize the ggtt_pin_bias value > + * based on the 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); > + GEM_BUG_ON(range_overflows(i915->wopcm.guc.base, i915->wopcm.guc.size, > + i915->wopcm.size)); > > - guc->ggtt_pin_bias = i915->wopcm.size - i915->wopcm.guc.base; > + guc->ggtt_pin_bias = i915->wopcm.guc.size; > } > > /** > -- > 2.17.1 >
On Fri, 20 Jul 2018 15:33:51 +0200, Jakub Bartmiński <jakub.bartminski@intel.com> wrote: > It would appear that the calculated GuC pin bias was larger than it > should be, as the GuC address space does NOT contain the "HW contexts > RSVD" > part of the WOPCM. Thus, the GuC pin bias is simply the GuC WOPCM size. > > 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 | 50 ++++++++++++++------------------ > 1 file changed, 22 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc.c > b/drivers/gpu/drm/i915/intel_guc.c > index e12bd259df17..17753952933e 100644 > --- a/drivers/gpu/drm/i915/intel_guc.c > +++ b/drivers/gpu/drm/i915/intel_guc.c > @@ -582,50 +582,44 @@ int intel_guc_resume(struct intel_guc *guc) > * > * :: > * > - * +==============> +====================+ <== GUC_GGTT_TOP > - * ^ | | > - * | | | > - * | | DRAM | > - * | | Memory | > - * | | | > - * GuC | | > - * Address +========> +====================+ <== WOPCM Top > - * Space ^ | HW contexts RSVD | > - * | | | WOPCM | > - * | | +==> +--------------------+ <== GuC WOPCM Top > - * | GuC ^ | | > - * | GGTT | | | > - * | Pin GuC | GuC | > - * | Bias WOPCM | WOPCM | > - * | | Size | | > - * | | | | | > - * v v v | | > - * +=====+=====+==> +====================+ <== GuC WOPCM Base > - * | Non-GuC WOPCM | > - * | (HuC/Reserved) | > - * +====================+ <== WOPCM Base > + * +============> +====================+ <== GUC_GGTT_TOP > + * ^ | | > + * | | | > + * | | DRAM | > + * | | Memory | > + * | | | > + * GuC | | > + * Address +====> +====================+ <== GuC WOPCM Top > + * Space ^ | | > + * | | | | > + * | GuC | GuC | > + * | WOPCM | WOPCM | > + * | Size | | > + * | | | | > + * v v | | > + * +=======+====> +====================+ <== GuC WOPCM Base as things are now simpler, can you clarify this diagram little more, like this: + ----------- +====================+ <= FFFF FFFF ^ | Reserved | | +====================+ <= GUC_GGTT_TOP | | | GuC | DRAM | Address | | Space + -- +====================+ <= GuC's ggtt_pin_bias | ^ | | | | | | | GuC | | | WOPCM | WOPCM | | Size | | | | | | v v | | + ------ + -- +====================+ <= 0000 0000 > * > * The lower part of GuC Address Space [0, ggtt_pin_bias) is mapped to > WOPCM > * while upper part of GuC Address Space [ggtt_pin_bias, GUC_GGTT_TOP) > is mapped > - * to DRAM. The value of the GuC ggtt_pin_bias is determined by WOPCM > size and > - * actual GuC WOPCM size. > + * to DRAM. The value of the GuC ggtt_pin_bias is the 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. > + * This function will calculate and initialize the ggtt_pin_bias value > + * based on the 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); hmm, maybe we should only care about i915->wopcm.guc.size ? > - GEM_BUG_ON(i915->wopcm.size < i915->wopcm.guc.base); > + GEM_BUG_ON(range_overflows(i915->wopcm.guc.base, i915->wopcm.guc.size, > + i915->wopcm.size)); why do you want to validate base/size here? you don't use guc.base anymore and, btw, it should be already calculated/verified in intel_wopcm.c > - guc->ggtt_pin_bias = i915->wopcm.size - i915->wopcm.guc.base; > + guc->ggtt_pin_bias = i915->wopcm.guc.size; > } > /** maybe we should also add Bspec: 1180 as patch seems to be aligned with it with all above, Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c index e12bd259df17..17753952933e 100644 --- a/drivers/gpu/drm/i915/intel_guc.c +++ b/drivers/gpu/drm/i915/intel_guc.c @@ -582,50 +582,44 @@ int intel_guc_resume(struct intel_guc *guc) * * :: * - * +==============> +====================+ <== GUC_GGTT_TOP - * ^ | | - * | | | - * | | DRAM | - * | | Memory | - * | | | - * GuC | | - * Address +========> +====================+ <== WOPCM Top - * Space ^ | HW contexts RSVD | - * | | | WOPCM | - * | | +==> +--------------------+ <== GuC WOPCM Top - * | GuC ^ | | - * | GGTT | | | - * | Pin GuC | GuC | - * | Bias WOPCM | WOPCM | - * | | Size | | - * | | | | | - * v v v | | - * +=====+=====+==> +====================+ <== GuC WOPCM Base - * | Non-GuC WOPCM | - * | (HuC/Reserved) | - * +====================+ <== WOPCM Base + * +============> +====================+ <== GUC_GGTT_TOP + * ^ | | + * | | | + * | | DRAM | + * | | Memory | + * | | | + * GuC | | + * Address +====> +====================+ <== GuC WOPCM Top + * Space ^ | | + * | | | | + * | GuC | GuC | + * | WOPCM | WOPCM | + * | Size | | + * | | | | + * v v | | + * +=======+====> +====================+ <== GuC WOPCM Base * * The lower part of GuC Address Space [0, ggtt_pin_bias) is mapped to WOPCM * while upper part of GuC Address Space [ggtt_pin_bias, GUC_GGTT_TOP) is mapped - * to DRAM. The value of the GuC ggtt_pin_bias is determined by WOPCM size and - * actual GuC WOPCM size. + * to DRAM. The value of the GuC ggtt_pin_bias is the 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. + * This function will calculate and initialize the ggtt_pin_bias value + * based on the 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); + GEM_BUG_ON(range_overflows(i915->wopcm.guc.base, i915->wopcm.guc.size, + i915->wopcm.size)); - guc->ggtt_pin_bias = i915->wopcm.size - i915->wopcm.guc.base; + guc->ggtt_pin_bias = i915->wopcm.guc.size; } /**
It would appear that the calculated GuC pin bias was larger than it should be, as the GuC address space does NOT contain the "HW contexts RSVD" part of the WOPCM. Thus, the GuC pin bias is simply the GuC WOPCM size. 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 | 50 ++++++++++++++------------------ 1 file changed, 22 insertions(+), 28 deletions(-)