[Intel-gfx] [PATCH 05/10] drm/i915/guc: Split guc_wq_item_append

[Chris Wilson]

Quoting Michał Winiarski (2017-10-05 10:13:44)
> We're using a special preempt context for HW to preempt into. We don't
> want to emit any requests there, but we still need to wrap this context
> into a valid GuC work item.
> Let's cleanup the functions operating on GuC work items.
> We can extract guc_request_add - responsible for adding GuC work item and
> ringing the doorbell, and guc_wq_item_append - used by the function
> above, not tied to the concept of gem request.
> 
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Jeff Mcgee <jeff.mcgee at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Oscar Mateo <oscar.mateo at intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 55 ++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index e8052e86426d..2ce2bd6ed509 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -69,7 +69,7 @@
>   * WQ_TYPE_INORDER is needed to support legacy submission via GuC, which
>   * represents in-order queue. The kernel driver packs ring tail pointer and an
>   * ELSP context descriptor dword into Work Item.
> - * See guc_wq_item_append()
> + * See guc_add_request()
>   *
>   * ADS:
>   * The Additional Data Struct (ADS) has pointers for different buffers used by
> @@ -357,7 +357,7 @@ static void guc_stage_desc_init(struct intel_guc *guc,
>                  * submission or, in other words, not using a direct submission
>                  * model) the KMD's LRCA is not used for any work submission.
>                  * Instead, the GuC uses the LRCA of the user mode context (see
> -                * guc_wq_item_append below).
> +                * guc_add_request below).
>                  */
>                 lrc->context_desc = lower_32_bits(ce->lrc_desc);
>  
> @@ -409,22 +409,18 @@ static void guc_stage_desc_fini(struct intel_guc *guc,
>  
>  /* Construct a Work Item and append it to the GuC's Work Queue */
>  static void guc_wq_item_append(struct i915_guc_client *client,
> -                              struct drm_i915_gem_request *rq)
> +                              u32 target_engine, u32 context_desc,
> +                              u32 ring_tail, u32 fence_id)
>  {
>         /* wqi_len is in DWords, and does not include the one-word header */
>         const size_t wqi_size = sizeof(struct guc_wq_item);
>         const u32 wqi_len = wqi_size / sizeof(u32) - 1;
> -       struct intel_engine_cs *engine = rq->engine;
> -       struct i915_gem_context *ctx = rq->ctx;
>         struct guc_process_desc *desc = __get_process_desc(client);
>         struct guc_wq_item *wqi;
> -       u32 ring_tail, wq_off;
> +       u32 wq_off;
>  
>         lockdep_assert_held(&client->wq_lock);
>  
> -       ring_tail = intel_ring_set_tail(rq->ring, rq->tail) / sizeof(u64);
> -       GEM_BUG_ON(ring_tail > WQ_RING_TAIL_MAX);
> -
>         /* For now workqueue item is 4 DWs; workqueue buffer is 2 pages. So we
>          * should not have the case where structure wqi is across page, neither
>          * wrapped to the beginning. This simplifies the implementation below.
> @@ -446,15 +442,14 @@ static void guc_wq_item_append(struct i915_guc_client *client,
>         /* Now fill in the 4-word work queue item */
>         wqi->header = WQ_TYPE_INORDER |
>                       (wqi_len << WQ_LEN_SHIFT) |
> -                     (engine->guc_id << WQ_TARGET_SHIFT) |
> +                     (target_engine << WQ_TARGET_SHIFT) |
>                       WQ_NO_WCFLUSH_WAIT;
> -
> -       wqi->context_desc = lower_32_bits(intel_lr_context_descriptor(ctx, engine));
> -
> +       wqi->context_desc = context_desc;
>         wqi->submit_element_info = ring_tail << WQ_RING_TAIL_SHIFT;
> -       wqi->fence_id = rq->global_seqno;
> +       GEM_BUG_ON(ring_tail > WQ_RING_TAIL_MAX);
> +       wqi->fence_id = fence_id;

Hmm. Are causing a problem for ourselves with the reuse of fence_ids?
Afaik, fence_id is currently unused, but I did think at some point we
will pass around guc fences.

Probably doesn't matter until all the missing pieces of the jigsaw are
put together, by which point fence_id will probably no longer be
global_seqno.

Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris