Re: [Intel-gfx] [PATCH] drm/i915/guc: Decouple GuC engine id from ring id
On 01/23/2016 10:25 AM, Chris Wilson wrote:
On Fri, Jan 22, 2016 at 03:06:28PM -0800, yu@intel.com wrote:
> From: Alex Dai
>
> Previously GuC uses ring id as engine id because of same definition.
> But this is not true since this commit:
>
> commit de1add360522c876c25ef2ab1c94bdb509ab
> Author: Tvrtko Ursulin
> Date: Fri Jan 15 15:12:50 2016 +
>
> drm/i915: Decouple execbuf uAPI from internal implementation
>
> Added GuC engine id into GuC interface to decouple it from ring id used
> by driver.
>
> Signed-off-by: Alex Dai
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
> index c5db235..9a4e01e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2446,7 +2446,6 @@ static void i915_guc_client_info(struct seq_file *m,
> struct drm_i915_private *dev_priv,
> struct i915_guc_client *client)
> {
> - struct intel_engine_cs *ring;
>uint64_t tot = 0;
>uint32_t i;
>
> @@ -2461,10 +2460,9 @@ static void i915_guc_client_info(struct seq_file *m,
>seq_printf(m, "\tFailed doorbell: %u\n", client->b_fail);
>seq_printf(m, "\tLast submission result: %d\n", client->retcode);
>
> - for_each_ring(ring, dev_priv, i) {
> - seq_printf(m, "\tSubmissions: %llu %s\n",
> - client->submissions[i],
> - ring->name);
> + for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
> + seq_printf(m, "\tSubmissions: %llu, engine %d\n",
> + client->submissions[i], i);
>tot += client->submissions[i];
>}
>seq_printf(m, "\tTotal: %llu\n", tot);
> @@ -2477,7 +2475,6 @@ static int i915_guc_info(struct seq_file *m, void *data)
>struct drm_i915_private *dev_priv = dev->dev_private;
>struct intel_guc guc;
>struct i915_guc_client client = {};
> - struct intel_engine_cs *ring;
>enum intel_ring_id i;
>u64 total = 0;
>
> @@ -2501,9 +2498,9 @@ static int i915_guc_info(struct seq_file *m, void *data)
>seq_printf(m, "GuC last action error code: %d\n", guc.action_err);
>
>seq_printf(m, "\nGuC submissions:\n");
> - for_each_ring(ring, dev_priv, i) {
> - seq_printf(m, "\t%-24s: %10llu, last seqno 0x%08x %9d\n",
> - ring->name, guc.submissions[i],
> + for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
> + seq_printf(m, "\tengine %d: %10llu, last seqno 0x%08x %9d\n",
> + i, guc.submissions[i],
>guc.last_seqno[i], guc.last_seqno[i]);
>total += guc.submissions[i];
For debugfs, would it not be more convenient to use the ring->name and
show the corresponding guc engine id?
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 51ae5c1..601e2c8 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -365,6 +365,14 @@ static void guc_init_proc_desc(struct intel_guc *guc,
>kunmap_atomic(base);
> }
>
> +static const enum intel_ring_id guc_engine_map[GUC_MAX_ENGINES_NUM] = {
> + [GUC_RENDER_ENGINE] = RCS,
> + [GUC_VIDEO_ENGINE] = VCS,
> + [GUC_BLITTER_ENGINE] = BCS,
> + [GUC_VIDEOENHANCE_ENGINE] = VECS,
> + [GUC_VIDEO_ENGINE2] = VCS2
> +};
> +
> /*
> * Initialise/clear the context descriptor shared with the GuC firmware.
> *
> @@ -388,9 +396,10 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
>desc.priority = client->priority;
>desc.db_id = client->doorbell_id;
>
> - for (i = 0; i < I915_NUM_RINGS; i++) {
> + for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
>struct guc_execlist_context *lrc = &desc.lrc[i];
Again, would it not be more consistent to iterate over engines
(for_each_ring) and use struct guc_execlist_context *lrc =
&desc.lrc[ring->guc_id] ?
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index b12f2aa..b69eadb 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -158,6 +158,7 @@ struct intel_engine_cs {
> #define I915_NUM_RINGS 5
> #define _VCS(n) (VCS + (n))
>unsigned int exec_id;
> + unsigned int guc_engine_id;
Is the tautology useful? It is an engine, so intel_engine_cs->guc_id
would mean the correspondance map between our engines and the guc's.
Yes, guc_id is a good name. And I agree with you, using for_each_ring
where possible will be more consistent with other diver code. Thanks for
your review and comments.
Alex
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/guc: Decouple GuC engine id from ring id
On Fri, Jan 22, 2016 at 03:06:28PM -0800, yu@intel.com wrote:
> From: Alex Dai
>
> Previously GuC uses ring id as engine id because of same definition.
> But this is not true since this commit:
>
> commit de1add360522c876c25ef2ab1c94bdb509ab
> Author: Tvrtko Ursulin
> Date: Fri Jan 15 15:12:50 2016 +
>
> drm/i915: Decouple execbuf uAPI from internal implementation
>
> Added GuC engine id into GuC interface to decouple it from ring id used
> by driver.
>
> Signed-off-by: Alex Dai
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index c5db235..9a4e01e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2446,7 +2446,6 @@ static void i915_guc_client_info(struct seq_file *m,
>struct drm_i915_private *dev_priv,
>struct i915_guc_client *client)
> {
> - struct intel_engine_cs *ring;
> uint64_t tot = 0;
> uint32_t i;
>
> @@ -2461,10 +2460,9 @@ static void i915_guc_client_info(struct seq_file *m,
> seq_printf(m, "\tFailed doorbell: %u\n", client->b_fail);
> seq_printf(m, "\tLast submission result: %d\n", client->retcode);
>
> - for_each_ring(ring, dev_priv, i) {
> - seq_printf(m, "\tSubmissions: %llu %s\n",
> - client->submissions[i],
> - ring->name);
> + for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
> + seq_printf(m, "\tSubmissions: %llu, engine %d\n",
> + client->submissions[i], i);
> tot += client->submissions[i];
> }
> seq_printf(m, "\tTotal: %llu\n", tot);
> @@ -2477,7 +2475,6 @@ static int i915_guc_info(struct seq_file *m, void *data)
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_guc guc;
> struct i915_guc_client client = {};
> - struct intel_engine_cs *ring;
> enum intel_ring_id i;
> u64 total = 0;
>
> @@ -2501,9 +2498,9 @@ static int i915_guc_info(struct seq_file *m, void *data)
> seq_printf(m, "GuC last action error code: %d\n", guc.action_err);
>
> seq_printf(m, "\nGuC submissions:\n");
> - for_each_ring(ring, dev_priv, i) {
> - seq_printf(m, "\t%-24s: %10llu, last seqno 0x%08x %9d\n",
> - ring->name, guc.submissions[i],
> + for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
> + seq_printf(m, "\tengine %d: %10llu, last seqno 0x%08x %9d\n",
> + i, guc.submissions[i],
> guc.last_seqno[i], guc.last_seqno[i]);
> total += guc.submissions[i];
For debugfs, would it not be more convenient to use the ring->name and
show the corresponding guc engine id?
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
> b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 51ae5c1..601e2c8 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -365,6 +365,14 @@ static void guc_init_proc_desc(struct intel_guc *guc,
> kunmap_atomic(base);
> }
>
> +static const enum intel_ring_id guc_engine_map[GUC_MAX_ENGINES_NUM] = {
> + [GUC_RENDER_ENGINE] = RCS,
> + [GUC_VIDEO_ENGINE] = VCS,
> + [GUC_BLITTER_ENGINE] = BCS,
> + [GUC_VIDEOENHANCE_ENGINE] = VECS,
> + [GUC_VIDEO_ENGINE2] = VCS2
> +};
> +
> /*
> * Initialise/clear the context descriptor shared with the GuC firmware.
> *
> @@ -388,9 +396,10 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
> desc.priority = client->priority;
> desc.db_id = client->doorbell_id;
>
> - for (i = 0; i < I915_NUM_RINGS; i++) {
> + for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
> struct guc_execlist_context *lrc = &desc.lrc[i];
Again, would it not be more consistent to iterate over engines
(for_each_ring) and use struct guc_execlist_context *lrc =
&desc.lrc[ring->guc_id] ?
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
> b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index b12f2aa..b69eadb 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -158,6 +158,7 @@ struct intel_engine_cs {
> #define I915_NUM_RINGS 5
> #define _VCS(n) (VCS + (n))
> unsigned int exec_id;
> + unsigned int guc_engine_id;
Is the tautology useful? It is an engine, so intel_engine_cs->guc_id
would mean the correspondance map between our engines and the guc's.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915/guc: Decouple GuC engine id from ring id
From: Alex Dai
Previously GuC uses ring id as engine id because of same definition.
But this is not true since this commit:
commit de1add360522c876c25ef2ab1c94bdb509ab
Author: Tvrtko Ursulin
Date: Fri Jan 15 15:12:50 2016 +
drm/i915: Decouple execbuf uAPI from internal implementation
Added GuC engine id into GuC interface to decouple it from ring id used
by driver.
Signed-off-by: Alex Dai
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index c5db235..9a4e01e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2446,7 +2446,6 @@ static void i915_guc_client_info(struct seq_file *m,
struct drm_i915_private *dev_priv,
struct i915_guc_client *client)
{
- struct intel_engine_cs *ring;
uint64_t tot = 0;
uint32_t i;
@@ -2461,10 +2460,9 @@ static void i915_guc_client_info(struct seq_file *m,
seq_printf(m, "\tFailed doorbell: %u\n", client->b_fail);
seq_printf(m, "\tLast submission result: %d\n", client->retcode);
- for_each_ring(ring, dev_priv, i) {
- seq_printf(m, "\tSubmissions: %llu %s\n",
- client->submissions[i],
- ring->name);
+ for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
+ seq_printf(m, "\tSubmissions: %llu, engine %d\n",
+ client->submissions[i], i);
tot += client->submissions[i];
}
seq_printf(m, "\tTotal: %llu\n", tot);
@@ -2477,7 +2475,6 @@ static int i915_guc_info(struct seq_file *m, void *data)
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_guc guc;
struct i915_guc_client client = {};
- struct intel_engine_cs *ring;
enum intel_ring_id i;
u64 total = 0;
@@ -2501,9 +2498,9 @@ static int i915_guc_info(struct seq_file *m, void *data)
seq_printf(m, "GuC last action error code: %d\n", guc.action_err);
seq_printf(m, "\nGuC submissions:\n");
- for_each_ring(ring, dev_priv, i) {
- seq_printf(m, "\t%-24s: %10llu, last seqno 0x%08x %9d\n",
- ring->name, guc.submissions[i],
+ for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
+ seq_printf(m, "\tengine %d: %10llu, last seqno 0x%08x %9d\n",
+ i, guc.submissions[i],
guc.last_seqno[i], guc.last_seqno[i]);
total += guc.submissions[i];
}
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 51ae5c1..601e2c8 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -365,6 +365,14 @@ static void guc_init_proc_desc(struct intel_guc *guc,
kunmap_atomic(base);
}
+static const enum intel_ring_id guc_engine_map[GUC_MAX_ENGINES_NUM] = {
+ [GUC_RENDER_ENGINE] = RCS,
+ [GUC_VIDEO_ENGINE] = VCS,
+ [GUC_BLITTER_ENGINE] = BCS,
+ [GUC_VIDEOENHANCE_ENGINE] = VECS,
+ [GUC_VIDEO_ENGINE2] = VCS2
+};
+
/*
* Initialise/clear the context descriptor shared with the GuC firmware.
*
@@ -388,9 +396,10 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
desc.priority = client->priority;
desc.db_id = client->doorbell_id;
- for (i = 0; i < I915_NUM_RINGS; i++) {
+ for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
struct guc_execlist_context *lrc = &desc.lrc[i];
- struct intel_ringbuffer *ringbuf = ctx->engine[i].ringbuf;
+ enum intel_ring_id ring_id = guc_engine_map[i];
+ struct intel_ringbuffer *ringbuf = ctx->engine[ring_id].ringbuf;
struct intel_engine_cs *ring;
struct drm_i915_gem_object *obj;
uint64_t ctx_desc;
@@ -402,7 +411,7 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
* 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.
*/
- obj = ctx->engine[i].state;
+ obj = ctx->engine[ring_id].state;
if (!obj)
break; /* XXX: continue? */
@@ -414,7 +423,7 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
lrc->ring_lcra = i915_gem_obj_ggtt_offset(obj) +
LRC_STATE_PN * PAGE_SIZE;
lrc->context_id = (client->ctx_index << GUC_ELC_CTXID_OFFSET) |
- (ring->id << GUC_ELC_ENGINE_OFFSET);
+ (i << GUC_ELC_ENGINE_OFFSET);
obj = ringbuf->obj;
@@ -423,7 +432,7 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
lrc->ring_next_free_location = lrc->ri
