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

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

On 21/07/16 12:56, Dave Gordon wrote:
> 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.

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx