From: "Michal Wajdeczko" <michal.wajdeczko@intel.com>
To: intel-gfx@lists.freedesktop.org,
"Michał Winiarski" <michal.winiarski@intel.com>
Cc: Dave Gordon <david.s.gordon@intel.com>
Subject: Re: [PATCH v2 06/14] drm/i915/guc: Add a second client, to be used for preemption
Date: Thu, 19 Oct 2017 21:36:39 +0200 [thread overview]
Message-ID: <op.y8dffdplxaggs7@mwajdecz-mobl1.ger.corp.intel.com> (raw)
In-Reply-To: <20171019183619.6235-7-michal.winiarski@intel.com>
On Thu, 19 Oct 2017 20:36:11 +0200, Michał Winiarski
<michal.winiarski@intel.com> wrote:
> From: Dave Gordon <david.s.gordon@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@intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Jeff McGee <jeff.mcgee@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@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);
> }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2017-10-19 19:36 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-19 18:36 [PATCH 00/14] Preemption with GuC, third attempt Michał Winiarski
2017-10-19 18:36 ` [PATCH 01/14] drm/i915/guc: Do not use 0 for GuC doorbell cookie Michał Winiarski
2017-10-21 6:28 ` Sagar Arun Kamble
2017-10-19 18:36 ` [PATCH 02/14] drm/i915/guc: Extract GuC stage desc pool creation into a helper Michał Winiarski
2017-10-19 18:36 ` [PATCH v2 03/14] drm/i915/guc: Allocate separate shared data object for GuC communication Michał Winiarski
2017-10-19 18:36 ` [PATCH 04/14] drm/i915/guc: Initialize GuC before restarting engines Michał Winiarski
2017-10-25 18:44 ` Chris Wilson
2017-10-19 18:36 ` [PATCH 05/14] drm/i915/guc: Add preemption action to GuC firmware interface Michał Winiarski
2017-10-19 18:36 ` [PATCH v2 06/14] drm/i915/guc: Add a second client, to be used for preemption Michał Winiarski
2017-10-19 19:36 ` Michal Wajdeczko [this message]
2017-10-19 18:36 ` [PATCH 07/14] drm/i915/guc: Split guc_wq_item_append Michał Winiarski
2017-10-19 18:36 ` [PATCH v2 08/14] drm/i915: Extract "emit write" part of emit breadcrumb functions Michał Winiarski
2017-10-19 18:36 ` [PATCH 09/14] drm/i915: Add information needed to track engine preempt state Michał Winiarski
2017-10-19 18:36 ` [PATCH 10/14] drm/i915/guc: Keep request->priority for its lifetime Michał Winiarski
2017-10-19 18:36 ` [PATCH v2 11/14] drm/i915: Rename helpers used for unwinding, use macro for can_preempt Michał Winiarski
2017-10-19 19:18 ` Chris Wilson
2017-10-19 19:20 ` Chris Wilson
2017-10-19 18:36 ` [PATCH v3 12/14] drm/i915/guc: Preemption! With GuC Michał Winiarski
2017-10-19 19:14 ` Chris Wilson
2017-10-19 19:16 ` Chris Wilson
2017-10-20 9:00 ` Chris Wilson
2017-10-20 17:00 ` Chris Wilson
2017-10-23 23:14 ` Daniele Ceraolo Spurio
2017-10-25 18:15 ` Chris Wilson
2017-10-19 18:36 ` [PATCH 13/14] drm/i915/guc: Workaround the missing user interrupt after preemption Michał Winiarski
2017-10-19 19:44 ` Chris Wilson
2017-10-20 17:22 ` Chris Wilson
2017-10-20 17:42 ` Daniele Ceraolo Spurio
2017-10-20 16:08 ` Chris Wilson
2017-10-19 18:36 ` [PATCH 14/14] HAX Enable GuC Submission for CI Michał Winiarski
2017-10-19 19:11 ` ✗ Fi.CI.BAT: failure for Preemption with GuC, third attempt Patchwork
2017-10-26 21:21 ` 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=op.y8dffdplxaggs7@mwajdecz-mobl1.ger.corp.intel.com \
--to=michal.wajdeczko@intel.com \
--cc=david.s.gordon@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=michal.winiarski@intel.com \
/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.