[PATCH 01/10] drm/i915/bdw: Implement a basic PM interrupt handler

[PATCH 01/10] drm/i915/bdw: Implement a basic PM interrupt handler

[deepak.s]

From: Ben Widawsky <benjamin.widawsky@intel.com>

Almost all of it is reusable from the existing code. The primary
difference is we need to do even less in the interrupt handler, since
interrupts are not shared in the same way.

The patch is mostly a copy-paste of the existing snb+ code, with updates
to the relevant parts requiring changes to the interrupt handling. As
such it /should/ be relatively trivial. It's highly likely that I missed
some places where I need a gen8 version of the PM interrupts, but it has
become invisible to me by now.

This patch could probably be split into adding the new functions,
followed by actually handling the interrupts. Since the code is
currently disabled (and broken) I think the patch stands better by
itself.

v2: Move the commit about not touching the ringbuffer interrupt to the
snb_* function where it belongs (Rodrigo)

v3: Rebased on Paulo's runtime PM changes

v4: Not well validated, but rebase on
commit 730488b2eddded4497f63f70867b1256cd9e117c
Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
Date:   Fri Mar 7 20:12:32 2014 -0300

    drm/i915: kill dev_priv->pm.regsave

v5: Rebased on latest code base. (Deepak)

v6: Remove conflict markers, Unnecessary empty line and use right
IIR interrupt (Ville)

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Deepak S <deepak.s@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c  | 75 ++++++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_reg.h  |  1 +
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 drivers/gpu/drm/i915/intel_pm.c  | 38 ++++++++++++++++++--
 4 files changed, 112 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 7a4d3ae..cf29668 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -248,6 +248,49 @@ static bool ivb_can_enable_err_int(struct drm_device *dev)
 	return true;
 }
 
+/**
+  * bdw_update_pm_irq - update GT interrupt 2
+  * @dev_priv: driver private
+  * @interrupt_mask: mask of interrupt bits to update
+  * @enabled_irq_mask: mask of interrupt bits to enable
+  *
+  * Copied from the snb function, updated with relevant register offsets
+  */
+static void bdw_update_pm_irq(struct drm_i915_private *dev_priv,
+			      uint32_t interrupt_mask,
+			      uint32_t enabled_irq_mask)
+{
+	uint32_t new_val;
+
+	assert_spin_locked(&dev_priv->irq_lock);
+
+	if (dev_priv->pm.irqs_disabled) {
+		WARN(1, "IRQs disabled\n");
+		return;
+	}
+
+	new_val = dev_priv->pm_irq_mask;
+	new_val &= ~interrupt_mask;
+	new_val |= (~enabled_irq_mask & interrupt_mask);
+
+	if (new_val != dev_priv->pm_irq_mask) {
+		dev_priv->pm_irq_mask = new_val;
+		I915_WRITE(GEN8_GT_IMR(2), I915_READ(GEN8_GT_IMR(2)) |
+					   dev_priv->pm_irq_mask);
+		POSTING_READ(GEN8_GT_IMR(2));
+	}
+}
+
+void bdw_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
+{
+	bdw_update_pm_irq(dev_priv, mask, mask);
+}
+
+void bdw_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
+{
+	bdw_update_pm_irq(dev_priv, mask, 0);
+}
+
 static bool cpt_can_enable_serr_int(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1126,8 +1169,12 @@ static void gen6_pm_rps_work(struct work_struct *work)
 	spin_lock_irq(&dev_priv->irq_lock);
 	pm_iir = dev_priv->rps.pm_iir;
 	dev_priv->rps.pm_iir = 0;
-	/* Make sure not to corrupt PMIMR state used by ringbuffer code */
-	snb_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
+	if (IS_BROADWELL(dev_priv->dev))
+		bdw_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
+	else {
+		/* Make sure not to corrupt PMIMR state used by ringbuffer */
+		snb_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
+	}
 	spin_unlock_irq(&dev_priv->irq_lock);
 
 	/* Make sure we didn't queue anything we're not going to process. */
@@ -1324,6 +1371,19 @@ static void snb_gt_irq_handler(struct drm_device *dev,
 		ivybridge_parity_error_irq_handler(dev, gt_iir);
 }
 
+static void gen8_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
+{
+	if ((pm_iir & dev_priv->pm_rps_events) == 0)
+		return;
+
+	spin_lock(&dev_priv->irq_lock);
+	dev_priv->rps.pm_iir |= pm_iir & dev_priv->pm_rps_events;
+	bdw_disable_pm_irq(dev_priv, pm_iir & dev_priv->pm_rps_events);
+	spin_unlock(&dev_priv->irq_lock);
+
+	queue_work(dev_priv->wq, &dev_priv->rps.work);
+}
+
 static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
 				       struct drm_i915_private *dev_priv,
 				       u32 master_ctl)
@@ -1359,6 +1419,17 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
 			DRM_ERROR("The master control interrupt lied (GT1)!\n");
 	}
 
+	if (master_ctl & GEN8_GT_PM_IRQ) {
+		tmp = I915_READ(GEN8_GT_IIR(2));
+		if (tmp & dev_priv->pm_rps_events) {
+			ret = IRQ_HANDLED;
+			gen8_rps_irq_handler(dev_priv, tmp);
+			I915_WRITE(GEN8_GT_IIR(2),
+				   tmp & dev_priv->pm_rps_events);
+		} else
+			DRM_ERROR("The master control interrupt lied (PM)!\n");
+	}
+
 	if (master_ctl & GEN8_GT_VECS_IRQ) {
 		tmp = I915_READ(GEN8_GT_IIR(3));
 		if (tmp) {
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8f84555..c2dd436 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4193,6 +4193,7 @@ enum punit_power_well {
 #define  GEN8_DE_PIPE_A_IRQ		(1<<16)
 #define  GEN8_DE_PIPE_IRQ(pipe)		(1<<(16+pipe))
 #define  GEN8_GT_VECS_IRQ		(1<<6)
+#define  GEN8_GT_PM_IRQ			(1<<4)
 #define  GEN8_GT_VCS2_IRQ		(1<<3)
 #define  GEN8_GT_VCS1_IRQ		(1<<2)
 #define  GEN8_GT_BCS_IRQ		(1<<1)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c551472..932c042 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -650,6 +650,8 @@ void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void ilk_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void snb_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
+void bdw_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
+void bdw_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void intel_runtime_pm_disable_interrupts(struct drm_device *dev);
 void intel_runtime_pm_restore_interrupts(struct drm_device *dev);
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 75c1c76..3dccee6 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3210,6 +3210,26 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
 	trace_intel_gpu_freq_change(vlv_gpu_freq(dev_priv, val));
 }
 
+static void gen8_disable_rps_interrupts(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
+	I915_WRITE(GEN8_GT_IER(2), I915_READ(GEN8_GT_IER(2)) &
+				   ~dev_priv->pm_rps_events);
+	/* Complete PM interrupt masking here doesn't race with the rps work
+	 * item again unmasking PM interrupts because that is using a different
+	 * register (GEN8_GT_IMR(2)) to mask PM interrupts. The only risk is in
+	 * leaving stale bits in GEN8_GT_IIR(2) and GEN8_GT_IMR(2) which
+	 * gen8_enable_rps will clean up. */
+
+	spin_lock_irq(&dev_priv->irq_lock);
+	dev_priv->rps.pm_iir = 0;
+	spin_unlock_irq(&dev_priv->irq_lock);
+
+	I915_WRITE(GEN8_GT_IIR(2), dev_priv->pm_rps_events);
+}
+
 static void gen6_disable_rps_interrupts(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3236,7 +3256,10 @@ static void gen6_disable_rps(struct drm_device *dev)
 	I915_WRITE(GEN6_RC_CONTROL, 0);
 	I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
 
-	gen6_disable_rps_interrupts(dev);
+	if (IS_BROADWELL(dev))
+		gen8_disable_rps_interrupts(dev);
+	else
+		gen6_disable_rps_interrupts(dev);
 }
 
 static void valleyview_disable_rps(struct drm_device *dev)
@@ -3276,6 +3299,17 @@ int intel_enable_rc6(const struct drm_device *dev)
 	return INTEL_RC6_ENABLE;
 }
 
+static void gen8_enable_rps_interrupts(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	spin_lock_irq(&dev_priv->irq_lock);
+	WARN_ON(dev_priv->rps.pm_iir);
+	bdw_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
+	I915_WRITE(GEN8_GT_IIR(2), dev_priv->pm_rps_events);
+	spin_unlock_irq(&dev_priv->irq_lock);
+}
+
 static void gen6_enable_rps_interrupts(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3378,7 +3412,7 @@ static void gen8_enable_rps(struct drm_device *dev)
 
 	gen6_set_rps(dev, (I915_READ(GEN6_GT_PERF_STATUS) & 0xff00) >> 8);
 
-	gen6_enable_rps_interrupts(dev);
+	gen8_enable_rps_interrupts(dev);
 
 	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
 }
-- 
1.9.1

[PATCH 02/10] drm/i915: Enable PM Interrupts target via Display Interface.

[deepak.s]

From: Deepak S <deepak.s@linux.intel.com>

In BDW, Apart from unmasking up/down threshold interrupts. we need
to umask bit 32 of PM_INTRMASK to route interrupts to target via Display
Interface.

v2: Add (1<<31) mask (Ville)

Signed-off-by: Deepak S <deepak.s@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 1 +
 drivers/gpu/drm/i915/intel_pm.c | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c2dd436..b951d61 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5105,6 +5105,7 @@ enum punit_power_well {
 #define GEN6_RC6p_THRESHOLD			0xA0BC
 #define GEN6_RC6pp_THRESHOLD			0xA0C0
 #define GEN6_PMINTRMSK				0xA168
+#define GEN8_PMINTR_REDIRECT_TO_NON_DISP	(1<<31)
 
 #define GEN6_PMISR				0x44020
 #define GEN6_PMIMR				0x44024 /* rps_lock */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 3dccee6..f3c5bce 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3066,6 +3066,8 @@ static u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val)
 	if (INTEL_INFO(dev_priv->dev)->gen <= 7 && !IS_HASWELL(dev_priv->dev))
 		mask |= GEN6_PM_RP_UP_EI_EXPIRED;
 
+	mask |= GEN8_PMINTR_REDIRECT_TO_NON_DISP;
+
 	return ~mask;
 }
 
-- 
1.9.1

[PATCH 03/10] drm/i915/chv: Enable Render Standby (RC6) for Cheeryview

[deepak.s]

From: Deepak S <deepak.s@linux.intel.com>

v2: Configure PCBR if BIOS fails allocate pcbr (deepak)

v3: Fix PCBR condition check during CHV RC6 Enable flag set

Signed-off-by: Deepak S <deepak.s@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |   1 +
 drivers/gpu/drm/i915/intel_pm.c | 100 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 99 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b951d61..7090b42 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5134,6 +5134,7 @@ enum punit_power_well {
 #define GEN6_GT_GFX_RC6				0x138108
 #define GEN6_GT_GFX_RC6p			0x13810C
 #define GEN6_GT_GFX_RC6pp			0x138110
+#define VLV_PCBR_ADDR_SHIFT			12
 
 #define GEN6_PCODE_MAILBOX			0x138124
 #define   GEN6_PCODE_READY			(1<<31)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f3c5bce..421a4cc 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3264,6 +3264,18 @@ static void gen6_disable_rps(struct drm_device *dev)
 		gen6_disable_rps_interrupts(dev);
 }
 
+static void cherryview_disable_rps(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	I915_WRITE(GEN6_RC_CONTROL, 0);
+
+	if (dev_priv->vlv_pctx) {
+		drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
+		dev_priv->vlv_pctx = NULL;
+	}
+}
+
 static void valleyview_disable_rps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3642,6 +3654,28 @@ static void valleyview_check_pctx(struct drm_i915_private *dev_priv)
 			     dev_priv->vlv_pctx->stolen->start);
 }
 
+static void cherryview_setup_pctx(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	unsigned long pctx_paddr;
+	struct i915_gtt *gtt = &dev_priv->gtt;
+	u32 pcbr;
+	int pctx_size = 32*1024;
+
+	pcbr = I915_READ(VLV_PCBR);
+	if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) {
+		/*
+		 * From the Gunit register HAS:
+		 * The Gfx driver is expected to program this register and ensure
+		 * proper allocation within Gfx stolen memory.  For example, this
+		 * register should be programmed such than the PCBR range does not
+		 * overlap with other relevant ranges.
+		 */
+		pctx_paddr = (dev_priv->mm.stolen_base + gtt->stolen_size - pctx_size);
+		I915_WRITE(VLV_PCBR, pctx_paddr);
+	}
+}
+
 static void valleyview_setup_pctx(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3697,6 +3731,61 @@ static void valleyview_cleanup_pctx(struct drm_device *dev)
 	dev_priv->vlv_pctx = NULL;
 }
 
+static void cherryview_enable_rps(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_ring_buffer *ring;
+	u32 gtfifodbg, rc6_mode = 0, pcbr;
+	int i;
+
+	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
+
+	if ((gtfifodbg = I915_READ(GTFIFODBG))) {
+		DRM_DEBUG_DRIVER("GT fifo had a previous error %x\n",
+				 gtfifodbg);
+		I915_WRITE(GTFIFODBG, gtfifodbg);
+	}
+
+	cherryview_setup_pctx(dev);
+
+	/* 1a & 1b: Get forcewake during program sequence. Although the driver
+	 * hasn't enabled a state yet where we need forcewake, BIOS may have.*/
+	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
+
+	/* 2a: Program RC6 thresholds.*/
+	I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 40 << 16);
+	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */
+	I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25); /* 25 * 1280ns */
+
+	for_each_ring(ring, dev_priv, i)
+		I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10);
+
+	I915_WRITE(GEN6_RC6_THRESHOLD, 50000); /* 50/125ms per EI */
+
+	/* allows RC6 residency counter to work */
+	I915_WRITE(VLV_COUNTER_CONTROL,
+		   _MASKED_BIT_ENABLE(VLV_COUNT_RANGE_HIGH |
+				      VLV_MEDIA_RC6_COUNT_EN |
+				      VLV_RENDER_RC6_COUNT_EN));
+
+	/* Todo: If BIOS has not configured PCBR
+	 *       then allocate in BIOS Reserved */
+
+	/* For now we assume BIOS is allocating and populating the PCBR  */
+	pcbr = I915_READ(VLV_PCBR);
+
+	DRM_DEBUG_DRIVER("PCBR offset : 0x%x\n", pcbr);
+
+	/* 3: Enable RC6 */
+	if ((intel_enable_rc6(dev) & INTEL_RC6_ENABLE) &&
+						(pcbr >> VLV_PCBR_ADDR_SHIFT))
+		rc6_mode = GEN6_RC_CTL_EI_MODE(1) | VLV_RC_CTL_CTX_RST_PARALLEL;
+
+	I915_WRITE(GEN6_RC_CONTROL, rc6_mode);
+
+	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
+}
+
 static void valleyview_enable_rps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -4550,7 +4639,9 @@ void intel_disable_gt_powersave(struct drm_device *dev)
 		cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
 		cancel_work_sync(&dev_priv->rps.work);
 		mutex_lock(&dev_priv->rps.hw_lock);
-		if (IS_VALLEYVIEW(dev))
+		if (IS_CHERRYVIEW(dev))
+			cherryview_disable_rps(dev);
+		else if (IS_VALLEYVIEW(dev))
 			valleyview_disable_rps(dev);
 		else
 			gen6_disable_rps(dev);
@@ -4568,7 +4659,9 @@ static void intel_gen6_powersave_work(struct work_struct *work)
 
 	mutex_lock(&dev_priv->rps.hw_lock);
 
-	if (IS_VALLEYVIEW(dev)) {
+	if (IS_CHERRYVIEW(dev)) {
+		cherryview_enable_rps(dev);
+	} else if (IS_VALLEYVIEW(dev)) {
 		valleyview_enable_rps(dev);
 	} else if (IS_BROADWELL(dev)) {
 		gen8_enable_rps(dev);
@@ -4590,6 +4683,8 @@ void intel_enable_gt_powersave(struct drm_device *dev)
 		ironlake_enable_rc6(dev);
 		intel_init_emon(dev);
 	} else if (IS_GEN6(dev) || IS_GEN7(dev)) {
+		if (IS_VALLEYVIEW(dev))
+			valleyview_setup_pctx(dev);
 		/*
 		 * PCU communication is slow and this doesn't need to be
 		 * done at any specific time, so do this out of our fast path
@@ -5175,6 +5270,7 @@ static void valleyview_init_clock_gating(struct drm_device *dev)
 		dev_priv->mem_freq = 1333;
 		break;
 	}
+
 	DRM_DEBUG_DRIVER("DDR speed: %d MHz", dev_priv->mem_freq);
 
 	dev_priv->vlv_cdclk_freq = valleyview_cur_cdclk(dev_priv);
-- 
1.9.1

[PATCH 04/10] drm/i915/chv: Added CHV specific register read and write

[deepak.s]

From: Deepak S <deepak.s@linux.intel.com>

Support to individually control Media/Render well based on the register access.
Add CHV specific write function to habdle difference between registers
that are sadowed vs those that need forcewake even for writes.

v2: Drop write FIFO for CHV and add comman well forcewake (Ville)

v3: Fix for decrementing fw count in chv read/write. (Deepak)

Signed-off-by: Deepak S <deepak.s@linux.intel.com>
[vsyrjala: Move the register range macros into intel_uncore.c]
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 139 +++++++++++++++++++++++++++++++++---
 1 file changed, 131 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 2a72bab..11741e4 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -495,6 +495,31 @@ void assert_force_wake_inactive(struct drm_i915_private *dev_priv)
 	((reg) >= 0x22000 && (reg) < 0x24000) ||\
 	((reg) >= 0x30000 && (reg) < 0x40000))
 
+#define FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg) \
+	(((reg) >= 0x2000 && (reg) < 0x4000) ||\
+	((reg) >= 0x5000 && (reg) < 0x8000) ||\
+	((reg) >= 0x8300 && (reg) < 0x8500) ||\
+	((reg) >= 0xB000 && (reg) < 0xC000) ||\
+	((reg) >= 0xE000 && (reg) < 0xE800))
+
+#define FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)\
+	(((reg) >= 0x8800 && (reg) < 0x8900) ||\
+	((reg) >= 0xD000 && (reg) < 0xD800) ||\
+	((reg) >= 0x12000 && (reg) < 0x14000) ||\
+	((reg) >= 0x1A000 && (reg) < 0x1C000) ||\
+	((reg) >= 0x1E800 && (reg) < 0x1EA00) ||\
+	((reg) >= 0x30000 && (reg) < 0x40000))
+
+#define FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)\
+	(((reg) >= 0x4000 && (reg) < 0x5000) ||\
+	((reg) >= 0x8000 && (reg) < 0x8300) ||\
+	((reg) >= 0x8500 && (reg) < 0x8600) ||\
+	((reg) >= 0x9000 && (reg) < 0xB000) ||\
+	((reg) >= 0xC000 && (reg) < 0xc800) ||\
+	((reg) >= 0xF000 && (reg) < 0x10000) ||\
+	((reg) >= 0x14000 && (reg) < 0x14400) ||\
+	((reg) >= 0x22000 && (reg) < 0x24000))
+
 static void
 ilk_dummy_write(struct drm_i915_private *dev_priv)
 {
@@ -588,7 +613,48 @@ vlv_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
 	REG_READ_FOOTER; \
 }
 
+#define __chv_read(x) \
+static u##x \
+chv_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
+	unsigned fwengine = 0; \
+	REG_READ_HEADER(x); \
+	if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) { \
+		fwengine = FORCEWAKE_RENDER; \
+	} \
+	else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) { \
+		fwengine = FORCEWAKE_MEDIA; \
+	} \
+	else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) { \
+		fwengine = FORCEWAKE_ALL; \
+	} \
+	if (FORCEWAKE_RENDER & fwengine) { \
+		if (dev_priv->uncore.fw_rendercount++ == 0) \
+			(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
+								fwengine); \
+	} \
+	if (FORCEWAKE_MEDIA & fwengine) { \
+		if (dev_priv->uncore.fw_mediacount++ == 0) \
+			(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
+								fwengine); \
+	} \
+	val = __raw_i915_read##x(dev_priv, reg); \
+	if (FORCEWAKE_RENDER & fwengine) { \
+		if (--dev_priv->uncore.fw_rendercount == 0) \
+			(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
+								fwengine); \
+	} \
+	if (FORCEWAKE_MEDIA & fwengine) { \
+		if (--dev_priv->uncore.fw_mediacount == 0) \
+			(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
+								fwengine); \
+	} \
+	REG_READ_FOOTER; \
+}
 
+__chv_read(8)
+__chv_read(16)
+__chv_read(32)
+__chv_read(64)
 __vlv_read(8)
 __vlv_read(16)
 __vlv_read(32)
@@ -606,6 +672,7 @@ __gen4_read(16)
 __gen4_read(32)
 __gen4_read(64)
 
+#undef __chv_read
 #undef __vlv_read
 #undef __gen6_read
 #undef __gen5_read
@@ -710,6 +777,49 @@ gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace
 	REG_WRITE_FOOTER; \
 }
 
+#define __chv_write(x) \
+static void \
+chv_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
+	unsigned fwengine = 0; \
+	bool __needs_put = !is_gen8_shadowed(dev_priv, reg); \
+	REG_WRITE_HEADER; \
+	if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) { \
+		fwengine = FORCEWAKE_RENDER; \
+	} \
+	else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) { \
+		fwengine = FORCEWAKE_MEDIA; \
+	} \
+	else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) { \
+		fwengine = FORCEWAKE_ALL; \
+	} \
+	if (__needs_put && (FORCEWAKE_RENDER & fwengine)) { \
+			if (dev_priv->uncore.fw_rendercount++ == 0) \
+				(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
+									fwengine); \
+	} \
+	if (__needs_put && (FORCEWAKE_MEDIA & fwengine)) { \
+		if (dev_priv->uncore.fw_mediacount++ == 0) \
+			(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
+								fwengine); \
+	} \
+	__raw_i915_write##x(dev_priv, reg, val); \
+	if (__needs_put && (FORCEWAKE_RENDER & fwengine)) { \
+			if (--dev_priv->uncore.fw_rendercount == 0) \
+				(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
+									fwengine); \
+	} \
+	if (__needs_put && (FORCEWAKE_MEDIA & fwengine)) { \
+		if (--dev_priv->uncore.fw_mediacount == 0) \
+			(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
+								fwengine); \
+	} \
+	REG_WRITE_FOOTER; \
+}
+
+__chv_write(8)
+__chv_write(16)
+__chv_write(32)
+__chv_write(64)
 __gen8_write(8)
 __gen8_write(16)
 __gen8_write(32)
@@ -731,6 +841,7 @@ __gen4_write(16)
 __gen4_write(32)
 __gen4_write(64)
 
+#undef __chv_write
 #undef __gen8_write
 #undef __hsw_write
 #undef __gen6_write
@@ -794,14 +905,26 @@ void intel_uncore_init(struct drm_device *dev)
 
 	switch (INTEL_INFO(dev)->gen) {
 	default:
-		dev_priv->uncore.funcs.mmio_writeb  = gen8_write8;
-		dev_priv->uncore.funcs.mmio_writew  = gen8_write16;
-		dev_priv->uncore.funcs.mmio_writel  = gen8_write32;
-		dev_priv->uncore.funcs.mmio_writeq  = gen8_write64;
-		dev_priv->uncore.funcs.mmio_readb  = gen6_read8;
-		dev_priv->uncore.funcs.mmio_readw  = gen6_read16;
-		dev_priv->uncore.funcs.mmio_readl  = gen6_read32;
-		dev_priv->uncore.funcs.mmio_readq  = gen6_read64;
+		if (IS_CHERRYVIEW(dev)) {
+			dev_priv->uncore.funcs.mmio_writeb  = chv_write8;
+			dev_priv->uncore.funcs.mmio_writew  = chv_write16;
+			dev_priv->uncore.funcs.mmio_writel  = chv_write32;
+			dev_priv->uncore.funcs.mmio_writeq  = chv_write64;
+			dev_priv->uncore.funcs.mmio_readb  = chv_read8;
+			dev_priv->uncore.funcs.mmio_readw  = chv_read16;
+			dev_priv->uncore.funcs.mmio_readl  = chv_read32;
+			dev_priv->uncore.funcs.mmio_readq  = chv_read64;
+
+		} else {
+			dev_priv->uncore.funcs.mmio_writeb  = gen8_write8;
+			dev_priv->uncore.funcs.mmio_writew  = gen8_write16;
+			dev_priv->uncore.funcs.mmio_writel  = gen8_write32;
+			dev_priv->uncore.funcs.mmio_writeq  = gen8_write64;
+			dev_priv->uncore.funcs.mmio_readb  = gen6_read8;
+			dev_priv->uncore.funcs.mmio_readw  = gen6_read16;
+			dev_priv->uncore.funcs.mmio_readl  = gen6_read32;
+			dev_priv->uncore.funcs.mmio_readq  = gen6_read64;
+		}
 		break;
 	case 7:
 	case 6:
-- 
1.9.1

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

[PATCH 05/10] drm/i915/chv: Enable RPS (Turbo) for Cheeryview

[deepak.s]

From: Deepak S <deepak.s@linux.intel.com>

v2: Disable media turbo and Add DOWN_IDLE_AVG support (Ville)

v3: Mass rename of the dev_priv->rps variables in upstream.

Signed-off-by: Deepak S <deepak.s@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h       |  1 +
 drivers/gpu/drm/i915/i915_reg.h       | 10 +++++
 drivers/gpu/drm/i915/intel_pm.c       | 82 ++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_sideband.c | 15 +++++++
 4 files changed, 107 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7d6acb4..ead2714 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2542,6 +2542,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
 u32 vlv_punit_read(struct drm_i915_private *dev_priv, u8 addr);
 void vlv_punit_write(struct drm_i915_private *dev_priv, u8 addr, u32 val);
 u32 vlv_nc_read(struct drm_i915_private *dev_priv, u8 addr);
+u32 chv_nc_read(struct drm_i915_private *dev_priv, u8 addr);
 u32 vlv_gpio_nc_read(struct drm_i915_private *dev_priv, u32 reg);
 void vlv_gpio_nc_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
 u32 vlv_cck_read(struct drm_i915_private *dev_priv, u32 reg);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 7090b42..f3042bb 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -475,6 +475,7 @@
 #define VLV_IOSF_DATA				(VLV_DISPLAY_BASE + 0x2104)
 #define VLV_IOSF_ADDR				(VLV_DISPLAY_BASE + 0x2108)
 
+#define   CHV_IOSF_PORT_NC			0x04
 /* See configdb bunit SB addr map */
 #define BUNIT_REG_BISOC				0x11
 
@@ -520,6 +521,14 @@ enum punit_power_well {
 #define PUNIT_FUSE_BUS2				0xf6 /* bits 47:40 */
 #define PUNIT_FUSE_BUS1				0xf5 /* bits 55:48 */
 
+#define CHV_IOSF_NC_FB_GFX_FREQ_FUSE		0xdb
+#define CHV_FB_GFX_MAX_FREQ_FUSE_SHIFT		16
+#define CHV_FB_GFX_MAX_FREQ_FUSE_MASK		0xff
+
+#define CHV_IOSF_NC_FB_GFX_RPE_FUSE		0xdf
+#define CHV_FB_RPE_FREQ_SHIFT			8
+#define CHV_FB_RPE_FREQ_MASK			0xff
+
 #define IOSF_NC_FB_GFX_FREQ_FUSE		0x1c
 #define   FB_GFX_MAX_FREQ_FUSE_SHIFT		3
 #define   FB_GFX_MAX_FREQ_FUSE_MASK		0x000007f8
@@ -747,6 +756,7 @@ enum punit_power_well {
 #define   SANDYBRIDGE_FENCE_PITCH_SHIFT	32
 #define   GEN7_FENCE_MAX_PITCH_VAL	0x0800
 
+
 /* control register for cpu gtt access */
 #define TILECTL				0x101000
 #define   TILECTL_SWZCTL			(1 << 0)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 421a4cc..b37d108 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3615,6 +3615,38 @@ void gen6_update_ring_freq(struct drm_device *dev)
 	}
 }
 
+int cherryview_rps_max_freq(struct drm_i915_private *dev_priv)
+{
+	u32 val, rp0;
+
+	val = chv_nc_read(dev_priv, CHV_IOSF_NC_FB_GFX_FREQ_FUSE);
+
+	rp0 = (val >> CHV_FB_GFX_MAX_FREQ_FUSE_SHIFT) &
+					CHV_FB_GFX_MAX_FREQ_FUSE_MASK;
+
+	return rp0;
+}
+
+static int cherryview_rps_rpe_freq(struct drm_i915_private *dev_priv)
+{
+	u32 val, rpe;
+
+	val = chv_nc_read(dev_priv, CHV_IOSF_NC_FB_GFX_RPE_FUSE);
+	rpe = (val >> CHV_FB_RPE_FREQ_SHIFT) & CHV_FB_RPE_FREQ_MASK;
+
+	return rpe;
+}
+
+int cherryview_rps_min_freq(struct drm_i915_private *dev_priv)
+{
+	u32 val, rpn;
+
+	val = chv_nc_read(dev_priv, CHV_IOSF_NC_FB_GFX_FREQ_FUSE);
+	rpn = (val >> CHV_FB_RPE_FREQ_SHIFT) & CHV_FB_RPE_FREQ_MASK;
+
+	return rpn;
+}
+
 int valleyview_rps_max_freq(struct drm_i915_private *dev_priv)
 {
 	u32 val, rp0;
@@ -3735,7 +3767,7 @@ static void cherryview_enable_rps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_ring_buffer *ring;
-	u32 gtfifodbg, rc6_mode = 0, pcbr;
+	u32 gtfifodbg, val, rc6_mode = 0, pcbr;
 	int i;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
@@ -3783,6 +3815,54 @@ static void cherryview_enable_rps(struct drm_device *dev)
 
 	I915_WRITE(GEN6_RC_CONTROL, rc6_mode);
 
+	/* 4 Program defaults and thresholds for RPS*/
+	I915_WRITE(GEN6_RP_UP_THRESHOLD, 59400);
+	I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 245000);
+	I915_WRITE(GEN6_RP_UP_EI, 66000);
+	I915_WRITE(GEN6_RP_DOWN_EI, 350000);
+
+	I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 10);
+
+	/* 5: Enable RPS */
+	I915_WRITE(GEN6_RP_CONTROL,
+		   GEN6_RP_MEDIA_HW_NORMAL_MODE |
+		   GEN6_RP_MEDIA_IS_GFX |
+		   GEN6_RP_ENABLE |
+		   GEN6_RP_UP_BUSY_AVG |
+		   GEN6_RP_DOWN_IDLE_AVG);
+
+	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
+
+	DRM_DEBUG_DRIVER("GPLL enabled? %s\n", val & 0x10 ? "yes" : "no");
+	DRM_DEBUG_DRIVER("GPU status: 0x%08x\n", val);
+
+	dev_priv->rps.cur_freq = (val >> 8) & 0xff;
+	DRM_DEBUG_DRIVER("current GPU freq: %d MHz (%u)\n",
+			 vlv_gpu_freq(dev_priv, dev_priv->rps.cur_freq),
+			 dev_priv->rps.cur_freq);
+
+	dev_priv->rps.max_freq_softlimit = cherryview_rps_max_freq(dev_priv);
+	dev_priv->rps.max_freq = dev_priv->rps.max_freq_softlimit;
+	DRM_DEBUG_DRIVER("max GPU freq: %d MHz (%u)\n",
+			 vlv_gpu_freq(dev_priv, dev_priv->rps.max_freq_softlimit),
+			 dev_priv->rps.max_freq_softlimit);
+
+	dev_priv->rps.efficient_freq = cherryview_rps_rpe_freq(dev_priv);
+	DRM_DEBUG_DRIVER("RPe GPU freq: %d MHz (%u)\n",
+			 vlv_gpu_freq(dev_priv, dev_priv->rps.efficient_freq),
+			 dev_priv->rps.efficient_freq);
+
+	dev_priv->rps.min_freq_softlimit = cherryview_rps_min_freq(dev_priv);
+	DRM_DEBUG_DRIVER("min GPU freq: %d MHz (%u)\n",
+			 vlv_gpu_freq(dev_priv, dev_priv->rps.min_freq_softlimit),
+			 dev_priv->rps.min_freq_softlimit);
+
+	DRM_DEBUG_DRIVER("setting GPU freq to %d MHz (%u)\n",
+			 vlv_gpu_freq(dev_priv, dev_priv->rps.efficient_freq),
+			 dev_priv->rps.efficient_freq);
+
+	valleyview_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
+
 	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
index b1a5514..8f6904d 100644
--- a/drivers/gpu/drm/i915/intel_sideband.c
+++ b/drivers/gpu/drm/i915/intel_sideband.c
@@ -106,6 +106,21 @@ void vlv_bunit_write(struct drm_i915_private *dev_priv, u32 reg, u32 val)
 			PUNIT_OPCODE_REG_WRITE, reg, &val);
 }
 
+u32 chv_nc_read(struct drm_i915_private *dev_priv, u8 addr)
+{
+	u32 val = 0;
+
+	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
+
+	mutex_lock(&dev_priv->dpio_lock);
+	vlv_sideband_rw(dev_priv, PCI_DEVFN(2, 0), CHV_IOSF_PORT_NC,
+			PUNIT_OPCODE_REG_READ, addr, &val);
+	mutex_unlock(&dev_priv->dpio_lock);
+
+	return val;
+}
+
+
 u32 vlv_nc_read(struct drm_i915_private *dev_priv, u8 addr)
 {
 	u32 val = 0;
-- 
1.9.1

[PATCH 06/10] drm/i915/chv: Streamline CHV forcewake stuff

[deepak.s]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Streamline the CHV forcewake functions just like was done for VLV.

This will also fix a bug in accessing the common well registers,
where we'd end up trying to wake up the wells too many times
since we'd call force_wake_get/put twice per register access, with
FORCEFAKE_ALL both times.

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 88 ++++++++++++++-----------------------
 1 file changed, 32 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 11741e4..f1264e2 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -619,35 +619,22 @@ chv_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
 	unsigned fwengine = 0; \
 	REG_READ_HEADER(x); \
 	if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) { \
-		fwengine = FORCEWAKE_RENDER; \
-	} \
-	else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) { \
-		fwengine = FORCEWAKE_MEDIA; \
-	} \
-	else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) { \
-		fwengine = FORCEWAKE_ALL; \
-	} \
-	if (FORCEWAKE_RENDER & fwengine) { \
-		if (dev_priv->uncore.fw_rendercount++ == 0) \
-			(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
-								fwengine); \
-	} \
-	if (FORCEWAKE_MEDIA & fwengine) { \
-		if (dev_priv->uncore.fw_mediacount++ == 0) \
-			(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
-								fwengine); \
+		if (dev_priv->uncore.fw_rendercount == 0) \
+			fwengine = FORCEWAKE_RENDER; \
+	} else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) { \
+		if (dev_priv->uncore.fw_mediacount == 0) \
+			fwengine = FORCEWAKE_MEDIA; \
+	} else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) { \
+		if (dev_priv->uncore.fw_rendercount == 0) \
+			fwengine |= FORCEWAKE_RENDER; \
+		if (dev_priv->uncore.fw_mediacount == 0) \
+			fwengine |= FORCEWAKE_MEDIA; \
 	} \
+	if (fwengine) \
+		dev_priv->uncore.funcs.force_wake_get(dev_priv, fwengine); \
 	val = __raw_i915_read##x(dev_priv, reg); \
-	if (FORCEWAKE_RENDER & fwengine) { \
-		if (--dev_priv->uncore.fw_rendercount == 0) \
-			(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
-								fwengine); \
-	} \
-	if (FORCEWAKE_MEDIA & fwengine) { \
-		if (--dev_priv->uncore.fw_mediacount == 0) \
-			(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
-								fwengine); \
-	} \
+	if (fwengine) \
+		dev_priv->uncore.funcs.force_wake_put(dev_priv, fwengine); \
 	REG_READ_FOOTER; \
 }
 
@@ -781,38 +768,27 @@ gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace
 static void \
 chv_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
 	unsigned fwengine = 0; \
-	bool __needs_put = !is_gen8_shadowed(dev_priv, reg); \
+	bool shadowed = is_gen8_shadowed(dev_priv, reg); \
 	REG_WRITE_HEADER; \
-	if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) { \
-		fwengine = FORCEWAKE_RENDER; \
-	} \
-	else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) { \
-		fwengine = FORCEWAKE_MEDIA; \
-	} \
-	else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) { \
-		fwengine = FORCEWAKE_ALL; \
-	} \
-	if (__needs_put && (FORCEWAKE_RENDER & fwengine)) { \
-			if (dev_priv->uncore.fw_rendercount++ == 0) \
-				(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
-									fwengine); \
-	} \
-	if (__needs_put && (FORCEWAKE_MEDIA & fwengine)) { \
-		if (dev_priv->uncore.fw_mediacount++ == 0) \
-			(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
-								fwengine); \
+	if (!shadowed) { \
+		if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) { \
+			if (dev_priv->uncore.fw_rendercount == 0) \
+				fwengine = FORCEWAKE_RENDER; \
+		} else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) { \
+			if (dev_priv->uncore.fw_mediacount == 0) \
+				fwengine = FORCEWAKE_MEDIA; \
+		} else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) { \
+			if (dev_priv->uncore.fw_rendercount == 0) \
+				fwengine |= FORCEWAKE_RENDER; \
+			if (dev_priv->uncore.fw_mediacount == 0) \
+				fwengine |= FORCEWAKE_MEDIA; \
+		} \
 	} \
+	if (fwengine) \
+		dev_priv->uncore.funcs.force_wake_get(dev_priv, fwengine); \
 	__raw_i915_write##x(dev_priv, reg, val); \
-	if (__needs_put && (FORCEWAKE_RENDER & fwengine)) { \
-			if (--dev_priv->uncore.fw_rendercount == 0) \
-				(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
-									fwengine); \
-	} \
-	if (__needs_put && (FORCEWAKE_MEDIA & fwengine)) { \
-		if (--dev_priv->uncore.fw_mediacount == 0) \
-			(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
-								fwengine); \
-	} \
+	if (fwengine) \
+		dev_priv->uncore.funcs.force_wake_put(dev_priv, fwengine); \
 	REG_WRITE_FOOTER; \
 }
 
-- 
1.9.1

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

[PATCH 07/10] drm/i915/chv: CHV doesn't need WaRsForcewakeWaitTC0

[deepak.s]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Skip __gen6_gt_wait_for_thread_c0() on CHV.

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index f1264e2..b0f0651 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -229,8 +229,8 @@ static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
 	}
 
 	/* WaRsForcewakeWaitTC0:vlv */
-	__gen6_gt_wait_for_thread_c0(dev_priv);
-
+	if (!IS_CHERRYVIEW(dev_priv->dev))
+		__gen6_gt_wait_for_thread_c0(dev_priv);
 }
 
 static void __vlv_force_wake_put(struct drm_i915_private *dev_priv,
-- 
1.9.1

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

[PATCH 08/10] drm/i915/chv: Skip gen6_gt_check_fifodbg() on CHV

[deepak.s]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

CHV uses the gen8 shadow register mechanism so we shouldn't be
checking the GT FIFO status.

This effectively removes the posting read, so add an explicit
posting read using FORCEWAKE_ACK_VLV (which is what use in
vlv_forcewake_reset()).

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index b0f0651..7170506 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -248,9 +248,10 @@ static void __vlv_force_wake_put(struct drm_i915_private *dev_priv,
 		__raw_i915_write32(dev_priv, FORCEWAKE_MEDIA_VLV,
 				_MASKED_BIT_DISABLE(FORCEWAKE_KERNEL));
 
-	/* The below doubles as a POSTING_READ */
-	gen6_gt_check_fifodbg(dev_priv);
-
+	/* something from same cacheline, but !FORCEWAKE_VLV */
+	__raw_posting_read(dev_priv, FORCEWAKE_ACK_VLV);
+	if (!IS_CHERRYVIEW(dev_priv->dev))
+		gen6_gt_check_fifodbg(dev_priv);
 }
 
 static void vlv_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
-- 
1.9.1

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

[PATCH 09/10] drm/i915/chv: Added CHV specific DDR fetch into init_clock_gating

[deepak.s]

From: Deepak S <deepak.s@linux.intel.com>

Signed-off-by: Deepak S <deepak.s@linux.intel.com>
[vsyrjala: Fix merge fubmle where the code ended up in
g4x_disable_trickle_feed() instead of cherryview_init_clock_gating()]
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b37d108..fdb66aa 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3831,7 +3831,18 @@ static void cherryview_enable_rps(struct drm_device *dev)
 		   GEN6_RP_UP_BUSY_AVG |
 		   GEN6_RP_DOWN_IDLE_AVG);
 
+	/* ToDo: Update the mem freq based on latest spec [CHV]*/
 	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
+	switch ((val >> 6) & 3) {
+	case 0:
+	case 1:
+	case 2:
+		dev_priv->mem_freq = 1600;
+		break;
+	case 3:
+		dev_priv->mem_freq = 2000;
+		break;
+	}
 
 	DRM_DEBUG_DRIVER("GPLL enabled? %s\n", val & 0x10 ? "yes" : "no");
 	DRM_DEBUG_DRIVER("GPU status: 0x%08x\n", val);
-- 
1.9.1

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

[PATCH 10/10] drm/i915/chv: Freq(opcode) request value for CHV.

[deepak.s]

From: Deepak S <deepak.s@linux.intel.com>

On CHV, All the freq request should be even. S0, we need to make sure we
request the opcode accordingly.

Signed-off-by: Deepak S <deepak.s@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 9 +++++++++
 drivers/gpu/drm/i915/i915_irq.c | 4 ++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ead2714..5435d87 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2641,6 +2641,15 @@ timespec_to_jiffies_timeout(const struct timespec *value)
 	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
 }
 
+/* rps/turbo related */
+static inline int i915_rps_freq_change(struct drm_device *dev)
+{
+	if (IS_CHERRYVIEW(dev))
+		return 2;
+
+	return 1;
+}
+
 /*
  * If you need to wait X milliseconds between events A and B, but event B
  * doesn't happen exactly after event A, you record the timestamp (jiffies) of
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index cf29668..11538fe 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1190,7 +1190,7 @@ static void gen6_pm_rps_work(struct work_struct *work)
 		if (adj > 0)
 			adj *= 2;
 		else
-			adj = 1;
+			adj = i915_rps_freq_change(dev_priv->dev);
 		new_delay = dev_priv->rps.cur_freq + adj;
 
 		/*
@@ -1209,7 +1209,7 @@ static void gen6_pm_rps_work(struct work_struct *work)
 		if (adj < 0)
 			adj *= 2;
 		else
-			adj = -1;
+			adj = -1 * i915_rps_freq_change(dev_priv->dev);
 		new_delay = dev_priv->rps.cur_freq + adj;
 	} else { /* unknown event */
 		new_delay = dev_priv->rps.cur_freq;
-- 
1.9.1

Re: [PATCH 01/10] drm/i915/bdw: Implement a basic PM interrupt handler

[Ben Widawsky]

On Mon, Apr 21, 2014 at 01:34:05PM +0530, deepak.s@linux.intel.com wrote:
> From: Ben Widawsky <benjamin.widawsky@intel.com>
> 
> Almost all of it is reusable from the existing code. The primary
> difference is we need to do even less in the interrupt handler, since
> interrupts are not shared in the same way.
> 
> The patch is mostly a copy-paste of the existing snb+ code, with updates
> to the relevant parts requiring changes to the interrupt handling. As
> such it /should/ be relatively trivial. It's highly likely that I missed
> some places where I need a gen8 version of the PM interrupts, but it has
> become invisible to me by now.
> 
> This patch could probably be split into adding the new functions,
> followed by actually handling the interrupts. Since the code is
> currently disabled (and broken) I think the patch stands better by
> itself.
> 
> v2: Move the commit about not touching the ringbuffer interrupt to the
> snb_* function where it belongs (Rodrigo)
> 
> v3: Rebased on Paulo's runtime PM changes
> 
> v4: Not well validated, but rebase on
> commit 730488b2eddded4497f63f70867b1256cd9e117c
> Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Date:   Fri Mar 7 20:12:32 2014 -0300
> 
>     drm/i915: kill dev_priv->pm.regsave
> 
> v5: Rebased on latest code base. (Deepak)
> 
> v6: Remove conflict markers, Unnecessary empty line and use right
> IIR interrupt (Ville)
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c  | 75 ++++++++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/i915_reg.h  |  1 +
>  drivers/gpu/drm/i915/intel_drv.h |  2 ++
>  drivers/gpu/drm/i915/intel_pm.c  | 38 ++++++++++++++++++--
>  4 files changed, 112 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 7a4d3ae..cf29668 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -248,6 +248,49 @@ static bool ivb_can_enable_err_int(struct drm_device *dev)
>  	return true;
>  }
>  
> +/**
> +  * bdw_update_pm_irq - update GT interrupt 2
> +  * @dev_priv: driver private
> +  * @interrupt_mask: mask of interrupt bits to update
> +  * @enabled_irq_mask: mask of interrupt bits to enable
> +  *
> +  * Copied from the snb function, updated with relevant register offsets
> +  */
> +static void bdw_update_pm_irq(struct drm_i915_private *dev_priv,
> +			      uint32_t interrupt_mask,
> +			      uint32_t enabled_irq_mask)
> +{
> +	uint32_t new_val;
> +
> +	assert_spin_locked(&dev_priv->irq_lock);
> +
> +	if (dev_priv->pm.irqs_disabled) {
> +		WARN(1, "IRQs disabled\n");
> +		return;
> +	}

If you end up needing to rev again:
if (WARN(dev_priv->pm.irqs_disabled, "IRQs disabled\n"))
	return;

> +
> +	new_val = dev_priv->pm_irq_mask;
> +	new_val &= ~interrupt_mask;
> +	new_val |= (~enabled_irq_mask & interrupt_mask);
> +
> +	if (new_val != dev_priv->pm_irq_mask) {
> +		dev_priv->pm_irq_mask = new_val;
> +		I915_WRITE(GEN8_GT_IMR(2), I915_READ(GEN8_GT_IMR(2)) |
> +					   dev_priv->pm_irq_mask);
> +		POSTING_READ(GEN8_GT_IMR(2));
> +	}
> +}
> +
> +void bdw_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> +{
> +	bdw_update_pm_irq(dev_priv, mask, mask);
> +}
> +
> +void bdw_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> +{
> +	bdw_update_pm_irq(dev_priv, mask, 0);
> +}
> +
>  static bool cpt_can_enable_serr_int(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1126,8 +1169,12 @@ static void gen6_pm_rps_work(struct work_struct *work)
>  	spin_lock_irq(&dev_priv->irq_lock);
>  	pm_iir = dev_priv->rps.pm_iir;
>  	dev_priv->rps.pm_iir = 0;
> -	/* Make sure not to corrupt PMIMR state used by ringbuffer code */
> -	snb_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
> +	if (IS_BROADWELL(dev_priv->dev))
> +		bdw_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
> +	else {
> +		/* Make sure not to corrupt PMIMR state used by ringbuffer */
> +		snb_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
> +	}
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  
>  	/* Make sure we didn't queue anything we're not going to process. */
> @@ -1324,6 +1371,19 @@ static void snb_gt_irq_handler(struct drm_device *dev,
>  		ivybridge_parity_error_irq_handler(dev, gt_iir);
>  }
>  
> +static void gen8_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
> +{
> +	if ((pm_iir & dev_priv->pm_rps_events) == 0)
> +		return;
> +
> +	spin_lock(&dev_priv->irq_lock);
> +	dev_priv->rps.pm_iir |= pm_iir & dev_priv->pm_rps_events;
> +	bdw_disable_pm_irq(dev_priv, pm_iir & dev_priv->pm_rps_events);
> +	spin_unlock(&dev_priv->irq_lock);
> +
> +	queue_work(dev_priv->wq, &dev_priv->rps.work);
> +}
> +
>  static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
>  				       struct drm_i915_private *dev_priv,
>  				       u32 master_ctl)
> @@ -1359,6 +1419,17 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
>  			DRM_ERROR("The master control interrupt lied (GT1)!\n");
>  	}
>  
> +	if (master_ctl & GEN8_GT_PM_IRQ) {
> +		tmp = I915_READ(GEN8_GT_IIR(2));
> +		if (tmp & dev_priv->pm_rps_events) {
> +			ret = IRQ_HANDLED;
> +			gen8_rps_irq_handler(dev_priv, tmp);
> +			I915_WRITE(GEN8_GT_IIR(2),
> +				   tmp & dev_priv->pm_rps_events);
> +		} else
> +			DRM_ERROR("The master control interrupt lied (PM)!\n");
> +	}
> +
>  	if (master_ctl & GEN8_GT_VECS_IRQ) {
>  		tmp = I915_READ(GEN8_GT_IIR(3));
>  		if (tmp) {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8f84555..c2dd436 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4193,6 +4193,7 @@ enum punit_power_well {
>  #define  GEN8_DE_PIPE_A_IRQ		(1<<16)
>  #define  GEN8_DE_PIPE_IRQ(pipe)		(1<<(16+pipe))
>  #define  GEN8_GT_VECS_IRQ		(1<<6)
> +#define  GEN8_GT_PM_IRQ			(1<<4)
>  #define  GEN8_GT_VCS2_IRQ		(1<<3)
>  #define  GEN8_GT_VCS1_IRQ		(1<<2)
>  #define  GEN8_GT_BCS_IRQ		(1<<1)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c551472..932c042 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -650,6 +650,8 @@ void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
>  void ilk_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
>  void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
>  void snb_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> +void bdw_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> +void bdw_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
>  void intel_runtime_pm_disable_interrupts(struct drm_device *dev);
>  void intel_runtime_pm_restore_interrupts(struct drm_device *dev);
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 75c1c76..3dccee6 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3210,6 +3210,26 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
>  	trace_intel_gpu_freq_change(vlv_gpu_freq(dev_priv, val));
>  }
>  
> +static void gen8_disable_rps_interrupts(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> +	I915_WRITE(GEN8_GT_IER(2), I915_READ(GEN8_GT_IER(2)) &
> +				   ~dev_priv->pm_rps_events);
> +	/* Complete PM interrupt masking here doesn't race with the rps work
> +	 * item again unmasking PM interrupts because that is using a different
> +	 * register (GEN8_GT_IMR(2)) to mask PM interrupts. The only risk is in
> +	 * leaving stale bits in GEN8_GT_IIR(2) and GEN8_GT_IMR(2) which
> +	 * gen8_enable_rps will clean up. */
> +
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	dev_priv->rps.pm_iir = 0;
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +
> +	I915_WRITE(GEN8_GT_IIR(2), dev_priv->pm_rps_events);
> +}
> +
>  static void gen6_disable_rps_interrupts(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3236,7 +3256,10 @@ static void gen6_disable_rps(struct drm_device *dev)
>  	I915_WRITE(GEN6_RC_CONTROL, 0);
>  	I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
>  
> -	gen6_disable_rps_interrupts(dev);
> +	if (IS_BROADWELL(dev))
> +		gen8_disable_rps_interrupts(dev);
> +	else
> +		gen6_disable_rps_interrupts(dev);
>  }
>  
>  static void valleyview_disable_rps(struct drm_device *dev)
> @@ -3276,6 +3299,17 @@ int intel_enable_rc6(const struct drm_device *dev)
>  	return INTEL_RC6_ENABLE;
>  }
>  
> +static void gen8_enable_rps_interrupts(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	WARN_ON(dev_priv->rps.pm_iir);
> +	bdw_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
> +	I915_WRITE(GEN8_GT_IIR(2), dev_priv->pm_rps_events);
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +}
> +
>  static void gen6_enable_rps_interrupts(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3378,7 +3412,7 @@ static void gen8_enable_rps(struct drm_device *dev)
>  
>  	gen6_set_rps(dev, (I915_READ(GEN6_GT_PERF_STATUS) & 0xff00) >> 8);
>  
> -	gen6_enable_rps_interrupts(dev);
> +	gen8_enable_rps_interrupts(dev);
>  
>  	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
>  }

lgtm
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 02/10] drm/i915: Enable PM Interrupts target via Display Interface.

[Ben Widawsky]

On Mon, Apr 21, 2014 at 01:34:06PM +0530, deepak.s@linux.intel.com wrote:

For backporters, putting drm/i915/bdw would be helpful.

> From: Deepak S <deepak.s@linux.intel.com>
> 
> In BDW, Apart from unmasking up/down threshold interrupts. we need
> to umask bit 32 of PM_INTRMASK to route interrupts to target via Display
> Interface.
> 
> v2: Add (1<<31) mask (Ville)
> 
> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 1 +
>  drivers/gpu/drm/i915/intel_pm.c | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c2dd436..b951d61 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5105,6 +5105,7 @@ enum punit_power_well {
>  #define GEN6_RC6p_THRESHOLD			0xA0BC
>  #define GEN6_RC6pp_THRESHOLD			0xA0C0
>  #define GEN6_PMINTRMSK				0xA168
> +#define GEN8_PMINTR_REDIRECT_TO_NON_DISP	(1<<31)
>  
>  #define GEN6_PMISR				0x44020
>  #define GEN6_PMIMR				0x44024 /* rps_lock */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 3dccee6..f3c5bce 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3066,6 +3066,8 @@ static u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val)
>  	if (INTEL_INFO(dev_priv->dev)->gen <= 7 && !IS_HASWELL(dev_priv->dev))
>  		mask |= GEN6_PM_RP_UP_EI_EXPIRED;
>  
> +	mask |= GEN8_PMINTR_REDIRECT_TO_NON_DISP;
> +
>  	return ~mask;
>  }
if (IS_BROADWELL(dev)), or gen>= 8
	mask |= GEN8_PMINTR_REDIRECT_TO_NON_DISP;

I didn't read if CHV actually needs it, or not.

With that fixed:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 03/10] drm/i915/chv: Enable Render Standby (RC6) for Cheeryview

[Ben Widawsky]

On Mon, Apr 21, 2014 at 01:34:07PM +0530, deepak.s@linux.intel.com wrote:
> From: Deepak S <deepak.s@linux.intel.com>
> 
> v2: Configure PCBR if BIOS fails allocate pcbr (deepak)
> 
> v3: Fix PCBR condition check during CHV RC6 Enable flag set
> 
> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h |   1 +
>  drivers/gpu/drm/i915/intel_pm.c | 100 +++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 99 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b951d61..7090b42 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5134,6 +5134,7 @@ enum punit_power_well {
>  #define GEN6_GT_GFX_RC6				0x138108
>  #define GEN6_GT_GFX_RC6p			0x13810C
>  #define GEN6_GT_GFX_RC6pp			0x138110
> +#define VLV_PCBR_ADDR_SHIFT			12
>  
>  #define GEN6_PCODE_MAILBOX			0x138124
>  #define   GEN6_PCODE_READY			(1<<31)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f3c5bce..421a4cc 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3264,6 +3264,18 @@ static void gen6_disable_rps(struct drm_device *dev)
>  		gen6_disable_rps_interrupts(dev);
>  }
>  
> +static void cherryview_disable_rps(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	I915_WRITE(GEN6_RC_CONTROL, 0);
> +
> +	if (dev_priv->vlv_pctx) {
> +		drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
> +		dev_priv->vlv_pctx = NULL;
> +	}
> +}
> +
>  static void valleyview_disable_rps(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3642,6 +3654,28 @@ static void valleyview_check_pctx(struct drm_i915_private *dev_priv)
>  			     dev_priv->vlv_pctx->stolen->start);
>  }
>  
> +static void cherryview_setup_pctx(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	unsigned long pctx_paddr;
> +	struct i915_gtt *gtt = &dev_priv->gtt;
> +	u32 pcbr;
> +	int pctx_size = 32*1024;
> +
> +	pcbr = I915_READ(VLV_PCBR);
> +	if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) {
> +		/*
> +		 * From the Gunit register HAS:
> +		 * The Gfx driver is expected to program this register and ensure
> +		 * proper allocation within Gfx stolen memory.  For example, this
> +		 * register should be programmed such than the PCBR range does not
> +		 * overlap with other relevant ranges.
> +		 */
> +		pctx_paddr = (dev_priv->mm.stolen_base + gtt->stolen_size - pctx_size);
> +		I915_WRITE(VLV_PCBR, pctx_paddr);
> +	}
> +}
> +

Is there a reason we did not follow the same idioms as Valleyview?
Shouldn't we be building a stolen object like we do there, and then
using that?

Furthermore, we need to make sure we make the stolen allocator aware for
the case where pcbr is not zero, like we do for valleyview.

I think the best solution here is to try to combine the valleyview and
cherryview logic for this function. Extract out size, and most of the
rest looks pretty similar.

For enabling, I am fine with it as is though provided it's hidden by
preliminary flag.

>  static void valleyview_setup_pctx(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3697,6 +3731,61 @@ static void valleyview_cleanup_pctx(struct drm_device *dev)
>  	dev_priv->vlv_pctx = NULL;
>  }
>  
> +static void cherryview_enable_rps(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_ring_buffer *ring;
> +	u32 gtfifodbg, rc6_mode = 0, pcbr;
> +	int i;
> +
> +	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> +
> +	if ((gtfifodbg = I915_READ(GTFIFODBG))) {
> +		DRM_DEBUG_DRIVER("GT fifo had a previous error %x\n",
> +				 gtfifodbg);
> +		I915_WRITE(GTFIFODBG, gtfifodbg);
> +	}
> +
> +	cherryview_setup_pctx(dev);
> +
> +	/* 1a & 1b: Get forcewake during program sequence. Although the driver
> +	 * hasn't enabled a state yet where we need forcewake, BIOS may have.*/
> +	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> +
> +	/* 2a: Program RC6 thresholds.*/
> +	I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 40 << 16);
> +	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */
> +	I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25); /* 25 * 1280ns */
> +
> +	for_each_ring(ring, dev_priv, i)
> +		I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10);
> +
> +	I915_WRITE(GEN6_RC6_THRESHOLD, 50000); /* 50/125ms per EI */
> +
> +	/* allows RC6 residency counter to work */
> +	I915_WRITE(VLV_COUNTER_CONTROL,
> +		   _MASKED_BIT_ENABLE(VLV_COUNT_RANGE_HIGH |
> +				      VLV_MEDIA_RC6_COUNT_EN |
> +				      VLV_RENDER_RC6_COUNT_EN));
> +
> +	/* Todo: If BIOS has not configured PCBR
> +	 *       then allocate in BIOS Reserved */
> +
> +	/* For now we assume BIOS is allocating and populating the PCBR  */
> +	pcbr = I915_READ(VLV_PCBR);
> +
> +	DRM_DEBUG_DRIVER("PCBR offset : 0x%x\n", pcbr);
> +
> +	/* 3: Enable RC6 */
> +	if ((intel_enable_rc6(dev) & INTEL_RC6_ENABLE) &&
> +						(pcbr >> VLV_PCBR_ADDR_SHIFT))
> +		rc6_mode = GEN6_RC_CTL_EI_MODE(1) | VLV_RC_CTL_CTX_RST_PARALLEL;
> +
> +	I915_WRITE(GEN6_RC_CONTROL, rc6_mode);
> +
> +	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> +}
> +

Can you send me a link to the doc for this off-list? I am having trouble
tracking it down.

