On 21/07/16 12:56, Dave Gordon wrote:
On 21/07/16 10:31, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Static table wastes space for invalid combinations and
engines which are not supported by Gen6 (legacy semaphores).
Replace it with a function devised by Dave Gordon.
I have verified that it generates the same mappings between
mbox selectors and signalling registers.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Cc: Dave Gordon <david.s.gordon@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/i915_reg.h | 7 ++---
drivers/gpu/drm/i915/intel_engine_cs.c | 48
+++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_ringbuffer.c | 40
++-------------------------
drivers/gpu/drm/i915/intel_ringbuffer.h | 3 +++
4 files changed, 57 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index 8bfde75789f6..28aa876e2d87 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1604,9 +1604,10 @@ enum skl_disp_power_wells {
#define RING_HEAD(base) _MMIO((base)+0x34)
#define RING_START(base) _MMIO((base)+0x38)
#define RING_CTL(base) _MMIO((base)+0x3c)
-#define RING_SYNC_0(base) _MMIO((base)+0x40)
-#define RING_SYNC_1(base) _MMIO((base)+0x44)
-#define RING_SYNC_2(base) _MMIO((base)+0x48)
+#define RING_SYNC(base, n) _MMIO((base) + 0x40 + (n) * 4)
+#define RING_SYNC_0(base) RING_SYNC(base, 0)
+#define RING_SYNC_1(base) RING_SYNC(base, 1)
+#define RING_SYNC_2(base) RING_SYNC(base, 2)
#define GEN6_RVSYNC (RING_SYNC_0(RENDER_RING_BASE))
#define GEN6_RBSYNC (RING_SYNC_1(RENDER_RING_BASE))
#define GEN6_RVESYNC (RING_SYNC_2(RENDER_RING_BASE))
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c
b/drivers/gpu/drm/i915/intel_engine_cs.c
index f4a35ec78481..9837fddae259 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -209,3 +209,51 @@ int intel_engine_init_common(struct
intel_engine_cs *engine)
return i915_cmd_parser_init_ring(engine);
}
+
+#define I915_NUM_GEN6_SEMAPHORE_ENGINES (4)
+
+static int gen6_sem_f(unsigned int x, unsigned int y)
+{
+ if (x == y)
+ return -1;
+
+ x = intel_engines[x].guc_id;
+ y = intel_engines[y].guc_id;
You could have the caller pass two engine pointers rather than
converting passing indexes that aren't actually the values needed.
Can't do that, need to use the intel_engines static tables since
dev_priv->engine arrray is not yet fully initialized at this point.
Or you could have the caller pass the 'hw_id' (probably better than
'guc_id') directly.
It is called guc_id in this table and it is the only one.
+
+ if (x >= I915_NUM_GEN6_SEMAPHORE_ENGINES ||
+ y >= I915_NUM_GEN6_SEMAPHORE_ENGINES)
+ return -1;
And maybe move all the error checking out, so this function *just*
contains the tricksy calculation below?
Oh I don't know, it is at a single place like this. But I do agree it is
making the function impure. I did think about it but concluded it does
not matter hugely since it is all very little code.
+
+ x -= x >= y;
+ if (y == 1)
+ x = 3 - x;
+ x += y & 1;
+ return x % 3;
+}
+
+u32 gen6_wait_mbox(enum intel_engine_id x, enum intel_engine_id y)
+{
+ int r;
+
+ r = gen6_sem_f(x, y);
+ if (r < 0)
+ return MI_SEMAPHORE_SYNC_INVALID;
+
+ if (r == 1)
+ r = 2;
+ else if (r == 2)
+ r = 1;
BTW this is ((-r) % 3). Since gen6_sem_f() already does a "% 3" at the
end you might want to pass it a flag and let it do the negation when
required.
int gen6_sem_f2(unsigned int hw_x, unsigned int hw_y, bool wait)
{
hw_x -= hw_x >= hw_y;
hw_x += hw_y & 1;
hw_x ^= hw_y & hw_x >> hw_y; /* WTF? */
return (wait ? -hw_x : hw_x) % 3;
}
Now I got three flavours to pick from! :)
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
