Re: [PATCH v2 00/28] Replace seqno values with request structures

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Nov 21, 2014 at 09:52:37PM +0100, Daniel Vetter wrote:
> On Thu, Nov 20, 2014 at 09:32:34AM +0000, Daniel, Thomas wrote:
> > > -----Original Message-----
> > > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> > > Of Daniel Vetter
> > > Sent: Wednesday, November 19, 2014 7:29 PM
> > > To: Harrison, John C
> > > Cc: Intel-GFX@Lists.FreeDesktop.Org
> > > Subject: Re: [Intel-gfx] [PATCH v2 00/28] Replace seqno values with request
> > > structures
> > > 
> > > On Fri, Nov 14, 2014 at 12:18:51PM +0000, John.C.Harrison@Intel.com wrote:
> > > > From: John Harrison <John.C.Harrison@Intel.com>
> > > >
> > > > 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.
> > > >
> > > > This patch set does the work of converting most of the driver to use
> > > > request structures in preference to seqno values. The only place left
> > > > that still uses seqnos is the semaphore code. It was decided to leave
> > > > that alone for the time being as the semaphores are hardware based and
> > > > the hardware only understands seqno values.
> > > >
> > > > v2: Rebased to newer nightly tree which significantly changed some of
> > > > the display MMIO flip code (including making __wait_request public and
> > > > renaming it).
> > > >
> > > > [Patches against drm-intel-nightly tree fetched 13/11/2014]
> > > >
> > > > John Harrison (28):
> > > >   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: Convert i915_gem_ring_throttle to use requests
> > > >   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: Add IRQ friendly request deference facility
> > > >   drm/i915: Convert mmio_flip::seqno to struct request
> > > >   drm/i915: Convert __wait_seqno() to __wait_request()
> > > >   drm/i915: Remove obsolete seqno parameter from 'i915_add_request'
> > > >   drm/i915: Convert 'flip_queued_seqno' into 'flip_queued_request'
> > > >   drm/i915: Convert trace functions from seqno to request
> > > >   drm/i915: Convert 'trace_irq' to use requests rather than seqnos
> > > >   drm/i915: Convert 'ring_idle()' to use requests not seqnos
> > > >   drm/i915: Connect requests to rings at creation not submission
> > > >   drm/i915: Convert 'i915_seqno_passed' calls into
> > > 'i915_gem_request_completed'
> > > >   drm/i915: Remove the now redundant 'obj->ring'
> > > >   drm/i915: Cache request completion status
> > > >   drm/i915: Zero fill the request structure
> > > >   drm/i915: Spinlock protection for request list
> > > >   drm/i915: Interrupt driven request completion
> > > >   drm/i915: Remove obsolete parameter to
> > > i915_gem_request_completed()
> > > >   drm/i915: Add unique id to the request structure for debugging
> > > >   drm/i915: Additional request structure tracing
> > > 
> > > Erhm this stuff is blocking refactoring work since I don't want to wreak major
> > > havoc with the codebase to avoid rebase hell for you. That means reviewing
> > > and merging early, even if the tail of the patch series isn't 100% clear yet.
> > > 
> > > Imo everything past patch 21 is fairly optional icing on the cake here.
> > > And if there early patches are non-controversial then I'd like them to pull in
> > > even if there's some detail missing in a patch in the middle.
> > > 
> > > But I haven't seen r-b tags anywhere really :(
> > Well there is some review discussion going on, but my remaining gripe
> > about a double init for a new list isn't a showstopper.  So in the
> > interests of unblocking other work I will give my r-b to this v2
> > patchset.
> > 
> > Reviewed-by: Thomas Daniel <Thomas.Daniel@intel.com>
> 
> Ok, I've tried to vacuum this up but it conflicts. And this kind of
> large-scale refactoring isn't really the stuff I feel safe to fix up while
> applying. So can you please rebase this (perhaps together with Thomas just
> to make sure there's not last minute screwup and the r-b is still valid)?

Aside, for rebasing just do the up to patch 21 since I'm kinda not yet
sold on the caching. I think we should directly go to struct fence (which
we need anyway for android syncpts) which already has state caching. And
even if we merge these patches I prefer to give the first batch a bit of
time on the line to hang out and dry.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx