diff mbox

[v2] drm/i915: Initialize legacy semaphores from engine hw id indexed array

Message ID 1470913558-14882-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tvrtko Ursulin Aug. 11, 2016, 11:05 a.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Build the legacy semaphore initialisation array using the engine
hardware ids instead of driver internal ones. This makes the
static array size dependent only on the number of gen6 semaphore
engines.

Also makes the per-engine semaphore wait and signal tables
hardware id indexed saving some more space.

v2: Refactor I915_GEN6_NUM_ENGINES to GEN6_SEMAPHORE_LAST. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 55 +++++++++++++++++----------------
 drivers/gpu/drm/i915/intel_ringbuffer.h |  5 +--
 2 files changed, 32 insertions(+), 28 deletions(-)

Comments

Chris Wilson Aug. 11, 2016, 11:20 a.m. UTC | #1 
On Thu, Aug 11, 2016 at 12:05:58PM +0100, Tvrtko Ursulin wrote:
> @@ -1394,9 +1393,13 @@ static int gen6_signal(struct drm_i915_gem_request *req)
>  	if (ret)
>  		return ret;
>  
> -	for_each_engine_id(useless, dev_priv, id) {
> -		i915_reg_t mbox_reg = req->engine->semaphore.mbox.signal[id];
> +	for_each_engine(engine, dev_priv) {
> +		i915_reg_t mbox_reg;
> +
> +		if (engine->hw_id > GEN6_SEMAPHORE_LAST)
> +			continue;

Yeah, this makes more sense to me...

> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index ac568808aeb1..c2d8677b5b9f 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -277,11 +277,12 @@ struct intel_engine_cs {
>  		u32	sync_seqno[I915_NUM_ENGINES-1];
>  
>  		union {
> +#define GEN6_SEMAPHORE_LAST VECS_HW
>  			struct {
>  				/* our mbox written by others */
> -				u32		wait[I915_NUM_ENGINES];
> +				u32		wait[GEN6_SEMAPHORE_LAST + 1];

but I agree with your hesistation here.

#define GEN6_SEMAPHORE_LAST VECS_HW
#define GEN6_NUM_SEMAPHORES (GEN6_SEMAPHORE_LAST_HW + 1)
#define GEN6_MASK_SEMAPHORES GENMASK(GEN6_SEMAPHORE_LAST, 0)

if (!(BIT(engine->hw_id) & GEN6_SEMAPHORES_MASK))
	continue;

(can't decide on NUM/MASK_SEM or SEM_COUNT/MASK)

would that look better?
-Chris
Tvrtko Ursulin Aug. 11, 2016, 2:45 p.m. UTC | #2 
On 11/08/16 12:20, Chris Wilson wrote:
> On Thu, Aug 11, 2016 at 12:05:58PM +0100, Tvrtko Ursulin wrote:
>> @@ -1394,9 +1393,13 @@ static int gen6_signal(struct drm_i915_gem_request *req)
>>   	if (ret)
>>   		return ret;
>>
>> -	for_each_engine_id(useless, dev_priv, id) {
>> -		i915_reg_t mbox_reg = req->engine->semaphore.mbox.signal[id];
>> +	for_each_engine(engine, dev_priv) {
>> +		i915_reg_t mbox_reg;
>> +
>> +		if (engine->hw_id > GEN6_SEMAPHORE_LAST)
>> +			continue;
>
> Yeah, this makes more sense to me...
>
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> index ac568808aeb1..c2d8677b5b9f 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> @@ -277,11 +277,12 @@ struct intel_engine_cs {
>>   		u32	sync_seqno[I915_NUM_ENGINES-1];
>>
>>   		union {
>> +#define GEN6_SEMAPHORE_LAST VECS_HW
>>   			struct {
>>   				/* our mbox written by others */
>> -				u32		wait[I915_NUM_ENGINES];
>> +				u32		wait[GEN6_SEMAPHORE_LAST + 1];
>
> but I agree with your hesistation here.
>
> #define GEN6_SEMAPHORE_LAST VECS_HW
> #define GEN6_NUM_SEMAPHORES (GEN6_SEMAPHORE_LAST_HW + 1)
> #define GEN6_MASK_SEMAPHORES GENMASK(GEN6_SEMAPHORE_LAST, 0)
>
> if (!(BIT(engine->hw_id) & GEN6_SEMAPHORES_MASK))
> 	continue;
>
> (can't decide on NUM/MASK_SEM or SEM_COUNT/MASK)
>
> would that look better?

Yeah it is better, will do that with NUM/MASK.

Regards,

Tvrtko
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index ed19868df9c6..d38e7b94f728 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1385,8 +1385,7 @@  static int gen6_signal(struct drm_i915_gem_request *req)
 {
 	struct intel_ring *ring = req->ring;
 	struct drm_i915_private *dev_priv = req->i915;
-	struct intel_engine_cs *useless;
-	enum intel_engine_id id;
+	struct intel_engine_cs *engine;
 	int ret, num_rings;
 
 	num_rings = INTEL_INFO(dev_priv)->num_rings;
@@ -1394,9 +1393,13 @@  static int gen6_signal(struct drm_i915_gem_request *req)
 	if (ret)
 		return ret;
 
-	for_each_engine_id(useless, dev_priv, id) {
-		i915_reg_t mbox_reg = req->engine->semaphore.mbox.signal[id];
+	for_each_engine(engine, dev_priv) {
+		i915_reg_t mbox_reg;
+
+		if (engine->hw_id > GEN6_SEMAPHORE_LAST)
+			continue;
 
+		mbox_reg = req->engine->semaphore.mbox.signal[engine->hw_id];
 		if (i915_mmio_reg_valid(mbox_reg)) {
 			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
 			intel_ring_emit_reg(ring, mbox_reg);
@@ -1543,7 +1546,7 @@  gen6_ring_sync_to(struct drm_i915_gem_request *req,
 	u32 dw1 = MI_SEMAPHORE_MBOX |
 		  MI_SEMAPHORE_COMPARE |
 		  MI_SEMAPHORE_REGISTER;
-	u32 wait_mbox = signal->engine->semaphore.mbox.wait[req->engine->id];
+	u32 wait_mbox = signal->engine->semaphore.mbox.wait[req->engine->hw_id];
 	int ret;
 
 	WARN_ON(wait_mbox == MI_SEMAPHORE_SYNC_INVALID);
@@ -2671,41 +2674,41 @@  static void intel_ring_init_semaphores(struct drm_i915_private *dev_priv,
 		 * initialized as INVALID.  Gen8 will initialize the
 		 * sema between VCS2 and RCS later.
 		 */
-		for (i = 0; i < I915_NUM_ENGINES; i++) {
+		for (i = 0; i <= GEN6_SEMAPHORE_LAST; i++) {
 			static const struct {
 				u32 wait_mbox;
 				i915_reg_t mbox_reg;
-			} sem_data[I915_NUM_ENGINES][I915_NUM_ENGINES] = {
-				[RCS] = {
-					[VCS] =  { .wait_mbox = MI_SEMAPHORE_SYNC_RV,  .mbox_reg = GEN6_VRSYNC },
-					[BCS] =  { .wait_mbox = MI_SEMAPHORE_SYNC_RB,  .mbox_reg = GEN6_BRSYNC },
-					[VECS] = { .wait_mbox = MI_SEMAPHORE_SYNC_RVE, .mbox_reg = GEN6_VERSYNC },
+			} sem_data[GEN6_SEMAPHORE_LAST + 1][GEN6_SEMAPHORE_LAST + 1] = {
+				[RCS_HW] = {
+					[VCS_HW] =  { .wait_mbox = MI_SEMAPHORE_SYNC_RV,  .mbox_reg = GEN6_VRSYNC },
+					[BCS_HW] =  { .wait_mbox = MI_SEMAPHORE_SYNC_RB,  .mbox_reg = GEN6_BRSYNC },
+					[VECS_HW] = { .wait_mbox = MI_SEMAPHORE_SYNC_RVE, .mbox_reg = GEN6_VERSYNC },
 				},
-				[VCS] = {
-					[RCS] =  { .wait_mbox = MI_SEMAPHORE_SYNC_VR,  .mbox_reg = GEN6_RVSYNC },
-					[BCS] =  { .wait_mbox = MI_SEMAPHORE_SYNC_VB,  .mbox_reg = GEN6_BVSYNC },
-					[VECS] = { .wait_mbox = MI_SEMAPHORE_SYNC_VVE, .mbox_reg = GEN6_VEVSYNC },
+				[VCS_HW] = {
+					[RCS_HW] =  { .wait_mbox = MI_SEMAPHORE_SYNC_VR,  .mbox_reg = GEN6_RVSYNC },
+					[BCS_HW] =  { .wait_mbox = MI_SEMAPHORE_SYNC_VB,  .mbox_reg = GEN6_BVSYNC },
+					[VECS_HW] = { .wait_mbox = MI_SEMAPHORE_SYNC_VVE, .mbox_reg = GEN6_VEVSYNC },
 				},
-				[BCS] = {
-					[RCS] =  { .wait_mbox = MI_SEMAPHORE_SYNC_BR,  .mbox_reg = GEN6_RBSYNC },
-					[VCS] =  { .wait_mbox = MI_SEMAPHORE_SYNC_BV,  .mbox_reg = GEN6_VBSYNC },
-					[VECS] = { .wait_mbox = MI_SEMAPHORE_SYNC_BVE, .mbox_reg = GEN6_VEBSYNC },
+				[BCS_HW] = {
+					[RCS_HW] =  { .wait_mbox = MI_SEMAPHORE_SYNC_BR,  .mbox_reg = GEN6_RBSYNC },
+					[VCS_HW] =  { .wait_mbox = MI_SEMAPHORE_SYNC_BV,  .mbox_reg = GEN6_VBSYNC },
+					[VECS_HW] = { .wait_mbox = MI_SEMAPHORE_SYNC_BVE, .mbox_reg = GEN6_VEBSYNC },
 				},
-				[VECS] = {
-					[RCS] =  { .wait_mbox = MI_SEMAPHORE_SYNC_VER, .mbox_reg = GEN6_RVESYNC },
-					[VCS] =  { .wait_mbox = MI_SEMAPHORE_SYNC_VEV, .mbox_reg = GEN6_VVESYNC },
-					[BCS] =  { .wait_mbox = MI_SEMAPHORE_SYNC_VEB, .mbox_reg = GEN6_BVESYNC },
+				[VECS_HW] = {
+					[RCS_HW] =  { .wait_mbox = MI_SEMAPHORE_SYNC_VER, .mbox_reg = GEN6_RVESYNC },
+					[VCS_HW] =  { .wait_mbox = MI_SEMAPHORE_SYNC_VEV, .mbox_reg = GEN6_VVESYNC },
+					[BCS_HW] =  { .wait_mbox = MI_SEMAPHORE_SYNC_VEB, .mbox_reg = GEN6_BVESYNC },
 				},
 			};
 			u32 wait_mbox;
 			i915_reg_t mbox_reg;
 
-			if (i == engine->id || i == VCS2) {
+			if (i == engine->hw_id) {
 				wait_mbox = MI_SEMAPHORE_SYNC_INVALID;
 				mbox_reg = GEN6_NOSYNC;
 			} else {
-				wait_mbox = sem_data[engine->id][i].wait_mbox;
-				mbox_reg = sem_data[engine->id][i].mbox_reg;
+				wait_mbox = sem_data[engine->hw_id][i].wait_mbox;
+				mbox_reg = sem_data[engine->hw_id][i].mbox_reg;
 			}
 
 			engine->semaphore.mbox.wait[i] = wait_mbox;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index ac568808aeb1..c2d8677b5b9f 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -277,11 +277,12 @@  struct intel_engine_cs {
 		u32	sync_seqno[I915_NUM_ENGINES-1];
 
 		union {
+#define GEN6_SEMAPHORE_LAST VECS_HW
 			struct {
 				/* our mbox written by others */
-				u32		wait[I915_NUM_ENGINES];
+				u32		wait[GEN6_SEMAPHORE_LAST + 1];
 				/* mboxes this ring signals to */
-				i915_reg_t	signal[I915_NUM_ENGINES];
+				i915_reg_t	signal[GEN6_SEMAPHORE_LAST + 1];
 			} mbox;
 			u64		signal_ggtt[I915_NUM_ENGINES];
 		};