Re: [PATCH 12/13] drm/i915: Consolidate legacy semaphore initialization

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxx>

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@xxxxxxxxx>

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@xxxxxxxxxxxx>
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


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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux