| Message ID | 20170228141209.117636-1-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 28, 2017 at 02:12:09PM +0000, Michal Wajdeczko wrote: > Generally we are using macros for any hardware identifiers as these > may change between Gens. Do the same with hardware engine ids. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > --- > drivers/gpu/drm/i915/intel_engine_cs.c | 2 +- > drivers/gpu/drm/i915/intel_ringbuffer.h | 41 ++++++++++++++++++++------------- > 2 files changed, 26 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index 4db2f23..8df53ae 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -29,7 +29,7 @@ > static const struct engine_info { > const char *name; > unsigned exec_id; > - enum intel_engine_hw_id hw_id; > + unsigned hw_id; > u32 mmio_base; > unsigned irq_shift; > int (*init_legacy)(struct intel_engine_cs *engine); > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index 3dd6eee..9cc7863 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -186,26 +186,35 @@ struct i915_ctx_workarounds { > struct drm_i915_gem_request; > struct intel_render_state; > > + > +/* > + * Engine IDs definitions. > + * Keep instances of the same type engine together. > + */ > +enum intel_engine_id { > + RCS = 0, > + BCS, > + VCS, > + VCS2, > +#define _VCS(n) (VCS + (n)) > + VECS > +}; > + > +/* Hardware Engine ID definitions */ > +#define RCS_HW 0 > +#define VCS_HW 1 > +#define BCS_HW 2 > +#define VECS_HW 3 > +#define VCS2_HW 4 So don't put them in the header if they may have inconsistent meanings. It only a field which we supply to hardware and can simply be defined in intel_engine_cs.c and treated as an opaque field elsewhere. We will keep using our own classification (enum engine_id and whatnot) to refer to the engines in the driver. -Chris
On Tue, Feb 28, 2017 at 04:43:02PM +0000, Chris Wilson wrote: > On Tue, Feb 28, 2017 at 02:12:09PM +0000, Michal Wajdeczko wrote: > > +/* Hardware Engine ID definitions */ > > +#define RCS_HW 0 > > +#define VCS_HW 1 > > +#define BCS_HW 2 > > +#define VECS_HW 3 > > +#define VCS2_HW 4 > > So don't put them in the header if they may have inconsistent meanings. Or if you do want to keep them in a header, either i915_reg.h or intel_engine_reg.h as somewhere out of the way, and clear that they are not meant for the rest of the bookkeeping in intel_ringbuffer.h. -Chris
On Tue, Feb 28, 2017 at 04:52:34PM +0000, Chris Wilson wrote: > On Tue, Feb 28, 2017 at 04:43:02PM +0000, Chris Wilson wrote: > > On Tue, Feb 28, 2017 at 02:12:09PM +0000, Michal Wajdeczko wrote: > > > +/* Hardware Engine ID definitions */ > > > +#define RCS_HW 0 > > > +#define VCS_HW 1 > > > +#define BCS_HW 2 > > > +#define VECS_HW 3 > > > +#define VCS2_HW 4 > > > > So don't put them in the header if they may have inconsistent meanings. > > Or if you do want to keep them in a header, either i915_reg.h or > intel_engine_reg.h as somewhere out of the way, and clear that they are > not meant for the rest of the bookkeeping in intel_ringbuffer.h. I can't find nice spot for these engine IDs in the i915_reg.h Can I just move these definitions to the top of this header? There are already some comments/defs that refer to the Bspec, so it should be clear that they are not the same as enums from intel_engine_id. -Michal
On Tue, Feb 28, 2017 at 07:07:38PM +0100, Michal Wajdeczko wrote: > On Tue, Feb 28, 2017 at 04:52:34PM +0000, Chris Wilson wrote: > > On Tue, Feb 28, 2017 at 04:43:02PM +0000, Chris Wilson wrote: > > > On Tue, Feb 28, 2017 at 02:12:09PM +0000, Michal Wajdeczko wrote: > > > > +/* Hardware Engine ID definitions */ > > > > +#define RCS_HW 0 > > > > +#define VCS_HW 1 > > > > +#define BCS_HW 2 > > > > +#define VECS_HW 3 > > > > +#define VCS2_HW 4 > > > > > > So don't put them in the header if they may have inconsistent meanings. > > > > Or if you do want to keep them in a header, either i915_reg.h or > > intel_engine_reg.h as somewhere out of the way, and clear that they are > > not meant for the rest of the bookkeeping in intel_ringbuffer.h. > > I can't find nice spot for these engine IDs in the i915_reg.h > > Can I just move these definitions to the top of this header? I would rather we spend a little effort on splitting our driver API from hw innards. -Chris
On Tue, Feb 28, 2017 at 09:53:37PM +0000, Chris Wilson wrote: > On Tue, Feb 28, 2017 at 07:07:38PM +0100, Michal Wajdeczko wrote: > > On Tue, Feb 28, 2017 at 04:52:34PM +0000, Chris Wilson wrote: > > > On Tue, Feb 28, 2017 at 04:43:02PM +0000, Chris Wilson wrote: > > > > On Tue, Feb 28, 2017 at 02:12:09PM +0000, Michal Wajdeczko wrote: > > > > > +/* Hardware Engine ID definitions */ > > > > > +#define RCS_HW 0 > > > > > +#define VCS_HW 1 > > > > > +#define BCS_HW 2 > > > > > +#define VECS_HW 3 > > > > > +#define VCS2_HW 4 > > > > > > > > So don't put them in the header if they may have inconsistent meanings. > > > > > > Or if you do want to keep them in a header, either i915_reg.h or > > > intel_engine_reg.h as somewhere out of the way, and clear that they are > > > not meant for the rest of the bookkeeping in intel_ringbuffer.h. > > > > I can't find nice spot for these engine IDs in the i915_reg.h > > > > Can I just move these definitions to the top of this header? > > I would rather we spend a little effort on splitting our driver API from > hw innards. Sounds reasonable. As it looks that engine->hw_id is mostly used in code related to semaphores, I'll move these engine definitions to i915_reg.h near MI_SEMAPHORE_SIGNAL instruction. In case of guc_id, it looks that these engine ids are already defined in intel_guc_fwif.h (see GUC_RENDER_ENGINE..GUC_VIDEO_ENGINE2) For now they are the same, but who knows what the future brings ;) -Michal
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index 4db2f23..8df53ae 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -29,7 +29,7 @@ static const struct engine_info { const char *name; unsigned exec_id; - enum intel_engine_hw_id hw_id; + unsigned hw_id; u32 mmio_base; unsigned irq_shift; int (*init_legacy)(struct intel_engine_cs *engine); diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 3dd6eee..9cc7863 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -186,26 +186,35 @@ struct i915_ctx_workarounds { struct drm_i915_gem_request; struct intel_render_state; + +/* + * Engine IDs definitions. + * Keep instances of the same type engine together. + */ +enum intel_engine_id { + RCS = 0, + BCS, + VCS, + VCS2, +#define _VCS(n) (VCS + (n)) + VECS +}; + +/* Hardware Engine ID definitions */ +#define RCS_HW 0 +#define VCS_HW 1 +#define BCS_HW 2 +#define VECS_HW 3 +#define VCS2_HW 4 + + struct intel_engine_cs { struct drm_i915_private *i915; const char *name; - enum intel_engine_id { - RCS = 0, - BCS, - VCS, - VCS2, /* Keep instances of the same type engine together. */ - VECS - } id; -#define _VCS(n) (VCS + (n)) + enum intel_engine_id id; unsigned int exec_id; - enum intel_engine_hw_id { - RCS_HW = 0, - VCS_HW, - BCS_HW, - VECS_HW, - VCS2_HW - } hw_id; - enum intel_engine_hw_id guc_id; /* XXX same as hw_id? */ + unsigned int hw_id; + unsigned int guc_id; u32 mmio_base; unsigned int irq_shift; struct intel_ring *buffer;
Generally we are using macros for any hardware identifiers as these may change between Gens. Do the same with hardware engine ids. Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> --- drivers/gpu/drm/i915/intel_engine_cs.c | 2 +- drivers/gpu/drm/i915/intel_ringbuffer.h | 41 ++++++++++++++++++++------------- 2 files changed, 26 insertions(+), 17 deletions(-)