[PATCH 0/5] Dynamic Parity Detection/Correction

[PATCH 0/5] Dynamic Parity Detection/Correction

[Ben Widawsky]

Unfortunately dinq is not working on my IVB at this moment, so I was forced to
base these patches on din ie. that's why I've added Chris' patch to the series
manually.

Regarding whether or not to actually upstream these patches, I think it
would be awesome if distros could let us know how interested they are in
incorporating this. It is of particular use for any applications using
the GPU for compute. Even if distros don't want it, have the
uevent/interrupt is nice to incorporate, but I would think twice about
the sysfs interface.

Now for the explanation (you may want to get a coffee first):
Internal to the GPU is a cache referred to in docs as L3. The smallest
unit of the cache which is addressable is called a row. There are x rows
in each subbank, and y subbanks in each of the z banks.

HW provides two extra rows per subbank, and a software mechanism to
remap these rows. The addressing after remapping is handled
transparently to software. There is also an interrupt generated by the
render CS to tell us when a parity error occurs in one of the rows.

There is one portion currently unimplemented in the series; we are
required to issue a GPU reset before we remap a row. The documents I
have do not make it clear *exactly* why the gpu reset must occur, but I
believe, similar to Linux, it is the windows mechanism for basically
telling GPU clients that whatever work they've submitted needs to be
resubmitted.

There are various clients which use the L3, however none of these should
be utilized during simple modeset/fbcon.  Therefore, I believe the
following algorithm is guaranteed to work:
1. On boot check some non-volatile storage for bad r/b/s
2. load i915
3. disable bad rbs ASAP
4. Wait forever for uevent of bad r/b/s
5. store r/b/s in some non-volatile storage
6. reboot; goto 1

If we had the reset working, we could avoid the reboot, and instead do:
1. On boot check some non-volatile storage for bad r/b/s
2. load i915
3. disable bad rbs ASAP
4. Wait forever for uevent of bad r/b/s
5. store r/b/s in some non-volatile storage
6. gpu reset; goto 3

The reset is essentially used to "automatically" make all GPU clients
aware that they may need to resubmit their data. The problem with
algorithm #2 without the reset is that there is no way (afaict) to map
the RBS to a BO, and so we have no way to even figure out if the bad
data was propagated to the BO. So an alternative to reset is if system
software detects the uevent, it can send a signal to all known (or
computation based) GPU clients.

See the intel-gpu-tools app as a reference for how to use the sysfs
interface.

Ben Widawsky (4):
  drm/i915: Dynamic Parity Detection handling
  drm/i915: enable parity error interrupts
  drm/i915: remap l3 on hw init
  drm/i915: l3 parity sysfs interface

Chris Wilson (1):
  drm/i915: Use a global lock for modifying global irq flags

 drivers/gpu/drm/i915/i915_drv.h         |    5 ++
 drivers/gpu/drm/i915/i915_gem.c         |   26 +++++++
 drivers/gpu/drm/i915/i915_irq.c         |   87 ++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_reg.h         |   20 +++++
 drivers/gpu/drm/i915/i915_sysfs.c       |  128 ++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_ringbuffer.c |   45 +++++++----
 drivers/gpu/drm/i915/intel_ringbuffer.h |    3 +-
 7 files changed, 293 insertions(+), 21 deletions(-)

-- 
1.7.10

[PATCH 1/5] drm/i915: Use a global lock for modifying global irq flags

[Ben Widawsky]

From: Chris Wilson <chris@chris-wilson.co.uk>

We were attempting to use a per-ring spinlock whilst modifying global
IRQ flags. A recipe for rare missed interrupts.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Acked-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Conflicts:

	drivers/gpu/drm/i915/intel_ringbuffer.c
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |   31 ++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_ringbuffer.h |    3 +--
 2 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 12d9bc7..3b38157 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -613,17 +613,18 @@ gen5_ring_get_irq(struct intel_ring_buffer *ring)
 {
 	struct drm_device *dev = ring->dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
+	unsigned long flags;
 
 	if (!dev->irq_enabled)
 		return false;
 
-	spin_lock(&ring->irq_lock);
+	spin_lock_irqsave(&dev_priv->irq_lock, flags);
 	if (ring->irq_refcount++ == 0) {
 		dev_priv->gt_irq_mask &= ~ring->irq_enable_mask;
 		I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
 		POSTING_READ(GTIMR);
 	}
-	spin_unlock(&ring->irq_lock);
+	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
 
 	return true;
 }
@@ -633,14 +634,15 @@ gen5_ring_put_irq(struct intel_ring_buffer *ring)
 {
 	struct drm_device *dev = ring->dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
+	unsigned long flags;
 
-	spin_lock(&ring->irq_lock);
+	spin_lock_irqsave(&dev_priv->irq_lock, flags);
 	if (--ring->irq_refcount == 0) {
 		dev_priv->gt_irq_mask |= ring->irq_enable_mask;
 		I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
 		POSTING_READ(GTIMR);
 	}
-	spin_unlock(&ring->irq_lock);
+	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
 }
 
 static bool
@@ -648,17 +650,18 @@ i9xx_ring_get_irq(struct intel_ring_buffer *ring)
 {
 	struct drm_device *dev = ring->dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
+	unsigned long flags;
 
 	if (!dev->irq_enabled)
 		return false;
 
-	spin_lock(&ring->irq_lock);
+	spin_lock_irqsave(&dev_priv->irq_lock, flags);
 	if (ring->irq_refcount++ == 0) {
 		dev_priv->irq_mask &= ~ring->irq_enable_mask;
 		I915_WRITE(IMR, dev_priv->irq_mask);
 		POSTING_READ(IMR);
 	}
-	spin_unlock(&ring->irq_lock);
+	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
 
 	return true;
 }
@@ -668,14 +671,15 @@ i9xx_ring_put_irq(struct intel_ring_buffer *ring)
 {
 	struct drm_device *dev = ring->dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
+	unsigned long flags;
 
-	spin_lock(&ring->irq_lock);
+	spin_lock_irqsave(&dev_priv->irq_lock, flags);
 	if (--ring->irq_refcount == 0) {
 		dev_priv->irq_mask |= ring->irq_enable_mask;
 		I915_WRITE(IMR, dev_priv->irq_mask);
 		POSTING_READ(IMR);
 	}
-	spin_unlock(&ring->irq_lock);
+	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
 }
 
 void intel_ring_setup_status_page(struct intel_ring_buffer *ring)
@@ -754,6 +758,7 @@ gen6_ring_get_irq(struct intel_ring_buffer *ring)
 {
 	struct drm_device *dev = ring->dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
+	unsigned long flags;
 
 	if (!dev->irq_enabled)
 	       return false;
@@ -763,14 +768,14 @@ gen6_ring_get_irq(struct intel_ring_buffer *ring)
 	 * blt/bsd rings on ivb. */
 	gen6_gt_force_wake_get(dev_priv);
 
-	spin_lock(&ring->irq_lock);
+	spin_lock_irqsave(&dev_priv->irq_lock, flags);
 	if (ring->irq_refcount++ == 0) {
 		I915_WRITE_IMR(ring, ~ring->irq_enable_mask);
 		dev_priv->gt_irq_mask &= ~ring->irq_enable_mask;
 		I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
 		POSTING_READ(GTIMR);
 	}
-	spin_unlock(&ring->irq_lock);
+	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
 
 	return true;
 }
@@ -780,15 +785,16 @@ gen6_ring_put_irq(struct intel_ring_buffer *ring)
 {
 	struct drm_device *dev = ring->dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
+	unsigned long flags;
 
-	spin_lock(&ring->irq_lock);
+	spin_lock_irqsave(&dev_priv->irq_lock, flags);
 	if (--ring->irq_refcount == 0) {
 		I915_WRITE_IMR(ring, ~0);
 		dev_priv->gt_irq_mask |= ring->irq_enable_mask;
 		I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
 		POSTING_READ(GTIMR);
 	}
-	spin_unlock(&ring->irq_lock);
+	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
 
 	gen6_gt_force_wake_put(dev_priv);
 }
@@ -922,7 +928,6 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 	ring->size = 32 * PAGE_SIZE;
 
 	init_waitqueue_head(&ring->irq_queue);
-	spin_lock_init(&ring->irq_lock);
 
 	if (I915_NEED_GFX_HWS(dev)) {
 		ret = init_status_page(ring);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 06a66ad..e0b25bb 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -56,8 +56,7 @@ struct  intel_ring_buffer {
 	 */
 	u32		last_retired_head;
 
-	spinlock_t	irq_lock;
-	u32		irq_refcount;
+	u32		irq_refcount;		/* protected by dev_priv->irq_lock */
 	u32		irq_enable_mask;	/* bitmask to enable ring interrupt */
 	u32		irq_seqno;		/* last seq seem at irq time */
 	u32		trace_irq_seqno;
-- 
1.7.10

[PATCH 2/5] drm/i915: Dynamic Parity Detection handling

[Ben Widawsky]

On IVB hardware we are given an interrupt whenever a L3 parity error
occurs in the L3 cache. The L3 cache is used by internal GPU clients
only.  This is a very rare occurrence (in fact to test this I need to
use specially instrumented silicon).

When a row in the L3 cache detects a parity error the HW generates an
interrupt. The interrupt is masked in GTIMR until we get a chance to
read some registers and alert userspace via a uevent. With this
information userspace can use a sysfs interface (follow-up patch) to
remap those rows.

Way above my level of understanding, but if a given row fails, it is
statistically more likely to fail again than a row which has not failed.
Therefore it is desirable for an operating system to maintain a lifelong
list of failing rows and always remap any bad rows on driver load.
Hardware limits the number of rows that are remappable per bank/subbank,
and should more than that many rows detect parity errors, software
should maintain a list of the most frequent errors, and remap those
rows.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h |    2 +
 drivers/gpu/drm/i915/i915_irq.c |   83 +++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h |   17 ++++++++
 3 files changed, 102 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 69e1539..9505fc0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -804,6 +804,8 @@ typedef struct drm_i915_private {
 
 	struct drm_property *broadcast_rgb_property;
 	struct drm_property *force_audio_property;
+
+	struct work_struct parity_error_work;
 } drm_i915_private_t;
 
 enum hdmi_force_audio {
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ab023ca..81e5a7d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -430,6 +430,83 @@ static void gen6_pm_rps_work(struct work_struct *work)
 	mutex_unlock(&dev_priv->dev->struct_mutex);
 }
 
+
+/**
+ * ivybridge_parity_work - Workqueue called when a parity error interrupt
+ * occurred.
+ *
+ * Doesn't actually do anything except notify userspace so that userspace may
+ * disable things later on.
+ */
+static void ivybridge_parity_work(struct work_struct *work)
+{
+	drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
+						    parity_error_work);
+
+	u32 error_status, row, bank, subbank;
+	char *parity_event[5];
+	uint32_t misccpctl;
+	unsigned long flags;
+
+	/* We must turn off DOP level clock gating to access the L3 registers.
+	 * In order to prevent a get/put style interface, acquire struct mutex
+	 * any time we access those registers.
+	 */
+	mutex_lock(&dev_priv->dev->struct_mutex);
+
+	misccpctl = I915_READ(GEN7_MISCCPCTL);
+	I915_WRITE(GEN7_MISCCPCTL, misccpctl & ~GEN7_DOP_CLOCK_GATE_ENABLE);
+	POSTING_READ(GEN7_MISCCPCTL);
+
+	error_status = I915_READ(GEN7_L3CDERRST1);
+	row = GEN7_PARITY_ERROR_ROW(error_status);
+	bank = GEN7_PARITY_ERROR_BANK(error_status);
+	subbank = GEN7_PARITY_ERROR_SUBBANK(error_status);
+
+	I915_WRITE(GEN7_L3CDERRST1, GEN7_PARITY_ERROR_VALID |
+				    GEN7_L3CDERRST1_ENABLE);
+	POSTING_READ(GEN7_L3CDERRST1);
+
+	I915_WRITE(GEN7_MISCCPCTL, misccpctl);
+
+	spin_lock_irqsave(&dev_priv->irq_lock, flags);
+	dev_priv->gt_irq_mask &= ~GT_GEN7_L3_PARITY_ERROR_INTERRUPT;
+	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
+	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
+
+	mutex_unlock(&dev_priv->dev->struct_mutex);
+
+	parity_event[0] = "L3_PARITY_ERROR=1";
+	parity_event[1] = kasprintf(GFP_KERNEL, "ROW=%d", row);
+	parity_event[2] = kasprintf(GFP_KERNEL, "BANK=%d", bank);
+	parity_event[3] = kasprintf(GFP_KERNEL, "SUBBANK=%d", subbank);
+	parity_event[4] = NULL;
+
+	kobject_uevent_env(&dev_priv->dev->primary->kdev.kobj,
+			   KOBJ_CHANGE, parity_event);
+
+	kfree(parity_event[3]);
+	kfree(parity_event[2]);
+	kfree(parity_event[1]);
+}
+
+void ivybridge_handle_parity_error(struct drm_device *dev)
+{
+	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
+	unsigned long flags;
+
+	if (WARN_ON(IS_GEN6(dev)))
+		return;
+
+	spin_lock_irqsave(&dev_priv->irq_lock, flags);
+	dev_priv->gt_irq_mask |= GT_GEN7_L3_PARITY_ERROR_INTERRUPT;
+	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
+	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
+
+	queue_work(dev_priv->wq, &dev_priv->parity_error_work);
+	DRM_INFO("Parity error interrupt. Scheduling work\n");
+}
+
 static void snb_gt_irq_handler(struct drm_device *dev,
 			       struct drm_i915_private *dev_priv,
 			       u32 gt_iir)
@@ -449,6 +526,9 @@ static void snb_gt_irq_handler(struct drm_device *dev,
 		DRM_ERROR("GT error interrupt 0x%08x\n", gt_iir);
 		i915_handle_error(dev, false);
 	}
+
+	if (gt_iir & GT_GEN7_L3_PARITY_ERROR_INTERRUPT)
+		ivybridge_handle_parity_error(dev);
 }
 
 static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,
@@ -1978,6 +2058,9 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 	if (IS_GEN6(dev) || IS_IVYBRIDGE(dev))
 		INIT_WORK(&dev_priv->rps_work, gen6_pm_rps_work);
 
+	if (IS_IVYBRIDGE(dev))
+		INIT_WORK(&dev_priv->parity_error_work, ivybridge_parity_work);
+
 	I915_WRITE(HWSTAM, 0xeffe);
 
 	/* XXX hotplug from PCH */
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5ac9837..72db6a9 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4030,6 +4030,23 @@
 #define   GEN6_RC6			3
 #define   GEN6_RC7			4
 
+#define GEN7_MISCCPCTL			(0x9424)
+#define   GEN7_DOP_CLOCK_GATE_ENABLE	(1<<0)
+
+/* IVYBRIDGE DPF */
+#define GEN7_L3CDERRST1			0xB008 /* L3CD Error Status 1 */
+#define   GEN7_L3CDERRST1_ROW_MASK	(0x7ff<<14)
+#define   GEN7_PARITY_ERROR_VALID	(1<<13)
+#define   GEN7_L3CDERRST1_BANK_MASK	(3<<11)
+#define   GEN7_L3CDERRST1_SUBBANK_MASK	(7<<8)
+#define GEN7_PARITY_ERROR_ROW(reg) \
+		((reg & GEN7_L3CDERRST1_ROW_MASK) >> 14)
+#define GEN7_PARITY_ERROR_BANK(reg) \
+		((reg & GEN7_L3CDERRST1_BANK_MASK) >> 11)
+#define GEN7_PARITY_ERROR_SUBBANK(reg) \
+		((reg & GEN7_L3CDERRST1_SUBBANK_MASK) >> 8)
+#define   GEN7_L3CDERRST1_ENABLE	(1<<7)
+
 #define G4X_AUD_VID_DID			0x62020
 #define INTEL_AUDIO_DEVCL		0x808629FB
 #define INTEL_AUDIO_DEVBLC		0x80862801
-- 
1.7.10

[PATCH 3/5] drm/i915: enable parity error interrupts

[Ben Widawsky]

The previous patch put all the code, and handlers in place. It should
now be safe to enable the parity error interrupt. The parity error must
be unmasked in both the GTIMR, and the CS IMR. Unfortunately, the docs
aren't clear about this; nevertheless it's the truth.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_irq.c         |    4 ++--
 drivers/gpu/drm/i915/intel_ringbuffer.c |   14 ++++++++++++--
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 81e5a7d..334e1b3 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2235,13 +2235,13 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
 		   DE_PIPEB_VBLANK_IVB);
 	POSTING_READ(DEIER);
 
-	dev_priv->gt_irq_mask = ~0;
+	dev_priv->gt_irq_mask = ~GT_GEN7_L3_PARITY_ERROR_INTERRUPT;
 
 	I915_WRITE(GTIIR, I915_READ(GTIIR));
 	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
 
 	render_irqs = GT_USER_INTERRUPT | GEN6_BSD_USER_INTERRUPT |
-		GEN6_BLITTER_USER_INTERRUPT;
+		GEN6_BLITTER_USER_INTERRUPT | GT_GEN7_L3_PARITY_ERROR_INTERRUPT;
 	I915_WRITE(GTIER, render_irqs);
 	POSTING_READ(GTIER);
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 3b38157..02762b1 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -420,6 +420,9 @@ static int init_render_ring(struct intel_ring_buffer *ring)
 			   INSTPM_FORCE_ORDERING << 16 | INSTPM_FORCE_ORDERING);
 	}
 
+	if (IS_IVYBRIDGE(dev))
+		I915_WRITE_IMR(ring, ~GEN6_RENDER_L3_PARITY_ERROR);
+
 	return ret;
 }
 