>  static void valleyview_enable_rps(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -4550,7 +4639,9 @@ void intel_disable_gt_powersave(struct drm_device *dev)
>  		cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
>  		cancel_work_sync(&dev_priv->rps.work);
>  		mutex_lock(&dev_priv->rps.hw_lock);
> -		if (IS_VALLEYVIEW(dev))
> +		if (IS_CHERRYVIEW(dev))
> +			cherryview_disable_rps(dev);
> +		else if (IS_VALLEYVIEW(dev))
>  			valleyview_disable_rps(dev);
>  		else
>  			gen6_disable_rps(dev);
> @@ -4568,7 +4659,9 @@ static void intel_gen6_powersave_work(struct work_struct *work)
>  
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  
> -	if (IS_VALLEYVIEW(dev)) {
> +	if (IS_CHERRYVIEW(dev)) {
> +		cherryview_enable_rps(dev);
> +	} else if (IS_VALLEYVIEW(dev)) {
>  		valleyview_enable_rps(dev);
>  	} else if (IS_BROADWELL(dev)) {
>  		gen8_enable_rps(dev);
> @@ -4590,6 +4683,8 @@ void intel_enable_gt_powersave(struct drm_device *dev)
>  		ironlake_enable_rc6(dev);
>  		intel_init_emon(dev);
>  	} else if (IS_GEN6(dev) || IS_GEN7(dev)) {
> +		if (IS_VALLEYVIEW(dev))
> +			valleyview_setup_pctx(dev);

Spurious hunk?

>  		/*
>  		 * PCU communication is slow and this doesn't need to be
>  		 * done at any specific time, so do this out of our fast path
> @@ -5175,6 +5270,7 @@ static void valleyview_init_clock_gating(struct drm_device *dev)
>  		dev_priv->mem_freq = 1333;
>  		break;
>  	}
> +

Spurious hunk?

>  	DRM_DEBUG_DRIVER("DDR speed: %d MHz", dev_priv->mem_freq);
>  
>  	dev_priv->vlv_cdclk_freq = valleyview_cur_cdclk(dev_priv);

I think for enabling this patch is fine as is. It does need some of the
fixes I mentioned about. So I'd suggest adding FIXMes with what I
request, and calling this
Acked-by: Ben Widawsky <ben@bwidawsk.net>

To move it up to a reviewed by, I need to find the PM doc (which I
probably have, just don't know it), and I'd like the setup function
fixed.



-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 03/10] drm/i915/chv: Enable Render Standby (RC6) for Cheeryview

[Ben Widawsky]

Also, s/Cheeryview/Cherryview

On Fri, Apr 25, 2014 at 02:42:26PM -0700, Ben Widawsky wrote:
> On Mon, Apr 21, 2014 at 01:34:07PM +0530, deepak.s@linux.intel.com wrote:
> > From: Deepak S <deepak.s@linux.intel.com>
> > 
> > v2: Configure PCBR if BIOS fails allocate pcbr (deepak)
> > 
> > v3: Fix PCBR condition check during CHV RC6 Enable flag set
> > 
> > Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h |   1 +
> >  drivers/gpu/drm/i915/intel_pm.c | 100 +++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 99 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index b951d61..7090b42 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5134,6 +5134,7 @@ enum punit_power_well {
> >  #define GEN6_GT_GFX_RC6				0x138108
> >  #define GEN6_GT_GFX_RC6p			0x13810C
> >  #define GEN6_GT_GFX_RC6pp			0x138110
> > +#define VLV_PCBR_ADDR_SHIFT			12
> >  
> >  #define GEN6_PCODE_MAILBOX			0x138124
> >  #define   GEN6_PCODE_READY			(1<<31)
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index f3c5bce..421a4cc 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3264,6 +3264,18 @@ static void gen6_disable_rps(struct drm_device *dev)
> >  		gen6_disable_rps_interrupts(dev);
> >  }
> >  
> > +static void cherryview_disable_rps(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	I915_WRITE(GEN6_RC_CONTROL, 0);
> > +
> > +	if (dev_priv->vlv_pctx) {
> > +		drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
> > +		dev_priv->vlv_pctx = NULL;
> > +	}
> > +}
> > +
> >  static void valleyview_disable_rps(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -3642,6 +3654,28 @@ static void valleyview_check_pctx(struct drm_i915_private *dev_priv)
> >  			     dev_priv->vlv_pctx->stolen->start);
> >  }
> >  
> > +static void cherryview_setup_pctx(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	unsigned long pctx_paddr;
> > +	struct i915_gtt *gtt = &dev_priv->gtt;
> > +	u32 pcbr;
> > +	int pctx_size = 32*1024;
> > +
> > +	pcbr = I915_READ(VLV_PCBR);
> > +	if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) {
> > +		/*
> > +		 * From the Gunit register HAS:
> > +		 * The Gfx driver is expected to program this register and ensure
> > +		 * proper allocation within Gfx stolen memory.  For example, this
> > +		 * register should be programmed such than the PCBR range does not
> > +		 * overlap with other relevant ranges.
> > +		 */
> > +		pctx_paddr = (dev_priv->mm.stolen_base + gtt->stolen_size - pctx_size);
> > +		I915_WRITE(VLV_PCBR, pctx_paddr);
> > +	}
> > +}
> > +
> 
> Is there a reason we did not follow the same idioms as Valleyview?
> Shouldn't we be building a stolen object like we do there, and then
> using that?
> 
> Furthermore, we need to make sure we make the stolen allocator aware for
> the case where pcbr is not zero, like we do for valleyview.
> 
> I think the best solution here is to try to combine the valleyview and
> cherryview logic for this function. Extract out size, and most of the
> rest looks pretty similar.
> 
> For enabling, I am fine with it as is though provided it's hidden by
> preliminary flag.
> 
> >  static void valleyview_setup_pctx(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -3697,6 +3731,61 @@ static void valleyview_cleanup_pctx(struct drm_device *dev)
> >  	dev_priv->vlv_pctx = NULL;
> >  }
> >  
> > +static void cherryview_enable_rps(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_ring_buffer *ring;
> > +	u32 gtfifodbg, rc6_mode = 0, pcbr;
> > +	int i;
> > +
> > +	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> > +
> > +	if ((gtfifodbg = I915_READ(GTFIFODBG))) {
> > +		DRM_DEBUG_DRIVER("GT fifo had a previous error %x\n",
> > +				 gtfifodbg);
> > +		I915_WRITE(GTFIFODBG, gtfifodbg);
> > +	}
> > +
> > +	cherryview_setup_pctx(dev);
> > +
> > +	/* 1a & 1b: Get forcewake during program sequence. Although the driver
> > +	 * hasn't enabled a state yet where we need forcewake, BIOS may have.*/
> > +	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> > +
> > +	/* 2a: Program RC6 thresholds.*/
> > +	I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 40 << 16);
> > +	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */
> > +	I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25); /* 25 * 1280ns */
> > +
> > +	for_each_ring(ring, dev_priv, i)
> > +		I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10);
> > +
> > +	I915_WRITE(GEN6_RC6_THRESHOLD, 50000); /* 50/125ms per EI */
> > +
> > +	/* allows RC6 residency counter to work */
> > +	I915_WRITE(VLV_COUNTER_CONTROL,
> > +		   _MASKED_BIT_ENABLE(VLV_COUNT_RANGE_HIGH |
> > +				      VLV_MEDIA_RC6_COUNT_EN |
> > +				      VLV_RENDER_RC6_COUNT_EN));
> > +
> > +	/* Todo: If BIOS has not configured PCBR
> > +	 *       then allocate in BIOS Reserved */
> > +
> > +	/* For now we assume BIOS is allocating and populating the PCBR  */
> > +	pcbr = I915_READ(VLV_PCBR);
> > +
> > +	DRM_DEBUG_DRIVER("PCBR offset : 0x%x\n", pcbr);
> > +
> > +	/* 3: Enable RC6 */
> > +	if ((intel_enable_rc6(dev) & INTEL_RC6_ENABLE) &&
> > +						(pcbr >> VLV_PCBR_ADDR_SHIFT))
> > +		rc6_mode = GEN6_RC_CTL_EI_MODE(1) | VLV_RC_CTL_CTX_RST_PARALLEL;
> > +
> > +	I915_WRITE(GEN6_RC_CONTROL, rc6_mode);
> > +
> > +	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> > +}
> > +
> 
> Can you send me a link to the doc for this off-list? I am having trouble
> tracking it down.
> 
> >  static void valleyview_enable_rps(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -4550,7 +4639,9 @@ void intel_disable_gt_powersave(struct drm_device *dev)
> >  		cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
> >  		cancel_work_sync(&dev_priv->rps.work);
> >  		mutex_lock(&dev_priv->rps.hw_lock);
> > -		if (IS_VALLEYVIEW(dev))
> > +		if (IS_CHERRYVIEW(dev))
> > +			cherryview_disable_rps(dev);
> > +		else if (IS_VALLEYVIEW(dev))
> >  			valleyview_disable_rps(dev);
> >  		else
> >  			gen6_disable_rps(dev);
> > @@ -4568,7 +4659,9 @@ static void intel_gen6_powersave_work(struct work_struct *work)
> >  
> >  	mutex_lock(&dev_priv->rps.hw_lock);
> >  
> > -	if (IS_VALLEYVIEW(dev)) {
> > +	if (IS_CHERRYVIEW(dev)) {
> > +		cherryview_enable_rps(dev);
> > +	} else if (IS_VALLEYVIEW(dev)) {
> >  		valleyview_enable_rps(dev);
> >  	} else if (IS_BROADWELL(dev)) {
> >  		gen8_enable_rps(dev);
> > @@ -4590,6 +4683,8 @@ void intel_enable_gt_powersave(struct drm_device *dev)
> >  		ironlake_enable_rc6(dev);
> >  		intel_init_emon(dev);
> >  	} else if (IS_GEN6(dev) || IS_GEN7(dev)) {
> > +		if (IS_VALLEYVIEW(dev))
> > +			valleyview_setup_pctx(dev);
> 
> Spurious hunk?
> 
> >  		/*
> >  		 * PCU communication is slow and this doesn't need to be
> >  		 * done at any specific time, so do this out of our fast path
> > @@ -5175,6 +5270,7 @@ static void valleyview_init_clock_gating(struct drm_device *dev)
> >  		dev_priv->mem_freq = 1333;
> >  		break;
> >  	}
> > +
> 
> Spurious hunk?
> 
> >  	DRM_DEBUG_DRIVER("DDR speed: %d MHz", dev_priv->mem_freq);
> >  
> >  	dev_priv->vlv_cdclk_freq = valleyview_cur_cdclk(dev_priv);
> 
> I think for enabling this patch is fine as is. It does need some of the
> fixes I mentioned about. So I'd suggest adding FIXMes with what I
> request, and calling this
> Acked-by: Ben Widawsky <ben@bwidawsk.net>
> 
> To move it up to a reviewed by, I need to find the PM doc (which I
> probably have, just don't know it), and I'd like the setup function
> fixed.
> 
> 
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 04/10] drm/i915/chv: Added CHV specific register read and write

[Ben Widawsky]

On Mon, Apr 21, 2014 at 01:34:08PM +0530, deepak.s@linux.intel.com wrote:
> From: Deepak S <deepak.s@linux.intel.com>
> 
> Support to individually control Media/Render well based on the register access.
> Add CHV specific write function to habdle difference between registers
> that are sadowed vs those that need forcewake even for writes.
> 
> v2: Drop write FIFO for CHV and add comman well forcewake (Ville)
> 
> v3: Fix for decrementing fw count in chv read/write. (Deepak)
> 
> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> [vsyrjala: Move the register range macros into intel_uncore.c]
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I left some comments on the first sending of this patch, and it's not
clear if you ignored them intentionally or not. Inquiring minds would
like to know.

I'll repeat some of the ones I feel are more important below.

> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 139 +++++++++++++++++++++++++++++++++---
>  1 file changed, 131 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 2a72bab..11741e4 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -495,6 +495,31 @@ void assert_force_wake_inactive(struct drm_i915_private *dev_priv)
>  	((reg) >= 0x22000 && (reg) < 0x24000) ||\
>  	((reg) >= 0x30000 && (reg) < 0x40000))
>  
> +#define FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg) \
> +	(((reg) >= 0x2000 && (reg) < 0x4000) ||\
> +	((reg) >= 0x5000 && (reg) < 0x8000) ||\
> +	((reg) >= 0x8300 && (reg) < 0x8500) ||\
> +	((reg) >= 0xB000 && (reg) < 0xC000) ||\
> +	((reg) >= 0xE000 && (reg) < 0xE800))
> +
> +#define FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)\
> +	(((reg) >= 0x8800 && (reg) < 0x8900) ||\
> +	((reg) >= 0xD000 && (reg) < 0xD800) ||\
> +	((reg) >= 0x12000 && (reg) < 0x14000) ||\
> +	((reg) >= 0x1A000 && (reg) < 0x1C000) ||\
> +	((reg) >= 0x1E800 && (reg) < 0x1EA00) ||\
> +	((reg) >= 0x30000 && (reg) < 0x40000))
> +
> +#define FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)\
> +	(((reg) >= 0x4000 && (reg) < 0x5000) ||\
> +	((reg) >= 0x8000 && (reg) < 0x8300) ||\
> +	((reg) >= 0x8500 && (reg) < 0x8600) ||\
> +	((reg) >= 0x9000 && (reg) < 0xB000) ||\
> +	((reg) >= 0xC000 && (reg) < 0xc800) ||\
> +	((reg) >= 0xF000 && (reg) < 0x10000) ||\
> +	((reg) >= 0x14000 && (reg) < 0x14400) ||\
> +	((reg) >= 0x22000 && (reg) < 0x24000))
> +
>  static void
>  ilk_dummy_write(struct drm_i915_private *dev_priv)
>  {
> @@ -588,7 +613,48 @@ vlv_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
>  	REG_READ_FOOTER; \
>  }
>  
> +#define __chv_read(x) \
> +static u##x \
> +chv_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
> +	unsigned fwengine = 0; \
> +	REG_READ_HEADER(x); \
> +	if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) { \
> +		fwengine = FORCEWAKE_RENDER; \
> +	} \
> +	else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) { \
> +		fwengine = FORCEWAKE_MEDIA; \
> +	} \
> +	else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) { \
> +		fwengine = FORCEWAKE_ALL; \
> +	} \

These don't following linux kernel coding style

> +	if (FORCEWAKE_RENDER & fwengine) { \
> +		if (dev_priv->uncore.fw_rendercount++ == 0) \
> +			(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
> +								fwengine); \
> +	} \
> +	if (FORCEWAKE_MEDIA & fwengine) { \
> +		if (dev_priv->uncore.fw_mediacount++ == 0) \
> +			(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
> +								fwengine); \
> +	} \
> +	val = __raw_i915_read##x(dev_priv, reg); \
> +	if (FORCEWAKE_RENDER & fwengine) { \
> +		if (--dev_priv->uncore.fw_rendercount == 0) \
> +			(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
> +								fwengine); \
> +	} \
> +	if (FORCEWAKE_MEDIA & fwengine) { \
> +		if (--dev_priv->uncore.fw_mediacount == 0) \
> +			(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
> +								fwengine); \
> +	} \
> +	REG_READ_FOOTER; \
> +}

It seems like it makes a lot more sense to pass register offset to
force_wake_put() and let the logic occur there instead of making a big
ugly macro. We can cheat and use the existing functions since fw_engine
was defined as an int, and the register range fits within that.

>  
> +__chv_read(8)
> +__chv_read(16)
> +__chv_read(32)
> +__chv_read(64)
>  __vlv_read(8)
>  __vlv_read(16)
>  __vlv_read(32)
> @@ -606,6 +672,7 @@ __gen4_read(16)
>  __gen4_read(32)
>  __gen4_read(64)
>  
> +#undef __chv_read
>  #undef __vlv_read
>  #undef __gen6_read
>  #undef __gen5_read
> @@ -710,6 +777,49 @@ gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace
>  	REG_WRITE_FOOTER; \
>  }
>  
> +#define __chv_write(x) \
> +static void \
> +chv_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
> +	unsigned fwengine = 0; \
> +	bool __needs_put = !is_gen8_shadowed(dev_priv, reg); \
> +	REG_WRITE_HEADER; \
> +	if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) { \
> +		fwengine = FORCEWAKE_RENDER; \
> +	} \
> +	else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) { \
> +		fwengine = FORCEWAKE_MEDIA; \
> +	} \
> +	else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) { \
> +		fwengine = FORCEWAKE_ALL; \
> +	} \

coding convention again

> +	if (__needs_put && (FORCEWAKE_RENDER & fwengine)) { \
> +			if (dev_priv->uncore.fw_rendercount++ == 0) \
> +				(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
> +									fwengine); \
> +	} \
> +	if (__needs_put && (FORCEWAKE_MEDIA & fwengine)) { \
> +		if (dev_priv->uncore.fw_mediacount++ == 0) \
> +			(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
> +								fwengine); \
> +	} \
> +	__raw_i915_write##x(dev_priv, reg, val); \
> +	if (__needs_put && (FORCEWAKE_RENDER & fwengine)) { \
> +			if (--dev_priv->uncore.fw_rendercount == 0) \
> +				(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
> +									fwengine); \
> +	} \
> +	if (__needs_put && (FORCEWAKE_MEDIA & fwengine)) { \
> +		if (--dev_priv->uncore.fw_mediacount == 0) \
> +			(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
> +								fwengine); \
> +	} \

Same comment above

> +	REG_WRITE_FOOTER; \
> +}
> +
> +__chv_write(8)
> +__chv_write(16)
> +__chv_write(32)
> +__chv_write(64)
>  __gen8_write(8)
>  __gen8_write(16)
>  __gen8_write(32)
> @@ -731,6 +841,7 @@ __gen4_write(16)
>  __gen4_write(32)
>  __gen4_write(64)
>  
> +#undef __chv_write
>  #undef __gen8_write
>  #undef __hsw_write
>  #undef __gen6_write
> @@ -794,14 +905,26 @@ void intel_uncore_init(struct drm_device *dev)
>  
>  	switch (INTEL_INFO(dev)->gen) {
>  	default:
> -		dev_priv->uncore.funcs.mmio_writeb  = gen8_write8;
> -		dev_priv->uncore.funcs.mmio_writew  = gen8_write16;
> -		dev_priv->uncore.funcs.mmio_writel  = gen8_write32;
> -		dev_priv->uncore.funcs.mmio_writeq  = gen8_write64;
> -		dev_priv->uncore.funcs.mmio_readb  = gen6_read8;
> -		dev_priv->uncore.funcs.mmio_readw  = gen6_read16;
> -		dev_priv->uncore.funcs.mmio_readl  = gen6_read32;
> -		dev_priv->uncore.funcs.mmio_readq  = gen6_read64;
> +		if (IS_CHERRYVIEW(dev)) {
> +			dev_priv->uncore.funcs.mmio_writeb  = chv_write8;
> +			dev_priv->uncore.funcs.mmio_writew  = chv_write16;
> +			dev_priv->uncore.funcs.mmio_writel  = chv_write32;
> +			dev_priv->uncore.funcs.mmio_writeq  = chv_write64;
> +			dev_priv->uncore.funcs.mmio_readb  = chv_read8;
> +			dev_priv->uncore.funcs.mmio_readw  = chv_read16;
> +			dev_priv->uncore.funcs.mmio_readl  = chv_read32;
> +			dev_priv->uncore.funcs.mmio_readq  = chv_read64;
> +
> +		} else {
> +			dev_priv->uncore.funcs.mmio_writeb  = gen8_write8;
> +			dev_priv->uncore.funcs.mmio_writew  = gen8_write16;
> +			dev_priv->uncore.funcs.mmio_writel  = gen8_write32;
> +			dev_priv->uncore.funcs.mmio_writeq  = gen8_write64;
> +			dev_priv->uncore.funcs.mmio_readb  = gen6_read8;
> +			dev_priv->uncore.funcs.mmio_readw  = gen6_read16;
> +			dev_priv->uncore.funcs.mmio_readl  = gen6_read32;
> +			dev_priv->uncore.funcs.mmio_readq  = gen6_read64;
> +		}
>  		break;
>  	case 7:
>  	case 6:

I think if you make CHV have it's own forcewake get/put, then you can
just use the the same MMIO functions.

Like the previous patch, I am having trouble finding where the register
ranges exist. Assuming those are correct (and I'm guessing Ville checked
thoroughly), the patch looks correct.

So I'd prefer to at least explore my suggestions, but it's
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/10] drm/i915/chv: Enable RPS (Turbo) for Cheeryview

[Ben Widawsky]

s/Cheeryview/Cherryview

On Mon, Apr 21, 2014 at 01:34:09PM +0530, deepak.s@linux.intel.com wrote:
> From: Deepak S <deepak.s@linux.intel.com>
> 
> v2: Disable media turbo and Add DOWN_IDLE_AVG support (Ville)
> 
> v3: Mass rename of the dev_priv->rps variables in upstream.
> 
> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |  1 +
>  drivers/gpu/drm/i915/i915_reg.h       | 10 +++++
>  drivers/gpu/drm/i915/intel_pm.c       | 82 ++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_sideband.c | 15 +++++++
>  4 files changed, 107 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7d6acb4..ead2714 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2542,6 +2542,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
>  u32 vlv_punit_read(struct drm_i915_private *dev_priv, u8 addr);
>  void vlv_punit_write(struct drm_i915_private *dev_priv, u8 addr, u32 val);
>  u32 vlv_nc_read(struct drm_i915_private *dev_priv, u8 addr);
> +u32 chv_nc_read(struct drm_i915_private *dev_priv, u8 addr);
>  u32 vlv_gpio_nc_read(struct drm_i915_private *dev_priv, u32 reg);
>  void vlv_gpio_nc_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
>  u32 vlv_cck_read(struct drm_i915_private *dev_priv, u32 reg);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 7090b42..f3042bb 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -475,6 +475,7 @@
>  #define VLV_IOSF_DATA				(VLV_DISPLAY_BASE + 0x2104)
>  #define VLV_IOSF_ADDR				(VLV_DISPLAY_BASE + 0x2108)
>  
> +#define   CHV_IOSF_PORT_NC			0x04
>  /* See configdb bunit SB addr map */
>  #define BUNIT_REG_BISOC				0x11
>  
> @@ -520,6 +521,14 @@ enum punit_power_well {
>  #define PUNIT_FUSE_BUS2				0xf6 /* bits 47:40 */
>  #define PUNIT_FUSE_BUS1				0xf5 /* bits 55:48 */
>  
> +#define CHV_IOSF_NC_FB_GFX_FREQ_FUSE		0xdb
> +#define CHV_FB_GFX_MAX_FREQ_FUSE_SHIFT		16
> +#define CHV_FB_GFX_MAX_FREQ_FUSE_MASK		0xff

I'm having trouble finding this one.

> +
> +#define CHV_IOSF_NC_FB_GFX_RPE_FUSE		0xdf
> +#define CHV_FB_RPE_FREQ_SHIFT			8
> +#define CHV_FB_RPE_FREQ_MASK			0xff
> +

Oddly, I found this one, but some places seem to contradict it's
meaning. Perhaps I am reading it incorrectly.

>  #define IOSF_NC_FB_GFX_FREQ_FUSE		0x1c
>  #define   FB_GFX_MAX_FREQ_FUSE_SHIFT		3
>  #define   FB_GFX_MAX_FREQ_FUSE_MASK		0x000007f8
> @@ -747,6 +756,7 @@ enum punit_power_well {
>  #define   SANDYBRIDGE_FENCE_PITCH_SHIFT	32
>  #define   GEN7_FENCE_MAX_PITCH_VAL	0x0800
>  
> +

spurious

>  /* control register for cpu gtt access */
>  #define TILECTL				0x101000
>  #define   TILECTL_SWZCTL			(1 << 0)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 421a4cc..b37d108 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3615,6 +3615,38 @@ void gen6_update_ring_freq(struct drm_device *dev)
>  	}
>  }
>  
> +int cherryview_rps_max_freq(struct drm_i915_private *dev_priv)
> +{
> +	u32 val, rp0;
> +
> +	val = chv_nc_read(dev_priv, CHV_IOSF_NC_FB_GFX_FREQ_FUSE);
> +
> +	rp0 = (val >> CHV_FB_GFX_MAX_FREQ_FUSE_SHIFT) &
> +					CHV_FB_GFX_MAX_FREQ_FUSE_MASK;

Looks a bit weird style

> +
> +	return rp0;
> +}
> +
> +static int cherryview_rps_rpe_freq(struct drm_i915_private *dev_priv)
> +{
> +	u32 val, rpe;
> +
> +	val = chv_nc_read(dev_priv, CHV_IOSF_NC_FB_GFX_RPE_FUSE);
> +	rpe = (val >> CHV_FB_RPE_FREQ_SHIFT) & CHV_FB_RPE_FREQ_MASK;
> +
> +	return rpe;
> +}
> +
> +int cherryview_rps_min_freq(struct drm_i915_private *dev_priv)
> +{
> +	u32 val, rpn;
> +
> +	val = chv_nc_read(dev_priv, CHV_IOSF_NC_FB_GFX_FREQ_FUSE);
> +	rpn = (val >> CHV_FB_RPE_FREQ_SHIFT) & CHV_FB_RPE_FREQ_MASK;
> +
> +	return rpn;
> +}
> +

Is this actually min? I think it's looks a lot better to just call
cherryview_rps_rpe_freq() from this function instead of copy pasting the
code.

>  int valleyview_rps_max_freq(struct drm_i915_private *dev_priv)
>  {
>  	u32 val, rp0;
> @@ -3735,7 +3767,7 @@ static void cherryview_enable_rps(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_ring_buffer *ring;
> -	u32 gtfifodbg, rc6_mode = 0, pcbr;
> +	u32 gtfifodbg, val, rc6_mode = 0, pcbr;
>  	int i;
>  
>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> @@ -3783,6 +3815,54 @@ static void cherryview_enable_rps(struct drm_device *dev)
>  
>  	I915_WRITE(GEN6_RC_CONTROL, rc6_mode);
>  
> +	/* 4 Program defaults and thresholds for RPS*/
> +	I915_WRITE(GEN6_RP_UP_THRESHOLD, 59400);
> +	I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 245000);
> +	I915_WRITE(GEN6_RP_UP_EI, 66000);
> +	I915_WRITE(GEN6_RP_DOWN_EI, 350000);
> +
> +	I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 10);
> +
> +	/* 5: Enable RPS */
> +	I915_WRITE(GEN6_RP_CONTROL,
> +		   GEN6_RP_MEDIA_HW_NORMAL_MODE |
> +		   GEN6_RP_MEDIA_IS_GFX |
> +		   GEN6_RP_ENABLE |
> +		   GEN6_RP_UP_BUSY_AVG |
> +		   GEN6_RP_DOWN_IDLE_AVG);
> +
> +	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
> +
> +	DRM_DEBUG_DRIVER("GPLL enabled? %s\n", val & 0x10 ? "yes" : "no");
> +	DRM_DEBUG_DRIVER("GPU status: 0x%08x\n", val);
> +
> +	dev_priv->rps.cur_freq = (val >> 8) & 0xff;
> +	DRM_DEBUG_DRIVER("current GPU freq: %d MHz (%u)\n",
> +			 vlv_gpu_freq(dev_priv, dev_priv->rps.cur_freq),
> +			 dev_priv->rps.cur_freq);
> +
> +	dev_priv->rps.max_freq_softlimit = cherryview_rps_max_freq(dev_priv);
> +	dev_priv->rps.max_freq = dev_priv->rps.max_freq_softlimit;
> +	DRM_DEBUG_DRIVER("max GPU freq: %d MHz (%u)\n",
> +			 vlv_gpu_freq(dev_priv, dev_priv->rps.max_freq_softlimit),
> +			 dev_priv->rps.max_freq_softlimit);
> +
> +	dev_priv->rps.efficient_freq = cherryview_rps_rpe_freq(dev_priv);
> +	DRM_DEBUG_DRIVER("RPe GPU freq: %d MHz (%u)\n",
> +			 vlv_gpu_freq(dev_priv, dev_priv->rps.efficient_freq),
> +			 dev_priv->rps.efficient_freq);
> +
> +	dev_priv->rps.min_freq_softlimit = cherryview_rps_min_freq(dev_priv);
> +	DRM_DEBUG_DRIVER("min GPU freq: %d MHz (%u)\n",
> +			 vlv_gpu_freq(dev_priv, dev_priv->rps.min_freq_softlimit),
> +			 dev_priv->rps.min_freq_softlimit);
> +
> +	DRM_DEBUG_DRIVER("setting GPU freq to %d MHz (%u)\n",
> +			 vlv_gpu_freq(dev_priv, dev_priv->rps.efficient_freq),
> +			 dev_priv->rps.efficient_freq);
> +
> +	valleyview_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
> +
>  	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
> index b1a5514..8f6904d 100644
> --- a/drivers/gpu/drm/i915/intel_sideband.c
> +++ b/drivers/gpu/drm/i915/intel_sideband.c
> @@ -106,6 +106,21 @@ void vlv_bunit_write(struct drm_i915_private *dev_priv, u32 reg, u32 val)
>  			PUNIT_OPCODE_REG_WRITE, reg, &val);
>  }
>  
> +u32 chv_nc_read(struct drm_i915_private *dev_priv, u8 addr)
> +{
> +	u32 val = 0;
> +
> +	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> +
> +	mutex_lock(&dev_priv->dpio_lock);
> +	vlv_sideband_rw(dev_priv, PCI_DEVFN(2, 0), CHV_IOSF_PORT_NC,
> +			PUNIT_OPCODE_REG_READ, addr, &val);
> +	mutex_unlock(&dev_priv->dpio_lock);
> +
> +	return val;
> +}
> +
> +
>  u32 vlv_nc_read(struct drm_i915_private *dev_priv, u8 addr)
>  {
>  	u32 val = 0;

Looks fine as an enabling patch hidden behind prelim HW. I need some
more info/help to do a proper review.
Acked-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 06/10] drm/i915/chv: Streamline CHV forcewake stuff

[Ben Widawsky]

On Mon, Apr 21, 2014 at 01:34:10PM +0530, deepak.s@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Streamline the CHV forcewake functions just like was done for VLV.
> 
> This will also fix a bug in accessing the common well registers,
> where we'd end up trying to wake up the wells too many times
> since we'd call force_wake_get/put twice per register access, with
> FORCEFAKE_ALL both times.
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Already reviewed. I guess this patch makes what I was requesting earlier
a bit harder to implement. :-(

> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 88 ++++++++++++++-----------------------
>  1 file changed, 32 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 11741e4..f1264e2 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -619,35 +619,22 @@ chv_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
>  	unsigned fwengine = 0; \
>  	REG_READ_HEADER(x); \
>  	if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) { \
> -		fwengine = FORCEWAKE_RENDER; \
> -	} \
> -	else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) { \
> -		fwengine = FORCEWAKE_MEDIA; \
> -	} \
> -	else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) { \
> -		fwengine = FORCEWAKE_ALL; \
> -	} \
> -	if (FORCEWAKE_RENDER & fwengine) { \
> -		if (dev_priv->uncore.fw_rendercount++ == 0) \
> -			(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
> -								fwengine); \
> -	} \
> -	if (FORCEWAKE_MEDIA & fwengine) { \
> -		if (dev_priv->uncore.fw_mediacount++ == 0) \
> -			(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
> -								fwengine); \
> +		if (dev_priv->uncore.fw_rendercount == 0) \
> +			fwengine = FORCEWAKE_RENDER; \
> +	} else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) { \
> +		if (dev_priv->uncore.fw_mediacount == 0) \
> +			fwengine = FORCEWAKE_MEDIA; \
> +	} else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) { \
> +		if (dev_priv->uncore.fw_rendercount == 0) \
> +			fwengine |= FORCEWAKE_RENDER; \
> +		if (dev_priv->uncore.fw_mediacount == 0) \
> +			fwengine |= FORCEWAKE_MEDIA; \
>  	} \
> +	if (fwengine) \
> +		dev_priv->uncore.funcs.force_wake_get(dev_priv, fwengine); \
>  	val = __raw_i915_read##x(dev_priv, reg); \
> -	if (FORCEWAKE_RENDER & fwengine) { \
> -		if (--dev_priv->uncore.fw_rendercount == 0) \
> -			(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
> -								fwengine); \
> -	} \
> -	if (FORCEWAKE_MEDIA & fwengine) { \
> -		if (--dev_priv->uncore.fw_mediacount == 0) \
> -			(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
> -								fwengine); \
> -	} \
> +	if (fwengine) \
> +		dev_priv->uncore.funcs.force_wake_put(dev_priv, fwengine); \
>  	REG_READ_FOOTER; \
>  }
>  
> @@ -781,38 +768,27 @@ gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace
>  static void \
>  chv_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
>  	unsigned fwengine = 0; \
> -	bool __needs_put = !is_gen8_shadowed(dev_priv, reg); \
> +	bool shadowed = is_gen8_shadowed(dev_priv, reg); \
>  	REG_WRITE_HEADER; \
> -	if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) { \
> -		fwengine = FORCEWAKE_RENDER; \
> -	} \
> -	else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) { \
> -		fwengine = FORCEWAKE_MEDIA; \
> -	} \
> -	else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) { \
> -		fwengine = FORCEWAKE_ALL; \
> -	} \
> -	if (__needs_put && (FORCEWAKE_RENDER & fwengine)) { \
> -			if (dev_priv->uncore.fw_rendercount++ == 0) \
> -				(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
> -									fwengine); \
> -	} \
> -	if (__needs_put && (FORCEWAKE_MEDIA & fwengine)) { \
> -		if (dev_priv->uncore.fw_mediacount++ == 0) \
> -			(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
> -								fwengine); \
> +	if (!shadowed) { \
> +		if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) { \
> +			if (dev_priv->uncore.fw_rendercount == 0) \
> +				fwengine = FORCEWAKE_RENDER; \
> +		} else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) { \
> +			if (dev_priv->uncore.fw_mediacount == 0) \
> +				fwengine = FORCEWAKE_MEDIA; \
> +		} else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) { \
> +			if (dev_priv->uncore.fw_rendercount == 0) \
> +				fwengine |= FORCEWAKE_RENDER; \
> +			if (dev_priv->uncore.fw_mediacount == 0) \
> +				fwengine |= FORCEWAKE_MEDIA; \
> +		} \

Nice target here we could extract out as common between read/write.

>  	} \
> +	if (fwengine) \
> +		dev_priv->uncore.funcs.force_wake_get(dev_priv, fwengine); \
>  	__raw_i915_write##x(dev_priv, reg, val); \
> -	if (__needs_put && (FORCEWAKE_RENDER & fwengine)) { \
> -			if (--dev_priv->uncore.fw_rendercount == 0) \
> -				(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
> -									fwengine); \
> -	} \
> -	if (__needs_put && (FORCEWAKE_MEDIA & fwengine)) { \
> -		if (--dev_priv->uncore.fw_mediacount == 0) \
> -			(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
> -								fwengine); \
> -	} \
> +	if (fwengine) \
> +		dev_priv->uncore.funcs.force_wake_put(dev_priv, fwengine); \
>  	REG_WRITE_FOOTER; \
>  }
>  

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/10] drm/i915/chv: Skip gen6_gt_check_fifodbg() on CHV

[Ben Widawsky]

On Mon, Apr 21, 2014 at 01:34:12PM +0530, deepak.s@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> CHV uses the gen8 shadow register mechanism so we shouldn't be
> checking the GT FIFO status.
> 
> This effectively removes the posting read, so add an explicit
> posting read using FORCEWAKE_ACK_VLV (which is what use in
> vlv_forcewake_reset()).
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index b0f0651..7170506 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -248,9 +248,10 @@ static void __vlv_force_wake_put(struct drm_i915_private *dev_priv,
>  		__raw_i915_write32(dev_priv, FORCEWAKE_MEDIA_VLV,
>  				_MASKED_BIT_DISABLE(FORCEWAKE_KERNEL));
>  
> -	/* The below doubles as a POSTING_READ */
> -	gen6_gt_check_fifodbg(dev_priv);
> -
> +	/* something from same cacheline, but !FORCEWAKE_VLV */
> +	__raw_posting_read(dev_priv, FORCEWAKE_ACK_VLV);
> +	if (!IS_CHERRYVIEW(dev_priv->dev))
> +		gen6_gt_check_fifodbg(dev_priv);

You could save a read for the VLV case, but no big deal.

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

>  }
>  
>  static void vlv_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/10] drm/i915/chv: Added CHV specific DDR fetch into init_clock_gating

[Ben Widawsky]

On Mon, Apr 21, 2014 at 01:34:13PM +0530, deepak.s@linux.intel.com wrote:
> From: Deepak S <deepak.s@linux.intel.com>
> 
> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> [vsyrjala: Fix merge fubmle where the code ended up in
> g4x_disable_trickle_feed() instead of cherryview_init_clock_gating()]
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index b37d108..fdb66aa 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3831,7 +3831,18 @@ static void cherryview_enable_rps(struct drm_device *dev)
>  		   GEN6_RP_UP_BUSY_AVG |
>  		   GEN6_RP_DOWN_IDLE_AVG);
>  
> +	/* ToDo: Update the mem freq based on latest spec [CHV]*/
>  	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
> +	switch ((val >> 6) & 3) {
> +	case 0:
> +	case 1:
> +	case 2:
> +		dev_priv->mem_freq = 1600;
> +		break;
> +	case 3:
> +		dev_priv->mem_freq = 2000;
> +		break;
> +	}
>  
>  	DRM_DEBUG_DRIVER("GPLL enabled? %s\n", val & 0x10 ? "yes" : "no");
>  	DRM_DEBUG_DRIVER("GPU status: 0x%08x\n", val);

This, like all the other patches related to freq. don't seem to be
findable by me. Seems fine for enabling

Acked-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/10] drm/i915/chv: Freq(opcode) request value for CHV.

[Ben Widawsky]

On Mon, Apr 21, 2014 at 01:34:14PM +0530, deepak.s@linux.intel.com wrote:
> From: Deepak S <deepak.s@linux.intel.com>
> 
> On CHV, All the freq request should be even. S0, we need to make sure we
> request the opcode accordingly.
> 
> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 9 +++++++++
>  drivers/gpu/drm/i915/i915_irq.c | 4 ++--
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ead2714..5435d87 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2641,6 +2641,15 @@ timespec_to_jiffies_timeout(const struct timespec *value)
>  	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
>  }
>  
> +/* rps/turbo related */
> +static inline int i915_rps_freq_change(struct drm_device *dev)
> +{
> +	if (IS_CHERRYVIEW(dev))
> +		return 2;
> +
> +	return 1;
> +}
> +
>  /*
>   * If you need to wait X milliseconds between events A and B, but event B
>   * doesn't happen exactly after event A, you record the timestamp (jiffies) of
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index cf29668..11538fe 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1190,7 +1190,7 @@ static void gen6_pm_rps_work(struct work_struct *work)
>  		if (adj > 0)
>  			adj *= 2;
>  		else
> -			adj = 1;
> +			adj = i915_rps_freq_change(dev_priv->dev);
>  		new_delay = dev_priv->rps.cur_freq + adj;
>  
>  		/*
> @@ -1209,7 +1209,7 @@ static void gen6_pm_rps_work(struct work_struct *work)
>  		if (adj < 0)
>  			adj *= 2;
>  		else
> -			adj = -1;
> +			adj = -1 * i915_rps_freq_change(dev_priv->dev);
>  		new_delay = dev_priv->rps.cur_freq + adj;
>  	} else { /* unknown event */
>  		new_delay = dev_priv->rps.cur_freq;

splitting hairs a bit, but adding a new function that isn't named well
doesn't really improve readability. Since we only ever do it for one
case, I think this logic is better stuffed in Cherryview specific
_set_rps() function.

I don't see anything incorrect though. I'd prefer my recommendation, but
it's 
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 03/10] drm/i915/chv: Enable Render Standby (RC6) for Cheeryview

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 7020 bytes --]

On Mon, 2014-04-21 at 13:34 +0530, deepak.s@linux.intel.com wrote:
> From: Deepak S <deepak.s@linux.intel.com>
> 
> v2: Configure PCBR if BIOS fails allocate pcbr (deepak)
> 
> v3: Fix PCBR condition check during CHV RC6 Enable flag set
> 
> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h |   1 +
>  drivers/gpu/drm/i915/intel_pm.c | 100 +++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 99 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b951d61..7090b42 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5134,6 +5134,7 @@ enum punit_power_well {
>  #define GEN6_GT_GFX_RC6				0x138108
>  #define GEN6_GT_GFX_RC6p			0x13810C
>  #define GEN6_GT_GFX_RC6pp			0x138110
> +#define VLV_PCBR_ADDR_SHIFT			12
>  
>  #define GEN6_PCODE_MAILBOX			0x138124
>  #define   GEN6_PCODE_READY			(1<<31)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f3c5bce..421a4cc 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3264,6 +3264,18 @@ static void gen6_disable_rps(struct drm_device *dev)
>  		gen6_disable_rps_interrupts(dev);
>  }
>  
> +static void cherryview_disable_rps(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	I915_WRITE(GEN6_RC_CONTROL, 0);
> +
> +	if (dev_priv->vlv_pctx) {
> +		drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
> +		dev_priv->vlv_pctx = NULL;
> +	}
> +}
> +
>  static void valleyview_disable_rps(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3642,6 +3654,28 @@ static void valleyview_check_pctx(struct drm_i915_private *dev_priv)
>  			     dev_priv->vlv_pctx->stolen->start);
>  }
>  
> +static void cherryview_setup_pctx(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	unsigned long pctx_paddr;
> +	struct i915_gtt *gtt = &dev_priv->gtt;
> +	u32 pcbr;
> +	int pctx_size = 32*1024;
> +
> +	pcbr = I915_READ(VLV_PCBR);
> +	if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) {
> +		/*
> +		 * From the Gunit register HAS:
> +		 * The Gfx driver is expected to program this register and ensure
> +		 * proper allocation within Gfx stolen memory.  For example, this
> +		 * register should be programmed such than the PCBR range does not
> +		 * overlap with other relevant ranges.
> +		 */
> +		pctx_paddr = (dev_priv->mm.stolen_base + gtt->stolen_size - pctx_size);

This area should be reserved.

> +		I915_WRITE(VLV_PCBR, pctx_paddr);
> +	}
> +}
> +
>  static void valleyview_setup_pctx(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3697,6 +3731,61 @@ static void valleyview_cleanup_pctx(struct drm_device *dev)
>  	dev_priv->vlv_pctx = NULL;
>  }
>  
> +static void cherryview_enable_rps(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_ring_buffer *ring;
> +	u32 gtfifodbg, rc6_mode = 0, pcbr;
> +	int i;
> +
> +	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> +
> +	if ((gtfifodbg = I915_READ(GTFIFODBG))) {
> +		DRM_DEBUG_DRIVER("GT fifo had a previous error %x\n",
> +				 gtfifodbg);
> +		I915_WRITE(GTFIFODBG, gtfifodbg);
> +	}
> +
> +	cherryview_setup_pctx(dev);

This should be called from intel_init_gt_powersave().

> +
> +	/* 1a & 1b: Get forcewake during program sequence. Although the driver
> +	 * hasn't enabled a state yet where we need forcewake, BIOS may have.*/
> +	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> +
> +	/* 2a: Program RC6 thresholds.*/
> +	I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 40 << 16);
> +	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */
> +	I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25); /* 25 * 1280ns */
> +
> +	for_each_ring(ring, dev_priv, i)
> +		I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10);
> +
> +	I915_WRITE(GEN6_RC6_THRESHOLD, 50000); /* 50/125ms per EI */
> +
> +	/* allows RC6 residency counter to work */
> +	I915_WRITE(VLV_COUNTER_CONTROL,
> +		   _MASKED_BIT_ENABLE(VLV_COUNT_RANGE_HIGH |
> +				      VLV_MEDIA_RC6_COUNT_EN |
> +				      VLV_RENDER_RC6_COUNT_EN));
> +
> +	/* Todo: If BIOS has not configured PCBR
> +	 *       then allocate in BIOS Reserved */
> +
> +	/* For now we assume BIOS is allocating and populating the PCBR  */
> +	pcbr = I915_READ(VLV_PCBR);
> +
> +	DRM_DEBUG_DRIVER("PCBR offset : 0x%x\n", pcbr);
> +
> +	/* 3: Enable RC6 */
> +	if ((intel_enable_rc6(dev) & INTEL_RC6_ENABLE) &&
> +						(pcbr >> VLV_PCBR_ADDR_SHIFT))

