Re: [Intel-gfx] [PATCH 09/18] drm/i915: Simplify request_alloc by returning the allocated request

[Chris Wilson]

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

[Joonas Lahtinen]

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] =
> +