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 certain functions. > > > >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. > 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 -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
