On Thu, Oct 13, 2016 at 02:04:18PM +0300, Joonas Lahtinen wrote:
> On pe, 2016-10-07 at 10:46 +0100, Chris Wilson wrote:
> > @@ -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.
It is here, so that the unlikely error handling is not inside the
inlined function, but where we expect the code to grow to handle the
more complex locking requirements.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
