diff mbox series

[v5,3/5] drm/i915: Remove unnecessary ggtt_offset_bias from i915_gem_context

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

Commit Message

Bartminski, Jakub July 25, 2018, 10:56 a.m. UTC
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(-)

Comments

Michal Wajdeczko July 25, 2018, 5:55 p.m. UTC | #1
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 mbox series

Patch

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);