Re: [PATCH 18/31] drm/i915: Simplify request_alloc by returning the allocated request

From: Dave Gordon <david.s.gordon@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 18/31] drm/i915: Simplify request_alloc by returning the allocated request
Date: Thu, 28 Jul 2016 13:48:32 +0100	[thread overview]
Message-ID: <f68041c7-dd75-d8a0-3613-af441786177d@intel.com> (raw)
In-Reply-To: <20160727152818.GR31051@nuc-i3427.alporthouse.com>

On 27/07/16 16:28, Chris Wilson wrote:
> On Wed, Jul 27, 2016 at 12:08:35PM +0100, Dave Gordon wrote:
>> On 25/07/16 10:18, Joonas Lahtinen wrote:
>>> On ma, 2016-07-25 at 08:44 +0100, Chris Wilson wrote:
>>>> If is simpler and leads to more readable code through the callstack if
>>>> the allocation returns the allocated struct through the return value.
>>>>
>>>> The importance of this is that it no longer looks like we accidentally
>>>> allocate requests as side-effect of calling .
>>>
>>> Dave seems to have expressed to wish to review this around January, so
>>> CC'ing him here.
>>>
>> >From me,
>>>
>>> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>>
>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>> ---
>>>> drivers/gpu/drm/i915/i915_drv.h            |  3 +-
>>>> drivers/gpu/drm/i915/i915_gem.c            | 75 ++++++++----------------------
>>>> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 12 ++---
>>>> drivers/gpu/drm/i915/i915_gem_request.c    | 58 ++++++++---------------
>>>> drivers/gpu/drm/i915/i915_trace.h          | 13 +++---
>>>> drivers/gpu/drm/i915/intel_display.c       | 36 ++++++--------
>>>> drivers/gpu/drm/i915/intel_lrc.c           |  2 +-
>>>> drivers/gpu/drm/i915/intel_overlay.c       | 20 ++++----
>>>> 8 files changed, 79 insertions(+), 140 deletions(-)
>>
>> The actual code looks fine, but the patch amalgamates several
>> different changes, only one  of which is mentioned in the
>> description above.
>>
>> 1. Change the signature of i915_gem_object_sync() and its internal
>>    subfunction __i915_gem_object_sync() to always require a request
>>    as input, with corresponding rework of the callsites. This enables
>>    the removal of the internal call to i915_gem_request_alloc().
>>
>> 2. Change API of i915_gem_request_alloc() not to allow NULL ctx,
>>    which involves changing various of the remaining callsites.
>
> Reverts the unwanted the change.

No, 2 and 3 together do NOT revert the change 268270883 because that's 
what originally changed the API of i915_gem_request_alloc() to return a 
pointer directly, rather than via an output parameter. Which everyone 
agrees is a good idea :)

Allowing NULL here was a useful step in the elimination of the per-ring 
default context (which was after all your idea, IIRC). Now that there 
aren't so many callsites it's not so ugly to have those callers mention 
the (now-unified) kernel context.

>> 3. Flatten __i915_gem_request_alloc() into i915_gem_request_alloc().
>>
>> The description really only covers (3), which is the simplest and
>> most mechanical of the changes. So preferably two or three separate
>> patches, in whatever order makes the most sense, with a bit more
>> description of each.
>
> It mentions 1 explicitly which was the reason why the API was originally
> intended to be this.
> -Chris

Strange, I can't see any *explicit* mention of i915_gem_object_sync(), 
only some vague allusion to "certain functions".

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