Re: [Intel-gfx] [PATCH 09/18] drm/i915: Simplify request_alloc by returning the allocated request
On Thu, Jul 21, 2016 at 04:07:59PM +0300, Joonas Lahtinen wrote: > On ke, 2016-07-20 at 14:12 +0100, Chris Wilson wrote: > > - 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); > > Will somebody go nuts for changing the tracing just like so? I remember > somebody treating it somewhat of an ABI. They do, but they shouldn't! The format is discoverable though the tracing interface It's a moot point here as the trace output is unchanged, just the trace point call simplified to match the parent. > > @@ -1636,7 +1635,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void > > *data, > > params->dispatch_flags = dispatch_flags; > > params->batch_obj = batch_obj; > > params->ctx = ctx; > > - params->request = req; > > Not sure why you especially want to always use params->request form? Stack deduplication. It'll be clearer eventually. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/18] drm/i915: Simplify request_alloc by returning the allocated request
On ke, 2016-07-20 at 14:12 +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.
>
> Signed-off-by: Chris Wilson
> ---
> 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(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f32ec6db5bfa..3f67431577e3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3168,8 +3168,7 @@ static inline void i915_gem_object_unpin_map(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 95dbcfd94a80..77d7c0b012f4 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2842,51 +2842,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 = i915_gem_request_get_engine(from_req);
> - 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 = to_i915(obj->base.dev);
> - ret = __i915_wait_request(from_req,
> - i915->mm.interruptible,
> + ret = __i915_wait_request(from,
> + from->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) {
> - struct drm_i915_gem_request *req;
> -
> - req = i915_gem_request_alloc(to, NULL);
> - if (IS_ERR(req))
> - return PTR_ERR(req);
> -
> - *to_req = req;
> - }
> -
> - 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);
Will somebody go nuts for changing the tracing just like so? I remember
somebody treating it somewhat of an ABI.
> + ret = to->engine->semaphore.sync_to(to, from->engine, seqno);
> if (ret)
> return ret;
>
> @@ -2894,8 +2878,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_
[Intel-gfx] [PATCH 09/18] drm/i915: Simplify request_alloc by returning the allocated request
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
---
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(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f32ec6db5bfa..3f67431577e3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3168,8 +3168,7 @@ static inline void i915_gem_object_unpin_map(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 95dbcfd94a80..77d7c0b012f4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2842,51 +2842,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 = i915_gem_request_get_engine(from_req);
- 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 = to_i915(obj->base.dev);
- ret = __i915_wait_request(from_req,
- i915->mm.interruptible,
+ ret = __i915_wait_request(from,
+ from->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) {
- struct drm_i915_gem_request *req;
-
- req = i915_gem_request_alloc(to, NULL);
- if (IS_ERR(req))
- return PTR_ERR(req);
-
- *to_req = req;
- }
-
- 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;
@@ -2894,8 +2878,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;
@@ -2905,17 +2889,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
