Re: [PATCH v3 3/3] drm/i915/guc: local optimisations and updating comments

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 19/04/16 12:45, Dave Gordon wrote:
> Tidying up guc_init_proc_desc() and adding commentary to the client
> structure after the recent change in GuC page mapping strategy.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 38 ++++++++++++++----------------
>   drivers/gpu/drm/i915/intel_guc.h           | 23 ++++++++++++++++++
>   2 files changed, 41 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 135f094..5bbb13b 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -358,12 +358,14 @@ static void guc_init_proc_desc(struct intel_guc *guc,
>   static void guc_init_ctx_desc(struct intel_guc *guc,
>   			      struct i915_guc_client *client)
>   {
> +	struct drm_i915_gem_object *client_obj = client->client_obj;
>   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>   	struct intel_engine_cs *engine;
>   	struct intel_context *ctx = client->owner;
>   	struct guc_context_desc desc;
>   	struct sg_table *sg;
>   	enum intel_engine_id id;
> +	u32 gfx_addr;
>
>   	memset(&desc, 0, sizeof(desc));
>
> @@ -392,16 +394,17 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
>   		lrc->context_desc = (u32)ctx_desc;
>
>   		/* The state page is after PPHWSP */
> -		lrc->ring_lcra = i915_gem_obj_ggtt_offset(obj) +
> -				LRC_STATE_PN * PAGE_SIZE;
> +		gfx_addr = i915_gem_obj_ggtt_offset(obj);
> +		lrc->ring_lcra = gfx_addr + LRC_STATE_PN * PAGE_SIZE;
>   		lrc->context_id = (client->ctx_index << GUC_ELC_CTXID_OFFSET) |
>   				(engine->guc_id << GUC_ELC_ENGINE_OFFSET);
>
>   		obj = ctx->engine[id].ringbuf->obj;
> +		gfx_addr = i915_gem_obj_ggtt_offset(obj);
>
> -		lrc->ring_begin = i915_gem_obj_ggtt_offset(obj);
> -		lrc->ring_end = lrc->ring_begin + obj->base.size - 1;
> -		lrc->ring_next_free_location = lrc->ring_begin;
> +		lrc->ring_begin = gfx_addr;
> +		lrc->ring_end = gfx_addr + obj->base.size - 1;
> +		lrc->ring_next_free_location = gfx_addr;
>   		lrc->ring_current_tail_pointer_value = 0;
>
>   		desc.engines_used |= (1 << engine->guc_id);
> @@ -410,22 +413,17 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
>   	WARN_ON(desc.engines_used == 0);
>
>   	/*
> -	 * The CPU address is only needed at certain points, so kmap_atomic on
> -	 * demand instead of storing it in the ctx descriptor.
> -	 * XXX: May make debug easier to have it mapped
> +	 * The doorbell, process descriptor, and workqueue are all parts
> +	 * of the client object, which the GuC will reference via the GGTT
>   	 */
> -	desc.db_trigger_cpu = 0;
> -	desc.db_trigger_uk = client->doorbell_offset +
> -		i915_gem_obj_ggtt_offset(client->client_obj);
> -	desc.db_trigger_phy = client->doorbell_offset +
> -		sg_dma_address(client->client_obj->pages->sgl);
> -
> -	desc.process_desc = client->proc_desc_offset +
> -		i915_gem_obj_ggtt_offset(client->client_obj);
> -
> -	desc.wq_addr = client->wq_offset +
> -		i915_gem_obj_ggtt_offset(client->client_obj);
> -
> +	gfx_addr = i915_gem_obj_ggtt_offset(client_obj);
> +	desc.db_trigger_phy = sg_dma_address(client_obj->pages->sgl) +
> +				client->doorbell_offset;
> +	desc.db_trigger_cpu = (uintptr_t)client->client_base +
> +				client->doorbell_offset;

This changed from zero to the address. Was it buggy before?

> +	desc.db_trigger_uk = gfx_addr + client->doorbell_offset;
> +	desc.process_desc = gfx_addr + client->proc_desc_offset;
> +	desc.wq_addr = gfx_addr + client->wq_offset;
>   	desc.wq_size = client->wq_size;
>
>   	/*
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 19ca593..c503ccd 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -29,6 +29,29 @@
>
>   struct drm_i915_gem_request;
>
> +/*
> + * This structure primarily describes the GEM object shared with the GuC.
> + * The GEM object is held for the entire lifetime of our interaction with
> + * the GuC, being allocated before the GuC is loaded with its firmware.
> + * Because there's no way to update the address used by the GuC after
> + * initialisation, the shared object must stay pinned into the GGTT as
> + * long as the GuC is in use. We also keep the first page (only) mapped
> + * into kernel address space, as it includes shared data that must be
> + * updated on every request submission.
> + *
> + * The single GEM object described here is actually made up of several
> + * separate areas, as far as the GuC is concerned. The first page (kept
> + * kmap'd) includes the "process decriptor" which holds sequence data for

Typo - descriptor. :) (spotted by Thunderbird)

> + * the doorbell, and one cacheline which actually *is* the doorbell; a
> + * write to this will "ring" the doorbell (i.e. send an interrupt to the
> + * GuC). The subsequent  pages of the client object constitute the work
> + * queue (a circular array of work items), again described in the process
> + * descriptor. Work queue pages are mapped momentarily as required.
> + *
> + * Finally, we also keep a few statistics here, including the number of
> + * submissions to each engine, and a record of the last submission failure
> + * (if any).
> + */
>   struct i915_guc_client {
>   	struct drm_i915_gem_object *client_obj;
>   	void *client_base;		/* first page (only) of above	*/
>

Looks fine, only question mark over desc.db_trigger_cpu is preventing r-b.

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx