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.
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/i915_drv.h | 3 +-
drivers/gpu/drm/i915/i915_gem.c | 82 ++++++++++--------------------
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 8 +--
drivers/gpu/drm/i915/i915_gem_request.c | 22 +++-----
drivers/gpu/drm/i915/i915_gem_request.h | 6 +--
drivers/gpu/drm/i915/i915_trace.h | 15 +++---
drivers/gpu/drm/i915/intel_display.c | 25 +++++----
drivers/gpu/drm/i915/intel_lrc.c | 6 +--
drivers/gpu/drm/i915/intel_overlay.c | 24 ++++-----
9 files changed, 77 insertions(+), 114 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 44e8738c5310..0c580124d46d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2786,8 +2786,7 @@ static inline void i915_gem_object_unpin_vmap(struct drm_i915_gem_object *obj)
int __must_check i915_mutex_lock_interruptible(struct drm_device *dev);
int i915_gem_object_sync(struct drm_i915_gem_object *obj,
- struct intel_engine_cs *to,
- struct drm_i915_gem_request **to_req);
+ struct drm_i915_gem_request *to);
void i915_vma_move_to_active(struct i915_vma *vma,
struct drm_i915_gem_request *req);
int i915_gem_dumb_create(struct drm_file *file_priv,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1c6beb154d07..5b5afdcd9634 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2550,47 +2550,35 @@ out:
static int
__i915_gem_object_sync(struct drm_i915_gem_object *obj,
- struct intel_engine_cs *to,
- struct drm_i915_gem_request *from_req,
- struct drm_i915_gem_request **to_req)
+ struct drm_i915_gem_request *to,
+ struct drm_i915_gem_request *from)
{
- struct intel_engine_cs *from;
int ret;
- from = from_req->engine;
- if (to == from)
+ if (to->engine == from->engine)
return 0;
- if (i915_gem_request_completed(from_req))
+ if (i915_gem_request_completed(from))
return 0;
if (!i915.semaphores) {
- struct drm_i915_private *i915 = from_req->i915;
- ret = __i915_wait_request(from_req,
- i915->mm.interruptible,
+ ret = __i915_wait_request(from,
+ to->i915->mm.interruptible,
NULL,
NO_WAITBOOST);
if (ret)
return ret;
- i915_gem_object_retire_request(obj, from_req);
+ i915_gem_object_retire_request(obj, from);
} else {
- int idx = intel_engine_sync_index(from, to);
- u32 seqno = i915_gem_request_get_seqno(from_req);
+ int idx = intel_engine_sync_index(from->engine, to->engine);
+ u32 seqno = i915_gem_request_get_seqno(from);
- WARN_ON(!to_req);
-
- if (seqno <= from->semaphore.sync_seqno[idx])
+ if (seqno <= from->engine->semaphore.sync_seqno[idx])
return 0;
- if (*to_req == NULL) {
- ret = i915_gem_request_alloc(to, to->default_context, to_req);
- if (ret)
- return ret;
- }
-
- trace_i915_gem_ring_sync_to(*to_req, from, from_req);
- ret = to->semaphore.sync_to(*to_req, from, seqno);
+ trace_i915_gem_ring_sync_to(to, from);
+ ret = to->engine->semaphore.sync_to(to, from->engine, seqno);
if (ret)
return ret;
@@ -2598,8 +2586,8 @@ __i915_gem_object_sync(struct drm_i915_gem_object *obj,
* might have just caused seqno wrap under
* the radar.
*/
- from->semaphore.sync_seqno[idx] =
- i915_gem_request_get_seqno(obj->last_read_req[from->id]);
+ from->engine->semaphore.sync_seqno[idx] =
+ i915_gem_request_get_seqno(obj->last_read_req[from->engine->id]);
}
return 0;
@@ -2609,17 +2597,12 @@ __i915_gem_object_sync(struct drm_i915_gem_object *obj,
* i915_gem_object_sync - sync an object to a ring.
*
* @obj: object which may be in use on another ring.
- * @to: ring we wish to use the object on. May be NULL.
- * @to_req: request we wish to use the object for. See below.
- * This will be allocated and returned if a request is
- * required but not passed in.
+ * @to: request we are wishing to use
*
* This code is meant to abstract object synchronization with the GPU.
- * Calling with NULL implies synchronizing the object with the CPU
- * rather than a particular GPU ring. Conceptually we serialise writes
- * between engines inside the GPU. We only allow one engine to write
- * into a buffer at any time, but multiple readers. To ensure each has
- * a coherent view of memory, we must:
+ * Conceptually we serialise writes between engines inside the GPU.
+ * We only allow one engine to write into a buffer at any time, but
+ * multiple readers. To ensure each has a coherent view of memory, we must:
*
* - If there is an outstanding write request to the object, the new
* request must wait for it to complete (either CPU or in hw, requests
@@ -2628,22 +2611,11 @@ __i915_gem_object_sync(struct drm_i915_gem_object *obj,
* - If we are a write request (pending_write_domain is set), the new
* request must wait for outstanding read requests to complete.
*
- * For CPU synchronisation (NULL to) no request is required. For syncing with
- * rings to_req must be non-NULL. However, a request does not have to be
- * pre-allocated. If *to_req is NULL and sync commands will be emitted then a
- * request will be allocated automatically and returned through *to_req. Note
- * that it is not guaranteed that commands will be emitted (because the system
- * might already be idle). Hence there is no need to create a request that
- * might never have any work submitted. Note further that if a request is
- * returned in *to_req, it is the responsibility of the caller to submit
- * that request (after potentially adding more work to it).
- *
* Returns 0 if successful, else propagates up the lower layer error.
*/
int
i915_gem_object_sync(struct drm_i915_gem_object *obj,
- struct intel_engine_cs *to,
- struct drm_i915_gem_request **to_req)
+ struct drm_i915_gem_request *to)
{
const bool readonly = obj->base.pending_write_domain == 0;
struct drm_i915_gem_request *req[I915_NUM_RINGS];
@@ -2652,9 +2624,6 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
if (!obj->active)
return 0;
- if (to == NULL)
- return i915_gem_object_wait_rendering(obj, readonly);
-
n = 0;
if (readonly) {
if (obj->last_write_req)
@@ -2665,7 +2634,7 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
req[n++] = obj->last_read_req[i];
}
for (i = 0; i < n; i++) {
- ret = __i915_gem_object_sync(obj, to, req[i], to_req);
+ ret = __i915_gem_object_sync(obj, to, req[i]);
if (ret)
return ret;
}
@@ -2783,9 +2752,9 @@ int i915_gpu_idle(struct drm_device *dev)
if (!i915.enable_execlists) {
struct drm_i915_gem_request *req;
- ret = i915_gem_request_alloc(ring, ring->default_context, &req);
- if (ret)
- return ret;
+ req = i915_gem_request_alloc(ring, ring->default_context);
+ if (IS_ERR(req))
+ return PTR_ERR(req);
ret = i915_switch_context(req);
i915_add_request_no_flush(req);
@@ -4263,8 +4232,9 @@ i915_gem_init_hw(struct drm_device *dev)
WARN_ON(!ring->default_context);
- ret = i915_gem_request_alloc(ring, ring->default_context, &req);
- if (ret) {
+ req = i915_gem_request_alloc(ring, ring->default_context);
+ if (IS_ERR(req)) {
+ ret = PTR_ERR(req);
i915_gem_cleanup_ringbuffer(dev);
goto out;
}
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index a56fae99a1bc..3956d74d8c8c 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -952,7 +952,7 @@ i915_gem_execbuffer_move_to_gpu(struct drm_i915_gem_request *req,
struct drm_i915_gem_object *obj = vma->obj;
if (obj->active & other_rings) {
- ret = i915_gem_object_sync(obj, req->engine, &req);
+ ret = i915_gem_object_sync(obj, req);
if (ret)
return ret;
}
@@ -1595,9 +1595,11 @@ 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 = i915_gem_request_alloc(ring, ctx, ¶ms->request);
- if (ret)
+ params->request = i915_gem_request_alloc(ring, ctx);
+ if (IS_ERR(params->request)) {
+ ret = PTR_ERR(params->request);
goto err_batch_unpin;
+ }
ret = i915_gem_request_add_to_client(params->request, file);
if (ret) {
diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index e911430575fe..ce663acc9c7d 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -195,9 +195,9 @@ i915_gem_get_seqno(struct drm_i915_private *dev_priv, u32 *seqno)
return 0;
}
-int i915_gem_request_alloc(struct intel_engine_cs *engine,
- struct intel_context *ctx,
- struct drm_i915_gem_request **req_out)
+struct drm_i915_gem_request *
+i915_gem_request_alloc(struct intel_engine_cs *engine,
+ struct intel_context *ctx)
{
struct drm_i915_private *dev_priv = engine->i915;
unsigned reset_counter = i915_reset_counter(&dev_priv->gpu_error);
@@ -205,22 +205,17 @@ int i915_gem_request_alloc(struct intel_engine_cs *engine,
u32 seqno;
int ret;
- if (!req_out)
- return -EINVAL;
-
- *req_out = NULL;
-
/* ABI: Before userspace accesses the GPU (e.g. execbuffer), report
* EIO if the GPU is already wedged, or EAGAIN to drop the struct_mutex
* and restart.
*/
ret = i915_gem_check_wedge(reset_counter, dev_priv->mm.interruptible);
if (ret)
- return ret;
+ return ERR_PTR(ret);
req = kmem_cache_zalloc(dev_priv->requests, GFP_KERNEL);
if (req == NULL)
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);
ret = i915_gem_get_seqno(dev_priv, &seqno);
if (ret)
@@ -265,15 +260,14 @@ int i915_gem_request_alloc(struct intel_engine_cs *engine,
* free code.
*/
i915_gem_request_cancel(req);
- return ret;
+ return ERR_PTR(ret);
}
- *req_out = req;
- return 0;
+ return req;
err:
kmem_cache_free(dev_priv->requests, req);
- return ret;
+ return ERR_PTR(ret);
}
int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
index 086950567db4..2da9e0b5dfc7 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.h
+++ b/drivers/gpu/drm/i915/i915_gem_request.h
@@ -118,9 +118,9 @@ struct drm_i915_gem_request {
int elsp_submitted;
};
-int i915_gem_request_alloc(struct intel_engine_cs *ring,
- struct intel_context *ctx,
- struct drm_i915_gem_request **req_out);
+struct drm_i915_gem_request *
+i915_gem_request_alloc(struct intel_engine_cs *ring,
+ struct intel_context *ctx);
void i915_gem_request_cancel(struct drm_i915_gem_request *req);
int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
struct drm_file *file);
diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index 95cab4776401..85469e3c740a 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -460,10 +460,9 @@ TRACE_EVENT(i915_gem_evict_vm,
);
TRACE_EVENT(i915_gem_ring_sync_to,
- TP_PROTO(struct drm_i915_gem_request *to_req,
- struct intel_engine_cs *from,
- struct drm_i915_gem_request *req),
- TP_ARGS(to_req, from, req),
+ TP_PROTO(struct drm_i915_gem_request *to,
+ struct drm_i915_gem_request *from),
+ TP_ARGS(to, from),
TP_STRUCT__entry(
__field(u32, dev)
@@ -473,10 +472,10 @@ TRACE_EVENT(i915_gem_ring_sync_to,
),
TP_fast_assign(
- __entry->dev = from->dev->primary->index;
- __entry->sync_from = from->id;
- __entry->sync_to = to_req->engine->id;
- __entry->seqno = i915_gem_request_get_seqno(req);
+ __entry->dev = from->i915->dev->primary->index;
+ __entry->sync_from = from->engine->id;
+ __entry->sync_to = to->engine->id;
+ __entry->seqno = from->fence.seqno;
),
TP_printk("dev=%u, sync-from=%u, sync-to=%u, seqno=%u",
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f8717c5627dd..ec52fff7e0b0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11669,15 +11669,21 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
* into the display plane and skip any waits.
*/
if (!mmio_flip) {
- ret = i915_gem_object_sync(obj, ring, &request);
- if (ret)
+ request = i915_gem_request_alloc(ring, ring->default_context);
+ if (IS_ERR(request)) {
+ ret = PTR_ERR(request);
goto cleanup_pending;
+ }
+
+ ret = i915_gem_object_sync(obj, request);
+ if (ret)
+ goto cleanup_request;
}
ret = intel_pin_and_fence_fb_obj(crtc->primary, fb,
crtc->primary->state);
if (ret)
- goto cleanup_pending;
+ goto cleanup_request;
work->gtt_offset = intel_plane_obj_offset(to_intel_plane(primary),
obj, 0);
@@ -11691,23 +11697,15 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
i915_gem_request_assign(&work->flip_queued_req,
obj->last_write_req);
} else {
- if (!request) {
- ret = i915_gem_request_alloc(ring, ring->default_context, &request);
- if (ret)
- goto cleanup_unpin;
- }
-
ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, request,
page_flip_flags);
if (ret)
goto cleanup_unpin;
+ i915_add_request_no_flush(request);
i915_gem_request_assign(&work->flip_queued_req, request);
}
- if (request)
- i915_add_request_no_flush(request);
-
work->flip_queued_vblank = drm_crtc_vblank_count(crtc);
work->enable_stall_check = true;
@@ -11725,9 +11723,10 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
cleanup_unpin:
intel_unpin_fb_obj(fb, crtc->primary->state);
-cleanup_pending:
+cleanup_request:
if (request)
i915_add_request_no_flush(request);
+cleanup_pending:
atomic_dec(&intel_crtc->unpin_work_count);
mutex_unlock(&dev->struct_mutex);
cleanup:
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index b889680f7491..82b21a883732 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -629,7 +629,7 @@ static int execlists_move_to_gpu(struct drm_i915_gem_request *req,
struct drm_i915_gem_object *obj = vma->obj;
if (obj->active & other_rings) {
- ret = i915_gem_object_sync(obj, req->engine, &req);
+ ret = i915_gem_object_sync(obj, req);
if (ret)
return ret;
}
@@ -2264,8 +2264,8 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx,
if (ctx != engine->default_context && engine->init_context) {
struct drm_i915_gem_request *req;
- ret = i915_gem_request_alloc(engine, ctx, &req);
- if (ret) {
+ req = i915_gem_request_alloc(engine, ctx);
+ if (IS_ERR(req)) {
DRM_ERROR("ring create req: %d\n",
ret);
goto error_ringbuf;
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index cb73d16848b0..df71c01f28f1 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -240,9 +240,9 @@ static int intel_overlay_on(struct intel_overlay *overlay)
WARN_ON(overlay->active);
WARN_ON(IS_I830(dev) && !(dev_priv->quirks & QUIRK_PIPEA_FORCE));
- ret = i915_gem_request_alloc(ring, ring->default_context, &req);
- if (ret)
- return ret;
+ req = i915_gem_request_alloc(ring, ring->default_context);
+ if (IS_ERR(req))
+ return PTR_ERR(req);
ret = intel_ring_begin(req, 4);
if (ret) {
@@ -283,9 +283,9 @@ static int intel_overlay_continue(struct intel_overlay *overlay,
if (tmp & (1 << 17))
DRM_DEBUG("overlay underrun, DOVSTA: %x\n", tmp);
- ret = i915_gem_request_alloc(ring, ring->default_context, &req);
- if (ret)
- return ret;
+ req = i915_gem_request_alloc(ring, ring->default_context);
+ if (IS_ERR(req))
+ return PTR_ERR(req);
ret = intel_ring_begin(req, 2);
if (ret) {
@@ -349,9 +349,9 @@ static int intel_overlay_off(struct intel_overlay *overlay)
* of the hw. Do it in both cases */
flip_addr |= OFC_UPDATE;
- ret = i915_gem_request_alloc(ring, ring->default_context, &req);
- if (ret)
- return ret;
+ req = i915_gem_request_alloc(ring, ring->default_context);
+ if (IS_ERR(req))
+ return PTR_ERR(req);
ret = intel_ring_begin(req, 6);
if (ret) {
@@ -423,9 +423,9 @@ static int intel_overlay_release_old_vid(struct intel_overlay *overlay)
/* synchronous slowpath */
struct drm_i915_gem_request *req;
- ret = i915_gem_request_alloc(ring, ring->default_context, &req);
- if (ret)
- return ret;
+ req = i915_gem_request_alloc(ring, ring->default_context);
+ if (req)
+ return PTR_ERR(req);
ret = intel_ring_begin(req, 2);
if (ret) {
--
2.7.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
