[PATCH v4 1/4] drm/i915/bdw+: Move misc display IRQ handling to it own function

[PATCH v4 1/4] drm/i915/bdw+: Move misc display IRQ handling to it own function

[José Roberto de Souza]

Just moving it to reduce the tabs and avoid break code lines.
No behavior changes intended here.

v2:
- Reading misc display IRQ outside of gen8_de_misc_irq_handler() as
other irq handlers (Dhinakaran)

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 45 ++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b92cfd69134b..a1299f10ed49 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2768,6 +2768,28 @@ static u32 gen8_de_port_aux_mask(struct drm_i915_private *dev_priv)
 	return mask;
 }
 
+static void
+gen8_de_misc_irq_handler(struct drm_i915_private *dev_priv, u32 iir)
+{
+	bool found = false;
+
+	if (iir & GEN8_DE_MISC_GSE) {
+		intel_opregion_asle_intr(dev_priv);
+		found = true;
+	}
+
+	if (iir & GEN8_DE_EDP_PSR) {
+		u32 psr_iir = I915_READ(EDP_PSR_IIR);
+
+		intel_psr_irq_handler(dev_priv, psr_iir);
+		I915_WRITE(EDP_PSR_IIR, psr_iir);
+		found = true;
+	}
+
+	if (!found)
+		DRM_ERROR("Unexpected DE Misc interrupt\n");
+}
+
 static irqreturn_t
 gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
 {
@@ -2778,29 +2800,12 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
 	if (master_ctl & GEN8_DE_MISC_IRQ) {
 		iir = I915_READ(GEN8_DE_MISC_IIR);
 		if (iir) {
-			bool found = false;
-
 			I915_WRITE(GEN8_DE_MISC_IIR, iir);
 			ret = IRQ_HANDLED;
-
-			if (iir & GEN8_DE_MISC_GSE) {
-				intel_opregion_asle_intr(dev_priv);
-				found = true;
-			}
-
-			if (iir & GEN8_DE_EDP_PSR) {
-				u32 psr_iir = I915_READ(EDP_PSR_IIR);
-
-				intel_psr_irq_handler(dev_priv, psr_iir);
-				I915_WRITE(EDP_PSR_IIR, psr_iir);
-				found = true;
-			}
-
-			if (!found)
-				DRM_ERROR("Unexpected DE Misc interrupt\n");
-		}
-		else
+			gen8_de_misc_irq_handler(dev_priv, iir);
+		} else {
 			DRM_ERROR("The master control interrupt lied (DE MISC)!\n");
+		}
 	}
 
 	if (INTEL_GEN(dev_priv) >= 11 && (master_ctl & GEN11_DE_HPD_IRQ)) {
-- 
2.21.0

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

[PATCH v4 2/4] drm/i915: Add _TRANS2()

[José Roberto de Souza]

A new macro that is going to be added in a further patch will need to
adjust the offset returned by _MMIO_TRANS2(), so here adding
_TRANS2() and moving most of the implementation of _MMIO_TRANS2() to
it and while at it taking the opportunity to rename pipe to trans.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b74824f0b5b1..9ef306b79e0d 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -250,9 +250,10 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
 #define _MMIO_PIPE2(pipe, reg)		_MMIO(INTEL_INFO(dev_priv)->pipe_offsets[pipe] - \
 					      INTEL_INFO(dev_priv)->pipe_offsets[PIPE_A] + (reg) + \
 					      DISPLAY_MMIO_BASE(dev_priv))
-#define _MMIO_TRANS2(pipe, reg)		_MMIO(INTEL_INFO(dev_priv)->trans_offsets[(pipe)] - \
-					      INTEL_INFO(dev_priv)->trans_offsets[TRANSCODER_A] + (reg) + \
-					      DISPLAY_MMIO_BASE(dev_priv))
+#define _TRANS2(trans, reg)		(INTEL_INFO(dev_priv)->trans_offsets[(trans)] - \
+					 INTEL_INFO(dev_priv)->trans_offsets[TRANSCODER_A] + (reg) + \
+					 DISPLAY_MMIO_BASE(dev_priv))
+#define _MMIO_TRANS2(trans, reg)	_MMIO(_TRANS2(trans, reg))
 #define _CURSOR2(pipe, reg)		_MMIO(INTEL_INFO(dev_priv)->cursor_offsets[(pipe)] - \
 					      INTEL_INFO(dev_priv)->cursor_offsets[PIPE_A] + (reg) + \
 					      DISPLAY_MMIO_BASE(dev_priv))
-- 
2.21.0

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

[PATCH v4 3/4] drm/i915: Make PSR registers relative to transcoders

[José Roberto de Souza]

PSR registers are a mess, some have the full address while others just
have the additional offset from psr_mmio_base.

psr_mmio_base is nothing more than TRANSCODER_EDP_OFFSET + 0x800 and
using it makes more difficult for people with an PSR register address
or PSR register name from from BSpec as i915 also don't match the
name.

Other reason to make relative to transcoder is that since BDW every
transcoder have PSR registers, so in theory it should be possible to
have PSR enabled in a non-eDP transcoder.

For Haswell PSR registers are relative to 0x64000, so
mmio_base_adjust and _TRANS2_PSR() was added so we can overcome
this limitation in a single place.
PSR2 registers and PSR_EVENT was added after Haswell so that is why
_TRANS2_PSR() is not used in some macros.

The only registers that can not be relative to transcoder are
PSR_IMR and PSR_IIR that are not relative to anything, so keeping it
hardcoded.

Also removing BDW_EDP_PSR_BASE from GVT because it is not used as it
is the only PSR register that GVT have.

v4:
- Moved definition of _TRANS2_PSR() and _MMIO_TRANS2_PSR() to the
beginning of i915_reg.h (Jani)

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Zhi Wang <zhi.a.wang@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/gvt/handlers.c |  1 -
 drivers/gpu/drm/i915/i915_drv.h     |  5 +--
 drivers/gpu/drm/i915/i915_reg.h     | 48 ++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_psr.c    | 11 +++++--
 4 files changed, 45 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c
index 18f01eeb2510..749e3e4204f2 100644
--- a/drivers/gpu/drm/i915/gvt/handlers.c
+++ b/drivers/gpu/drm/i915/gvt/handlers.c
@@ -2776,7 +2776,6 @@ static int init_broadwell_mmio_info(struct intel_gvt *gvt)
 	MMIO_D(CHICKEN_PIPESL_1(PIPE_C), D_BDW_PLUS);
 
 	MMIO_D(WM_MISC, D_BDW);
