| Message ID | 1469102447-4254-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 21, 2016 at 01:00:47PM +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > 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. So just how big was that table? How big are the functions replacing it? > v2: Add a comment describing what gen6_sem_f does. > v3: This time with git add. I like having the table a lot... Even if we don't find the function convincing we should add that comment. > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Dave Gordon <david.s.gordon@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_reg.h | 7 +-- > drivers/gpu/drm/i915/intel_engine_cs.c | 93 +++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_ringbuffer.c | 40 +------------- > drivers/gpu/drm/i915/intel_ringbuffer.h | 3 ++ > 4 files changed, 102 insertions(+), 41 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 9397ddec26b9..c2fe718582c8 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..19455b20b322 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -209,3 +209,96 @@ int intel_engine_init_common(struct intel_engine_cs *engine) > > return i915_cmd_parser_init_ring(engine); > } > + > +#define I915_NUM_GEN6_SEMAPHORE_ENGINES (4) > + > +/* > + * For Gen6 semaphores where the driver issues MI_SEMAPHORE_MBOX commands > + * with register selects so that a specific engine can wake up another engine > + * waiting on a matching register, the matrix of required register selects > + * looks like this: > + * > + * | RCS | VCS | BCS | VECS > + * -----+---------------------------+---------------------------+---------------------------+--------------------------- > + * RCS | MI_SEMAPHORE_SYNC_INVALID | MI_SEMAPHORE_SYNC_VR | MI_SEMAPHORE_SYNC_BR | MI_SEMAPHORE_SYNC_VER > + * VCS | MI_SEMAPHORE_SYNC_RV | MI_SEMAPHORE_SYNC_INVALID | MI_SEMAPHORE_SYNC_BV | MI_SEMAPHORE_SYNC_VEV > + * BCS | MI_SEMAPHORE_SYNC_RB | MI_SEMAPHORE_SYNC_VB | MI_SEMAPHORE_SYNC_INVALID | MI_SEMAPHORE_SYNC_VEB > + * VECS | MI_SEMAPHORE_SYNC_RVE | MI_SEMAPHORE_SYNC_VVE | MI_SEMAPHORE_SYNC_BVE | MI_SEMAPHORE_SYNC_INVALID > + * > + * This distilled to integers looks like this: > + * > + * | 0 | 1 | 2 | 3 > + * --+-----+-----+-----+----- > + * 0 | -1 | 0 | 2 | 1 > + * 1 | 2 | -1 | 0 | 1 > + * 2 | 0 | 2 | -1 | 1 > + * 3 | 2 | 1 | 0 | -1 > + * > + * In the opposite direction, the same table showing register addresses is: > + * > + * | RCS | VCS | BCS | VECS > + * -----+--------------+--------------+--------------+-------------- > + * RCS | GEN6_NOSYNC | GEN6_RVSYNC | GEN6_RBSYNC | GEN6_RVESYNC > + * VCS | GEN6_VRSYNC | GEN6_NOSYNC | GEN6_VBSYNC | GEN6_VVESYNC > + * BCS | GEN6_VRSYNC | GEN6_BVSYNC | GEN6_NOSYNC | GEN6_BVESYNC > + * VECS | GEN6_VERSYNC | GEN6_VEVSYNC | GEN6_VEBSYNC | GEN6_NOSYNC > + * > + * Again this distilled to integers looks like this: > + * > + * | 0 | 1 | 2 | 3 > + * --+-----+-----+-----+----- > + * 0 | -1 | 0 | 1 | 2 > + * 1 | 1 | -1 | 0 | 2 > + * 2 | 0 | 1 | -1 | 2 > + * 3 | 1 | 2 | 0 | -1 > + * > + * The function gen6_sem_f expresses the above table. We also notice that the > + * difference between the first and second tabe is only a transpose of ones to > + * twos and twos to ones. > + */ > + > +static int gen6_sem_f(unsigned int x, unsigned int y) gen6_sema_select gen6_semaphore_flag > +{ > + if (x == y) > + return -1; > + > + x = intel_engines[x].guc_id; > + y = intel_engines[y].guc_id; hw_id. > + > + if (x >= I915_NUM_GEN6_SEMAPHORE_ENGINES || > + y >= I915_NUM_GEN6_SEMAPHORE_ENGINES) > + return -1; > + /* * X * | 0 | 1 | 2 | 3 * --+-----+-----+-----+----- * 0 | | 0 | 1 | 2 * Y 1 | 1 | | 0 | 2 * 2 | 0 | 1 | | 2 * 3 | 1 | 2 | 0 | */ > + 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) static... > +{ > + 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; > + > + return r << 16; /* Convert semaphore sync field to its wait flag */ switch (gen6_sem_f(x, y)) { case 0: return 0; case 1: return 2 << 16; case 2: return 1 << 16; default: eturn MI_SEMAPHORE_SYNC_INVALID; } > +} > + > +i915_reg_t gen6_signal_reg(enum intel_engine_id x, enum intel_engine_id y) static... > +{ > + int r; > + > + r = gen6_sem_f(x, y); > + if (r < 0) > + return GEN6_NOSYNC; > + > + return RING_SYNC(intel_engines[y].mmio_base, r); > +}
On 21/07/16 13:59, Chris Wilson wrote: > On Thu, Jul 21, 2016 at 01:00:47PM +0100, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> 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. > > So just how big was that table? How big are the functions replacing it? With I915_NUM_ENGINES of 5 table is 5 * 5 * (2 * 4) = 200 bytes. With the patch .text grows by 144 bytes here and .rodata shrinks by 256. So a net gain of 112 bytes with my config. Conclusion is that as long as we got five engines it is not that interesting to get rid of the table. >> v2: Add a comment describing what gen6_sem_f does. >> v3: This time with git add. > > I like having the table a lot... Even if we don't find the function > convincing we should add that comment. > >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Dave Gordon <david.s.gordon@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> --- >> drivers/gpu/drm/i915/i915_reg.h | 7 +-- >> drivers/gpu/drm/i915/intel_engine_cs.c | 93 +++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_ringbuffer.c | 40 +------------- >> drivers/gpu/drm/i915/intel_ringbuffer.h | 3 ++ >> 4 files changed, 102 insertions(+), 41 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 9397ddec26b9..c2fe718582c8 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..19455b20b322 100644 >> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >> @@ -209,3 +209,96 @@ int intel_engine_init_common(struct intel_engine_cs *engine) >> >> return i915_cmd_parser_init_ring(engine); >> } >> + >> +#define I915_NUM_GEN6_SEMAPHORE_ENGINES (4) >> + >> +/* >> + * For Gen6 semaphores where the driver issues MI_SEMAPHORE_MBOX commands >> + * with register selects so that a specific engine can wake up another engine >> + * waiting on a matching register, the matrix of required register selects >> + * looks like this: >> + * >> + * | RCS | VCS | BCS | VECS >> + * -----+---------------------------+---------------------------+---------------------------+--------------------------- >> + * RCS | MI_SEMAPHORE_SYNC_INVALID | MI_SEMAPHORE_SYNC_VR | MI_SEMAPHORE_SYNC_BR | MI_SEMAPHORE_SYNC_VER >> + * VCS | MI_SEMAPHORE_SYNC_RV | MI_SEMAPHORE_SYNC_INVALID | MI_SEMAPHORE_SYNC_BV | MI_SEMAPHORE_SYNC_VEV >> + * BCS | MI_SEMAPHORE_SYNC_RB | MI_SEMAPHORE_SYNC_VB | MI_SEMAPHORE_SYNC_INVALID | MI_SEMAPHORE_SYNC_VEB >> + * VECS | MI_SEMAPHORE_SYNC_RVE | MI_SEMAPHORE_SYNC_VVE | MI_SEMAPHORE_SYNC_BVE | MI_SEMAPHORE_SYNC_INVALID >> + * >> + * This distilled to integers looks like this: >> + * >> + * | 0 | 1 | 2 | 3 >> + * --+-----+-----+-----+----- >> + * 0 | -1 | 0 | 2 | 1 >> + * 1 | 2 | -1 | 0 | 1 >> + * 2 | 0 | 2 | -1 | 1 >> + * 3 | 2 | 1 | 0 | -1 >> + * >> + * In the opposite direction, the same table showing register addresses is: >> + * >> + * | RCS | VCS | BCS | VECS >> + * -----+--------------+--------------+--------------+-------------- >> + * RCS | GEN6_NOSYNC | GEN6_RVSYNC | GEN6_RBSYNC | GEN6_RVESYNC >> + * VCS | GEN6_VRSYNC | GEN6_NOSYNC | GEN6_VBSYNC | GEN6_VVESYNC >> + * BCS | GEN6_VRSYNC | GEN6_BVSYNC | GEN6_NOSYNC | GEN6_BVESYNC >> + * VECS | GEN6_VERSYNC | GEN6_VEVSYNC | GEN6_VEBSYNC | GEN6_NOSYNC >> + * >> + * Again this distilled to integers looks like this: >> + * >> + * | 0 | 1 | 2 | 3 >> + * --+-----+-----+-----+----- >> + * 0 | -1 | 0 | 1 | 2 >> + * 1 | 1 | -1 | 0 | 2 >> + * 2 | 0 | 1 | -1 | 2 >> + * 3 | 1 | 2 | 0 | -1 >> + * >> + * The function gen6_sem_f expresses the above table. We also notice that the >> + * difference between the first and second tabe is only a transpose of ones to >> + * twos and twos to ones. >> + */ >> + >> +static int gen6_sem_f(unsigned int x, unsigned int y) > > gen6_sema_select > gen6_semaphore_flag Pick one name to replace gen6_sem_f you mean? >> +{ >> + if (x == y) >> + return -1; >> + >> + x = intel_engines[x].guc_id; >> + y = intel_engines[y].guc_id; > > hw_id. Some guys named Chris and Dave removed it. ;D > >> + >> + if (x >= I915_NUM_GEN6_SEMAPHORE_ENGINES || >> + y >= I915_NUM_GEN6_SEMAPHORE_ENGINES) >> + return -1; >> + > > /* > * X > * | 0 | 1 | 2 | 3 > * --+-----+-----+-----+----- > * 0 | | 0 | 1 | 2 > * Y 1 | 1 | | 0 | 2 > * 2 | 0 | 1 | | 2 > * 3 | 1 | 2 | 0 | > */ You want another copy of the table here? > >> + 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) > > static... It is called from intel_ringbuffer.c. > >> +{ >> + 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; >> + >> + return r << 16; > > /* Convert semaphore sync field to its wait flag */ > switch (gen6_sem_f(x, y)) { > case 0: return 0; > case 1: return 2 << 16; > case 2: return 1 << 16; > default: eturn MI_SEMAPHORE_SYNC_INVALID; > > } Meh. :) Bike-shedding territory. > >> +} >> + >> +i915_reg_t gen6_signal_reg(enum intel_engine_id x, enum intel_engine_id y) > > static... The same as gen6_wait_mbox, called from intel_ringbuffer.c. >> +{ >> + int r; >> + >> + r = gen6_sem_f(x, y); >> + if (r < 0) >> + return GEN6_NOSYNC; >> + >> + return RING_SYNC(intel_engines[y].mmio_base, r); >> +} > Regards, Tvrtko
On 21/07/16 14:16, Tvrtko Ursulin wrote: [snip] >>> +{ >>> + if (x == y) >>> + return -1; >>> + >>> + x = intel_engines[x].guc_id; >>> + y = intel_engines[y].guc_id; >> >> hw_id. > > Some guys named Chris and Dave removed it. ;D I need to take this back, I was confusing two tables and some past discussions. In this one we don't have hw_id. Regards, Tvrtko
On Thu, Jul 21, 2016 at 02:16:22PM +0100, Tvrtko Ursulin wrote: > > On 21/07/16 13:59, Chris Wilson wrote: > >On Thu, Jul 21, 2016 at 01:00:47PM +0100, Tvrtko Ursulin wrote: > >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > >>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. > > > >So just how big was that table? How big are the functions replacing it? > > With I915_NUM_ENGINES of 5 table is 5 * 5 * (2 * 4) = 200 bytes. > > With the patch .text grows by 144 bytes here and .rodata shrinks by > 256. So a net gain of 112 bytes with my config. Conclusion is that > as long as we got five engines it is not that interesting to get rid > of the table. > > >>v2: Add a comment describing what gen6_sem_f does. > >>v3: This time with git add. > > > >I like having the table a lot... Even if we don't find the function > >convincing we should add that comment. > > > >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>Cc: Dave Gordon <david.s.gordon@intel.com> > >>Cc: Chris Wilson <chris@chris-wilson.co.uk> > >>--- > >> drivers/gpu/drm/i915/i915_reg.h | 7 +-- > >> drivers/gpu/drm/i915/intel_engine_cs.c | 93 +++++++++++++++++++++++++++++++++ > >> drivers/gpu/drm/i915/intel_ringbuffer.c | 40 +------------- > >> drivers/gpu/drm/i915/intel_ringbuffer.h | 3 ++ > >> 4 files changed, 102 insertions(+), 41 deletions(-) > >> > >>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > >>index 9397ddec26b9..c2fe718582c8 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..19455b20b322 100644 > >>--- a/drivers/gpu/drm/i915/intel_engine_cs.c > >>+++ b/drivers/gpu/drm/i915/intel_engine_cs.c > >>@@ -209,3 +209,96 @@ int intel_engine_init_common(struct intel_engine_cs *engine) > >> > >> return i915_cmd_parser_init_ring(engine); > >> } > >>+ > >>+#define I915_NUM_GEN6_SEMAPHORE_ENGINES (4) > >>+ > >>+/* > >>+ * For Gen6 semaphores where the driver issues MI_SEMAPHORE_MBOX commands > >>+ * with register selects so that a specific engine can wake up another engine > >>+ * waiting on a matching register, the matrix of required register selects > >>+ * looks like this: > >>+ * > >>+ * | RCS | VCS | BCS | VECS > >>+ * -----+---------------------------+---------------------------+---------------------------+--------------------------- > >>+ * RCS | MI_SEMAPHORE_SYNC_INVALID | MI_SEMAPHORE_SYNC_VR | MI_SEMAPHORE_SYNC_BR | MI_SEMAPHORE_SYNC_VER > >>+ * VCS | MI_SEMAPHORE_SYNC_RV | MI_SEMAPHORE_SYNC_INVALID | MI_SEMAPHORE_SYNC_BV | MI_SEMAPHORE_SYNC_VEV > >>+ * BCS | MI_SEMAPHORE_SYNC_RB | MI_SEMAPHORE_SYNC_VB | MI_SEMAPHORE_SYNC_INVALID | MI_SEMAPHORE_SYNC_VEB > >>+ * VECS | MI_SEMAPHORE_SYNC_RVE | MI_SEMAPHORE_SYNC_VVE | MI_SEMAPHORE_SYNC_BVE | MI_SEMAPHORE_SYNC_INVALID > >>+ * > >>+ * This distilled to integers looks like this: > >>+ * > >>+ * | 0 | 1 | 2 | 3 > >>+ * --+-----+-----+-----+----- > >>+ * 0 | -1 | 0 | 2 | 1 > >>+ * 1 | 2 | -1 | 0 | 1 > >>+ * 2 | 0 | 2 | -1 | 1 > >>+ * 3 | 2 | 1 | 0 | -1 > >>+ * > >>+ * In the opposite direction, the same table showing register addresses is: > >>+ * > >>+ * | RCS | VCS | BCS | VECS > >>+ * -----+--------------+--------------+--------------+-------------- > >>+ * RCS | GEN6_NOSYNC | GEN6_RVSYNC | GEN6_RBSYNC | GEN6_RVESYNC > >>+ * VCS | GEN6_VRSYNC | GEN6_NOSYNC | GEN6_VBSYNC | GEN6_VVESYNC > >>+ * BCS | GEN6_VRSYNC | GEN6_BVSYNC | GEN6_NOSYNC | GEN6_BVESYNC > >>+ * VECS | GEN6_VERSYNC | GEN6_VEVSYNC | GEN6_VEBSYNC | GEN6_NOSYNC > >>+ * > >>+ * Again this distilled to integers looks like this: > >>+ * > >>+ * | 0 | 1 | 2 | 3 > >>+ * --+-----+-----+-----+----- > >>+ * 0 | -1 | 0 | 1 | 2 > >>+ * 1 | 1 | -1 | 0 | 2 > >>+ * 2 | 0 | 1 | -1 | 2 > >>+ * 3 | 1 | 2 | 0 | -1 > >>+ * > >>+ * The function gen6_sem_f expresses the above table. We also notice that the > >>+ * difference between the first and second tabe is only a transpose of ones to > >>+ * twos and twos to ones. > >>+ */ > >>+ > >>+static int gen6_sem_f(unsigned int x, unsigned int y) > > > >gen6_sema_select > >gen6_semaphore_flag > > Pick one name to replace gen6_sem_f you mean? Yes, you contracted semaphore to sema last time (and that stuck). And by _f I assume you mean mathematical function, which is a little boring. > >>+{ > >>+ if (x == y) > >>+ return -1; > >>+ > >>+ x = intel_engines[x].guc_id; > >>+ y = intel_engines[y].guc_id; > > > >hw_id. > > Some guys named Chris and Dave removed it. ;D I did?... I was aiming at removing guc_id! I was aware Dave was arguing against removing guc_id just in case the firmware differed in future from the existing gen5+ id. > >>+ > >>+ if (x >= I915_NUM_GEN6_SEMAPHORE_ENGINES || > >>+ y >= I915_NUM_GEN6_SEMAPHORE_ENGINES) > >>+ return -1; > >>+ > > > >/* > > * X > > * | 0 | 1 | 2 | 3 > > * --+-----+-----+-----+----- > > * 0 | | 0 | 1 | 2 > > * Y 1 | 1 | | 0 | 2 > > * 2 | 0 | 1 | | 2 > > * 3 | 1 | 2 | 0 | > > */ > > You want another copy of the table here? Yes. In particular, I need to know which axis is X and which is Y. Having the table here is much easier to compare to the output of the code (same screen). > >>+ 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) > > > >static... > > It is called from intel_ringbuffer.c. Hmm. This was in intel_ringbuffer.c, at least I assumed so as this only applies to legacy submission, for gen6-7. > >>+{ > >>+ 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; > >>+ > >>+ return r << 16; > > > >/* Convert semaphore sync field to its wait flag */ > >switch (gen6_sem_f(x, y)) { > >case 0: return 0; > >case 1: return 2 << 16; > >case 2: return 1 << 16; > >default: eturn MI_SEMAPHORE_SYNC_INVALID; > > > >} > > Meh. :) Bike-shedding territory. I know which I found easier to understand ;) -Chris
On 21/07/16 14:31, Chris Wilson wrote: > On Thu, Jul 21, 2016 at 02:16:22PM +0100, Tvrtko Ursulin wrote: >> >> On 21/07/16 13:59, Chris Wilson wrote: >>> On Thu, Jul 21, 2016 at 01:00:47PM +0100, Tvrtko Ursulin wrote: >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> >>>> 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. >>> >>> So just how big was that table? How big are the functions replacing it? >> >> With I915_NUM_ENGINES of 5 table is 5 * 5 * (2 * 4) = 200 bytes. >> >> With the patch .text grows by 144 bytes here and .rodata shrinks by >> 256. So a net gain of 112 bytes with my config. Conclusion is that >> as long as we got five engines it is not that interesting to get rid >> of the table. >> >>>> v2: Add a comment describing what gen6_sem_f does. >>>> v3: This time with git add. >>> >>> I like having the table a lot... Even if we don't find the function >>> convincing we should add that comment. >>> >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> Cc: Dave Gordon <david.s.gordon@intel.com> >>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>>> --- >>>> drivers/gpu/drm/i915/i915_reg.h | 7 +-- >>>> drivers/gpu/drm/i915/intel_engine_cs.c | 93 +++++++++++++++++++++++++++++++++ >>>> drivers/gpu/drm/i915/intel_ringbuffer.c | 40 +------------- >>>> drivers/gpu/drm/i915/intel_ringbuffer.h | 3 ++ >>>> 4 files changed, 102 insertions(+), 41 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >>>> index 9397ddec26b9..c2fe718582c8 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..19455b20b322 100644 >>>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >>>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >>>> @@ -209,3 +209,96 @@ int intel_engine_init_common(struct intel_engine_cs *engine) >>>> >>>> return i915_cmd_parser_init_ring(engine); >>>> } >>>> + >>>> +#define I915_NUM_GEN6_SEMAPHORE_ENGINES (4) >>>> + >>>> +/* >>>> + * For Gen6 semaphores where the driver issues MI_SEMAPHORE_MBOX commands >>>> + * with register selects so that a specific engine can wake up another engine >>>> + * waiting on a matching register, the matrix of required register selects >>>> + * looks like this: >>>> + * >>>> + * | RCS | VCS | BCS | VECS >>>> + * -----+---------------------------+---------------------------+---------------------------+--------------------------- >>>> + * RCS | MI_SEMAPHORE_SYNC_INVALID | MI_SEMAPHORE_SYNC_VR | MI_SEMAPHORE_SYNC_BR | MI_SEMAPHORE_SYNC_VER >>>> + * VCS | MI_SEMAPHORE_SYNC_RV | MI_SEMAPHORE_SYNC_INVALID | MI_SEMAPHORE_SYNC_BV | MI_SEMAPHORE_SYNC_VEV >>>> + * BCS | MI_SEMAPHORE_SYNC_RB | MI_SEMAPHORE_SYNC_VB | MI_SEMAPHORE_SYNC_INVALID | MI_SEMAPHORE_SYNC_VEB >>>> + * VECS | MI_SEMAPHORE_SYNC_RVE | MI_SEMAPHORE_SYNC_VVE | MI_SEMAPHORE_SYNC_BVE | MI_SEMAPHORE_SYNC_INVALID >>>> + * >>>> + * This distilled to integers looks like this: >>>> + * >>>> + * | 0 | 1 | 2 | 3 >>>> + * --+-----+-----+-----+----- >>>> + * 0 | -1 | 0 | 2 | 1 >>>> + * 1 | 2 | -1 | 0 | 1 >>>> + * 2 | 0 | 2 | -1 | 1 >>>> + * 3 | 2 | 1 | 0 | -1 >>>> + * >>>> + * In the opposite direction, the same table showing register addresses is: >>>> + * >>>> + * | RCS | VCS | BCS | VECS >>>> + * -----+--------------+--------------+--------------+-------------- >>>> + * RCS | GEN6_NOSYNC | GEN6_RVSYNC | GEN6_RBSYNC | GEN6_RVESYNC >>>> + * VCS | GEN6_VRSYNC | GEN6_NOSYNC | GEN6_VBSYNC | GEN6_VVESYNC >>>> + * BCS | GEN6_VRSYNC | GEN6_BVSYNC | GEN6_NOSYNC | GEN6_BVESYNC >>>> + * VECS | GEN6_VERSYNC | GEN6_VEVSYNC | GEN6_VEBSYNC | GEN6_NOSYNC >>>> + * >>>> + * Again this distilled to integers looks like this: >>>> + * >>>> + * | 0 | 1 | 2 | 3 >>>> + * --+-----+-----+-----+----- >>>> + * 0 | -1 | 0 | 1 | 2 >>>> + * 1 | 1 | -1 | 0 | 2 >>>> + * 2 | 0 | 1 | -1 | 2 >>>> + * 3 | 1 | 2 | 0 | -1 >>>> + * >>>> + * The function gen6_sem_f expresses the above table. We also notice that the >>>> + * difference between the first and second tabe is only a transpose of ones to >>>> + * twos and twos to ones. >>>> + */ >>>> + >>>> +static int gen6_sem_f(unsigned int x, unsigned int y) >>> >>> gen6_sema_select >>> gen6_semaphore_flag >> >> Pick one name to replace gen6_sem_f you mean? > > Yes, you contracted semaphore to sema last time (and that stuck). And by > _f I assume you mean mathematical function, which is a little boring. Yes, and boring or not it is wrong since it is not a pure mathematical function so I will rename it. >>>> +{ >>>> + if (x == y) >>>> + return -1; >>>> + >>>> + x = intel_engines[x].guc_id; >>>> + y = intel_engines[y].guc_id; >>> >>> hw_id. >> >> Some guys named Chris and Dave removed it. ;D > > I did?... I was aiming at removing guc_id! I was aware Dave was arguing > against removing guc_id just in case the firmware differed in future > from the existing gen5+ id. I retracted that comment later, just confused things. >>>> + >>>> + if (x >= I915_NUM_GEN6_SEMAPHORE_ENGINES || >>>> + y >= I915_NUM_GEN6_SEMAPHORE_ENGINES) >>>> + return -1; >>>> + >>> >>> /* >>> * X >>> * | 0 | 1 | 2 | 3 >>> * --+-----+-----+-----+----- >>> * 0 | | 0 | 1 | 2 >>> * Y 1 | 1 | | 0 | 2 >>> * 2 | 0 | 1 | | 2 >>> * 3 | 1 | 2 | 0 | >>> */ >> >> You want another copy of the table here? > > Yes. In particular, I need to know which axis is X and which is Y. > Having the table here is much easier to compare to the output of the > code (same screen). Ok. >>>> + 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) >>> >>> static... >> >> It is called from intel_ringbuffer.c. > > Hmm. This was in intel_ringbuffer.c, at least I assumed so as this only > applies to legacy submission, for gen6-7. It uses the static intel_engines array since the dev_priv->engines are not initialized yet by the time it runs, for an engine. Could as an alternative make the engine init phase multi-pass. Maybe. Not sure what repercussions for the cleanup path that would have. Regards, Tvrtko
On Thu, Jul 21, 2016 at 02:46:01PM +0100, Tvrtko Ursulin wrote: > > On 21/07/16 14:31, Chris Wilson wrote: > >Hmm. This was in intel_ringbuffer.c, at least I assumed so as this only > >applies to legacy submission, for gen6-7. > > It uses the static intel_engines array since the dev_priv->engines > are not initialized yet by the time it runs, for an engine. Hmm, right. > Could as an alternative make the engine init phase multi-pass. > Maybe. Not sure what repercussions for the cleanup path that would > have. Or expose the intel_engines[] array. Double pass (setup, then init) is better than exposing the array. I think I am preferring that and trying to keep the semaphore logic in the intel_ringbuffer.c silo. -Chris
On 21/07/16 14:46, Tvrtko Ursulin wrote: > > On 21/07/16 14:31, Chris Wilson wrote: >> On Thu, Jul 21, 2016 at 02:16:22PM +0100, Tvrtko Ursulin wrote: >>> >>> On 21/07/16 13:59, Chris Wilson wrote: >>>> On Thu, Jul 21, 2016 at 01:00:47PM +0100, Tvrtko Ursulin wrote: >>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>>> >>>>> 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. >>>> >>>> So just how big was that table? How big are the functions replacing it? >>> >>> With I915_NUM_ENGINES of 5 table is 5 * 5 * (2 * 4) = 200 bytes. >>> >>> With the patch .text grows by 144 bytes here and .rodata shrinks by >>> 256. So a net gain of 112 bytes with my config. Conclusion is that >>> as long as we got five engines it is not that interesting to get rid >>> of the table. Since the semaphore matrix is only relevant to a specific gen, you could remove it from the multi-generational engine-list and instead just have it in the gen-specific code that needs it. That way it won't continue to grow as new engines are added. The one gen that needs it is fixed at 4x4, so it could just be a 16-byte lookup table, or 32 bits (0b11001001_10110001_00101101_10010011) if you really want to save space ;-) >>>>> v2: Add a comment describing what gen6_sem_f does. >>>>> v3: This time with git add. >>>> >>>> I like having the table a lot... Even if we don't find the function >>>> convincing we should add that comment. >>>> >>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>>> Cc: Dave Gordon <david.s.gordon@intel.com> >>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>>>> --- >>>>> drivers/gpu/drm/i915/i915_reg.h | 7 +-- >>>>> drivers/gpu/drm/i915/intel_engine_cs.c | 93 >>>>> +++++++++++++++++++++++++++++++++ >>>>> drivers/gpu/drm/i915/intel_ringbuffer.c | 40 +------------- >>>>> drivers/gpu/drm/i915/intel_ringbuffer.h | 3 ++ >>>>> 4 files changed, 102 insertions(+), 41 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h >>>>> b/drivers/gpu/drm/i915/i915_reg.h >>>>> index 9397ddec26b9..c2fe718582c8 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..19455b20b322 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >>>>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >>>>> @@ -209,3 +209,96 @@ int intel_engine_init_common(struct >>>>> intel_engine_cs *engine) >>>>> >>>>> return i915_cmd_parser_init_ring(engine); >>>>> } >>>>> + >>>>> +#define I915_NUM_GEN6_SEMAPHORE_ENGINES (4) >>>>> + >>>>> +/* >>>>> + * For Gen6 semaphores where the driver issues MI_SEMAPHORE_MBOX >>>>> commands >>>>> + * with register selects so that a specific engine can wake up >>>>> another engine >>>>> + * waiting on a matching register, the matrix of required register >>>>> selects >>>>> + * looks like this: >>>>> + * >>>>> + * | RCS | VCS >>>>> | BCS | VECS >>>>> + * >>>>> -----+---------------------------+---------------------------+---------------------------+--------------------------- >>>>> >>>>> + * RCS | MI_SEMAPHORE_SYNC_INVALID | MI_SEMAPHORE_SYNC_VR >>>>> | MI_SEMAPHORE_SYNC_BR | MI_SEMAPHORE_SYNC_VER >>>>> + * VCS | MI_SEMAPHORE_SYNC_RV | MI_SEMAPHORE_SYNC_INVALID >>>>> | MI_SEMAPHORE_SYNC_BV | MI_SEMAPHORE_SYNC_VEV >>>>> + * BCS | MI_SEMAPHORE_SYNC_RB | MI_SEMAPHORE_SYNC_VB | >>>>> MI_SEMAPHORE_SYNC_INVALID | MI_SEMAPHORE_SYNC_VEB >>>>> + * VECS | MI_SEMAPHORE_SYNC_RVE | MI_SEMAPHORE_SYNC_VVE >>>>> | MI_SEMAPHORE_SYNC_BVE | MI_SEMAPHORE_SYNC_INVALID >>>>> + * >>>>> + * This distilled to integers looks like this: >>>>> + * >>>>> + * | 0 | 1 | 2 | 3 >>>>> + * --+-----+-----+-----+----- >>>>> + * 0 | -1 | 0 | 2 | 1 >>>>> + * 1 | 2 | -1 | 0 | 1 >>>>> + * 2 | 0 | 2 | -1 | 1 >>>>> + * 3 | 2 | 1 | 0 | -1 Actually (and conveniently) MI_SEMAPHORE_SYNC_INVALID is 3 (<<16) so we don't really need to return -1 and then map it to INVALID, we can just use 0-3 directly. The binary string I wrote above represents this table; then to get the result we want it just has to be shifted. >>>>> + * >>>>> + * In the opposite direction, the same table showing register >>>>> addresses is: >>>>> + * >>>>> + * | RCS | VCS | BCS | VECS >>>>> + * -----+--------------+--------------+--------------+-------------- >>>>> + * RCS | GEN6_NOSYNC | GEN6_RVSYNC | GEN6_RBSYNC | GEN6_RVESYNC >>>>> + * VCS | GEN6_VRSYNC | GEN6_NOSYNC | GEN6_VBSYNC | GEN6_VVESYNC >>>>> + * BCS | GEN6_VRSYNC | GEN6_BVSYNC | GEN6_NOSYNC | GEN6_BVESYNC >>>>> + * VECS | GEN6_VERSYNC | GEN6_VEVSYNC | GEN6_VEBSYNC | GEN6_NOSYNC >>>>> + * >>>>> + * Again this distilled to integers looks like this: >>>>> + * >>>>> + * | 0 | 1 | 2 | 3 >>>>> + * --+-----+-----+-----+----- >>>>> + * 0 | -1 | 0 | 1 | 2 >>>>> + * 1 | 1 | -1 | 0 | 2 >>>>> + * 2 | 0 | 1 | -1 | 2 >>>>> + * 3 | 1 | 2 | 0 | -1 With that table as the first function f1(returning 0-3), the second function could just be a lookup in a 4-entry array indexed by the result. Or convert 3 to NOSYNC, then the rest is (3-f1(x,y)) % 3. I think those might give the best combination of code+data size :) >>>> /* >>>> * X >>>> * | 0 | 1 | 2 | 3 >>>> * --+-----+-----+-----+----- >>>> * 0 | | 0 | 1 | 2 >>>> * Y 1 | 1 | | 0 | 2 >>>> * 2 | 0 | 1 | | 2 >>>> * 3 | 1 | 2 | 0 | >>>> */ >>> >>> You want another copy of the table here? >> >> Yes. In particular, I need to know which axis is X and which is Y. >> Having the table here is much easier to compare to the output of the >> code (same screen). > > Ok. Let's call them 'from' and 'to' (or 'signaller' and 'waiter', though that's rather long) rather than x & y, .Dave. >>>>> + 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) >>>> >>>> static... >>> >>> It is called from intel_ringbuffer.c. >> >> Hmm. This was in intel_ringbuffer.c, at least I assumed so as this only >> applies to legacy submission, for gen6-7. > > It uses the static intel_engines array since the dev_priv->engines are > not initialized yet by the time it runs, for an engine. > > Could as an alternative make the engine init phase multi-pass. Maybe. > Not sure what repercussions for the cleanup path that would have. > > Regards, > Tvrtko
On 22/07/16 13:42, Dave Gordon wrote: > On 21/07/16 14:46, Tvrtko Ursulin wrote: >> On 21/07/16 14:31, Chris Wilson wrote: >>> On Thu, Jul 21, 2016 at 02:16:22PM +0100, Tvrtko Ursulin wrote: >>>> >>>> On 21/07/16 13:59, Chris Wilson wrote: >>>>> On Thu, Jul 21, 2016 at 01:00:47PM +0100, Tvrtko Ursulin wrote: >>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>>>> >>>>>> 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. >>>>> >>>>> So just how big was that table? How big are the functions replacing >>>>> it? >>>> >>>> With I915_NUM_ENGINES of 5 table is 5 * 5 * (2 * 4) = 200 bytes. >>>> >>>> With the patch .text grows by 144 bytes here and .rodata shrinks by >>>> 256. So a net gain of 112 bytes with my config. Conclusion is that >>>> as long as we got five engines it is not that interesting to get rid >>>> of the table. > > Since the semaphore matrix is only relevant to a specific gen, you could > remove it from the multi-generational engine-list and instead just have > it in the gen-specific code that needs it. That way it won't continue to > grow as new engines are added. The one gen that needs it is fixed at > 4x4, so it could just be a 16-byte lookup table, or 32 bits > (0b11001001_10110001_00101101_10010011) if you really want to save space > ;-) Not so much save space today as prevent from it exploding in the future. Rewriting the table to use hw_ids sounds like the best idea so far so I'll go with that. Regards, Tvrtko
On 22/07/16 13:51, Tvrtko Ursulin wrote: > > On 22/07/16 13:42, Dave Gordon wrote: >> On 21/07/16 14:46, Tvrtko Ursulin wrote: >>> On 21/07/16 14:31, Chris Wilson wrote: >>>> On Thu, Jul 21, 2016 at 02:16:22PM +0100, Tvrtko Ursulin wrote: >>>>> >>>>> On 21/07/16 13:59, Chris Wilson wrote: >>>>>> On Thu, Jul 21, 2016 at 01:00:47PM +0100, Tvrtko Ursulin wrote: >>>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>>>>> >>>>>>> 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. >>>>>> >>>>>> So just how big was that table? How big are the functions replacing >>>>>> it? >>>>> >>>>> With I915_NUM_ENGINES of 5 table is 5 * 5 * (2 * 4) = 200 bytes. >>>>> >>>>> With the patch .text grows by 144 bytes here and .rodata shrinks by >>>>> 256. So a net gain of 112 bytes with my config. Conclusion is that >>>>> as long as we got five engines it is not that interesting to get rid >>>>> of the table. >> >> Since the semaphore matrix is only relevant to a specific gen, you could >> remove it from the multi-generational engine-list and instead just have >> it in the gen-specific code that needs it. That way it won't continue to >> grow as new engines are added. The one gen that needs it is fixed at >> 4x4, so it could just be a 16-byte lookup table, or 32 bits >> (0b11001001_10110001_00101101_10010011) if you really want to save space >> ;-) > > Not so much save space today as prevent from it exploding in the future. > > Rewriting the table to use hw_ids sounds like the best idea so far so > I'll go with that. > > Regards, > > Tvrtko + * This distilled to integers looks like this: + * + * | 0 | 1 | 2 | 3 + * --+-----+-----+-----+----- + * 0 | -1 | 0 | 2 | 1 + * 1 | 2 | -1 | 0 | 1 + * 2 | 0 | 2 | -1 | 1 + * 3 | 2 | 1 | 0 | -1 static unsigned int sem_wait_mbox(unsigned int x, unsigned int y) { const unsigned int z = 0b11000110011110000100111001100011; return (z >> (8*y + 2*x)) & 3; } x 0 1 2 3 ------- 0: 3 0 2 1 y 1: 2 3 0 1 2: 0 2 3 1 3: 2 1 0 3 No complicated code, and hardly any table :) .Dave.
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 9397ddec26b9..c2fe718582c8 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..19455b20b322 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -209,3 +209,96 @@ int intel_engine_init_common(struct intel_engine_cs *engine) return i915_cmd_parser_init_ring(engine); } + +#define I915_NUM_GEN6_SEMAPHORE_ENGINES (4) + +/* + * For Gen6 semaphores where the driver issues MI_SEMAPHORE_MBOX commands + * with register selects so that a specific engine can wake up another engine + * waiting on a matching register, the matrix of required register selects + * looks like this: + * + * | RCS | VCS | BCS | VECS + * -----+---------------------------+---------------------------+---------------------------+--------------------------- + * RCS | MI_SEMAPHORE_SYNC_INVALID | MI_SEMAPHORE_SYNC_VR | MI_SEMAPHORE_SYNC_BR | MI_SEMAPHORE_SYNC_VER + * VCS | MI_SEMAPHORE_SYNC_RV | MI_SEMAPHORE_SYNC_INVALID | MI_SEMAPHORE_SYNC_BV | MI_SEMAPHORE_SYNC_VEV + * BCS | MI_SEMAPHORE_SYNC_RB | MI_SEMAPHORE_SYNC_VB | MI_SEMAPHORE_SYNC_INVALID | MI_SEMAPHORE_SYNC_VEB + * VECS | MI_SEMAPHORE_SYNC_RVE | MI_SEMAPHORE_SYNC_VVE | MI_SEMAPHORE_SYNC_BVE | MI_SEMAPHORE_SYNC_INVALID + * + * This distilled to integers looks like this: + * + * | 0 | 1 | 2 | 3 + * --+-----+-----+-----+----- + * 0 | -1 | 0 | 2 | 1 + * 1 | 2 | -1 | 0 | 1 + * 2 | 0 | 2 | -1 | 1 + * 3 | 2 | 1 | 0 | -1 + * + * In the opposite direction, the same table showing register addresses is: + * + * | RCS | VCS | BCS | VECS + * -----+--------------+--------------+--------------+-------------- + * RCS | GEN6_NOSYNC | GEN6_RVSYNC | GEN6_RBSYNC | GEN6_RVESYNC + * VCS | GEN6_VRSYNC | GEN6_NOSYNC | GEN6_VBSYNC | GEN6_VVESYNC + * BCS | GEN6_VRSYNC | GEN6_BVSYNC | GEN6_NOSYNC | GEN6_BVESYNC + * VECS | GEN6_VERSYNC | GEN6_VEVSYNC | GEN6_VEBSYNC | GEN6_NOSYNC + * + * Again this distilled to integers looks like this: + * + * | 0 | 1 | 2 | 3 + * --+-----+-----+-----+----- + * 0 | -1 | 0 | 1 | 2 + * 1 | 1 | -1 | 0 | 2 + * 2 | 0 | 1 | -1 | 2 + * 3 | 1 | 2 | 0 | -1 + * + * The function gen6_sem_f expresses the above table. We also notice that the + * difference between the first and second tabe is only a transpose of ones to + * twos and twos to ones. + */ + +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; + + if (x >= I915_NUM_GEN6_SEMAPHORE_ENGINES || + y >= I915_NUM_GEN6_SEMAPHORE_ENGINES) + return -1; + + 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; + + return r << 16; +} + +i915_reg_t gen6_signal_reg(enum intel_engine_id x, enum intel_engine_id y) +{ + int r; + + r = gen6_sem_f(x, y); + if (r < 0) + return GEN6_NOSYNC; + + return RING_SYNC(intel_engines[y].mmio_base, r); +} diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 0b5d1de8a7fb..fabf8b6bcae6 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -2745,44 +2745,8 @@ static void intel_ring_init_semaphores(struct drm_i915_private *dev_priv, * sema between VCS2 and RCS later. */ for (i = 0; i < I915_NUM_ENGINES; 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 }, - }, - [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 }, - }, - [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 }, - }, - [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 }, - }, - }; - u32 wait_mbox; - i915_reg_t mbox_reg; - - if (i == engine->id || i == VCS2) { - 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; - } - - engine->semaphore.mbox.wait[i] = wait_mbox; - engine->semaphore.mbox.signal[i] = mbox_reg; + engine->semaphore.mbox.wait[i] = gen6_wait_mbox(engine->id, i); + engine->semaphore.mbox.signal[i] = gen6_signal_reg(engine->id, i); } } } diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 0f8019488d33..cb3d22752b07 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -493,6 +493,9 @@ int intel_ring_invalidate_all_caches(struct drm_i915_gem_request *req); int intel_init_pipe_control(struct intel_engine_cs *engine, int size); void intel_fini_pipe_control(struct intel_engine_cs *engine); +u32 gen6_wait_mbox(enum intel_engine_id x, enum intel_engine_id y); +i915_reg_t gen6_signal_reg(enum intel_engine_id x, enum intel_engine_id y); + void intel_engine_setup_common(struct intel_engine_cs *engine); int intel_engine_init_common(struct intel_engine_cs *engine);