@@ -770,7 +773,11 @@ gen6_ring_get_irq(struct intel_ring_buffer *ring)
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
 	if (ring->irq_refcount++ == 0) {
-		I915_WRITE_IMR(ring, ~ring->irq_enable_mask);
+		if (IS_IVYBRIDGE(dev) && ring->id == RCS)
+			I915_WRITE_IMR(ring, ~(ring->irq_enable_mask |
+						GEN6_RENDER_L3_PARITY_ERROR));
+		else
+			I915_WRITE_IMR(ring, ~ring->irq_enable_mask);
 		dev_priv->gt_irq_mask &= ~ring->irq_enable_mask;
 		I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
 		POSTING_READ(GTIMR);
@@ -789,7 +796,10 @@ gen6_ring_put_irq(struct intel_ring_buffer *ring)
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
 	if (--ring->irq_refcount == 0) {
-		I915_WRITE_IMR(ring, ~0);
+		if (IS_IVYBRIDGE(dev) && ring->id == RCS)
+			I915_WRITE_IMR(ring, ~GEN6_RENDER_L3_PARITY_ERROR);
+		else
+			I915_WRITE_IMR(ring, ~0);
 		dev_priv->gt_irq_mask |= ring->irq_enable_mask;
 		I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
 		POSTING_READ(GTIMR);
-- 
1.7.10

[PATCH 4/5] drm/i915: remap l3 on hw init

[Ben Widawsky]

If any l3 rows have been previously remapped, we must remap them after
GPU reset/resume too.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h |    3 +++
 drivers/gpu/drm/i915/i915_gem.c |   26 ++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h |    3 +++
 3 files changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9505fc0..e9efe17 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -641,6 +641,8 @@ typedef struct drm_i915_private {
 		/** PPGTT used for aliasing the PPGTT with the GTT */
 		struct i915_hw_ppgtt *aliasing_ppgtt;
 
+		u32 *l3_remap_info;
+
 		struct shrinker inactive_shrinker;
 
 		/**
@@ -1290,6 +1292,7 @@ int __must_check i915_gem_object_set_domain(struct drm_i915_gem_object *obj,
 					    uint32_t write_domain);
 int __must_check i915_gem_object_finish_gpu(struct drm_i915_gem_object *obj);
 int __must_check i915_gem_init_hw(struct drm_device *dev);
+void i915_gem_l3_remap(struct drm_device *dev);
 void i915_gem_init_swizzling(struct drm_device *dev);
 void i915_gem_init_ppgtt(struct drm_device *dev);
 void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7bc4a40..bb3ef9f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3475,6 +3475,30 @@ i915_gem_idle(struct drm_device *dev)
 	return 0;
 }
 
+void i915_gem_l3_remap(struct drm_device *dev)
+{
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	u32 misccpctl;
+	int i;
+
+	if (!dev_priv->mm.l3_remap_info)
+		return;
+
+	WARN_ON(!IS_IVYBRIDGE(dev));
+
+	misccpctl = I915_READ(GEN7_MISCCPCTL);
+	I915_WRITE(GEN7_MISCCPCTL, misccpctl & ~GEN7_DOP_CLOCK_GATE_ENABLE);
+	POSTING_READ(GEN7_MISCCPCTL);
+
+	for (i = 0; i < GEN7_L3LOG_SIZE; i += 4)
+		I915_WRITE(GEN7_L3LOG_BASE + i, dev_priv->mm.l3_remap_info[i/4]);
+
+	/* Make sure all the writes land before disabling dop clock gating */
+	POSTING_READ(GEN7_L3LOG_BASE);
+
+	I915_WRITE(GEN7_MISCCPCTL, misccpctl);
+}
+
 void i915_gem_init_swizzling(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
@@ -3566,6 +3590,8 @@ i915_gem_init_hw(struct drm_device *dev)
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	int ret;
 
+	i915_gem_l3_remap(dev);
+
 	i915_gem_init_swizzling(dev);
 
 	ret = intel_init_render_ring_buffer(dev);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 72db6a9..8c546fd 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4047,6 +4047,9 @@
 		((reg & GEN7_L3CDERRST1_SUBBANK_MASK) >> 8)
 #define   GEN7_L3CDERRST1_ENABLE	(1<<7)
 
+#define GEN7_L3LOG_BASE			0xB070
+#define GEN7_L3LOG_SIZE			0x80
+
 #define G4X_AUD_VID_DID			0x62020
 #define INTEL_AUDIO_DEVCL		0x808629FB
 #define INTEL_AUDIO_DEVBLC		0x80862801
-- 
1.7.10

[PATCH 5/5] drm/i915: l3 parity sysfs interface

[Ben Widawsky]

Dumb binary interfaces which allow root-only updates of the cache
remapping registers. As mentioned in a previous patch, software using
this interface needs to know about HW limits, and other programming
considerations as the kernel interface does no checking for these things
on the root-only interface.

v1: Drop extra posting reads (Chris)
Return negative values in the sysfs interfaces on errors (Chris)

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_sysfs.c |  128 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 126 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index 79f8344..ed77cbf 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -29,6 +29,7 @@
 #include <linux/module.h>
 #include <linux/stat.h>
 #include <linux/sysfs.h>
+#include "intel_drv.h"
 #include "i915_drv.h"
 
 static u32 calc_residency(struct drm_device *dev, const u32 reg)
@@ -92,20 +93,143 @@ static struct attribute_group rc6_attr_group = {
 	.attrs =  rc6_attrs
 };
 
+static int l3_access_valid(struct drm_device *dev, loff_t offset)
+{
+	if (!IS_IVYBRIDGE(dev))
+		return -EPERM;
+
+	if (offset % 4 != 0)
+		return -EPERM;
+
+	if (offset >= GEN7_L3LOG_SIZE)
+		return -ENXIO;
+
+	return 0;
+}
+
+static ssize_t
+i915_l3_read(struct file *filp, struct kobject *kobj,
+	     struct bin_attribute *attr, char *buf,
+	     loff_t offset, size_t count)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
+	struct drm_device *drm_dev = dminor->dev;
+	struct drm_i915_private *dev_priv = drm_dev->dev_private;
+	uint32_t misccpctl;
+	int i, ret;
+
+	ret = l3_access_valid(drm_dev, offset);
+	if (ret)
+		return ret;
+
+	ret = i915_mutex_lock_interruptible(drm_dev);
+	if (ret)
+		return ret;
+
+	misccpctl = I915_READ(GEN7_MISCCPCTL);
+	I915_WRITE(GEN7_MISCCPCTL, misccpctl & ~GEN7_DOP_CLOCK_GATE_ENABLE);
+
+	for (i = offset; count >= 4 && i < GEN7_L3LOG_SIZE; i += 4, count -= 4)
+		*((uint32_t *)(&buf[i])) = I915_READ(GEN7_L3LOG_BASE + i);
+
+	I915_WRITE(GEN7_MISCCPCTL, misccpctl);
+
+	mutex_unlock(&drm_dev->struct_mutex);
+
+	return i - offset;
+}
+
+static ssize_t
+i915_l3_write(struct file *filp, struct kobject *kobj,
+	      struct bin_attribute *attr, char *buf,
+	      loff_t offset, size_t count)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
+	struct drm_device *drm_dev = dminor->dev;
+	struct drm_i915_private *dev_priv = drm_dev->dev_private;
+	u32 *temp = NULL;
+	int i, ret;
+
+	ret = l3_access_valid(drm_dev, offset);
+	if (ret)
+		return ret;
+
+	ret = i915_mutex_lock_interruptible(drm_dev);
+	if (ret)
+		return ret;
+
+	if (!dev_priv->mm.l3_remap_info) {
+		temp = kzalloc(GEN7_L3LOG_SIZE, GFP_KERNEL);
+		if (!temp) {
+			mutex_unlock(&drm_dev->struct_mutex);
+			return -ENOMEM;
+		}
+	}
+
+	ret = i915_gpu_idle(drm_dev, true);
+	if (ret) {
+		kfree(temp);
+		mutex_unlock(&drm_dev->struct_mutex);
+		return ret;
+	}
+
+	/* TODO: Ideally we really want a GPU reset here to make sure errors
+	 * aren't propagated Since I cannot find a stable way to reset the GPU
+	 * at this point it is left as a TODO.
+	*/
+
+	if (dev_priv->mm.l3_remap_info)
+		temp = dev_priv->mm.l3_remap_info;
+
+	dev_priv->mm.l3_remap_info = temp;
+
+	for (i = offset; count >= 4 && i < GEN7_L3LOG_SIZE; i += 4, count -= 4) {
+		u32 remap = I915_READ(GEN7_L3LOG_BASE + i);
+		if (remap && remap != *temp)
+			DRM_ERROR("0x%x was already programmed to %x\n",
+				  GEN7_L3LOG_BASE + i, remap);
+		*temp++ = *(uint32_t *)(&buf[i]);
+	}
+
+	i915_gem_l3_remap(drm_dev);
+
+	mutex_unlock(&drm_dev->struct_mutex);
+
+	return offset - i;
+}
+
+static struct bin_attribute dpf_attrs = {
+	.attr = {.name = "l3_parity", .mode = (S_IRUSR | S_IWUSR)},
+	.size = GEN7_L3LOG_SIZE,
+	.read = i915_l3_read,
+	.write = i915_l3_write,
+	.mmap = NULL
+};
+
 void i915_setup_sysfs(struct drm_device *dev)
 {
 	int ret;
 
-	/* ILK doesn't have any residency information */
+	/* ILK and below don't yet have relevant sysfs files */
 	if (INTEL_INFO(dev)->gen < 6)
 		return;
 
 	ret = sysfs_merge_group(&dev->primary->kdev.kobj, &rc6_attr_group);
 	if (ret)
-		DRM_ERROR("sysfs setup failed\n");
+		DRM_ERROR("RC6 residency sysfs setup failed\n");
+
+	if (!IS_IVYBRIDGE(dev))
+		return;
+
+	ret = device_create_bin_file(&dev->primary->kdev, &dpf_attrs);
+	if (ret)
+		DRM_ERROR("l3 parity sysfs setup failed\n");
 }
 
 void i915_teardown_sysfs(struct drm_device *dev)
 {
+	device_remove_bin_file(&dev->primary->kdev,  &dpf_attrs);
 	sysfs_unmerge_group(&dev->primary->kdev.kobj, &rc6_attr_group);
 }
-- 
1.7.10

[PATCH] l3 parity tool

[Ben Widawsky]

This tool is used to program, and read the l3 parity information.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 lib/intel_chipset.h     |    2 +
 tools/Makefile.am       |    3 +-
 tools/intel_l3_parity.c |  150 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 154 insertions(+), 1 deletion(-)
 create mode 100644 tools/intel_l3_parity.c

diff --git a/lib/intel_chipset.h b/lib/intel_chipset.h
index 668bf59..7bc5e17 100755
--- a/lib/intel_chipset.h
+++ b/lib/intel_chipset.h
@@ -82,6 +82,7 @@
 #define PCI_CHIP_IVYBRIDGE_GT2	0x0162
 #define PCI_CHIP_IVYBRIDGE_M_GT1	0x0156 /* mobile */
 #define PCI_CHIP_IVYBRIDGE_M_GT2	0x0166
+#define PCI_CHIP_IVYBRIDGE_S_GT2	0x016a
 #define PCI_CHIP_IVYBRIDGE_S		0x015a /* server */
 
 #define IS_MOBILE(devid)	(devid == PCI_CHIP_I855_GM || \
@@ -149,6 +150,7 @@
 				 devid == PCI_CHIP_IVYBRIDGE_GT2 || \
 				 devid == PCI_CHIP_IVYBRIDGE_M_GT1 || \
 				 devid == PCI_CHIP_IVYBRIDGE_M_GT2 || \
+				 devid == PCI_CHIP_IVYBRIDGE_S_GT2 || \
 				 devid == PCI_CHIP_IVYBRIDGE_S)
 
 
diff --git a/tools/Makefile.am b/tools/Makefile.am
index 058835c..d352316 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -14,7 +14,8 @@ bin_PROGRAMS = 				\
 	intel_reg_snapshot 		\
 	intel_reg_write 		\
 	intel_reg_read 			\
-	intel_forcewaked
+	intel_forcewaked		\
+	intel_l3_parity
 
 noinst_PROGRAMS = 			\
 	intel_dump_decode 		\
diff --git a/tools/intel_l3_parity.c b/tools/intel_l3_parity.c
new file mode 100644
index 0000000..1c16903
--- /dev/null
+++ b/tools/intel_l3_parity.c
@@ -0,0 +1,150 @@
+/*
+ * Copyright © 2012 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Ben Widawsky <ben@bwidawsk.net>
+ *
+ */
+
+#define _GNU_SOURCE
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include "drmtest.h"
+
+#define NUM_BANKS 4
+#define NUM_SUBBANKS 8
+#define NUM_REGS (NUM_BANKS * NUM_SUBBANKS)
+
+struct __attribute__ ((__packed__)) l3_log_register {
+	uint32_t row0_enable	: 1;
+	uint32_t rsvd2		: 4;
+	uint32_t row0		: 11;
+	uint32_t row1_enable	: 1;
+	uint32_t rsvd1		: 4;
+	uint32_t row1		: 11;
+} l3log[NUM_BANKS][NUM_SUBBANKS];
+
+static void dumpit(void)
+{
+	int i, j;
+
+	for (i = 0; i < NUM_BANKS; i++) {
+		for (j = 0; j < NUM_SUBBANKS; j++) {
+			struct l3_log_register *reg = &l3log[i][j];
+
+		if (reg->row0_enable)
+			printf("Row %d, Bank %d, Subbank %d is disable\n",
+			       reg->row0, i, j);
+		if (reg->row1_enable)
+			printf("Row %d, Bank %d, Subbank %d is disable\n",
+			       reg->row1, i, j);
+		}
+	}
+}
+
+static int disable_rbs(int row, int bank, int sbank)
+{
+	struct l3_log_register *reg = &l3log[bank][sbank];
+
+	// can't map more than 2 rows
+	if (reg->row0_enable && reg->row1_enable)
+		return -1;
+
+	// can't remap the same row twice
+	if ((reg->row0_enable && reg->row0 == row) ||
+	    (reg->row1_enable && reg->row1 == row)) {
+		return -1;
+	}
+
+	if (reg->row0_enable) {
+		reg->row1 = row;
+		reg->row1_enable = 1;
+	} else {
+		reg->row0 = row;
+		reg->row0_enable = 1;
+	}
+
+	return 0;
+}
+
+static int do_parse(int argc, char *argv[])
+{
+	int row, bank, sbank, i, ret;
+
+	for (i = 1; i < argc; i++) {
+		ret = sscanf(argv[i], "%d,%d,%d", &row, &bank, &sbank);
+		if (ret != 3)
+			return i;
+		assert(disable_rbs(row, bank, sbank) == 0);
+	}
+	return 0;
+}
+
+int main(int argc, char *argv[])
+{
+	const int device = drm_get_card(0);
+	char *path;
+	int fd, ret;
+
+	ret = asprintf(&path, "/sys/class/drm/card%d/l3_parity", device);
+	assert(ret != -1);
+
+	fd = open(path, O_RDWR);
+	if (fd == -1) {
+		perror("Opening sysfs");
+		exit(EXIT_FAILURE);
+	}
+
+	ret = read(fd, l3log, NUM_REGS * sizeof(uint32_t));
+	if (ret == -1) {
+		perror("Reading sysfs");
+		exit(EXIT_FAILURE);
+	}
+
+	assert(lseek(fd, 0, SEEK_SET) == 0);
+
+	if (argc == 1) {
+		dumpit();
+		exit(EXIT_SUCCESS);
+	}
+
+	ret = do_parse(argc, argv);
+	if (ret != 0) {
+		fprintf(stderr, "Malformed command line at %s\n", argv[ret]);
+		exit(EXIT_FAILURE);
+	}
+
+	ret = write(fd, l3log, NUM_REGS * sizeof(uint32_t));
+	if (ret == -1) {
+		perror("Writing sysfs");
+		exit(EXIT_FAILURE);
+	}
+
+	close(fd);
+
+	exit(EXIT_SUCCESS);
+}
-- 
1.7.10

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

Re: [PATCH 2/5] drm/i915: Dynamic Parity Detection handling

[Daniel Vetter]

On Fri, Apr 27, 2012 at 05:40:18PM -0700, Ben Widawsky wrote:
> On IVB hardware we are given an interrupt whenever a L3 parity error
> occurs in the L3 cache. The L3 cache is used by internal GPU clients
> only.  This is a very rare occurrence (in fact to test this I need to
> use specially instrumented silicon).
> 
> When a row in the L3 cache detects a parity error the HW generates an
> interrupt. The interrupt is masked in GTIMR until we get a chance to
> read some registers and alert userspace via a uevent. With this
> information userspace can use a sysfs interface (follow-up patch) to
> remap those rows.
> 
> Way above my level of understanding, but if a given row fails, it is
> statistically more likely to fail again than a row which has not failed.
> Therefore it is desirable for an operating system to maintain a lifelong
> list of failing rows and always remap any bad rows on driver load.
> Hardware limits the number of rows that are remappable per bank/subbank,
> and should more than that many rows detect parity errors, software
> should maintain a list of the most frequent errors, and remap those
> rows.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |    2 +
>  drivers/gpu/drm/i915/i915_irq.c |   83 +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h |   17 ++++++++
>  3 files changed, 102 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 69e1539..9505fc0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -804,6 +804,8 @@ typedef struct drm_i915_private {
>  
>  	struct drm_property *broadcast_rgb_property;
>  	struct drm_property *force_audio_property;
> +
> +	struct work_struct parity_error_work;
>  } drm_i915_private_t;
>  
>  enum hdmi_force_audio {
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index ab023ca..81e5a7d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -430,6 +430,83 @@ static void gen6_pm_rps_work(struct work_struct *work)
>  	mutex_unlock(&dev_priv->dev->struct_mutex);
>  }
>  
> +
> +/**
> + * ivybridge_parity_work - Workqueue called when a parity error interrupt
> + * occurred.
> + *
> + * Doesn't actually do anything except notify userspace so that userspace may
> + * disable things later on.
> + */
> +static void ivybridge_parity_work(struct work_struct *work)
> +{
> +	drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
> +						    parity_error_work);
> +
> +	u32 error_status, row, bank, subbank;
> +	char *parity_event[5];
> +	uint32_t misccpctl;
> +	unsigned long flags;
> +
> +	/* We must turn off DOP level clock gating to access the L3 registers.
> +	 * In order to prevent a get/put style interface, acquire struct mutex
> +	 * any time we access those registers.
> +	 */
> +	mutex_lock(&dev_priv->dev->struct_mutex);
> +
> +	misccpctl = I915_READ(GEN7_MISCCPCTL);
> +	I915_WRITE(GEN7_MISCCPCTL, misccpctl & ~GEN7_DOP_CLOCK_GATE_ENABLE);
> +	POSTING_READ(GEN7_MISCCPCTL);
> +
> +	error_status = I915_READ(GEN7_L3CDERRST1);
> +	row = GEN7_PARITY_ERROR_ROW(error_status);
> +	bank = GEN7_PARITY_ERROR_BANK(error_status);
> +	subbank = GEN7_PARITY_ERROR_SUBBANK(error_status);
> +
> +	I915_WRITE(GEN7_L3CDERRST1, GEN7_PARITY_ERROR_VALID |
> +				    GEN7_L3CDERRST1_ENABLE);
> +	POSTING_READ(GEN7_L3CDERRST1);
> +
> +	I915_WRITE(GEN7_MISCCPCTL, misccpctl);
> +
> +	spin_lock_irqsave(&dev_priv->irq_lock, flags);
> +	dev_priv->gt_irq_mask &= ~GT_GEN7_L3_PARITY_ERROR_INTERRUPT;
> +	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> +	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
> +
> +	mutex_unlock(&dev_priv->dev->struct_mutex);
> +
> +	parity_event[0] = "L3_PARITY_ERROR=1";
> +	parity_event[1] = kasprintf(GFP_KERNEL, "ROW=%d", row);
> +	parity_event[2] = kasprintf(GFP_KERNEL, "BANK=%d", bank);
> +	parity_event[3] = kasprintf(GFP_KERNEL, "SUBBANK=%d", subbank);
> +	parity_event[4] = NULL;
> +
> +	kobject_uevent_env(&dev_priv->dev->primary->kdev.kobj,
> +			   KOBJ_CHANGE, parity_event);
> +
> +	kfree(parity_event[3]);
> +	kfree(parity_event[2]);
> +	kfree(parity_event[1]);
> +}
> +
> +void ivybridge_handle_parity_error(struct drm_device *dev)
> +{
> +	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> +	unsigned long flags;
> +
> +	if (WARN_ON(IS_GEN6(dev)))
> +		return;
> +
> +	spin_lock_irqsave(&dev_priv->irq_lock, flags);
> +	dev_priv->gt_irq_mask |= GT_GEN7_L3_PARITY_ERROR_INTERRUPT;
> +	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> +	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
> +
> +	queue_work(dev_priv->wq, &dev_priv->parity_error_work);
> +	DRM_INFO("Parity error interrupt. Scheduling work\n");
> +}
> +
>  static void snb_gt_irq_handler(struct drm_device *dev,
>  			       struct drm_i915_private *dev_priv,
>  			       u32 gt_iir)
> @@ -449,6 +526,9 @@ static void snb_gt_irq_handler(struct drm_device *dev,
>  		DRM_ERROR("GT error interrupt 0x%08x\n", gt_iir);
>  		i915_handle_error(dev, false);
>  	}
> +
> +	if (gt_iir & GT_GEN7_L3_PARITY_ERROR_INTERRUPT)
> +		ivybridge_handle_parity_error(dev);
>  }
>  
>  static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,
> @@ -1978,6 +2058,9 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
>  	if (IS_GEN6(dev) || IS_IVYBRIDGE(dev))
>  		INIT_WORK(&dev_priv->rps_work, gen6_pm_rps_work);
>  
> +	if (IS_IVYBRIDGE(dev))
> +		INIT_WORK(&dev_priv->parity_error_work, ivybridge_parity_work);
> +

work init has moved to intel_irq_init in dinq, and for good reasons as
I've figured out after merging the patch: _preinstall is also called on
resume, and if we're unlucky we have a work item outstanding from before
the suspend, so that the we re-init a life work item. The core work queue
code doesn't approve of that, resulting in decent hilarity (NULL deref
after suspend).
-Daniel

>  	I915_WRITE(HWSTAM, 0xeffe);
>  
>  	/* XXX hotplug from PCH */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 5ac9837..72db6a9 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4030,6 +4030,23 @@
>  #define   GEN6_RC6			3
>  #define   GEN6_RC7			4
>  
> +#define GEN7_MISCCPCTL			(0x9424)
> +#define   GEN7_DOP_CLOCK_GATE_ENABLE	(1<<0)
> +
> +/* IVYBRIDGE DPF */
> +#define GEN7_L3CDERRST1			0xB008 /* L3CD Error Status 1 */
> +#define   GEN7_L3CDERRST1_ROW_MASK	(0x7ff<<14)
> +#define   GEN7_PARITY_ERROR_VALID	(1<<13)
> +#define   GEN7_L3CDERRST1_BANK_MASK	(3<<11)
> +#define   GEN7_L3CDERRST1_SUBBANK_MASK	(7<<8)
> +#define GEN7_PARITY_ERROR_ROW(reg) \
> +		((reg & GEN7_L3CDERRST1_ROW_MASK) >> 14)
> +#define GEN7_PARITY_ERROR_BANK(reg) \
> +		((reg & GEN7_L3CDERRST1_BANK_MASK) >> 11)
> +#define GEN7_PARITY_ERROR_SUBBANK(reg) \
> +		((reg & GEN7_L3CDERRST1_SUBBANK_MASK) >> 8)
> +#define   GEN7_L3CDERRST1_ENABLE	(1<<7)
> +
>  #define G4X_AUD_VID_DID			0x62020
>  #define INTEL_AUDIO_DEVCL		0x808629FB
>  #define INTEL_AUDIO_DEVBLC		0x80862801
> -- 
> 1.7.10
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 5/5] drm/i915: l3 parity sysfs interface

[Daniel Vetter]

On Fri, Apr 27, 2012 at 05:40:21PM -0700, Ben Widawsky wrote:
> Dumb binary interfaces which allow root-only updates of the cache
> remapping registers. As mentioned in a previous patch, software using
> this interface needs to know about HW limits, and other programming
> considerations as the kernel interface does no checking for these things
> on the root-only interface.
> 
> v1: Drop extra posting reads (Chris)
> Return negative values in the sysfs interfaces on errors (Chris)
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_sysfs.c |  128 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 126 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index 79f8344..ed77cbf 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -29,6 +29,7 @@
>  #include <linux/module.h>
>  #include <linux/stat.h>
>  #include <linux/sysfs.h>
> +#include "intel_drv.h"
>  #include "i915_drv.h"
>  
>  static u32 calc_residency(struct drm_device *dev, const u32 reg)
> @@ -92,20 +93,143 @@ static struct attribute_group rc6_attr_group = {
>  	.attrs =  rc6_attrs
>  };
>  
> +static int l3_access_valid(struct drm_device *dev, loff_t offset)
> +{
> +	if (!IS_IVYBRIDGE(dev))
> +		return -EPERM;
> +
> +	if (offset % 4 != 0)
> +		return -EPERM;
> +
> +	if (offset >= GEN7_L3LOG_SIZE)
> +		return -ENXIO;
> +
> +	return 0;
> +}
> +
> +static ssize_t
> +i915_l3_read(struct file *filp, struct kobject *kobj,
> +	     struct bin_attribute *attr, char *buf,
> +	     loff_t offset, size_t count)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
> +	struct drm_device *drm_dev = dminor->dev;
> +	struct drm_i915_private *dev_priv = drm_dev->dev_private;
> +	uint32_t misccpctl;
> +	int i, ret;
> +
> +	ret = l3_access_valid(drm_dev, offset);
> +	if (ret)
> +		return ret;
> +
> +	ret = i915_mutex_lock_interruptible(drm_dev);
> +	if (ret)
> +		return ret;
> +
> +	misccpctl = I915_READ(GEN7_MISCCPCTL);
> +	I915_WRITE(GEN7_MISCCPCTL, misccpctl & ~GEN7_DOP_CLOCK_GATE_ENABLE);
> +
> +	for (i = offset; count >= 4 && i < GEN7_L3LOG_SIZE; i += 4, count -= 4)
> +		*((uint32_t *)(&buf[i])) = I915_READ(GEN7_L3LOG_BASE + i);
> +
> +	I915_WRITE(GEN7_MISCCPCTL, misccpctl);
> +
> +	mutex_unlock(&drm_dev->struct_mutex);
> +
> +	return i - offset;
> +}
> +
> +static ssize_t
> +i915_l3_write(struct file *filp, struct kobject *kobj,
> +	      struct bin_attribute *attr, char *buf,
> +	      loff_t offset, size_t count)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
> +	struct drm_device *drm_dev = dminor->dev;
> +	struct drm_i915_private *dev_priv = drm_dev->dev_private;
> +	u32 *temp = NULL;
> +	int i, ret;
> +
> +	ret = l3_access_valid(drm_dev, offset);
> +	if (ret)
> +		return ret;
> +
> +	ret = i915_mutex_lock_interruptible(drm_dev);
> +	if (ret)
> +		return ret;
> +
> +	if (!dev_priv->mm.l3_remap_info) {
> +		temp = kzalloc(GEN7_L3LOG_SIZE, GFP_KERNEL);
> +		if (!temp) {
> +			mutex_unlock(&drm_dev->struct_mutex);
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	ret = i915_gpu_idle(drm_dev, true);
> +	if (ret) {
> +		kfree(temp);
> +		mutex_unlock(&drm_dev->struct_mutex);
> +		return ret;
> +	}
> +
> +	/* TODO: Ideally we really want a GPU reset here to make sure errors
> +	 * aren't propagated Since I cannot find a stable way to reset the GPU
> +	 * at this point it is left as a TODO.
> +	*/
> +
> +	if (dev_priv->mm.l3_remap_info)
> +		temp = dev_priv->mm.l3_remap_info;
> +
> +	dev_priv->mm.l3_remap_info = temp;
> +
> +	for (i = offset; count >= 4 && i < GEN7_L3LOG_SIZE; i += 4, count -= 4) {
> +		u32 remap = I915_READ(GEN7_L3LOG_BASE + i);
> +		if (remap && remap != *temp)
> +			DRM_ERROR("0x%x was already programmed to %x\n",
> +				  GEN7_L3LOG_BASE + i, remap);
> +		*temp++ = *(uint32_t *)(&buf[i]);
> +	}
> +
> +	i915_gem_l3_remap(drm_dev);
> +
> +	mutex_unlock(&drm_dev->struct_mutex);
> +
> +	return offset - i;
> +}
> +
> +static struct bin_attribute dpf_attrs = {
> +	.attr = {.name = "l3_parity", .mode = (S_IRUSR | S_IWUSR)},
> +	.size = GEN7_L3LOG_SIZE,
> +	.read = i915_l3_read,
> +	.write = i915_l3_write,
> +	.mmap = NULL
> +};
> +
>  void i915_setup_sysfs(struct drm_device *dev)
>  {
>  	int ret;
>  
> -	/* ILK doesn't have any residency information */
> +	/* ILK and below don't yet have relevant sysfs files */
>  	if (INTEL_INFO(dev)->gen < 6)
>  		return;
>  
>  	ret = sysfs_merge_group(&dev->primary->kdev.kobj, &rc6_attr_group);
>  	if (ret)
> -		DRM_ERROR("sysfs setup failed\n");
> +		DRM_ERROR("RC6 residency sysfs setup failed\n");
> +
> +	if (!IS_IVYBRIDGE(dev))
> +		return;
> +
> +	ret = device_create_bin_file(&dev->primary->kdev, &dpf_attrs);
> +	if (ret)
> +		DRM_ERROR("l3 parity sysfs setup failed\n");

Imo these checks are confusing, usually we check for features instead of
their absence and enclose the relevant code in the if block, i.e.

if (gen >= 6)
	merge_rc6_residency_stuff;

if (IS_IVB)
	merge_dpf_attrs_file;

-Daniel

>  }
>  
>  void i915_teardown_sysfs(struct drm_device *dev)
>  {
> +	device_remove_bin_file(&dev->primary->kdev,  &dpf_attrs);
>  	sysfs_unmerge_group(&dev->primary->kdev.kobj, &rc6_attr_group);
>  }
> -- 
> 1.7.10
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 5/5] drm/i915: l3 parity sysfs interface

