| Message ID | 1423828140-10653-9-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 13, 2015 at 11:48:17AM +0000, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@Intel.com> > > The alloc_request() function does not actually return the newly allocated > request. Instead, it must be pulled from ring->outstanding_lazy_request. This > patch fixes this so that code can create a request and start using it knowing > exactly which request it actually owns. Why do we have different functions in the first place? -Chris
On Fri, Feb 13, 2015 at 12:21:29PM +0000, Chris Wilson wrote: > On Fri, Feb 13, 2015 at 11:48:17AM +0000, John.C.Harrison@Intel.com wrote: > > From: John Harrison <John.C.Harrison@Intel.com> > > > > The alloc_request() function does not actually return the newly allocated > > request. Instead, it must be pulled from ring->outstanding_lazy_request. This > > patch fixes this so that code can create a request and start using it knowing > > exactly which request it actually owns. > > Why do we have different functions in the first place? There seems to be a bit a layer fumble going on with the lrc alloc request also pinning the lrc context. We could pull that out and then share the function again since there's indeed no reason no to. At least afaics. Also we should probably assign the ctx (if there is any) right in the request alloc function so that these two bits are always tied together. -Daniel
On 25/02/2015 21:08, Daniel Vetter wrote: > On Fri, Feb 13, 2015 at 12:21:29PM +0000, Chris Wilson wrote: >> On Fri, Feb 13, 2015 at 11:48:17AM +0000, John.C.Harrison@Intel.com wrote: >>> From: John Harrison <John.C.Harrison@Intel.com> >>> >>> The alloc_request() function does not actually return the newly allocated >>> request. Instead, it must be pulled from ring->outstanding_lazy_request. This >>> patch fixes this so that code can create a request and start using it knowing >>> exactly which request it actually owns. >> Why do we have different functions in the first place? > There seems to be a bit a layer fumble going on with the lrc alloc request > also pinning the lrc context. We could pull that out and then share the > function again since there's indeed no reason no to. At least afaics. > > Also we should probably assign the ctx (if there is any) right in the > request alloc function so that these two bits are always tied together. > -Daniel See later patch 'set context in request creation...'. That moves the ctx assignment out of _i915_add_request() and into alloc_request() for the legacy code path. Thus bringing the legacy and lrc versions back into alignment. As for the pinning, I am leaving that exactly as is as I don't really know the ins and outs of it. One of the execlist experts might be able to comment as to whether that is the right place for it or not. Also, I am currently working on the conversion to struct fence. As part of that, the request allocation changes quite a bit. Rather than have two clones of the code that have to be independently maintained, I have a patch to unify the common portion. We then have i915_gem_request_alloc() that all the driver calls instead of the indirected function pointer. That then does all the common work and chains on to the legacy/execlist specific helper at the end (which currently only sets the ringbuffer field for legacy mode and also does the LRC pinning for execlist mode).
On Fri, Feb 27, 2015 at 12:34:29PM +0000, John Harrison wrote: > On 25/02/2015 21:08, Daniel Vetter wrote: > >On Fri, Feb 13, 2015 at 12:21:29PM +0000, Chris Wilson wrote: > >>On Fri, Feb 13, 2015 at 11:48:17AM +0000, John.C.Harrison@Intel.com wrote: > >>>From: John Harrison <John.C.Harrison@Intel.com> > >>> > >>>The alloc_request() function does not actually return the newly allocated > >>>request. Instead, it must be pulled from ring->outstanding_lazy_request. This > >>>patch fixes this so that code can create a request and start using it knowing > >>>exactly which request it actually owns. > >>Why do we have different functions in the first place? > >There seems to be a bit a layer fumble going on with the lrc alloc request > >also pinning the lrc context. We could pull that out and then share the > >function again since there's indeed no reason no to. At least afaics. > > > >Also we should probably assign the ctx (if there is any) right in the > >request alloc function so that these two bits are always tied together. > >-Daniel > > See later patch 'set context in request creation...'. That moves the ctx > assignment out of _i915_add_request() and into alloc_request() for the > legacy code path. Thus bringing the legacy and lrc versions back into > alignment. As for the pinning, I am leaving that exactly as is as I don't > really know the ins and outs of it. One of the execlist experts might be > able to comment as to whether that is the right place for it or not. > > Also, I am currently working on the conversion to struct fence. As part of > that, the request allocation changes quite a bit. Rather than have two > clones of the code that have to be independently maintained, I have a patch > to unify the common portion. We then have i915_gem_request_alloc() that all > the driver calls instead of the indirected function pointer. That then does > all the common work and chains on to the legacy/execlist specific helper at > the end (which currently only sets the ringbuffer field for legacy mode and > also does the LRC pinning for execlist mode). That sounds exactly like what I'd want to see here. So no need for additional frobbing in the mean time imo. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 7959dfa..92c183f 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1902,7 +1902,8 @@ struct drm_i915_private { /* Abstract the submission mechanism (legacy ringbuffer or execlists) away */ struct { int (*alloc_request)(struct intel_engine_cs *ring, - struct intel_context *ctx); + struct intel_context *ctx, + struct drm_i915_gem_request **req_out); int (*do_execbuf)(struct i915_execbuffer_params *params, struct drm_i915_gem_execbuffer2 *args, struct list_head *vmas); diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 61471e9..37dcc6f 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1353,6 +1353,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, struct i915_address_space *vm; struct i915_execbuffer_params params_master; /* XXX: will be removed later */ struct i915_execbuffer_params *params = ¶ms_master; + struct drm_i915_gem_request *request; const u32 ctx_id = i915_execbuffer2_get_context_id(*args); u32 dispatch_flags; int ret; @@ -1531,7 +1532,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, params->batch_obj_vm_offset = i915_gem_obj_offset(batch_obj, vm); /* Allocate a request for this batch buffer nice and early. */ - ret = dev_priv->gt.alloc_request(ring, ctx); + ret = dev_priv->gt.alloc_request(ring, ctx, &request); if (ret) goto err; diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 2f906a2..325ef2c 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -847,13 +847,17 @@ void intel_lr_context_unpin(struct intel_engine_cs *ring, } int intel_logical_ring_alloc_request(struct intel_engine_cs *ring, - struct intel_context *ctx) + struct intel_context *ctx, + struct drm_i915_gem_request **req_out) { struct drm_i915_gem_request *request; struct drm_i915_private *dev_private = ring->dev->dev_private; int ret; - if (ring->outstanding_lazy_request) + if (!req_out) + return -EINVAL; + + if ((*req_out = ring->outstanding_lazy_request) != NULL) return 0; request = kzalloc(sizeof(*request), GFP_KERNEL); @@ -888,7 +892,7 @@ int intel_logical_ring_alloc_request(struct intel_engine_cs *ring, i915_gem_context_reference(request->ctx); request->ringbuf = ctx->engine[ring->id].ringbuf; - ring->outstanding_lazy_request = request; + *req_out = ring->outstanding_lazy_request = request; return 0; } @@ -1041,6 +1045,7 @@ static int logical_ring_prepare(struct intel_ringbuffer *ringbuf, int intel_logical_ring_begin(struct intel_ringbuffer *ringbuf, struct intel_context *ctx, int num_dwords) { + struct drm_i915_gem_request *req; struct intel_engine_cs *ring = ringbuf->ring; struct drm_device *dev = ring->dev; struct drm_i915_private *dev_priv = dev->dev_private; @@ -1056,7 +1061,7 @@ int intel_logical_ring_begin(struct intel_ringbuffer *ringbuf, return ret; /* Preallocate the olr before touching the ring */ - ret = intel_logical_ring_alloc_request(ring, ctx); + ret = intel_logical_ring_alloc_request(ring, ctx, &req); if (ret) return ret; diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h index 2b1bf83..b4620b9 100644 --- a/drivers/gpu/drm/i915/intel_lrc.h +++ b/drivers/gpu/drm/i915/intel_lrc.h @@ -35,7 +35,8 @@ /* Logical Rings */ int __must_check intel_logical_ring_alloc_request(struct intel_engine_cs *ring, - struct intel_context *ctx); + struct intel_context *ctx, + struct drm_i915_gem_request **req_out); void intel_logical_ring_stop(struct intel_engine_cs *ring); void intel_logical_ring_cleanup(struct intel_engine_cs *ring); int intel_logical_rings_init(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index c80e20d..40b5d83 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -2085,13 +2085,18 @@ int intel_ring_idle(struct intel_engine_cs *ring) } int -intel_ring_alloc_request(struct intel_engine_cs *ring, struct intel_context *ctx) +intel_ring_alloc_request(struct intel_engine_cs *ring, + struct intel_context *ctx, + struct drm_i915_gem_request **req_out) { int ret; struct drm_i915_gem_request *request; struct drm_i915_private *dev_private = ring->dev->dev_private; - if (ring->outstanding_lazy_request) + if (!req_out) + return -EINVAL; + + if ((*req_out = ring->outstanding_lazy_request) != NULL) return 0; request = kzalloc(sizeof(*request), GFP_KERNEL); @@ -2109,7 +2114,7 @@ intel_ring_alloc_request(struct intel_engine_cs *ring, struct intel_context *ctx return ret; } - ring->outstanding_lazy_request = request; + *req_out = ring->outstanding_lazy_request = request; return 0; } @@ -2137,6 +2142,7 @@ static int __intel_ring_prepare(struct intel_engine_cs *ring, int intel_ring_begin(struct intel_engine_cs *ring, int num_dwords) { + struct drm_i915_gem_request *req; struct drm_i915_private *dev_priv = ring->dev->dev_private; int ret; @@ -2150,7 +2156,7 @@ int intel_ring_begin(struct intel_engine_cs *ring, return ret; /* Preallocate the olr before touching the ring */ - ret = intel_ring_alloc_request(ring, NULL); + ret = intel_ring_alloc_request(ring, NULL, &req); if (ret) return ret; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 74df0fc..fdeaa66 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -393,7 +393,8 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs *ring); int __must_check intel_ring_begin(struct intel_engine_cs *ring, int n); int __must_check intel_ring_cacheline_align(struct intel_engine_cs *ring); int __must_check intel_ring_alloc_request(struct intel_engine_cs *ring, - struct intel_context *ctx); + struct intel_context *ctx, + struct drm_i915_gem_request **req_out); static inline void intel_ring_emit(struct intel_engine_cs *ring, u32 data) {