Re: [RFC 00/21] Replace seqno values with request structures

[John Harrison <John.C.Harrison@Intel.com>]

On 19/10/2014 15:21, Daniel Vetter wrote:
> On Fri, Oct 10, 2014 at 01:03:17PM +0100, John Harrison wrote:
>> I have just posted an updated subset of the patch series. Note that one
>> patch has been inserted in the middle and the first one has been dropped.
>> The correct sequence is now:
>>
>>     01    drm/i915: Remove redundant parameter to
>>     i915_gem_object_wait_rendering__tail()
>>     02    drm/i915: Ensure OLS & PLR are always in sync
>>     03    drm/i915: Add reference count to request structure
>>     04    drm/i915: Add helper functions to aid seqno -> request transition
>>     05    drm/i915: Replace last_[rwf]_seqno with last_[rwf]_req
>>     06    drm/i915: Ensure requests stick around during waits
>>     07    drm/i915: Remove 'outstanding_lazy_seqno'
>>     08    drm/i915: Make 'i915_gem_check_olr' actually check by request
>>     not seqno
>>     09    drm/i915: Convert 'last_flip_req' to be a request not a seqno
>>     10    drm/i915: Convert i915_wait_seqno to i915_wait_request
>>     11    drm/i915: Convert 'i915_add_request' to take a request not a seqno
>>     12    drm/i915: Convert mmio_flip::seqno to struct request
>>     13    drm/i915: Convert 'flip_queued_seqno' into 'flip_queued_request'
>>     14    drm/i915: Connect requests to rings at creation not submission
>>     15    drm/i915: Convert most 'i915_seqno_passed' calls into
>>     'i915_gem_request_completed'
>>     16    drm/i915: Convert __wait_seqno() to __wait_request()
>>     17    drm/i915: Convert trace functions from seqno to request
>>     18    drm/i915: Convert 'trace_irq' to use requests rather than seqnos
>>     19    drm/i915: Convert semaphores to handle requests not seqnos
>>     20    drm/i915: Convert 'ring_idle()' to use requests not seqnos
>>     21    drm/i915: Remove 'obj->ring'
>>     22    drm/i915: Cache request completion status
>>     23    drm/i915: Zero fill the request structure
>>     24    drm/i915: Defer seqno allocation until actual hardware
>>     submission time
>>
>>
>> The whole set in its latest and greatest form has also been uploaded to the
>> drm-private git as 'topic/seqno-request'.
> Ok, read through the entire pile and looks good from a high level I think.
> Review summary is really just "please less BUG_ON and more commit
> message". I think even the few funky things can probably just be explained
> away with a good commit message.
A few of the BUG_ONs disappear again along the way and others are just 
converting existing BUG_ONs from seqnos to requests.

The minimal messaging was because the intention was to get something 
posted as soon as possible in order to be reviewed as soon as possible 
if only from a 'is this what you had in mind' point of view. I didn't 
realise you were going to be out of office for so long. There didn't 
seem much value in writing reams of description only to be told a day 
later that I've misunderstood your design spec and gone off in 
completely the wrong direction.


> That aside I think even for RFC the commit message really should be there,
> since that's what I need to be able to efficiently review patches. If the
> series is littered with FIXME comments all over that's ok (that's what an
> rfc is for really), but the justifications and tricky points should all be
> there. So I highly recommend to self-review patch series (using the review
> training slides) even for an rfc to make sure there's not too many bubbles
> in the pipeline.
>
> Presuming there's no dragon hidden in any of the open issues I'll sign up
> someone else for the in-depth review.
>
> Cheers, Daniel
>
>>
>> On 06/10/2014 15:15, John.C.Harrison@Intel.com wrote:
>>> From: John Harrison <John.C.Harrison@Intel.com>
>>>
>>> Work in progress for replacing seqno usage with requst structures.
>>>
>>> There is a general feeling that it is better to move away from using a simple
>>> integer 'seqno' value to track batch buffer completion. Instead, the request
>>> structure should be used. That provides for much more flexibility going
>>> forwards. Especially which things like a GPU scheduler (which can re-order batch
>>> buffers and hence seqnos after submission to the hardware), Android sync points
>>> and other such features which potentially make seqno usage more and more
>>> complex.
>>>
>>> The current set of patches do most of the seqno to request structure conversion.
>>> There are still a couple of direct seqno comparisons in the semaphore code. The
>>> final conversion of a seqno test into a 'completed' flag inside the request
>>> structure is still do to as well. Along with whatever changes are required to
>>> maintain such a flag.
>>>
>>> The patches are being posted now to make sure that the various people involved
>>> agree that it is heading in the right direction.
>>>
>>> [Patches against drm-intel-nightly tree fetched 11/09/2014]
>>>
>>> John Harrison (21):
>>>    Bug: missing i915_seqno_passed() call?
>>>    drm/i915: Remove redundant parameter to i915_gem_object_wait_rendering__tail()
>>>    drm/i915: Ensure OLS & PLR are always in sync
>>>    drm/i915: Add reference count to request structure
>>>    drm/i915: Add helper functions to aid seqno -> request transition
>>>    drm/i915: Replace last_[rwf]_seqno with last_[rwf]_req
>>>    drm/i915: Ensure requests stick around during waits
>>>    drm/i915: Remove 'outstanding_lazy_seqno'
>>>    drm/i915: Make 'i915_gem_check_olr' actually check by request not seqno
>>>    drm/i915: Convert 'last_flip_req' to be a request not a seqno
>>>    drm/i915: Convert i915_wait_seqno to i915_wait_request
>>>    drm/i915: Convert 'i915_add_request' to take a request not a seqno
>>>    drm/i915: Convert mmio_flip::seqno to struct request
>>>    drm/i915: Convert 'flip_queued_seqno' into 'flip_queued_request'
>>>    drm/i915: Convert most 'i915_seqno_passed' calls into 'i915_gem_request_completed'
>>>    drm/i915: Convert __wait_seqno() to __wait_request()
>>>    drm/i915: Convert trace functions from seqno to request
>>>    drm/i915: Convert 'trace_irq' to use requests rather than seqnos
>>>    drm/i915: Convert semaphores to handle requests not seqnos
>>>    drm/i915: Convert 'ring_idle()' to use requests not seqnos
>>>    drm/i915: Remove 'obj->ring'
>>>
>>>   drivers/gpu/drm/i915/i915_debugfs.c        |   23 +--
>>>   drivers/gpu/drm/i915/i915_drv.h            |   64 +++++--
>>>   drivers/gpu/drm/i915/i915_gem.c            |  265 ++++++++++++++--------------
>>>   drivers/gpu/drm/i915/i915_gem_context.c    |   12 +-
>>>   drivers/gpu/drm/i915/i915_gem_execbuffer.c |    8 +-
>>>   drivers/gpu/drm/i915/i915_gem_gtt.h        |    4 +-
>>>   drivers/gpu/drm/i915/i915_gem_tiling.c     |    2 +-
>>>   drivers/gpu/drm/i915/i915_gpu_error.c      |   19 +-
>>>   drivers/gpu/drm/i915/i915_irq.c            |   12 +-
>>>   drivers/gpu/drm/i915/i915_trace.h          |   47 ++---
>>>   drivers/gpu/drm/i915/intel_display.c       |   47 ++---
>>>   drivers/gpu/drm/i915/intel_drv.h           |    4 +-
>>>   drivers/gpu/drm/i915/intel_lrc.c           |   49 ++---
>>>   drivers/gpu/drm/i915/intel_overlay.c       |   17 +-
>>>   drivers/gpu/drm/i915/intel_ringbuffer.c    |   88 +++++----
>>>   drivers/gpu/drm/i915/intel_ringbuffer.h    |   24 ++-
>>>   16 files changed, 392 insertions(+), 293 deletions(-)
>>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>