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