-	MMIO_D(_MMIO(BDW_EDP_PSR_BASE), D_BDW);
 
 	MMIO_D(_MMIO(0x6671c), D_BDW_PLUS);
 	MMIO_D(_MMIO(0x66c00), D_BDW_PLUS);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 066fd2a12851..58748f23511f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -494,6 +494,8 @@ struct i915_drrs {
 };
 
 struct i915_psr {
+	/* different than zero only on HSW see _TRANS2_PSR() for more info */
+	u32 mmio_base_adjust;
 	struct mutex lock;
 
 #define I915_PSR_DEBUG_MODE_MASK	0x0f
@@ -508,6 +510,7 @@ struct i915_psr {
 	bool enabled;
 	struct intel_dp *dp;
 	enum pipe pipe;
+	enum transcoder transcoder;
 	bool active;
 	struct work_struct work;
 	unsigned busy_frontbuffer_bits;
@@ -1534,8 +1537,6 @@ struct drm_i915_private {
 	/* MMIO base address for MIPI regs */
 	u32 mipi_mmio_base;
 
-	u32 psr_mmio_base;
-
 	u32 pps_mmio_base;
 
 	wait_queue_head_t gmbus_wait_queue;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9ef306b79e0d..987006946802 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -258,6 +258,10 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
 					      INTEL_INFO(dev_priv)->cursor_offsets[PIPE_A] + (reg) + \
 					      DISPLAY_MMIO_BASE(dev_priv))
 
+/* PSR registers on HSW is not relative to eDP transcoder */
+#define _TRANS2_PSR(reg)	(_TRANS2(dev_priv->psr.transcoder, (reg)) - dev_priv->psr.mmio_base_adjust)
+#define _MMIO_TRANS2_PSR(reg)	_MMIO(_TRANS2_PSR(reg))
+
 #define __MASKED_FIELD(mask, value) ((mask) << 16 | (value))
 #define _MASKED_FIELD(mask, value) ({					   \
 	if (__builtin_constant_p(mask))					   \
@@ -4213,9 +4217,11 @@ enum {
 #define PIPE_MULT(trans)	_MMIO_TRANS2(trans, _PIPE_MULT_A)
 
 /* HSW+ eDP PSR registers */
-#define HSW_EDP_PSR_BASE	0x64800
-#define BDW_EDP_PSR_BASE	0x6f800
-#define EDP_PSR_CTL				_MMIO(dev_priv->psr_mmio_base + 0)
+#define HSW_EDP_PSR_BASE	0x64000
+
+#define _SRD_CTL_A				0x60800
+#define _SRD_CTL_EDP				0x6F800
+#define EDP_PSR_CTL				_MMIO_TRANS2_PSR(_SRD_CTL_A)
 #define   EDP_PSR_ENABLE			(1 << 31)
 #define   BDW_PSR_SINGLE_FRAME			(1 << 30)
 #define   EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK	(1 << 29) /* SW can't modify */
@@ -4252,16 +4258,22 @@ enum {
 #define   EDP_PSR_TRANSCODER_A_SHIFT		8
 #define   EDP_PSR_TRANSCODER_EDP_SHIFT		0
 
-#define EDP_PSR_AUX_CTL				_MMIO(dev_priv->psr_mmio_base + 0x10)
+#define _SRD_AUX_CTL_A				0x60810
+#define _SRD_AUX_CTL_EDP			0x6F810
+#define EDP_PSR_AUX_CTL				_MMIO_TRANS2_PSR(_SRD_AUX_CTL_A)
 #define   EDP_PSR_AUX_CTL_TIME_OUT_MASK		(3 << 26)
 #define   EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK	(0x1f << 20)
 #define   EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK	(0xf << 16)
 #define   EDP_PSR_AUX_CTL_ERROR_INTERRUPT	(1 << 11)
 #define   EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK	(0x7ff)
 
-#define EDP_PSR_AUX_DATA(i)			_MMIO(dev_priv->psr_mmio_base + 0x14 + (i) * 4) /* 5 registers */
+#define _SRD_AUX_DATA_A				0x60814
+#define _SRD_AUX_DATA_EDP			0x6F814
+#define EDP_PSR_AUX_DATA(i)			_MMIO(_TRANS2_PSR(_SRD_AUX_DATA_A) + (i) + 4) /* 5 registers */
 
-#define EDP_PSR_STATUS				_MMIO(dev_priv->psr_mmio_base + 0x40)
+#define _SRD_STATUS_A				0x60840
+#define _SRD_STATUS_EDP				0x6F840
+#define EDP_PSR_STATUS				_MMIO_TRANS2_PSR(_SRD_STATUS_A)
 #define   EDP_PSR_STATUS_STATE_MASK		(7 << 29)
 #define   EDP_PSR_STATUS_STATE_SHIFT		29
 #define   EDP_PSR_STATUS_STATE_IDLE		(0 << 29)
@@ -4286,10 +4298,15 @@ enum {
 #define   EDP_PSR_STATUS_SENDING_TP1		(1 << 4)
 #define   EDP_PSR_STATUS_IDLE_MASK		0xf
 
-#define EDP_PSR_PERF_CNT		_MMIO(dev_priv->psr_mmio_base + 0x44)
+#define _SRD_PERF_CNT_A			0x60844
+#define _SRD_PERF_CNT_EDP		0x6F844
+#define EDP_PSR_PERF_CNT		_MMIO_TRANS2_PSR(_SRD_PERF_CNT_A)
 #define   EDP_PSR_PERF_CNT_MASK		0xffffff
 
-#define EDP_PSR_DEBUG				_MMIO(dev_priv->psr_mmio_base + 0x60) /* PSR_MASK on SKL+ */
+/* PSR_MASK on SKL+ */
+#define _SRD_DEBUG_A				0x60860
+#define _SRD_DEBUG_EDP				0x6F860
+#define EDP_PSR_DEBUG				_MMIO_TRANS2_PSR(_SRD_DEBUG_A)
 #define   EDP_PSR_DEBUG_MASK_MAX_SLEEP         (1 << 28)
 #define   EDP_PSR_DEBUG_MASK_LPSP              (1 << 27)
 #define   EDP_PSR_DEBUG_MASK_MEMUP             (1 << 26)
@@ -4297,7 +4314,9 @@ enum {
 #define   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE    (1 << 16) /* Reserved in ICL+ */
 #define   EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15) /* SKL+ */
 
-#define EDP_PSR2_CTL			_MMIO(0x6f900)
+#define _PSR2_CTL_A			0x60900
+#define _PSR2_CTL_EDP			0x6F900
+#define EDP_PSR2_CTL			_MMIO_TRANS2(dev_priv->psr.transcoder, _PSR2_CTL_A)
 #define   EDP_PSR2_ENABLE		(1 << 31)
 #define   EDP_SU_TRACK_ENABLE		(1 << 30)
 #define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and CNL+ */
@@ -4338,14 +4357,15 @@ enum {
 #define  PSR_EVENT_LPSP_MODE_EXIT		(1 << 1)
 #define  PSR_EVENT_PSR_DISABLE			(1 << 0)
 
-#define EDP_PSR2_STATUS			_MMIO(0x6f940)
+#define _PSR2_STATUS_A			0x60940
+#define _PSR2_STATUS_EDP		0x6F940
+#define EDP_PSR2_STATUS			_MMIO_TRANS2(dev_priv->psr.transcoder, _PSR2_STATUS_A)
 #define EDP_PSR2_STATUS_STATE_MASK     (0xf << 28)
 #define EDP_PSR2_STATUS_STATE_SHIFT    28
 
-#define _PSR2_SU_STATUS_0		0x6F914
-#define _PSR2_SU_STATUS_1		0x6F918
-#define _PSR2_SU_STATUS_2		0x6F91C
-#define _PSR2_SU_STATUS(index)		_MMIO(_PICK_EVEN((index), _PSR2_SU_STATUS_0, _PSR2_SU_STATUS_1))
+#define _PSR2_SU_STATUS_A		0x60914
+#define _PSR2_SU_STATUS_EDP		0x6F914
+#define _PSR2_SU_STATUS(index)		_MMIO(_TRANS2(dev_priv->psr.transcoder, _PSR2_SU_STATUS_A) + (index) * 4)
 #define PSR2_SU_STATUS(frame)		(_PSR2_SU_STATUS((frame) / 3))
 #define PSR2_SU_STATUS_SHIFT(frame)	(((frame) % 3) * 10)
 #define PSR2_SU_STATUS_MASK(frame)	(0x3ff << PSR2_SU_STATUS_SHIFT(frame))
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 963663ba0edf..da351c8f86d7 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -742,6 +742,14 @@ static void intel_psr_enable_locked(struct drm_i915_private *dev_priv,
 	dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state);
 	dev_priv->psr.busy_frontbuffer_bits = 0;
 	dev_priv->psr.pipe = to_intel_crtc(crtc_state->base.crtc)->pipe;
+	dev_priv->psr.transcoder = crtc_state->cpu_transcoder;
+
+	if (IS_HASWELL(dev_priv)) {
+		u32 trans_offset = INTEL_INFO(dev_priv)->trans_offsets[dev_priv->psr.transcoder];
+
+		WARN_ON(trans_offset < HSW_EDP_PSR_BASE);
+		dev_priv->psr.mmio_base_adjust = trans_offset - HSW_EDP_PSR_BASE;
+	}
 
 	DRM_DEBUG_KMS("Enabling PSR%s\n",
 		      dev_priv->psr.psr2_enabled ? "2" : "1");
@@ -1206,9 +1214,6 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
 	if (!HAS_PSR(dev_priv))
 		return;
 
-	dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ?
-		HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE;
-
 	if (!dev_priv->psr.sink_support)
 		return;
 
-- 
2.21.0

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

[PATCH v4 4/4] drm/i915: Add transcoder parameter to PSR registers macros

[José Roberto de Souza]

Lets make PSR register macros explicit about what transcoder is used
to calculate the register offset.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 18 ++++++----
 drivers/gpu/drm/i915/i915_reg.h     | 24 +++++++-------
 drivers/gpu/drm/i915/intel_psr.c    | 51 +++++++++++++++++------------
 3 files changed, 53 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5823ffb17821..2a0f5871e9a8 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2470,7 +2470,7 @@ psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m)
 			"BUF_ON",
 			"TG_ON"
 		};
-		val = I915_READ(EDP_PSR2_STATUS);
+		val = I915_READ(EDP_PSR2_STATUS(dev_priv->psr.transcoder));
 		status_val = (val & EDP_PSR2_STATUS_STATE_MASK) >>
 			      EDP_PSR2_STATUS_STATE_SHIFT;
 		if (status_val < ARRAY_SIZE(live_status))
@@ -2486,7 +2486,7 @@ psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m)
 			"SRDOFFACK",
 			"SRDENT_ON",
 		};
-		val = I915_READ(EDP_PSR_STATUS);
+		val = I915_READ(EDP_PSR_STATUS(dev_priv->psr.transcoder));
 		status_val = (val & EDP_PSR_STATUS_STATE_MASK) >>
 			      EDP_PSR_STATUS_STATE_SHIFT;
 		if (status_val < ARRAY_SIZE(live_status))
@@ -2529,10 +2529,10 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 		goto unlock;
 
 	if (psr->psr2_enabled) {
-		val = I915_READ(EDP_PSR2_CTL);
+		val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder));
 		enabled = val & EDP_PSR2_ENABLE;
 	} else {
-		val = I915_READ(EDP_PSR_CTL);
+		val = I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder));
 		enabled = val & EDP_PSR_ENABLE;
 	}
 	seq_printf(m, "Source PSR ctl: %s [0x%08x]\n",
@@ -2545,7 +2545,8 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 	 * SKL+ Perf counter is reset to 0 everytime DC state is entered
 	 */
 	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
-		val = I915_READ(EDP_PSR_PERF_CNT) & EDP_PSR_PERF_CNT_MASK;
+		val = I915_READ(EDP_PSR_PERF_CNT(dev_priv->psr.transcoder));
+		val &= EDP_PSR_PERF_CNT_MASK;
 		seq_printf(m, "Performance counter: %u\n", val);
 	}
 
@@ -2563,8 +2564,11 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 		 * Reading all 3 registers before hand to minimize crossing a
 		 * frame boundary between register reads
 		 */
-		for (frame = 0; frame < PSR2_SU_STATUS_FRAMES; frame += 3)
-			su_frames_val[frame / 3] = I915_READ(PSR2_SU_STATUS(frame));
+		for (frame = 0; frame < PSR2_SU_STATUS_FRAMES; frame += 3) {
+			val = I915_READ(PSR2_SU_STATUS(dev_priv->psr.transcoder,
+						       frame));
+			su_frames_val[frame / 3] = val;
+		}
 
 		seq_puts(m, "Frame:\tPSR2 SU blocks:\n");
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 987006946802..e52914987ee2 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -259,8 +259,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
 					      DISPLAY_MMIO_BASE(dev_priv))
 
 /* PSR registers on HSW is not relative to eDP transcoder */
-#define _TRANS2_PSR(reg)	(_TRANS2(dev_priv->psr.transcoder, (reg)) - dev_priv->psr.mmio_base_adjust)
-#define _MMIO_TRANS2_PSR(reg)	_MMIO(_TRANS2_PSR(reg))
+#define _TRANS2_PSR(trans, reg)		(_TRANS2(trans, (reg)) - dev_priv->psr.mmio_base_adjust)
+#define _MMIO_TRANS2_PSR(trans, reg)	_MMIO(_TRANS2_PSR(trans, reg))
 
 #define __MASKED_FIELD(mask, value) ((mask) << 16 | (value))
 #define _MASKED_FIELD(mask, value) ({					   \
@@ -4221,7 +4221,7 @@ enum {
 
 #define _SRD_CTL_A				0x60800
 #define _SRD_CTL_EDP				0x6F800
-#define EDP_PSR_CTL				_MMIO_TRANS2_PSR(_SRD_CTL_A)
+#define EDP_PSR_CTL(trans)			_MMIO_TRANS2_PSR(trans, _SRD_CTL_A)
 #define   EDP_PSR_ENABLE			(1 << 31)
 #define   BDW_PSR_SINGLE_FRAME			(1 << 30)
 #define   EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK	(1 << 29) /* SW can't modify */
@@ -4260,7 +4260,7 @@ enum {
 
 #define _SRD_AUX_CTL_A				0x60810
 #define _SRD_AUX_CTL_EDP			0x6F810
-#define EDP_PSR_AUX_CTL				_MMIO_TRANS2_PSR(_SRD_AUX_CTL_A)
+#define EDP_PSR_AUX_CTL(trans)			_MMIO_TRANS2_PSR(trans, _SRD_AUX_CTL_A)
 #define   EDP_PSR_AUX_CTL_TIME_OUT_MASK		(3 << 26)
 #define   EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK	(0x1f << 20)
 #define   EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK	(0xf << 16)
@@ -4269,11 +4269,11 @@ enum {
 
 #define _SRD_AUX_DATA_A				0x60814
 #define _SRD_AUX_DATA_EDP			0x6F814
-#define EDP_PSR_AUX_DATA(i)			_MMIO(_TRANS2_PSR(_SRD_AUX_DATA_A) + (i) + 4) /* 5 registers */
+#define EDP_PSR_AUX_DATA(trans, i)		_MMIO(_TRANS2_PSR(trans, _SRD_AUX_DATA_A) + (i) + 4) /* 5 registers */
 
 #define _SRD_STATUS_A				0x60840
 #define _SRD_STATUS_EDP				0x6F840
-#define EDP_PSR_STATUS				_MMIO_TRANS2_PSR(_SRD_STATUS_A)
+#define EDP_PSR_STATUS(trans)			_MMIO_TRANS2_PSR(trans, _SRD_STATUS_A)
 #define   EDP_PSR_STATUS_STATE_MASK		(7 << 29)
 #define   EDP_PSR_STATUS_STATE_SHIFT		29
 #define   EDP_PSR_STATUS_STATE_IDLE		(0 << 29)
@@ -4300,13 +4300,13 @@ enum {
 
 #define _SRD_PERF_CNT_A			0x60844
 #define _SRD_PERF_CNT_EDP		0x6F844
-#define EDP_PSR_PERF_CNT		_MMIO_TRANS2_PSR(_SRD_PERF_CNT_A)
+#define EDP_PSR_PERF_CNT(trans)		_MMIO_TRANS2_PSR(trans, _SRD_PERF_CNT_A)
 #define   EDP_PSR_PERF_CNT_MASK		0xffffff
 
 /* PSR_MASK on SKL+ */
 #define _SRD_DEBUG_A				0x60860
 #define _SRD_DEBUG_EDP				0x6F860
-#define EDP_PSR_DEBUG				_MMIO_TRANS2_PSR(_SRD_DEBUG_A)
+#define EDP_PSR_DEBUG(trans)			_MMIO_TRANS2_PSR(trans, _SRD_DEBUG_A)
 #define   EDP_PSR_DEBUG_MASK_MAX_SLEEP         (1 << 28)
 #define   EDP_PSR_DEBUG_MASK_LPSP              (1 << 27)
 #define   EDP_PSR_DEBUG_MASK_MEMUP             (1 << 26)
@@ -4316,7 +4316,7 @@ enum {
 
 #define _PSR2_CTL_A			0x60900
 #define _PSR2_CTL_EDP			0x6F900
-#define EDP_PSR2_CTL			_MMIO_TRANS2(dev_priv->psr.transcoder, _PSR2_CTL_A)
+#define EDP_PSR2_CTL(trans)		_MMIO_TRANS2(trans, _PSR2_CTL_A)
 #define   EDP_PSR2_ENABLE		(1 << 31)
 #define   EDP_SU_TRACK_ENABLE		(1 << 30)
 #define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and CNL+ */
@@ -4359,14 +4359,14 @@ enum {
 
 #define _PSR2_STATUS_A			0x60940
 #define _PSR2_STATUS_EDP		0x6F940
-#define EDP_PSR2_STATUS			_MMIO_TRANS2(dev_priv->psr.transcoder, _PSR2_STATUS_A)
+#define EDP_PSR2_STATUS(trans)		_MMIO_TRANS2(trans, _PSR2_STATUS_A)
 #define EDP_PSR2_STATUS_STATE_MASK     (0xf << 28)
 #define EDP_PSR2_STATUS_STATE_SHIFT    28
 
 #define _PSR2_SU_STATUS_A		0x60914
 #define _PSR2_SU_STATUS_EDP		0x6F914
-#define _PSR2_SU_STATUS(index)		_MMIO(_TRANS2(dev_priv->psr.transcoder, _PSR2_SU_STATUS_A) + (index) * 4)
-#define PSR2_SU_STATUS(frame)		(_PSR2_SU_STATUS((frame) / 3))
+#define _PSR2_SU_STATUS(trans, index)	_MMIO(_TRANS2(trans, _PSR2_SU_STATUS_A) + (index) * 4)
+#define PSR2_SU_STATUS(trans, frame)		(_PSR2_SU_STATUS(trans, (frame) / 3))
 #define PSR2_SU_STATUS_SHIFT(frame)	(((frame) % 3) * 10)
 #define PSR2_SU_STATUS_MASK(frame)	(0x3ff << PSR2_SU_STATUS_SHIFT(frame))
 #define PSR2_SU_STATUS_FRAMES		8
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index da351c8f86d7..108161d950d3 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -399,7 +399,7 @@ static void hsw_psr_setup_aux(struct intel_dp *intel_dp)
 
 	BUILD_BUG_ON(sizeof(aux_msg) > 20);
 	for (i = 0; i < sizeof(aux_msg); i += 4)
-		I915_WRITE(EDP_PSR_AUX_DATA(i >> 2),
+		I915_WRITE(EDP_PSR_AUX_DATA(dev_priv->psr.transcoder, i >> 2),
 			   intel_dp_pack_aux(&aux_msg[i], sizeof(aux_msg) - i));
 
 	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
@@ -410,7 +410,7 @@ static void hsw_psr_setup_aux(struct intel_dp *intel_dp)
 
 	/* Select only valid bits for SRD_AUX_CTL */
 	aux_ctl &= psr_aux_mask;
-	I915_WRITE(EDP_PSR_AUX_CTL, aux_ctl);
+	I915_WRITE(EDP_PSR_AUX_CTL(dev_priv->psr.transcoder), aux_ctl);
 }
 
 static void intel_psr_enable_sink(struct intel_dp *intel_dp)
@@ -500,8 +500,9 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
 	if (INTEL_GEN(dev_priv) >= 8)
 		val |= EDP_PSR_CRC_ENABLE;
 
-	val |= I915_READ(EDP_PSR_CTL) & EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK;
-	I915_WRITE(EDP_PSR_CTL, val);
+	val |= (I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder)) &
+		EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK);
+	I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), val);
 }
 
 static void hsw_activate_psr2(struct intel_dp *intel_dp)
@@ -537,9 +538,9 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
 	 * PSR2 HW is incorrectly using EDP_PSR_TP1_TP3_SEL and BSpec is
 	 * recommending keep this bit unset while PSR2 is enabled.
 	 */
-	I915_WRITE(EDP_PSR_CTL, 0);
+	I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), 0);
 
-	I915_WRITE(EDP_PSR2_CTL, val);
+	I915_WRITE(EDP_PSR2_CTL(dev_priv->psr.transcoder), val);
 }
 
 static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
@@ -658,8 +659,8 @@ static void intel_psr_activate(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
 
 	if (INTEL_GEN(dev_priv) >= 9)
-		WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
-	WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
+		WARN_ON(I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder)) & EDP_PSR2_ENABLE);
+	WARN_ON(I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder)) & EDP_PSR_ENABLE);
 	WARN_ON(dev_priv->psr.active);
 	lockdep_assert_held(&dev_priv->psr.lock);
 
@@ -729,7 +730,7 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp,
 	if (INTEL_GEN(dev_priv) < 11)
 		mask |= EDP_PSR_DEBUG_MASK_DISP_REG_WRITE;
 
-	I915_WRITE(EDP_PSR_DEBUG, mask);
+	I915_WRITE(EDP_PSR_DEBUG(dev_priv->psr.transcoder), mask);
 }
 
 static void intel_psr_enable_locked(struct drm_i915_private *dev_priv,
@@ -799,20 +800,27 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
 	u32 val;
 
 	if (!dev_priv->psr.active) {
-		if (INTEL_GEN(dev_priv) >= 9)
-			WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
-		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
+		if (INTEL_GEN(dev_priv) >= 9) {
+			val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder));
+			WARN_ON(val & EDP_PSR2_ENABLE);
+		}
+
+		val = I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder));
+		WARN_ON(val & EDP_PSR_ENABLE);
+
 		return;
 	}
 
 	if (dev_priv->psr.psr2_enabled) {
-		val = I915_READ(EDP_PSR2_CTL);
+		val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder));
 		WARN_ON(!(val & EDP_PSR2_ENABLE));
-		I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
+		val &= ~EDP_PSR2_ENABLE;
+		I915_WRITE(EDP_PSR2_CTL(dev_priv->psr.transcoder), val);
 	} else {
-		val = I915_READ(EDP_PSR_CTL);
+		val = I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder));
 		WARN_ON(!(val & EDP_PSR_ENABLE));
-		I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
+		val &= ~EDP_PSR_ENABLE;
+		I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), val);
 	}
 	dev_priv->psr.active = false;
 }
@@ -834,10 +842,10 @@ static void intel_psr_disable_locked(struct intel_dp *intel_dp)
 	intel_psr_exit(dev_priv);
 
 	if (dev_priv->psr.psr2_enabled) {
-		psr_status = EDP_PSR2_STATUS;
+		psr_status = EDP_PSR2_STATUS(dev_priv->psr.transcoder);
 		psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
 	} else {
-		psr_status = EDP_PSR_STATUS;
+		psr_status = EDP_PSR_STATUS(dev_priv->psr.transcoder);
 		psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
 	}
 
@@ -964,7 +972,8 @@ int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state,
 	 * defensive enough to cover everything.
 	 */
 
-	return __intel_wait_for_register(&dev_priv->uncore, EDP_PSR_STATUS,
+	return __intel_wait_for_register(&dev_priv->uncore,
+					 EDP_PSR_STATUS(dev_priv->psr.transcoder),
 					 EDP_PSR_STATUS_STATE_MASK,
 					 EDP_PSR_STATUS_STATE_IDLE, 2, 50,
 					 out_value);
@@ -980,10 +989,10 @@ static bool __psr_wait_for_idle_locked(struct drm_i915_private *dev_priv)
 		return false;
 
 	if (dev_priv->psr.psr2_enabled) {
-		reg = EDP_PSR2_STATUS;
+		reg = EDP_PSR2_STATUS(dev_priv->psr.transcoder);
 		mask = EDP_PSR2_STATUS_STATE_MASK;
 	} else {
-		reg = EDP_PSR_STATUS;
+		reg = EDP_PSR_STATUS(dev_priv->psr.transcoder);
 		mask = EDP_PSR_STATUS_STATE_MASK;
 	}
 
-- 
2.21.0

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

✗ Fi.CI.CHECKPATCH: warning for series starting with [v4,1/4] drm/i915/bdw+: Move misc display IRQ handling to it own function

[Patchwork]

== Series Details ==

Series: series starting with [v4,1/4] drm/i915/bdw+: Move misc display IRQ handling to it own function
URL   : https://patchwork.freedesktop.org/series/59672/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
9a6f3b42a779 drm/i915/bdw+: Move misc display IRQ handling to it own function
7c059b623995 drm/i915: Add _TRANS2()
-:29: WARNING:LONG_LINE: line over 100 characters
#29: FILE: drivers/gpu/drm/i915/i915_reg.h:254:
+					 INTEL_INFO(dev_priv)->trans_offsets[TRANSCODER_A] + (reg) + \

total: 0 errors, 1 warnings, 0 checks, 13 lines checked
b92d121924eb drm/i915: Make PSR registers relative to transcoders
-:96: WARNING:LONG_LINE: line over 100 characters
#96: FILE: drivers/gpu/drm/i915/i915_reg.h:262:
+#define _TRANS2_PSR(reg)	(_TRANS2(dev_priv->psr.transcoder, (reg)) - dev_priv->psr.mmio_base_adjust)

-:134: WARNING:LONG_LINE_COMMENT: line over 100 characters
#134: FILE: drivers/gpu/drm/i915/i915_reg.h:4272:
+#define EDP_PSR_AUX_DATA(i)			_MMIO(_TRANS2_PSR(_SRD_AUX_DATA_A) + (i) + 4) /* 5 registers */

-:189: WARNING:LONG_LINE: line over 100 characters
#189: FILE: drivers/gpu/drm/i915/i915_reg.h:4368:
+#define _PSR2_SU_STATUS(index)		_MMIO(_TRANS2(dev_priv->psr.transcoder, _PSR2_SU_STATUS_A) + (index) * 4)

total: 0 errors, 3 warnings, 0 checks, 149 lines checked
f3e8eb0c781a drm/i915: Add transcoder parameter to PSR registers macros
-:113: WARNING:LONG_LINE: line over 100 characters
#113: FILE: drivers/gpu/drm/i915/i915_reg.h:4272:
+#define EDP_PSR_AUX_DATA(trans, i)		_MMIO(_TRANS2_PSR(trans, _SRD_AUX_DATA_A) + (i) + 4) /* 5 registers */

total: 0 errors, 1 warnings, 0 checks, 252 lines checked

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

✗ Fi.CI.SPARSE: warning for series starting with [v4,1/4] drm/i915/bdw+: Move misc display IRQ handling to it own function

[Patchwork]

== Series Details ==

Series: series starting with [v4,1/4] drm/i915/bdw+: Move misc display IRQ handling to it own function
URL   : https://patchwork.freedesktop.org/series/59672/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915/bdw+: Move misc display IRQ handling to it own function
Okay!

Commit: drm/i915: Add _TRANS2()
Okay!

Commit: drm/i915: Make PSR registers relative to transcoders
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3616:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3617:16: warning: expression using sizeof(void)

Commit: drm/i915: Add transcoder parameter to PSR registers macros
Okay!

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

✓ Fi.CI.BAT: success for series starting with [v4,1/4] drm/i915/bdw+: Move misc display IRQ handling to it own function

[Patchwork]

== Series Details ==

Series: series starting with [v4,1/4] drm/i915/bdw+: Move misc display IRQ handling to it own function
URL   : https://patchwork.freedesktop.org/series/59672/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5951 -> Patchwork_12825
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/59672/revisions/1/mbox/

Known issues
------------

  Here are the changes found in Patchwork_12825 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@semaphore:
    - fi-bdw-5557u:       NOTRUN -> SKIP [fdo#109271] +38

  * igt@gem_ctx_exec@basic:
    - fi-icl-u3:          NOTRUN -> INCOMPLETE [fdo#107713]

  * igt@kms_pipe_crc_basic@read-crc-pipe-a:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103191]

  
#### Possible fixes ####

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - fi-byt-clapper:     FAIL [fdo#103191] -> PASS

  
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271


Participating hosts (46 -> 42)
------------------------------

  Additional (2): fi-bdw-5557u fi-icl-u3 
  Missing    (6): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-icl-y fi-bdw-samus 


Build changes
-------------

    * Linux: CI_DRM_5951 -> Patchwork_12825

  CI_DRM_5951: 08cf1213114f21fc0b1a17eac081a2db8c03311f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4956: 1d921615b0b706f25c856aa0eb096f274380c199 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12825: f3e8eb0c781a6a88cd6f67a64f4b93e132cb37fe @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

f3e8eb0c781a drm/i915: Add transcoder parameter to PSR registers macros
b92d121924eb drm/i915: Make PSR registers relative to transcoders
7c059b623995 drm/i915: Add _TRANS2()
9a6f3b42a779 drm/i915/bdw+: Move misc display IRQ handling to it own function

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12825/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 1/4] drm/i915/bdw+: Move misc display IRQ handling to it own function

[Dhinakaran Pandiyan]

On Wed, 2019-04-17 at 15:37 -0700, José Roberto de Souza wrote:
> Just moving it to reduce the tabs and avoid break code lines.
> No behavior changes intended here.
> 
> v2:
> - Reading misc display IRQ outside of gen8_de_misc_irq_handler() as
> other irq handlers (Dhinakaran)
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 45 ++++++++++++++++++---------------
>  1 file changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b92cfd69134b..a1299f10ed49 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2768,6 +2768,28 @@ static u32 gen8_de_port_aux_mask(struct
> drm_i915_private *dev_priv)
>  	return mask;
>  }
>  
> +static void
> +gen8_de_misc_irq_handler(struct drm_i915_private *dev_priv, u32 iir)
> +{
> +	bool found = false;
> +
> +	if (iir & GEN8_DE_MISC_GSE) {
> +		intel_opregion_asle_intr(dev_priv);
> +		found = true;
> +	}
> +
> +	if (iir & GEN8_DE_EDP_PSR) {
> +		u32 psr_iir = I915_READ(EDP_PSR_IIR);
> +
> +		intel_psr_irq_handler(dev_priv, psr_iir);
> +		I915_WRITE(EDP_PSR_IIR, psr_iir);

Two things unrelated to your refactoring, 

1) we can ack the irq right after reading the iir bits. I had noticed this a
while back but never got to changing it.

2) we don't DRM_ERROR() for unexpected PSR interrupts like how other handlers
do. Not sure what's the point though, other than getting to know that the
hardware is broken. 

Ville, any idea why unexpected interrupts warrant DRM_ERROR().


This patch is 
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>


> +		found = true;
> +	}
> +
> +	if (!found)
> +		DRM_ERROR("Unexpected DE Misc interrupt\n");
> +}
> +
>  static irqreturn_t
>  gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
>  {
> @@ -2778,29 +2800,12 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv,
> u32 master_ctl)
>  	if (master_ctl & GEN8_DE_MISC_IRQ) {
>  		iir = I915_READ(GEN8_DE_MISC_IIR);
>  		if (iir) {
> -			bool found = false;
> -
>  			I915_WRITE(GEN8_DE_MISC_IIR, iir);
>  			ret = IRQ_HANDLED;
> -
> -			if (iir & GEN8_DE_MISC_GSE) {
> -				intel_opregion_asle_intr(dev_priv);
> -				found = true;
> -			}
> -
> -			if (iir & GEN8_DE_EDP_PSR) {
> -				u32 psr_iir = I915_READ(EDP_PSR_IIR);
> -
> -				intel_psr_irq_handler(dev_priv, psr_iir);
> -				I915_WRITE(EDP_PSR_IIR, psr_iir);
> -				found = true;
> -			}
> -
> -			if (!found)
> -				DRM_ERROR("Unexpected DE Misc interrupt\n");
> -		}
> -		else
> +			gen8_de_misc_irq_handler(dev_priv, iir);
> +		} else {
>  			DRM_ERROR("The master control interrupt lied (DE
> MISC)!\n");
> +		}
>  	}
>  
>  	if (INTEL_GEN(dev_priv) >= 11 && (master_ctl & GEN11_DE_HPD_IRQ)) {

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

Re: [PATCH v4 2/4] drm/i915: Add _TRANS2()

[Dhinakaran Pandiyan]

On Wed, 2019-04-17 at 15:37 -0700, José Roberto de Souza wrote:
> A new macro that is going to be added in a further patch will need to
> adjust the offset returned by _MMIO_TRANS2(), so here adding
> _TRANS2() and moving most of the implementation of _MMIO_TRANS2() to
> it and while at it taking the opportunity to rename pipe to trans.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b74824f0b5b1..9ef306b79e0d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -250,9 +250,10 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  #define _MMIO_PIPE2(pipe, reg)		_MMIO(INTEL_INFO(dev_priv)-
> > pipe_offsets[pipe] - \
> 
>  					      INTEL_INFO(dev_priv)-
> > pipe_offsets[PIPE_A] + (reg) + \
> 
>  					      DISPLAY_MMIO_BASE(dev_priv))
> -#define _MMIO_TRANS2(pipe, reg)		_MMIO(INTEL_INFO(dev_priv)-
> > trans_offsets[(pipe)] - \
> 
> -					      INTEL_INFO(dev_priv)-
> > trans_offsets[TRANSCODER_A] + (reg) + \
> 
> -					      DISPLAY_MMIO_BASE(dev_priv))
> +#define _TRANS2(trans, reg)		(INTEL_INFO(dev_priv)-

_TRANS() and _MMIO_TRANS() name the first argument "tran"

Can you please keep the same name "tran"?

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiya@intel.com>

> > trans_offsets[(trans)] - \
> 
> +					 INTEL_INFO(dev_priv)-
> > trans_offsets[TRANSCODER_A] + (reg) + \
> 
> +					 DISPLAY_MMIO_BASE(dev_priv))
> +#define _MMIO_TRANS2(trans, reg)	_MMIO(_TRANS2(trans, reg))
>  #define _CURSOR2(pipe, reg)		_MMIO(INTEL_INFO(dev_priv)-
> > cursor_offsets[(pipe)] - \
> 
>  					      INTEL_INFO(dev_priv)-
> > cursor_offsets[PIPE_A] + (reg) + \
> 
>  					      DISPLAY_MMIO_BASE(dev_priv))

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

✓ Fi.CI.IGT: success for series starting with [v4,1/4] drm/i915/bdw+: Move misc display IRQ handling to it own function

[Patchwork]

== Series Details ==

Series: series starting with [v4,1/4] drm/i915/bdw+: Move misc display IRQ handling to it own function
URL   : https://patchwork.freedesktop.org/series/59672/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5951_full -> Patchwork_12825_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Known issues
------------

  Here are the changes found in Patchwork_12825_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_isolation@rcs0-s3:
    - shard-apl:          PASS -> DMESG-WARN [fdo#108566] +4

  * igt@gem_eio@in-flight-suspend:
    - shard-apl:          PASS -> INCOMPLETE [fdo#103927] +1

  * igt@gem_exec_schedule@preempt-other-chain-blt:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] +35

  * igt@gem_exec_suspend@basic-s3:
    - shard-kbl:          PASS -> INCOMPLETE [fdo#103665]

  * igt@i915_pm_rc6_residency@rc6-accuracy:
    - shard-kbl:          PASS -> SKIP [fdo#109271]

  * igt@kms_atomic_transition@3x-modeset-transitions-nonblocking:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +1

  * igt@kms_atomic_transition@4x-modeset-transitions-nonblocking-fencing:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +2

  * igt@kms_atomic_transition@5x-modeset-transitions:
    - shard-skl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +8

  * igt@kms_cursor_crc@cursor-64x64-suspend:
    - shard-skl:          PASS -> INCOMPLETE [fdo#104108]

  * igt@kms_cursor_legacy@2x-long-flip-vs-cursor-atomic:
    - shard-glk:          PASS -> FAIL [fdo#104873]

  * igt@kms_dp_dsc@basic-dsc-enable-edp:
    - shard-iclb:         PASS -> SKIP [fdo#109349]

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-skl:          PASS -> FAIL [fdo#105363]

  * igt@kms_flip_tiling@flip-to-y-tiled:
    - shard-iclb:         PASS -> FAIL [fdo#107931] / [fdo#108134]

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-blt:
    - shard-iclb:         PASS -> FAIL [fdo#103167] +2

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-shrfb-msflip-blt:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] +13

  * igt@kms_frontbuffer_tracking@psr-suspend:
    - shard-skl:          PASS -> INCOMPLETE [fdo#104108] / [fdo#106978]

  * igt@kms_mmap_write_crc@main:
    - shard-iclb:         PASS -> INCOMPLETE [fdo#107713]

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
    - shard-kbl:          PASS -> DMESG-WARN [fdo#108566] +4

  * igt@kms_plane_alpha_blend@pipe-a-alpha-7efc:
    - shard-skl:          NOTRUN -> FAIL [fdo#108145] +2

  * igt@kms_plane_alpha_blend@pipe-b-alpha-transparant-fb:
    - shard-kbl:          NOTRUN -> FAIL [fdo#108145]

  * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
    - shard-skl:          PASS -> FAIL [fdo#108145] / [fdo#110403]

  * igt@kms_psr@psr2_primary_page_flip:
    - shard-iclb:         PASS -> SKIP [fdo#109441] +1

  * igt@kms_rotation_crc@multiplane-rotation-cropping-top:
    - shard-kbl:          PASS -> FAIL [fdo#109016]

  * igt@perf_pmu@semaphore-wait-vcs1:
    - shard-skl:          NOTRUN -> SKIP [fdo#109271] +99

  * igt@tools_test@tools_test:
    - shard-apl:          PASS -> SKIP [fdo#109271]

  
#### Possible fixes ####

  * igt@gem_workarounds@suspend-resume:
    - shard-apl:          DMESG-WARN [fdo#108566] -> PASS +7

  * igt@i915_pm_rc6_residency@rc6-accuracy:
    - shard-snb:          SKIP [fdo#109271] -> PASS

  * igt@i915_suspend@sysfs-reader:
    - shard-kbl:          DMESG-WARN [fdo#108566] -> PASS

  * igt@kms_flip@plain-flip-ts-check-interruptible:
    - shard-skl:          FAIL [fdo#100368] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-blt:
    - shard-iclb:         FAIL [fdo#103167] -> PASS +5

  * igt@kms_plane_lowres@pipe-a-tiling-y:
    - shard-iclb:         FAIL [fdo#103166] -> PASS

  * igt@kms_psr@no_drrs:
    - shard-iclb:         FAIL [fdo#108341] -> PASS

  * igt@kms_psr@psr2_sprite_blt:
    - shard-iclb:         SKIP [fdo#109441] -> PASS

  * igt@kms_rotation_crc@multiplane-rotation-cropping-bottom:
    - shard-kbl:          DMESG-FAIL [fdo#105763] -> PASS

  * igt@kms_setmode@basic:
    - shard-skl:          FAIL [fdo#99912] -> PASS

  
#### Warnings ####

  * igt@i915_pm_rpm@dpms-mode-unset-non-lpsp:
    - shard-skl:          SKIP [fdo#109271] -> INCOMPLETE [fdo#107807]

  * igt@kms_frontbuffer_tracking@psr-2p-primscrn-spr-indfb-draw-pwrite:
    - shard-apl:          SKIP [fdo#109271] -> INCOMPLETE [fdo#103927]

  
  [fdo#100368]: https://bugs.freedesktop.org/show_bug.cgi?id=100368
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#104873]: https://bugs.freedesktop.org/show_bug.cgi?id=104873
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
  [fdo#106978]: https://bugs.freedesktop.org/show_bug.cgi?id=106978
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#107931]: https://bugs.freedesktop.org/show_bug.cgi?id=107931
  [fdo#108134]: https://bugs.freedesktop.org/show_bug.cgi?id=108134
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108341]: https://bugs.freedesktop.org/show_bug.cgi?id=108341
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#109016]: https://bugs.freedesktop.org/show_bug.cgi?id=109016
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (10 -> 9)
------------------------------

  Missing    (1): shard-hsw 


Build changes
-------------

    * Linux: CI_DRM_5951 -> Patchwork_12825

  CI_DRM_5951: 08cf1213114f21fc0b1a17eac081a2db8c03311f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4956: 1d921615b0b706f25c856aa0eb096f274380c199 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12825: f3e8eb0c781a6a88cd6f67a64f4b93e132cb37fe @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12825/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 1/4] drm/i915/bdw+: Move misc display IRQ handling to it own function

[Ville Syrjälä]

On Wed, Apr 17, 2019 at 08:50:58PM -0700, Dhinakaran Pandiyan wrote:
> On Wed, 2019-04-17 at 15:37 -0700, José Roberto de Souza wrote:
> > Just moving it to reduce the tabs and avoid break code lines.
> > No behavior changes intended here.
> > 
> > v2:
> > - Reading misc display IRQ outside of gen8_de_misc_irq_handler() as
> > other irq handlers (Dhinakaran)
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 45 ++++++++++++++++++---------------
> >  1 file changed, 25 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index b92cfd69134b..a1299f10ed49 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2768,6 +2768,28 @@ static u32 gen8_de_port_aux_mask(struct
> > drm_i915_private *dev_priv)
> >  	return mask;
> >  }
> >  
> > +static void
> > +gen8_de_misc_irq_handler(struct drm_i915_private *dev_priv, u32 iir)
> > +{
> > +	bool found = false;
> > +
> > +	if (iir & GEN8_DE_MISC_GSE) {
> > +		intel_opregion_asle_intr(dev_priv);
> > +		found = true;
> > +	}
> > +
> > +	if (iir & GEN8_DE_EDP_PSR) {
> > +		u32 psr_iir = I915_READ(EDP_PSR_IIR);
> > +
> > +		intel_psr_irq_handler(dev_priv, psr_iir);
> > +		I915_WRITE(EDP_PSR_IIR, psr_iir);
> 
> Two things unrelated to your refactoring, 
> 
> 1) we can ack the irq right after reading the iir bits. I had noticed this a
> while back but never got to changing it.

See https://patchwork.freedesktop.org/series/59512/

> 
> 2) we don't DRM_ERROR() for unexpected PSR interrupts like how other handlers
> do. Not sure what's the point though, other than getting to know that the
> hardware is broken. 
> 
> Ville, any idea why unexpected interrupts warrant DRM_ERROR().

I suppose people wanted some confirmation that the code and hardware
are doing the right thing.

> 
> 
> This patch is 
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> 
> > +		found = true;
> > +	}
> > +
> > +	if (!found)
> > +		DRM_ERROR("Unexpected DE Misc interrupt\n");
> > +}
> > +
> >  static irqreturn_t
> >  gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
> >  {
> > @@ -2778,29 +2800,12 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv,
> > u32 master_ctl)
> >  	if (master_ctl & GEN8_DE_MISC_IRQ) {
> >  		iir = I915_READ(GEN8_DE_MISC_IIR);
> >  		if (iir) {
> > -			bool found = false;
> > -
> >  			I915_WRITE(GEN8_DE_MISC_IIR, iir);
> >  			ret = IRQ_HANDLED;
> > -
> > -			if (iir & GEN8_DE_MISC_GSE) {
> > -				intel_opregion_asle_intr(dev_priv);
> > -				found = true;
> > -			}
> > -
> > -			if (iir & GEN8_DE_EDP_PSR) {
> > -				u32 psr_iir = I915_READ(EDP_PSR_IIR);
> > -
> > -				intel_psr_irq_handler(dev_priv, psr_iir);
> > -				I915_WRITE(EDP_PSR_IIR, psr_iir);
> > -				found = true;
> > -			}
> > -
> > -			if (!found)
> > -				DRM_ERROR("Unexpected DE Misc interrupt\n");
> > -		}
> > -		else
> > +			gen8_de_misc_irq_handler(dev_priv, iir);
> > +		} else {
> >  			DRM_ERROR("The master control interrupt lied (DE
> > MISC)!\n");
> > +		}
> >  	}
> >  
> >  	if (INTEL_GEN(dev_priv) >= 11 && (master_ctl & GEN11_DE_HPD_IRQ)) {

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

Re: [PATCH v4 3/4] drm/i915: Make PSR registers relative to transcoders

[Dhinakaran Pandiyan]

On Wed, 2019-04-17 at 15:37 -0700, José Roberto de Souza wrote:
> PSR registers are a mess, some have the full address while others just
> have the additional offset from psr_mmio_base.
> 
> psr_mmio_base is nothing more than TRANSCODER_EDP_OFFSET + 0x800 and
> using it makes more difficult for people with an PSR register address
> or PSR register name from from BSpec as i915 also don't match the
> name.
I'd like this to be rewritten for clarity.

> 
> Other reason to make relative to transcoder is that since BDW every
> transcoder have PSR registers, so in theory it should be possible to
> have PSR enabled in a non-eDP transcoder.
> 
> For Haswell PSR registers are relative to 0x64000, so
> mmio_base_adjust and _TRANS2_PSR() was added so we can overcome
> this limitation in a single place.
> PSR2 registers and PSR_EVENT was added after Haswell so that is why
> _TRANS2_PSR() is not used in some macros.
> 
> The only registers that can not be relative to transcoder are
> PSR_IMR and PSR_IIR that are not relative to anything, so keeping it
> hardcoded.
> 
> Also removing BDW_EDP_PSR_BASE from GVT because it is not used as it
> is the only PSR register that GVT have.
> 
> v4:
> - Moved definition of _TRANS2_PSR() and _MMIO_TRANS2_PSR() to the
> beginning of i915_reg.h (Jani)
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Zhi Wang <zhi.a.wang@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/gvt/handlers.c |  1 -
>  drivers/gpu/drm/i915/i915_drv.h     |  5 +--
>  drivers/gpu/drm/i915/i915_reg.h     | 48 ++++++++++++++++++++---------
>  drivers/gpu/drm/i915/intel_psr.c    | 11 +++++--
>  4 files changed, 45 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c
> b/drivers/gpu/drm/i915/gvt/handlers.c
> index 18f01eeb2510..749e3e4204f2 100644
> --- a/drivers/gpu/drm/i915/gvt/handlers.c
> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> @@ -2776,7 +2776,6 @@ static int init_broadwell_mmio_info(struct intel_gvt
> *gvt)
>  	MMIO_D(CHICKEN_PIPESL_1(PIPE_C), D_BDW_PLUS);
>  
>  	MMIO_D(WM_MISC, D_BDW);
> -	MMIO_D(_MMIO(BDW_EDP_PSR_BASE), D_BDW);
>  
>  	MMIO_D(_MMIO(0x6671c), D_BDW_PLUS);
>  	MMIO_D(_MMIO(0x66c00), D_BDW_PLUS);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 066fd2a12851..58748f23511f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -494,6 +494,8 @@ struct i915_drrs {
>  };
>  
>  struct i915_psr {
> +	/* different than zero only on HSW see _TRANS2_PSR() for more info */
> +	u32 mmio_base_adjust;

If you rename this to hsw_base_adjust, the name becomes self-documenting and we
can get rid of the comment.

>  	struct mutex lock;
>  
>  #define I915_PSR_DEBUG_MODE_MASK	0x0f
> @@ -508,6 +510,7 @@ struct i915_psr {
>  	bool enabled;
>  	struct intel_dp *dp;
>  	enum pipe pipe;
> +	enum transcoder transcoder;
>  	bool active;
>  	struct work_struct work;
>  	unsigned busy_frontbuffer_bits;
> @@ -1534,8 +1537,6 @@ struct drm_i915_private {
>  	/* MMIO base address for MIPI regs */
>  	u32 mipi_mmio_base;
>  
> -	u32 psr_mmio_base;
> -
>  	u32 pps_mmio_base;
>  
>  	wait_queue_head_t gmbus_wait_queue;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9ef306b79e0d..987006946802 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -258,6 +258,10 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  					      INTEL_INFO(dev_priv)-
> >cursor_offsets[PIPE_A] + (reg) + \
>  					      DISPLAY_MMIO_BASE(dev_priv))
>  
> +/* PSR registers on HSW is not relative to eDP transcoder */
> +#define _TRANS2_PSR(reg)	(_TRANS2(dev_priv->psr.transcoder, (reg)) -
> dev_priv->psr.mmio_base_adjust)
> +#define _MMIO_TRANS2_PSR(reg)	_MMIO(_TRANS2_PSR(reg))
> +

I think the macro should be defined in it's final form in this commit. While I
understand you want to split the patch for making review easy, not convinced we
it's a good idea to add macros that change in the next patch.

We can define the macro to accept (tran, reg) as arguments in this patch but
have intel_psr_compute_config() reject non-eDP transcoders.

intel_psr_compute_config(intel_dp, crtc_state)
{
	if (crtc_state->cpu_transcoder
!= TRANSCODER_EDP)
		return;
	...
}
This allows you to add the macro without changing any functionality.


If you insist on not making changes to intel_psr.c, how about introducing the
full macro in this patch but hard-coding TRANCODER_EDP

#define PSR_CTL(tran)	_MMIO_TRANS2_PSR(TRANSCODER_EDP, _SRD_CTL_A)
#define PSR_STATUS(tran) _MMIO_TRANS2_PSR(TRANSCODER_EDP, _SRD_STATUS_A)

Comments about _MMIO_TRANS2_PSR below.


>  #define __MASKED_FIELD(mask, value) ((mask) << 16 | (value))
>  #define _MASKED_FIELD(mask, value) ({					
>    \
>  	if (__builtin_constant_p(mask))					   \
> @@ -4213,9 +4217,11 @@ enum {
>  #define PIPE_MULT(trans)	_MMIO_TRANS2(trans, _PIPE_MULT_A)
>  
>  /* HSW+ eDP PSR registers */
> -#define HSW_EDP_PSR_BASE	0x64800
> -#define BDW_EDP_PSR_BASE	0x6f800
> -#define EDP_PSR_CTL				_MMIO(dev_priv->psr_mmio_base +
> 0)
> +#define HSW_EDP_PSR_BASE	0x64000
> +
> +#define _SRD_CTL_A				0x60800
> +#define _SRD_CTL_EDP				0x6F800
						   ^f (use lower case for hex digits) here and elsewhere in this patch.

> +#define EDP_PSR_CTL				_MMIO_TRANS2_PSR(_SRD_CTL_A)
>  #define   EDP_PSR_ENABLE			(1 << 31)
>  #define   BDW_PSR_SINGLE_FRAME			(1 << 30)
>  #define   EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK	(1 << 29) /* SW can't
> modify */
> @@ -4252,16 +4258,22 @@ enum {
>  #define   EDP_PSR_TRANSCODER_A_SHIFT		8
>  #define   EDP_PSR_TRANSCODER_EDP_SHIFT		0
>  
> -#define EDP_PSR_AUX_CTL				_MMIO(dev_priv-
> >psr_mmio_base + 0x10)
> +#define _SRD_AUX_CTL_A				0x60810
> +#define _SRD_AUX_CTL_EDP			0x6F810
> +#define EDP_PSR_AUX_CTL				_MMIO_TRANS2_PSR(_SRD_AU
> X_CTL_A)
>  #define   EDP_PSR_AUX_CTL_TIME_OUT_MASK		(3 << 26)
>  #define   EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK	(0x1f << 20)
>  #define   EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK	(0xf << 16)
>  #define   EDP_PSR_AUX_CTL_ERROR_INTERRUPT	(1 << 11)
>  #define   EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK	(0x7ff)
>  
> -#define EDP_PSR_AUX_DATA(i)			_MMIO(dev_priv->psr_mmio_base +
> 0x14 + (i) * 4) /* 5 registers */
> +#define _SRD_AUX_DATA_A				0x60814
> +#define _SRD_AUX_DATA_EDP			0x6F814
> +#define EDP_PSR_AUX_DATA(i)			_MMIO(_TRANS2_PSR(_SRD_AUX_DATA_
> A) + (i) + 4) /* 5 registers */
>  
> -#define EDP_PSR_STATUS				_MMIO(dev_priv-
> >psr_mmio_base + 0x40)
> +#define _SRD_STATUS_A				0x60840
> +#define _SRD_STATUS_EDP				0x6F840
> +#define EDP_PSR_STATUS				_MMIO_TRANS2_PSR(_SRD_ST
> ATUS_A)
>  #define   EDP_PSR_STATUS_STATE_MASK		(7 << 29)
>  #define   EDP_PSR_STATUS_STATE_SHIFT		29
>  #define   EDP_PSR_STATUS_STATE_IDLE		(0 << 29)
> @@ -4286,10 +4298,15 @@ enum {
>  #define   EDP_PSR_STATUS_SENDING_TP1		(1 << 4)
>  #define   EDP_PSR_STATUS_IDLE_MASK		0xf
>  
> -#define EDP_PSR_PERF_CNT		_MMIO(dev_priv->psr_mmio_base + 0x44)
> +#define _SRD_PERF_CNT_A			0x60844
> +#define _SRD_PERF_CNT_EDP		0x6F844
> +#define EDP_PSR_PERF_CNT		_MMIO_TRANS2_PSR(_SRD_PERF_CNT_A)
>  #define   EDP_PSR_PERF_CNT_MASK		0xffffff
>  
> -#define EDP_PSR_DEBUG				_MMIO(dev_priv-
> >psr_mmio_base + 0x60) /* PSR_MASK on SKL+ */
> +/* PSR_MASK on SKL+ */
> +#define _SRD_DEBUG_A				0x60860
> +#define _SRD_DEBUG_EDP				0x6F860
> +#define EDP_PSR_DEBUG				_MMIO_TRANS2_PSR(_SRD_DE
> BUG_A)
>  #define   EDP_PSR_DEBUG_MASK_MAX_SLEEP         (1 << 28)
>  #define   EDP_PSR_DEBUG_MASK_LPSP              (1 << 27)
>  #define   EDP_PSR_DEBUG_MASK_MEMUP             (1 << 26)
> @@ -4297,7 +4314,9 @@ enum {
>  #define   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE    (1 << 16) /* Reserved in ICL+
> */
>  #define   EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15) /* SKL+ */
>  
> -#define EDP_PSR2_CTL			_MMIO(0x6f900)
> +#define _PSR2_CTL_A			0x60900
> +#define _PSR2_CTL_EDP			0x6F900
> +#define EDP_PSR2_CTL			_MMIO_TRANS2(dev_priv->psr.transcoder,
> _PSR2_CTL_A)
>  #define   EDP_PSR2_ENABLE		(1 << 31)
>  #define   EDP_SU_TRACK_ENABLE		(1 << 30)
>  #define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and CNL+ */
> @@ -4338,14 +4357,15 @@ enum {
>  #define  PSR_EVENT_LPSP_MODE_EXIT		(1 << 1)
>  #define  PSR_EVENT_PSR_DISABLE			(1 << 0)
>  
> -#define EDP_PSR2_STATUS			_MMIO(0x6f940)
> +#define _PSR2_STATUS_A			0x60940
This is a completely made up register, isn't it? 

> +#define _PSR2_STATUS_EDP		0x6F940
> +#define EDP_PSR2_STATUS			_MMIO_TRANS2(dev_priv-
> >psr.transcoder, _PSR2_STATUS_A)
>  #define EDP_PSR2_STATUS_STATE_MASK     (0xf << 28)
>  #define EDP_PSR2_STATUS_STATE_SHIFT    28
>  
> -#define _PSR2_SU_STATUS_0		0x6F914
> -#define _PSR2_SU_STATUS_1		0x6F918
> -#define _PSR2_SU_STATUS_2		0x6F91C
> -#define _PSR2_SU_STATUS(index)		_MMIO(_PICK_EVEN((index),
> _PSR2_SU_STATUS_0, _PSR2_SU_STATUS_1))
> +#define _PSR2_SU_STATUS_A		0x60914
> +#define _PSR2_SU_STATUS_EDP		0x6F914
> +#define _PSR2_SU_STATUS(index)		_MMIO(_TRANS2(dev_priv-
> >psr.transcoder, _PSR2_SU_STATUS_A) + (index) * 4)
>  #define PSR2_SU_STATUS(frame)		(_PSR2_SU_STATUS((frame) / 3))
>  #define PSR2_SU_STATUS_SHIFT(frame)	(((frame) % 3) * 10)
>  #define PSR2_SU_STATUS_MASK(frame)	(0x3ff << PSR2_SU_STATUS_SHIFT(frame))
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 963663ba0edf..da351c8f86d7 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -742,6 +742,14 @@ static void intel_psr_enable_locked(struct
> drm_i915_private *dev_priv,
>  	dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state);
>  	dev_priv->psr.busy_frontbuffer_bits = 0;
>  	dev_priv->psr.pipe = to_intel_crtc(crtc_state->base.crtc)->pipe;
> +	dev_priv->psr.transcoder = crtc_state->cpu_transcoder;
> +
> +	if (IS_HASWELL(dev_priv)) {
> +		u32 trans_offset = INTEL_INFO(dev_priv)->trans_offsets[dev_priv-
> >psr.transcoder];
> +
> +		WARN_ON(trans_offset < HSW_EDP_PSR_BASE);
> +		dev_priv->psr.mmio_base_adjust = trans_offset -
> HSW_EDP_PSR_BASE;

The HSW PSR registers are not per-transcoder registers and as such it just feels
wrong to compute .mmio_base_adjust based on the transcoder. Here's what I
propose


#define HSW_EDP_PSR_BASE		0x64800
#define _SRD_CTL_A			0x60800
#define _MMIO_HSW_PSR_ADJ(reg)		_MMIO(reg - _SRD_CTL_A + HSW_EDP_PSR_BASE)
#define _MMIO_PSR_ADJ(tran, reg)	IS_HASWELL(dev_priv) ? 	\								_MMIO_HS
W_PSR_ADJ(reg) :\
 	                                       _MMIO_TRANS2(tran, reg)
#define EDP_PSR_CTL(tran)       _MMIO_PSR_ADJ(tran, _SRD_CTL_A)
#define EDP_PSR_STATUS(tran)	_MMIO_PSR_ADJ(tran, _SRD_STATUS_A)
...

Or 

#define HSW_EDP_PSR_BASE                        0x64800
#define BDW_EDP_PSR_BASE                        0x6f800
#define _MMIO_PSR_ADJ(tran, reg)		_MMIO_TRANS2(tran, reg - dev_priv-
>psr.hsw_base_adj)
#define EDP_PSR_CTL(tran)                       _MMIO_PSR_ADJ(tran, _SRD_CTL_A)

with

> +	}
>  
>  	DRM_DEBUG_KMS("Enabling PSR%s\n",
>  		      dev_priv->psr.psr2_enabled ? "2" : "1");
> @@ -1206,9 +1214,6 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
>  	if (!HAS_PSR(dev_priv))
>  		return;
>  
> -	dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ?
> -		HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE;
> -
	if (IS_HASWELL(dev_priv))
		dev_priv->hsw_base_adj =
BDW_EDP_PSR_BASE - HSW_EDP_PSR_BASE;

	

>  	if (!dev_priv->psr.sink_support)
>  		return;
>  

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

Re: [PATCH v4 2/4] drm/i915: Add _TRANS2()

[Pandiyan, Dhinakaran]

On Wed, 2019-04-17 at 15:37 -0700, José Roberto de Souza wrote:
> A new macro that is going to be added in a further patch will need to
> adjust the offset returned by _MMIO_TRANS2(), so here adding
> _TRANS2() and moving most of the implementation of _MMIO_TRANS2() to
> it and while at it taking the opportunity to rename pipe to trans.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b74824f0b5b1..9ef306b79e0d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -250,9 +250,10 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  #define _MMIO_PIPE2(pipe, reg)		_MMIO(INTEL_INFO(dev_priv)-
> >pipe_offsets[pipe] - \
>  					      INTEL_INFO(dev_priv)-
> >pipe_offsets[PIPE_A] + (reg) + \
>  					      DISPLAY_MMIO_BASE(dev_priv))
> -#define _MMIO_TRANS2(pipe, reg)		_MMIO(INTEL_INFO(dev_priv)-
> >trans_offsets[(pipe)] - \
> -					      INTEL_INFO(dev_priv)-
> >trans_offsets[TRANSCODER_A] + (reg) + \
> -					      DISPLAY_MMIO_BASE(dev_priv))
> +#define _TRANS2(trans, reg)		(INTEL_INFO(dev_priv)-
_TRANS() and _MMIO_TRANS() name the first argument "tran"

Can you please keep the same name "tran"?

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiya@intel.com>

> >trans_offsets[(trans)] - \
> +					 INTEL_INFO(dev_priv)-
> >trans_offsets[TRANSCODER_A] + (reg) + \
> +					 DISPLAY_MMIO_BASE(dev_priv))
> +#define _MMIO_TRANS2(trans, reg)	_MMIO(_TRANS2(trans, reg))
>  #define _CURSOR2(pipe, reg)		_MMIO(INTEL_INFO(dev_priv)-
> >cursor_offsets[(pipe)] - \
>  					      INTEL_INFO(dev_priv)-
> >cursor_offsets[PIPE_A] + (reg) + \
>  					      DISPLAY_MMIO_BASE(dev_priv))
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 2/4] drm/i915: Add _TRANS2()

[Lucas De Marchi]

On Wed, Apr 17, 2019 at 09:09:40PM -0700, Dhinakaran Pandiyan wrote:
>On Wed, 2019-04-17 at 15:37 -0700, José Roberto de Souza wrote:
>> A new macro that is going to be added in a further patch will need to
>> adjust the offset returned by _MMIO_TRANS2(), so here adding
>> _TRANS2() and moving most of the implementation of _MMIO_TRANS2() to
>> it and while at it taking the opportunity to rename pipe to trans.
>>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index b74824f0b5b1..9ef306b79e0d 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -250,9 +250,10 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>>  #define _MMIO_PIPE2(pipe, reg)		_MMIO(INTEL_INFO(dev_priv)-
>> > pipe_offsets[pipe] - \
>>
>>  					      INTEL_INFO(dev_priv)-
>> > pipe_offsets[PIPE_A] + (reg) + \
>>
>>  					      DISPLAY_MMIO_BASE(dev_priv))
>> -#define _MMIO_TRANS2(pipe, reg)		_MMIO(INTEL_INFO(dev_priv)-
>> > trans_offsets[(pipe)] - \
>>
>> -					      INTEL_INFO(dev_priv)-
>> > trans_offsets[TRANSCODER_A] + (reg) + \
>>
>> -					      DISPLAY_MMIO_BASE(dev_priv))
>> +#define _TRANS2(trans, reg)		(INTEL_INFO(dev_priv)-
>
>_TRANS() and _MMIO_TRANS() name the first argument "tran"
>
>Can you please keep the same name "tran"?

humn... but we also have several local "trans" variables, HTOTAL(),
HBLANK, etc, and there's also "trans_offsets" that is used here.

_TRANS() and _MMIO_TRANS() look more like the offenders to me.


For the patch, regardless of the argument name:

Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

Lucas De Marchi



>
>Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiya@intel.com>
>
>> > trans_offsets[(trans)] - \
>>
>> +					 INTEL_INFO(dev_priv)-
>> > trans_offsets[TRANSCODER_A] + (reg) + \
>>
>> +					 DISPLAY_MMIO_BASE(dev_priv))
>> +#define _MMIO_TRANS2(trans, reg)	_MMIO(_TRANS2(trans, reg))
>>  #define _CURSOR2(pipe, reg)		_MMIO(INTEL_INFO(dev_priv)-
>> > cursor_offsets[(pipe)] - \
>>
>>  					      INTEL_INFO(dev_priv)-
>> > cursor_offsets[PIPE_A] + (reg) + \
>>
>>  					      DISPLAY_MMIO_BASE(dev_priv))
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx