Re: [Intel-gfx] [PATCH 02/12] drm/i915/guc: simplify guc client
On Tue, Jul 09, 2019 at 05:54:27PM -0700, Daniele Ceraolo Spurio wrote:
> We originally added support, in some cases partial, for different modes
> of operations via guc clients:
>
> - proxy vs direct submission;
> - variable engine mask per-client.
>
> We only ever used one flow (all submissions via a single proxy), so the
> other code paths haven't been exercised and are most likely
> non-functional. The guc firmware interface is also in the process of
> being updated to better fit the i915 flow and our client abstraction
> will need to change accordingly (or possibly go away entirely), so these
> old unused paths can be considered dead and removed.
>
> Signed-off-by: Daniele Ceraolo Spurio
> Cc: Chris Wilson
> Cc: Michal Wajdeczko
> Cc: Matthew Brost
> Cc: John Harrison
> Acked-by: Matthew Brost
drm/i915/guc: simplify guc client
^ Sentence case
Reviewed-by: Michał Winiarski
-Michał
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 3 +-
> drivers/gpu/drm/i915/intel_guc_submission.c | 73 ++---
> drivers/gpu/drm/i915/intel_guc_submission.h | 2 -
> drivers/gpu/drm/i915/selftests/intel_guc.c | 12 +---
> 4 files changed, 8 insertions(+), 82 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index b4d195677877..dc65a6131a5b 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2021,7 +2021,6 @@ 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 intel_guc_client *client = guc->execbuf_client;
> intel_engine_mask_t tmp;
> int index;
>
> @@ -2051,7 +2050,7 @@ static int i915_guc_stage_pool(struct seq_file *m, void
> *data)
> desc->wq_addr, desc->wq_size);
> seq_putc(m, '\n');
>
> - for_each_engine_masked(engine, dev_priv, client->engines, tmp) {
> + for_each_engine(engine, dev_priv, tmp) {
> u32 guc_engine_id = engine->guc_id;
> struct guc_execlist_context *lrc =
> &desc->lrc[guc_engine_id];
> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c
> b/drivers/gpu/drm/i915/intel_guc_submission.c
> index 8520bb224175..30692f8289bd 100644
> --- a/drivers/gpu/drm/i915/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
> @@ -363,10 +363,7 @@ static void guc_stage_desc_pool_destroy(struct intel_guc
> *guc)
> static void guc_stage_desc_init(struct intel_guc_client *client)
> {
> struct intel_guc *guc = client->guc;
> - struct i915_gem_context *ctx = client->owner;
> - struct i915_gem_engines_iter it;
> struct guc_stage_desc *desc;
> - struct intel_context *ce;
> u32 gfx_addr;
>
> desc = __get_stage_desc(client);
> @@ -380,55 +377,6 @@ static void guc_stage_desc_init(struct intel_guc_client
> *client)
> desc->priority = client->priority;
> desc->db_id = client->doorbell_id;
>
> - for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) {
> - struct guc_execlist_context *lrc;
> -
> - if (!(ce->engine->mask & client->engines))
> - continue;
> -
> - /* TODO: We have a design issue to be solved here. Only when we
> - * receive the first batch, we know which engine is used by the
> - * user. But here GuC expects the lrc and ring to be pinned. It
> - * is not an issue for default context, which is the only one
> - * for now who owns a GuC client. But for future owner of GuC
> - * client, need to make sure lrc is pinned prior to enter here.
> - */
> - if (!ce->state)
> - break; /* XXX: continue? */
> -
> - /*
> - * XXX: When this is a GUC_STAGE_DESC_ATTR_KERNEL client (proxy
> - * submission or, in other words, not using a direct submission
> - * model) the KMD's LRCA is not used for any work submission.
> - * Instead, the GuC uses the LRCA of the user mode context (see
> - * guc_add_request below).
> - */
> - lrc = &desc->lrc[ce->engine->guc_id];
> - lrc->context_desc = lower_32_bits(ce->lrc_desc);
> -
> - /* The state page is after PPHWSP */
> - lrc->ring_lrca = intel_guc_ggtt_offset(guc, ce->state) +
> - LRC_STATE_PN * PAGE_SIZE;
> -
> - /* XXX: In direct submission, the GuC wants the HW context id
> - * here. In proxy submission, it wants the stage id
> - */
> - lrc->context_id = (client->stage_id << GUC_ELC_CTXID
[Intel-gfx] [PATCH 02/12] drm/i915/guc: simplify guc client
We originally added support, in some cases partial, for different modes
of operations via guc clients:
- proxy vs direct submission;
- variable engine mask per-client.
We only ever used one flow (all submissions via a single proxy), so the
other code paths haven't been exercised and are most likely
non-functional. The guc firmware interface is also in the process of
being updated to better fit the i915 flow and our client abstraction
will need to change accordingly (or possibly go away entirely), so these
old unused paths can be considered dead and removed.
Signed-off-by: Daniele Ceraolo Spurio
Cc: Chris Wilson
Cc: Michal Wajdeczko
Cc: Matthew Brost
Cc: John Harrison
Acked-by: Matthew Brost
---
drivers/gpu/drm/i915/i915_debugfs.c | 3 +-
drivers/gpu/drm/i915/intel_guc_submission.c | 73 ++---
drivers/gpu/drm/i915/intel_guc_submission.h | 2 -
drivers/gpu/drm/i915/selftests/intel_guc.c | 12 +---
4 files changed, 8 insertions(+), 82 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index b4d195677877..dc65a6131a5b 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2021,7 +2021,6 @@ 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 intel_guc_client *client = guc->execbuf_client;
intel_engine_mask_t tmp;
int index;
@@ -2051,7 +2050,7 @@ static int i915_guc_stage_pool(struct seq_file *m, void
*data)
desc->wq_addr, desc->wq_size);
seq_putc(m, '\n');
- for_each_engine_masked(engine, dev_priv, client->engines, tmp) {
+ for_each_engine(engine, dev_priv, tmp) {
u32 guc_engine_id = engine->guc_id;
struct guc_execlist_context *lrc =
&desc->lrc[guc_engine_id];
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c
b/drivers/gpu/drm/i915/intel_guc_submission.c
index 8520bb224175..30692f8289bd 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -363,10 +363,7 @@ static void guc_stage_desc_pool_destroy(struct intel_guc
*guc)
static void guc_stage_desc_init(struct intel_guc_client *client)
{
struct intel_guc *guc = client->guc;
- struct i915_gem_context *ctx = client->owner;
- struct i915_gem_engines_iter it;
struct guc_stage_desc *desc;
- struct intel_context *ce;
u32 gfx_addr;
desc = __get_stage_desc(client);
@@ -380,55 +377,6 @@ static void guc_stage_desc_init(struct intel_guc_client
*client)
desc->priority = client->priority;
desc->db_id = client->doorbell_id;
- for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) {
- struct guc_execlist_context *lrc;
-
- if (!(ce->engine->mask & client->engines))
- continue;
-
- /* TODO: We have a design issue to be solved here. Only when we
-* receive the first batch, we know which engine is used by the
-* user. But here GuC expects the lrc and ring to be pinned. It
-* is not an issue for default context, which is the only one
-* for now who owns a GuC client. But for future owner of GuC
-* client, need to make sure lrc is pinned prior to enter here.
-*/
- if (!ce->state)
- break; /* XXX: continue? */
-
- /*
-* XXX: When this is a GUC_STAGE_DESC_ATTR_KERNEL client (proxy
-* submission or, in other words, not using a direct submission
-* model) the KMD's LRCA is not used for any work submission.
-* Instead, the GuC uses the LRCA of the user mode context (see
-* guc_add_request below).
-*/
- lrc = &desc->lrc[ce->engine->guc_id];
- lrc->context_desc = lower_32_bits(ce->lrc_desc);
-
- /* The state page is after PPHWSP */
- lrc->ring_lrca = intel_guc_ggtt_offset(guc, ce->state) +
-LRC_STATE_PN * PAGE_SIZE;
-
- /* XXX: In direct submission, the GuC wants the HW context id
-* here. In proxy submission, it wants the stage id
-*/
- lrc->context_id = (client->stage_id << GUC_ELC_CTXID_OFFSET) |
- (ce->engine->guc_id << GUC_ELC_ENGINE_OFFSET);
-
- lrc->ring_begin = intel_guc_ggtt_offset(guc, ce->ring->vma);
- lrc->ring_end = lrc->ring_begin + ce->ring->size - 1;
- lrc->ring_next_free_location = lrc
