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

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre