Re: [Intel-gfx] [PATCH v3 2/3] drm/i915: Fix userptr deadlock with aliased GTT mmappings

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org
Subject: Re: [Intel-gfx] [PATCH v3 2/3] drm/i915: Fix userptr deadlock with aliased GTT mmappings
Date: Thu, 10 Sep 2015 10:51:30 +0100	[thread overview]
Message-ID: <20150910095130.GR32324@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <55F150EE.1080503@linux.intel.com>

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