On ke, 2016-09-14 at 07:52 +0100, Chris Wilson wrote:
> +static void gen8_emit_wa_tail(struct drm_i915_gem_request *request, u32 *out)
> {
> - struct intel_ring *ring = request->ring;
> - int ret;
> -
> - ret = intel_ring_begin(request, 6 + WA_TAIL_DWORDS);
> - if (ret)
> - return ret;
> + out[0] = MI_NOOP;
> + out[1] = MI_NOOP;
How about storing 'out' on the request and still using the emit
wrapper? I assume passing a separate variable might end up rather
fragile.
> -static int gen8_rcs_signal(struct drm_i915_gem_request *req)
> +static u32 *gen8_rcs_signal(struct drm_i915_gem_request *req, u32 *out)
> {
> - struct intel_ring *ring = req->ring;
> struct drm_i915_private *dev_priv = req->i915;
> struct intel_engine_cs *waiter;
> enum intel_engine_id id;
> - int ret, num_rings;
> -
> - num_rings = INTEL_INFO(dev_priv)->num_rings;
> - ret = intel_ring_begin(req, (num_rings-1) * 8);
> - if (ret)
> - return ret;
>
> for_each_engine_id(waiter, dev_priv, id) {
> u64 gtt_offset = req->engine->semaphore.signal_ggtt[id];
> if (gtt_offset == MI_SEMAPHORE_SYNC_INVALID)
> continue;
>
> - intel_ring_emit(ring, GFX_OP_PIPE_CONTROL(6));
> - intel_ring_emit(ring,
> - PIPE_CONTROL_GLOBAL_GTT_IVB |
> - PIPE_CONTROL_QW_WRITE |
> - PIPE_CONTROL_CS_STALL);
> - intel_ring_emit(ring, lower_32_bits(gtt_offset));
> - intel_ring_emit(ring, upper_32_bits(gtt_offset));
> - intel_ring_emit(ring, req->global_seqno);
> - intel_ring_emit(ring, 0);
> - intel_ring_emit(ring,
> - MI_SEMAPHORE_SIGNAL |
> - MI_SEMAPHORE_TARGET(waiter->hw_id));
> - intel_ring_emit(ring, 0);
> + *out++ = GFX_OP_PIPE_CONTROL(6);
> + *out++ = (PIPE_CONTROL_GLOBAL_GTT_IVB |
> + PIPE_CONTROL_QW_WRITE |
> + PIPE_CONTROL_CS_STALL);
> + *out++ = lower_32_bits(gtt_offset);
> + *out++ = upper_32_bits(gtt_offset);
> + *out++ = req->global_seqno;
> + *out++ = 0;
> + *out++ = (MI_SEMAPHORE_SIGNAL |
> + MI_SEMAPHORE_TARGET(waiter->hw_id));
> + *out++ = 0;
And this is mixing a different approach in... Pick one and convert
others to it. I like having the wrapper for easier grepping of emitted
commands, but YMMV.
> +static void gen6_sema_emit_request(struct drm_i915_gem_request *req,
> + u32 *out)
> {
> - int ret;
> -
> - ret = req->engine->semaphore.signal(req);
> - if (ret)
> - return ret;
> -
> - return i9xx_emit_request(req);
> + return i9xx_emit_request(req, req->engine->semaphore.signal(req, out));
Not sure if this chaining is beautiful, I'd split signal to its own
line of "out = ...".
Those addressed,
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
