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.lahtinen@xxxxxxxxxxxxxxx>
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