This may break runtime PM, if PCBR isn't setup, since we assume that if
intel_enable_rc6() returns a non-zero value RC6 will be truly enabled.
So the check for PCBR should be moved to sanitize_rc6_option(). Also
note that for CHV PCBR setup cannot fail atm.

> +		rc6_mode = GEN6_RC_CTL_EI_MODE(1) | VLV_RC_CTL_CTX_RST_PARALLEL;
> +
> +	I915_WRITE(GEN6_RC_CONTROL, rc6_mode);
> +
> +	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> +}
> +
>  static void valleyview_enable_rps(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -4550,7 +4639,9 @@ void intel_disable_gt_powersave(struct drm_device *dev)
>  		cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
>  		cancel_work_sync(&dev_priv->rps.work);
>  		mutex_lock(&dev_priv->rps.hw_lock);
> -		if (IS_VALLEYVIEW(dev))
> +		if (IS_CHERRYVIEW(dev))
> +			cherryview_disable_rps(dev);
> +		else if (IS_VALLEYVIEW(dev))
>  			valleyview_disable_rps(dev);
>  		else
>  			gen6_disable_rps(dev);
> @@ -4568,7 +4659,9 @@ static void intel_gen6_powersave_work(struct work_struct *work)
>  
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  
> -	if (IS_VALLEYVIEW(dev)) {
> +	if (IS_CHERRYVIEW(dev)) {
> +		cherryview_enable_rps(dev);
> +	} else if (IS_VALLEYVIEW(dev)) {
>  		valleyview_enable_rps(dev);
>  	} else if (IS_BROADWELL(dev)) {
>  		gen8_enable_rps(dev);
> @@ -4590,6 +4683,8 @@ void intel_enable_gt_powersave(struct drm_device *dev)
>  		ironlake_enable_rc6(dev);
>  		intel_init_emon(dev);
>  	} else if (IS_GEN6(dev) || IS_GEN7(dev)) {
> +		if (IS_VALLEYVIEW(dev))
> +			valleyview_setup_pctx(dev);

Spurious hunk, as Ben pointed out, and the GEN check will be false for
CherryView. The corresponding GEN check must be updated in
intel_disable_gt_powersave() too.

>  		/*
>  		 * PCU communication is slow and this doesn't need to be
>  		 * done at any specific time, so do this out of our fast path
> @@ -5175,6 +5270,7 @@ static void valleyview_init_clock_gating(struct drm_device *dev)
>  		dev_priv->mem_freq = 1333;
>  		break;
>  	}
> +
>  	DRM_DEBUG_DRIVER("DDR speed: %d MHz", dev_priv->mem_freq);
>  
>  	dev_priv->vlv_cdclk_freq = valleyview_cur_cdclk(dev_priv);


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 03/10] drm/i915/chv: Enable Render Standby (RC6) for Cheeryview

[Daniel Vetter]

On Mon, Apr 28, 2014 at 05:29:46PM +0300, Imre Deak wrote:
> > +static void cherryview_setup_pctx(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	unsigned long pctx_paddr;
> > +	struct i915_gtt *gtt = &dev_priv->gtt;
> > +	u32 pcbr;
> > +	int pctx_size = 32*1024;
> > +
> > +	pcbr = I915_READ(VLV_PCBR);
> > +	if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) {
> > +		/*
> > +		 * From the Gunit register HAS:
> > +		 * The Gfx driver is expected to program this register and ensure
> > +		 * proper allocation within Gfx stolen memory.  For example, this
> > +		 * register should be programmed such than the PCBR range does not
> > +		 * overlap with other relevant ranges.
> > +		 */
> > +		pctx_paddr = (dev_priv->mm.stolen_base + gtt->stolen_size - pctx_size);
> 
> This area should be reserved.

We've had a really lengthy discussion internally about the bios-reserved
chunk in stolen. It was stalled due to (imo unjustified) fear to leak
information what the bios actually uses this for.

If we need to reserve more of stolen than we currently do we need to pick
up that approach again instead of adding more bandaids.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 03/10] drm/i915/chv: Enable Render Standby (RC6) for Cheeryview

[Deepak S]

On Monday 28 April 2014 08:15 PM, Daniel Vetter wrote:
> On Mon, Apr 28, 2014 at 05:29:46PM +0300, Imre Deak wrote:
>>> +static void cherryview_setup_pctx(struct drm_device *dev)
>>> +{
>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>> +	unsigned long pctx_paddr;
>>> +	struct i915_gtt *gtt = &dev_priv->gtt;
>>> +	u32 pcbr;
>>> +	int pctx_size = 32*1024;
>>> +
>>> +	pcbr = I915_READ(VLV_PCBR);
>>> +	if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) {
>>> +		/*
>>> +		 * From the Gunit register HAS:
>>> +		 * The Gfx driver is expected to program this register and ensure
>>> +		 * proper allocation within Gfx stolen memory.  For example, this
>>> +		 * register should be programmed such than the PCBR range does not
>>> +		 * overlap with other relevant ranges.
>>> +		 */
>>> +		pctx_paddr = (dev_priv->mm.stolen_base + gtt->stolen_size - pctx_size);
>> This area should be reserved.
> We've had a really lengthy discussion internally about the bios-reserved
> chunk in stolen. It was stalled due to (imo unjustified) fear to leak
> information what the bios actually uses this for.
>
> If we need to reserve more of stolen than we currently do we need to pick
> up that approach again instead of adding more bandaids.
> -Daniel

Agreed. Will change accordingly.

Re: [PATCH 03/10] drm/i915/chv: Enable Render Standby (RC6) for Cheeryview

[Deepak S]

Thanks for the review. I will address the comments


On Monday 28 April 2014 07:59 PM, Imre Deak wrote:
> On Mon, 2014-04-21 at 13:34 +0530, deepak.s@linux.intel.com wrote:
>> From: Deepak S <deepak.s@linux.intel.com>
>>
>> v2: Configure PCBR if BIOS fails allocate pcbr (deepak)
>>
>> v3: Fix PCBR condition check during CHV RC6 Enable flag set
>>
>> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h |   1 +
>>   drivers/gpu/drm/i915/intel_pm.c | 100 +++++++++++++++++++++++++++++++++++++++-
>>   2 files changed, 99 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index b951d61..7090b42 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -5134,6 +5134,7 @@ enum punit_power_well {
>>   #define GEN6_GT_GFX_RC6				0x138108
>>   #define GEN6_GT_GFX_RC6p			0x13810C
>>   #define GEN6_GT_GFX_RC6pp			0x138110
>> +#define VLV_PCBR_ADDR_SHIFT			12
>>   
>>   #define GEN6_PCODE_MAILBOX			0x138124
>>   #define   GEN6_PCODE_READY			(1<<31)
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index f3c5bce..421a4cc 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3264,6 +3264,18 @@ static void gen6_disable_rps(struct drm_device *dev)
>>   		gen6_disable_rps_interrupts(dev);
>>   }
>>   
>> +static void cherryview_disable_rps(struct drm_device *dev)
>> +{
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +	I915_WRITE(GEN6_RC_CONTROL, 0);
>> +
>> +	if (dev_priv->vlv_pctx) {
>> +		drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
>> +		dev_priv->vlv_pctx = NULL;
>> +	}
>> +}
>> +
>>   static void valleyview_disable_rps(struct drm_device *dev)
>>   {
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -3642,6 +3654,28 @@ static void valleyview_check_pctx(struct drm_i915_private *dev_priv)
>>   			     dev_priv->vlv_pctx->stolen->start);
>>   }
>>   
>> +static void cherryview_setup_pctx(struct drm_device *dev)
>> +{
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	unsigned long pctx_paddr;
>> +	struct i915_gtt *gtt = &dev_priv->gtt;
>> +	u32 pcbr;
>> +	int pctx_size = 32*1024;
>> +
>> +	pcbr = I915_READ(VLV_PCBR);
>> +	if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) {
>> +		/*
>> +		 * From the Gunit register HAS:
>> +		 * The Gfx driver is expected to program this register and ensure
>> +		 * proper allocation within Gfx stolen memory.  For example, this
>> +		 * register should be programmed such than the PCBR range does not
>> +		 * overlap with other relevant ranges.
>> +		 */
>> +		pctx_paddr = (dev_priv->mm.stolen_base + gtt->stolen_size - pctx_size);
> This area should be reserved.
>
>> +		I915_WRITE(VLV_PCBR, pctx_paddr);
>> +	}
>> +}
>> +
>>   static void valleyview_setup_pctx(struct drm_device *dev)
>>   {
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -3697,6 +3731,61 @@ static void valleyview_cleanup_pctx(struct drm_device *dev)
>>   	dev_priv->vlv_pctx = NULL;
>>   }
>>   
>> +static void cherryview_enable_rps(struct drm_device *dev)
>> +{
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct intel_ring_buffer *ring;
>> +	u32 gtfifodbg, rc6_mode = 0, pcbr;
>> +	int i;
>> +
>> +	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>> +
>> +	if ((gtfifodbg = I915_READ(GTFIFODBG))) {
>> +		DRM_DEBUG_DRIVER("GT fifo had a previous error %x\n",
>> +				 gtfifodbg);
>> +		I915_WRITE(GTFIFODBG, gtfifodbg);
>> +	}
>> +
>> +	cherryview_setup_pctx(dev);
> This should be called from intel_init_gt_powersave().
>
>> +
>> +	/* 1a & 1b: Get forcewake during program sequence. Although the driver
>> +	 * hasn't enabled a state yet where we need forcewake, BIOS may have.*/
>> +	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
>> +
>> +	/* 2a: Program RC6 thresholds.*/
>> +	I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 40 << 16);
>> +	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */
>> +	I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25); /* 25 * 1280ns */
>> +
>> +	for_each_ring(ring, dev_priv, i)
>> +		I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10);
>> +
>> +	I915_WRITE(GEN6_RC6_THRESHOLD, 50000); /* 50/125ms per EI */
>> +
>> +	/* allows RC6 residency counter to work */
>> +	I915_WRITE(VLV_COUNTER_CONTROL,
>> +		   _MASKED_BIT_ENABLE(VLV_COUNT_RANGE_HIGH |
>> +				      VLV_MEDIA_RC6_COUNT_EN |
>> +				      VLV_RENDER_RC6_COUNT_EN));
>> +
>> +	/* Todo: If BIOS has not configured PCBR
>> +	 *       then allocate in BIOS Reserved */
>> +
>> +	/* For now we assume BIOS is allocating and populating the PCBR  */
>> +	pcbr = I915_READ(VLV_PCBR);
>> +
>> +	DRM_DEBUG_DRIVER("PCBR offset : 0x%x\n", pcbr);
>> +
>> +	/* 3: Enable RC6 */
>> +	if ((intel_enable_rc6(dev) & INTEL_RC6_ENABLE) &&
>> +						(pcbr >> VLV_PCBR_ADDR_SHIFT))
> This may break runtime PM, if PCBR isn't setup, since we assume that if
> intel_enable_rc6() returns a non-zero value RC6 will be truly enabled.
> So the check for PCBR should be moved to sanitize_rc6_option(). Also
> note that for CHV PCBR setup cannot fail atm.
>
>> +		rc6_mode = GEN6_RC_CTL_EI_MODE(1) | VLV_RC_CTL_CTX_RST_PARALLEL;
>> +
>> +	I915_WRITE(GEN6_RC_CONTROL, rc6_mode);
>> +
>> +	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
>> +}
>> +
>>   static void valleyview_enable_rps(struct drm_device *dev)
>>   {
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -4550,7 +4639,9 @@ void intel_disable_gt_powersave(struct drm_device *dev)
>>   		cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
>>   		cancel_work_sync(&dev_priv->rps.work);
>>   		mutex_lock(&dev_priv->rps.hw_lock);
>> -		if (IS_VALLEYVIEW(dev))
>> +		if (IS_CHERRYVIEW(dev))
>> +			cherryview_disable_rps(dev);
>> +		else if (IS_VALLEYVIEW(dev))
>>   			valleyview_disable_rps(dev);
>>   		else
>>   			gen6_disable_rps(dev);
>> @@ -4568,7 +4659,9 @@ static void intel_gen6_powersave_work(struct work_struct *work)
>>   
>>   	mutex_lock(&dev_priv->rps.hw_lock);
>>   
>> -	if (IS_VALLEYVIEW(dev)) {
>> +	if (IS_CHERRYVIEW(dev)) {
>> +		cherryview_enable_rps(dev);
>> +	} else if (IS_VALLEYVIEW(dev)) {
>>   		valleyview_enable_rps(dev);
>>   	} else if (IS_BROADWELL(dev)) {
>>   		gen8_enable_rps(dev);
>> @@ -4590,6 +4683,8 @@ void intel_enable_gt_powersave(struct drm_device *dev)
>>   		ironlake_enable_rc6(dev);
>>   		intel_init_emon(dev);
>>   	} else if (IS_GEN6(dev) || IS_GEN7(dev)) {
>> +		if (IS_VALLEYVIEW(dev))
>> +			valleyview_setup_pctx(dev);
> Spurious hunk, as Ben pointed out, and the GEN check will be false for
> CherryView. The corresponding GEN check must be updated in
> intel_disable_gt_powersave() too.
>
>>   		/*
>>   		 * PCU communication is slow and this doesn't need to be
>>   		 * done at any specific time, so do this out of our fast path
>> @@ -5175,6 +5270,7 @@ static void valleyview_init_clock_gating(struct drm_device *dev)
>>   		dev_priv->mem_freq = 1333;
>>   		break;
>>   	}
>> +
>>   	DRM_DEBUG_DRIVER("DDR speed: %d MHz", dev_priv->mem_freq);
>>   
>>   	dev_priv->vlv_cdclk_freq = valleyview_cur_cdclk(dev_priv);

Re: [PATCH 03/10] drm/i915/chv: Enable Render Standby (RC6) for Cheeryview

[Deepak S]

Thanks for the review. I will address the comments

On Saturday 26 April 2014 03:12 AM, Ben Widawsky wrote:
> On Mon, Apr 21, 2014 at 01:34:07PM +0530, deepak.s@linux.intel.com wrote:
>> From: Deepak S <deepak.s@linux.intel.com>
>>
>> v2: Configure PCBR if BIOS fails allocate pcbr (deepak)
>>
>> v3: Fix PCBR condition check during CHV RC6 Enable flag set
>>
>> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h |   1 +
>>   drivers/gpu/drm/i915/intel_pm.c | 100 +++++++++++++++++++++++++++++++++++++++-
>>   2 files changed, 99 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index b951d61..7090b42 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -5134,6 +5134,7 @@ enum punit_power_well {
>>   #define GEN6_GT_GFX_RC6				0x138108
>>   #define GEN6_GT_GFX_RC6p			0x13810C
>>   #define GEN6_GT_GFX_RC6pp			0x138110
>> +#define VLV_PCBR_ADDR_SHIFT			12
>>   
>>   #define GEN6_PCODE_MAILBOX			0x138124
>>   #define   GEN6_PCODE_READY			(1<<31)
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index f3c5bce..421a4cc 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3264,6 +3264,18 @@ static void gen6_disable_rps(struct drm_device *dev)
>>   		gen6_disable_rps_interrupts(dev);
>>   }
>>   
>> +static void cherryview_disable_rps(struct drm_device *dev)
>> +{
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +	I915_WRITE(GEN6_RC_CONTROL, 0);
>> +
>> +	if (dev_priv->vlv_pctx) {
>> +		drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
>> +		dev_priv->vlv_pctx = NULL;
>> +	}
>> +}
>> +
>>   static void valleyview_disable_rps(struct drm_device *dev)
>>   {
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -3642,6 +3654,28 @@ static void valleyview_check_pctx(struct drm_i915_private *dev_priv)
>>   			     dev_priv->vlv_pctx->stolen->start);
>>   }
>>   
>> +static void cherryview_setup_pctx(struct drm_device *dev)
>> +{
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	unsigned long pctx_paddr;
>> +	struct i915_gtt *gtt = &dev_priv->gtt;
>> +	u32 pcbr;
>> +	int pctx_size = 32*1024;
>> +
>> +	pcbr = I915_READ(VLV_PCBR);
>> +	if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) {
>> +		/*
>> +		 * From the Gunit register HAS:
>> +		 * The Gfx driver is expected to program this register and ensure
>> +		 * proper allocation within Gfx stolen memory.  For example, this
>> +		 * register should be programmed such than the PCBR range does not
>> +		 * overlap with other relevant ranges.
>> +		 */
>> +		pctx_paddr = (dev_priv->mm.stolen_base + gtt->stolen_size - pctx_size);
>> +		I915_WRITE(VLV_PCBR, pctx_paddr);
>> +	}
>> +}
>> +
> Is there a reason we did not follow the same idioms as Valleyview?
> Shouldn't we be building a stolen object like we do there, and then
> using that?
>
> Furthermore, we need to make sure we make the stolen allocator aware for
> the case where pcbr is not zero, like we do for valleyview.
>
> I think the best solution here is to try to combine the valleyview and
> cherryview logic for this function. Extract out size, and most of the
> rest looks pretty similar.
>
> For enabling, I am fine with it as is though provided it's hidden by
> preliminary flag.
>
>>   static void valleyview_setup_pctx(struct drm_device *dev)
>>   {
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -3697,6 +3731,61 @@ static void valleyview_cleanup_pctx(struct drm_device *dev)
>>   	dev_priv->vlv_pctx = NULL;
>>   }
>>   
>> +static void cherryview_enable_rps(struct drm_device *dev)
>> +{
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct intel_ring_buffer *ring;
>> +	u32 gtfifodbg, rc6_mode = 0, pcbr;
>> +	int i;
>> +
>> +	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>> +
>> +	if ((gtfifodbg = I915_READ(GTFIFODBG))) {
>> +		DRM_DEBUG_DRIVER("GT fifo had a previous error %x\n",
>> +				 gtfifodbg);
>> +		I915_WRITE(GTFIFODBG, gtfifodbg);
>> +	}
>> +
>> +	cherryview_setup_pctx(dev);
>> +
>> +	/* 1a & 1b: Get forcewake during program sequence. Although the driver
>> +	 * hasn't enabled a state yet where we need forcewake, BIOS may have.*/
>> +	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
>> +
>> +	/* 2a: Program RC6 thresholds.*/
>> +	I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 40 << 16);
>> +	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */
>> +	I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25); /* 25 * 1280ns */
>> +
>> +	for_each_ring(ring, dev_priv, i)
>> +		I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10);
>> +
>> +	I915_WRITE(GEN6_RC6_THRESHOLD, 50000); /* 50/125ms per EI */
>> +
>> +	/* allows RC6 residency counter to work */
>> +	I915_WRITE(VLV_COUNTER_CONTROL,
>> +		   _MASKED_BIT_ENABLE(VLV_COUNT_RANGE_HIGH |
>> +				      VLV_MEDIA_RC6_COUNT_EN |
>> +				      VLV_RENDER_RC6_COUNT_EN));
>> +
>> +	/* Todo: If BIOS has not configured PCBR
>> +	 *       then allocate in BIOS Reserved */
>> +
>> +	/* For now we assume BIOS is allocating and populating the PCBR  */
>> +	pcbr = I915_READ(VLV_PCBR);
>> +
>> +	DRM_DEBUG_DRIVER("PCBR offset : 0x%x\n", pcbr);
>> +
>> +	/* 3: Enable RC6 */
>> +	if ((intel_enable_rc6(dev) & INTEL_RC6_ENABLE) &&
>> +						(pcbr >> VLV_PCBR_ADDR_SHIFT))
>> +		rc6_mode = GEN6_RC_CTL_EI_MODE(1) | VLV_RC_CTL_CTX_RST_PARALLEL;
>> +
>> +	I915_WRITE(GEN6_RC_CONTROL, rc6_mode);
>> +
>> +	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
>> +}
>> +
> Can you send me a link to the doc for this off-list? I am having trouble
> tracking it down.
>
>>   static void valleyview_enable_rps(struct drm_device *dev)
>>   {
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -4550,7 +4639,9 @@ void intel_disable_gt_powersave(struct drm_device *dev)
>>   		cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
>>   		cancel_work_sync(&dev_priv->rps.work);
>>   		mutex_lock(&dev_priv->rps.hw_lock);
>> -		if (IS_VALLEYVIEW(dev))
>> +		if (IS_CHERRYVIEW(dev))
>> +			cherryview_disable_rps(dev);
>> +		else if (IS_VALLEYVIEW(dev))
>>   			valleyview_disable_rps(dev);
>>   		else
>>   			gen6_disable_rps(dev);
>> @@ -4568,7 +4659,9 @@ static void intel_gen6_powersave_work(struct work_struct *work)
>>   
>>   	mutex_lock(&dev_priv->rps.hw_lock);
>>   
>> -	if (IS_VALLEYVIEW(dev)) {
>> +	if (IS_CHERRYVIEW(dev)) {
>> +		cherryview_enable_rps(dev);
>> +	} else if (IS_VALLEYVIEW(dev)) {
>>   		valleyview_enable_rps(dev);
>>   	} else if (IS_BROADWELL(dev)) {
>>   		gen8_enable_rps(dev);
>> @@ -4590,6 +4683,8 @@ void intel_enable_gt_powersave(struct drm_device *dev)
>>   		ironlake_enable_rc6(dev);
>>   		intel_init_emon(dev);
>>   	} else if (IS_GEN6(dev) || IS_GEN7(dev)) {
>> +		if (IS_VALLEYVIEW(dev))
>> +			valleyview_setup_pctx(dev);
> Spurious hunk?
>
>>   		/*
>>   		 * PCU communication is slow and this doesn't need to be
>>   		 * done at any specific time, so do this out of our fast path
>> @@ -5175,6 +5270,7 @@ static void valleyview_init_clock_gating(struct drm_device *dev)
>>   		dev_priv->mem_freq = 1333;
>>   		break;
>>   	}
>> +
> Spurious hunk?
>
>>   	DRM_DEBUG_DRIVER("DDR speed: %d MHz", dev_priv->mem_freq);
>>   
>>   	dev_priv->vlv_cdclk_freq = valleyview_cur_cdclk(dev_priv);
> I think for enabling this patch is fine as is. It does need some of the
> fixes I mentioned about. So I'd suggest adding FIXMes with what I
> request, and calling this
> Acked-by: Ben Widawsky <ben@bwidawsk.net>
>
> To move it up to a reviewed by, I need to find the PM doc (which I
> probably have, just don't know it), and I'd like the setup function
> fixed.
>
>
>

Re: [PATCH 04/10] drm/i915/chv: Added CHV specific register read and write

[Deepak S]

Thanks Ben. Apologies for delayed response.

I am incorporating the review comment changes next set of patch review.


On Saturday 26 April 2014 03:24 AM, Ben Widawsky wrote:
> On Mon, Apr 21, 2014 at 01:34:08PM +0530, deepak.s@linux.intel.com wrote:
>> From: Deepak S <deepak.s@linux.intel.com>
>>
>> Support to individually control Media/Render well based on the register access.
>> Add CHV specific write function to habdle difference between registers
>> that are sadowed vs those that need forcewake even for writes.
>>
>> v2: Drop write FIFO for CHV and add comman well forcewake (Ville)
>>
>> v3: Fix for decrementing fw count in chv read/write. (Deepak)
>>
>> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
>> [vsyrjala: Move the register range macros into intel_uncore.c]
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> I left some comments on the first sending of this patch, and it's not
> clear if you ignored them intentionally or not. Inquiring minds would
> like to know.
>
> I'll repeat some of the ones I feel are more important below.
>
>> ---
>>   drivers/gpu/drm/i915/intel_uncore.c | 139 +++++++++++++++++++++++++++++++++---
>>   1 file changed, 131 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index 2a72bab..11741e4 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -495,6 +495,31 @@ void assert_force_wake_inactive(struct drm_i915_private *dev_priv)
>>   	((reg) >= 0x22000 && (reg) < 0x24000) ||\
>>   	((reg) >= 0x30000 && (reg) < 0x40000))
>>   
>> +#define FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg) \
>> +	(((reg) >= 0x2000 && (reg) < 0x4000) ||\
>> +	((reg) >= 0x5000 && (reg) < 0x8000) ||\
>> +	((reg) >= 0x8300 && (reg) < 0x8500) ||\
>> +	((reg) >= 0xB000 && (reg) < 0xC000) ||\
>> +	((reg) >= 0xE000 && (reg) < 0xE800))
>> +
>> +#define FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)\
>> +	(((reg) >= 0x8800 && (reg) < 0x8900) ||\
>> +	((reg) >= 0xD000 && (reg) < 0xD800) ||\
>> +	((reg) >= 0x12000 && (reg) < 0x14000) ||\
>> +	((reg) >= 0x1A000 && (reg) < 0x1C000) ||\
>> +	((reg) >= 0x1E800 && (reg) < 0x1EA00) ||\
>> +	((reg) >= 0x30000 && (reg) < 0x40000))
>> +
>> +#define FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)\
>> +	(((reg) >= 0x4000 && (reg) < 0x5000) ||\
>> +	((reg) >= 0x8000 && (reg) < 0x8300) ||\
>> +	((reg) >= 0x8500 && (reg) < 0x8600) ||\
>> +	((reg) >= 0x9000 && (reg) < 0xB000) ||\
>> +	((reg) >= 0xC000 && (reg) < 0xc800) ||\
>> +	((reg) >= 0xF000 && (reg) < 0x10000) ||\
>> +	((reg) >= 0x14000 && (reg) < 0x14400) ||\
>> +	((reg) >= 0x22000 && (reg) < 0x24000))
>> +
>>   static void
>>   ilk_dummy_write(struct drm_i915_private *dev_priv)
>>   {
>> @@ -588,7 +613,48 @@ vlv_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
>>   	REG_READ_FOOTER; \
>>   }
>>   
>> +#define __chv_read(x) \
>> +static u##x \
>> +chv_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
>> +	unsigned fwengine = 0; \
>> +	REG_READ_HEADER(x); \
>> +	if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) { \
>> +		fwengine = FORCEWAKE_RENDER; \
>> +	} \
>> +	else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) { \
>> +		fwengine = FORCEWAKE_MEDIA; \
>> +	} \
>> +	else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) { \
>> +		fwengine = FORCEWAKE_ALL; \
>> +	} \
> These don't following linux kernel coding style
>
>> +	if (FORCEWAKE_RENDER & fwengine) { \
>> +		if (dev_priv->uncore.fw_rendercount++ == 0) \
>> +			(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
>> +								fwengine); \
>> +	} \
>> +	if (FORCEWAKE_MEDIA & fwengine) { \
>> +		if (dev_priv->uncore.fw_mediacount++ == 0) \
>> +			(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
>> +								fwengine); \
>> +	} \
>> +	val = __raw_i915_read##x(dev_priv, reg); \
>> +	if (FORCEWAKE_RENDER & fwengine) { \
>> +		if (--dev_priv->uncore.fw_rendercount == 0) \
>> +			(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
>> +								fwengine); \
>> +	} \
>> +	if (FORCEWAKE_MEDIA & fwengine) { \
>> +		if (--dev_priv->uncore.fw_mediacount == 0) \
>> +			(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
>> +								fwengine); \
>> +	} \
>> +	REG_READ_FOOTER; \
>> +}
> It seems like it makes a lot more sense to pass register offset to
> force_wake_put() and let the logic occur there instead of making a big
> ugly macro. We can cheat and use the existing functions since fw_engine
> was defined as an int, and the register range fits within that.
>
>>   
>> +__chv_read(8)
>> +__chv_read(16)
>> +__chv_read(32)
>> +__chv_read(64)
>>   __vlv_read(8)
>>   __vlv_read(16)
>>   __vlv_read(32)
>> @@ -606,6 +672,7 @@ __gen4_read(16)
>>   __gen4_read(32)
>>   __gen4_read(64)
>>   
>> +#undef __chv_read
>>   #undef __vlv_read
>>   #undef __gen6_read
>>   #undef __gen5_read
>> @@ -710,6 +777,49 @@ gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace
>>   	REG_WRITE_FOOTER; \
>>   }
>>   
>> +#define __chv_write(x) \
>> +static void \
>> +chv_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
>> +	unsigned fwengine = 0; \
>> +	bool __needs_put = !is_gen8_shadowed(dev_priv, reg); \
>> +	REG_WRITE_HEADER; \
>> +	if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) { \
>> +		fwengine = FORCEWAKE_RENDER; \
>> +	} \
>> +	else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) { \
>> +		fwengine = FORCEWAKE_MEDIA; \
>> +	} \
>> +	else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) { \
>> +		fwengine = FORCEWAKE_ALL; \
>> +	} \
> coding convention again
>
>> +	if (__needs_put && (FORCEWAKE_RENDER & fwengine)) { \
>> +			if (dev_priv->uncore.fw_rendercount++ == 0) \
>> +				(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
>> +									fwengine); \
>> +	} \
>> +	if (__needs_put && (FORCEWAKE_MEDIA & fwengine)) { \
>> +		if (dev_priv->uncore.fw_mediacount++ == 0) \
>> +			(dev_priv)->uncore.funcs.force_wake_get(dev_priv, \
>> +								fwengine); \
>> +	} \
>> +	__raw_i915_write##x(dev_priv, reg, val); \
>> +	if (__needs_put && (FORCEWAKE_RENDER & fwengine)) { \
>> +			if (--dev_priv->uncore.fw_rendercount == 0) \
>> +				(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
>> +									fwengine); \
>> +	} \
>> +	if (__needs_put && (FORCEWAKE_MEDIA & fwengine)) { \
>> +		if (--dev_priv->uncore.fw_mediacount == 0) \
>> +			(dev_priv)->uncore.funcs.force_wake_put(dev_priv, \
>> +								fwengine); \
>> +	} \
> Same comment above
>
>> +	REG_WRITE_FOOTER; \
>> +}
>> +
>> +__chv_write(8)
>> +__chv_write(16)
>> +__chv_write(32)
>> +__chv_write(64)
>>   __gen8_write(8)
>>   __gen8_write(16)
>>   __gen8_write(32)
>> @@ -731,6 +841,7 @@ __gen4_write(16)
>>   __gen4_write(32)
>>   __gen4_write(64)
>>   
>> +#undef __chv_write
>>   #undef __gen8_write
>>   #undef __hsw_write
>>   #undef __gen6_write
>> @@ -794,14 +905,26 @@ void intel_uncore_init(struct drm_device *dev)
>>   
>>   	switch (INTEL_INFO(dev)->gen) {
>>   	default:
>> -		dev_priv->uncore.funcs.mmio_writeb  = gen8_write8;
>> -		dev_priv->uncore.funcs.mmio_writew  = gen8_write16;
>> -		dev_priv->uncore.funcs.mmio_writel  = gen8_write32;
>> -		dev_priv->uncore.funcs.mmio_writeq  = gen8_write64;
>> -		dev_priv->uncore.funcs.mmio_readb  = gen6_read8;
>> -		dev_priv->uncore.funcs.mmio_readw  = gen6_read16;
>> -		dev_priv->uncore.funcs.mmio_readl  = gen6_read32;
>> -		dev_priv->uncore.funcs.mmio_readq  = gen6_read64;
>> +		if (IS_CHERRYVIEW(dev)) {
>> +			dev_priv->uncore.funcs.mmio_writeb  = chv_write8;
>> +			dev_priv->uncore.funcs.mmio_writew  = chv_write16;
>> +			dev_priv->uncore.funcs.mmio_writel  = chv_write32;
>> +			dev_priv->uncore.funcs.mmio_writeq  = chv_write64;
>> +			dev_priv->uncore.funcs.mmio_readb  = chv_read8;
>> +			dev_priv->uncore.funcs.mmio_readw  = chv_read16;
>> +			dev_priv->uncore.funcs.mmio_readl  = chv_read32;
>> +			dev_priv->uncore.funcs.mmio_readq  = chv_read64;
>> +
>> +		} else {
>> +			dev_priv->uncore.funcs.mmio_writeb  = gen8_write8;
>> +			dev_priv->uncore.funcs.mmio_writew  = gen8_write16;
>> +			dev_priv->uncore.funcs.mmio_writel  = gen8_write32;
>> +			dev_priv->uncore.funcs.mmio_writeq  = gen8_write64;
>> +			dev_priv->uncore.funcs.mmio_readb  = gen6_read8;
>> +			dev_priv->uncore.funcs.mmio_readw  = gen6_read16;
>> +			dev_priv->uncore.funcs.mmio_readl  = gen6_read32;
>> +			dev_priv->uncore.funcs.mmio_readq  = gen6_read64;
>> +		}
>>   		break;
>>   	case 7:
>>   	case 6:
> I think if you make CHV have it's own forcewake get/put, then you can
> just use the the same MMIO functions.
>
> Like the previous patch, I am having trouble finding where the register
> ranges exist. Assuming those are correct (and I'm guessing Ville checked
> thoroughly), the patch looks correct.
>
> So I'd prefer to at least explore my suggestions, but it's
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
>

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