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

[Chris Wilson]

On Fri, Nov 04, 2016 at 09:36:31AM +0000, Chris Wilson wrote:
> 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?

Actually, I like using atomics better here. It is definitely consistent
as we then don't mix the raw atomics and the helpers.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre