[Intel-gfx] [PATCH 3/5] drm/i915/guc: use a separate GuC client for each engine
Dave Gordon
david.s.gordon at intel.com
Tue Jul 19 15:13:41 UTC 2016
On 19/07/16 16:02, Tvrtko Ursulin wrote:
>
> On 19/07/16 13:59, Dave Gordon wrote:
>> When using a single GuC client for multiple engines, the i915 driver has
>> to merge all work items into a single work queue, which the GuC firmware
>> then demultiplexes into separate submission queues per engine. In
>> theory, this could lead to the single queue becoming a bottleneck in
>> which an excess of outstanding work for one or more engines might
>> prevent work for an idle engine reaching the hardware.
>>
>> To reduce this risk, we can create one GuC client per engine. Each will
>> have its own workqueue, to be used only for work targeting a single
>> engine, so there will be no cross-engine contention for workqueue slots.
>>
>> Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_debugfs.c | 25 ++++++++++++++++-----
>> drivers/gpu/drm/i915/i915_guc_submission.c | 35
>> +++++++++++++++++++-----------
>> drivers/gpu/drm/i915/intel_guc.h | 2 +-
>> 3 files changed, 42 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 90aef45..5cbb8ef 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2570,20 +2570,26 @@ static int i915_guc_info(struct seq_file *m,
>> void *data)
>> struct drm_device *dev = node->minor->dev;
>> struct drm_i915_private *dev_priv = to_i915(dev);
>> struct intel_guc guc;
>> - struct i915_guc_client client = {};
>> + struct i915_guc_client *clients;
>> struct intel_engine_cs *engine;
>> + enum intel_engine_id id;
>> u64 total = 0;
>>
>> if (!HAS_GUC_SCHED(dev_priv))
>> return 0;
>>
>> + clients = kcalloc(I915_NUM_ENGINES, sizeof(*clients), GFP_KERNEL);
>> + if (clients == NULL)
>> + return -ENOMEM;
>> +
>> if (mutex_lock_interruptible(&dev->struct_mutex))
>> - return 0;
>> + goto done;
>>
>> /* Take a local copy of the GuC data, so we can dump it at
>> leisure */
>> guc = dev_priv->guc;
>> - if (guc.execbuf_client)
>> - client = *guc.execbuf_client;
>> + for_each_engine_id(engine, dev_priv, id)
>> + if (guc.exec_clients[id])
>> + clients[id] = *guc.exec_clients[id];
>>
>> mutex_unlock(&dev->struct_mutex);
>>
>> @@ -2606,11 +2612,18 @@ static int i915_guc_info(struct seq_file *m,
>> void *data)
>> }
>> seq_printf(m, "\t%s: %llu\n", "Total", total);
>>
>> - seq_printf(m, "\nGuC execbuf client @ %p:\n", guc.execbuf_client);
>> - i915_guc_client_info(m, dev_priv, &client);
>> + for_each_engine_id(engine, dev_priv, id) {
>> + seq_printf(m, "\nGuC exec_client[%d] @ %p:\n",
>> + id, guc.exec_clients[id]);
>
> Minor and not a blocker for this patch, but I would potentially
> re-consider if printing out the client pointer is useful.
It's really only useful to know whether it's NULL or not; but printing
the pointer itself is simpler than printing a message saying that. This
is a debugfs interface, so the content is pretty much ad-hoc.
.Dave.
>> + if (guc.exec_clients[id])
>> + i915_guc_client_info(m, dev_priv, &clients[id]);
>> + }
>>
>> /* Add more as required ... */
>>
>> +done:
>> + kfree(clients);
>> +
>> return 0;
>> }
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index dc5f485..b0f9945 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -434,7 +434,9 @@ static void guc_fini_ctx_desc(struct intel_guc *guc,
>> int i915_guc_wq_check_space(struct drm_i915_gem_request *request)
>> {
>> const size_t wqi_size = sizeof(struct guc_wq_item);
>> - struct i915_guc_client *gc = request->i915->guc.execbuf_client;
>> + enum intel_engine_id engine_id = request->engine->id;
>> + struct intel_guc *guc = &request->i915->guc;
>> + struct i915_guc_client *gc = guc->exec_clients[engine_id];
>> struct guc_process_desc *desc;
>> u32 freespace;
>>
>> @@ -589,7 +591,7 @@ int i915_guc_submit(struct drm_i915_gem_request *rq)
>> {
>> unsigned int engine_id = rq->engine->id;
>> struct intel_guc *guc = &rq->i915->guc;
>> - struct i915_guc_client *client = guc->execbuf_client;
>> + struct i915_guc_client *client = guc->exec_clients[engine_id];
>> int b_ret;
>>
>> guc_add_workqueue_item(client, rq);
>> @@ -723,7 +725,7 @@ static bool guc_doorbell_check(struct intel_guc
>> *guc, uint16_t db_id)
>> */
>> static void guc_init_doorbell_hw(struct intel_guc *guc)
>> {
>> - struct i915_guc_client *client = guc->execbuf_client;
>> + struct i915_guc_client *client = guc->exec_clients[RCS];
>> uint16_t db_id;
>> int i, err;
>>
>> @@ -1004,17 +1006,21 @@ int i915_guc_submission_enable(struct
>> drm_i915_private *dev_priv)
>> {
>> struct intel_guc *guc = &dev_priv->guc;
>> struct i915_guc_client *client;
>> + struct intel_engine_cs *engine;
>>
>> - /* client for execbuf submission */
>> - client = guc_client_alloc(dev_priv,
>> - GUC_CTX_PRIORITY_KMD_NORMAL,
>> - dev_priv->kernel_context);
>> - if (!client) {
>> - DRM_ERROR("Failed to create execbuf guc_client\n");
>> - return -ENOMEM;
>> + for_each_engine(engine, dev_priv) {
>> + /* client for execbuf submission */
>> + client = guc_client_alloc(dev_priv,
>> + GUC_CTX_PRIORITY_KMD_NORMAL,
>> + dev_priv->kernel_context);
>> + if (!client) {
>> + DRM_ERROR("Failed to create GuC client(s)\n");
>> + return -ENOMEM;
>> + }
>> +
>> + guc->exec_clients[engine->id] = client;
>> }
>>
>> - guc->execbuf_client = client;
>> host2guc_sample_forcewake(guc, client);
>> guc_init_doorbell_hw(guc);
>>
>> @@ -1024,9 +1030,12 @@ int i915_guc_submission_enable(struct
>> drm_i915_private *dev_priv)
>> void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
>> {
>> struct intel_guc *guc = &dev_priv->guc;
>> + struct intel_engine_cs *engine;
>>
>> - guc_client_free(dev_priv, guc->execbuf_client);
>> - guc->execbuf_client = NULL;
>> + for_each_engine(engine, dev_priv) {
>> + guc_client_free(dev_priv, guc->exec_clients[engine->id]);
>> + guc->exec_clients[engine->id] = NULL;
>> + }
>> }
>>
>> void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index 3e3e743..7b4cc4d 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -132,7 +132,7 @@ struct intel_guc {
>> struct drm_i915_gem_object *ctx_pool_obj;
>> struct ida ctx_ids;
>>
>> - struct i915_guc_client *execbuf_client;
>> + struct i915_guc_client *exec_clients[I915_NUM_ENGINES];
>>
>> DECLARE_BITMAP(doorbell_bitmap, GUC_MAX_DOORBELLS);
>> uint32_t db_cacheline; /* Cyclic counter mod pagesize */
>>
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>
> Regards,
> Tvrtko
More information about the Intel-gfx
mailing list