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

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	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:44:14 +0100	[thread overview]
Message-ID: <55F150EE.1080503@linux.intel.com> (raw)
In-Reply-To: <20150909150300.GG32324@nuc-i3427.alporthouse.com>

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.

For the queueue/via_worker/deferred maybe _schedule_get_pages_worker?

Tvrtko