[Intel-gfx] [PATCH 43/55] drm/i915: Refactor activity tracking for requests

[Joonas Lahtinen]

On ke, 2016-07-27 at 08:57 +0100, Chris Wilson wrote:
> On Wed, Jul 27, 2016 at 10:40:14AM +0300, Joonas Lahtinen wrote:
> > 
> > On ma, 2016-07-25 at 18:32 +0100, Chris Wilson wrote:
> > > 
> > > @@ -172,6 +176,24 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request)
> > >  	 */
> > >  	request->ring->last_retired_head = request->postfix;
> > >  
> > > +	/* Walk through the active list, calling retire on each. This allows
> > > +	 * objects to track their GPU activity and mark themselves as idle
> > > +	 * when their *last* active request is completed (updating state
> > > +	 * tracking lists for eviction, active references for GEM, etc).
> > > +	 *
> > > +	 * As the ->retire() may free the node, we decouple it first and
> > > +	 * pass along the auxiliary information (to avoid dereferencing
> > > +	 * the node after the callback).
> > > +	 */
> > > +	list_for_each_entry_safe(active, next, &request->active_list, link) {
> > > +		prefetchw(next);
> > Would not this be an improvement to go to list_for_each_entry{,_safe}
> > rather?
> It's been tried before. It's not a universal improvement. This loop is
> one of the top functions in the profiles when handling large request
> objects, with perf implying that the memory loads are where our time
> goes..

This maybe worth a comment on top of the prefetchw.

> > > @@ -705,10 +723,13 @@ int i915_wait_request(struct drm_i915_gem_request *req)
> > >  {
> > >  	int ret;
> > >  
> > > -	GEM_BUG_ON(!req);
> > >  	lockdep_assert_held(&req->i915->drm.struct_mutex);
> > > +	GEM_BUG_ON(list_empty(&req->link));
> > Humm, why no waiting on requests without the tracker object? Or then
> > need to use __i915_wait_request? Kerneldoc might be useful.
> ?
> 
> 

Ok, question was maybe irrelevant reading another round.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation