Re: [PATCH 13/18] drm/i915: Stop passing caller's num_dwords to engine->semaphore.signal()

Joonas Lahtinen
2016-07-22T08:16:02Z

On ke, 2016-07-20 at 14:12 +0100, Chris Wilson wrote:
> Rather than pass in the num_dwords that the caller wishes to use after
> the signal command packet, split the breadcrumb emission into two phases
> and have both the signal and breadcrumb individiually acquire space on
> the ring. This makes the interface simpler for the reader, and will
> simplify for patches.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 51 ++++++++++++++-------------------
>  drivers/gpu/drm/i915/intel_ringbuffer.h |  4 +--
>  2 files changed, 23 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 907d933d62aa..9c66745fc8d7 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1308,10 +1308,8 @@ static void render_ring_cleanup(struct intel_engine_cs *engine)
>   intel_fini_pipe_control(engine);
>  }
>
> -static int gen8_rcs_signal(struct drm_i915_gem_request *signaller_req,
> -    unsigned int num_dwords)
> +static int gen8_rcs_signal(struct drm_i915_gem_request *signaller_req)
>  {
> -#define MBOX_UPDATE_DWORDS 8
>   struct intel_ring *signaller = signaller_req->ring;
>   struct drm_i915_private *dev_priv = signaller_req->i915;
>   struct intel_engine_cs *waiter;
> @@ -1319,10 +1317,7 @@ static int gen8_rcs_signal(struct drm_i915_gem_request *signaller_req,
>   int ret, num_rings;
>
>   num_rings = hweight32(INTEL_INFO(dev_priv)->ring_mask);
> - num_dwords += (num_rings-1) * MBOX_UPDATE_DWORDS;
> -#undef MBOX_UPDATE_DWORDS
> -
> - ret = intel_ring_begin(signaller_req, num_dwords);
> + ret = intel_ring_begin(signaller_req, (num_rings-1) * 8);

Magic number. Just make the defines GEN?_?CS_MBOX_UPDATE_DWORDS?
>   if (ret)
>   return ret;
>
> @@ -1346,14 +1341,13 @@ static int gen8_rcs_signal(struct drm_i915_gem_request *signaller_req,
>   MI_SEMAPHORE_TARGET(waiter->hw_id));
>   intel_ring_emit(signaller, 0);
>   }
> + intel_ring_advance(signaller);
>
>   return 0;
>  }
>
> -static int gen8_xcs_signal(struct drm_i915_gem_request *signaller_req,
> -    unsigned int num_dwords)
> +static int gen8_xcs_signal(struct drm_i915_gem_request *signaller_req)
>  {
> -#define MBOX_UPDATE_DWORDS 6
>   struct intel_ring *signaller = signaller_req->ring;
>   struct drm_i915_private *dev_priv = signaller_req->i915;
>   struct intel_engine_cs *waiter;
> @@ -1361,10 +1355,7 @@ static int gen8_xcs_signal(struct drm_i915_gem_request *signaller_req,
>   int ret, num_rings;
>
>   num_rings = hweight32(INTEL_INFO(dev_priv)->ring_mask);
> - num_dwords += (num_rings-1) * MBOX_UPDATE_DWORDS;
> -#undef MBOX_UPDATE_DWORDS
> -
> - ret = intel_ring_begin(signaller_req, num_dwords);
> + ret = intel_ring_begin(signaller_req, (num_rings-1) * 6);

Magic number, see above.

>   if (ret)
>   return ret;
>
> @@ -1386,12 +1377,12 @@ static int gen8_xcs_signal(struct drm_i915_gem_request *signaller_req,
>   MI_SEMAPHORE_TARGET(waiter->hw_id));
>   intel_ring_emit(signaller, 0);
>   }
> + intel_ring_advance(signaller);
>
>   return 0;
>  }
>
> -static int gen6_signal(struct drm_i915_gem_request *signaller_req,
> -        unsigned int num_dwords)
> +static int gen6_signal(struct drm_i915_gem_request *signaller_req)
>  {
>   struct intel_ring *signaller = signaller_req->ring;
>   struct drm_i915_private *dev_priv = signaller_req->i915;
> @@ -1399,12 +1390,8 @@ static int gen6_signal(struct drm_i915_gem_request *signaller_req,
>   enum intel_engine_id id;
>   int ret, num_rings;
>
> -#define MBOX_UPDATE_DWORDS 3
>   num_rings = hweight32(INTEL_INFO(dev_priv)->ring_mask);
> - num_dwords += round_up((num_rings-1) * MBOX_UPDATE_DWORDS, 2);
> -#undef MBOX_UPDATE_DWORDS
> -
> - ret = intel_ring_begin(signaller_req, num_dwords);
> + ret = intel_ring_begin(signaller_req, round_up((num_rings-1) * 3, 2));

Magic.

>   if (ret)
>   return ret;
>
> @@ -1422,6 +1409,7 @@ static int gen6_signal(struct drm_i915_gem_request *signaller_req,
>   /* If num_dwords was rounded, make sure the tail pointer is correct */
>   if (num_rings % 2 == 0)
>   intel_ring_emit(signaller, MI_NOOP);
> + intel_ring_advance(signaller);
>
>   return 0;
>  }
> @@ -1439,11 +1427,13 @@ static int gen6_emit_request(struct drm_i915_gem_request *req)
>   struct intel_ring *ring = req->ring;
>   int ret;
>
> - if (req->engine->semaphore.signal)
> - ret = req->engine->semaphore.signal(req, 4);
> - else
> - ret = intel_ring_begin(req, 4);
> + if (req->engine->semaphore.signal) {
> + ret = req->engine->semaphore.signal(req);
> + if (ret)
> + return ret;
> + }
>

An alias "engine" would be cool while touching, it is there for ring,
too.

> + ret = intel_ring_begin(req, 4);

Magic.

>   if (ret)
>   return ret;
>
> @@ -1464,10 +1454,13 @@ static int gen8_render_emit_request(struct drm_i915_gem_request *req)
>   struct intel_ring *ring = req->ring;
>   int ret;
>
> - if (engine->semaphore.signal)
> - ret = engine->semaphore.signal(req, 8);
> - else
> - ret = intel_ring_begin(req, 8);
> + if (engine->semaphore.signal) {
> + ret = engine->semaphore.signal(req);
> + if (ret)
> + return ret;
> + }
> +
> + ret = intel_ring_begin(req, 8);

Pre-existing magic, no excuse not to convert.

Regards, Joonas

>   if (ret)
>   return ret;
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 856b732ddbbd..08e86204a3d5 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -278,9 +278,7 @@ struct intel_engine_cs {
>   int (*sync_to)(struct drm_i915_gem_request *to_req,
>      struct intel_engine_cs *from,
>      u32 seqno);
> - int (*signal)(struct drm_i915_gem_request *signaller_req,
> -   /* num_dwords needed by caller */
> -   unsigned int num_dwords);
> + int (*signal)(struct drm_i915_gem_request *signaller_req);
>   } semaphore;
>
>   /* Execlists */
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation