On ti, 2016-04-26 at 21:06 +0100, Chris Wilson wrote: > In the next patches, we want to move the work out of freeing the request > and into its retirement (so that we can free the request without > requiring the struct_mutex). This means that we cannot rely on > unreferencing the request to completely teardown the request any more > and so we need to manually unwind the failed allocation. In doing so, we > reorder the allocation in order to make the unwind simple (and ensure > that we don't try to unwind a partial request that may have modified > global state) and so we end up pushing the initial preallocation down > into the engine request initialisation functions where we have the > requisite control over the state of the request. > > Moving the initial preallocation into the engine is less than ideal: it > moves logic to handle a specific problem with request handling out of > the common code. On the other hand, it does allow those backends > significantly more flexibility in performing its allocations. > Adding John as CC, > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> > Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_gem.c | 28 +++++++++------------------- > drivers/gpu/drm/i915/intel_lrc.c | 16 ++++++++++++++-- > drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +- > 3 files changed, 24 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 0e27484bd28a..d7ff5e79182f 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2766,15 +2766,6 @@ __i915_gem_request_alloc(struct intel_engine_cs *engine, > req->ctx = ctx; > i915_gem_context_reference(req->ctx); > > - if (i915.enable_execlists) > - ret = intel_logical_ring_alloc_request_extras(req); > - else > - ret = intel_ring_alloc_request_extras(req); > - if (ret) { > - i915_gem_context_unreference(req->ctx); > - goto err; > - } > - > /* > * Reserve space in the ring buffer for all the commands required to > * eventually emit this request. This is to guarantee that the > @@ -2783,20 +2774,19 @@ __i915_gem_request_alloc(struct intel_engine_cs *engine, > * away, e.g. because a GPU scheduler has deferred it. > */ > req->reserved_space = MIN_SPACE_FOR_ADD_REQUEST; > - ret = intel_ring_begin(req, 0); > - if (ret) { > - /* > - * At this point, the request is fully allocated even if not > - * fully prepared. Thus it can be cleaned up using the proper > - * free code. > - */ > - i915_gem_request_unreference(req); > - return ret; > - } > + > + if (i915.enable_execlists) > + ret = intel_logical_ring_alloc_request_extras(req); > + else > + ret = intel_ring_alloc_request_extras(req); > + if (ret) > + goto err_ctx; > > *req_out = req; > return 0; > > +err_ctx: > + i915_gem_context_unreference(ctx); > err: > kmem_cache_free(dev_priv->requests, req); > return ret; > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 910044cf143e..01517dd7069b 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -698,7 +698,7 @@ static int execlists_move_to_gpu(struct drm_i915_gem_request *req, > > int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request) > { > - int ret = 0; > + int ret; > > request->ringbuf = request->ctx->engine[request->engine->id].ringbuf; > > @@ -715,9 +715,21 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request > return ret; > } > > - if (request->ctx != request->i915->kernel_context) > + if (request->ctx != request->i915->kernel_context) { > ret = intel_lr_context_pin(request->ctx, request->engine); > + if (ret) > + return ret; > + } > > + ret = intel_ring_begin(request, 0); > + if (ret) > + goto err_unpin; > + > + return 0; > + > +err_unpin: > + if (request->ctx != request->i915->kernel_context) > + intel_lr_context_unpin(request->ctx, request->engine); > return ret; > } > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index ba5946b9fa06..1193372f74fd 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -2333,7 +2333,7 @@ int intel_engine_idle(struct intel_engine_cs *engine) > int intel_ring_alloc_request_extras(struct drm_i915_gem_request *request) > { > request->ringbuf = request->engine->buffer; > - return 0; > + return intel_ring_begin(request, 0); > } The names of these two _extras functions with the added functionality do not make sense. Regards, Joonas > > static int wait_for_space(struct drm_i915_gem_request *req, int bytes) -- Joonas Lahtinen Open Source Technology Center Intel Corporation _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx