Execlists v5

From: Oscar Mateo <[email protected]>

In this patch:

commit 78382593e921c88371abd019aca8978db3248a8f

Author: Oscar Mateo <[email protected]>
Date:   Thu Jul 3 16:28:05 2014 +0100

drm/i915: Extract the actual workload submission mechanism from execbuffer

So that we isolate the legacy ringbuffer submission mechanism, which becomes a good candidate to be abstracted away. This is prep-work for Execlists (which will its own workload submission mechanism).

No functional changes.

I changed the order in which the args checking is done. I don't know why I did (brain fade?) but itś not right. I haven't seen any ill effect from this, but the Execlists version of this function will have problems if the order is not correct.

Signed-off-by Oscar Mateo <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   86 ++++++++++++++--------------
 1 file changed, 43 insertions(+), 43 deletions(-)

drivers/gpu/drm/i915/i915_gem_execbuffer.c +43 -43

index 60998fc..c5115957 100644
@@ -1042,6 +1042,43 @@ legacy_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
 	u32 instp_mask;
 	int i, ret = 0;
 
+	instp_mode = args->flags & I915_EXEC_CONSTANTS_MASK;
+	instp_mask = I915_EXEC_CONSTANTS_MASK;
+	switch (instp_mode) {
+	case I915_EXEC_CONSTANTS_REL_GENERAL:
+	case I915_EXEC_CONSTANTS_ABSOLUTE:
+	case I915_EXEC_CONSTANTS_REL_SURFACE:
+		if (instp_mode != 0 && ring != &dev_priv->ring[RCS]) {
+			DRM_DEBUG("non-0 rel constants mode on non-RCS\n");
+			ret = -EINVAL;
+			goto error;
+		}
+
+		if (instp_mode != dev_priv->relative_constants_mode) {
+			if (INTEL_INFO(dev)->gen < 4) {
+				DRM_DEBUG("no rel constants on pre-gen4\n");
+				ret = -EINVAL;
+				goto error;
+			}
+
+			if (INTEL_INFO(dev)->gen > 5 &&
+			    instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
+				DRM_DEBUG("rel surface constants mode invalid on gen5+\n");
+				ret = -EINVAL;
+				goto error;
+			}
+
+			/* The HW changed the meaning on this bit on gen6 */
+			if (INTEL_INFO(dev)->gen >= 6)
+				instp_mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
+		}
+		break;
+	default:
+		DRM_DEBUG("execbuf with unknown constants: %d\n", instp_mode);
+		ret = -EINVAL;
+		goto error;
+	}
+
 	if (args->num_cliprects != 0) {
 		if (ring != &dev_priv->ring[RCS]) {
 			DRM_DEBUG("clip rectangles are only valid with the render ring\n");

@@ -1085,6 +1122,12 @@ legacy_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
 		}
 	}
 
+	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
+		ret = i915_reset_gen7_sol_offsets(dev, ring);
+		if (ret)
+			goto error;
+	}
+
 	ret = i915_gem_execbuffer_move_to_gpu(ring, vmas);
 	if (ret)
 		goto error;

@@ -1093,43 +1136,6 @@ legacy_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
 	if (ret)
 		goto error;
 
-	instp_mode = args->flags & I915_EXEC_CONSTANTS_MASK;
-	instp_mask = I915_EXEC_CONSTANTS_MASK;
-	switch (instp_mode) {
-	case I915_EXEC_CONSTANTS_REL_GENERAL:
-	case I915_EXEC_CONSTANTS_ABSOLUTE:
-	case I915_EXEC_CONSTANTS_REL_SURFACE:
-		if (instp_mode != 0 && ring != &dev_priv->ring[RCS]) {
-			DRM_DEBUG("non-0 rel constants mode on non-RCS\n");
-			ret = -EINVAL;
-			goto error;
-		}
-
-		if (instp_mode != dev_priv->relative_constants_mode) {
-			if (INTEL_INFO(dev)->gen < 4) {
-				DRM_DEBUG("no rel constants on pre-gen4\n");
-				ret = -EINVAL;
-				goto error;
-			}
-
-			if (INTEL_INFO(dev)->gen > 5 &&
-			    instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
-				DRM_DEBUG("rel surface constants mode invalid on gen5+\n");
-				ret = -EINVAL;
-				goto error;
-			}
-
-			/* The HW changed the meaning on this bit on gen6 */
-			if (INTEL_INFO(dev)->gen >= 6)
-				instp_mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
-		}
-		break;
-	default:
-		DRM_DEBUG("execbuf with unknown constants: %d\n", instp_mode);
-		ret = -EINVAL;
-		goto error;
-	}
-
 	if (ring == &dev_priv->ring[RCS] &&
 			instp_mode != dev_priv->relative_constants_mode) {
 		ret = intel_ring_begin(ring, 4);

@@ -1145,12 +1151,6 @@ legacy_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
 		dev_priv->relative_constants_mode = instp_mode;
 	}
 
-	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
-		ret = i915_reset_gen7_sol_offsets(dev, ring);
-		if (ret)
-			goto error;
-	}
-
 	exec_len = args->batch_len;
 	if (cliprects) {
 		for (i = 0; i < args->num_cliprects; i++) {

Signature · 6 lines

1.7.9.5

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 20 lines · Thomas Daniel

From: Oscar Mateo <[email protected]>

In this patch:

commit 78382593e921c88371abd019aca8978db3248a8f

Author: Oscar Mateo <[email protected]>
Date:   Thu Jul 3 16:28:05 2014 +0100

drm/i915: Extract the actual workload submission mechanism from execbuffer

So that we isolate the legacy ringbuffer submission mechanism, which becomes a good candidate to be abstracted away. This is prep-work for Execlists (which will its own workload submission mechanism).

No functional changes.

I changed the order in which the args checking is done. I don't know why I did (brain fade?) but itś not right. I haven't seen any ill effect from this, but the Execlists version of this function will have problems if the order is not correct.

Quoted 1 line

Signed-off-by Oscar Mateo <[email protected]>

I don't think this matters - the point of no return for legacy execbuf is the call to ring->dispatch. After that nothing may fail any more. But as long as we track state correctly (e.g. if we've switched the context already) we'll be fine.

From: Oscar Mateo <[email protected]>

GEN8 brings an expansion of the HW contexts: "Logical Ring Contexts". These expanded contexts enable a number of new abilities, especially "Execlists".

The macro is defined to off until we have things in place to hope to work.

v2: Rename "advanced contexts" to the more correct "logical ring contexts".

v3: Add a module parameter to enable execlists. Execlist are relatively new, and so it'd be wise to be able to switch back to ring submission to debug subtle problems that will inevitably arise.

v4: Add an intel_enable_execlists function.

v5: Sanitize early, as suggested by Daniel. Remove lrc_enabled.

Signed-off-by Ben Widawsky <[email protected]> (v1)

Signed-off-by Damien Lespiau <[email protected]> (v3)

Signed-off-by Oscar Mateo <[email protected]> (v2, v4 & v5)

---
 drivers/gpu/drm/i915/i915_drv.h    |    2 ++
 drivers/gpu/drm/i915/i915_gem.c    |    3 +++
 drivers/gpu/drm/i915/i915_params.c |    6 ++++++
 drivers/gpu/drm/i915/intel_lrc.c   |   11 +++++++++++
 drivers/gpu/drm/i915/intel_lrc.h   |    3 +++
 5 files changed, 25 insertions(+)

drivers/gpu/drm/i915/i915_drv.h +2 -0

index 54c2bd9..a793d6d 100644
@@ -2037,6 +2037,7 @@ struct drm_i915_cmd_table {
 #define I915_NEED_GFX_HWS(dev)	(INTEL_INFO(dev)->need_gfx_hws)
 
 #define HAS_HW_CONTEXTS(dev)	(INTEL_INFO(dev)->gen >= 6)
+#define HAS_LOGICAL_RING_CONTEXTS(dev)	0
 #define HAS_ALIASING_PPGTT(dev)	(INTEL_INFO(dev)->gen >= 6)
 #define HAS_PPGTT(dev)		(INTEL_INFO(dev)->gen >= 7 && !IS_GEN8(dev))
 #define USES_PPGTT(dev)		intel_enable_ppgtt(dev, false)

@@ -2122,6 +2123,7 @@ struct i915_params {
 	int enable_rc6;
 	int enable_fbc;
 	int enable_ppgtt;
+	int enable_execlists;
 	int enable_psr;
 	unsigned int preliminary_hw_support;
 	int disable_power_well;

drivers/gpu/drm/i915/i915_gem.c +0 -0

index e5d4d73..d8bf4fa 100644

drivers/gpu/drm/i915/i915_gem.c +1 -0

+
 	mutex_lock(&dev->struct_mutex);
 
 	if (IS_VALLEYVIEW(dev)) {

drivers/gpu/drm/i915/i915_params.c +6 -0

index bbdee21..7f0fb72 100644
@@ -35,6 +35,7 @@ struct i915_params i915 __read_mostly = {
 	.vbt_sdvo_panel_type = -1,
 	.enable_rc6 = -1,
 	.enable_fbc = -1,
+	.enable_execlists = -1,
 	.enable_hangcheck = true,
 	.enable_ppgtt = -1,
 	.enable_psr = 1,

@@ -117,6 +118,11 @@ MODULE_PARM_DESC(enable_ppgtt,
 	"Override PPGTT usage. "
 	"(-1=auto [default], 0=disabled, 1=aliasing, 2=full)");
 
+module_param_named(enable_execlists, i915.enable_execlists, int, 0400);
+MODULE_PARM_DESC(enable_execlists,
+	"Override execlists usage. "
+	"(-1=auto [default], 0=disabled, 1=enabled)");
+
 module_param_named(enable_psr, i915.enable_psr, int, 0600);
 MODULE_PARM_DESC(enable_psr, "Enable PSR (default: true)");
 

drivers/gpu/drm/i915/intel_lrc.c +11 -0

index 49bb6fc..21f7f1c 100644
@@ -40,3 +40,14 @@
 #include <drm/drmP.h>
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
+
+int intel_sanitize_enable_execlists(struct drm_device *dev, int enable_execlists)
+{
+	if (enable_execlists == 0)
+		return 0;
+
+	if (HAS_LOGICAL_RING_CONTEXTS(dev) && USES_PPGTT(dev))
+		return 1;
+
+	return 0;
+}

drivers/gpu/drm/i915/intel_lrc.h +3 -0

index f6830a4..75ee9c3 100644
@@ -24,4 +24,7 @@
 #ifndef _INTEL_LRC_H_
 #define _INTEL_LRC_H_
 
+/* Execlists */
+int intel_sanitize_enable_execlists(struct drm_device *dev, int enable_execlists);
+
 #endif /* _INTEL_LRC_H_ */

Signature · 6 lines

1.7.9.5

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

From: Oscar Mateo <[email protected]>

For the moment this is just a placeholder, but it shows one of the main differences between the good ol' HW contexts and the shiny new Logical Ring Contexts: LR contexts allocate and free their own backing objects. Another difference is that the allocation is deferred (as the create function name suggests), but that does not happen in this patch yet, because for the moment we are only dealing with the default context.

Early in the series we had our own gen8_gem_context_init/fini functions, but the truth is they now look almost the same as the legacy hw context init/fini functions. We can always split them later if this ceases to be the case.

Also, we do not fall back to legacy ringbuffers when logical ring context initialization fails (not very likely to happen and, even if it does, hw contexts would probably fail as well).

v2: Daniel says "explain, do not showcase".

Signed-off-by Oscar Mateo <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem_context.c |   29 +++++++++++++++++++++++------
 drivers/gpu/drm/i915/intel_lrc.c        |   15 +++++++++++++++
 drivers/gpu/drm/i915/intel_lrc.h        |    5 +++++
 3 files changed, 43 insertions(+), 6 deletions(-)

drivers/gpu/drm/i915/i915_gem_context.c +23 -6

index de72a28..718150e 100644
@@ -182,7 +182,10 @@ void i915_gem_context_free(struct kref *ctx_ref)
 						   typeof(*ctx), ref);
 	struct i915_hw_ppgtt *ppgtt = NULL;
 
-	if (ctx->legacy_hw_ctx.rcs_state) {
+	if (i915.enable_execlists) {
+		ppgtt = ctx_to_ppgtt(ctx);
+		intel_lr_context_free(ctx);
+	} else if (ctx->legacy_hw_ctx.rcs_state) {
 		/* We refcount even the aliasing PPGTT to keep the code symmetric */
 		if (USES_PPGTT(ctx->legacy_hw_ctx.rcs_state->base.dev))
 			ppgtt = ctx_to_ppgtt(ctx);

@@ -419,7 +422,11 @@ int i915_gem_context_init(struct drm_device *dev)
 	if (WARN_ON(dev_priv->ring[RCS].default_context))
 		return 0;
 
-	if (HAS_HW_CONTEXTS(dev)) {
+	if (i915.enable_execlists) {
+		/* NB: intentionally left blank. We will allocate our own
+		 * backing objects as we need them, thank you very much */
+		dev_priv->hw_context_size = 0;
+	} else if (HAS_HW_CONTEXTS(dev)) {
 		dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
 		if (dev_priv->hw_context_size > (1<<20)) {
 			DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size %d\n",

@@ -435,11 +442,20 @@ int i915_gem_context_init(struct drm_device *dev)
 		return PTR_ERR(ctx);
 	}
 
-	/* NB: RCS will hold a ref for all rings */
-	for (i = 0; i < I915_NUM_RINGS; i++)
-		dev_priv->ring[i].default_context = ctx;
+	for (i = 0; i < I915_NUM_RINGS; i++) {
+		struct intel_engine_cs *ring = &dev_priv->ring[i];
+
+		/* NB: RCS will hold a ref for all rings */
+		ring->default_context = ctx;
+
+		/* FIXME: we really only want to do this for initialized rings */
+		if (i915.enable_execlists)
+			intel_lr_context_deferred_create(ctx, ring);
+	}
 
-	DRM_DEBUG_DRIVER("%s context support initialized\n", dev_priv->hw_context_size ? "HW" : "fake");
+	DRM_DEBUG_DRIVER("%s context support initialized\n",
+			i915.enable_execlists ? "LR" :
+			dev_priv->hw_context_size ? "HW" : "fake");
 	return 0;
 }
 

@@ -781,6 +797,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
 	struct intel_context *ctx;
 	int ret;
 
+	/* FIXME: allow user-created LR contexts as well */
 	if (!hw_context_enabled(dev))
 		return -ENODEV;
 

drivers/gpu/drm/i915/intel_lrc.c +0 -0

index 21f7f1c..8cc6b55 100644
@@ -51,3 +51,18 @@ int intel_sanitize_enable_execlists(struct drm_device *dev, int enable_execlists
 
 	return 0;
 }

drivers/gpu/drm/i915/intel_lrc.c +5 -0

+
+	/* TODO */
+
+	return 0;
+}

drivers/gpu/drm/i915/intel_lrc.h +5 -0

index 75ee9c3..3b93572 100644
@@ -24,6 +24,11 @@
 #ifndef _INTEL_LRC_H_
 #define _INTEL_LRC_H_
 
+/* Logical Ring Contexts */
+void intel_lr_context_free(struct intel_context *ctx);
+int intel_lr_context_deferred_create(struct intel_context *ctx,
+				     struct intel_engine_cs *ring);
+
 /* Execlists */
 int intel_sanitize_enable_execlists(struct drm_device *dev, int enable_execlists);
 

Signature · 6 lines

1.7.9.5

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Depending upon one module option to be sanitized (through USES_PPGTT) for the other is a bit too fragile for my taste. At least WARN about this.

Cc Ben Widawsky <[email protected]>

Cc Damien Lespiau <[email protected]>

Cc Oscar Mateo <[email protected]>

Signed-off-by Daniel Vetter <[email protected]>

---
 drivers/gpu/drm/i915/intel_lrc.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/gpu/drm/i915/intel_lrc.c +2 -0

index 21f7f1cce86e..44721292eb77 100644
@@ -43,6 +43,8 @@
 
 int intel_sanitize_enable_execlists(struct drm_device *dev, int enable_execlists)
 {
+	WARN_ON(i915.enable_ppgtt == -1);
+
 	if (enable_execlists == 0)
 		return 0;
 

Signature · 1 line

2.0.1

From: Oscar Mateo <[email protected]>

Any given ringbuffer is unequivocally tied to one context and one engine. By setting the appropriate pointers to them, the ringbuffer struct holds all the infromation you might need to submit a workload for processing, Execlists style.

Signed-off-by Oscar Mateo <[email protected]>

---
 drivers/gpu/drm/i915/intel_lrc.c        |    2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.c |    2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.h |    3 +++
 3 files changed, 7 insertions(+)

drivers/gpu/drm/i915/intel_lrc.c +2 -0

index 0a12b8c..2eb7db6 100644
@@ -132,6 +132,8 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
 		return ret;
 	}
 
+	ringbuf->ring = ring;
+	ringbuf->ctx = ctx;
 	ringbuf->size = 32 * PAGE_SIZE;
 	ringbuf->effective_size = ringbuf->size;
 	ringbuf->head = 0;

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

index 01e9840..279dda4 100644

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

 	memset(ring->semaphore.sync_seqno, 0, sizeof(ring->semaphore.sync_seqno));
 
 	init_waitqueue_head(&ring->irq_queue);

drivers/gpu/drm/i915/intel_ringbuffer.h +3 -0

index 053d004..be40788 100644
@@ -88,6 +88,9 @@ struct intel_ringbuffer {
 	struct drm_i915_gem_object *obj;
 	void __iomem *virtual_start;
 
+	struct intel_engine_cs *ring;
+	struct intel_context *ctx;
+
 	u32 head;
 	u32 tail;
 	int space;

Signature · 6 lines

1.7.9.5

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 32 lines · Thomas Daniel

From: Oscar Mateo <[email protected]>

Any given ringbuffer is unequivocally tied to one context and one engine. By setting the appropriate pointers to them, the ringbuffer struct holds all the infromation you might need to submit a workload for processing, Execlists style.

Signed-off-by Oscar Mateo <[email protected]>

---
 drivers/gpu/drm/i915/intel_lrc.c        |    2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.c |    2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.h |    3 +++
 3 files changed, 7 insertions(+)

drivers/gpu/drm/i915/intel_lrc.c +2 -0

index 0a12b8c..2eb7db6 100644
@@ -132,6 +132,8 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
 		return ret;
 	}
 
+	ringbuf->ring = ring;
+	ringbuf->ctx = ctx;
 	ringbuf->size = 32 * PAGE_SIZE;
 	ringbuf->effective_size = ringbuf->size;
 	ringbuf->head = 0;

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

index 01e9840..279dda4 100644

drivers/gpu/drm/i915/intel_ringbuffer.c static int intel_init_ring_buffer(struct drm_device *dev, · line 1574

Quoted 5 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +2 -0

@@ -1570,6 +1570,8 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 	INIT_LIST_HEAD(&ring->active_list);
 	INIT_LIST_HEAD(&ring->request_list);
 	ringbuf->size = 32 * PAGE_SIZE;
+	ringbuf->ring = ring;
+	ringbuf->ctx = ring->default_context;

That doesn't make a terribly lot of sense tbh. I fear it's one of these slight confusions which will take tons of patches to clean up. Why exactly do we need the ring->ctx pointer?

If we only need this for lrc I want to name it accordingly, to make sure legacy code doesn't grow stupid ideas. And also we should only initialize this in the lrc ctx init then.

All patches up to this one merged.

-Daniel

Quoted 24 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

 	memset(ring->semaphore.sync_seqno, 0, sizeof(ring->semaphore.sync_seqno));
 
 	init_waitqueue_head(&ring->irq_queue);

drivers/gpu/drm/i915/intel_ringbuffer.h +3 -0

index 053d004..be40788 100644
@@ -88,6 +88,9 @@ struct intel_ringbuffer {
 	struct drm_i915_gem_object *obj;
 	void __iomem *virtual_start;
 
+	struct intel_engine_cs *ring;
+	struct intel_context *ctx;
+
 	u32 head;
 	u32 tail;
 	int space;

Signature · 6 lines

1.7.9.5

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Monday, August 11, 2014 3:21 PM
To: Daniel, Thomas
Cc: [email protected]
Subject: Re: [Intel-gfx] [PATCH 08/43] drm/i915/bdw: Add a context and an engine pointers to the ringbuffer

Quoted 15 lines · Daniel Vetter

Quoted 14 lines · Thomas Daniel

From: Oscar Mateo <[email protected]>

Any given ringbuffer is unequivocally tied to one context and one engine. By setting the appropriate pointers to them, the ringbuffer struct holds all the infromation you might need to submit a workload for processing, Execlists style.

Signed-off-by Oscar Mateo <[email protected]>

---
 drivers/gpu/drm/i915/intel_lrc.c        |    2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.c |    2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.h |    3 +++
 3 files changed, 7 insertions(+)

Quoted 6 lines

drivers/gpu/drm/i915/intel_lrc.c int intel_lr_context_deferred_create(struct

Quoted 6 lines

drivers/gpu/drm/i915/intel_lrc.c +0 -0

index 0a12b8c..2eb7db6 100644
@@ -132,6 +132,8 @@ int intel_lr_context_deferred_create(struct

intel_context *ctx,

Quoted 14 lines

drivers/gpu/drm/i915/intel_ringbuffer.c static int intel_init_ring_buffer(struct

Quoted 14 lines

drivers/gpu/drm/i915/intel_lrc.c +2 -0

 		return ret;
 	}
 
+	ringbuf->ring = ring;
+	ringbuf->ctx = ctx;
 	ringbuf->size = 32 * PAGE_SIZE;
 	ringbuf->effective_size = ringbuf->size;
 	ringbuf->head = 0;

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

index 01e9840..279dda4 100644
@@ -1570,6 +1570,8 @@ static int intel_init_ring_buffer(struct

drm_device *dev,

Quoted 14 lines

drivers/gpu/drm/i915/intel_ringbuffer.c static int intel_init_ring_buffer(struct drm_device *dev, · line 1574

Quoted 5 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +2 -0

 	INIT_LIST_HEAD(&ring->active_list);
 	INIT_LIST_HEAD(&ring->request_list);
 	ringbuf->size = 32 * PAGE_SIZE;
+	ringbuf->ring = ring;
+	ringbuf->ctx = ring->default_context;

That doesn't make a terribly lot of sense tbh. I fear it's one of these slight confusions which will take tons of patches to clean up. Why exactly do we need the ring->ctx pointer?

If we only need this for lrc I want to name it accordingly, to make sure legacy code doesn't grow stupid ideas. And also we should only initialize this in the lrc ctx init then.

Quoted 1 line

All patches up to this one merged.

Ok, I've discussed this quickly with Damien on irc. We decided to cut away the ring->ctx part of this patch for now to be able to move on. -Daniel

Quoted 25 lines

-Daniel

Quoted 22 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

 	memset(ring->semaphore.sync_seqno, 0,
 sizeof(ring->semaphore.sync_seqno));
 

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

 	init_waitqueue_head(&ring->irq_queue);

drivers/gpu/drm/i915/intel_ringbuffer.h +3 -1

index 053d004..be40788 100644
@@ -88,6 +88,9 @@ struct intel_ringbuffer {
 	struct drm_i915_gem_object *obj;
 	void __iomem *virtual_start;
 
+	struct intel_engine_cs *ring;
+	struct intel_context *ctx;
+
 	u32 head;
 	u32 tail;
 	int space;
--

1.7.9.5

Quoted 9 lines

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Quoted 63 lines

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Monday, August 11, 2014 3:21 PM
To: Daniel, Thomas
Cc: [email protected]
Subject: Re: [Intel-gfx] [PATCH 08/43] drm/i915/bdw: Add a context and an engine pointers to the ringbuffer

Quoted 15 lines · Daniel Vetter

Quoted 14 lines · Thomas Daniel

From: Oscar Mateo <[email protected]>

Any given ringbuffer is unequivocally tied to one context and one engine. By setting the appropriate pointers to them, the ringbuffer struct holds all the infromation you might need to submit a workload for processing, Execlists style.

Signed-off-by Oscar Mateo <[email protected]>

---
 drivers/gpu/drm/i915/intel_lrc.c        |    2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.c |    2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.h |    3 +++
 3 files changed, 7 insertions(+)

Quoted 6 lines

drivers/gpu/drm/i915/intel_lrc.c int intel_lr_context_deferred_create(struct

Quoted 6 lines

drivers/gpu/drm/i915/intel_lrc.c +0 -0

index 0a12b8c..2eb7db6 100644
@@ -132,6 +132,8 @@ int intel_lr_context_deferred_create(struct

intel_context *ctx,

Quoted 14 lines

drivers/gpu/drm/i915/intel_ringbuffer.c static int intel_init_ring_buffer(struct

Quoted 14 lines

drivers/gpu/drm/i915/intel_lrc.c +2 -0

 		return ret;
 	}
 
+	ringbuf->ring = ring;
+	ringbuf->ctx = ctx;
 	ringbuf->size = 32 * PAGE_SIZE;
 	ringbuf->effective_size = ringbuf->size;
 	ringbuf->head = 0;

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

index 01e9840..279dda4 100644
@@ -1570,6 +1570,8 @@ static int intel_init_ring_buffer(struct

drm_device *dev,

Quoted 14 lines

drivers/gpu/drm/i915/intel_ringbuffer.c static int intel_init_ring_buffer(struct drm_device *dev, · line 1574

Quoted 5 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +2 -0

 	INIT_LIST_HEAD(&ring->active_list);
 	INIT_LIST_HEAD(&ring->request_list);
 	ringbuf->size = 32 * PAGE_SIZE;
+	ringbuf->ring = ring;
+	ringbuf->ctx = ring->default_context;

That doesn't make a terribly lot of sense tbh. I fear it's one of these slight confusions which will take tons of patches to clean up. Why exactly do we need the ring->ctx pointer?

If we only need this for lrc I want to name it accordingly, to make sure legacy code doesn't grow stupid ideas. And also we should only initialize this in the lrc ctx init then.

Quoted 5 lines

Quoted 1 line

All patches up to this one merged.

Ok, I've discussed this quickly with Damien on irc. We decided to cut away the ring->ctx part of this patch for now to be able to move on. -Daniel

As you've seen, removing ringbuffer->ctx causes serious problems with the plumbing later on. This can be renamed (perhaps to lrc) and removed from legacy init.

Each ring buffer belongs to a specific context - it makes sense to me to keep this information within the ringbuffer structure so that we don't have to pass the context pointer around everywhere.

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Wednesday, August 13, 2014 4:16 PM
To: Daniel, Thomas
Cc: Daniel Vetter; [email protected]
Subject: Re: [Intel-gfx] [PATCH 08/43] drm/i915/bdw: Add a context and an engine pointers to the ringbuffer

Quoted 11 lines · Thomas

Quoted 11 lines

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Monday, August 11, 2014 3:21 PM
To: Daniel, Thomas
Cc: [email protected]
Subject: Re: [Intel-gfx] [PATCH 08/43] drm/i915/bdw: Add a context and an engine pointers to the ringbuffer

Quoted 5 lines

Quoted 5 lines · Daniel Vetter

Quoted 4 lines · Thomas Daniel

Quoted 3 lines

From: Oscar Mateo <[email protected]>

Any given ringbuffer is unequivocally tied to one context and one

engine.

Quoted 52 lines

Quoted 47 lines

Quoted 11 lines

Quoted 11 lines

By setting the appropriate pointers to them, the ringbuffer struct holds all the infromation you might need to submit a workload for processing, Execlists style.

Signed-off-by Oscar Mateo <[email protected]>

---
 drivers/gpu/drm/i915/intel_lrc.c        |    2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.c |    2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.h |    3 +++
 3 files changed, 7 insertions(+)

Quoted 6 lines

drivers/gpu/drm/i915/intel_lrc.c int intel_lr_context_deferred_create(struct

Quoted 6 lines

drivers/gpu/drm/i915/intel_lrc.c +0 -0

index 0a12b8c..2eb7db6 100644
@@ -132,6 +132,8 @@ int intel_lr_context_deferred_create(struct

intel_context *ctx,

Quoted 14 lines

drivers/gpu/drm/i915/intel_ringbuffer.c static int intel_init_ring_buffer(struct

Quoted 14 lines

drivers/gpu/drm/i915/intel_lrc.c +2 -0

 		return ret;
 	}
 
+	ringbuf->ring = ring;
+	ringbuf->ctx = ctx;
 	ringbuf->size = 32 * PAGE_SIZE;
 	ringbuf->effective_size = ringbuf->size;
 	ringbuf->head = 0;

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

index 01e9840..279dda4 100644
@@ -1570,6 +1570,8 @@ static int intel_init_ring_buffer(struct

drm_device *dev,

Quoted 14 lines

drivers/gpu/drm/i915/intel_ringbuffer.c static int intel_init_ring_buffer(struct drm_device *dev, · line 1574

Quoted 5 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +2 -0

 	INIT_LIST_HEAD(&ring->active_list);
 	INIT_LIST_HEAD(&ring->request_list);
 	ringbuf->size = 32 * PAGE_SIZE;
+	ringbuf->ring = ring;
+	ringbuf->ctx = ring->default_context;

That doesn't make a terribly lot of sense tbh. I fear it's one of these slight confusions which will take tons of patches to clean up. Why exactly do we need the ring->ctx pointer?

If we only need this for lrc I want to name it accordingly, to make sure legacy code doesn't grow stupid ideas. And also we should only initialize this in the lrc ctx init then.

Quoted 5 lines

Quoted 1 line

All patches up to this one merged.

Ok, I've discussed this quickly with Damien on irc. We decided to cut away the ring->ctx part of this patch for now to be able to move on. -Daniel

Quoted 7 lines

As you've seen, removing ringbuffer->ctx causes serious problems with the plumbing later on. This can be renamed (perhaps to lrc) and removed from legacy init.

Each ring buffer belongs to a specific context - it makes sense to me to keep this information within the ringbuffer structure so that we don't have to pass the context pointer around everywhere.

I agree that it causes trouble with the follow-up patches, but I'm not sold on this being a terrible good idea. After all for ELSP we don't want to submit a ring, we want to submit the full context. So if the code that's supposed to do the execlist ctx submission only has the pointer to the ring object, the layer looks a bit wrong.

Same was iirc about the add_request part.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

When it comes to the execlist submission (actually as early as the execlist request queueing), the engine and context are indeed used and required. intel_logical_ring_advance_and_submit() is the lrc function analogous to __intel_ring_advance() and I believe the initial creation of intel_lrc.c was actually done by copying intel_ringbuffer.c. This explains why some of the lrc code is perhaps not as it would have been if this had been designed from scratch, and there is room for future improvement. advance_and_submit therefore only gets the ringbuffer struct and uses the context pointer in that struct to get the logical ring context itself. At that point the engine, context and new tail pointer are handed over to the execlist queue backend.

Quoted 11 lines · Thomas

When it comes to the execlist submission (actually as early as the execlist request queueing), the engine and context are indeed used and required. intel_logical_ring_advance_and_submit() is the lrc function analogous to __intel_ring_advance() and I believe the initial creation of intel_lrc.c was actually done by copying intel_ringbuffer.c. This explains why some of the lrc code is perhaps not as it would have been if this had been designed from scratch, and there is room for future improvement. advance_and_submit therefore only gets the ringbuffer struct and uses the context pointer in that struct to get the logical ring context itself. At that point the engine, context and new tail pointer are handed over to the execlist queue backend.

I guess I need to clarify: Does it make sense to move the ELSP respectively the submission to the execlist scheduler queue out of there up a few levels into the execlist cmd submission function? Is it possible or is there some technical reason that I'm overlooking?

I want to know what exactly I'm dealing with here before I sign up for it by merging the patches as-is and asking for a cleanup. I doesn't look bad really, but there's always a good chance that I've overlooked a bigger dragon.

Since you have the patches and worked with them I'm asking you such explorative questions. Ofc I can do this checking myself, but that takes time ... This doesn't mean that you have to implement the changes, just be reasonable confident that it will work out as a cleanup on top.

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Thursday, August 14, 2014 4:37 PM
To: Daniel, Thomas
Cc: Daniel Vetter; [email protected]
Subject: Re: [Intel-gfx] [PATCH 08/43] drm/i915/bdw: Add a context and an engine pointers to the ringbuffer

Quoted 3 lines · Daniel Vetter

Quoted 2 lines · Thomas

When it comes to the execlist submission (actually as early as the execlist request queueing), the engine and context are indeed used and

required.

Quoted 6 lines

Quoted 6 lines

intel_logical_ring_advance_and_submit() is the lrc function analogous to
__intel_ring_advance() and I believe the initial creation of intel_lrc.c was actually done by copying intel_ringbuffer.c. This explains why some of the lrc code is perhaps not as it would have been if this had been designed from scratch, and there is room for future

improvement.

Quoted 9 lines

Quoted 4 lines

advance_and_submit therefore only gets the ringbuffer struct and uses the context pointer in that struct to get the logical ring context itself. At that point the engine, context and new tail pointer are handed over to the execlist queue backend.

I guess I need to clarify: Does it make sense to move the ELSP respectively the submission to the execlist scheduler queue out of there up a few levels into the execlist cmd submission function? Is it possible or is there some technical reason that I'm overlooking?

Quoted 10 lines

I want to know what exactly I'm dealing with here before I sign up for it by merging the patches as-is and asking for a cleanup. I doesn't look bad really, but there's always a good chance that I've overlooked a bigger dragon.

Since you have the patches and worked with them I'm asking you such explorative questions. Ofc I can do this checking myself, but that takes time ... This doesn't mean that you have to implement the changes, just be reasonable confident that it will work out as a cleanup on

top.

To clarify more the context: Currently you're replies sound like "This is what it looks like and I don't really know why nor whether we can change that".

That's not confidence instilling and that makes maintainers reluctant to merge patches for fear of needing to fix things themselves ;-)

-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Quoted 28 lines

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Thursday, August 14, 2014 4:37 PM
To: Daniel, Thomas
Cc: Daniel Vetter; [email protected]
Subject: Re: [Intel-gfx] [PATCH 08/43] drm/i915/bdw: Add a context and an engine pointers to the ringbuffer

Quoted 3 lines · Daniel Vetter

Quoted 2 lines · Thomas

When it comes to the execlist submission (actually as early as the execlist request queueing), the engine and context are indeed used and

required.

Quoted 6 lines

Quoted 6 lines

intel_logical_ring_advance_and_submit() is the lrc function analogous to
__intel_ring_advance() and I believe the initial creation of intel_lrc.c was actually done by copying intel_ringbuffer.c. This explains why some of the lrc code is perhaps not as it would have been if this had been designed from scratch, and there is room for future

improvement.

Quoted 5 lines

Quoted 4 lines

advance_and_submit therefore only gets the ringbuffer struct and uses the context pointer in that struct to get the logical ring context itself. At that point the engine, context and new tail pointer are handed over to the execlist queue backend.

Quoted 4 lines

Quoted 4 lines

I guess I need to clarify: Does it make sense to move the ELSP respectively the submission to the execlist scheduler queue out of there up a few levels into the execlist cmd submission function? Is it possible or is there some technical reason that I'm overlooking?

Yes this would make sense, and we already have a separate emit_request vfunc which is only used in lrc mode so we can for example change the signature to accept a drm_i915_gem_request* directly and take the ctx and engine from there.

Quoted 10 lines

Quoted 9 lines

I want to know what exactly I'm dealing with here before I sign up for it by merging the patches as-is and asking for a cleanup. I doesn't look bad really, but there's always a good chance that I've overlooked a bigger dragon.

Since you have the patches and worked with them I'm asking you such explorative questions. Ofc I can do this checking myself, but that takes time ... This doesn't mean that you have to implement the changes, just be reasonable confident that it will work out as a cleanup on

top.

Understood.

Quoted 3 lines

To clarify more the context: Currently you're replies sound like "This is what it looks like and I don't really know why nor whether we can change that".

You're right, I don't know why it was done this way. But I can see that there is no problem to change it later - it's only a piece of code after all...

Quoted 3 lines

That's not confidence instilling and that makes maintainers reluctant to merge patches for fear of needing to fix things themselves ;-)

If it helps, I can tell you that several guys in our team are working with this code and we have a vested interest in making sure the quality is as high as possible.

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]]
Sent: Thursday, August 14, 2014 5:28 PM
To: Intel Graphics Development
Cc: Mateo Lozano, Oscar; Daniel, Thomas; Daniel Vetter
Subject: [PATCH] drm/i915: Add temporary ring->ctx backpointer

From: Oscar Mateo <[email protected]>

The execlist patches have a bit a convoluted and long history and due to that have the actual submission still misplaced deeply burried in the low-level ringbuffer handling code. This design goes back to the legacy ringbuffer code with its tricky lazy request and simple work submissiion using ring tail writes. For that reason they need a
ring->ctx backpointer.

The goal is to unburry that code and move it up into a level where the full execlist context is available so that we can ditch this backpointer. Until that's done make it really obvious that there's work still to be done.

Cc Oscar Mateo <[email protected]>

Cc Thomas Daniel <[email protected]>

Signed-off-by Daniel Vetter <[email protected]>

--

Thomas, please ack this patch and the general plan we've discussed.

Then I'll start pulling in more patches and I'll do the s/ctx/FIXME_lrc_ctx/ on the fly.

(quoted diff) +0 -2

-Daniel
---
 drivers/gpu/drm/i915/intel_lrc.c        | 2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.h | 7 +++++++
 2 files changed, 9 insertions(+)

drivers/gpu/drm/i915/intel_lrc.c +2 -0

index 6b5f416b5c0d..c2352d1b23fa 100644
@@ -1086,6 +1086,8 @@ int intel_lr_context_deferred_create(struct
 intel_context *ctx,
 	}
 
 	ringbuf->ring = ring;
+	ringbuf->FIXME_lrc_ctx = ctx;
+
 	ringbuf->size = 32 * PAGE_SIZE;
 	ringbuf->effective_size = ringbuf->size;

drivers/gpu/drm/i915/intel_lrc.c +0 -0

 	ringbuf->head = 0;

drivers/gpu/drm/i915/intel_ringbuffer.h +7 -1

index 24437da91f77..26785ca72530 100644
@@ -99,6 +99,13 @@ struct intel_ringbuffer {
 
 	struct intel_engine_cs *ring;
 
+	/*
+	 * FIXME: This backpointer is an artifact of the history of how the
+	 * execlist patches came into being. It will get removed once the basic
+	 * code has landed.
+	 */
+	struct intel_context *FIXME_lrc_ctx;
+
 	u32 head;
 	u32 tail;
 	int space;
--

2.0.1

From: Oscar Mateo <[email protected]>

The backing objects and ringbuffers for contexts created via open fd are actually empty until the user starts sending execbuffers to them. At that point, we allocate & populate them. We do this because, at create time, we really don't know which engine is going to be used with the context later on (and we don't want to waste memory on objects that we might never use).

v2: As contexts created via ioctl can only be used with the render ring, we have enough information to allocate & populate them right away.

v3: Defer the creation always, even with ioctl-created contexts, as requested by Daniel Vetter.

Signed-off-by Oscar Mateo <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem_context.c    |    7 +++----
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    8 ++++++++
 2 files changed, 11 insertions(+), 4 deletions(-)

drivers/gpu/drm/i915/i915_gem_context.c +3 -4

index 48d7476..fbe7278 100644
@@ -784,9 +784,9 @@ int i915_switch_context(struct intel_engine_cs *ring,
 	return do_switch(ring, to);
 }
 
-static bool hw_context_enabled(struct drm_device *dev)
+static bool contexts_enabled(struct drm_device *dev)
 {
-	return to_i915(dev)->hw_context_size;
+	return i915.enable_execlists || to_i915(dev)->hw_context_size;
 }
 
 int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,

@@ -797,8 +797,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
 	struct intel_context *ctx;
 	int ret;
 
-	/* FIXME: allow user-created LR contexts as well */
-	if (!hw_context_enabled(dev))
+	if (!contexts_enabled(dev))
 		return -ENODEV;
 
 	ret = i915_mutex_lock_interruptible(dev);

drivers/gpu/drm/i915/i915_gem_execbuffer.c +8 -0

index c5115957..4e9b387 100644
@@ -951,6 +951,14 @@ i915_gem_validate_context(struct drm_device *dev, struct drm_file *file,
 		return ERR_PTR(-EIO);
 	}
 
+	if (i915.enable_execlists && !ctx->engine[ring->id].state) {
+		int ret = intel_lr_context_deferred_create(ctx, ring);
+		if (ret) {
+			DRM_DEBUG("Could not create LRC %u: %d\n", ctx_id, ret);
+			return ERR_PTR(ret);
+		}
+	}
+
 	return ctx;
 }
 

Signature · 6 lines

1.7.9.5

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

From: Oscar Mateo <[email protected]>

These two functions make no sense in an Logical Ring Context & Execlists world.

v2: We got rid of lrc_enabled and centralized everything in the sanitized i915.enbale_execlists instead.

Signed-off-by Oscar Mateo <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem_context.c |    9 +++++++++
 1 file changed, 9 insertions(+)

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

index fbe7278..288f5de 100644

drivers/gpu/drm/i915/i915_gem_context.c +1 -0

+
 	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
 
 	if (to->legacy_hw_ctx.rcs_state == NULL) { /* We have the fake context */

Signature · 6 lines

1.7.9.5

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Monday, August 11, 2014 3:30 PM
To: Daniel, Thomas
Cc: [email protected]
Subject: Re: [Intel-gfx] [PATCH 11/43] drm/i915/bdw: Render moot context reset and switch with Execlists

drivers/gpu/drm/i915/i915_gem_context.c void i915_gem_context_reset(struct drm_device

Quoted 19 lines · Thomas Daniel

From: Oscar Mateo <[email protected]>

These two functions make no sense in an Logical Ring Context & Execlists world.

v2: We got rid of lrc_enabled and centralized everything in the sanitized i915.enbale_execlists instead.

Signed-off-by Oscar Mateo <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem_context.c |    9 +++++++++
 1 file changed, 9 insertions(+)

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

index fbe7278..288f5de 100644
@@ -380,6 +380,9 @@ void i915_gem_context_reset(struct drm_device

*dev)

Quoted 3 lines

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int i;
 

drivers/gpu/drm/i915/i915_gem_context.c void i915_gem_context_reset(struct drm_device *dev) · line 384

Quoted 2 lines

drivers/gpu/drm/i915/i915_gem_context.c +2 -0

+	if (i915.enable_execlists)
+		return;

This will conflict badly with Alistair's patch at a functional level. I'm pretty sure that we want _some_ form of reset for the context state, since the hw didn't just magically load the previously running context. So NACK on this hunk.

drivers/gpu/drm/i915/i915_gem_context.c void i915_gem_context_reset(struct drm_device · line 386

Quoted 3 lines

drivers/gpu/drm/i915/i915_gem_context.c +1 -0

+
 	/* Prevent the hardware from restoring the last context (which

hung) on

Quoted 6 lines

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

 	 * the next switch */
 	for (i = 0; i < I915_NUM_RINGS; i++) { @@ -514,6 +517,9 @@ int

i915_gem_context_enable(struct drm_i915_private *dev_priv)

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

 		ppgtt->enable(ppgtt);
 	}

drivers/gpu/drm/i915/i915_gem_context.c

Quoted 2 lines

drivers/gpu/drm/i915/i915_gem_context.c +2 -0

+	if (i915.enable_execlists)
+		return 0;

Again this conflicts with Alistair's patch. Furthermore it looks redudant since you no-op out i915_switch_context separately.

Quoted 9 lines

drivers/gpu/drm/i915/i915_gem_context.c +1 -0

+
 	/* FIXME: We should make this work, even in reset */
 	if (i915_reset_in_progress(&dev_priv->gpu_error))
 		return 0;

@@ -769,6 +775,9 @@ int i915_switch_context(struct intel_engine_cs
 *ring,  {
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
 

drivers/gpu/drm/i915/i915_gem_context.c int i915_switch_context(struct intel_engine_cs · line 779

Quoted 2 lines

drivers/gpu/drm/i915/i915_gem_context.c +2 -0

+	if (i915.enable_execlists)
+		return 0;

I've hoped we don't need this with the legacy ringbuffer cmdsubmission fuly split out. If there are other paths (resume, gpu reset) where this comes into play then I guess we need to look at where the best place is to make this call. So until this comes with a bit a better justification I'll punt on this for now. -Daniel

Quoted 9 lines

drivers/gpu/drm/i915/i915_gem_context.c +1 -0

+
 	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
 
 	if (to->legacy_hw_ctx.rcs_state == NULL) { /* We have the fake

context */
--
1.7.9.5

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Quoted 35 lines

-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Monday, August 11, 2014 3:30 PM
To: Daniel, Thomas
Cc: [email protected]
Subject: Re: [Intel-gfx] [PATCH 11/43] drm/i915/bdw: Render moot context reset and switch with Execlists

drivers/gpu/drm/i915/i915_gem_context.c void i915_gem_context_reset(struct drm_device

Quoted 19 lines · Thomas Daniel

From: Oscar Mateo <[email protected]>

These two functions make no sense in an Logical Ring Context & Execlists world.

v2: We got rid of lrc_enabled and centralized everything in the sanitized i915.enbale_execlists instead.

Signed-off-by Oscar Mateo <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem_context.c |    9 +++++++++
 1 file changed, 9 insertions(+)

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

index fbe7278..288f5de 100644
@@ -380,6 +380,9 @@ void i915_gem_context_reset(struct drm_device

*dev)

Quoted 3 lines

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int i;
 

Quoted 6 lines

drivers/gpu/drm/i915/i915_gem_context.c void i915_gem_context_reset(struct drm_device *dev) · line 384

Quoted 2 lines

drivers/gpu/drm/i915/i915_gem_context.c +2 -0

+	if (i915.enable_execlists)
+		return;

This will conflict badly with Alistair's patch at a functional level. I'm pretty sure that we want _some_ form of reset for the context state, since the hw didn't just magically load the previously running context. So NACK on this hunk.

OK I'll wait to see the final version of Alistair's patch and decide what to do about this hunk.

Quoted 10 lines

drivers/gpu/drm/i915/i915_gem_context.c void i915_gem_context_reset(struct drm_device · line 386

Quoted 3 lines

drivers/gpu/drm/i915/i915_gem_context.c +1 -0

+
 	/* Prevent the hardware from restoring the last context (which

hung) on

Quoted 6 lines

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

 	 * the next switch */
 	for (i = 0; i < I915_NUM_RINGS; i++) { @@ -514,6 +517,9 @@ int

i915_gem_context_enable(struct drm_i915_private *dev_priv)

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

 		ppgtt->enable(ppgtt);
 	}

Quoted 5 lines

drivers/gpu/drm/i915/i915_gem_context.c

Quoted 2 lines

drivers/gpu/drm/i915/i915_gem_context.c +2 -0

+	if (i915.enable_execlists)
+		return 0;

Again this conflicts with Alistair's patch. Furthermore it looks redudant since you no-op out i915_switch_context separately.

I don't think this is a conflict. Doesn't Alistair's change here just involve writing PDPs for full PPGTT? We don't want to do that in lrc mode.

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/i915/i915_gem_context.c +1 -0

+
 	/* FIXME: We should make this work, even in reset */
 	if (i915_reset_in_progress(&dev_priv->gpu_error))
 		return 0;

@@ -769,6 +775,9 @@ int i915_switch_context(struct intel_engine_cs
 *ring,  {
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
 

Quoted 8 lines

drivers/gpu/drm/i915/i915_gem_context.c int i915_switch_context(struct intel_engine_cs · line 779

Quoted 2 lines

drivers/gpu/drm/i915/i915_gem_context.c +2 -0

+	if (i915.enable_execlists)
+		return 0;

I've hoped we don't need this with the legacy ringbuffer cmdsubmission fuly split out. If there are other paths (resume, gpu reset) where this comes into play then I guess we need to look at where the best place is to make this call. So until this comes with a bit a better justification I'll punt on this for now. -Daniel

Yes, the command submission lrc path doesn't call this but other codepaths do. If we keep the check in context_enable() the only remaining call I see is in i915_gpu_idle(). I don't mind if the check is done there but perhaps a WARN_ON should then be added into switch_context() because we don't want to be putting illegal MI_SET_CONTEXT commands into the ring.

These two functions make no sense in an Logical Ring Context & Execlists world.

v2: We got rid of lrc_enabled and centralized everything in the sanitized i915.enable_execlists instead.

Signed-off-by Oscar Mateo <[email protected]>

v3: Rebased. Corrected a typo in comment for i915_switch_context and added a comment that it should not be called in execlist mode. Added WARN_ON if i915_switch_context is called in execlist mode. Moved check for execlist mode out of i915_switch_context and into callers. Added comment in context_reset explaining why nothing is done in execlist mode.

Quoted 6 lines · Thomas Daniel

These two functions make no sense in an Logical Ring Context & Execlists world.

v2: We got rid of lrc_enabled and centralized everything in the sanitized i915.enable_execlists instead.

Quoted 8 lines

Signed-off-by Oscar Mateo <[email protected]>

v3: Rebased. Corrected a typo in comment for i915_switch_context and added a comment that it should not be called in execlist mode. Added WARN_ON if i915_switch_context is called in execlist mode. Moved check for execlist mode out of i915_switch_context and into callers. Added comment in context_reset explaining why nothing is done in execlist mode.

No, this is not the way. The requirement is to reduce the number of special cases not increase them. These should be evaluated to be no-ops when execlists is used.

Quoted 16 lines · Chris Wilson

Quoted 6 lines · Thomas Daniel

These two functions make no sense in an Logical Ring Context & Execlists world.

v2: We got rid of lrc_enabled and centralized everything in the sanitized i915.enable_execlists instead.

Quoted 8 lines

Signed-off-by Oscar Mateo <[email protected]>

v3: Rebased. Corrected a typo in comment for i915_switch_context and added a comment that it should not be called in execlist mode. Added WARN_ON if i915_switch_context is called in execlist mode. Moved check for execlist mode out of i915_switch_context and into callers. Added comment in context_reset explaining why nothing is done in execlist mode.

Quoted 3 lines

No, this is not the way. The requirement is to reduce the number of special cases not increase them. These should be evaluated to be no-ops when execlists is used.

I think it's ok-ish for now. Maybe we need to reconsider when we wire up lrc reclaim - which is the real user of the switch_context in gpu_idle. The problem I have though is that I can't parse the subject of the patch, someone please translate that to simplified English for me. I can do the replacement while applying.
-Daniel

Quoted 16 lines · Chris Wilson

Quoted 6 lines · Thomas Daniel

These two functions make no sense in an Logical Ring Context & Execlists world.

v2: We got rid of lrc_enabled and centralized everything in the sanitized i915.enable_execlists instead.

Quoted 8 lines

Signed-off-by Oscar Mateo <[email protected]>

v3: Rebased. Corrected a typo in comment for i915_switch_context and added a comment that it should not be called in execlist mode. Added WARN_ON if i915_switch_context is called in execlist mode. Moved check for execlist mode out of i915_switch_context and into callers. Added comment in context_reset explaining why nothing is done in execlist mode.

Quoted 3 lines

No, this is not the way. The requirement is to reduce the number of special cases not increase them. These should be evaluated to be no-ops when execlists is used.

I think it's ok-ish for now. Maybe we need to reconsider when we wire up lrc reclaim - which is the real user of the switch_context in gpu_idle. The problem I have though is that I can't parse the subject of the patch, someone please translate that to simplified English for me. I can do the replacement while applying.

Quoted 21 lines · Daniel Vetter

Quoted 16 lines · Chris Wilson

Quoted 6 lines · Thomas Daniel

These two functions make no sense in an Logical Ring Context & Execlists world.

v2: We got rid of lrc_enabled and centralized everything in the sanitized i915.enable_execlists instead.

Quoted 8 lines

Signed-off-by Oscar Mateo <[email protected]>

v3: Rebased. Corrected a typo in comment for i915_switch_context and added a comment that it should not be called in execlist mode. Added WARN_ON if i915_switch_context is called in execlist mode. Moved check for execlist mode out of i915_switch_context and into callers. Added comment in context_reset explaining why nothing is done in execlist mode.

Quoted 3 lines

No, this is not the way. The requirement is to reduce the number of special cases not increase them. These should be evaluated to be no-ops when execlists is used.

Quoted 5 lines

I think it's ok-ish for now. Maybe we need to reconsider when we wire up lrc reclaim - which is the real user of the switch_context in gpu_idle. The problem I have though is that I can't parse the subject of the patch, someone please translate that to simplified English for me. I can do the replacement while applying.

No, it is not. execlists is badly designed and this is a further symptom of that.
-Chris

Quoted 28 lines · Chris Wilson

Quoted 26 lines · Daniel Vetter

Quoted 16 lines · Chris Wilson

Quoted 6 lines · Thomas Daniel

These two functions make no sense in an Logical Ring Context & Execlists world.

v2: We got rid of lrc_enabled and centralized everything in the sanitized i915.enable_execlists instead.

Quoted 8 lines

Signed-off-by Oscar Mateo <[email protected]>

v3: Rebased. Corrected a typo in comment for i915_switch_context and added a comment that it should not be called in execlist mode. Added WARN_ON if i915_switch_context is called in execlist mode. Moved check for execlist mode out of i915_switch_context and into callers. Added comment in context_reset explaining why nothing is done in execlist mode.

Quoted 3 lines

No, this is not the way. The requirement is to reduce the number of special cases not increase them. These should be evaluated to be no-ops when execlists is used.

I think it's ok-ish for now. Maybe we need to reconsider when we wire up lrc reclaim - which is the real user of the switch_context in gpu_idle. The problem I have though is that I can't parse the subject of the patch, someone please translate that to simplified English for me. I can do the replacement while applying.

Quoted 4 lines

No, it is not. execlists is badly designed and this is a further symptom of that.
-Chris

Thomas is not available and I am replying on his behalf. Is the following subject is good for this patch?

"Don't execute context reset and switch when using Execlists"

From: Oscar Mateo <[email protected]>

This is mostly for correctness so that we know we are running the LR context correctly (this is, the PDPs are contained inside the context object).

v2: Move the check to inside the enable PPGTT function. The switch happens in two places: the legacy context switch (that we won't hit when Execlists are enabled) and the PPGTT enable, which unfortunately we need. This would look much nicer if the ppgtt->enable was part of the ring init, where it logically belongs.

Signed-off-by Oscar Mateo <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem_gtt.c |    5 +++++
 1 file changed, 5 insertions(+)

drivers/gpu/drm/i915/i915_gem_gtt.c +0 -0

index 5188936..ccd70f5 100644

From: Oscar Mateo <[email protected]>

This is mostly for correctness so that we know we are running the LR context correctly (this is, the PDPs are contained inside the context object).

v2: Move the check to inside the enable PPGTT function. The switch happens in two places: the legacy context switch (that we won't hit when Execlists are enabled) and the PPGTT enable, which unfortunately we need. This would look much nicer if the ppgtt->enable was part of the ring init, where it logically belongs.

v3: Move the check to the start of the enable PPGTT function. None of the legacy PPGTT enabling is required when using LRCs as the PPGTT is enabled in the context descriptor and the PDPs are written in the LRC.

Signed-off-by Oscar Mateo <[email protected]>

Signed-off-by Thomas Daniel <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem_gtt.c |    5 +++++
 1 file changed, 5 insertions(+)

drivers/gpu/drm/i915/i915_gem_gtt.c +5 -0

index 5188936..cfbf272 100644
@@ -843,6 +843,11 @@ static int gen8_ppgtt_enable(struct i915_hw_ppgtt *ppgtt)
 	struct intel_engine_cs *ring;
 	int j, ret;
 
+	/* In the case of Execlists, we don't want to write the PDPs
+	 * in the legacy way (they live inside the context now) */
+	if (i915.enable_execlists)
+		return 0;
+
 	for_each_ring(ring, dev_priv, j) {
 		I915_WRITE(RING_MODE_GEN7(ring),
 			   _MASKED_BIT_ENABLE(GFX_PPGTT_ENABLE));

Signature · 6 lines

1.7.9.5

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

From: Oscar Mateo <[email protected]>

This is mostly for correctness so that we know we are running the LR context correctly (this is, the PDPs are contained inside the context object).

v2: Move the check to inside the enable PPGTT function. The switch happens in two places: the legacy context switch (that we won't hit when Execlists are enabled) and the PPGTT enable, which unfortunately we need. This would look much nicer if the ppgtt->enable was part of the ring init, where it logically belongs.

v3: Move the check to the start of the enable PPGTT function. None of the legacy PPGTT enabling is required when using LRCs as the PPGTT is enabled in the context descriptor and the PDPs are written in the LRC.

Signed-off-by Oscar Mateo <[email protected]>

Signed-off-by Thomas Daniel <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem_gtt.c |    5 +++++
 1 file changed, 5 insertions(+)

drivers/gpu/drm/i915/i915_gem_gtt.c +0 -0

index 5188936..cfbf272 100644

drivers/gpu/drm/i915/i915_gem_gtt.c +4 -0

+	 * in the legacy way (they live inside the context now) */
+	if (i915.enable_execlists)
+		return 0;
+
 	for_each_ring(ring, dev_priv, j) {
 		I915_WRITE(RING_MODE_GEN7(ring),
 			   _MASKED_BIT_ENABLE(GFX_PPGTT_ENABLE));

Signature · 6 lines

1.7.9.5

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

From: Oscar Mateo <[email protected]>

As suggested by Daniel Vetter. The idea, in subsequent patches, is to provide an alternative to these vfuncs for the Execlists submission mechanism.

v2: Splitted into two and reordered to illustrate our intentions, instead of showing it off. Also, remove the add_request vfunc and added the stop_ring one.

Signed-off-by Oscar Mateo <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h            |   24 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem.c            |   15 +++++++++++----
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   20 ++++++++++----------
 3 files changed, 45 insertions(+), 14 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -0

index ff2c373..1caed52 100644
@@ -1617,6 +1617,21 @@ struct drm_i915_private {
 	/* Old ums support infrastructure, same warning applies. */
 	struct i915_ums_state ums;
 
+	/* Abstract the submission mechanism (legacy ringbuffer or execlists) away */
+	struct {

drivers/gpu/drm/i915/i915_drv.h +11 -0

+	} gt;
+
 	/*
 	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
 	 * will be rejected. Instead look for a better place.

@@ -2224,6 +2239,14 @@ int i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
 			      struct drm_file *file_priv);
 int i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
 			     struct drm_file *file_priv);
+int i915_gem_ringbuffer_submission(struct drm_device *dev,
+				   struct drm_file *file,
+				   struct intel_engine_cs *ring,
+				   struct intel_context *ctx,
+				   struct drm_i915_gem_execbuffer2 *args,
+				   struct list_head *vmas,
+				   struct drm_i915_gem_object *batch_obj,
+				   u64 exec_start, u32 flags);
 int i915_gem_execbuffer(struct drm_device *dev, void *data,
 			struct drm_file *file_priv);
 int i915_gem_execbuffer2(struct drm_device *dev, void *data,

@@ -2376,6 +2399,7 @@ void i915_gem_reset(struct drm_device *dev);
 bool i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
 int __must_check i915_gem_object_finish_gpu(struct drm_i915_gem_object *obj);
 int __must_check i915_gem_init(struct drm_device *dev);
+int i915_gem_init_rings(struct drm_device *dev);
 int __must_check i915_gem_init_hw(struct drm_device *dev);
 int i915_gem_l3_remap(struct intel_engine_cs *ring, int slice);
 void i915_gem_init_swizzling(struct drm_device *dev);

drivers/gpu/drm/i915/i915_gem.c +11 -4

index d8bf4fa..6544286 100644
@@ -4518,7 +4518,7 @@ i915_gem_stop_ringbuffers(struct drm_device *dev)
 	int i;
 
 	for_each_ring(ring, dev_priv, i)
-		intel_stop_ring_buffer(ring);
+		dev_priv->gt.stop_ring(ring);
 }
 
 int

@@ -4635,7 +4635,7 @@ intel_enable_blt(struct drm_device *dev)
 	return true;
 }
 
-static int i915_gem_init_rings(struct drm_device *dev)
+int i915_gem_init_rings(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;

@@ -4718,7 +4718,7 @@ i915_gem_init_hw(struct drm_device *dev)
 
 	i915_gem_init_swizzling(dev);
 
-	ret = i915_gem_init_rings(dev);
+	ret = dev_priv->gt.init_rings(dev);
 	if (ret)
 		return ret;
 

@@ -4759,6 +4759,13 @@ int i915_gem_init(struct drm_device *dev)
 			DRM_DEBUG_DRIVER("allow wake ack timed out\n");
 	}
 
+	if (!i915.enable_execlists) {
+		dev_priv->gt.do_execbuf = i915_gem_ringbuffer_submission;
+		dev_priv->gt.init_rings = i915_gem_init_rings;
+		dev_priv->gt.cleanup_ring = intel_cleanup_ring_buffer;
+		dev_priv->gt.stop_ring = intel_stop_ring_buffer;
+	}
+
 	i915_gem_init_userptr(dev);
 	i915_gem_init_global_gtt(dev);
 

@@ -4794,7 +4801,7 @@ i915_gem_cleanup_ringbuffer(struct drm_device *dev)
 	int i;
 
 	for_each_ring(ring, dev_priv, i)
-		intel_cleanup_ring_buffer(ring);
+		dev_priv->gt.cleanup_ring(ring);
 }
 
 int

drivers/gpu/drm/i915/i915_gem_execbuffer.c +10 -10

index 4e9b387..8c63d79 100644
@@ -1034,14 +1034,14 @@ i915_reset_gen7_sol_offsets(struct drm_device *dev,
 	return 0;
 }
 
-static int
-legacy_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
-			     struct intel_engine_cs *ring,
-			     struct intel_context *ctx,
-			     struct drm_i915_gem_execbuffer2 *args,
-			     struct list_head *vmas,
-			     struct drm_i915_gem_object *batch_obj,
-			     u64 exec_start, u32 flags)
+int
+i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
+			       struct intel_engine_cs *ring,
+			       struct intel_context *ctx,
+			       struct drm_i915_gem_execbuffer2 *args,
+			       struct list_head *vmas,
+			       struct drm_i915_gem_object *batch_obj,
+			       u64 exec_start, u32 flags)
 {
 	struct drm_clip_rect *cliprects = NULL;
 	struct drm_i915_private *dev_priv = dev->dev_private;

@@ -1408,8 +1408,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	else
 		exec_start += i915_gem_obj_offset(batch_obj, vm);
 
-	ret = legacy_ringbuffer_submission(dev, file, ring, ctx,
-			args, &eb->vmas, batch_obj, exec_start, flags);
+	ret = dev_priv->gt.do_execbuf(dev, file, ring, ctx, args,
+			&eb->vmas, batch_obj, exec_start, flags);
 	if (ret)
 		goto err;
 

Signature · 6 lines

1.7.9.5

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 27 lines · Thomas Daniel

From: Oscar Mateo <[email protected]>

As suggested by Daniel Vetter. The idea, in subsequent patches, is to provide an alternative to these vfuncs for the Execlists submission mechanism.

v2: Splitted into two and reordered to illustrate our intentions, instead of showing it off. Also, remove the add_request vfunc and added the stop_ring one.

Signed-off-by Oscar Mateo <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h            |   24 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem.c            |   15 +++++++++++----
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   20 ++++++++++----------
 3 files changed, 45 insertions(+), 14 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -0

index ff2c373..1caed52 100644
@@ -1617,6 +1617,21 @@ struct drm_i915_private {
 	/* Old ums support infrastructure, same warning applies. */
 	struct i915_ums_state ums;
 
+	/* Abstract the submission mechanism (legacy ringbuffer or execlists) away */
+	struct {

drivers/gpu/drm/i915/i915_drv.h struct drm_i915_private { · line 1631

Quoted 10 lines

drivers/gpu/drm/i915/i915_drv.h +10 -0

+		int (*do_execbuf) (struct drm_device *dev, struct drm_file *file,
+				   struct intel_engine_cs *ring,
+				   struct intel_context *ctx,
+				   struct drm_i915_gem_execbuffer2 *args,
+				   struct list_head *vmas,
+				   struct drm_i915_gem_object *batch_obj,
+				   u64 exec_start, u32 flags);
+		int (*init_rings) (struct drm_device *dev);
+		void (*cleanup_ring) (struct intel_engine_cs *ring);
+		void (*stop_ring) (struct intel_engine_cs *ring);

My rule of thumb is that with just one caller it's probably better to not have a vtable - just makes it harder to follow the code flow.

Usually (with non-borked code design at least) init/teardown functions have only one caller, so there's a good chance I'll ask you to remove them again.

drivers/gpu/drm/i915/i915_drv.h struct drm_i915_private { · line 1632

Quoted 2 lines

drivers/gpu/drm/i915/i915_drv.h +1 -0

+		bool (*is_ring_initialized) (struct intel_engine_cs *ring);

This one is unused and I didn't really like the taste of it. So I killed it.
-Daniel

Quoted 127 lines

drivers/gpu/drm/i915/i915_drv.h +11 -0

+	} gt;
+
 	/*
 	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
 	 * will be rejected. Instead look for a better place.

@@ -2224,6 +2239,14 @@ int i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
 			      struct drm_file *file_priv);
 int i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
 			     struct drm_file *file_priv);
+int i915_gem_ringbuffer_submission(struct drm_device *dev,
+				   struct drm_file *file,
+				   struct intel_engine_cs *ring,
+				   struct intel_context *ctx,
+				   struct drm_i915_gem_execbuffer2 *args,
+				   struct list_head *vmas,
+				   struct drm_i915_gem_object *batch_obj,
+				   u64 exec_start, u32 flags);
 int i915_gem_execbuffer(struct drm_device *dev, void *data,
 			struct drm_file *file_priv);
 int i915_gem_execbuffer2(struct drm_device *dev, void *data,

@@ -2376,6 +2399,7 @@ void i915_gem_reset(struct drm_device *dev);
 bool i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
 int __must_check i915_gem_object_finish_gpu(struct drm_i915_gem_object *obj);
 int __must_check i915_gem_init(struct drm_device *dev);
+int i915_gem_init_rings(struct drm_device *dev);
 int __must_check i915_gem_init_hw(struct drm_device *dev);
 int i915_gem_l3_remap(struct intel_engine_cs *ring, int slice);
 void i915_gem_init_swizzling(struct drm_device *dev);

drivers/gpu/drm/i915/i915_gem.c +11 -4

index d8bf4fa..6544286 100644
@@ -4518,7 +4518,7 @@ i915_gem_stop_ringbuffers(struct drm_device *dev)
 	int i;
 
 	for_each_ring(ring, dev_priv, i)
-		intel_stop_ring_buffer(ring);
+		dev_priv->gt.stop_ring(ring);
 }
 
 int

@@ -4635,7 +4635,7 @@ intel_enable_blt(struct drm_device *dev)
 	return true;
 }
 
-static int i915_gem_init_rings(struct drm_device *dev)
+int i915_gem_init_rings(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;

@@ -4718,7 +4718,7 @@ i915_gem_init_hw(struct drm_device *dev)
 
 	i915_gem_init_swizzling(dev);
 
-	ret = i915_gem_init_rings(dev);
+	ret = dev_priv->gt.init_rings(dev);
 	if (ret)
 		return ret;
 

@@ -4759,6 +4759,13 @@ int i915_gem_init(struct drm_device *dev)
 			DRM_DEBUG_DRIVER("allow wake ack timed out\n");
 	}
 
+	if (!i915.enable_execlists) {
+		dev_priv->gt.do_execbuf = i915_gem_ringbuffer_submission;
+		dev_priv->gt.init_rings = i915_gem_init_rings;
+		dev_priv->gt.cleanup_ring = intel_cleanup_ring_buffer;
+		dev_priv->gt.stop_ring = intel_stop_ring_buffer;
+	}
+
 	i915_gem_init_userptr(dev);
 	i915_gem_init_global_gtt(dev);
 

@@ -4794,7 +4801,7 @@ i915_gem_cleanup_ringbuffer(struct drm_device *dev)
 	int i;
 
 	for_each_ring(ring, dev_priv, i)
-		intel_cleanup_ring_buffer(ring);
+		dev_priv->gt.cleanup_ring(ring);
 }
 
 int

drivers/gpu/drm/i915/i915_gem_execbuffer.c +10 -10

index 4e9b387..8c63d79 100644
@@ -1034,14 +1034,14 @@ i915_reset_gen7_sol_offsets(struct drm_device *dev,
 	return 0;
 }
 
-static int
-legacy_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
-			     struct intel_engine_cs *ring,
-			     struct intel_context *ctx,
-			     struct drm_i915_gem_execbuffer2 *args,
-			     struct list_head *vmas,
-			     struct drm_i915_gem_object *batch_obj,
-			     u64 exec_start, u32 flags)
+int
+i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
+			       struct intel_engine_cs *ring,
+			       struct intel_context *ctx,
+			       struct drm_i915_gem_execbuffer2 *args,
+			       struct list_head *vmas,
+			       struct drm_i915_gem_object *batch_obj,
+			       u64 exec_start, u32 flags)
 {
 	struct drm_clip_rect *cliprects = NULL;
 	struct drm_i915_private *dev_priv = dev->dev_private;

@@ -1408,8 +1408,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	else
 		exec_start += i915_gem_obj_offset(batch_obj, vm);
 
-	ret = legacy_ringbuffer_submission(dev, file, ring, ctx,
-			args, &eb->vmas, batch_obj, exec_start, flags);
+	ret = dev_priv->gt.do_execbuf(dev, file, ring, ctx, args,
+			&eb->vmas, batch_obj, exec_start, flags);
 	if (ret)
 		goto err;
 

Signature · 6 lines

1.7.9.5

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

From: Oscar Mateo <[email protected]>

As suggested by Daniel Vetter. The idea, in subsequent patches, is to provide an alternative to these vfuncs for the Execlists submission mechanism.

v2: Splitted into two and reordered to illustrate our intentions, instead of showing it off. Also, remove the add_request vfunc and added the stop_ring one.

Signed-off-by Oscar Mateo <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h            |   24 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem.c            |   15 +++++++++++----
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   20 ++++++++++----------
 3 files changed, 45 insertions(+), 14 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +3 -0

index ff2c373..1caed52 100644
@@ -1617,6 +1617,21 @@ struct drm_i915_private {
 	/* Old ums support infrastructure, same warning applies. */
 	struct i915_ums_state ums;
 
+	/* Abstract the submission mechanism (legacy ringbuffer or execlists) away */
+	struct {
+		int (*do_execbuf) (struct drm_device *dev, struct drm_file *file,

drivers/gpu/drm/i915/i915_drv.h +11 -0

+	} gt;
+
 	/*
 	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
 	 * will be rejected. Instead look for a better place.

@@ -2224,6 +2239,14 @@ int i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
 			      struct drm_file *file_priv);
 int i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
 			     struct drm_file *file_priv);
+int i915_gem_ringbuffer_submission(struct drm_device *dev,
+				   struct drm_file *file,
+				   struct intel_engine_cs *ring,
+				   struct intel_context *ctx,
+				   struct drm_i915_gem_execbuffer2 *args,
+				   struct list_head *vmas,
+				   struct drm_i915_gem_object *batch_obj,
+				   u64 exec_start, u32 flags);
 int i915_gem_execbuffer(struct drm_device *dev, void *data,
 			struct drm_file *file_priv);
 int i915_gem_execbuffer2(struct drm_device *dev, void *data,

@@ -2376,6 +2399,7 @@ void i915_gem_reset(struct drm_device *dev);
 bool i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
 int __must_check i915_gem_object_finish_gpu(struct drm_i915_gem_object *obj);
 int __must_check i915_gem_init(struct drm_device *dev);
+int i915_gem_init_rings(struct drm_device *dev);
 int __must_check i915_gem_init_hw(struct drm_device *dev);
 int i915_gem_l3_remap(struct intel_engine_cs *ring, int slice);
 void i915_gem_init_swizzling(struct drm_device *dev);

drivers/gpu/drm/i915/i915_gem.c +11 -4

index d8bf4fa..6544286 100644
@@ -4518,7 +4518,7 @@ i915_gem_stop_ringbuffers(struct drm_device *dev)
 	int i;
 
 	for_each_ring(ring, dev_priv, i)
-		intel_stop_ring_buffer(ring);
+		dev_priv->gt.stop_ring(ring);
 }
 
 int

@@ -4635,7 +4635,7 @@ intel_enable_blt(struct drm_device *dev)
 	return true;
 }
 
-static int i915_gem_init_rings(struct drm_device *dev)
+int i915_gem_init_rings(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;

@@ -4718,7 +4718,7 @@ i915_gem_init_hw(struct drm_device *dev)
 
 	i915_gem_init_swizzling(dev);
 
-	ret = i915_gem_init_rings(dev);
+	ret = dev_priv->gt.init_rings(dev);
 	if (ret)
 		return ret;
 

@@ -4759,6 +4759,13 @@ int i915_gem_init(struct drm_device *dev)
 			DRM_DEBUG_DRIVER("allow wake ack timed out\n");
 	}
 
+	if (!i915.enable_execlists) {
+		dev_priv->gt.do_execbuf = i915_gem_ringbuffer_submission;
+		dev_priv->gt.init_rings = i915_gem_init_rings;
+		dev_priv->gt.cleanup_ring = intel_cleanup_ring_buffer;
+		dev_priv->gt.stop_ring = intel_stop_ring_buffer;
+	}
+
 	i915_gem_init_userptr(dev);
 	i915_gem_init_global_gtt(dev);
 

@@ -4794,7 +4801,7 @@ i915_gem_cleanup_ringbuffer(struct drm_device *dev)
 	int i;
 
 	for_each_ring(ring, dev_priv, i)
-		intel_cleanup_ring_buffer(ring);
+		dev_priv->gt.cleanup_ring(ring);
 }
 
 int

drivers/gpu/drm/i915/i915_gem_execbuffer.c +10 -10

index 4e9b387..8c63d79 100644
@@ -1034,14 +1034,14 @@ i915_reset_gen7_sol_offsets(struct drm_device *dev,
 	return 0;
 }
 
-static int
-legacy_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
-			     struct intel_engine_cs *ring,
-			     struct intel_context *ctx,
-			     struct drm_i915_gem_execbuffer2 *args,
-			     struct list_head *vmas,
-			     struct drm_i915_gem_object *batch_obj,
-			     u64 exec_start, u32 flags)
+int
+i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
+			       struct intel_engine_cs *ring,
+			       struct intel_context *ctx,
+			       struct drm_i915_gem_execbuffer2 *args,
+			       struct list_head *vmas,
+			       struct drm_i915_gem_object *batch_obj,
+			       u64 exec_start, u32 flags)
 {
 	struct drm_clip_rect *cliprects = NULL;
 	struct drm_i915_private *dev_priv = dev->dev_private;

@@ -1408,8 +1408,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	else
 		exec_start += i915_gem_obj_offset(batch_obj, vm);
 
-	ret = legacy_ringbuffer_submission(dev, file, ring, ctx,
-			args, &eb->vmas, batch_obj, exec_start, flags);
+	ret = dev_priv->gt.do_execbuf(dev, file, ring, ctx, args,
+			&eb->vmas, batch_obj, exec_start, flags);
 	if (ret)
 		goto err;
 

Signature · 6 lines

1.7.9.5

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx