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@xxxxxxxxxxxxxxx> > >>> > >>>>Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > >>>>--- > >>>>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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
