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

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Oct 20, 2014 at 11:19:28AM +0100, John Harrison wrote:
> 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.

Yeah, there's a bunch of preexisting ones that I've let slip through. But
I've screamed at a BUG_ON that killed my machine once too often in the
past few months, so I've started to be really strict about them. If
they're just temporary then a WARN_ON should be about as informative, or
just drop them since if a pointer is NULL you'll blow up anyway a few
instructions later with an Oops.

> 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.

Oh, I guess then I've looked too closely at it ;-) From the high-level
point it's pretty much what I expected, so lgtm.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch