Re: [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests

From: Dave Gordon <david.s.gordon@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Jesse Barnes <jbarnes@virtuousgeek.org>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests
Date: Tue, 12 Jan 2016 13:02:27 +0000	[thread overview]
Message-ID: <5694F963.50906@intel.com> (raw)
In-Reply-To: <20160107165300.GR652@nuc-i3427.alporthouse.com>

On 07/01/16 16:53, Chris Wilson wrote:
> On Thu, Jan 07, 2016 at 08:49:38AM -0800, Jesse Barnes wrote:
>> On 01/07/2016 03:58 AM, Chris Wilson wrote:
>>> On Thu, Jan 07, 2016 at 10:20:50AM +0000, Dave Gordon wrote:
>>>> There are a number of places where the driver needs a request, but isn't
>>>> working on behalf of any specific user or in a specific context. At
>>>> present, we associate them with the per-engine default context. A future
>>>> patch will abolish those per-engine context pointers; but we can already
>>>> eliminate a lot of the references to them, just by making the allocator
>>>> allow NULL as a shorthand for "an appropriate context for this ring",
>>>> which will mean that the callers don't need to know anything about how
>>>> the "appropriate context" is found (e.g. per-ring vs per-device, etc).
>>>>
>>>> So this patch renames the existing i915_gem_request_alloc(), and makes
>>>> it local (static inline), and replaces it with a wrapper that provides
>>>> a default if the context is NULL, and also has a nicer calling
>>>> convention (doesn't require a pointer to an output parameter). Then we
>>>> change all callers to use the new convention:
>>>> OLD:
>>>> 	err = i915_gem_request_alloc(ring, user_ctx, &req);
>>>> 	if (err) ...
>>>> NEW:
>>>> 	req = i915_gem_request_alloc(ring, user_ctx);
>>>> 	if (IS_ERR(req)) ...
>>>> OLD:
>>>> 	err = i915_gem_request_alloc(ring, ring->default_context, &req);
>>>> 	if (err) ...
>>>> NEW:
>>>> 	req = i915_gem_request_alloc(ring, NULL);
>>>> 	if (IS_ERR(req)) ...
>>>
>>> Nak. You haven't fixed i915_gem_request_alloc() at all.
>>>
>>> http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=breadcrumbs&id=82c72e1a2b4385f0ab07dccee45acef38303e96f
>>> is the patch I have been carrying ever since.
>>
>> Can we stop with the "nak"?  This patch wraps the request alloc
>> differently than yours, but you haven't given details as to why you
>> think it's incorrect (see Dave's reply).
>
> I am annoyed that people do not review my patches and are duplicating
> work I did last year and the year before, without attempting to fix
> real bugs.
> -Chris

Chris, this patchset is totally directed towards fixing a specific bug, 
one which, moreover, arose a consequence of a patch YOU wrote:
b0366a5 drm/i915: intel_ring_initialized() must be simple and inline
(mea culpa too, obviously, since I was the one who rebased & pushed it).
Nick has a fix for the original bug, which involves reversing the 
teardown order, but can't now use it since b0366a5, so the bug remains. 
Nick's fix can be made to work if we replace the per-engine default 
contexts with the global one, which you've already agreed is a good idea 
(I think it was your idea in the first place!).

We can't take your patch because it doesn't apply to nightly. If you 
provide a standalone version that's not entangled with 100 other patches 
I'll happily review it. Or I might cherry-pick your existing one out of 
the 190-element patchset and try to rebase it onto nightly, which is how 
b0366a5 got in in the first place. I suspect it would look very much 
like mine then ...

Maybe we should just revert b0366a5 instead? Even though it was quite 
nice in itself ...

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