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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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


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