Re: [PATCH] drm/i915/guc: Decouple GuC engine id from ring id

Yu Dai
2016-01-23T18:55:09Z

On 01/23/2016 10:25 AM, Chris Wilson wrote:
> On Fri, Jan 22, 2016 at 03:06:28PM -0800, yu.dai@intel.com wrote:
> > From: Alex Dai <yu.dai@intel.com>
> >
> > Previously GuC uses ring id as engine id because of same definition.
> > But this is not true since this commit:
> >
> > commit de1add360522c876c25ef2bbbbab1c94bdb509ab
> > Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Date: Fri Jan 15 15:12:50 2016 +0000
> >
> > 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 <yu.dai@intel.com>
> >
> > 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