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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
