Re: [Intel-gfx] [PATCH] drm/i915: Don't use enums for hardware engine id

[Michal Wajdeczko]

On Tue, Feb 28, 2017 at 09:53:37PM +, 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 +, Chris Wilson wrote:
> > > On Tue, Feb 28, 2017 at 04:43:02PM +, Chris Wilson wrote:
> > > > On Tue, Feb 28, 2017 at 02:12:09PM +, 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



___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Don't use enums for hardware engine id

[Chris Wilson]

On Tue, Feb 28, 2017 at 07:07:38PM +0100, Michal Wajdeczko wrote:
> On Tue, Feb 28, 2017 at 04:52:34PM +, Chris Wilson wrote:
> > On Tue, Feb 28, 2017 at 04:43:02PM +, Chris Wilson wrote:
> > > On Tue, Feb 28, 2017 at 02:12:09PM +, Michal Wajdeczko wrote:
> > > > +/* Hardware Engine ID definitions */
> > > > +#define RCS_HW 0
> > > > +#define VCS_HW 1
> > > > +#define BCS_HW 2
> > > > +#define VECS_HW3
> > > > +#define VCS2_HW4
> > > 
> > > 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

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Don't use enums for hardware engine id

[Michal Wajdeczko]

On Tue, Feb 28, 2017 at 04:52:34PM +, Chris Wilson wrote:
> On Tue, Feb 28, 2017 at 04:43:02PM +, Chris Wilson wrote:
> > On Tue, Feb 28, 2017 at 02:12:09PM +, 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

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Don't use enums for hardware engine id

[Chris Wilson]

On Tue, Feb 28, 2017 at 04:43:02PM +, Chris Wilson wrote:
> On Tue, Feb 28, 2017 at 02:12:09PM +, Michal Wajdeczko wrote:
> > +/* Hardware Engine ID definitions */
> > +#define RCS_HW 0
> > +#define VCS_HW 1
> > +#define BCS_HW 2
> > +#define VECS_HW3
> > +#define VCS2_HW4
> 
> 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

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Don't use enums for hardware engine id

[Chris Wilson]

On Tue, Feb 28, 2017 at 02:12:09PM +, 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 
> Cc: Daniele Ceraolo Spurio 
> ---
>  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

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Don't use enums for hardware engine id

[Michal Wajdeczko]

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 
Cc: Daniele Ceraolo Spurio 
---
 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_HW3
+#define VCS2_HW4
+
+
 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;
-- 
2.7.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx