[Intel-gfx] [PATCH] drm/i915: Fix pages pin counting around swizzle quirk

[Chris Wilson]

On Fri, Nov 04, 2016 at 10:50:44AM +0200, Joonas Lahtinen wrote:
> On ke, 2016-11-02 at 09:43 +0000, Chris Wilson wrote:
> > @@ -2458,17 +2459,16 @@ int __i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
> >  	if (err)
> >  		return err;
> >  
> > -	if (likely(obj->mm.pages)) {
> > -		__i915_gem_object_pin_pages(obj);
> > -		goto unlock;
> > -	}
> > -
> > -	GEM_BUG_ON(i915_gem_object_has_pinned_pages(obj));
> > +	if (unlikely(!obj->mm.pages)) {
> > +		GEM_BUG_ON(i915_gem_object_has_pinned_pages(obj));
> > +		err = ____i915_gem_object_get_pages(obj);
> > +		if (err)
> > +			goto unlock;
> >  
> > -	err = ____i915_gem_object_get_pages(obj);
> > -	if (!err)
> > -		atomic_set_release(&obj->mm.pages_pin_count, 1);
> > +		smp_mb__before_atomic();
> 
> This is not cool without atomic in sight. Inline wrap as
> __i915_gem_object_pages_mb() or something.

My first thought was to put in ____i915_gem_object_get_pages() since it
closes the action of setting up the obj->mm.pages and co. I didn't like
that because the association then with the use of the pages_pin_count as
the mutex was not as apparent. Now that you cannot see the atomic_inc()
at all here, you are left confused!

Would you rather this just used the raw atomic_inc() here?

> 
> > @@ -3707,6 +3707,7 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
> >  {
> >  	int ret = 0;
> >  
> > +	GEM_BUG_ON(!i915_gem_object_has_pinned_pages(vma->obj));
> 
> Rather confusing, simple mind would think as
> __i915_gem_object_pin_pages has GEM_BUG_ON(!obj->mm.pages),
> the next branch would never be taken?

GEM_BUG_ON(vma == obj) ? Sorry not parsing very well this morning.

GEM_BUG_ON(!obj->mm.pages) would be a weaker form of the above. The
challenge is to express that the vma->page is only valid for the current
lifespan of the obj->mm.pages, should we regenerate that sg_table, we
need to regenerate the vma->pages. So I want to say that we must be
holding a pages_pin_count to utilize the vma->pages.

> >  	if (vma->pages)
> >  		return 0;

-- 
Chris Wilson, Intel Open Source Technology Centre