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

[Yu Dai <yu.dai@intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx