All of lore.kernel.org
 help / color / mirror / Atom feed
From: John Harrison <John.C.Harrison@Intel.com>
To: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 4/4] drm/i915/guc: kill the GuC client
Date: Thu, 14 Nov 2019 16:22:50 -0800	[thread overview]
Message-ID: <cea87371-d9b2-c41e-6cee-8468143b2d34@Intel.com> (raw)
In-Reply-To: <20191106222550.20752-5-daniele.ceraolospurio@intel.com>

On 11/6/2019 14:25, Daniele Ceraolo Spurio wrote:
> We now only use 1 client without any plan to add more. The client is
> also only holding information about the WQ and the process desc, so we
> can just move those in the intel_guc structure and always use stage_id
> 0.
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: John Harrison <John.C.Harrison@Intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_reset.c         |   6 +-
>   drivers/gpu/drm/i915/gt/uc/intel_guc.h        |   8 +-
>   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h   |   1 -
>   .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 272 +++++-------------
>   .../gpu/drm/i915/gt/uc/intel_guc_submission.h |  45 +--
>   drivers/gpu/drm/i915/i915_debugfs.c           |  11 -
>   6 files changed, 87 insertions(+), 256 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
> index f03e000051c1..d2d88d0bc9d7 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> @@ -21,6 +21,7 @@
>   #include "intel_reset.h"
>   
>   #include "uc/intel_guc.h"
> +#include "uc/intel_guc_submission.h"
>   
>   #define RESET_MAX_RETRIES 3
>   
> @@ -1070,6 +1071,7 @@ static inline int intel_gt_reset_engine(struct intel_engine_cs *engine)
>   int intel_engine_reset(struct intel_engine_cs *engine, const char *msg)
>   {
>   	struct intel_gt *gt = engine->gt;
> +	bool uses_guc = intel_engine_in_guc_submission_mode(engine);
>   	int ret;
>   
>   	GEM_TRACE("%s flags=%lx\n", engine->name, gt->reset.flags);
> @@ -1085,14 +1087,14 @@ int intel_engine_reset(struct intel_engine_cs *engine, const char *msg)
>   			   "Resetting %s for %s\n", engine->name, msg);
>   	atomic_inc(&engine->i915->gpu_error.reset_engine_count[engine->uabi_class]);
>   
> -	if (!engine->gt->uc.guc.execbuf_client)
> +	if (!uses_guc)
>   		ret = intel_gt_reset_engine(engine);
>   	else
>   		ret = intel_guc_reset_engine(&engine->gt->uc.guc, engine);
>   	if (ret) {
>   		/* If we fail here, we expect to fallback to a global reset */
>   		DRM_DEBUG_DRIVER("%sFailed to reset %s, ret=%d\n",
> -				 engine->gt->uc.guc.execbuf_client ? "GuC " : "",
> +				 uses_guc ? "GuC " : "",
>   				 engine->name, ret);
>   		goto out;
>   	}
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> index b2d1766e689a..cd09c912e361 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> @@ -46,9 +46,13 @@ struct intel_guc {
>   
>   	struct i915_vma *stage_desc_pool;
>   	void *stage_desc_pool_vaddr;
> -	struct ida stage_ids;
>   
> -	struct intel_guc_client *execbuf_client;
> +	struct i915_vma *workqueue;
> +	void *workqueue_vaddr;
> +	spinlock_t wq_lock;
> +
> +	struct i915_vma *proc_desc;
> +	void *proc_desc_vaddr;
>   
>   	/* Control params for fw initialization */
>   	u32 params[GUC_CTL_MAX_DWORDS];
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> index 1e8e4af7d9ca..a6b733c146c9 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> @@ -31,7 +31,6 @@
>   
>   #define GUC_DOORBELL_INVALID		256
>   
> -#define GUC_PD_SIZE			(PAGE_SIZE)
>   #define GUC_WQ_SIZE			(PAGE_SIZE * 2)
>   
>   /* Work queue item header definitions */
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index 0088c3417641..71788589f9fe 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -27,24 +27,13 @@
>    * code) matches the old submission model and will be updated as part of the
>    * upgrade to the new flow.
>    *
> - * GuC client:
> - * A intel_guc_client refers to a submission path through GuC. Currently, there
> - * is only one client, which is charged with all submissions to the GuC. This
> - * struct is the owner of a process descriptor and a workqueue (both of them
> - * inside a single gem object that contains all required pages for these
> - * elements).
> - *
>    * GuC stage 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 intel_guc_client and a
> - * guc_stage_desc (via the client's stage_id), so effectively only one
> - * gets used. This stage descriptor lets the GuC know about the workqueue and
> + * descriptors, and shares them with the GuC. Currently, we only use one
> + * descriptor. This stage descriptor lets the GuC know about the 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_stage_desc associated to execbuf client contains information about the
> - * default kernel context only, but this is essentially unused). This is called
> + * where the GuC uses the LRCA sent via the work item instead. This is called
>    * a "proxy" submission.
>    *
>    * The Scratch registers:
> @@ -71,33 +60,45 @@ static inline struct i915_priolist *to_priolist(struct rb_node *rb)
>   	return rb_entry(rb, struct i915_priolist, node);
>   }
>   
> -static inline bool is_high_priority(struct intel_guc_client *client)
> +static struct guc_stage_desc *__get_stage_desc(struct intel_guc *guc, u32 id)
>   {
> -	return (client->priority == GUC_CLIENT_PRIORITY_KMD_HIGH ||
> -		client->priority == GUC_CLIENT_PRIORITY_HIGH);
> +	struct guc_stage_desc *base = guc->stage_desc_pool_vaddr;
> +
> +	return &base[id];
>   }
>   
> -static struct guc_stage_desc *__get_stage_desc(struct intel_guc_client *client)
> +static int guc_workqueue_create(struct intel_guc *guc)
>   {
> -	struct guc_stage_desc *base = client->guc->stage_desc_pool_vaddr;
> -
> -	return &base[client->stage_id];
> +	return intel_guc_allocate_and_map_vma(guc, GUC_WQ_SIZE, &guc->workqueue,
> +					      &guc->workqueue_vaddr);
>   }
>   
> -static inline struct guc_process_desc *
> -__get_process_desc(struct intel_guc_client *client)
> +static void guc_workqueue_destroy(struct intel_guc *guc)
>   {
> -	return client->vaddr;
> +	i915_vma_unpin_and_release(&guc->workqueue, I915_VMA_RELEASE_MAP);
>   }
>   
>   /*
>    * Initialise the process descriptor shared with the GuC firmware.
>    */
> -static void guc_proc_desc_init(struct intel_guc_client *client)
> +static int guc_proc_desc_create(struct intel_guc *guc)
> +{
> +	const u32 size = PAGE_ALIGN(sizeof(struct guc_process_desc));
> +
> +	return intel_guc_allocate_and_map_vma(guc, size, &guc->proc_desc,
> +					      &guc->proc_desc_vaddr);
> +}
> +
> +static void guc_proc_desc_destroy(struct intel_guc *guc)
> +{
> +	i915_vma_unpin_and_release(&guc->proc_desc, I915_VMA_RELEASE_MAP);
> +}
> +
> +static void guc_proc_desc_init(struct intel_guc *guc)
>   {
>   	struct guc_process_desc *desc;
>   
> -	desc = memset(__get_process_desc(client), 0, sizeof(*desc));
> +	desc = memset(guc->proc_desc_vaddr, 0, sizeof(*desc));
>   
>   	/*
>   	 * XXX: pDoorbell and WQVBaseAddress are pointers in process address
> @@ -108,39 +109,27 @@ static void guc_proc_desc_init(struct intel_guc_client *client)
>   	desc->wq_base_addr = 0;
>   	desc->db_base_addr = 0;
>   
> -	desc->stage_id = client->stage_id;
>   	desc->wq_size_bytes = GUC_WQ_SIZE;
>   	desc->wq_status = WQ_STATUS_ACTIVE;
> -	desc->priority = client->priority;
> +	desc->priority = GUC_CLIENT_PRIORITY_KMD_NORMAL;
>   }
>   
> -static void guc_proc_desc_fini(struct intel_guc_client *client)
> +static void guc_proc_desc_fini(struct intel_guc *guc)
>   {
> -	struct guc_process_desc *desc;
> -
> -	desc = __get_process_desc(client);
> -	memset(desc, 0, sizeof(*desc));
> +	memset(guc->proc_desc_vaddr, 0, sizeof(struct guc_process_desc));
>   }
>   
>   static int guc_stage_desc_pool_create(struct intel_guc *guc)
>   {
>   	u32 size = PAGE_ALIGN(sizeof(struct guc_stage_desc) *
>   			      GUC_MAX_STAGE_DESCRIPTORS);
> -	int ret;
> -
> -	ret = intel_guc_allocate_and_map_vma(guc, size, &guc->stage_desc_pool,
> -					     &guc->stage_desc_pool_vaddr);
> -	if (ret)
> -		return ret;
> -
> -	ida_init(&guc->stage_ids);
>   
> -	return 0;
> +	return intel_guc_allocate_and_map_vma(guc, size, &guc->stage_desc_pool,
> +					      &guc->stage_desc_pool_vaddr);
>   }
>   
>   static void guc_stage_desc_pool_destroy(struct intel_guc *guc)
>   {
> -	ida_destroy(&guc->stage_ids);
>   	i915_vma_unpin_and_release(&guc->stage_desc_pool, I915_VMA_RELEASE_MAP);
>   }
>   
> @@ -148,58 +137,49 @@ static void guc_stage_desc_pool_destroy(struct intel_guc *guc)
>    * Initialise/clear the stage descriptor shared with the GuC firmware.
>    *
>    * This descriptor tells the GuC where (in GGTT space) to find the important
> - * data structures relating to this client (process descriptor, write queue,
> + * data structures related to work submission (process descriptor, write queue,
>    * etc).
>    */
> -static void guc_stage_desc_init(struct intel_guc_client *client)
> +static void guc_stage_desc_init(struct intel_guc *guc)
>   {
> -	struct intel_guc *guc = client->guc;
>   	struct guc_stage_desc *desc;
> -	u32 gfx_addr;
>   
> -	desc = __get_stage_desc(client);
> +	/* we only use 1 stage desc, so hardcode it to 0 */
> +	desc = __get_stage_desc(guc, 0);
>   	memset(desc, 0, sizeof(*desc));
>   
>   	desc->attribute = GUC_STAGE_DESC_ATTR_ACTIVE |
>   			  GUC_STAGE_DESC_ATTR_KERNEL;
> -	if (is_high_priority(client))
> -		desc->attribute |= GUC_STAGE_DESC_ATTR_PREEMPT;
> -	desc->stage_id = client->stage_id;
> -	desc->priority = client->priority;
>   
> -	/*
> -	 * The process descriptor and workqueue are all parts of the client
> -	 * object, which the GuC will reference via the GGTT
> -	 */
> -	gfx_addr = intel_guc_ggtt_offset(guc, client->vma);
> -	desc->process_desc = gfx_addr;
> -	desc->wq_addr = gfx_addr + GUC_PD_SIZE;
> -	desc->wq_size = GUC_WQ_SIZE;
> +	desc->stage_id = 0;
> +	desc->priority = GUC_CLIENT_PRIORITY_KMD_NORMAL;
>   
> -	desc->desc_private = ptr_to_u64(client);
> +	desc->process_desc = intel_guc_ggtt_offset(guc, guc->proc_desc);
> +	desc->wq_addr = intel_guc_ggtt_offset(guc, guc->workqueue);
> +	desc->wq_size = GUC_WQ_SIZE;
>   }
>   
> -static void guc_stage_desc_fini(struct intel_guc_client *client)
> +static void guc_stage_desc_fini(struct intel_guc *guc)
>   {
>   	struct guc_stage_desc *desc;
>   
> -	desc = __get_stage_desc(client);
> +	desc = __get_stage_desc(guc, 0);
>   	memset(desc, 0, sizeof(*desc));
>   }
>   
>   /* Construct a Work Item and append it to the GuC's Work Queue */
> -static void guc_wq_item_append(struct intel_guc_client *client,
> +static void guc_wq_item_append(struct intel_guc *guc,
>   			       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 guc_process_desc *desc = __get_process_desc(client);
> +	struct guc_process_desc *desc = guc->proc_desc_vaddr;
>   	struct guc_wq_item *wqi;
>   	u32 wq_off;
>   
> -	lockdep_assert_held(&client->wq_lock);
> +	lockdep_assert_held(&guc->wq_lock);
>   
>   	/* 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
> @@ -220,7 +200,7 @@ static void guc_wq_item_append(struct intel_guc_client *client,
>   	GEM_BUG_ON(wq_off & (wqi_size - 1));
>   
>   	/* WQ starts from the page after process_desc */
This comment is now invalid. With that fixed:
Reviewed-by: John Harrison <John.C.Harrison@Intel.com>

> -	wqi = client->vaddr + wq_off + GUC_PD_SIZE;
> +	wqi = guc->workqueue_vaddr + wq_off;
>   
>   	/* Now fill in the 4-word work queue item */
>   	wqi->header = WQ_TYPE_INORDER |
> @@ -238,12 +218,11 @@ static void guc_wq_item_append(struct intel_guc_client *client,
>   
>   static void guc_add_request(struct intel_guc *guc, struct i915_request *rq)
>   {
> -	struct intel_guc_client *client = guc->execbuf_client;
>   	struct intel_engine_cs *engine = rq->engine;
>   	u32 ctx_desc = lower_32_bits(rq->hw_context->lrc_desc);
>   	u32 ring_tail = intel_ring_set_tail(rq->ring, rq->tail) / sizeof(u64);
>   
> -	guc_wq_item_append(client, engine->guc_id, ctx_desc,
> +	guc_wq_item_append(guc, engine->guc_id, ctx_desc,
>   			   ring_tail, rq->fence.seqno);
>   }
>   
> @@ -267,9 +246,8 @@ static void guc_submit(struct intel_engine_cs *engine,
>   		       struct i915_request **end)
>   {
>   	struct intel_guc *guc = &engine->gt->uc.guc;
> -	struct intel_guc_client *client = guc->execbuf_client;
>   
> -	spin_lock(&client->wq_lock);
> +	spin_lock(&guc->wq_lock);
>   
>   	do {
>   		struct i915_request *rq = *out++;
> @@ -278,7 +256,7 @@ static void guc_submit(struct intel_engine_cs *engine,
>   		guc_add_request(guc, rq);
>   	} while (out != end);
>   
> -	spin_unlock(&client->wq_lock);
> +	spin_unlock(&guc->wq_lock);
>   }
>   
>   static inline int rq_prio(const struct i915_request *rq)
> @@ -529,125 +507,6 @@ static void guc_reset_finish(struct intel_engine_cs *engine)
>    * path of guc_submit() above.
>    */
>   
> -/**
> - * guc_client_alloc() - Allocate an intel_guc_client
> - * @guc:	the intel_guc structure
> - * @priority:	four levels priority _CRITICAL, _HIGH, _NORMAL and _LOW
> - *		The kernel client to replace ExecList submission is created with
> - *		NORMAL priority. Priority of a client for scheduler can be HIGH,
> - *		while a preemption context can use CRITICAL.
> - *
> - * Return:	An intel_guc_client object if success, else NULL.
> - */
> -static struct intel_guc_client *
> -guc_client_alloc(struct intel_guc *guc, u32 priority)
> -{
> -	struct intel_guc_client *client;
> -	struct i915_vma *vma;
> -	void *vaddr;
> -	int ret;
> -
> -	client = kzalloc(sizeof(*client), GFP_KERNEL);
> -	if (!client)
> -		return ERR_PTR(-ENOMEM);
> -
> -	client->guc = guc;
> -	client->priority = priority;
> -	spin_lock_init(&client->wq_lock);
> -
> -	ret = ida_simple_get(&guc->stage_ids, 0, GUC_MAX_STAGE_DESCRIPTORS,
> -			     GFP_KERNEL);
> -	if (ret < 0)
> -		goto err_client;
> -
> -	client->stage_id = ret;
> -
> -	/* The first page is doorbell/proc_desc. Two followed pages are wq. */
> -	vma = intel_guc_allocate_vma(guc, GUC_PD_SIZE + GUC_WQ_SIZE);
> -	if (IS_ERR(vma)) {
> -		ret = PTR_ERR(vma);
> -		goto err_id;
> -	}
> -
> -	/* We'll keep just the first (doorbell/proc) page permanently kmap'd. */
> -	client->vma = vma;
> -
> -	vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
> -	if (IS_ERR(vaddr)) {
> -		ret = PTR_ERR(vaddr);
> -		goto err_vma;
> -	}
> -	client->vaddr = vaddr;
> -
> -	DRM_DEBUG_DRIVER("new priority %u client %p: stage_id %u\n",
> -			 priority, client, client->stage_id);
> -
> -	return client;
> -
> -err_vma:
> -	i915_vma_unpin_and_release(&client->vma, 0);
> -err_id:
> -	ida_simple_remove(&guc->stage_ids, client->stage_id);
> -err_client:
> -	kfree(client);
> -	return ERR_PTR(ret);
> -}
> -
> -static void guc_client_free(struct intel_guc_client *client)
> -{
> -	i915_vma_unpin_and_release(&client->vma, I915_VMA_RELEASE_MAP);
> -	ida_simple_remove(&client->guc->stage_ids, client->stage_id);
> -	kfree(client);
> -}
> -
> -static int guc_clients_create(struct intel_guc *guc)
> -{
> -	struct intel_guc_client *client;
> -
> -	GEM_BUG_ON(guc->execbuf_client);
> -
> -	client = guc_client_alloc(guc, GUC_CLIENT_PRIORITY_KMD_NORMAL);
> -	if (IS_ERR(client)) {
> -		DRM_ERROR("Failed to create GuC client for submission!\n");
> -		return PTR_ERR(client);
> -	}
> -	guc->execbuf_client = client;
> -
> -	return 0;
> -}
> -
> -static void guc_clients_destroy(struct intel_guc *guc)
> -{
> -	struct intel_guc_client *client;
> -
> -	client = fetch_and_zero(&guc->execbuf_client);
> -	if (client)
> -		guc_client_free(client);
> -}
> -
> -static void __guc_client_enable(struct intel_guc_client *client)
> -{
> -	guc_proc_desc_init(client);
> -	guc_stage_desc_init(client);
> -}
> -
> -static void __guc_client_disable(struct intel_guc_client *client)
> -{
> -	guc_stage_desc_fini(client);
> -	guc_proc_desc_fini(client);
> -}
> -
> -static void guc_clients_enable(struct intel_guc *guc)
> -{
> -	__guc_client_enable(guc->execbuf_client);
> -}
> -
> -static void guc_clients_disable(struct intel_guc *guc)
> -{
> -	if (guc->execbuf_client)
> -		__guc_client_disable(guc->execbuf_client);
> -}
> -
>   /*
>    * Set up the memory resources to be shared with the GuC (via the GGTT)
>    * at firmware loading time.
> @@ -668,12 +527,20 @@ int intel_guc_submission_init(struct intel_guc *guc)
>   	 */
>   	GEM_BUG_ON(!guc->stage_desc_pool);
>   
> -	ret = guc_clients_create(guc);
> +	ret = guc_workqueue_create(guc);
>   	if (ret)
>   		goto err_pool;
>   
> +	ret = guc_proc_desc_create(guc);
> +	if (ret)
> +		goto err_workqueue;
> +
> +	spin_lock_init(&guc->wq_lock);
> +
>   	return 0;
>   
> +err_workqueue:
> +	guc_workqueue_destroy(guc);
>   err_pool:
>   	guc_stage_desc_pool_destroy(guc);
>   	return ret;
> @@ -681,10 +548,11 @@ int intel_guc_submission_init(struct intel_guc *guc)
>   
>   void intel_guc_submission_fini(struct intel_guc *guc)
>   {
> -	guc_clients_destroy(guc);
> -
> -	if (guc->stage_desc_pool)
> +	if (guc->stage_desc_pool) {
> +		guc_proc_desc_destroy(guc);
> +		guc_workqueue_destroy(guc);
>   		guc_stage_desc_pool_destroy(guc);
> +	}
>   }
>   
>   static void guc_interrupts_capture(struct intel_gt *gt)
> @@ -816,9 +684,8 @@ void intel_guc_submission_enable(struct intel_guc *guc)
>   		     sizeof(struct guc_wq_item) *
>   		     I915_NUM_ENGINES > GUC_WQ_SIZE);
>   
> -	GEM_BUG_ON(!guc->execbuf_client);
> -
> -	guc_clients_enable(guc);
> +	guc_proc_desc_init(guc);
> +	guc_stage_desc_init(guc);
>   
>   	/* Take over from manual control of ELSP (execlists) */
>   	guc_interrupts_capture(gt);
> @@ -836,7 +703,9 @@ void intel_guc_submission_disable(struct intel_guc *guc)
>   	GEM_BUG_ON(gt->awake); /* GT should be parked first */
>   
>   	guc_interrupts_release(gt);
> -	guc_clients_disable(guc);
> +
> +	guc_stage_desc_fini(guc);
> +	guc_proc_desc_fini(guc);
>   }
>   
>   static bool __guc_submission_support(struct intel_guc *guc)
> @@ -854,3 +723,8 @@ void intel_guc_submission_init_early(struct intel_guc *guc)
>   {
>   	guc->submission_supported = __guc_submission_support(guc);
>   }
> +
> +bool intel_engine_in_guc_submission_mode(const struct intel_engine_cs *engine)
> +{
> +	return engine->set_default_submission == guc_set_default_submission;
> +}
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
> index e2deb4e6f42a..e402a2932592 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
> @@ -6,48 +6,10 @@
>   #ifndef _INTEL_GUC_SUBMISSION_H_
>   #define _INTEL_GUC_SUBMISSION_H_
>   
> -#include <linux/spinlock.h>
> +#include <linux/types.h>
>   
> -#include "gt/intel_engine_types.h"
> -
> -#include "i915_gem.h"
> -#include "i915_selftest.h"
> -
> -struct drm_i915_private;
> -
> -/*
> - * This structure primarily describes the GEM object shared with the GuC.
> - * The specs sometimes refer to this object as a "GuC context", but we use
> - * the term "client" to avoid confusion with hardware contexts. This
> - * 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 descriptor" which holds sequence data for
> - * 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.
> - */
> -struct intel_guc_client {
> -	struct i915_vma *vma;
> -	void *vaddr;
> -	struct intel_guc *guc;
> -
> -	/* bitmap of (host) engine ids */
> -	u32 priority;
> -	u32 stage_id;
> -
> -	/* Protects GuC client's WQ access */
> -	spinlock_t wq_lock;
> -};
> +struct intel_guc;
> +struct intel_engine_cs;
>   
>   void intel_guc_submission_init_early(struct intel_guc *guc);
>   int intel_guc_submission_init(struct intel_guc *guc);
> @@ -56,5 +18,6 @@ void intel_guc_submission_disable(struct intel_guc *guc);
>   void intel_guc_submission_fini(struct intel_guc *guc);
>   int intel_guc_preempt_work_create(struct intel_guc *guc);
>   void intel_guc_preempt_work_destroy(struct intel_guc *guc);
> +bool intel_engine_in_guc_submission_mode(const struct intel_engine_cs *engine);
>   
>   #endif
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 5d5974e7f3ed..f32e7b016197 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1802,23 +1802,12 @@ static void i915_guc_log_info(struct seq_file *m,
>   static int i915_guc_info(struct seq_file *m, void *data)
>   {
>   	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> -	const struct intel_guc *guc = &dev_priv->gt.uc.guc;
> -	struct intel_guc_client *client = guc->execbuf_client;
>   
>   	if (!USES_GUC(dev_priv))
>   		return -ENODEV;
>   
>   	i915_guc_log_info(m, dev_priv);
>   
> -	if (!USES_GUC_SUBMISSION(dev_priv))
> -		return 0;
> -
> -	GEM_BUG_ON(!guc->execbuf_client);
> -
> -	seq_printf(m, "\nGuC execbuf client @ %p:\n", client);
> -	seq_printf(m, "\tPriority %d, GuC stage index: %u\n",
> -		   client->priority,
> -		   client->stage_id);
>   	/* Add more as required ... */
>   
>   	return 0;

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

WARNING: multiple messages have this Message-ID (diff)
From: John Harrison <John.C.Harrison@Intel.com>
To: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 4/4] drm/i915/guc: kill the GuC client
Date: Thu, 14 Nov 2019 16:22:50 -0800	[thread overview]
Message-ID: <cea87371-d9b2-c41e-6cee-8468143b2d34@Intel.com> (raw)
Message-ID: <20191115002250.Tf4E3NSBIg-WpeQZWSF3t5mXIRNt7rQMksKsePWQgWM@z> (raw)
In-Reply-To: <20191106222550.20752-5-daniele.ceraolospurio@intel.com>

On 11/6/2019 14:25, Daniele Ceraolo Spurio wrote:
> We now only use 1 client without any plan to add more. The client is
> also only holding information about the WQ and the process desc, so we
> can just move those in the intel_guc structure and always use stage_id
> 0.
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: John Harrison <John.C.Harrison@Intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_reset.c         |   6 +-
>   drivers/gpu/drm/i915/gt/uc/intel_guc.h        |   8 +-
>   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h   |   1 -
>   .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 272 +++++-------------
>   .../gpu/drm/i915/gt/uc/intel_guc_submission.h |  45 +--
>   drivers/gpu/drm/i915/i915_debugfs.c           |  11 -
>   6 files changed, 87 insertions(+), 256 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
> index f03e000051c1..d2d88d0bc9d7 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> @@ -21,6 +21,7 @@
>   #include "intel_reset.h"
>   
>   #include "uc/intel_guc.h"
> +#include "uc/intel_guc_submission.h"
>   
>   #define RESET_MAX_RETRIES 3
>   
> @@ -1070,6 +1071,7 @@ static inline int intel_gt_reset_engine(struct intel_engine_cs *engine)
>   int intel_engine_reset(struct intel_engine_cs *engine, const char *msg)
>   {
>   	struct intel_gt *gt = engine->gt;
> +	bool uses_guc = intel_engine_in_guc_submission_mode(engine);
>   	int ret;
>   
>   	GEM_TRACE("%s flags=%lx\n", engine->name, gt->reset.flags);
> @@ -1085,14 +1087,14 @@ int intel_engine_reset(struct intel_engine_cs *engine, const char *msg)
>   			   "Resetting %s for %s\n", engine->name, msg);
>   	atomic_inc(&engine->i915->gpu_error.reset_engine_count[engine->uabi_class]);
>   
> -	if (!engine->gt->uc.guc.execbuf_client)
> +	if (!uses_guc)
>   		ret = intel_gt_reset_engine(engine);
>   	else
>   		ret = intel_guc_reset_engine(&engine->gt->uc.guc, engine);
>   	if (ret) {
>   		/* If we fail here, we expect to fallback to a global reset */
>   		DRM_DEBUG_DRIVER("%sFailed to reset %s, ret=%d\n",
> -				 engine->gt->uc.guc.execbuf_client ? "GuC " : "",
> +				 uses_guc ? "GuC " : "",
>   				 engine->name, ret);
>   		goto out;
>   	}
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> index b2d1766e689a..cd09c912e361 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> @@ -46,9 +46,13 @@ struct intel_guc {
>   
>   	struct i915_vma *stage_desc_pool;
>   	void *stage_desc_pool_vaddr;
> -	struct ida stage_ids;
>   
> -	struct intel_guc_client *execbuf_client;
> +	struct i915_vma *workqueue;
> +	void *workqueue_vaddr;
> +	spinlock_t wq_lock;
> +
> +	struct i915_vma *proc_desc;
> +	void *proc_desc_vaddr;
>   
>   	/* Control params for fw initialization */
>   	u32 params[GUC_CTL_MAX_DWORDS];
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> index 1e8e4af7d9ca..a6b733c146c9 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> @@ -31,7 +31,6 @@
>   
>   #define GUC_DOORBELL_INVALID		256
>   
> -#define GUC_PD_SIZE			(PAGE_SIZE)
>   #define GUC_WQ_SIZE			(PAGE_SIZE * 2)
>   
>   /* Work queue item header definitions */
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index 0088c3417641..71788589f9fe 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -27,24 +27,13 @@
>    * code) matches the old submission model and will be updated as part of the
>    * upgrade to the new flow.
>    *
> - * GuC client:
> - * A intel_guc_client refers to a submission path through GuC. Currently, there
> - * is only one client, which is charged with all submissions to the GuC. This
> - * struct is the owner of a process descriptor and a workqueue (both of them
> - * inside a single gem object that contains all required pages for these
> - * elements).
> - *
>    * GuC stage 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 intel_guc_client and a
> - * guc_stage_desc (via the client's stage_id), so effectively only one
> - * gets used. This stage descriptor lets the GuC know about the workqueue and
> + * descriptors, and shares them with the GuC. Currently, we only use one
> + * descriptor. This stage descriptor lets the GuC know about the 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_stage_desc associated to execbuf client contains information about the
> - * default kernel context only, but this is essentially unused). This is called
> + * where the GuC uses the LRCA sent via the work item instead. This is called
>    * a "proxy" submission.
>    *
>    * The Scratch registers:
> @@ -71,33 +60,45 @@ static inline struct i915_priolist *to_priolist(struct rb_node *rb)
>   	return rb_entry(rb, struct i915_priolist, node);
>   }
>   
> -static inline bool is_high_priority(struct intel_guc_client *client)
> +static struct guc_stage_desc *__get_stage_desc(struct intel_guc *guc, u32 id)
>   {
> -	return (client->priority == GUC_CLIENT_PRIORITY_KMD_HIGH ||
> -		client->priority == GUC_CLIENT_PRIORITY_HIGH);
> +	struct guc_stage_desc *base = guc->stage_desc_pool_vaddr;
> +
> +	return &base[id];
>   }
>   
> -static struct guc_stage_desc *__get_stage_desc(struct intel_guc_client *client)
> +static int guc_workqueue_create(struct intel_guc *guc)
>   {
> -	struct guc_stage_desc *base = client->guc->stage_desc_pool_vaddr;
> -
> -	return &base[client->stage_id];
> +	return intel_guc_allocate_and_map_vma(guc, GUC_WQ_SIZE, &guc->workqueue,
> +					      &guc->workqueue_vaddr);
>   }
>   
> -static inline struct guc_process_desc *
> -__get_process_desc(struct intel_guc_client *client)
> +static void guc_workqueue_destroy(struct intel_guc *guc)
>   {
> -	return client->vaddr;
> +	i915_vma_unpin_and_release(&guc->workqueue, I915_VMA_RELEASE_MAP);
>   }
>   
>   /*
>    * Initialise the process descriptor shared with the GuC firmware.
>    */
> -static void guc_proc_desc_init(struct intel_guc_client *client)
> +static int guc_proc_desc_create(struct intel_guc *guc)
> +{
> +	const u32 size = PAGE_ALIGN(sizeof(struct guc_process_desc));
> +
> +	return intel_guc_allocate_and_map_vma(guc, size, &guc->proc_desc,
> +					      &guc->proc_desc_vaddr);
> +}
> +
> +static void guc_proc_desc_destroy(struct intel_guc *guc)
> +{
> +	i915_vma_unpin_and_release(&guc->proc_desc, I915_VMA_RELEASE_MAP);
> +}
> +
> +static void guc_proc_desc_init(struct intel_guc *guc)
>   {
>   	struct guc_process_desc *desc;
>   
> -	desc = memset(__get_process_desc(client), 0, sizeof(*desc));
> +	desc = memset(guc->proc_desc_vaddr, 0, sizeof(*desc));
>   
>   	/*
>   	 * XXX: pDoorbell and WQVBaseAddress are pointers in process address
> @@ -108,39 +109,27 @@ static void guc_proc_desc_init(struct intel_guc_client *client)
>   	desc->wq_base_addr = 0;
>   	desc->db_base_addr = 0;
>   
> -	desc->stage_id = client->stage_id;
>   	desc->wq_size_bytes = GUC_WQ_SIZE;
>   	desc->wq_status = WQ_STATUS_ACTIVE;
> -	desc->priority = client->priority;
> +	desc->priority = GUC_CLIENT_PRIORITY_KMD_NORMAL;
>   }
>   
> -static void guc_proc_desc_fini(struct intel_guc_client *client)
> +static void guc_proc_desc_fini(struct intel_guc *guc)
>   {
> -	struct guc_process_desc *desc;
> -
> -	desc = __get_process_desc(client);
> -	memset(desc, 0, sizeof(*desc));
> +	memset(guc->proc_desc_vaddr, 0, sizeof(struct guc_process_desc));
>   }
>   
>   static int guc_stage_desc_pool_create(struct intel_guc *guc)
>   {
>   	u32 size = PAGE_ALIGN(sizeof(struct guc_stage_desc) *
>   			      GUC_MAX_STAGE_DESCRIPTORS);
> -	int ret;
> -
> -	ret = intel_guc_allocate_and_map_vma(guc, size, &guc->stage_desc_pool,
> -					     &guc->stage_desc_pool_vaddr);
> -	if (ret)
> -		return ret;
> -
> -	ida_init(&guc->stage_ids);
>   
> -	return 0;
> +	return intel_guc_allocate_and_map_vma(guc, size, &guc->stage_desc_pool,
> +					      &guc->stage_desc_pool_vaddr);
>   }
>   
>   static void guc_stage_desc_pool_destroy(struct intel_guc *guc)
>   {
> -	ida_destroy(&guc->stage_ids);
>   	i915_vma_unpin_and_release(&guc->stage_desc_pool, I915_VMA_RELEASE_MAP);
>   }
>   
> @@ -148,58 +137,49 @@ static void guc_stage_desc_pool_destroy(struct intel_guc *guc)
>    * Initialise/clear the stage descriptor shared with the GuC firmware.
>    *
>    * This descriptor tells the GuC where (in GGTT space) to find the important
> - * data structures relating to this client (process descriptor, write queue,
> + * data structures related to work submission (process descriptor, write queue,
>    * etc).
>    */
> -static void guc_stage_desc_init(struct intel_guc_client *client)
> +static void guc_stage_desc_init(struct intel_guc *guc)
>   {
> -	struct intel_guc *guc = client->guc;
>   	struct guc_stage_desc *desc;
> -	u32 gfx_addr;
>   
> -	desc = __get_stage_desc(client);
> +	/* we only use 1 stage desc, so hardcode it to 0 */
> +	desc = __get_stage_desc(guc, 0);
>   	memset(desc, 0, sizeof(*desc));
>   
>   	desc->attribute = GUC_STAGE_DESC_ATTR_ACTIVE |
>   			  GUC_STAGE_DESC_ATTR_KERNEL;
> -	if (is_high_priority(client))
> -		desc->attribute |= GUC_STAGE_DESC_ATTR_PREEMPT;
> -	desc->stage_id = client->stage_id;
> -	desc->priority = client->priority;
>   
> -	/*
> -	 * The process descriptor and workqueue are all parts of the client
> -	 * object, which the GuC will reference via the GGTT
> -	 */
> -	gfx_addr = intel_guc_ggtt_offset(guc, client->vma);
> -	desc->process_desc = gfx_addr;
> -	desc->wq_addr = gfx_addr + GUC_PD_SIZE;
> -	desc->wq_size = GUC_WQ_SIZE;
> +	desc->stage_id = 0;
> +	desc->priority = GUC_CLIENT_PRIORITY_KMD_NORMAL;
>   
> -	desc->desc_private = ptr_to_u64(client);
> +	desc->process_desc = intel_guc_ggtt_offset(guc, guc->proc_desc);
> +	desc->wq_addr = intel_guc_ggtt_offset(guc, guc->workqueue);
> +	desc->wq_size = GUC_WQ_SIZE;
>   }
>   
> -static void guc_stage_desc_fini(struct intel_guc_client *client)
> +static void guc_stage_desc_fini(struct intel_guc *guc)
>   {
>   	struct guc_stage_desc *desc;
>   
> -	desc = __get_stage_desc(client);
> +	desc = __get_stage_desc(guc, 0);
>   	memset(desc, 0, sizeof(*desc));
>   }
>   
>   /* Construct a Work Item and append it to the GuC's Work Queue */
> -static void guc_wq_item_append(struct intel_guc_client *client,
> +static void guc_wq_item_append(struct intel_guc *guc,
>   			       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 guc_process_desc *desc = __get_process_desc(client);
> +	struct guc_process_desc *desc = guc->proc_desc_vaddr;
>   	struct guc_wq_item *wqi;
>   	u32 wq_off;
>   
> -	lockdep_assert_held(&client->wq_lock);
> +	lockdep_assert_held(&guc->wq_lock);
>   
>   	/* 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
> @@ -220,7 +200,7 @@ static void guc_wq_item_append(struct intel_guc_client *client,
>   	GEM_BUG_ON(wq_off & (wqi_size - 1));
>   
>   	/* WQ starts from the page after process_desc */
This comment is now invalid. With that fixed:
Reviewed-by: John Harrison <John.C.Harrison@Intel.com>

> -	wqi = client->vaddr + wq_off + GUC_PD_SIZE;
> +	wqi = guc->workqueue_vaddr + wq_off;
>   
>   	/* Now fill in the 4-word work queue item */
>   	wqi->header = WQ_TYPE_INORDER |
> @@ -238,12 +218,11 @@ static void guc_wq_item_append(struct intel_guc_client *client,
>   
>   static void guc_add_request(struct intel_guc *guc, struct i915_request *rq)
>   {
> -	struct intel_guc_client *client = guc->execbuf_client;
>   	struct intel_engine_cs *engine = rq->engine;
>   	u32 ctx_desc = lower_32_bits(rq->hw_context->lrc_desc);
>   	u32 ring_tail = intel_ring_set_tail(rq->ring, rq->tail) / sizeof(u64);
>   
> -	guc_wq_item_append(client, engine->guc_id, ctx_desc,
> +	guc_wq_item_append(guc, engine->guc_id, ctx_desc,
>   			   ring_tail, rq->fence.seqno);
>   }
>   
> @@ -267,9 +246,8 @@ static void guc_submit(struct intel_engine_cs *engine,
>   		       struct i915_request **end)
>   {
>   	struct intel_guc *guc = &engine->gt->uc.guc;
> -	struct intel_guc_client *client = guc->execbuf_client;
>   
> -	spin_lock(&client->wq_lock);
> +	spin_lock(&guc->wq_lock);
>   
>   	do {
>   		struct i915_request *rq = *out++;
> @@ -278,7 +256,7 @@ static void guc_submit(struct intel_engine_cs *engine,
>   		guc_add_request(guc, rq);
>   	} while (out != end);
>   
> -	spin_unlock(&client->wq_lock);
> +	spin_unlock(&guc->wq_lock);
>   }
>   
>   static inline int rq_prio(const struct i915_request *rq)
> @@ -529,125 +507,6 @@ static void guc_reset_finish(struct intel_engine_cs *engine)
>    * path of guc_submit() above.
>    */
>   
> -/**
> - * guc_client_alloc() - Allocate an intel_guc_client
> - * @guc:	the intel_guc structure
> - * @priority:	four levels priority _CRITICAL, _HIGH, _NORMAL and _LOW
> - *		The kernel client to replace ExecList submission is created with
> - *		NORMAL priority. Priority of a client for scheduler can be HIGH,
> - *		while a preemption context can use CRITICAL.
> - *
> - * Return:	An intel_guc_client object if success, else NULL.
> - */
> -static struct intel_guc_client *
> -guc_client_alloc(struct intel_guc *guc, u32 priority)
> -{
> -	struct intel_guc_client *client;
> -	struct i915_vma *vma;
> -	void *vaddr;
> -	int ret;
> -
> -	client = kzalloc(sizeof(*client), GFP_KERNEL);
> -	if (!client)
> -		return ERR_PTR(-ENOMEM);
> -
> -	client->guc = guc;
> -	client->priority = priority;
> -	spin_lock_init(&client->wq_lock);
> -
> -	ret = ida_simple_get(&guc->stage_ids, 0, GUC_MAX_STAGE_DESCRIPTORS,
> -			     GFP_KERNEL);
> -	if (ret < 0)
> -		goto err_client;
> -
> -	client->stage_id = ret;
> -
> -	/* The first page is doorbell/proc_desc. Two followed pages are wq. */
> -	vma = intel_guc_allocate_vma(guc, GUC_PD_SIZE + GUC_WQ_SIZE);
> -	if (IS_ERR(vma)) {
> -		ret = PTR_ERR(vma);
> -		goto err_id;
> -	}
> -
> -	/* We'll keep just the first (doorbell/proc) page permanently kmap'd. */
> -	client->vma = vma;
> -
> -	vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
> -	if (IS_ERR(vaddr)) {
> -		ret = PTR_ERR(vaddr);
> -		goto err_vma;
> -	}
> -	client->vaddr = vaddr;
> -
> -	DRM_DEBUG_DRIVER("new priority %u client %p: stage_id %u\n",
> -			 priority, client, client->stage_id);
> -
> -	return client;
> -
> -err_vma:
> -	i915_vma_unpin_and_release(&client->vma, 0);
> -err_id:
> -	ida_simple_remove(&guc->stage_ids, client->stage_id);
> -err_client:
> -	kfree(client);
> -	return ERR_PTR(ret);
> -}
> -
> -static void guc_client_free(struct intel_guc_client *client)
> -{
> -	i915_vma_unpin_and_release(&client->vma, I915_VMA_RELEASE_MAP);
> -	ida_simple_remove(&client->guc->stage_ids, client->stage_id);
> -	kfree(client);
> -}
> -
> -static int guc_clients_create(struct intel_guc *guc)
> -{
> -	struct intel_guc_client *client;
> -
> -	GEM_BUG_ON(guc->execbuf_client);
> -
> -	client = guc_client_alloc(guc, GUC_CLIENT_PRIORITY_KMD_NORMAL);
> -	if (IS_ERR(client)) {
> -		DRM_ERROR("Failed to create GuC client for submission!\n");
> -		return PTR_ERR(client);
> -	}
> -	guc->execbuf_client = client;
> -
> -	return 0;
> -}
> -
> -static void guc_clients_destroy(struct intel_guc *guc)
> -{
> -	struct intel_guc_client *client;
> -
> -	client = fetch_and_zero(&guc->execbuf_client);
> -	if (client)
> -		guc_client_free(client);
> -}
> -
> -static void __guc_client_enable(struct intel_guc_client *client)
> -{
> -	guc_proc_desc_init(client);
> -	guc_stage_desc_init(client);
> -}
> -
> -static void __guc_client_disable(struct intel_guc_client *client)
> -{
> -	guc_stage_desc_fini(client);
> -	guc_proc_desc_fini(client);
> -}
> -
> -static void guc_clients_enable(struct intel_guc *guc)
> -{
> -	__guc_client_enable(guc->execbuf_client);
> -}
> -
> -static void guc_clients_disable(struct intel_guc *guc)
> -{
> -	if (guc->execbuf_client)
> -		__guc_client_disable(guc->execbuf_client);
> -}
> -
>   /*
>    * Set up the memory resources to be shared with the GuC (via the GGTT)
>    * at firmware loading time.
> @@ -668,12 +527,20 @@ int intel_guc_submission_init(struct intel_guc *guc)
>   	 */
>   	GEM_BUG_ON(!guc->stage_desc_pool);
>   
> -	ret = guc_clients_create(guc);
> +	ret = guc_workqueue_create(guc);
>   	if (ret)
>   		goto err_pool;
>   
> +	ret = guc_proc_desc_create(guc);
> +	if (ret)
> +		goto err_workqueue;
> +
> +	spin_lock_init(&guc->wq_lock);
> +
>   	return 0;
>   
> +err_workqueue:
> +	guc_workqueue_destroy(guc);
>   err_pool:
>   	guc_stage_desc_pool_destroy(guc);
>   	return ret;
> @@ -681,10 +548,11 @@ int intel_guc_submission_init(struct intel_guc *guc)
>   
>   void intel_guc_submission_fini(struct intel_guc *guc)
>   {
> -	guc_clients_destroy(guc);
> -
> -	if (guc->stage_desc_pool)
> +	if (guc->stage_desc_pool) {
> +		guc_proc_desc_destroy(guc);
> +		guc_workqueue_destroy(guc);
>   		guc_stage_desc_pool_destroy(guc);
> +	}
>   }
>   
>   static void guc_interrupts_capture(struct intel_gt *gt)
> @@ -816,9 +684,8 @@ void intel_guc_submission_enable(struct intel_guc *guc)
>   		     sizeof(struct guc_wq_item) *
>   		     I915_NUM_ENGINES > GUC_WQ_SIZE);
>   
> -	GEM_BUG_ON(!guc->execbuf_client);
> -
> -	guc_clients_enable(guc);
> +	guc_proc_desc_init(guc);
> +	guc_stage_desc_init(guc);
>   
>   	/* Take over from manual control of ELSP (execlists) */
>   	guc_interrupts_capture(gt);
> @@ -836,7 +703,9 @@ void intel_guc_submission_disable(struct intel_guc *guc)
>   	GEM_BUG_ON(gt->awake); /* GT should be parked first */
>   
>   	guc_interrupts_release(gt);
> -	guc_clients_disable(guc);
> +
> +	guc_stage_desc_fini(guc);
> +	guc_proc_desc_fini(guc);
>   }
>   
>   static bool __guc_submission_support(struct intel_guc *guc)
> @@ -854,3 +723,8 @@ void intel_guc_submission_init_early(struct intel_guc *guc)
>   {
>   	guc->submission_supported = __guc_submission_support(guc);
>   }
> +
> +bool intel_engine_in_guc_submission_mode(const struct intel_engine_cs *engine)
> +{
> +	return engine->set_default_submission == guc_set_default_submission;
> +}
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
> index e2deb4e6f42a..e402a2932592 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
> @@ -6,48 +6,10 @@
>   #ifndef _INTEL_GUC_SUBMISSION_H_
>   #define _INTEL_GUC_SUBMISSION_H_
>   
> -#include <linux/spinlock.h>
> +#include <linux/types.h>
>   
> -#include "gt/intel_engine_types.h"
> -
> -#include "i915_gem.h"
> -#include "i915_selftest.h"
> -
> -struct drm_i915_private;
> -
> -/*
> - * This structure primarily describes the GEM object shared with the GuC.
> - * The specs sometimes refer to this object as a "GuC context", but we use
> - * the term "client" to avoid confusion with hardware contexts. This
> - * 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 descriptor" which holds sequence data for
> - * 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.
> - */
> -struct intel_guc_client {
> -	struct i915_vma *vma;
> -	void *vaddr;
> -	struct intel_guc *guc;
> -
> -	/* bitmap of (host) engine ids */
> -	u32 priority;
> -	u32 stage_id;
> -
> -	/* Protects GuC client's WQ access */
> -	spinlock_t wq_lock;
> -};
> +struct intel_guc;
> +struct intel_engine_cs;
>   
>   void intel_guc_submission_init_early(struct intel_guc *guc);
>   int intel_guc_submission_init(struct intel_guc *guc);
> @@ -56,5 +18,6 @@ void intel_guc_submission_disable(struct intel_guc *guc);
>   void intel_guc_submission_fini(struct intel_guc *guc);
>   int intel_guc_preempt_work_create(struct intel_guc *guc);
>   void intel_guc_preempt_work_destroy(struct intel_guc *guc);
> +bool intel_engine_in_guc_submission_mode(const struct intel_engine_cs *engine);
>   
>   #endif
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 5d5974e7f3ed..f32e7b016197 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1802,23 +1802,12 @@ static void i915_guc_log_info(struct seq_file *m,
>   static int i915_guc_info(struct seq_file *m, void *data)
>   {
>   	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> -	const struct intel_guc *guc = &dev_priv->gt.uc.guc;
> -	struct intel_guc_client *client = guc->execbuf_client;
>   
>   	if (!USES_GUC(dev_priv))
>   		return -ENODEV;
>   
>   	i915_guc_log_info(m, dev_priv);
>   
> -	if (!USES_GUC_SUBMISSION(dev_priv))
> -		return 0;
> -
> -	GEM_BUG_ON(!guc->execbuf_client);
> -
> -	seq_printf(m, "\nGuC execbuf client @ %p:\n", client);
> -	seq_printf(m, "\tPriority %d, GuC stage index: %u\n",
> -		   client->priority,
> -		   client->stage_id);
>   	/* Add more as required ... */
>   
>   	return 0;

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

  reply	other threads:[~2019-11-15  0:22 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-06 22:25 [PATCH 0/4] Start removing legacy guc code Daniele Ceraolo Spurio
2019-11-06 22:25 ` [Intel-gfx] " Daniele Ceraolo Spurio
2019-11-06 22:25 ` [PATCH 1/4] drm/i915/guc: Drop leftover preemption code Daniele Ceraolo Spurio
2019-11-06 22:25   ` [Intel-gfx] " Daniele Ceraolo Spurio
2019-11-07  8:17   ` Chris Wilson
2019-11-07  8:17     ` [Intel-gfx] " Chris Wilson
2019-11-06 22:25 ` [PATCH 2/4] drm/i915/guc: add a helper to allocate and map guc vma Daniele Ceraolo Spurio
2019-11-06 22:25   ` [Intel-gfx] " Daniele Ceraolo Spurio
2019-11-07  8:19   ` Chris Wilson
2019-11-07  8:19     ` [Intel-gfx] " Chris Wilson
2019-11-06 22:25 ` [PATCH 3/4] drm/i915/guc: kill doorbell code and selftests Daniele Ceraolo Spurio
2019-11-06 22:25   ` [Intel-gfx] " Daniele Ceraolo Spurio
2019-11-14 23:56   ` John Harrison
2019-11-14 23:56     ` [Intel-gfx] " John Harrison
2019-11-18 22:20     ` Daniele Ceraolo Spurio
2019-11-18 22:20       ` [Intel-gfx] " Daniele Ceraolo Spurio
2019-11-06 22:25 ` [PATCH 4/4] drm/i915/guc: kill the GuC client Daniele Ceraolo Spurio
2019-11-06 22:25   ` [Intel-gfx] " Daniele Ceraolo Spurio
2019-11-15  0:22   ` John Harrison [this message]
2019-11-15  0:22     ` John Harrison
2019-11-07  0:52 ` ✗ Fi.CI.CHECKPATCH: warning for Start removing legacy guc code Patchwork
2019-11-07  0:52   ` [Intel-gfx] " Patchwork
2019-11-07  1:08 ` ✗ Fi.CI.DOCS: " Patchwork
2019-11-07  1:08   ` [Intel-gfx] " Patchwork
2019-11-07  1:15 ` ✓ Fi.CI.BAT: success " Patchwork
2019-11-07  1:15   ` [Intel-gfx] " Patchwork
2019-11-08  4:36 ` ✓ Fi.CI.IGT: " Patchwork
2019-11-08  4:36   ` [Intel-gfx] " Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cea87371-d9b2-c41e-6cee-8468143b2d34@Intel.com \
    --to=john.c.harrison@intel.com \
    --cc=daniele.ceraolospurio@intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.