diff mbox

[v2,19/29] drm/i915: Add functions to emit register offsets to the ring

Message ID 1446672017-24497-20-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjala Nov. 4, 2015, 9:20 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When register type safety happens, we can't just try to emit the
register itself to the ring. Instead we'll need to extract the
offset from it first. Add some convenience functions that will do
that.

v2: Convert MOCS setup too

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c            |  2 +-
 drivers/gpu/drm/i915/i915_gem_context.c    |  4 ++--
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  4 ++--
 drivers/gpu/drm/i915/i915_gem_gtt.c        | 12 ++++++------
 drivers/gpu/drm/i915/intel_display.c       |  4 ++--
 drivers/gpu/drm/i915/intel_lrc.c           |  8 ++++----
 drivers/gpu/drm/i915/intel_lrc.h           |  5 +++++
 drivers/gpu/drm/i915/intel_mocs.c          |  8 ++++----
 drivers/gpu/drm/i915/intel_ringbuffer.c    |  4 ++--
 drivers/gpu/drm/i915/intel_ringbuffer.h    |  5 +++++
 10 files changed, 33 insertions(+), 23 deletions(-)

Comments

Chris Wilson Nov. 5, 2015, 11:03 a.m. UTC | #1 
On Wed, Nov 04, 2015 at 11:20:07PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> When register type safety happens, we can't just try to emit the
> register itself to the ring. Instead we'll need to extract the
> offset from it first. Add some convenience functions that will do
> that.
> 
> v2: Convert MOCS setup too
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

The only insane thing about this patch is the stupid ring emission API.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Ville Syrjala Nov. 5, 2015, 11:44 a.m. UTC | #2 
On Thu, Nov 05, 2015 at 11:03:38AM +0000, Chris Wilson wrote:
> On Wed, Nov 04, 2015 at 11:20:07PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > When register type safety happens, we can't just try to emit the
> > register itself to the ring. Instead we'll need to extract the
> > offset from it first. Add some convenience functions that will do
> > that.
> > 
> > v2: Convert MOCS setup too
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The only insane thing about this patch is the stupid ring emission API.

One extra idea just popped to my mind. Should I maybe make the
emit_reg() take the value too and emit both the reg offset and value?
They always come in pairs after all.

> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
Chris Wilson Nov. 5, 2015, 12:01 p.m. UTC | #3 
On Thu, Nov 05, 2015 at 01:44:08PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 05, 2015 at 11:03:38AM +0000, Chris Wilson wrote:
> > On Wed, Nov 04, 2015 at 11:20:07PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > When register type safety happens, we can't just try to emit the
> > > register itself to the ring. Instead we'll need to extract the
> > > offset from it first. Add some convenience functions that will do
> > > that.
> > > 
> > > v2: Convert MOCS setup too
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The only insane thing about this patch is the stupid ring emission API.
> 
> One extra idea just popped to my mind. Should I maybe make the
> emit_reg() take the value too and emit both the reg offset and value?
> They always come in pairs after all.

Uncertain. I think no. You need to do LRI and SRM separately (so that we
don't get confused between the register value to load and the memory
address to write to). But the more important factor for me, is that I
don't want to hide the individual calls to emit() - as they need to be
easily reviewed and checked against the count given to ring_begin().
-Chris
Ville Syrjala Nov. 5, 2015, 12:05 p.m. UTC | #4 
On Thu, Nov 05, 2015 at 12:01:51PM +0000, Chris Wilson wrote:
> On Thu, Nov 05, 2015 at 01:44:08PM +0200, Ville Syrjälä wrote:
> > On Thu, Nov 05, 2015 at 11:03:38AM +0000, Chris Wilson wrote:
> > > On Wed, Nov 04, 2015 at 11:20:07PM +0200, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > When register type safety happens, we can't just try to emit the
> > > > register itself to the ring. Instead we'll need to extract the
> > > > offset from it first. Add some convenience functions that will do
> > > > that.
> > > > 
> > > > v2: Convert MOCS setup too
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > The only insane thing about this patch is the stupid ring emission API.
> > 
> > One extra idea just popped to my mind. Should I maybe make the
> > emit_reg() take the value too and emit both the reg offset and value?
> > They always come in pairs after all.
> 
> Uncertain. I think no. You need to do LRI and SRM separately (so that we
> don't get confused between the register value to load and the memory
> address to write to). But the more important factor for me, is that I
> don't want to hide the individual calls to emit() - as they need to be
> easily reviewed and checked against the count given to ring_begin().

Indeed, that's a good reason for keeping it separate.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e1addf5..bfc79e6 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4593,7 +4593,7 @@  int i915_gem_l3_remap(struct drm_i915_gem_request *req, int slice)
 	 */
 	for (i = 0; i < GEN7_L3LOG_SIZE / 4; i++) {
 		intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
-		intel_ring_emit(ring, GEN7_L3LOG(slice, i));
+		intel_ring_emit_reg(ring, GEN7_L3LOG(slice, i));
 		intel_ring_emit(ring, remap_info[i]);
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 204dc7c..4b94004 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -556,7 +556,7 @@  mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
 				if (signaller == ring)
 					continue;
 
-				intel_ring_emit(ring, RING_PSMI_CTL(signaller->mmio_base));
+				intel_ring_emit_reg(ring, RING_PSMI_CTL(signaller->mmio_base));
 				intel_ring_emit(ring, _MASKED_BIT_ENABLE(GEN6_PSMI_SLEEP_MSG_DISABLE));
 			}
 		}
@@ -581,7 +581,7 @@  mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
 				if (signaller == ring)
 					continue;
 
-				intel_ring_emit(ring, RING_PSMI_CTL(signaller->mmio_base));
+				intel_ring_emit_reg(ring, RING_PSMI_CTL(signaller->mmio_base));
 				intel_ring_emit(ring, _MASKED_BIT_DISABLE(GEN6_PSMI_SLEEP_MSG_DISABLE));
 			}
 		}
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 6ed7d63a..a4c243c 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1114,7 +1114,7 @@  i915_reset_gen7_sol_offsets(struct drm_device *dev,
 
 	for (i = 0; i < 4; i++) {
 		intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
-		intel_ring_emit(ring, GEN7_SO_WRITE_OFFSET(i));
+		intel_ring_emit_reg(ring, GEN7_SO_WRITE_OFFSET(i));
 		intel_ring_emit(ring, 0);
 	}
 
@@ -1241,7 +1241,7 @@  i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
 
 		intel_ring_emit(ring, MI_NOOP);
 		intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
-		intel_ring_emit(ring, INSTPM);
+		intel_ring_emit_reg(ring, INSTPM);
 		intel_ring_emit(ring, instp_mask << 16 | instp_mode);
 		intel_ring_advance(ring);
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 016739e..8dfcfd5 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -666,10 +666,10 @@  static int gen8_write_pdp(struct drm_i915_gem_request *req,
 		return ret;
 
 	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
-	intel_ring_emit(ring, GEN8_RING_PDP_UDW(ring, entry));
+	intel_ring_emit_reg(ring, GEN8_RING_PDP_UDW(ring, entry));
 	intel_ring_emit(ring, upper_32_bits(addr));
 	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
-	intel_ring_emit(ring, GEN8_RING_PDP_LDW(ring, entry));
+	intel_ring_emit_reg(ring, GEN8_RING_PDP_LDW(ring, entry));
 	intel_ring_emit(ring, lower_32_bits(addr));
 	intel_ring_advance(ring);
 
@@ -1667,9 +1667,9 @@  static int hsw_mm_switch(struct i915_hw_ppgtt *ppgtt,
 		return ret;
 
 	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(2));
-	intel_ring_emit(ring, RING_PP_DIR_DCLV(ring));
+	intel_ring_emit_reg(ring, RING_PP_DIR_DCLV(ring));
 	intel_ring_emit(ring, PP_DIR_DCLV_2G);
-	intel_ring_emit(ring, RING_PP_DIR_BASE(ring));
+	intel_ring_emit_reg(ring, RING_PP_DIR_BASE(ring));
 	intel_ring_emit(ring, get_pd_offset(ppgtt));
 	intel_ring_emit(ring, MI_NOOP);
 	intel_ring_advance(ring);
@@ -1704,9 +1704,9 @@  static int gen7_mm_switch(struct i915_hw_ppgtt *ppgtt,
 		return ret;
 
 	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(2));
-	intel_ring_emit(ring, RING_PP_DIR_DCLV(ring));
+	intel_ring_emit_reg(ring, RING_PP_DIR_DCLV(ring));
 	intel_ring_emit(ring, PP_DIR_DCLV_2G);
-	intel_ring_emit(ring, RING_PP_DIR_BASE(ring));
+	intel_ring_emit_reg(ring, RING_PP_DIR_BASE(ring));
 	intel_ring_emit(ring, get_pd_offset(ppgtt));
 	intel_ring_emit(ring, MI_NOOP);
 	intel_ring_advance(ring);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5363a0f..ce87366 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11052,7 +11052,7 @@  static int intel_gen7_queue_flip(struct drm_device *dev,
 	 */
 	if (ring->id == RCS) {
 		intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
-		intel_ring_emit(ring, DERRMR);
+		intel_ring_emit_reg(ring, DERRMR);
 		intel_ring_emit(ring, ~(DERRMR_PIPEA_PRI_FLIP_DONE |
 					DERRMR_PIPEB_PRI_FLIP_DONE |
 					DERRMR_PIPEC_PRI_FLIP_DONE));
@@ -11062,7 +11062,7 @@  static int intel_gen7_queue_flip(struct drm_device *dev,
 		else
 			intel_ring_emit(ring, MI_STORE_REGISTER_MEM |
 					      MI_SRM_LRM_GLOBAL_GTT);
-		intel_ring_emit(ring, DERRMR);
+		intel_ring_emit_reg(ring, DERRMR);
 		intel_ring_emit(ring, ring->scratch.gtt_offset + 256);
 		if (IS_GEN8(dev)) {
 			intel_ring_emit(ring, 0);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 06180dc..38ee35a 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -921,7 +921,7 @@  int intel_execlists_submission(struct i915_execbuffer_params *params,
 
 		intel_logical_ring_emit(ringbuf, MI_NOOP);
 		intel_logical_ring_emit(ringbuf, MI_LOAD_REGISTER_IMM(1));
-		intel_logical_ring_emit(ringbuf, INSTPM);
+		intel_logical_ring_emit_reg(ringbuf, INSTPM);
 		intel_logical_ring_emit(ringbuf, instp_mask << 16 | instp_mode);
 		intel_logical_ring_advance(ringbuf);
 
@@ -1096,7 +1096,7 @@  static int intel_logical_ring_workarounds_emit(struct drm_i915_gem_request *req)
 
 	intel_logical_ring_emit(ringbuf, MI_LOAD_REGISTER_IMM(w->count));
 	for (i = 0; i < w->count; i++) {
-		intel_logical_ring_emit(ringbuf, w->reg[i].addr);
+		intel_logical_ring_emit_reg(ringbuf, w->reg[i].addr);
 		intel_logical_ring_emit(ringbuf, w->reg[i].value);
 	}
 	intel_logical_ring_emit(ringbuf, MI_NOOP);
@@ -1562,9 +1562,9 @@  static int intel_logical_ring_emit_pdps(struct drm_i915_gem_request *req)
 	for (i = GEN8_LEGACY_PDPES - 1; i >= 0; i--) {
 		const dma_addr_t pd_daddr = i915_page_dir_dma_addr(ppgtt, i);
 
-		intel_logical_ring_emit(ringbuf, GEN8_RING_PDP_UDW(ring, i));
+		intel_logical_ring_emit_reg(ringbuf, GEN8_RING_PDP_UDW(ring, i));
 		intel_logical_ring_emit(ringbuf, upper_32_bits(pd_daddr));
-		intel_logical_ring_emit(ringbuf, GEN8_RING_PDP_LDW(ring, i));
+		intel_logical_ring_emit_reg(ringbuf, GEN8_RING_PDP_LDW(ring, i));
 		intel_logical_ring_emit(ringbuf, lower_32_bits(pd_daddr));
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 4e60d54..f38a74b 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -70,6 +70,11 @@  static inline void intel_logical_ring_emit(struct intel_ringbuffer *ringbuf,
 	iowrite32(data, ringbuf->virtual_start + ringbuf->tail);
 	ringbuf->tail += 4;
 }
+static inline void intel_logical_ring_emit_reg(struct intel_ringbuffer *ringbuf,
+					       u32 reg)
+{
+	intel_logical_ring_emit(ringbuf, reg);
+}
 
 /* Logical Ring Contexts */
 
diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
index 8b8aec55..77febfe 100644
--- a/drivers/gpu/drm/i915/intel_mocs.c
+++ b/drivers/gpu/drm/i915/intel_mocs.c
@@ -210,7 +210,7 @@  static int emit_mocs_control_table(struct drm_i915_gem_request *req,
 				MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES));
 
 	for (index = 0; index < table->size; index++) {
-		intel_logical_ring_emit(ringbuf, mocs_register(ring, index));
+		intel_logical_ring_emit_reg(ringbuf, mocs_register(ring, index));
 		intel_logical_ring_emit(ringbuf,
 					table->table[index].control_value);
 	}
@@ -224,7 +224,7 @@  static int emit_mocs_control_table(struct drm_i915_gem_request *req,
 	 * that value to all the used entries.
 	 */
 	for (; index < GEN9_NUM_MOCS_ENTRIES; index++) {
-		intel_logical_ring_emit(ringbuf, mocs_register(ring, index));
+		intel_logical_ring_emit_reg(ringbuf, mocs_register(ring, index));
 		intel_logical_ring_emit(ringbuf, table->table[0].control_value);
 	}
 
@@ -272,7 +272,7 @@  static int emit_mocs_l3cc_table(struct drm_i915_gem_request *req,
 		value = (table->table[count].l3cc_value & 0xffff) |
 			((table->table[count + 1].l3cc_value & 0xffff) << 16);
 
-		intel_logical_ring_emit(ringbuf, GEN9_LNCFCMOCS(i));
+		intel_logical_ring_emit_reg(ringbuf, GEN9_LNCFCMOCS(i));
 		intel_logical_ring_emit(ringbuf, value);
 	}
 
@@ -289,7 +289,7 @@  static int emit_mocs_l3cc_table(struct drm_i915_gem_request *req,
 	 * they are reserved by the hardware.
 	 */
 	for (; i < GEN9_NUM_MOCS_ENTRIES / 2; i++) {
-		intel_logical_ring_emit(ringbuf, GEN9_LNCFCMOCS(i));
+		intel_logical_ring_emit_reg(ringbuf, GEN9_LNCFCMOCS(i));
 		intel_logical_ring_emit(ringbuf, value);
 
 		value = filler;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index c9b081f..c96a9a9 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -733,7 +733,7 @@  static int intel_ring_workarounds_emit(struct drm_i915_gem_request *req)
 
 	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(w->count));
 	for (i = 0; i < w->count; i++) {
-		intel_ring_emit(ring, w->reg[i].addr);
+		intel_ring_emit_reg(ring, w->reg[i].addr);
 		intel_ring_emit(ring, w->reg[i].value);
 	}
 	intel_ring_emit(ring, MI_NOOP);
@@ -1315,7 +1315,7 @@  static int gen6_signal(struct drm_i915_gem_request *signaller_req,
 		if (mbox_reg != GEN6_NOSYNC) {
 			u32 seqno = i915_gem_request_get_seqno(signaller_req);
 			intel_ring_emit(signaller, MI_LOAD_REGISTER_IMM(1));
-			intel_ring_emit(signaller, mbox_reg);
+			intel_ring_emit_reg(signaller, mbox_reg);
 			intel_ring_emit(signaller, seqno);
 		}
 	}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 58b1976..1ab5cb8 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -443,6 +443,11 @@  static inline void intel_ring_emit(struct intel_engine_cs *ring,
 	iowrite32(data, ringbuf->virtual_start + ringbuf->tail);
 	ringbuf->tail += 4;
 }
+static inline void intel_ring_emit_reg(struct intel_engine_cs *ring,
+				       u32 reg)
+{
+	intel_ring_emit(ring, reg);
+}
 static inline void intel_ring_advance(struct intel_engine_cs *ring)
 {
 	struct intel_ringbuffer *ringbuf = ring->buffer;