[Intel-gfx] [PATCH v3 05/12] drm/i915/guc: Add a second client, to be used for preemption
Michal Wajdeczko
michal.wajdeczko at intel.com
Thu Oct 26 12:50:58 UTC 2017
On Wed, 25 Oct 2017 22:00:13 +0200, Michał Winiarski
<michal.winiarski at intel.com> wrote:
> From: Dave Gordon <david.s.gordon at intel.com>
>
> This second client is created with priority KMD_HIGH, and marked
> as preemptive. This will allow us to request preemption using GuC
> actions.
>
> v2: Extract clients creation into a helper, debugfs fixups. (Michał)
> Recreate doorbell on init. (Daniele)
> Move clients into an array.
>
> v3: And move clients back from an array, to get rid of the enum (Michał)
>
> Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Jeff McGee <jeff.mcgee at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Oscar Mateo <oscar.mateo at intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 2 +
> drivers/gpu/drm/i915/i915_guc_submission.c | 112
> ++++++++++++++++++++---------
> drivers/gpu/drm/i915/intel_guc.h | 1 +
> 3 files changed, 82 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index c65e381b85f3..6e178d75bb17 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2484,6 +2484,8 @@ static int i915_guc_info(struct seq_file *m, void
> *data)
> seq_printf(m, "\nGuC execbuf client @ %p:\n", guc->execbuf_client);
> i915_guc_client_info(m, dev_priv, guc->execbuf_client);
> + seq_printf(m, "\nGuC preempt client @ %p:\n", guc->preempt_client);
> + i915_guc_client_info(m, dev_priv, guc->preempt_client);
> i915_guc_log_info(m, dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
> b/drivers/gpu/drm/i915/i915_guc_submission.c
> index d1a5613da24c..1d3123e74bc4 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -33,10 +33,11 @@
> *
> * GuC client:
> * A i915_guc_client refers to a submission path through GuC.
> Currently, there
> - * is 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).
> + * are two clients. One of them (the execbuf_client) is charged with all
> + * submissions to the GuC, the other one (preempt_client) is
> responsible for
> + * preempting the execbuf_client. 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).
> *
> * GuC stage descriptor:
> * During initialization, the driver allocates a static pool of 1024
> such
> @@ -363,6 +364,8 @@ static void guc_stage_desc_init(struct intel_guc
> *guc,
> memset(desc, 0, sizeof(*desc));
> desc->attribute = GUC_STAGE_DESC_ATTR_ACTIVE |
> GUC_STAGE_DESC_ATTR_KERNEL;
> + if (client->priority <= GUC_CLIENT_PRIORITY_HIGH)
Can we use
if (client->priority == GUC_CLIENT_PRIORITY_KMD_HIGH ||
client->priority == GUC_CLIENT_PRIORITY_HIGH)
I assume compiler will optimize it to the same asm, but then we can
avoid condition that may be read wrong (priority less then high)
> + desc->attribute |= GUC_STAGE_DESC_ATTR_PREEMPT;
> desc->stage_id = client->stage_id;
> desc->priority = client->priority;
> desc->db_id = client->doorbell_id;
> @@ -763,14 +766,18 @@ static int guc_init_doorbell_hw(struct intel_guc
> *guc)
> /* Now for every client (and not only execbuf_client) make sure their
> * doorbells are known by the GuC */
> - //for (client = client_list; client != NULL; client = client->next)
> - {
> - ret = __create_doorbell(client);
> - if (ret) {
> - DRM_ERROR("Couldn't recreate client %u doorbell: %d\n",
> - client->stage_id, ret);
> - return ret;
> - }
> + ret = __create_doorbell(guc->execbuf_client);
> + if (ret) {
> + DRM_ERROR("Couldn't recreate client %u doorbell: %d\n",
> + guc->execbuf_client->stage_id, ret);
This
> + return ret;
> + }
> +
> + ret = __create_doorbell(guc->preempt_client);
> + if (ret) {
> + DRM_ERROR("Couldn't recreate client %u doorbell: %d\n",
> + guc->preempt_client->stage_id, ret);
And this error can be moved to __create_doorbell to avoid duplication
> + return ret;
> }
> /* Read back & verify all (used & unused) doorbell registers */
> @@ -895,6 +902,47 @@ static void guc_client_free(struct i915_guc_client
> *client)
> kfree(client);
> }
> +static int guc_clients_create(struct intel_guc *guc)
> +{
> + struct drm_i915_private *dev_priv = guc_to_i915(guc);
> + struct i915_guc_client *client;
> +
> + client = guc_client_alloc(dev_priv,
> + INTEL_INFO(dev_priv)->ring_mask,
> + GUC_CLIENT_PRIORITY_KMD_NORMAL,
> + dev_priv->kernel_context);
> + if (IS_ERR(client)) {
> + DRM_ERROR("Failed to create GuC client for submission!\n");
> + return PTR_ERR(client);
> + }
> + guc->execbuf_client = client;
> +
> + client = guc_client_alloc(dev_priv,
> + INTEL_INFO(dev_priv)->ring_mask,
> + GUC_CLIENT_PRIORITY_KMD_HIGH,
> + dev_priv->preempt_context);
> + if (IS_ERR(client)) {
> + DRM_ERROR("Failed to create GuC client for preemption!\n");
> + guc_client_free(guc->execbuf_client);
> + guc->execbuf_client = NULL;
> + return PTR_ERR(client);
> + }
> + guc->preempt_client = client;
> +
> + return 0;
> +}
> +
> +static void guc_clients_destroy(struct intel_guc *guc)
> +{
> + struct i915_guc_client *client;
> +
> + client = fetch_and_zero(&guc->execbuf_client);
> + guc_client_free(client);
> +
> + client = fetch_and_zero(&guc->preempt_client);
> + guc_client_free(client);
> +}
> +
> static void guc_policy_init(struct guc_policy *policy)
> {
> policy->execution_quantum = POLICY_DEFAULT_EXECUTION_QUANTUM_US;
> @@ -1134,7 +1182,6 @@ static void i915_guc_submission_unpark(struct
> intel_engine_cs *engine)
> int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
> {
> struct intel_guc *guc = &dev_priv->guc;
> - struct i915_guc_client *client = guc->execbuf_client;
> struct intel_engine_cs *engine;
> enum intel_engine_id id;
> int err;
> @@ -1152,28 +1199,29 @@ int i915_guc_submission_enable(struct
> drm_i915_private *dev_priv)
> sizeof(struct guc_wq_item) *
> I915_NUM_ENGINES > GUC_WQ_SIZE);
> - if (!client) {
> - client = guc_client_alloc(dev_priv,
> - INTEL_INFO(dev_priv)->ring_mask,
> - GUC_CLIENT_PRIORITY_KMD_NORMAL,
> - dev_priv->kernel_context);
> - if (IS_ERR(client)) {
> - DRM_ERROR("Failed to create GuC client for execbuf!\n");
> - return PTR_ERR(client);
> - }
> -
> - guc->execbuf_client = client;
> + /*
> + * We're being called on both module initialization and on reset,
> + * until this flow is changed, we're using regular client presence to
> + * determine which case are we in, and whether we should allocate new
> + * clients or just reset their workqueues.
> + */
> + if (!guc->execbuf_client) {
> + GEM_BUG_ON(guc->preempt_client);
You can move this GEM_BUG_ON to guc_clients_create and check both
> + err = guc_clients_create(guc);
> + if (err)
> + return err;
> + } else {
> + guc_reset_wq(guc->execbuf_client);
> + guc_reset_wq(guc->preempt_client);
> }
> err = intel_guc_sample_forcewake(guc);
> if (err)
> - goto err_execbuf_client;
> -
> - guc_reset_wq(client);
> + goto err_free_clients;
> err = guc_init_doorbell_hw(guc);
> if (err)
> - goto err_execbuf_client;
> + goto err_free_clients;
> /* Take over from manual control of ELSP (execlists) */
> guc_interrupts_capture(dev_priv);
> @@ -1187,9 +1235,8 @@ int i915_guc_submission_enable(struct
> drm_i915_private *dev_priv)
> return 0;
> -err_execbuf_client:
> - guc_client_free(guc->execbuf_client);
> - guc->execbuf_client = NULL;
> +err_free_clients:
> + guc_clients_destroy(guc);
> return err;
> }
> @@ -1204,6 +1251,5 @@ void i915_guc_submission_disable(struct
> drm_i915_private *dev_priv)
> /* Revert back to manual ELSP submission */
> intel_engines_reset_default_submission(dev_priv);
> - guc_client_free(guc->execbuf_client);
> - guc->execbuf_client = NULL;
> + guc_clients_destroy(guc);
> }
> diff --git a/drivers/gpu/drm/i915/intel_guc.h
> b/drivers/gpu/drm/i915/intel_guc.h
> index aa1583167b0a..6ca55c5bed3c 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -58,6 +58,7 @@ struct intel_guc {
> void *shared_data_vaddr;
> struct i915_guc_client *execbuf_client;
> + struct i915_guc_client *preempt_client;
> DECLARE_BITMAP(doorbell_bitmap, GUC_NUM_DOORBELLS);
> /* Cyclic counter mod pagesize */
More information about the Intel-gfx
mailing list