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

[Dave Gordon]

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 at linux.intel.com>
>>>
>>>> Signed-off-by: Chris Wilson <chris at 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.