[PATCH] drm/i915: don't track relative-constants-mode

[PATCH] drm/i915: don't track relative-constants-mode

[Dave Gordon]

'relative_constants_mode' has always been tracked per-device, but this
has actually been wrong ever since hardware contexts were introduced, as
the INSTPM register is saved (and automatically restored) as part of the
render ring context. The software per-device value could therefore get
out of sync with the hardware per-context value.

Rather than track this correctly (per-context in LRC modes, per-device
in legacy ringbuffer mode), a simpler solution is just to give up trying
to track it at all, and always prefix each render batch with the single
instruction needed to explicitly set it to the required mode for the
current batch.

Test case (if anyone wants to write it) would be to create two contexts,
submit a batch with a non-default mode in the first context, submit a
batch with the default mode in the other context, submit another batch
in the first context, but this time in default mode. Without this patch,
the driver will fail to insert the instructions to reset INSTPM into
the first context's ringbuffer.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92448

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            |  2 --
 drivers/gpu/drm/i915/i915_gem.c            |  3 ---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 32 +++++++++++++-----------------
 3 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9fdaf23..7938da0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1728,8 +1728,6 @@ struct drm_i915_private {
 
 	const struct intel_device_info info;
 
-	int relative_constants_mode;
-
 	void __iomem *regs;
 
 	struct intel_uncore uncore;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d37b441..8c06778 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4615,9 +4615,6 @@ int i915_gem_init(struct drm_device *dev)
 			  i915_gem_idle_work_handler);
 	init_waitqueue_head(&dev_priv->gpu_error.wait_queue);
 	init_waitqueue_head(&dev_priv->gpu_error.reset_queue);
-
-	dev_priv->relative_constants_mode = I915_EXEC_CONSTANTS_REL_GENERAL;
-
 	init_waitqueue_head(&dev_priv->pending_flip_queue);
 
 	dev_priv->mm.interruptible = true;
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 601156c..abfb06e 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1462,30 +1462,28 @@ static void eb_export_fence(struct drm_i915_gem_object *obj,
 			return -EINVAL;
 		}
 
-		if (instp_mode != dev_priv->relative_constants_mode) {
-			if (INTEL_INFO(dev_priv)->gen < 4) {
-				DRM_DEBUG("no rel constants on pre-gen4\n");
-				return -EINVAL;
-			}
-
-			if (INTEL_INFO(dev_priv)->gen > 5 &&
-			    instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
-				DRM_DEBUG("rel surface constants mode invalid on gen5+\n");
-				return -EINVAL;
-			}
+		if (INTEL_INFO(dev_priv)->gen < 4) {
+			DRM_DEBUG("no rel constants on pre-gen4\n");
+			return -EINVAL;
+		}
 
-			/* The HW changed the meaning on this bit on gen6 */
-			if (INTEL_INFO(dev_priv)->gen >= 6)
-				instp_mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
+		if (INTEL_INFO(dev_priv)->gen > 5 &&
+		    instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
+			DRM_DEBUG("rel surface constants mode invalid on gen5+\n");
+			return -EINVAL;
 		}
+
+		/* The HW changed the meaning on this bit on gen6 */
+		if (INTEL_INFO(dev_priv)->gen >= 6)
+			instp_mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
+
 		break;
 	default:
 		DRM_DEBUG("execbuf with unknown constants: %d\n", instp_mode);
 		return -EINVAL;
 	}
 
