Re: [PATCH 08/10] drm/i915: Prelude to splitting i915_gem_do_execbuffer in two

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Dec 10, 2014 at 04:33:14PM +0000, Dave Gordon wrote:
> On 10/12/14 10:58, Daniel Vetter wrote:
> > On Tue, Dec 09, 2014 at 12:59:11PM +0000, John.C.Harrison@Intel.com wrote:
> >> From: John Harrison <John.C.Harrison@Intel.com>
> >>
> >> The scheduler decouples the submission of batch buffers to the driver with their
> >> submission to the hardware. This basically means splitting the execbuffer()
> >> function in half. This change rearranges some code ready for the split to occur.
> > 
> > Now there's the curios question: Where will the split in top/bottom halves
> > be? Without that I have no idea whether it makes sense and so can't review
> > this patch. At least if the goal is to really prep for the scheduler and
> > not just move a few lines around ;-)
> > -Daniel
> > 
> [snip]
> >>  
> >> +	i915_gem_execbuffer_move_to_active(vmas, ring);
> >> +
> >> +	/* To be split into two functions here... */
> >> +
> >> +	/* Unconditionally invalidate gpu caches and ensure that we do flush
> >> +	 * any residual writes from the previous batch.
> >> +	 */
> >> +	ret = logical_ring_invalidate_all_caches(ringbuf);
> 
> It'll be where the marker comment is above. Ahead of that point is stuff
> to do with setting up software state; after that we're talking to h/w.
> When the scheduler code goes it, it decouples the two by interposing at
> this point. Then batches go into it with s/w state set up, but don't get
> to talk to the h/w until they're selected for execution, possibly in a
> different order.

Oops, I guess I need see a doctor to check my eyesight ;-)

Two comments about code that's still in the bottom half:
- There's lots of input validation code still below that cutoff point
  which needs to be moved above to still be able to return -EINVAL. Test
  coverage is the critical part here, but I think we've closed most of our
  gaps. I guess a future patch will address this?

- retire_commands and so add_request are also in the cutoff. For shrinker
  interactions to work seamlessly we need to have both the objects on the
  active list and the request in the lists. So I expect more untangling
  there to separate the request emission to rings from the bookkeeping
  parts in add_request.

Also move_to_active will fall apart once we start to reorder requests I
think. Need to think about this case more, but we definitely need some
testcases with depencies and reordering by the scheduler.
-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