[Intel-gfx] [PATCH 15/22] drm/i915: Remove highly confusing i915_gem_obj_ggtt_pin()

[Chris Wilson]

On Thu, Jul 28, 2016 at 02:53:03PM +0300, Joonas Lahtinen wrote:
> On to, 2016-07-28 at 12:36 +0100, Chris Wilson wrote:
> > On Thu, Jul 28, 2016 at 01:38:14PM +0300, Joonas Lahtinen wrote:
> > > 
> > > On ke, 2016-07-27 at 12:14 +0100, Chris Wilson wrote:
> > > > 
> > > > Since i915_gem_obj_ggtt_pin() is an idiom breaking curry function for
> > > > i915_gem_object_ggtt_pin(), spare us the confustion and remove it.
> > > confustion should be combustion or confusion
> > > 
> > > > 
> > > > @@ -3741,7 +3742,8 @@ i915_gem_object_ggtt_pin(struct
> > > > drm_i915_gem_object *obj,
> > > >         struct i915_vma *vma;
> > > >         int ret;
> > > >  
> > > > -       BUG_ON(!view);
> > > > +       if (!view)
> > > > +               view = &i915_ggtt_view_normal;
> > > > 
> > > Convert other calling sites to take advantage of this if,
> > Patches have already been sent to do so, and will be along later as part
> > of the VMA handling outside of execbuf.
> > 
> > > 
> > > and also fix the error handling;
> > Hmm, what have I missed?
> > 
> > > 
> > > "i915_is_ggtt(vm) ? &i915_ggtt_view_normal : NULL"
> > > is used in i915_gem_object_pin and i915_gem_obj_lookup_or_create_vma
> > They are queued for removal.
> > 
> 
> Those two calling sites will rely on the BUG_ON(!view) and pass NULL to
> trigger it. So the BUG on should be upper level temporarily to help
> bisecting in future.

I'm still lost.

i915_gem_obj_lookup_or_create_ggtt_vma(), view is always !NULL (single
caller of i915_gem_object_ggtt_pin).
i915_gem_obj_lookup_or_create_vma(), vm could be NULL (though neither of
the two callers do pass NULL), but there isn't a single uppper layer to
insert a BUG.

i915_gem_object_pin() doesn't exist, i915_gem_object_ggtt_pin() is the
only remaining similar function. (The other pin is i915_vma_pin.) vm
there is implied to be &dev_priv->ggtt, and the view is either supplied
by the caller or set to the default normal view.

Do you want i915_gem_obj_lookup_or_create_ggtt_vma() to inherit the
BUG_ON(!view) removed from i915_gem_object_ggtt_pin() by this patch?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre