[Intel-gfx] [PATCH v2 06/14] drm/i915/guc: Add a second client, to be used for preemption
Michal Wajdeczko
michal.wajdeczko at intel.com
Thu Oct 19 19:36:39 UTC 2017
On Thu, 19 Oct 2017 20:36:11 +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.
>
> 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 | 11 +--
> drivers/gpu/drm/i915/i915_guc_submission.c | 107
> ++++++++++++++++++++---------
> drivers/gpu/drm/i915/intel_guc.h | 9 ++-
> drivers/gpu/drm/i915/intel_uncore.c | 2 +-
> 4 files changed, 91 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index c65e381b85f3..daf7a16a4ee2 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2459,7 +2459,7 @@ static bool check_guc_submission(struct seq_file
> *m)
> struct drm_i915_private *dev_priv = node_to_i915(m->private);
> const struct intel_guc *guc = &dev_priv->guc;
> - if (!guc->execbuf_client) {
> + if (!guc->client[SUBMIT]) {
> seq_printf(m, "GuC submission %s\n",
> HAS_GUC_SCHED(dev_priv) ?
> "disabled" :
> @@ -2474,6 +2474,7 @@ 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->guc;
> + u32 i;
> if (!check_guc_submission(m))
> return 0;
> @@ -2482,8 +2483,10 @@ static int i915_guc_info(struct seq_file *m, void
> *data)
> seq_printf(m, "\t%*pb\n", GUC_NUM_DOORBELLS, guc->doorbell_bitmap);
> seq_printf(m, "Doorbell next cacheline: 0x%x\n\n", guc->db_cacheline);
> - seq_printf(m, "\nGuC execbuf client @ %p:\n", guc->execbuf_client);
> - i915_guc_client_info(m, dev_priv, guc->execbuf_client);
> + for (i = 0; i < I915_GUC_NUM_CLIENTS; i++) {
> + seq_printf(m, "\nGuC client @ %p:\n", guc->client[i]);
> + i915_guc_client_info(m, dev_priv, guc->client[i]);
> + }
> i915_guc_log_info(m, dev_priv);
> @@ -2497,7 +2500,7 @@ static int i915_guc_stage_pool(struct seq_file *m,
> void *data)
> struct drm_i915_private *dev_priv = node_to_i915(m->private);
> const struct intel_guc *guc = &dev_priv->guc;
> struct guc_stage_desc *desc = guc->stage_desc_pool_vaddr;
> - struct i915_guc_client *client = guc->execbuf_client;
> + struct i915_guc_client *client = guc->client[SUBMIT];
> unsigned int tmp;
> int index;
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
> b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 0bd1fcffa78d..d8b8125aa4cc 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 (SUBMIT) is charged with all
> submissions to the
> + * GuC, the other one (PREEMPT) is responsible for preempting the
> SUBMIT one.
> + * 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).
Hmm, it's little unfortunate that SUBMIT and PREEMPT enums are defined
in intel_guc.h while mostly used in guc_submission.c
> *
> * 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)
> + desc->attribute |= GUC_STAGE_DESC_ATTR_PREEMPT;
> desc->stage_id = client->stage_id;
> desc->priority = client->priority;
> desc->db_id = client->doorbell_id;
> @@ -553,7 +556,7 @@ static void i915_guc_submit(struct intel_engine_cs
> *engine)
> {
> struct drm_i915_private *dev_priv = engine->i915;
> struct intel_guc *guc = &dev_priv->guc;
> - struct i915_guc_client *client = guc->execbuf_client;
> + struct i915_guc_client *client = guc->client[SUBMIT];
what about using helpers like:
static inline
struct i915_guc_client *guc_exec_client(struct intel_guc *guc)
{
return guc->client[SUBMIT];
}
> + struct i915_guc_client *client = guc->client[SUBMIT];
> struct intel_engine_execlists * const execlists = &engine->execlists;
> struct execlist_port *port = execlists->port;
> const unsigned int engine_id = engine->id;
> @@ -750,10 +753,11 @@ static int __reset_doorbell(struct
> i915_guc_client* client, u16 db_id)
> */
> static int guc_init_doorbell_hw(struct intel_guc *guc)
> {
> - struct i915_guc_client *client = guc->execbuf_client;
> + struct i915_guc_client *client = guc->client[SUBMIT];
> bool recreate_first_client = false;
> u16 db_id;
> int ret;
> + u32 i;
> /* For unused doorbells, make sure they are disabled */
> for_each_clear_bit(db_id, guc->doorbell_bitmap, GUC_NUM_DOORBELLS) {
> @@ -761,7 +765,7 @@ static int guc_init_doorbell_hw(struct intel_guc
> *guc)
> continue;
> if (has_doorbell(client)) {
> - /* Borrow execbuf_client (we will recreate it later) */
> + /* Borrow submit client (we will recreate it later) */
> destroy_doorbell(client);
> recreate_first_client = true;
> }
> @@ -780,14 +784,14 @@ static int guc_init_doorbell_hw(struct intel_guc
> *guc)
> __update_doorbell_desc(client, client->doorbell_id);
> }
> - /* Now for every client (and not only execbuf_client) make sure their
> + /* Now for every client (not only submission client) make sure their
> * doorbells are known by the GuC */
> - //for (client = client_list; client != NULL; client = client->next)
> + for (i = 0; i < I915_GUC_NUM_CLIENTS; i++)
> {
> - ret = __create_doorbell(client);
> + ret = __create_doorbell(guc->client[i]);
> if (ret) {
> DRM_ERROR("Couldn't recreate client %u doorbell: %d\n",
> - client->stage_id, ret);
> + guc->client[i]->stage_id, ret);
> return ret;
> }
> }
> @@ -914,6 +918,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->client[SUBMIT] = 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->client[SUBMIT]);
> + guc->client[SUBMIT] = NULL;
> + return PTR_ERR(client);
> + }
> + guc->client[PREEMPT] = client;
> +
> + return 0;
> +}
> +
> +static void guc_clients_destroy(struct intel_guc *guc)
> +{
> + struct i915_guc_client *client;
> + u32 i;
> +
> + for (i = 0; i < I915_GUC_NUM_CLIENTS; i++) {
> + client = fetch_and_zero(&guc->client[i]);
> + guc_client_free(client);
> + }
> +}
> +
> static void guc_policy_init(struct guc_policy *policy)
> {
> policy->execution_quantum = POLICY_DEFAULT_EXECUTION_QUANTUM_US;
> @@ -1143,7 +1188,6 @@ static void guc_interrupts_release(struct
> drm_i915_private *dev_priv)
> 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;
> @@ -1161,28 +1205,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->client[SUBMIT]) {
> + GEM_BUG_ON(guc->client[PREEMPT]);
> + err = guc_clients_create(guc);
> + if (err)
> + return err;
> + } else {
> + guc_reset_wq(guc->client[SUBMIT]);
> + guc_reset_wq(guc->client[PREEMPT]);
> }
> 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);
> @@ -1194,9 +1239,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;
> }
> @@ -1209,6 +1253,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..15eadf27b7ae 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -39,6 +39,13 @@
> * pool and doorbells. intel_guc owns a i915_guc_client to replace the
> legacy
> * ExecList submission.
> */
> +
> +#define I915_GUC_NUM_CLIENTS 2
> +enum i915_guc_client_id {
> + SUBMIT = 0,
> + PREEMPT
> +};
> +
> struct intel_guc {
> struct intel_uc_fw fw;
> struct intel_guc_log log;
> @@ -57,7 +64,7 @@ struct intel_guc {
> struct i915_vma *shared_data;
> void *shared_data_vaddr;
> - struct i915_guc_client *execbuf_client;
> + struct i915_guc_client *client[I915_GUC_NUM_CLIENTS];
> DECLARE_BITMAP(doorbell_bitmap, GUC_NUM_DOORBELLS);
> /* Cyclic counter mod pagesize */
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c
> b/drivers/gpu/drm/i915/intel_uncore.c
> index 20e3c65c0999..3d91d8a28d9f 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1795,7 +1795,7 @@ bool intel_has_gpu_reset(struct drm_i915_private
> *dev_priv)
> bool intel_has_reset_engine(struct drm_i915_private *dev_priv)
> {
> return (dev_priv->info.has_reset_engine &&
> - !dev_priv->guc.execbuf_client &&
> + !dev_priv->guc.client[SUBMIT] &&
> i915_modparams.reset >= 2);
> }
More information about the Intel-gfx
mailing list