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

[Dave Gordon]

On 28/07/16 16:10, Chris Wilson wrote:
> On Thu, Jul 28, 2016 at 01:48:32PM +0100, Dave Gordon wrote:
>> 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.
>
> I'm referring to the change that took it from the API here to what was
> merged, and so reverting back to what I originally wrote.
> -Chris

Whatever. Item 1 still needs to be a separate patch from 2&|3.

.Dave.