[Intel-gfx] [07/11] drm/i915/guc: Improve the GuC documentation & comments about proxy submissions

[Daniele Ceraolo Spurio]

On 10/03/17 08:28, Oscar Mateo wrote:
> While at it, fix a typo (s/ring_lcra/ring_lrca) and improve the naming of one
> firware interface field (s/ring_tail/submit_element_info, since it can contain
> more than just the ring tail).
>
> No change in functionality.
>
> Signed-off-by: Oscar Mateo <oscar.mateo at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 42 +++++++++++++++++++-----------
>  drivers/gpu/drm/i915/intel_guc_fwif.h      |  4 +--
>  2 files changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 996e9f3..4e82be6 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -28,16 +28,25 @@
>  /**
>   * DOC: GuC-based command submission
>   *
> - * i915_guc_client:
> - * We use the term client to avoid confusion with contexts. A i915_guc_client is
> - * equivalent to GuC object guc_context_desc. This context descriptor is
> - * allocated from a pool of 1024 entries. Kernel driver will allocate doorbell
> - * and workqueue for it. Also the process descriptor (guc_process_desc), which
> - * is mapped to client space. So the client can write Work Item then ring the
> - * doorbell.
> + * GuC client:
> + * A i915_guc_client refers to a unique user of the GuC. Currently, there is

After re-reading this I think that matching a client with a "unique 
user" is slightly misleading because i915 can have multiple 
i915_guc_client objects but it still counts as a single user. Maybe we 
could define the client as a "submission path" through GuC (a set of 
work-queue, doorbell, priority) or something similar.

It might be also worth keeping the statement about the term "client" 
being introduced by us to reduce confusion.

Thanks,
Daniele

> + * only one of these (the execbuf_client) and this one is charged with all
> + * submissions to the GuC. This struct is the owner of a doorbell, a process
> + * descriptor and a workqueue (all of them inside a single gem object that
> + * contains all required pages for these elements).
>   *
> - * To simplify the implementation, we allocate one gem object that contains all
> - * pages for doorbell, process descriptor and workqueue.
> + * GuC context descriptor:
> + * During initialization, the driver allocates a static pool of 1024 such
> + * descriptors, and shares them with the GuC.
> + * Currently, there exists a 1:1 mapping between a i915_guc_client and a
> + * guc_context_desc (via the client's context_index), so effectively only
> + * one guc_context_desc gets used. This context descriptor lets the GuC know
> + * about the doorbell, workqueue and process descriptor. Theoretically, it also
> + * lets the GuC know about our HW contexts (Context ID, etc...), but we actually
> + * employ a kind of submission where the GuC uses the LRCA sent via the work
> + * item instead (the single guc_context_desc associated to execbuf client
> + * contains information about the default kernel context only, but this is
> + * essentially unused). This is called a "proxy" submission.
>   *
>   * The Scratch registers:
>   * There are 16 MMIO-based registers start from 0xC180. The kernel driver writes
> @@ -308,10 +317,17 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
>  		if (!ce->state)
>  			break;	/* XXX: continue? */
>
> +		/*
> +		 * XXX: When this is a GUC_CTX_DESC_ATTR_KERNEL client (proxy
> +		 * 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).
> +		 */
>  		lrc->context_desc = lower_32_bits(ce->lrc_desc);
>
>  		/* The state page is after PPHWSP */
> -		lrc->ring_lcra =
> +		lrc->ring_lrca =
>  			guc_ggtt_offset(ce->state) + LRC_STATE_PN * PAGE_SIZE;
>  		lrc->context_id = (client->ctx_index << GUC_ELC_CTXID_OFFSET) |
>  				(guc_engine_id << GUC_ELC_ENGINE_OFFSET);
> @@ -342,10 +358,6 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
>  	desc->wq_addr = gfx_addr + client->wq_offset;
>  	desc->wq_size = client->wq_size;
>
> -	/*
> -	 * XXX: Take LRCs from an existing context if this is not an
> -	 * IsKMDCreatedContext client
> -	 */
>  	desc->desc_private = (uintptr_t)client;
>  }
>
> @@ -469,7 +481,7 @@ static void guc_wq_item_append(struct i915_guc_client *client,
>  	/* The GuC wants only the low-order word of the context descriptor */
>  	wqi->context_desc = (u32)intel_lr_context_descriptor(rq->ctx, engine);
>
> -	wqi->ring_tail = tail << WQ_RING_TAIL_SHIFT;
> +	wqi->submit_element_info = tail << WQ_RING_TAIL_SHIFT;
>  	wqi->fence_id = rq->global_seqno;
>  }
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
> index 3ae8cef..4edf40f 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
> @@ -251,7 +251,7 @@ struct guc_doorbell_info {
>  struct guc_wq_item {
>  	u32 header;
>  	u32 context_desc;
> -	u32 ring_tail;
> +	u32 submit_element_info;
>  	u32 fence_id;
>  } __packed;
>
> @@ -278,7 +278,7 @@ struct guc_execlist_context {
>  	u32 context_desc;
>  	u32 context_id;
>  	u32 ring_status;
> -	u32 ring_lcra;
> +	u32 ring_lrca;
>  	u32 ring_begin;
>  	u32 ring_end;
>  	u32 ring_next_free_location;
>