Re: [PATCH] drm/i915: Replace gen6 semaphore signal table with code

[Dave Gordon <david.s.gordon@intel.com>]

On 21/07/16 10:31, 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.
>
> 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  | 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.

Or you could have the caller pass the 'hw_id' (probably better than 
'guc_id') directly.

> +
> +	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?

> +
> +	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;
}

.Dave.

> +
> +	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 b844e6984ae7..049527d381de 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2731,44 +2731,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 05bab8bda63d..802adcd51569 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);
>
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx