| Message ID | 20180725105656.13348-3-jakub.bartminski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5,1/5] drm/i915/guc: Avoid wasting memory on incorrect GuC pin bias | expand |
On Wed, 25 Jul 2018 12:56:54 +0200, Jakub Bartmiński <jakub.bartminski@intel.com> wrote: > Since ggtt_offset_bias is now stored in ggtt.pin_bias, it is duplicated > inside i915_gem_context, and can instead be accessed directly from ggtt. I'm tempted to merge this patch with previous one (without renames) > > v3: > Added a helper function to retrieve the ggtt.pin_bias from the vma. > > v4: > Moved the helper function to the previous patch in the series. > Dropped the bias from intel_ring_pin. This introduces a slight functional > change since we are always pinning a ring a bit higher if GuC is present > even though we don't really need to. > > 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/i915_gem_context.c | 2 -- > drivers/gpu/drm/i915/i915_gem_context.h | 3 --- > drivers/gpu/drm/i915/intel_lrc.c | 6 ++---- > drivers/gpu/drm/i915/intel_ringbuffer.c | 14 ++++++-------- > drivers/gpu/drm/i915/intel_ringbuffer.h | 4 +--- > 5 files changed, 9 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c > b/drivers/gpu/drm/i915/i915_gem_context.c > index ae27caad1766..6067563750de 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -329,8 +329,6 @@ __create_hw_context(struct drm_i915_private *i915, > ctx->desc_template = > default_desc_template(i915, i915->mm.aliasing_ppgtt); > - ctx->ggtt_offset_bias = i915->ggtt.pin_bias; > - > return ctx; > err_pid: > diff --git a/drivers/gpu/drm/i915/i915_gem_context.h > b/drivers/gpu/drm/i915/i915_gem_context.h > index b116e4942c10..851dad6decd7 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.h > +++ b/drivers/gpu/drm/i915/i915_gem_context.h > @@ -147,9 +147,6 @@ struct i915_gem_context { > struct i915_sched_attr sched; > - /** ggtt_offset_bias: placement restriction for context objects */ > - u32 ggtt_offset_bias; > - > /** engine: per-engine logical HW state */ > struct intel_context { > struct i915_gem_context *gem_context; > diff --git a/drivers/gpu/drm/i915/intel_lrc.c > b/drivers/gpu/drm/i915/intel_lrc.c > index 35d37af0cb9a..c923eb998c28 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1363,9 +1363,7 @@ static int __context_pin(struct i915_gem_context > *ctx, struct i915_vma *vma) > } > flags = PIN_GLOBAL | PIN_HIGH; > - if (ctx->ggtt_offset_bias) > - flags |= PIN_OFFSET_BIAS | ctx->ggtt_offset_bias; > - > + flags |= PIN_OFFSET_BIAS | i915_ggtt_pin_bias(vma); This may unnecessarily turn on PIN_OFFSET_BIAS flag even if i915_ggtt_pin_bias() returns 0. Maybe this would be safer: if (i915_ggtt_pin_bias(vma)) flags |= PIN_OFFSET_BIAS | i915_ggtt_pin_bias(vma); > return i915_vma_pin(vma, 0, GEN8_LR_CONTEXT_ALIGN, flags); > } > @@ -1392,7 +1390,7 @@ __execlists_context_pin(struct intel_engine_cs > *engine, > goto unpin_vma; > } > - ret = intel_ring_pin(ce->ring, ctx->i915, ctx->ggtt_offset_bias); > + ret = intel_ring_pin(ce->ring, ctx->i915); > if (ret) > goto unpin_map; > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c > b/drivers/gpu/drm/i915/intel_ringbuffer.c > index f4bd185c9369..8a48325249a3 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -1005,9 +1005,7 @@ i915_emit_bb_start(struct i915_request *rq, > -int intel_ring_pin(struct intel_ring *ring, > - struct drm_i915_private *i915, > - unsigned int offset_bias) > +int intel_ring_pin(struct intel_ring *ring, struct drm_i915_private > *i915) > { > enum i915_map_type map = HAS_LLC(i915) ? I915_MAP_WB : I915_MAP_WC; > struct i915_vma *vma = ring->vma; > @@ -1017,10 +1015,11 @@ int intel_ring_pin(struct intel_ring *ring, > GEM_BUG_ON(ring->vaddr); > - > flags = PIN_GLOBAL; > - if (offset_bias) > - flags |= PIN_OFFSET_BIAS | offset_bias; > + > + /* Ring wraparound at offset 0 sometimes hangs. No idea why. */ > + flags |= PIN_OFFSET_BIAS | i915_ggtt_pin_bias(vma); same here > + > if (vma->obj->stolen) > flags |= PIN_MAPPABLE; > else > @@ -1404,8 +1403,7 @@ static int intel_init_ring_buffer(struct > intel_engine_cs *engine) > goto err; > } > - /* Ring wraparound at offset 0 sometimes hangs. No idea why. */ > - err = intel_ring_pin(ring, engine->i915, I915_GTT_PAGE_SIZE); > + err = intel_ring_pin(ring, engine->i915); > if (err) > goto err_ring; > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h > b/drivers/gpu/drm/i915/intel_ringbuffer.h > index d1eee08e5f6b..7fe07b2de2a7 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -784,9 +784,7 @@ struct intel_ring * > intel_engine_create_ring(struct intel_engine_cs *engine, > struct i915_timeline *timeline, > int size); > -int intel_ring_pin(struct intel_ring *ring, > - struct drm_i915_private *i915, > - unsigned int offset_bias); > +int intel_ring_pin(struct intel_ring *ring, struct drm_i915_private > *i915); > void intel_ring_reset(struct intel_ring *ring, u32 tail); > unsigned int intel_ring_update_space(struct intel_ring *ring); > void intel_ring_unpin(struct intel_ring *ring);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index ae27caad1766..6067563750de 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -329,8 +329,6 @@ __create_hw_context(struct drm_i915_private *i915, ctx->desc_template = default_desc_template(i915, i915->mm.aliasing_ppgtt); - ctx->ggtt_offset_bias = i915->ggtt.pin_bias; - return ctx; err_pid: diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h index b116e4942c10..851dad6decd7 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.h +++ b/drivers/gpu/drm/i915/i915_gem_context.h @@ -147,9 +147,6 @@ struct i915_gem_context { struct i915_sched_attr sched; - /** ggtt_offset_bias: placement restriction for context objects */ - u32 ggtt_offset_bias; - /** engine: per-engine logical HW state */ struct intel_context { struct i915_gem_context *gem_context; diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 35d37af0cb9a..c923eb998c28 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1363,9 +1363,7 @@ static int __context_pin(struct i915_gem_context *ctx, struct i915_vma *vma) } flags = PIN_GLOBAL | PIN_HIGH; - if (ctx->ggtt_offset_bias) - flags |= PIN_OFFSET_BIAS | ctx->ggtt_offset_bias; - + flags |= PIN_OFFSET_BIAS | i915_ggtt_pin_bias(vma); return i915_vma_pin(vma, 0, GEN8_LR_CONTEXT_ALIGN, flags); } @@ -1392,7 +1390,7 @@ __execlists_context_pin(struct intel_engine_cs *engine, goto unpin_vma; } - ret = intel_ring_pin(ce->ring, ctx->i915, ctx->ggtt_offset_bias); + ret = intel_ring_pin(ce->ring, ctx->i915); if (ret) goto unpin_map; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index f4bd185c9369..8a48325249a3 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1005,9 +1005,7 @@ i915_emit_bb_start(struct i915_request *rq, -int intel_ring_pin(struct intel_ring *ring, - struct drm_i915_private *i915, - unsigned int offset_bias) +int intel_ring_pin(struct intel_ring *ring, struct drm_i915_private *i915) { enum i915_map_type map = HAS_LLC(i915) ? I915_MAP_WB : I915_MAP_WC; struct i915_vma *vma = ring->vma; @@ -1017,10 +1015,11 @@ int intel_ring_pin(struct intel_ring *ring, GEM_BUG_ON(ring->vaddr); - flags = PIN_GLOBAL; - if (offset_bias) - flags |= PIN_OFFSET_BIAS | offset_bias; + + /* Ring wraparound at offset 0 sometimes hangs. No idea why. */ + flags |= PIN_OFFSET_BIAS | i915_ggtt_pin_bias(vma); + if (vma->obj->stolen) flags |= PIN_MAPPABLE; else @@ -1404,8 +1403,7 @@ static int intel_init_ring_buffer(struct intel_engine_cs *engine) goto err; } - /* Ring wraparound at offset 0 sometimes hangs. No idea why. */ - err = intel_ring_pin(ring, engine->i915, I915_GTT_PAGE_SIZE); + err = intel_ring_pin(ring, engine->i915); if (err) goto err_ring; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index d1eee08e5f6b..7fe07b2de2a7 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -784,9 +784,7 @@ struct intel_ring * intel_engine_create_ring(struct intel_engine_cs *engine, struct i915_timeline *timeline, int size); -int intel_ring_pin(struct intel_ring *ring, - struct drm_i915_private *i915, - unsigned int offset_bias); +int intel_ring_pin(struct intel_ring *ring, struct drm_i915_private *i915); void intel_ring_reset(struct intel_ring *ring, u32 tail); unsigned int intel_ring_update_space(struct intel_ring *ring); void intel_ring_unpin(struct intel_ring *ring);
Since ggtt_offset_bias is now stored in ggtt.pin_bias, it is duplicated inside i915_gem_context, and can instead be accessed directly from ggtt. v3: Added a helper function to retrieve the ggtt.pin_bias from the vma. v4: Moved the helper function to the previous patch in the series. Dropped the bias from intel_ring_pin. This introduces a slight functional change since we are always pinning a ring a bit higher if GuC is present even though we don't really need to. 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/i915_gem_context.c | 2 -- drivers/gpu/drm/i915/i915_gem_context.h | 3 --- drivers/gpu/drm/i915/intel_lrc.c | 6 ++---- drivers/gpu/drm/i915/intel_ringbuffer.c | 14 ++++++-------- drivers/gpu/drm/i915/intel_ringbuffer.h | 4 +--- 5 files changed, 9 insertions(+), 20 deletions(-)