[Intel-gfx] [PATCH v4] drm/i915: Add soft-pinning API for execbuffer

[Daniel, Thomas]

> -----Original Message-----
> From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
> Sent: Wednesday, July 15, 2015 4:47 PM
> To: Daniel, Thomas
> Cc: Goel, Akash; intel-gfx at lists.freedesktop.org; Belgaumkar, Vinay; Winiarski,
> Michal; Zou, Nanhai
> Subject: Re: [PATCH v4] drm/i915: Add soft-pinning API for execbuffer
> 
> On Wed, Jul 15, 2015 at 03:41:49PM +0000, Daniel, Thomas wrote:
> > > -----Original Message-----
> > > From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
> > > Sent: Wednesday, July 15, 2015 4:06 PM
> > > To: Goel, Akash
> > > Cc: Daniel, Thomas; intel-gfx at lists.freedesktop.org; Belgaumkar, Vinay;
> > > Winiarski, Michal; Zou, Nanhai
> > > Subject: Re: [PATCH v4] drm/i915: Add soft-pinning API for execbuffer
> > >
> > > On Wed, Jul 15, 2015 at 08:25:23PM +0530, Goel, Akash wrote:
> > > > >>+int
> > > > >>+i915_gem_evict_for_vma(struct i915_vma *target)
> > > > >>+{
> > > > >>+	struct drm_mm_node *node, *next;
> > > > >>+
> > > > >>+	list_for_each_entry_safe(node, next,
> > > > >>+			&target->vm->mm.head_node.node_list,
> > > > >>+			node_list) {
> > > > >>+		struct i915_vma *vma;
> > > > >>+		int ret;
> > > > >>+
> > > > >>+		if (node->start + node->size <= target->node.start)
> > > > >>+			continue;
> > > > >>+		if (node->start >= target->node.start + target-
> >node.size)
> > > > >>+			break;
> > > > >>+
> > > > >>+		vma = container_of(node, typeof(*vma), node);
> > > > >>+
> > > > >>+		if (vma->pin_count) {
> > > > >>+			/* We may need to evict a buffer in the same
> batch */
> > > > >>+			if (!vma->exec_entry)
> > > > >>+				return -EBUSY;
> > > > >>+
> > > > >>+			if (vma->exec_entry->flags &
> EXEC_OBJECT_PINNED)
> > > > >>+				/* Overlapping fixed objects in the
> same batch
> > > > >>*/
> > > > >>+				return -EINVAL;
> > > > >>+
> > > > >>+			return -ENOSPC;
> > > >
> > > > Can we actually hit this condition, considering the soft pinned
> > > > objects are now on the front side of 'eb->vmas' list ?
> > > > If we do encounter such a case, it probably means that the
> > > > overlapping object is already pinned from some other path.
> > >
> > > Note that softpinned objects are only first on the second pass through
> > > the reservation.
> > Eh?  I modified i915_gem_execbuffer_reserve() to always put the softpinned
> vmas first so they should never collide with objects in the same execbuff.
> 
> That would be a severe performance penalty for the usual case where we
> only have to run once through the list.
How so?  execbuffer_reserve() already traverses the entire vma list and does a list_move or list_move_tail onto ordered_vmas for each entry.  I've simply created a new list for the softpinned vmas and spliced the two together at the end.

> 
> > This contains special stuff which I don't have visibility of (mm.interval_tree,
> vma.active).
> 
> Yes. I mentioned that using softpin introduced some perf regressions.
> -Chris
Can we get your improvements as follow-up patches?

Cheers,
Thomas.