-	if (params->engine->id == RCS &&
-	    instp_mode != dev_priv->relative_constants_mode) {
+	if (params->engine->id == RCS) {
 		struct intel_ring *ring = params->request->ring;
 
 		ret = intel_ring_begin(params->request, 4);
@@ -1497,8 +1495,6 @@ static void eb_export_fence(struct drm_i915_gem_object *obj,
 		intel_ring_emit_reg(ring, INSTPM);
 		intel_ring_emit(ring, instp_mask << 16 | instp_mode);
 		intel_ring_advance(ring);
-
-		dev_priv->relative_constants_mode = instp_mode;
 	}
 
 	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
-- 
1.9.1

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

Re: [PATCH] drm/i915: don't track relative-constants-mode

[Chris Wilson]

On Fri, Aug 26, 2016 at 07:25:57PM +0100, Dave Gordon wrote:
> 'relative_constants_mode' has always been tracked per-device, but this
> has actually been wrong ever since hardware contexts were introduced, as
> the INSTPM register is saved (and automatically restored) as part of the
> render ring context. The software per-device value could therefore get
> out of sync with the hardware per-context value.
> 
> Rather than track this correctly (per-context in LRC modes, per-device
> in legacy ringbuffer mode), a simpler solution is just to give up trying
> to track it at all, and always prefix each render batch with the single
> instruction needed to explicitly set it to the required mode for the
> current batch.

Agreed. However always emitting it wasteful for those who never touch
it, i.e. everybody. Rather than track the last value, just track if it
ever changed from default then reassign it before every batch?

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 99d50c0a710c..394c9cd3ddd3 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1578,7 +1578,7 @@ execbuf_submit(struct i915_execbuffer_params *params,
                intel_ring_emit(ring, instp_mask << 16 | instp_mode);
                intel_ring_advance(ring);
 
-               dev_priv->relative_constants_mode = instp_mode;
+               dev_priv->relative_constants_mode = -1;
        }
 
        if (args->flags & I915_EXEC_GEN7_SOL_RESET) {


-- 
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 track relative-constants-mode

[Chris Wilson]

On Fri, Aug 26, 2016 at 07:47:19PM +0100, Chris Wilson wrote:
> On Fri, Aug 26, 2016 at 07:25:57PM +0100, Dave Gordon wrote:
> > 'relative_constants_mode' has always been tracked per-device, but this
> > has actually been wrong ever since hardware contexts were introduced, as
> > the INSTPM register is saved (and automatically restored) as part of the
> > render ring context. The software per-device value could therefore get
> > out of sync with the hardware per-context value.
> > 
> > Rather than track this correctly (per-context in LRC modes, per-device
> > in legacy ringbuffer mode), a simpler solution is just to give up trying
> > to track it at all, and always prefix each render batch with the single
> > instruction needed to explicitly set it to the required mode for the
> > current batch.
> 
> Agreed. However always emitting it wasteful for those who never touch
> it, i.e. everybody. Rather than track the last value, just track if it
> ever changed from default then reassign it before every batch?

And since nothing released has ever used it:

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a93efd471e94..1581f37d531b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -283,7 +283,8 @@ static int i915_getparam(struct drm_device *dev, void *data,
                value = 1;
                break;
        case I915_PARAM_HAS_EXEC_CONSTANTS:
+               WARN_ONCE(1, "Planned obsolence: please report to bugs.freedesktop.org\n");
                value = INTEL_INFO(dev)->gen >= 4;
                break;
        case I915_PARAM_HAS_RELAXED_DELTA:
                value = 1;

and deleting the code later would be an option.
-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 track relative-constants-mode

[Dave Gordon]

On 26/08/16 19:47, Chris Wilson wrote:
> On Fri, Aug 26, 2016 at 07:25:57PM +0100, Dave Gordon wrote:
>> 'relative_constants_mode' has always been tracked per-device, but this
>> has actually been wrong ever since hardware contexts were introduced, as
>> the INSTPM register is saved (and automatically restored) as part of the
>> render ring context. The software per-device value could therefore get
>> out of sync with the hardware per-context value.
>>
>> Rather than track this correctly (per-context in LRC modes, per-device
>> in legacy ringbuffer mode), a simpler solution is just to give up trying
>> to track it at all, and always prefix each render batch with the single
>> instruction needed to explicitly set it to the required mode for the
>> current batch.
>
> Agreed. However always emitting it wasteful for those who never touch
> it, i.e. everybody. Rather than track the last value, just track if it
> ever changed from default then reassign it before every batch?

But where to track whether it changed? If tracked globally, that will 
mean everyone pays if any one process ever submits a batch with a 
non-default value. If not global, then we still have the discrepancy 
between LRC modes (h/w value saved per-context) vs ringbuffer (maybe 
saved if device supports "hardware contexts", not saved otherwise).

Perhaps instead we should decide that between batches it shall always be 
left at the default value, so any batch that wants any other value must 
set it at the beginning and reset it when finished?

.Dave.

[PS: the reason I spotted this is because the tracking was broken with 
pre-emption enabled: you can no longer assume that the mode at the start 
of each batch is the same mode that the preceding batch used.]

> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 99d50c0a710c..394c9cd3ddd3 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1578,7 +1578,7 @@ execbuf_submit(struct i915_execbuffer_params *params,
>                 intel_ring_emit(ring, instp_mask << 16 | instp_mode);
>                 intel_ring_advance(ring);
>
> -               dev_priv->relative_constants_mode = instp_mode;
> +               dev_priv->relative_constants_mode = -1;
>         }
>
>         if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
>
>

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

[PATCH] drm/i915: don't track relative-constants-mode

[Dave Gordon]

'relative_constants_mode' has always been tracked per-device, but this
has actually been wrong ever since hardware contexts were introduced, as
the INSTPM register is saved (and automatically restored) as part of the
render ring context. The software per-device value could therefore get
out of sync with the hardware per-context value.

Rather than track this correctly (per-context), a simpler solution is
just to give up trying to track it at all, and always prefix each render
batch with the single instruction needed to explicitly set it to the
required mode for the current batch.

Test case (if anyone wants to write it) would be to create two contexts,
submit a batch with a non-default mode in the first context, submit a
batch with the default mode in the other context, submit another batch
in the first context, but this time in default mode. Without this patch,
the driver will fail to insert the instructions to reset INSTPM into
the first context's ringbuffer.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92448

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            |  2 --
 drivers/gpu/drm/i915/i915_gem.c            |  2 --
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 32 +++++++++++++-----------------
 drivers/gpu/drm/i915/intel_lrc.c           | 20 ++++++++-----------
 4 files changed, 22 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9a15ebe..9ddbb93 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1707,8 +1707,6 @@ struct drm_i915_private {
 
 	const struct intel_device_info info;
 
-	int relative_constants_mode;
-
 	void __iomem *regs;
 
 	struct intel_uncore uncore;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d0fa548..769982a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4947,8 +4947,6 @@ i915_gem_load(struct drm_device *dev)
 			  i915_gem_idle_work_handler);
 	init_waitqueue_head(&dev_priv->gpu_error.reset_queue);
 
-	dev_priv->relative_constants_mode = I915_EXEC_CONSTANTS_REL_GENERAL;
-
 	if (INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev))
 		dev_priv->num_fence_regs = 32;
 	else if (INTEL_INFO(dev)->gen >= 4 || IS_I945G(dev) || IS_I945GM(dev) || IS_G33(dev))
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 6ed7d63a..07ae12d 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1211,30 +1211,28 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
 			return -EINVAL;
 		}
 
-		if (instp_mode != dev_priv->relative_constants_mode) {
-			if (INTEL_INFO(dev)->gen < 4) {
-				DRM_DEBUG("no rel constants on pre-gen4\n");
-				return -EINVAL;
-			}
-
-			if (INTEL_INFO(dev)->gen > 5 &&
-			    instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
-				DRM_DEBUG("rel surface constants mode invalid on gen5+\n");
-				return -EINVAL;
-			}
+		if (INTEL_INFO(dev)->gen < 4) {
+			DRM_DEBUG("no rel constants on pre-gen4\n");
+			return -EINVAL;
+		}
 
-			/* The HW changed the meaning on this bit on gen6 */
-			if (INTEL_INFO(dev)->gen >= 6)
-				instp_mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
+		if (INTEL_INFO(dev)->gen > 5 &&
+		    instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
+			DRM_DEBUG("rel surface constants mode invalid on gen5+\n");
+			return -EINVAL;
 		}
+
+		/* 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);
 		return -EINVAL;
 	}
 
-	if (ring == &dev_priv->ring[RCS] &&
-	    instp_mode != dev_priv->relative_constants_mode) {
+	if (ring == &dev_priv->ring[RCS]) {
 		ret = intel_ring_begin(params->request, 4);
 		if (ret)
 			return ret;
@@ -1244,8 +1242,6 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
 		intel_ring_emit(ring, INSTPM);
 		intel_ring_emit(ring, instp_mask << 16 | instp_mode);
 		intel_ring_advance(ring);
-
-		dev_priv->relative_constants_mode = instp_mode;
 	}
 
 	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 3265427..8b3fe8f 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -889,15 +889,14 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
 			return -EINVAL;
 		}
 
-		if (instp_mode != dev_priv->relative_constants_mode) {
-			if (instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
-				DRM_DEBUG("rel surface constants mode invalid on gen5+\n");
-				return -EINVAL;
-			}
-
-			/* The HW changed the meaning on this bit on gen6 */
-			instp_mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
+		if (instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
+			DRM_DEBUG("rel surface constants mode invalid on gen5+\n");
+			return -EINVAL;
 		}
+
+		/* The HW changed the meaning on this bit on gen6 */
+		instp_mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
+
 		break;
 	default:
 		DRM_DEBUG("execbuf with unknown constants: %d\n", instp_mode);
@@ -913,8 +912,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
 	if (ret)
 		return ret;
 
-	if (ring == &dev_priv->ring[RCS] &&
-	    instp_mode != dev_priv->relative_constants_mode) {
+	if (ring == &dev_priv->ring[RCS]) {
 		ret = intel_logical_ring_begin(params->request, 4);
 		if (ret)
 			return ret;
@@ -924,8 +922,6 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
 		intel_logical_ring_emit(ringbuf, INSTPM);
 		intel_logical_ring_emit(ringbuf, instp_mask << 16 | instp_mode);
 		intel_logical_ring_advance(ringbuf);
-
-		dev_priv->relative_constants_mode = instp_mode;
 	}
 
 	exec_start = params->batch_obj_vm_offset +
-- 
1.9.1

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