On Thu, Sep 10, 2015 at 10:44:14AM +0100, Tvrtko Ursulin wrote:
>
> On 09/09/2015 04:03 PM, Chris Wilson wrote:
> >On Wed, Sep 09, 2015 at 02:56:16PM +0100, Tvrtko Ursulin wrote:
> >>
> >>Hi,
> >>
> >>On 08/10/2015 09:51 AM, Chris Wilson wrote:
> >>>+out:
> >>> drm_free_large(pvec);
> >>> return ret;
> >>>+
> >>>+err:
> >>>+ /* No pages here, no need for the mmu-notifier to wake us */
> >>>+ __i915_gem_userptr_set_active(obj, false);
> >>>+err_active:
> >>>+ release_pages(pvec, pinned, 0);
> >>>+ goto out;
> >>> }
> >>
> >>I don't like the goto dance. Would something like the below be clearer?
> >
> >We can condense it if we use a bool active and then feed everything
> >through the single exit path:
> >
> > active = false;
> > if (pinned < 0)
> > ret = pinned, pinned = 0;
> > else if (pinned < num_pages)
> > ret = __i915_gem_userptr_get_pages_queue(obj, &active);
> > else
> > ret = __i915_gem_userptr_set_pages(obj, pvec, num_pages);
> > if (ret) {
> > __i915_gem_userptr_set_active(obj, active);
> > release_pages(pvec, pinned, 0);
> > }
> > drm_free_large(pvec);
> > return ret;
> >
> >Not happy with _queue. I guess i915_gem_userptr_get_pages_via_worker()
> >is better. Or i915_gem_userptr_get_pages_deferred().
>
> Looks much better on a glance. If release_pages with pinned = 0 is OK.
It does a loop over for(int i = 0; i < pinned; i++); so it is fine as it
was.
> For the queueue/via_worker/deferred maybe _schedule_get_pages_worker?
I want to keep the i915_gem_object_get_pages prefix (haven't yet broken
that pattern, so no reason to start now), but _schedule seems
reasonable.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html