Re: [PATCH 2/5] drm/i915: Fallback to using CPU relocations for large batch buffers

From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Daniel Vetter <daniel@ffwll.ch>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 2/5] drm/i915: Fallback to using CPU relocations for large batch buffers
Date: Wed, 28 Jan 2015 10:14:47 +0100	[thread overview]
Message-ID: <20150128091447.GG4764@phenom.ffwll.local> (raw)
In-Reply-To: <20150127214300.GB27629@nuc-i3427.alporthouse.com>

On Tue, Jan 27, 2015 at 09:43:00PM +0000, Chris Wilson wrote:
> On Tue, Jan 27, 2015 at 04:09:37PM +0100, Daniel Vetter wrote:
> > On Wed, Jan 14, 2015 at 11:20:56AM +0000, Chris Wilson wrote:
> > >  static int
> > >  i915_gem_execbuffer_reserve_vma(struct i915_vma *vma,
> > >  				struct intel_engine_cs *ring,
> > > @@ -536,14 +589,21 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma,
> > >  	int ret;
> > >  
> > >  	flags = 0;
> > > -	if (entry->flags & __EXEC_OBJECT_NEEDS_MAP)
> > > -		flags |= PIN_GLOBAL | PIN_MAPPABLE;
> > > -	if (entry->flags & EXEC_OBJECT_NEEDS_GTT)
> > > -		flags |= PIN_GLOBAL;
> > > -	if (entry->flags & __EXEC_OBJECT_NEEDS_BIAS)
> > > -		flags |= BATCH_OFFSET_BIAS | PIN_OFFSET_BIAS;
> > > +	if (!drm_mm_node_allocated(&vma->node)) {
> > > +		if (entry->flags & __EXEC_OBJECT_NEEDS_MAP)
> > > +			flags |= PIN_GLOBAL | PIN_MAPPABLE;
> > > +		if (entry->flags & EXEC_OBJECT_NEEDS_GTT)
> > > +			flags |= PIN_GLOBAL;
> > > +		if (entry->flags & __EXEC_OBJECT_NEEDS_BIAS)
> > > +			flags |= BATCH_OFFSET_BIAS | PIN_OFFSET_BIAS;
> > > +	}
> > 
> > Hm, aren't we always calling reserve un buffers we know we've just
> > unbound? Keeping the flags computation would at least be a good selfcheck
> > about the consistency of our placing decisions, so I'd like to keep it.

I still think this isn't required, even with the ping-pong preventer below
kept. Maybe add a WARN_ON(drm_mm_node_allocated); just for paranoia
instead?

> > 
> > >  
> > >  	ret = i915_gem_object_pin(obj, vma->vm, entry->alignment, flags);
> > > +	if ((ret == -ENOSPC  || ret == -E2BIG) &&
> > > +	    only_mappable_for_reloc(entry->flags))
> > > +		ret = i915_gem_object_pin(obj, vma->vm,
> > > +					  entry->alignment,
> > > +					  flags & ~(PIN_GLOBAL | PIN_MAPPABLE));
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > @@ -605,13 +665,13 @@ eb_vma_misplaced(struct i915_vma *vma)
> > >  	    vma->node.start & (entry->alignment - 1))
> > >  		return true;
> > >  
> > > -	if (entry->flags & __EXEC_OBJECT_NEEDS_MAP && !obj->map_and_fenceable)
> > > -		return true;
> > > -
> > >  	if (entry->flags & __EXEC_OBJECT_NEEDS_BIAS &&
> > >  	    vma->node.start < BATCH_OFFSET_BIAS)
> > >  		return true;
> > >  
> > > +	if (entry->flags & __EXEC_OBJECT_NEEDS_MAP && !obj->map_and_fenceable)
> > > +		return !only_mappable_for_reloc(entry->flags);
> > 
> > Hm, this seems to contradict your commit message a bit since it'll result
> > in a behavioural change of what we try to push into mappable for relocs.
> > 
> > Shouldn't we instead just clear the NEEDS_MAP flag in reserve_vma when the
> > mappable pin fails and we fall back?
> 
> This pair of chunks is required to prevent the ping-pong you just
> described, which was very slow.

Makes sense, but imo then requires a comment (e.g. "prevent costly
ping-pong just for relocations") and some words in the commmit message.
-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