[Intel-gfx] [PATCH] drm/i915: Infrastructure for supporting different GGTT views per object

[Daniel Vetter]

On Mon, Dec 01, 2014 at 11:32:42AM +0000, Tvrtko Ursulin wrote:
> On 11/28/2014 05:31 PM, Daniel Vetter wrote:
> >On Thu, Nov 27, 2014 at 02:52:44PM +0000, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >>index 86cf428..6213c07 100644
> >>--- a/drivers/gpu/drm/i915/i915_gem.c
> >>+++ b/drivers/gpu/drm/i915/i915_gem.c
> >>@@ -2090,8 +2090,7 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
> >>  			/* For the unbound phase, this should be a no-op! */
> >>  			list_for_each_entry_safe(vma, v,
> >>  						 &obj->vma_list, vma_link)
> >>-				if (i915_vma_unbind(vma))
> >>-					break;
> >>+				i915_vma_unbind(vma);
> >
> >Why drop the early break if a vma_unbind fails? Looks like a superflous
> >hunk to me.
> 
> I wasn't sure about this. (Does it makes sense to try and unbind other VMAs
> if one couldn't be unbound?)
> 
> In fact, looking at it now, I am not sure about the unbind flow
> (i915_vma_unbind). Won't i915_gem_object_retire move all VMAs to inactive
> list on first VMA unbind? Retire only on last VMA going away?

Yeah only the first vma_unbind might fail with the current code. The
problem though is that you ignore all failures.

Aside: In general this is also about reducing the size of the diff to only
have essential changes. I'm happy when people throw in more cleanup, but
it should be done as follow-up/prep patches. This is because often the
only evidence for fixing a bug we have is "it bisects to this commit". So
making commits in complex code like gem minimal is the name of the game.

> >>@@ -5430,9 +5434,12 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
> >>  {
> >>  	struct i915_vma *vma;
> >>
> >>-	vma = list_first_entry(&obj->vma_list, typeof(*vma), vma_link);
> >>-	if (vma->vm != i915_obj_to_ggtt(obj))
> >>-		return NULL;
> >>+	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> >>+		if (vma->vm != i915_obj_to_ggtt(obj))
> >>+			continue;
> >>+		if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
> >>+			return vma;
> >>+	}
> >
> >We fairly put the ggtt vma into the head of the list. Imo better to keep
> >the head slot reserved for the ggtt normal view, might be some random code
> >relying upon this.
> 
> Ok.
> 
> >>diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> >>index 89a2f3d..77f1bdc 100644
> >>--- a/drivers/gpu/drm/i915/i915_gpu_error.c
> >>+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> >>@@ -717,10 +717,8 @@ static u32 capture_pinned_bo(struct drm_i915_error_buffer *err,
> >>  			break;
> >>
> >>  		list_for_each_entry(vma, &obj->vma_list, vma_link)
> >>-			if (vma->vm == vm && vma->pin_count > 0) {
> >>+			if (vma->vm == vm && vma->pin_count > 0)
> >>  				capture_bo(err++, vma);
> >>-				break;
> >
> >Not fully sure about this one, but can't hurt I guess.
> 
> Not sure if it is useful at the moment or at all?

Probably not useful right now. Otoh if we ever wire up the display fault
registers on modern platforms this migh become useful to cross-check that
the current display plane register settings match up with the
corresponding buffer. Won't hurt either though.

If you feel like make it a separate patch perhaps.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch