* [RFC 04/13] drm/i915: Force wake restore for TDR
@ 2013-12-16 16:02 Lister, Ian
2013-12-16 17:32 ` Daniel Vetter
0 siblings, 1 reply; 2+ messages in thread
From: Lister, Ian @ 2013-12-16 16:02 UTC (permalink / raw)
To: intel-gfx@lists.freedesktop.org
>From 698fdaf8a85fb7039dacdff3068be2018a486816 Mon Sep 17 00:00:00 2001
Message-Id: <698fdaf8a85fb7039dacdff3068be2018a486816.1387201899.git.ian.lister@intel.com>
In-Reply-To: <cover.1387201899.git.ian.lister@intel.com>
References: <cover.1387201899.git.ian.lister@intel.com>
From: ian-lister <ian.lister@intel.com>
Date: Mon, 9 Dec 2013 09:47:20 +0000
Subject: [RFC 04/13] drm/i915: Force wake restore for TDR
Abstracted force wake restore code from gen6_do_reset into its own function
"gen6_gt_force_wake_restore" as it will also be needed for per-engine reset.
It has been expanded to cope with the Valleyview specific forcewake restore.
Signed-off-by: ian-lister <ian.lister@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 4 +
drivers/gpu/drm/i915/i915_reg.h | 1 +
drivers/gpu/drm/i915/intel_ringbuffer.h | 1 +
drivers/gpu/drm/i915/intel_uncore.c | 197 ++++++++++++++++++++++++++++++--
4 files changed, 192 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f261ab5..b9c4876 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -459,6 +459,8 @@ struct intel_uncore {
unsigned fw_rendercount;
unsigned fw_mediacount;
+ unsigned ecobus;
+
struct delayed_work force_wake_work;
};
@@ -1927,6 +1929,8 @@ extern int i915_emit_box(struct drm_device *dev,
struct drm_clip_rect *box,
int DR1, int DR4);
extern int intel_gpu_reset(struct drm_device *dev);
+extern int intel_gpu_engine_reset(struct drm_device *dev,
+ enum intel_ring_id engine);
extern int i915_reset(struct drm_device *dev);
extern unsigned long i915_chipset_val(struct drm_i915_private *dev_priv);
extern unsigned long i915_mch_val(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index bda7562..e896bca 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -107,6 +107,7 @@
#define GEN6_GRDOM_RENDER (1 << 1)
#define GEN6_GRDOM_MEDIA (1 << 2)
#define GEN6_GRDOM_BLT (1 << 3)
+#define GEN6_GRDOM_VEBOX (1 << 4)
#define RING_PP_DIR_BASE(ring) ((ring)->mmio_base+0x228)
#define RING_PP_DIR_BASE_READ(ring) ((ring)->mmio_base+0x518)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index c15b97b..6417de1 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -65,6 +65,7 @@ struct intel_ring_hangcheck {
u32 hd;
int score;
enum intel_ring_hangcheck_action action;
+ u32 resets; /* Total resets applied to this ring/engine*/
};
struct intel_ring_buffer {
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 848bdaf..764e2af 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -283,6 +283,44 @@ void vlv_force_wake_put(struct drm_i915_private *dev_priv,
spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
}
+void vlv_force_wake_restore(struct drm_i915_private *dev_priv,
+ int fw_engine)
+{
+ /* Restore the current force wake state with the hardware
+ * WARNING: Caller *MUST* hold uncore.lock whilst calling this function
+ */
+
+ if (FORCEWAKE_RENDER & fw_engine) {
+ if (dev_priv->uncore.fw_rendercount
+ && (__raw_i915_read32(dev_priv, FORCEWAKE_ACK_VLV)
+ & FORCEWAKE_KERNEL) == 0)
+ dev_priv->uncore.funcs.force_wake_get(dev_priv,
+ FORCEWAKE_RENDER);
+ else if (!dev_priv->uncore.fw_rendercount
+ && (__raw_i915_read32(dev_priv, FORCEWAKE_ACK_VLV)
+ & FORCEWAKE_KERNEL) == 1)
+ dev_priv->uncore.funcs.force_wake_put(dev_priv,
+ FORCEWAKE_RENDER);
+ }
+
+ if (FORCEWAKE_MEDIA & fw_engine) {
+ if (dev_priv->uncore.fw_mediacount
+ && (__raw_i915_read32(dev_priv, FORCEWAKE_ACK_MEDIA_VLV)
+ & FORCEWAKE_KERNEL) == 0)
+ dev_priv->uncore.funcs.force_wake_get(dev_priv,
+ FORCEWAKE_MEDIA);
+ else if (!dev_priv->uncore.fw_mediacount
+ && (__raw_i915_read32(dev_priv, FORCEWAKE_ACK_MEDIA_VLV)
+ & FORCEWAKE_KERNEL) == 1)
+ dev_priv->uncore.funcs.force_wake_put(dev_priv,
+ FORCEWAKE_MEDIA);
+ }
+
+ dev_priv->uncore.fifo_count =
+ __raw_i915_read32(dev_priv, GTFIFOCTL)
+ & GT_FIFO_FREE_ENTRIES_MASK;
+}
+
static void gen6_force_wake_work(struct work_struct *work)
{
struct drm_i915_private *dev_priv =
@@ -397,6 +435,50 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
}
+void gen6_gt_force_wake_restore(struct drm_i915_private *dev_priv)
+{
+ /* Restore the current expected force wake state with the
+ * hardware. This may be required following a reset.
+ *
+ * WARNING: Caller *MUST* hold uncore.lock
+ *
+ * uncore.lock isn't taken in this function to allow the caller the
+ * flexibility to do other work immediately before/after
+ * whilst holding the lock.
+ */
+ struct drm_device *dev = dev_priv->dev;
+ u32 forcewake_ack;
+
+ if (IS_VALLEYVIEW(dev))
+ return vlv_force_wake_restore(dev_priv, FORCEWAKE_ALL);
+
+ if (IS_HASWELL(dev) || IS_GEN8(dev))
+ forcewake_ack = FORCEWAKE_ACK_HSW;
+ else if (IS_IVYBRIDGE(dev)
+ && (dev_priv->uncore.ecobus & FORCEWAKE_MT_ENABLE))
+ forcewake_ack = FORCEWAKE_MT_ACK;
+ else
+ forcewake_ack = 0;
+
+ if (dev_priv->uncore.forcewake_count
+ && (!forcewake_ack
+ || ((__raw_i915_read32(dev_priv, forcewake_ack)
+ & FORCEWAKE_KERNEL) == 0))) {
+ /* It was enabled, so re-enable it */
+ dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
+ } else if ((dev_priv->uncore.forcewake_count == 0)
+ && (!forcewake_ack
+ || ((__raw_i915_read32(dev_priv, forcewake_ack)
+ & FORCEWAKE_KERNEL) == 1))) {
+ /* It was disabled, so disable it */
+ dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
+ }
+
+ dev_priv->uncore.fifo_count =
+ __raw_i915_read32(dev_priv, GTFIFOCTL)
+ & GT_FIFO_FREE_ENTRIES_MASK;
+}
+
/* We give fast paths for the really cool registers */
#define NEEDS_FORCE_WAKE(dev_priv, reg) \
((reg) < 0x40000 && (reg) != FORCEWAKE)
@@ -661,8 +743,6 @@ void intel_uncore_init(struct drm_device *dev)
dev_priv->uncore.funcs.force_wake_get = __gen6_gt_force_wake_mt_get;
dev_priv->uncore.funcs.force_wake_put = __gen6_gt_force_wake_mt_put;
} else if (IS_IVYBRIDGE(dev)) {
- u32 ecobus;
-
/* IVB configs may use multi-threaded forcewake */
/* A small trick here - if the bios hasn't configured
@@ -674,11 +754,11 @@ void intel_uncore_init(struct drm_device *dev)
*/
mutex_lock(&dev->struct_mutex);
__gen6_gt_force_wake_mt_get(dev_priv, FORCEWAKE_ALL);
- ecobus = __raw_i915_read32(dev_priv, ECOBUS);
+ dev_priv->uncore.ecobus = __raw_i915_read32(dev_priv, ECOBUS);
__gen6_gt_force_wake_mt_put(dev_priv, FORCEWAKE_ALL);
mutex_unlock(&dev->struct_mutex);
- if (ecobus & FORCEWAKE_MT_ENABLE) {
+ if (dev_priv->uncore.ecobus & FORCEWAKE_MT_ENABLE) {
dev_priv->uncore.funcs.force_wake_get =
__gen6_gt_force_wake_mt_get;
dev_priv->uncore.funcs.force_wake_put =
@@ -897,16 +977,88 @@ static int gen6_do_reset(struct drm_device *dev)
intel_uncore_forcewake_reset(dev);
- /* If reset with a user forcewake, try to restore, otherwise turn it off */
- if (dev_priv->uncore.forcewake_count)
- dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
- else
- dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
+ spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
+ return ret;
+}
+
+static int gen6_do_engine_reset(struct drm_device *dev,
+ enum intel_ring_id engine)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct intel_ring_buffer *ring = &dev_priv->ring[engine];
+ int ret = -ENODEV;
+ unsigned long irqflags;
+ char *reset_event[2];
+ reset_event[1] = NULL;
- /* Restore fifo count */
- dev_priv->uncore.fifo_count = __raw_i915_read32(dev_priv, GTFIFOCTL) & GT_FIFO_FREE_ENTRIES_MASK;
+ DRM_DEBUG_TDR("Engine reset %d\n", engine);
+
+ /* Hold uncore.lock across reset to prevent any register access
+ * with forcewake not set correctly
+ */
+ spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
+
+ ring->hangcheck.resets++;
+
+ /* Reset the engine.
+ * GEN6_GDRST is not in the gt power well so no need to check
+ * for fifo space for the write or forcewake the chip for
+ * the read.
+ */
+ switch (engine) {
+ case RCS:
+ __raw_i915_write32(dev_priv, GEN6_GDRST, GEN6_GRDOM_RENDER);
+ /* Spin waiting for the device to ack the reset request */
+ ret = wait_for_atomic((__raw_i915_read32(dev_priv,
+ GEN6_GDRST)
+ & GEN6_GRDOM_RENDER) == 0, 500);
+ DRM_DEBUG_TDR("RCS Reset\n");
+ break;
+
+ case BCS:
+ __raw_i915_write32(dev_priv, GEN6_GDRST, GEN6_GRDOM_BLT);
+
+ /* Spin waiting for the device to ack the reset request */
+ ret = wait_for_atomic((__raw_i915_read32(dev_priv,
+ GEN6_GDRST)
+ & GEN6_GRDOM_BLT) == 0, 500);
+ DRM_DEBUG_TDR("BCS Reset\n");
+ break;
+
+ case VCS:
+ __raw_i915_write32(dev_priv, GEN6_GDRST, GEN6_GRDOM_MEDIA);
+
+ /* Spin waiting for the device to ack the reset request */
+ ret = wait_for_atomic((__raw_i915_read32(dev_priv,
+ GEN6_GDRST)
+ & GEN6_GRDOM_MEDIA) == 0, 500);
+ DRM_DEBUG_TDR("VCS Reset\n");
+ break;
+
+ case VECS:
+ I915_WRITE_NOTRACE(GEN6_GDRST, GEN6_GRDOM_VEBOX);
+
+ /* Spin waiting for the device to ack the reset request */
+ ret = wait_for_atomic((I915_READ_NOTRACE(GEN6_GDRST)
+ & GEN6_GRDOM_VEBOX) == 0, 500);
+ DRM_DEBUG_TDR("VECS Reset\n");
+ break;
+
+ default:
+ DRM_ERROR("Unexpected engine\n");
+ break;
+ }
+
+ gen6_gt_force_wake_restore(dev_priv);
spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
+
+ /* Do uevent outside of spinlock as uevent can sleep */
+ reset_event[0] = kasprintf(GFP_KERNEL, "RESET RING=%d", engine);
+ kobject_uevent_env(&dev->primary->kdev->kobj,
+ KOBJ_CHANGE, reset_event);
+ kfree(reset_event[0]);
+
return ret;
}
@@ -922,6 +1074,29 @@ int intel_gpu_reset(struct drm_device *dev)
}
}
+int intel_gpu_engine_reset(struct drm_device *dev, enum intel_ring_id engine)
+{
+ /* Reset an individual engine */
+ int ret = -ENODEV;
+
+ if (!dev)
+ return -EINVAL;
+
+ switch (INTEL_INFO(dev)->gen) {
+ case 7:
+ case 6:
+ ret = gen6_do_engine_reset(dev, engine);
+ break;
+ default:
+ DRM_ERROR("Engine reset not supported\n");
+ ret = -ENODEV;
+ break;
+ }
+
+ return ret;
+}
+
+
void intel_uncore_clear_errors(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
--
1.8.5.1
^ permalink raw reply related [flat|nested] 2+ messages in thread* Re: [RFC 04/13] drm/i915: Force wake restore for TDR
2013-12-16 16:02 [RFC 04/13] drm/i915: Force wake restore for TDR Lister, Ian
@ 2013-12-16 17:32 ` Daniel Vetter
0 siblings, 0 replies; 2+ messages in thread
From: Daniel Vetter @ 2013-12-16 17:32 UTC (permalink / raw)
To: Lister, Ian; +Cc: intel-gfx@lists.freedesktop.org
On Mon, Dec 16, 2013 at 04:02:49PM +0000, Lister, Ian wrote:
> From 698fdaf8a85fb7039dacdff3068be2018a486816 Mon Sep 17 00:00:00 2001
> Message-Id: <698fdaf8a85fb7039dacdff3068be2018a486816.1387201899.git.ian.lister@intel.com>
> In-Reply-To: <cover.1387201899.git.ian.lister@intel.com>
> References: <cover.1387201899.git.ian.lister@intel.com>
> From: ian-lister <ian.lister@intel.com>
> Date: Mon, 9 Dec 2013 09:47:20 +0000
> Subject: [RFC 04/13] drm/i915: Force wake restore for TDR
>
> Abstracted force wake restore code from gen6_do_reset into its own function
> "gen6_gt_force_wake_restore" as it will also be needed for per-engine reset.
> It has been expanded to cope with the Valleyview specific forcewake restore.
>
> Signed-off-by: ian-lister <ian.lister@intel.com>
Some comments inline.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_drv.h | 4 +
> drivers/gpu/drm/i915/i915_reg.h | 1 +
> drivers/gpu/drm/i915/intel_ringbuffer.h | 1 +
> drivers/gpu/drm/i915/intel_uncore.c | 197 ++++++++++++++++++++++++++++++--
> 4 files changed, 192 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f261ab5..b9c4876 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -459,6 +459,8 @@ struct intel_uncore {
> unsigned fw_rendercount;
> unsigned fw_mediacount;
>
> + unsigned ecobus;
This shouldn't be needed since we should assign proper vfunc entries at
init time. If I'm wrong and we indeed need this pls give it a descriptive
name like has_mt_forcewake or similar. Plus one-line comment here.
But on a quick read through of the patch it looks unneeded. If a later
patch needs it the commit message should mention this.
> +
> struct delayed_work force_wake_work;
> };
>
> @@ -1927,6 +1929,8 @@ extern int i915_emit_box(struct drm_device *dev,
> struct drm_clip_rect *box,
> int DR1, int DR4);
> extern int intel_gpu_reset(struct drm_device *dev);
> +extern int intel_gpu_engine_reset(struct drm_device *dev,
> + enum intel_ring_id engine);
> extern int i915_reset(struct drm_device *dev);
> extern unsigned long i915_chipset_val(struct drm_i915_private *dev_priv);
> extern unsigned long i915_mch_val(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index bda7562..e896bca 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -107,6 +107,7 @@
> #define GEN6_GRDOM_RENDER (1 << 1)
> #define GEN6_GRDOM_MEDIA (1 << 2)
> #define GEN6_GRDOM_BLT (1 << 3)
> +#define GEN6_GRDOM_VEBOX (1 << 4)
>
> #define RING_PP_DIR_BASE(ring) ((ring)->mmio_base+0x228)
> #define RING_PP_DIR_BASE_READ(ring) ((ring)->mmio_base+0x518)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index c15b97b..6417de1 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -65,6 +65,7 @@ struct intel_ring_hangcheck {
> u32 hd;
> int score;
> enum intel_ring_hangcheck_action action;
> + u32 resets; /* Total resets applied to this ring/engine*/
> };
>
> struct intel_ring_buffer {
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 848bdaf..764e2af 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -283,6 +283,44 @@ void vlv_force_wake_put(struct drm_i915_private *dev_priv,
> spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> }
>
> +void vlv_force_wake_restore(struct drm_i915_private *dev_priv,
> + int fw_engine)
> +{
> + /* Restore the current force wake state with the hardware
> + * WARNING: Caller *MUST* hold uncore.lock whilst calling this function
> + */
> +
> + if (FORCEWAKE_RENDER & fw_engine) {
> + if (dev_priv->uncore.fw_rendercount
> + && (__raw_i915_read32(dev_priv, FORCEWAKE_ACK_VLV)
> + & FORCEWAKE_KERNEL) == 0)
> + dev_priv->uncore.funcs.force_wake_get(dev_priv,
> + FORCEWAKE_RENDER);
> + else if (!dev_priv->uncore.fw_rendercount
> + && (__raw_i915_read32(dev_priv, FORCEWAKE_ACK_VLV)
> + & FORCEWAKE_KERNEL) == 1)
> + dev_priv->uncore.funcs.force_wake_put(dev_priv,
> + FORCEWAKE_RENDER);
> + }
> +
> + if (FORCEWAKE_MEDIA & fw_engine) {
> + if (dev_priv->uncore.fw_mediacount
> + && (__raw_i915_read32(dev_priv, FORCEWAKE_ACK_MEDIA_VLV)
> + & FORCEWAKE_KERNEL) == 0)
> + dev_priv->uncore.funcs.force_wake_get(dev_priv,
> + FORCEWAKE_MEDIA);
> + else if (!dev_priv->uncore.fw_mediacount
> + && (__raw_i915_read32(dev_priv, FORCEWAKE_ACK_MEDIA_VLV)
> + & FORCEWAKE_KERNEL) == 1)
> + dev_priv->uncore.funcs.force_wake_put(dev_priv,
> + FORCEWAKE_MEDIA);
> + }
> +
> + dev_priv->uncore.fifo_count =
> + __raw_i915_read32(dev_priv, GTFIFOCTL)
> + & GT_FIFO_FREE_ENTRIES_MASK;
> +}
Can't we have a generic forcewake restore function which does the right
thing depending upon domains? I.e. create a new function
intel_uncore_forcewake_restore which dtrt ... By the looks of it the
current code in gen6_do_reset is already broken on vlv.
Bonus points if this function is shared with the normal reset code (i.e.
the global reset) in gen6_do_reset.
> +
> static void gen6_force_wake_work(struct work_struct *work)
> {
> struct drm_i915_private *dev_priv =
> @@ -397,6 +435,50 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
> spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> }
>
> +void gen6_gt_force_wake_restore(struct drm_i915_private *dev_priv)
> +{
> + /* Restore the current expected force wake state with the
> + * hardware. This may be required following a reset.
> + *
> + * WARNING: Caller *MUST* hold uncore.lock
> + *
> + * uncore.lock isn't taken in this function to allow the caller the
> + * flexibility to do other work immediately before/after
> + * whilst holding the lock.
> + */
> + struct drm_device *dev = dev_priv->dev;
> + u32 forcewake_ack;
> +
> + if (IS_VALLEYVIEW(dev))
> + return vlv_force_wake_restore(dev_priv, FORCEWAKE_ALL);
> +
> + if (IS_HASWELL(dev) || IS_GEN8(dev))
> + forcewake_ack = FORCEWAKE_ACK_HSW;
> + else if (IS_IVYBRIDGE(dev)
> + && (dev_priv->uncore.ecobus & FORCEWAKE_MT_ENABLE))
> + forcewake_ack = FORCEWAKE_MT_ACK;
> + else
> + forcewake_ack = 0;
> +
> + if (dev_priv->uncore.forcewake_count
> + && (!forcewake_ack
> + || ((__raw_i915_read32(dev_priv, forcewake_ack)
> + & FORCEWAKE_KERNEL) == 0))) {
> + /* It was enabled, so re-enable it */
> + dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> + } else if ((dev_priv->uncore.forcewake_count == 0)
> + && (!forcewake_ack
> + || ((__raw_i915_read32(dev_priv, forcewake_ack)
> + & FORCEWAKE_KERNEL) == 1))) {
> + /* It was disabled, so disable it */
> + dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
> + }
> +
> + dev_priv->uncore.fifo_count =
> + __raw_i915_read32(dev_priv, GTFIFOCTL)
> + & GT_FIFO_FREE_ENTRIES_MASK;
> +}
> +
> /* We give fast paths for the really cool registers */
> #define NEEDS_FORCE_WAKE(dev_priv, reg) \
> ((reg) < 0x40000 && (reg) != FORCEWAKE)
> @@ -661,8 +743,6 @@ void intel_uncore_init(struct drm_device *dev)
> dev_priv->uncore.funcs.force_wake_get = __gen6_gt_force_wake_mt_get;
> dev_priv->uncore.funcs.force_wake_put = __gen6_gt_force_wake_mt_put;
> } else if (IS_IVYBRIDGE(dev)) {
> - u32 ecobus;
> -
> /* IVB configs may use multi-threaded forcewake */
>
> /* A small trick here - if the bios hasn't configured
> @@ -674,11 +754,11 @@ void intel_uncore_init(struct drm_device *dev)
> */
> mutex_lock(&dev->struct_mutex);
> __gen6_gt_force_wake_mt_get(dev_priv, FORCEWAKE_ALL);
> - ecobus = __raw_i915_read32(dev_priv, ECOBUS);
> + dev_priv->uncore.ecobus = __raw_i915_read32(dev_priv, ECOBUS);
> __gen6_gt_force_wake_mt_put(dev_priv, FORCEWAKE_ALL);
> mutex_unlock(&dev->struct_mutex);
>
> - if (ecobus & FORCEWAKE_MT_ENABLE) {
> + if (dev_priv->uncore.ecobus & FORCEWAKE_MT_ENABLE) {
> dev_priv->uncore.funcs.force_wake_get =
> __gen6_gt_force_wake_mt_get;
> dev_priv->uncore.funcs.force_wake_put =
> @@ -897,16 +977,88 @@ static int gen6_do_reset(struct drm_device *dev)
>
> intel_uncore_forcewake_reset(dev);
>
> - /* If reset with a user forcewake, try to restore, otherwise turn it off */
> - if (dev_priv->uncore.forcewake_count)
> - dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> - else
> - dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
> + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> + return ret;
> +}
> +
> +static int gen6_do_engine_reset(struct drm_device *dev,
> + enum intel_ring_id engine)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_ring_buffer *ring = &dev_priv->ring[engine];
> + int ret = -ENODEV;
> + unsigned long irqflags;
> + char *reset_event[2];
> + reset_event[1] = NULL;
>
> - /* Restore fifo count */
> - dev_priv->uncore.fifo_count = __raw_i915_read32(dev_priv, GTFIFOCTL) & GT_FIFO_FREE_ENTRIES_MASK;
> + DRM_DEBUG_TDR("Engine reset %d\n", engine);
> +
> + /* Hold uncore.lock across reset to prevent any register access
> + * with forcewake not set correctly
> + */
> + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> +
> + ring->hangcheck.resets++;
> +
> + /* Reset the engine.
> + * GEN6_GDRST is not in the gt power well so no need to check
> + * for fifo space for the write or forcewake the chip for
> + * the read.
> + */
> + switch (engine) {
> + case RCS:
> + __raw_i915_write32(dev_priv, GEN6_GDRST, GEN6_GRDOM_RENDER);
> + /* Spin waiting for the device to ack the reset request */
> + ret = wait_for_atomic((__raw_i915_read32(dev_priv,
> + GEN6_GDRST)
> + & GEN6_GRDOM_RENDER) == 0, 500);
> + DRM_DEBUG_TDR("RCS Reset\n");
> + break;
> +
> + case BCS:
> + __raw_i915_write32(dev_priv, GEN6_GDRST, GEN6_GRDOM_BLT);
> +
> + /* Spin waiting for the device to ack the reset request */
> + ret = wait_for_atomic((__raw_i915_read32(dev_priv,
> + GEN6_GDRST)
> + & GEN6_GRDOM_BLT) == 0, 500);
> + DRM_DEBUG_TDR("BCS Reset\n");
> + break;
> +
> + case VCS:
> + __raw_i915_write32(dev_priv, GEN6_GDRST, GEN6_GRDOM_MEDIA);
> +
> + /* Spin waiting for the device to ack the reset request */
> + ret = wait_for_atomic((__raw_i915_read32(dev_priv,
> + GEN6_GDRST)
> + & GEN6_GRDOM_MEDIA) == 0, 500);
> + DRM_DEBUG_TDR("VCS Reset\n");
> + break;
> +
> + case VECS:
> + I915_WRITE_NOTRACE(GEN6_GDRST, GEN6_GRDOM_VEBOX);
> +
> + /* Spin waiting for the device to ack the reset request */
> + ret = wait_for_atomic((I915_READ_NOTRACE(GEN6_GDRST)
> + & GEN6_GRDOM_VEBOX) == 0, 500);
> + DRM_DEBUG_TDR("VECS Reset\n");
> + break;
> +
> + default:
> + DRM_ERROR("Unexpected engine\n");
> + break;
> + }
> +
> + gen6_gt_force_wake_restore(dev_priv);
>
> spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +
> + /* Do uevent outside of spinlock as uevent can sleep */
> + reset_event[0] = kasprintf(GFP_KERNEL, "RESET RING=%d", engine);
> + kobject_uevent_env(&dev->primary->kdev->kobj,
> + KOBJ_CHANGE, reset_event);
> + kfree(reset_event[0]);
> +
> return ret;
> }
If your patch says "forcewake restore support" it shouldn't contain other
stuff like the per-engine reset code here. Or you need to adjust the
commit headline, but I think splitting out the forcewake dance makes
sense.
>
> @@ -922,6 +1074,29 @@ int intel_gpu_reset(struct drm_device *dev)
> }
> }
>
> +int intel_gpu_engine_reset(struct drm_device *dev, enum intel_ring_id engine)
> +{
> + /* Reset an individual engine */
> + int ret = -ENODEV;
> +
> + if (!dev)
> + return -EINVAL;
We don't cather to driver bugs, so a BUG_ON here is the right choice
(since we'll blow up in a NULL deref later on anyway).
The exception to that rule if it's hard to prove by review that the
invariatn holds (e.g. when the invariant depends upon long-term driver
state), then a WARN_ON+early exit is appropriate. But checking all callers
of this function to pass a proper dev pointer is trivial, so not such a
case.
> +
> + switch (INTEL_INFO(dev)->gen) {
> + case 7:
> + case 6:
> + ret = gen6_do_engine_reset(dev, engine);
> + break;
> + default:
> + DRM_ERROR("Engine reset not supported\n");
> + ret = -ENODEV;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +
> void intel_uncore_clear_errors(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> --
> 1.8.5.1
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2013-12-16 17:32 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-16 16:02 [RFC 04/13] drm/i915: Force wake restore for TDR Lister, Ian
2013-12-16 17:32 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox