[Intel-gfx] [PATCH 12/13] drm/i915: Consolidate legacy semaphore initialization

[Tvrtko Ursulin]

On 29/06/16 17:24, Chris Wilson wrote:
> On Wed, Jun 29, 2016 at 05:14:11PM +0100, Tvrtko Ursulin wrote:
>>
>> On 29/06/16 17:00, Chris Wilson wrote:
>>> On Wed, Jun 29, 2016 at 04:41:58PM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 29/06/16 16:34, Chris Wilson wrote:
>>>>> On Wed, Jun 29, 2016 at 04:09:31PM +0100, Tvrtko Ursulin wrote:
>>>>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>>>
>>>>>> Replace per-engine initialization with a common half-programatic,
>>>>>> half-data driven code for ease of maintenance and compactness.
>>>>>>
>>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>>
>>>>> This is the biggest pill to swallow (since our 5x5 table is only
>>>>> sparsely populated), but it looks correct, and more importantly easier to
>>>>> read.
>>>>
>>>> Yeah I was out of ideas on how to improve it. Fresh mind needed to
>>>> try and spot a pattern in how MI_SEMAPHORE_SYNC_* and GEN6_*SYNC map
>>>> to bits and registers respectively, and write it as a function.
>>>
>>> It's actually a very simple cyclic function based on register
>>> offset = base + (signaler hw_id - waiter hw_id - 1) % num_rings.
>>>
>>> (The only real challenge is picking the direction.)
>>>
>>> commit c8c99b0f0dea1ced5d0e10cdb9143356cc16b484
>>> Author: Ben Widawsky <ben at bwidawsk.net>
>>> Date:   Wed Sep 14 20:32:47 2011 -0700
>>>
>>>      drm/i915: Dumb down the semaphore logic
>>>
>>>      While I think the previous code is correct, it was hard to follow and
>>>      hard to debug. Since we already have a ring abstraction, might as well
>>>      use it to handle the semaphore updates and compares.
>>
>> Should I try to go back to that then? Since I am not too happy with
>> the sparse table...
>>
>> This has passed CI so we could merge some of it if that would help
>> your series, or wait until I rework this patch.
>
> The rule of thumb is incremental improvements tell a better story and
> should be easier to find a misstep. (My personal experience says the
> longer I play with a patch the larger it gets...)
>
> In short, you've already consolidated a lot of duplication in the vfuncs
> that will make my life easier (after some rebasing joy). Anything more
> is just icing on the cake. :)

It also looks like I have broke something, wonder how CI did not catch 
it or I am imagining things. "drm/i915: Consolidate dispatch_execbuffer 
vfunc" looks wrong wrt add_request and dispatch_execbuffer for gen8+. 
Leaving it for tomorrow.

Regards,

Tvrtko