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

[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 <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
+
+
 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

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

[Chris Wilson]

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

-- 
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: [PATCH] drm/i915: Don't use enums for hardware engine id

[Chris Wilson]

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

-- 
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: [PATCH] drm/i915: Don't use enums for hardware engine id

[Michal Wajdeczko]

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

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

✗ Fi.CI.BAT: failure for drm/i915: Don't use enums for hardware engine id

[Patchwork]

== Series Details ==

Series: drm/i915: Don't use enums for hardware engine id
URL   : https://patchwork.freedesktop.org/series/20396/
State : failure

== Summary ==

Series 20396v1 drm/i915: Don't use enums for hardware engine id
https://patchwork.freedesktop.org/api/1.0/series/20396/revisions/1/mbox/

Test drv_module_reload:
        Subgroup basic-reload:
                pass       -> DMESG-WARN (fi-byt-j1900)
Test gem_exec_fence:
        Subgroup await-hang-default:
                pass       -> DMESG-FAIL (fi-byt-j1900)
        Subgroup nb-await-default:
                pass       -> SKIP       (fi-byt-j1900)
Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-cmd:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-batch-kernel-default-uc:
                pass       -> SKIP       (fi-byt-j1900) fdo#100004
        Subgroup basic-batch-kernel-default-wb:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-uc-pro-default:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-uc-prw-default:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-uc-ro-default:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-uc-rw-default:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-uc-set-default:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-wb-pro-default:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-wb-prw-default:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-wb-ro-before-default:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-wb-ro-default:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-wb-rw-before-default:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-wb-rw-default:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-wb-set-default:
                pass       -> SKIP       (fi-byt-j1900)
Test gem_exec_gttfill:
        Subgroup basic:
                pass       -> SKIP       (fi-byt-j1900)
Test gem_exec_nop:
        Subgroup basic-parallel:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-series:
                pass       -> SKIP       (fi-byt-j1900)
Test gem_exec_parallel:
        Subgroup basic:
                pass       -> SKIP       (fi-byt-j1900)
Test gem_exec_parse:
        Subgroup basic-allowed:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-rejected:
                pass       -> SKIP       (fi-byt-j1900)
Test gem_exec_reloc:
        Subgroup basic-cpu:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-cpu-active:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-cpu-gtt:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-cpu-gtt-active:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-cpu-gtt-noreloc:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-cpu-noreloc:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-cpu-read:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-cpu-read-active:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-cpu-read-noreloc:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-gtt:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-gtt-active:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-gtt-cpu:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-gtt-cpu-active:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-gtt-cpu-noreloc:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-gtt-noreloc:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-gtt-read:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-gtt-read-active:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-gtt-read-noreloc:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-softpin:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-write-cpu:
                pass       -> SKIP       (fi-byt-j1900)
        Subgroup basic-write-cpu-active:
WARNING: Long output truncated

5d37006b578e38562382215e8782cfced9c992ce drm-tip: 2017y-02m-28d-16h-27m-13s UTC integration manifest
d0ed15b drm/i915: Don't use enums for hardware engine id

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4005/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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 +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

-- 
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: [PATCH] drm/i915: Don't use enums for hardware engine id

[Michal Wajdeczko]

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



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