[Ben Widawsky]

On Tue, 1 May 2012 20:24:44 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Apr 27, 2012 at 05:40:21PM -0700, Ben Widawsky wrote:
> > Dumb binary interfaces which allow root-only updates of the cache
> > remapping registers. As mentioned in a previous patch, software using
> > this interface needs to know about HW limits, and other programming
> > considerations as the kernel interface does no checking for these things
> > on the root-only interface.
> > 
> > v1: Drop extra posting reads (Chris)
> > Return negative values in the sysfs interfaces on errors (Chris)
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_sysfs.c |  128 ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 126 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> > index 79f8344..ed77cbf 100644
> > --- a/drivers/gpu/drm/i915/i915_sysfs.c
> > +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> > @@ -29,6 +29,7 @@
> >  #include <linux/module.h>
> >  #include <linux/stat.h>
> >  #include <linux/sysfs.h>
> > +#include "intel_drv.h"
> >  #include "i915_drv.h"
> >  
> >  static u32 calc_residency(struct drm_device *dev, const u32 reg)
> > @@ -92,20 +93,143 @@ static struct attribute_group rc6_attr_group = {
> >  	.attrs =  rc6_attrs
> >  };
> >  
> > +static int l3_access_valid(struct drm_device *dev, loff_t offset)
> > +{
> > +	if (!IS_IVYBRIDGE(dev))
> > +		return -EPERM;
> > +
> > +	if (offset % 4 != 0)
> > +		return -EPERM;
> > +
> > +	if (offset >= GEN7_L3LOG_SIZE)
> > +		return -ENXIO;
> > +
> > +	return 0;
> > +}
> > +
> > +static ssize_t
> > +i915_l3_read(struct file *filp, struct kobject *kobj,
> > +	     struct bin_attribute *attr, char *buf,
> > +	     loff_t offset, size_t count)
> > +{
> > +	struct device *dev = container_of(kobj, struct device, kobj);
> > +	struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
> > +	struct drm_device *drm_dev = dminor->dev;
> > +	struct drm_i915_private *dev_priv = drm_dev->dev_private;
> > +	uint32_t misccpctl;
> > +	int i, ret;
> > +
> > +	ret = l3_access_valid(drm_dev, offset);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = i915_mutex_lock_interruptible(drm_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	misccpctl = I915_READ(GEN7_MISCCPCTL);
> > +	I915_WRITE(GEN7_MISCCPCTL, misccpctl & ~GEN7_DOP_CLOCK_GATE_ENABLE);
> > +
> > +	for (i = offset; count >= 4 && i < GEN7_L3LOG_SIZE; i += 4, count -= 4)
> > +		*((uint32_t *)(&buf[i])) = I915_READ(GEN7_L3LOG_BASE + i);
> > +
> > +	I915_WRITE(GEN7_MISCCPCTL, misccpctl);
> > +
> > +	mutex_unlock(&drm_dev->struct_mutex);
> > +
> > +	return i - offset;
> > +}
> > +
> > +static ssize_t
> > +i915_l3_write(struct file *filp, struct kobject *kobj,
> > +	      struct bin_attribute *attr, char *buf,
> > +	      loff_t offset, size_t count)
> > +{
> > +	struct device *dev = container_of(kobj, struct device, kobj);
> > +	struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
> > +	struct drm_device *drm_dev = dminor->dev;
> > +	struct drm_i915_private *dev_priv = drm_dev->dev_private;
> > +	u32 *temp = NULL;
> > +	int i, ret;
> > +
> > +	ret = l3_access_valid(drm_dev, offset);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = i915_mutex_lock_interruptible(drm_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!dev_priv->mm.l3_remap_info) {
> > +		temp = kzalloc(GEN7_L3LOG_SIZE, GFP_KERNEL);
> > +		if (!temp) {
> > +			mutex_unlock(&drm_dev->struct_mutex);
> > +			return -ENOMEM;
> > +		}
> > +	}
> > +
> > +	ret = i915_gpu_idle(drm_dev, true);
> > +	if (ret) {
> > +		kfree(temp);
> > +		mutex_unlock(&drm_dev->struct_mutex);
> > +		return ret;
> > +	}
> > +
> > +	/* TODO: Ideally we really want a GPU reset here to make sure errors
> > +	 * aren't propagated Since I cannot find a stable way to reset the GPU
> > +	 * at this point it is left as a TODO.
> > +	*/
> > +
> > +	if (dev_priv->mm.l3_remap_info)
> > +		temp = dev_priv->mm.l3_remap_info;
> > +
> > +	dev_priv->mm.l3_remap_info = temp;
> > +
> > +	for (i = offset; count >= 4 && i < GEN7_L3LOG_SIZE; i += 4, count -= 4) {
> > +		u32 remap = I915_READ(GEN7_L3LOG_BASE + i);
> > +		if (remap && remap != *temp)
> > +			DRM_ERROR("0x%x was already programmed to %x\n",
> > +				  GEN7_L3LOG_BASE + i, remap);
> > +		*temp++ = *(uint32_t *)(&buf[i]);
> > +	}
> > +
> > +	i915_gem_l3_remap(drm_dev);
> > +
> > +	mutex_unlock(&drm_dev->struct_mutex);
> > +
> > +	return offset - i;
> > +}
> > +
> > +static struct bin_attribute dpf_attrs = {
> > +	.attr = {.name = "l3_parity", .mode = (S_IRUSR | S_IWUSR)},
> > +	.size = GEN7_L3LOG_SIZE,
> > +	.read = i915_l3_read,
> > +	.write = i915_l3_write,
> > +	.mmap = NULL
> > +};
> > +
> >  void i915_setup_sysfs(struct drm_device *dev)
> >  {
> >  	int ret;
> >  
> > -	/* ILK doesn't have any residency information */
> > +	/* ILK and below don't yet have relevant sysfs files */
> >  	if (INTEL_INFO(dev)->gen < 6)
> >  		return;
> >  
> >  	ret = sysfs_merge_group(&dev->primary->kdev.kobj, &rc6_attr_group);
> >  	if (ret)
> > -		DRM_ERROR("sysfs setup failed\n");
> > +		DRM_ERROR("RC6 residency sysfs setup failed\n");
> > +
> > +	if (!IS_IVYBRIDGE(dev))
> > +		return;
> > +
> > +	ret = device_create_bin_file(&dev->primary->kdev, &dpf_attrs);
> > +	if (ret)
> > +		DRM_ERROR("l3 parity sysfs setup failed\n");
> 
> Imo these checks are confusing, usually we check for features instead of
> their absence and enclose the relevant code in the if block, i.e.
> 
> if (gen >= 6)
> 	merge_rc6_residency_stuff;
> 
> if (IS_IVB)
> 	merge_dpf_attrs_file;
> 
> -Daniel
> 

I agree. I got lazy because sysfs is so sparse. This will be fixed after
I successfully run this on dinq.

Thanks.

> >  }
> >  
> >  void i915_teardown_sysfs(struct drm_device *dev)
> >  {
> > +	device_remove_bin_file(&dev->primary->kdev,  &dpf_attrs);
> >  	sysfs_unmerge_group(&dev->primary->kdev.kobj, &rc6_attr_group);
> >  }
> > -- 
> > 1.7.10
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

Re: [PATCH 2/5] drm/i915: Dynamic Parity Detection handling

[Jesse Barnes]

On Fri, 27 Apr 2012 17:40:18 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:
> +
> +/**
> + * ivybridge_parity_work - Workqueue called when a parity error interrupt
> + * occurred.
> + *
> + * Doesn't actually do anything except notify userspace so that userspace may
> + * disable things later on.

kdoc style comment but missing the @work: parameter.

Might be a good place to describe what userspace can do in response
(i.e. duplicate some of your commit message here).

> + */
> +static void ivybridge_parity_work(struct work_struct *work)
> +{
> +	drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
> +						    parity_error_work);
> +

Stray newline.

> +	u32 error_status, row, bank, subbank;
> +	char *parity_event[5];
> +	uint32_t misccpctl;
> +	unsigned long flags;
> +
> +	/* We must turn off DOP level clock gating to access the L3 registers.
> +	 * In order to prevent a get/put style interface, acquire struct mutex
> +	 * any time we access those registers.
> +	 */
> +	mutex_lock(&dev_priv->dev->struct_mutex);
> +
> +	misccpctl = I915_READ(GEN7_MISCCPCTL);
> +	I915_WRITE(GEN7_MISCCPCTL, misccpctl & ~GEN7_DOP_CLOCK_GATE_ENABLE);

DOP clock gating should be unconditionally disabled, you can move this
to the clock gating routine.

> +	POSTING_READ(GEN7_MISCCPCTL);
> +
> +	error_status = I915_READ(GEN7_L3CDERRST1);
> +	row = GEN7_PARITY_ERROR_ROW(error_status);
> +	bank = GEN7_PARITY_ERROR_BANK(error_status);
> +	subbank = GEN7_PARITY_ERROR_SUBBANK(error_status);
> +
> +	I915_WRITE(GEN7_L3CDERRST1, GEN7_PARITY_ERROR_VALID |
> +				    GEN7_L3CDERRST1_ENABLE);
> +	POSTING_READ(GEN7_L3CDERRST1);
> +
> +	I915_WRITE(GEN7_MISCCPCTL, misccpctl);
> +
> +	spin_lock_irqsave(&dev_priv->irq_lock, flags);
> +	dev_priv->gt_irq_mask &= ~GT_GEN7_L3_PARITY_ERROR_INTERRUPT;
> +	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> +	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);

Is there a debug register we can read to see if we missed any events at
this point?  A followon patch might be to see when the last event
occured, and if we get two or more in rapid succession (like right when
we re-enable interrupts) the kernel could automatically do the
remapping.  But that's totally optional; I doubt we'll see that in
practice except on machines we've specially broken in the lab. :)

> +
> +	mutex_unlock(&dev_priv->dev->struct_mutex);
> +
> +	parity_event[0] = "L3_PARITY_ERROR=1";
> +	parity_event[1] = kasprintf(GFP_KERNEL, "ROW=%d", row);
> +	parity_event[2] = kasprintf(GFP_KERNEL, "BANK=%d", bank);
> +	parity_event[3] = kasprintf(GFP_KERNEL, "SUBBANK=%d", subbank);
> +	parity_event[4] = NULL;
> +
> +	kobject_uevent_env(&dev_priv->dev->primary->kdev.kobj,
> +			   KOBJ_CHANGE, parity_event);

You have a DRM_INFO when the interrupt comes in, may as well spit out
the row/bank/subbank info here too.  Though I'd make them both DEBUG.

> +
> +	kfree(parity_event[3]);
> +	kfree(parity_event[2]);
> +	kfree(parity_event[1]);
> +}

Hm we really need a man page for our events and such, but that's a
separate patch.

> +
> +void ivybridge_handle_parity_error(struct drm_device *dev)
> +{
> +	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> +	unsigned long flags;
> +
> +	if (WARN_ON(IS_GEN6(dev)))
> +		return;

What's this for?  The user won't be able to do anything at this
point except get scared...  I'd either just make this return if GEN6 or
add a GEN7 check before we branch here in the first place from the
interrupt handler.

> +
> +	spin_lock_irqsave(&dev_priv->irq_lock, flags);
> +	dev_priv->gt_irq_mask |= GT_GEN7_L3_PARITY_ERROR_INTERRUPT;
> +	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> +	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
> +
> +	queue_work(dev_priv->wq, &dev_priv->parity_error_work);
> +	DRM_INFO("Parity error interrupt. Scheduling work\n");
> +}
> +
>  static void snb_gt_irq_handler(struct drm_device *dev,
>  			       struct drm_i915_private *dev_priv,
>  			       u32 gt_iir)
> @@ -449,6 +526,9 @@ static void snb_gt_irq_handler(struct drm_device *dev,
>  		DRM_ERROR("GT error interrupt 0x%08x\n", gt_iir);
>  		i915_handle_error(dev, false);
>  	}
> +
> +	if (gt_iir & GT_GEN7_L3_PARITY_ERROR_INTERRUPT)
> +		ivybridge_handle_parity_error(dev);
>  }
>  
>  static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,
> @@ -1978,6 +2058,9 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
>  	if (IS_GEN6(dev) || IS_IVYBRIDGE(dev))
>  		INIT_WORK(&dev_priv->rps_work, gen6_pm_rps_work);
>  
> +	if (IS_IVYBRIDGE(dev))
> +		INIT_WORK(&dev_priv->parity_error_work, ivybridge_parity_work);


Looks good otherwise, so with the above fixed up or denied:

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 3/5] drm/i915: enable parity error interrupts

[Jesse Barnes]

On Fri, 27 Apr 2012 17:40:19 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> The previous patch put all the code, and handlers in place. It should
> now be safe to enable the parity error interrupt. The parity error must
> be unmasked in both the GTIMR, and the CS IMR. Unfortunately, the docs
> aren't clear about this; nevertheless it's the truth.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---

Can't say I like all the IS_IVB branches, but they're probably ok until
we add the next IVB specific feature, then we can fork the functions
into IVB specific ones.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 4/5] drm/i915: remap l3 on hw init

[Jesse Barnes]

On Fri, 27 Apr 2012 17:40:20 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> If any l3 rows have been previously remapped, we must remap them after
> GPU reset/resume too.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |    3 +++
>  drivers/gpu/drm/i915/i915_gem.c |   26 ++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h |    3 +++
>  3 files changed, 32 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9505fc0..e9efe17 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -641,6 +641,8 @@ typedef struct drm_i915_private {
>  		/** PPGTT used for aliasing the PPGTT with the GTT */
>  		struct i915_hw_ppgtt *aliasing_ppgtt;
>  
> +		u32 *l3_remap_info;
> +
>  		struct shrinker inactive_shrinker;
>  
>  		/**
> @@ -1290,6 +1292,7 @@ int __must_check i915_gem_object_set_domain(struct drm_i915_gem_object *obj,
>  					    uint32_t write_domain);
>  int __must_check i915_gem_object_finish_gpu(struct drm_i915_gem_object *obj);
>  int __must_check i915_gem_init_hw(struct drm_device *dev);
> +void i915_gem_l3_remap(struct drm_device *dev);
>  void i915_gem_init_swizzling(struct drm_device *dev);
>  void i915_gem_init_ppgtt(struct drm_device *dev);
>  void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 7bc4a40..bb3ef9f 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3475,6 +3475,30 @@ i915_gem_idle(struct drm_device *dev)
>  	return 0;
>  }
>  
> +void i915_gem_l3_remap(struct drm_device *dev)
> +{
> +	drm_i915_private_t *dev_priv = dev->dev_private;
> +	u32 misccpctl;
> +	int i;
> +
> +	if (!dev_priv->mm.l3_remap_info)
> +		return;
> +
> +	WARN_ON(!IS_IVYBRIDGE(dev));

Should just be a return?

> +
> +	misccpctl = I915_READ(GEN7_MISCCPCTL);
> +	I915_WRITE(GEN7_MISCCPCTL, misccpctl & ~GEN7_DOP_CLOCK_GATE_ENABLE);
> +	POSTING_READ(GEN7_MISCCPCTL);
> +
> +	for (i = 0; i < GEN7_L3LOG_SIZE; i += 4)
> +		I915_WRITE(GEN7_L3LOG_BASE + i, dev_priv->mm.l3_remap_info[i/4]);
> +
> +	/* Make sure all the writes land before disabling dop clock gating */
> +	POSTING_READ(GEN7_L3LOG_BASE);

Same comment as before on the DOP gating...

> +
> +	I915_WRITE(GEN7_MISCCPCTL, misccpctl);
> +}
> +
>  void i915_gem_init_swizzling(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
> @@ -3566,6 +3590,8 @@ i915_gem_init_hw(struct drm_device *dev)
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	int ret;
>  
> +	i915_gem_l3_remap(dev);

Since this looks unconditional, so we'll get the backtrace on every
non-IVB system?



-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 5/5] drm/i915: l3 parity sysfs interface

[Jesse Barnes]

On Fri, 27 Apr 2012 17:40:21 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> Dumb binary interfaces which allow root-only updates of the cache
> remapping registers. As mentioned in a previous patch, software using
> this interface needs to know about HW limits, and other programming
> considerations as the kernel interface does no checking for these things
> on the root-only interface.
> 
> v1: Drop extra posting reads (Chris)
> Return negative values in the sysfs interfaces on errors (Chris)
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_sysfs.c |  128 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 126 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index 79f8344..ed77cbf 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -29,6 +29,7 @@
>  #include <linux/module.h>
>  #include <linux/stat.h>
>  #include <linux/sysfs.h>
> +#include "intel_drv.h"
>  #include "i915_drv.h"
>  
>  static u32 calc_residency(struct drm_device *dev, const u32 reg)
> @@ -92,20 +93,143 @@ static struct attribute_group rc6_attr_group = {
>  	.attrs =  rc6_attrs
>  };
>  
> +static int l3_access_valid(struct drm_device *dev, loff_t offset)
> +{
> +	if (!IS_IVYBRIDGE(dev))
> +		return -EPERM;

-ENODEV?

> +
> +	if (offset % 4 != 0)
> +		return -EPERM;

-EINVAL?

> +
> +	if (offset >= GEN7_L3LOG_SIZE)
> +		return -ENXIO;

-E2BIG or -ERANGE?

And maybe some debug messages here so people can debug their daemons in
case some of the return values are identical.

> +
> +	return 0;
> +}
> +
> +static ssize_t
> +i915_l3_read(struct file *filp, struct kobject *kobj,
> +	     struct bin_attribute *attr, char *buf,
> +	     loff_t offset, size_t count)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
> +	struct drm_device *drm_dev = dminor->dev;
> +	struct drm_i915_private *dev_priv = drm_dev->dev_private;
> +	uint32_t misccpctl;
> +	int i, ret;
> +
> +	ret = l3_access_valid(drm_dev, offset);
> +	if (ret)
> +		return ret;
> +
> +	ret = i915_mutex_lock_interruptible(drm_dev);
> +	if (ret)
> +		return ret;
> +
> +	misccpctl = I915_READ(GEN7_MISCCPCTL);
> +	I915_WRITE(GEN7_MISCCPCTL, misccpctl & ~GEN7_DOP_CLOCK_GATE_ENABLE);
> +
> +	for (i = offset; count >= 4 && i < GEN7_L3LOG_SIZE; i += 4, count -= 4)
> +		*((uint32_t *)(&buf[i])) = I915_READ(GEN7_L3LOG_BASE + i);
> +
> +	I915_WRITE(GEN7_MISCCPCTL, misccpctl);
> +
> +	mutex_unlock(&drm_dev->struct_mutex);
> +
> +	return i - offset;
> +}
> +
> +static ssize_t
> +i915_l3_write(struct file *filp, struct kobject *kobj,
> +	      struct bin_attribute *attr, char *buf,
> +	      loff_t offset, size_t count)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
> +	struct drm_device *drm_dev = dminor->dev;
> +	struct drm_i915_private *dev_priv = drm_dev->dev_private;
> +	u32 *temp = NULL;
> +	int i, ret;
> +
> +	ret = l3_access_valid(drm_dev, offset);
> +	if (ret)
> +		return ret;
> +
> +	ret = i915_mutex_lock_interruptible(drm_dev);
> +	if (ret)
> +		return ret;
> +
> +	if (!dev_priv->mm.l3_remap_info) {
> +		temp = kzalloc(GEN7_L3LOG_SIZE, GFP_KERNEL);
> +		if (!temp) {
> +			mutex_unlock(&drm_dev->struct_mutex);
> +			return -ENOMEM;
> +		}
> +	}

Grumble, lazy allocation.  But if we did this at driver load time we'd
also need a valid bit so we knew whether to try to load it or not...

> +
> +	ret = i915_gpu_idle(drm_dev, true);
> +	if (ret) {
> +		kfree(temp);
> +		mutex_unlock(&drm_dev->struct_mutex);
> +		return ret;
> +	}
> +
> +	/* TODO: Ideally we really want a GPU reset here to make sure errors
> +	 * aren't propagated Since I cannot find a stable way to reset the GPU
> +	 * at this point it is left as a TODO.
> +	*/

Do we want a reset?  Or just to restart the app that consumed the
error?  If the latter, then the current uevent ought to be enough,
potentially with a configurable SIGBUS or something.

> +
> +	if (dev_priv->mm.l3_remap_info)
> +		temp = dev_priv->mm.l3_remap_info;
> +
> +	dev_priv->mm.l3_remap_info = temp;

I'd combine these assignments with the kzalloc block above just to keep
things together.

> +
> +	for (i = offset; count >= 4 && i < GEN7_L3LOG_SIZE; i += 4, count -= 4) {
> +		u32 remap = I915_READ(GEN7_L3LOG_BASE + i);
> +		if (remap && remap != *temp)
> +			DRM_ERROR("0x%x was already programmed to %x\n",
> +				  GEN7_L3LOG_BASE + i, remap);
> +		*temp++ = *(uint32_t *)(&buf[i]);
> +	}

Is this an error?  Or just a "userspace is schizo" and therefore
just a DEBUG?

> +
> +	i915_gem_l3_remap(drm_dev);

You could add the warning/debug info to l3_remap instead, right?

> +
> +	mutex_unlock(&drm_dev->struct_mutex);
> +
> +	return offset - i;
> +}
> +
> +static struct bin_attribute dpf_attrs = {
> +	.attr = {.name = "l3_parity", .mode = (S_IRUSR | S_IWUSR)},
> +	.size = GEN7_L3LOG_SIZE,
> +	.read = i915_l3_read,
> +	.write = i915_l3_write,
> +	.mmap = NULL
> +};
> +
>  void i915_setup_sysfs(struct drm_device *dev)
>  {
>  	int ret;
>  
> -	/* ILK doesn't have any residency information */
> +	/* ILK and below don't yet have relevant sysfs files */
>  	if (INTEL_INFO(dev)->gen < 6)
>  		return;
>  
>  	ret = sysfs_merge_group(&dev->primary->kdev.kobj, &rc6_attr_group);
>  	if (ret)
> -		DRM_ERROR("sysfs setup failed\n");
> +		DRM_ERROR("RC6 residency sysfs setup failed\n");
> +
> +	if (!IS_IVYBRIDGE(dev))
> +		return;
> +
> +	ret = device_create_bin_file(&dev->primary->kdev, &dpf_attrs);
> +	if (ret)
> +		DRM_ERROR("l3 parity sysfs setup failed\n");
>  }
>  
>  void i915_teardown_sysfs(struct drm_device *dev)
>  {
> +	device_remove_bin_file(&dev->primary->kdev,  &dpf_attrs);
>  	sysfs_unmerge_group(&dev->primary->kdev.kobj, &rc6_attr_group);
>  }

I think Daniel covered the above.

With the above addressed:
Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/5] drm/i915: Dynamic Parity Detection handling

[Jesse Barnes]

On Fri, 25 May 2012 10:34:58 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > +	misccpctl = I915_READ(GEN7_MISCCPCTL);
> > +	I915_WRITE(GEN7_MISCCPCTL, misccpctl & ~GEN7_DOP_CLOCK_GATE_ENABLE);
> 
> DOP clock gating should be unconditionally disabled, you can move this
> to the clock gating routine.

Ok I dug into this at your prodding and take it back.  We can leave DOP
clock gating enabled; only VLV needs the unconditional disable, so we
don't need to worry about this if/until we add DPF there.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/5] drm/i915: Dynamic Parity Detection handling

[Ben Widawsky]

On Fri, 25 May 2012 10:34:58 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Fri, 27 Apr 2012 17:40:18 -0700
> Ben Widawsky <ben@bwidawsk.net> wrote:
> > +
> > +/**
> > + * ivybridge_parity_work - Workqueue called when a parity error interrupt
> > + * occurred.
> > + *
> > + * Doesn't actually do anything except notify userspace so that userspace may
> > + * disable things later on.
> 
> kdoc style comment but missing the @work: parameter.
> 
> Might be a good place to describe what userspace can do in response
> (i.e. duplicate some of your commit message here).
> 

Got it, Thanks.

> > + */
> > +static void ivybridge_parity_work(struct work_struct *work)
> > +{
> > +	drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
> > +						    parity_error_work);
> > +
> 
> Stray newline.

Got it, Thanks.

> 
> > +	u32 error_status, row, bank, subbank;
> > +	char *parity_event[5];
> > +	uint32_t misccpctl;
> > +	unsigned long flags;
> > +
> > +	/* We must turn off DOP level clock gating to access the L3 registers.
> > +	 * In order to prevent a get/put style interface, acquire struct mutex
> > +	 * any time we access those registers.
> > +	 */
> > +	mutex_lock(&dev_priv->dev->struct_mutex);
> > +
> > +	misccpctl = I915_READ(GEN7_MISCCPCTL);
> > +	I915_WRITE(GEN7_MISCCPCTL, misccpctl & ~GEN7_DOP_CLOCK_GATE_ENABLE);
> 
> DOP clock gating should be unconditionally disabled, you can move this
> to the clock gating routine.
> 
> > +	POSTING_READ(GEN7_MISCCPCTL);
> > +
> > +	error_status = I915_READ(GEN7_L3CDERRST1);
> > +	row = GEN7_PARITY_ERROR_ROW(error_status);
> > +	bank = GEN7_PARITY_ERROR_BANK(error_status);
> > +	subbank = GEN7_PARITY_ERROR_SUBBANK(error_status);
> > +
> > +	I915_WRITE(GEN7_L3CDERRST1, GEN7_PARITY_ERROR_VALID |
> > +				    GEN7_L3CDERRST1_ENABLE);
> > +	POSTING_READ(GEN7_L3CDERRST1);
> > +
> > +	I915_WRITE(GEN7_MISCCPCTL, misccpctl);
> > +
> > +	spin_lock_irqsave(&dev_priv->irq_lock, flags);
> > +	dev_priv->gt_irq_mask &= ~GT_GEN7_L3_PARITY_ERROR_INTERRUPT;
> > +	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> > +	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
> 
> Is there a debug register we can read to see if we missed any events at
> this point?  A followon patch might be to see when the last event
> occured, and if we get two or more in rapid succession (like right when
> we re-enable interrupts) the kernel could automatically do the
> remapping.  But that's totally optional; I doubt we'll see that in
> practice except on machines we've specially broken in the lab. :)
> 

I've come up with some similarly complex ideas in previous versions of
the patch, and Daniel seemed to be not so interested because of the
expectedly infrequent nature of the events. 

If Daniel is willing to accept the patch with your suggestion, I would
be happy to implement it. Of course the metric for "rapid succession"
would need to be agreed upon. I suggestion.

> > + +	mutex_unlock(&dev_priv->dev->struct_mutex); + +	parity_event[0]
> > = "L3_PARITY_ERROR=1"; +	parity_event[1] = kasprintf(GFP_KERNEL,
> > "ROW=%d", row); +	parity_event[2] = kasprintf(GFP_KERNEL,
> > "BANK=%d", bank); +	parity_event[3] = kasprintf(GFP_KERNEL,
> > "SUBBANK=%d", subbank); +	parity_event[4] = NULL; + +
> > kobject_uevent_env(&dev_priv->dev->primary->kdev.kobj, +
> > KOBJ_CHANGE, parity_event);
> 
> You have a DRM_INFO when the interrupt comes in, may as well spit out
> the row/bank/subbank info here too.  Though I'd make them both DEBUG.

Good point. I had that info in an earlier version of the patch. Not sure
when it got dropped. Also noted the DRM_DEBUG suggestion, thanks.

> 
> > + +	kfree(parity_event[3]); +	kfree(parity_event[2]); +
> > kfree(parity_event[1]); +}
> 
> Hm we really need a man page for our events and such, but that's a
> separate patch.
> 
> > + +void ivybridge_handle_parity_error(struct drm_device *dev) +{ +
> > drm_i915_private_t *dev_priv = (drm_i915_private_t *)
> > dev->dev_private; +	unsigned long flags; + +	if
> > (WARN_ON(IS_GEN6(dev))) +		return;
> 
> What's this for?  The user won't be able to do anything at this point
> except get scared...  I'd either just make this return if GEN6 or add
> a GEN7 check before we branch here in the first place from the
> interrupt handler.

It is really just to prevent developers from enabling this interrupt on
GEN6, or VLV... It really should have been if (!IS_IVYBRIDGE). I'll drop
the WARN_ON and just return.

> 
> > + +	spin_lock_irqsave(&dev_priv->irq_lock, flags); +
> > dev_priv->gt_irq_mask |= GT_GEN7_L3_PARITY_ERROR_INTERRUPT; +
> > I915_WRITE(GTIMR, dev_priv->gt_irq_mask); +
> > spin_unlock_irqrestore(&dev_priv->irq_lock, flags); + +
> > queue_work(dev_priv->wq, &dev_priv->parity_error_work); +
> > DRM_INFO("Parity error interrupt. Scheduling work\n"); +} + static
> > void snb_gt_irq_handler(struct drm_device *dev, struct
> > drm_i915_private *dev_priv, u32 gt_iir) @@ -449,6 +526,9 @@ static
> > void snb_gt_irq_handler(struct drm_device *dev, DRM_ERROR("GT error
> > interrupt 0x%08x\n", gt_iir); i915_handle_error(dev, false); } + +
> > if (gt_iir & GT_GEN7_L3_PARITY_ERROR_INTERRUPT) +
> > ivybridge_handle_parity_error(dev); }
> >  
> >  static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,
> >  @@ -1978,6 +2058,9 @@ static void ironlake_irq_preinstall(struct
> >  drm_device *dev) if (IS_GEN6(dev) || IS_IVYBRIDGE(dev))
> >  INIT_WORK(&dev_priv->rps_work, gen6_pm_rps_work);
> >  
> > +	if (IS_IVYBRIDGE(dev)) +
> > INIT_WORK(&dev_priv->parity_error_work, ivybridge_parity_work);
> 
> 
> Looks good otherwise, so with the above fixed up or denied:
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
I'll wait for Daniel to address your request about auto-remapping before
I add your r-b.

Thanks.



-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 4/5] drm/i915: remap l3 on hw init

[Ben Widawsky]

On Fri, 25 May 2012 10:39:57 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Fri, 27 Apr 2012 17:40:20 -0700
> Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> > If any l3 rows have been previously remapped, we must remap them after
> > GPU reset/resume too.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |    3 +++
> >  drivers/gpu/drm/i915/i915_gem.c |   26 ++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_reg.h |    3 +++
> >  3 files changed, 32 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 9505fc0..e9efe17 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -641,6 +641,8 @@ typedef struct drm_i915_private {
> >  		/** PPGTT used for aliasing the PPGTT with the GTT */
> >  		struct i915_hw_ppgtt *aliasing_ppgtt;
> >  
> > +		u32 *l3_remap_info;
> > +
> >  		struct shrinker inactive_shrinker;
> >  
> >  		/**
> > @@ -1290,6 +1292,7 @@ int __must_check i915_gem_object_set_domain(struct drm_i915_gem_object *obj,
> >  					    uint32_t write_domain);
> >  int __must_check i915_gem_object_finish_gpu(struct drm_i915_gem_object *obj);
> >  int __must_check i915_gem_init_hw(struct drm_device *dev);
> > +void i915_gem_l3_remap(struct drm_device *dev);
> >  void i915_gem_init_swizzling(struct drm_device *dev);
> >  void i915_gem_init_ppgtt(struct drm_device *dev);
> >  void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 7bc4a40..bb3ef9f 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3475,6 +3475,30 @@ i915_gem_idle(struct drm_device *dev)
> >  	return 0;
> >  }
> >  
> > +void i915_gem_l3_remap(struct drm_device *dev)
> > +{
> > +	drm_i915_private_t *dev_priv = dev->dev_private;
> > +	u32 misccpctl;
> > +	int i;
> > +
> > +	if (!dev_priv->mm.l3_remap_info)
> > +		return;
> > +
> > +	WARN_ON(!IS_IVYBRIDGE(dev));
> 
> Should just be a return?

I had this here since it's a public function, and we have a history of
calling functions on the wrong generation. So it just helps us find out
a little sooner we're doing the wrong thing. As you point out below
though, this doesn't give the behavior we want. Thanks for catching it.


> 
> > +
> > +	misccpctl = I915_READ(GEN7_MISCCPCTL);
> > +	I915_WRITE(GEN7_MISCCPCTL, misccpctl & ~GEN7_DOP_CLOCK_GATE_ENABLE);
> > +	POSTING_READ(GEN7_MISCCPCTL);
> > +
> > +	for (i = 0; i < GEN7_L3LOG_SIZE; i += 4)
> > +		I915_WRITE(GEN7_L3LOG_BASE + i, dev_priv->mm.l3_remap_info[i/4]);
> > +
> > +	/* Make sure all the writes land before disabling dop clock gating */
> > +	POSTING_READ(GEN7_L3LOG_BASE);
> 
> Same comment as before on the DOP gating...

I think we're good here now too.

> 
> > +
> > +	I915_WRITE(GEN7_MISCCPCTL, misccpctl);
> > +}
> > +
> >  void i915_gem_init_swizzling(struct drm_device *dev)
> >  {
> >  	drm_i915_private_t *dev_priv = dev->dev_private;
> > @@ -3566,6 +3590,8 @@ i915_gem_init_hw(struct drm_device *dev)
> >  	drm_i915_private_t *dev_priv = dev->dev_private;
> >  	int ret;
> >  
> > +	i915_gem_l3_remap(dev);
> 
> Since this looks unconditional, so we'll get the backtrace on every
> non-IVB system?

Yes. That seems like a problem.

> 
> 
> 

Reviewed-by?

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 5/5] drm/i915: l3 parity sysfs interface

[Ben Widawsky]

On Fri, 25 May 2012 10:51:19 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Fri, 27 Apr 2012 17:40:21 -0700
> Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> > Dumb binary interfaces which allow root-only updates of the cache
> > remapping registers. As mentioned in a previous patch, software using
> > this interface needs to know about HW limits, and other programming
> > considerations as the kernel interface does no checking for these things
> > on the root-only interface.
> > 
> > v1: Drop extra posting reads (Chris)
> > Return negative values in the sysfs interfaces on errors (Chris)
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_sysfs.c |  128 ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 126 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> > index 79f8344..ed77cbf 100644
> > --- a/drivers/gpu/drm/i915/i915_sysfs.c
> > +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> > @@ -29,6 +29,7 @@
> >  #include <linux/module.h>
> >  #include <linux/stat.h>
> >  #include <linux/sysfs.h>
> > +#include "intel_drv.h"
> >  #include "i915_drv.h"
> >  
> >  static u32 calc_residency(struct drm_device *dev, const u32 reg)
> > @@ -92,20 +93,143 @@ static struct attribute_group rc6_attr_group = {
> >  	.attrs =  rc6_attrs
> >  };
> >  
> > +static int l3_access_valid(struct drm_device *dev, loff_t offset)
> > +{
> > +	if (!IS_IVYBRIDGE(dev))
> > +		return -EPERM;
> 
> -ENODEV?
> 
> > +
> > +	if (offset % 4 != 0)
> > +		return -EPERM;
> 
> -EINVAL?
> 
> > +
> > +	if (offset >= GEN7_L3LOG_SIZE)
> > +		return -ENXIO;
> 
> -E2BIG or -ERANGE?
> 
> And maybe some debug messages here so people can debug their daemons in
> case some of the return values are identical.
> 

Good point, I've gone with EPERM, EINVAL, and ENXIO - so no more
duplication and added the debug message for good measure.

> > +
> > +	return 0;
> > +}
> > +
> > +static ssize_t
> > +i915_l3_read(struct file *filp, struct kobject *kobj,
> > +	     struct bin_attribute *attr, char *buf,
> > +	     loff_t offset, size_t count)
> > +{
> > +	struct device *dev = container_of(kobj, struct device, kobj);
> > +	struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
> > +	struct drm_device *drm_dev = dminor->dev;
> > +	struct drm_i915_private *dev_priv = drm_dev->dev_private;
> > +	uint32_t misccpctl;
> > +	int i, ret;
> > +
> > +	ret = l3_access_valid(drm_dev, offset);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = i915_mutex_lock_interruptible(drm_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	misccpctl = I915_READ(GEN7_MISCCPCTL);
> > +	I915_WRITE(GEN7_MISCCPCTL, misccpctl & ~GEN7_DOP_CLOCK_GATE_ENABLE);
> > +
> > +	for (i = offset; count >= 4 && i < GEN7_L3LOG_SIZE; i += 4, count -= 4)
> > +		*((uint32_t *)(&buf[i])) = I915_READ(GEN7_L3LOG_BASE + i);
> > +
> > +	I915_WRITE(GEN7_MISCCPCTL, misccpctl);
> > +
> > +	mutex_unlock(&drm_dev->struct_mutex);
> > +
> > +	return i - offset;
> > +}
> > +
> > +static ssize_t
> > +i915_l3_write(struct file *filp, struct kobject *kobj,
> > +	      struct bin_attribute *attr, char *buf,
> > +	      loff_t offset, size_t count)
> > +{
> > +	struct device *dev = container_of(kobj, struct device, kobj);
> > +	struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
> > +	struct drm_device *drm_dev = dminor->dev;
> > +	struct drm_i915_private *dev_priv = drm_dev->dev_private;
> > +	u32 *temp = NULL;
> > +	int i, ret;
> > +
> > +	ret = l3_access_valid(drm_dev, offset);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = i915_mutex_lock_interruptible(drm_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!dev_priv->mm.l3_remap_info) {
> > +		temp = kzalloc(GEN7_L3LOG_SIZE, GFP_KERNEL);
> > +		if (!temp) {
> > +			mutex_unlock(&drm_dev->struct_mutex);
> > +			return -ENOMEM;
> > +		}
> > +	}
> 
> Grumble, lazy allocation.  But if we did this at driver load time we'd
> also need a valid bit so we knew whether to try to load it or not...
> 

Just curious what the grumble is about? It is a sysfs entry after all.

> > +
> > +	ret = i915_gpu_idle(drm_dev, true);
> > +	if (ret) {
> > +		kfree(temp);
> > +		mutex_unlock(&drm_dev->struct_mutex);
> > +		return ret;
> > +	}
> > +
> > +	/* TODO: Ideally we really want a GPU reset here to make sure errors
> > +	 * aren't propagated Since I cannot find a stable way to reset the GPU
> > +	 * at this point it is left as a TODO.
> > +	*/
> 
> Do we want a reset?  Or just to restart the app that consumed the
> error?  If the latter, then the current uevent ought to be enough,
> potentially with a configurable SIGBUS or something.
> 

I've thought about this a great deal. I think as long as every app
referencing the buffer is killed, and we do a full pipeline flush after
the remapping, we should be able to avoid a reset. I like the idea of
SIGBUS, but it's a bit of work to track which buffer is actually bad,
and with buffer re-use, potentially harder to find the application that
cared. Therefore, I think reset is the easiest way to just make every
app reset.

> > +
> > +	if (dev_priv->mm.l3_remap_info)
> > +		temp = dev_priv->mm.l3_remap_info;
> > +
> > +	dev_priv->mm.l3_remap_info = temp;
> 
> I'd combine these assignments with the kzalloc block above just to keep
> things together.
> 

Leftover cruft from when I had reset code. Thanks.

> > +
> > +	for (i = offset; count >= 4 && i < GEN7_L3LOG_SIZE; i += 4, count -= 4) {
> > +		u32 remap = I915_READ(GEN7_L3LOG_BASE + i);
> > +		if (remap && remap != *temp)
> > +			DRM_ERROR("0x%x was already programmed to %x\n",
> > +				  GEN7_L3LOG_BASE + i, remap);
> > +		*temp++ = *(uint32_t *)(&buf[i]);
> > +	}
> 
> Is this an error?  Or just a "userspace is schizo" and therefore
> just a DEBUG?
> 

You're right. Thanks.

> > +
> > +	i915_gem_l3_remap(drm_dev);
> 
> You could add the warning/debug info to l3_remap instead, right?
> 

Yes. Fixed.

> > +
> > +	mutex_unlock(&drm_dev->struct_mutex);
> > +
> > +	return offset - i;
> > +}
> > +
> > +static struct bin_attribute dpf_attrs = {
> > +	.attr = {.name = "l3_parity", .mode = (S_IRUSR | S_IWUSR)},
> > +	.size = GEN7_L3LOG_SIZE,
> > +	.read = i915_l3_read,
> > +	.write = i915_l3_write,
> > +	.mmap = NULL
> > +};
> > +
> >  void i915_setup_sysfs(struct drm_device *dev)
> >  {
> >  	int ret;
> >  
> > -	/* ILK doesn't have any residency information */
> > +	/* ILK and below don't yet have relevant sysfs files */
> >  	if (INTEL_INFO(dev)->gen < 6)
> >  		return;
> >  
> >  	ret = sysfs_merge_group(&dev->primary->kdev.kobj, &rc6_attr_group);
> >  	if (ret)
> > -		DRM_ERROR("sysfs setup failed\n");
> > +		DRM_ERROR("RC6 residency sysfs setup failed\n");
> > +
> > +	if (!IS_IVYBRIDGE(dev))
> > +		return;
> > +
> > +	ret = device_create_bin_file(&dev->primary->kdev, &dpf_attrs);
> > +	if (ret)
> > +		DRM_ERROR("l3 parity sysfs setup failed\n");
> >  }
> >  
> >  void i915_teardown_sysfs(struct drm_device *dev)
> >  {
> > +	device_remove_bin_file(&dev->primary->kdev,  &dpf_attrs);
> >  	sysfs_unmerge_group(&dev->primary->kdev.kobj, &rc6_attr_group);
> >  }
> 
> I think Daniel covered the above.

Yep.

> 
> With the above addressed:
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 

Thanks.

-- 
Ben Widawsky, Intel Open Source Technology Center