On pe, 2016-10-07 at 10:46 +0100, Chris Wilson wrote: > +static inline int __must_check > +i915_gem_object_pin_pages(struct drm_i915_gem_object *obj) > { > - BUG_ON(obj->pages == NULL); > - obj->pages_pin_count++; > + lockdep_assert_held(&obj->base.dev->struct_mutex);
\n here. > + if (obj->mm.pages_pin_count++) > + return 0; > + > + return __i915_gem_object_get_pages(obj); I also second John here, what's up with the dummy wrapper convetion. Add TODO comments if you intend to introduce new stuff relying on that. > +} > + > +static inline void > +__i915_gem_object_pin_pages(struct drm_i915_gem_object *obj) > +{ > + lockdep_assert_held(&obj->base.dev->struct_mutex); Ditto. > + GEM_BUG_ON(!obj->mm.pages); > + obj->mm.pages_pin_count++; > +} > + > +static inline bool > +i915_gem_object_has_pinned_pages(struct drm_i915_gem_object *obj) > +{ > + return obj->mm.pages_pin_count; > +} > + > +static inline void > +__i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj) > +{ > + lockdep_assert_held(&obj->base.dev->struct_mutex); > + GEM_BUG_ON(!i915_gem_object_has_pinned_pages(obj)); > + GEM_BUG_ON(!obj->mm.pages); \n at least here. > + obj->mm.pages_pin_count--; > } > <SNIP> > @@ -812,7 +809,7 @@ int i915_gem_obj_prepare_shmem_write(struct > drm_i915_gem_object *obj, > obj->cache_dirty = true; > > intel_fb_obj_invalidate(obj, ORIGIN_CPU); > - obj->dirty = 1; > + obj->mm.dirty = true; The type is not bool dirty : 1, do we get yelled at by compiler? > @@ -1268,7 +1261,7 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_private *i915, > goto out_unpin; > > intel_fb_obj_invalidate(obj, ORIGIN_CPU); > - obj->dirty = true; > + obj->mm.dirty = true; I assume not as this has not been changed. Consistency is good anyway. > @@ -2483,24 +2474,25 @@ i915_gem_object_get_pages(struct drm_i915_gem_object > *obj) > > lockdep_assert_held(&obj->base.dev->struct_mutex); > > - if (obj->pages) > + if (obj->mm.pages) > return 0; > > - if (obj->madv != I915_MADV_WILLNEED) { > + if (obj->mm.madv != I915_MADV_WILLNEED) { > DRM_DEBUG("Attempting to obtain a purgeable object\n"); > + __i915_gem_object_unpin_pages(obj); Confusing to have teardown of another function in here. > return -EFAULT; > } > > - BUG_ON(obj->pages_pin_count); > - > ret = ops->get_pages(obj); > - if (ret) > + if (ret) { > + __i915_gem_object_unpin_pages(obj); And if you like *really* have to, at least try not to duplicate code. Bonus points form moving this to be proper teardown path where it has a counter-part. Strange side effects on failing function call do not lead to very understandable code structure, but major spaghetti. Can be fixed in follow-up patch too. Reviewed-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com> Regards, Joonas -- Joonas Lahtinen Open Source Technology Center Intel Corporation _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx