Re: [PATCH v6 14/25] drm/i915: Manually unwind after a failed request allocation

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	intel-gfx@lists.freedesktop.org,
	Mika Kuoppala <mika.kuoppala@linux.intel.com>,
	Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Subject: Re: [PATCH v6 14/25] drm/i915: Manually unwind after a failed request allocation
Date: Wed, 27 Apr 2016 14:03:28 +0100	[thread overview]
Message-ID: <5720B8A0.6040801@linux.intel.com> (raw)
In-Reply-To: <20160427125816.GD27856@nuc-i3427.alporthouse.com>

On 27/04/16 13:58, Chris Wilson wrote:
> On Wed, Apr 27, 2016 at 01:51:38PM +0100, Tvrtko Ursulin wrote:
>>
>> On 26/04/16 21:06, Chris Wilson wrote:
>>> In the next patches, we want to move the work out of freeing the request
>>> and into its retirement (so that we can free the request without
>>> requiring the struct_mutex). This means that we cannot rely on
>>> unreferencing the request to completely teardown the request any more
>>> and so we need to manually unwind the failed allocation. In doing so, we
>>> reorder the allocation in order to make the unwind simple (and ensure
>>> that we don't try to unwind a partial request that may have modified
>>> global state) and so we end up pushing the initial preallocation down
>>> into the engine request initialisation functions where we have the
>>> requisite control over the state of the request.
>>>
>>> Moving the initial preallocation into the engine is less than ideal: it
>>> moves logic to handle a specific problem with request handling out of
>>> the common code. On the other hand, it does allow those backends
>>> significantly more flexibility in performing its allocations.
>>
>> Could add _free_request_extras which would only be allowed to be
>> called from _alloc_request? That would enable not-polluting the
>> engine with common code I think.
>
> If you look at where I think it should be placed inside lrc, then we
> need multiple phases. Not that isn't much of a big deal:
>
> request_alloc:
>
> 	engine->pin_request()
>
> 	/* prep */
>
> 	engine->init_context()
>
> are more or less what we need, it will take a bit of organisation to
> align legacy / execlists. But it can be done.

Forgot to say, patch looks correct to me as it is. So r-b from that 
point of view anyway. Because in my mind solving the big performance 
sloppiness the series fixes is much more important than one (more) 
slight temporary design inelegance.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx