[PATCH 00/11] Enable PSR on Haswell.

[PATCH 00/11] Enable PSR on Haswell.

[Rodrigo Vivi]

I'm resending full series again because after accepting most of suggestions
and rebasing again on drm-intel-nightly most of patches got some kind of
conflict so the full series is here again.

First 3 patches on this series are already reviewed and I'd be glad if they
were merged asap to avoid future conflicts. This patches at least allows
people to know if they have psr panel or not.

For the rest I accepted most of suggestions and explained on previous emails
the ones I didn't accepted and why. However even the ones I didn't accepted
I tested and verified that they caused some kind of issue.

This version is working very fine for a long time in my machine. I'd appreciate if you could merge everything since now psr is disabled by default by kernel flag. So I'm 100% sure that this series won't cause any kind of regression for any user.

I understand that it would be good to deliver psr enabled by default however I'm changing this default behaviour because I'm sure that PSR will cause regression without userspace (DDX) help when using kde and xdm.

Thanks in advance,
Rodrigo.

Rodrigo Vivi (9):
  drm/i915: split aux_clock_divider logic in a separated function for
    reuse.
  drm/i915: Enable/Disable PSR
  drm/i915: Added debugfs support for PSR Status
  drm/i915: Match all PSR mode entry conditions before enabling it.
  drm/i915: add update function to disable/enable-back PSR
  drm/intel: add enable_psr module option and disable psr by default
  drm/i915: Adding global I915_PARAM for PSR ENABLED.
  drm/i915: Add functions to force psr exit
  drm/i915: Hook PSR functionality

Shobhit Kumar (2):
  drm: Added SDP and VSC structures for handling PSR for eDP
  drm/i915: Read the EDP DPCD and PSR Capability

 drivers/gpu/drm/i915/i915_debugfs.c  | 128 ++++++++++++
 drivers/gpu/drm/i915/i915_dma.c      |   3 +
 drivers/gpu/drm/i915/i915_drv.c      |   4 +
 drivers/gpu/drm/i915/i915_drv.h      |  15 ++
 drivers/gpu/drm/i915/i915_gem.c      |   2 +
 drivers/gpu/drm/i915/i915_reg.h      |  74 +++++++
 drivers/gpu/drm/i915/intel_ddi.c     |   2 +
 drivers/gpu/drm/i915/intel_display.c |   1 +
 drivers/gpu/drm/i915/intel_dp.c      | 373 ++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h     |  10 +
 include/drm/drm_dp_helper.h          |  31 ++-
 include/uapi/drm/i915_drm.h          |   1 +
 12 files changed, 618 insertions(+), 26 deletions(-)

-- 
1.7.11.7

[PATCH 01/11] drm: Added SDP and VSC structures for handling PSR for eDP

[Rodrigo Vivi]

From: Shobhit Kumar <shobhit.kumar@intel.com>

SDP header and SDP VSC header as per eDP 1.3 spec, section 3.5,
chapter "PSR Secondary Data Package Support".

v2: Modified and corrected the structures to be more in line for
kernel coding guidelines and rebased the code on Paulo's DP patchset
v3: removing unecessary identation at DP_RECEIVER_CAP_SIZE
v4: moving them to include/drm/drm_dp_helper.h and also already
    icluding EDP_PSR_RECEIVER_CAP_SIZE to add everything needed
    for PSR at once at drm_dp_helper.h
v5: Fix SDP VSC header and identation by (Paulo Zanoni) and
    remove i915 from title (Daniel Vetter)
v6: Fix spec version and move comments from code to commit message
    since numbers might change in the future (by Paulo Zanoni).

CC: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Sateesh Kavuri <sateesh.kavuri@intel.com>
Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 include/drm/drm_dp_helper.h | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index e8e1417..ae8dbfb 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -342,13 +342,42 @@ u8 drm_dp_get_adjust_request_voltage(u8 link_status[DP_LINK_STATUS_SIZE],
 u8 drm_dp_get_adjust_request_pre_emphasis(u8 link_status[DP_LINK_STATUS_SIZE],
 					  int lane);
 
-#define DP_RECEIVER_CAP_SIZE	0xf
+#define DP_RECEIVER_CAP_SIZE		0xf
+#define EDP_PSR_RECEIVER_CAP_SIZE	2
+
 void drm_dp_link_train_clock_recovery_delay(u8 dpcd[DP_RECEIVER_CAP_SIZE]);
 void drm_dp_link_train_channel_eq_delay(u8 dpcd[DP_RECEIVER_CAP_SIZE]);
 
 u8 drm_dp_link_rate_to_bw_code(int link_rate);
 int drm_dp_bw_code_to_link_rate(u8 link_bw);
 
+struct edp_sdp_header {
+	u8 HB0; /* Secondary Data Packet ID */
+	u8 HB1; /* Secondary Data Packet Type */
+	u8 HB2; /* 7:5 reserved, 4:0 revision number */
+	u8 HB3; /* 7:5 reserved, 4:0 number of valid data bytes */
+} __packed;
+
+#define EDP_SDP_HEADER_REVISION_MASK		0x1F
+#define EDP_SDP_HEADER_VALID_PAYLOAD_BYTES	0x1F
+
+struct edp_vsc_psr {
+	struct edp_sdp_header sdp_header;
+	u8 DB0; /* Stereo Interface */
+	u8 DB1; /* 0 - PSR State; 1 - Update RFB; 2 - CRC Valid */
+	u8 DB2; /* CRC value bits 7:0 of the R or Cr component */
+	u8 DB3; /* CRC value bits 15:8 of the R or Cr component */
+	u8 DB4; /* CRC value bits 7:0 of the G or Y component */
+	u8 DB5; /* CRC value bits 15:8 of the G or Y component */
+	u8 DB6; /* CRC value bits 7:0 of the B or Cb component */
+	u8 DB7; /* CRC value bits 15:8 of the B or Cb component */
+	u8 DB8_31[24]; /* Reserved */
+} __packed;
+
+#define EDP_VSC_PSR_STATE_ACTIVE	(1<<0)
+#define EDP_VSC_PSR_UPDATE_RFB		(1<<1)
+#define EDP_VSC_PSR_CRC_VALUES_VALID	(1<<2)
+
 static inline int
 drm_dp_max_link_rate(u8 dpcd[DP_RECEIVER_CAP_SIZE])
 {
-- 
1.7.11.7

[PATCH 02/11] drm/i915: Read the EDP DPCD and PSR Capability

[Rodrigo Vivi]

From: Shobhit Kumar <shobhit.kumar@intel.com>

v2: reuse of just created is_edp_psr and put it at right place.
v3: move is_edp_psr above intel_edp_disable
v4: remove parentheses. Noticed by Paulo.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 13 +++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 7db2cd7..bf0bfa1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1369,6 +1369,12 @@ static void intel_dp_get_config(struct intel_encoder *encoder,
 	}
 }
 
+static bool is_edp_psr(struct intel_dp *intel_dp)
+{
+	return is_edp(intel_dp) &&
+		intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED;
+}
+
 static void intel_disable_dp(struct intel_encoder *encoder)
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
@@ -2282,6 +2288,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	if (intel_dp->dpcd[DP_DPCD_REV] == 0)
 		return false; /* DPCD not present */
 
+	/* Check if the panel supports PSR */
+	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
+	intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
+				       intel_dp->psr_dpcd,
+				       sizeof(intel_dp->psr_dpcd));
+	if (is_edp_psr(intel_dp))
+		DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
 	if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
 	      DP_DWN_STRM_PORT_PRESENT))
 		return true; /* native DP sink */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5dfc1a0..d25726d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -487,6 +487,7 @@ struct intel_dp {
 	uint8_t link_bw;
 	uint8_t lane_count;
 	uint8_t dpcd[DP_RECEIVER_CAP_SIZE];
+	uint8_t psr_dpcd[EDP_PSR_RECEIVER_CAP_SIZE];
 	uint8_t downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
 	struct i2c_adapter adapter;
 	struct i2c_algo_dp_aux_data algo;
-- 
1.7.11.7

[PATCH 03/11] drm/i915: split aux_clock_divider logic in a separated function for reuse.

[Rodrigo Vivi]

Prep patch for reuse aux_clock_divider with EDP_PSR_AUX_CTL setup.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 58 +++++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index bf0bfa1..d273e36 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -276,29 +276,12 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
 	return status;
 }
 
-static int
-intel_dp_aux_ch(struct intel_dp *intel_dp,
-		uint8_t *send, int send_bytes,
-		uint8_t *recv, int recv_size)
+static uint32_t get_aux_clock_divider(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	uint32_t ch_ctl = intel_dp->aux_ch_ctl_reg;
-	uint32_t ch_data = ch_ctl + 4;
-	int i, ret, recv_bytes;
-	uint32_t status;
-	uint32_t aux_clock_divider;
-	int try, precharge;
-	bool has_aux_irq = INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev);
 
-	/* dp aux is extremely sensitive to irq latency, hence request the
-	 * lowest possible wakeup latency and so prevent the cpu from going into
-	 * deep sleep states.
-	 */
-	pm_qos_update_request(&dev_priv->pm_qos, 0);
-
-	intel_dp_check_edp(intel_dp);
 	/* The clock divider is based off the hrawclk,
 	 * and would like to run at 2MHz. So, take the
 	 * hrawclk value and divide by 2 and use that
@@ -307,23 +290,48 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 	 * clock divider.
 	 */
 	if (IS_VALLEYVIEW(dev)) {
-		aux_clock_divider = 100;
+		return 100;
 	} else if (intel_dig_port->port == PORT_A) {
 		if (HAS_DDI(dev))
-			aux_clock_divider = DIV_ROUND_CLOSEST(
+			return DIV_ROUND_CLOSEST(
 				intel_ddi_get_cdclk_freq(dev_priv), 2000);
 		else if (IS_GEN6(dev) || IS_GEN7(dev))
-			aux_clock_divider = 200; /* SNB & IVB eDP input clock at 400Mhz */
+			return 200; /* SNB & IVB eDP input clock at 400Mhz */
 		else
-			aux_clock_divider = 225; /* eDP input clock at 450Mhz */
+			return 225; /* eDP input clock at 450Mhz */
 	} else if (dev_priv->pch_id == INTEL_PCH_LPT_DEVICE_ID_TYPE) {
 		/* Workaround for non-ULT HSW */
-		aux_clock_divider = 74;
+		return 74;
 	} else if (HAS_PCH_SPLIT(dev)) {
-		aux_clock_divider = DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
+		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
 	} else {
-		aux_clock_divider = intel_hrawclk(dev) / 2;
+		return intel_hrawclk(dev) / 2;
 	}
+}
+
+static int
+intel_dp_aux_ch(struct intel_dp *intel_dp,
+		uint8_t *send, int send_bytes,
+		uint8_t *recv, int recv_size)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t ch_ctl = intel_dp->aux_ch_ctl_reg;
+	uint32_t ch_data = ch_ctl + 4;
+	int i, ret, recv_bytes;
+	uint32_t status;
+	uint32_t aux_clock_divider = get_aux_clock_divider(intel_dp);
+	int try, precharge;
+	bool has_aux_irq = INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev);
+
+	/* dp aux is extremely sensitive to irq latency, hence request the
+	 * lowest possible wakeup latency and so prevent the cpu from going into
+	 * deep sleep states.
+	 */
+	pm_qos_update_request(&dev_priv->pm_qos, 0);
+
+	intel_dp_check_edp(intel_dp);
 
 	if (IS_GEN6(dev))
 		precharge = 3;
-- 
1.7.11.7

[PATCH 04/11] drm/i915: Enable/Disable PSR

[Rodrigo Vivi]

Adding Enable and Disable PSR functionalities. This includes setting the
PSR configuration over AUX, sending SDP VSC DIP over the eDP PIPE config,
enabling PSR in the sink via DPCD register and finally enabling PSR on
the host.

This patch is based on initial PSR code by Sateesh Kavuri and Kumar Shobhit
but in a different implementation.

v2: * moved functions around and changed its names.
    * removed VSC DIP unset from disable.
    * remove FBC wa.
    * don't mask LSPS anymore.
    * incorporate new crtc usage after a rebase.
v3: Make a clear separation between Sink (Panel) and Source (HW) enabling.
v4: Fix identation and other style issues raised by checkpatch (by Paulo).
v5: Changes according to Paulo's review:
    static on write_vsc;
    avoid using dp_to_dev when already calling dp_to_dig_port;
    remove unecessary TP default time setting;
    remove unecessary interrupts disabling;
    remove unecessary wait_for_vblank when disabling psr;
v6: remove unecessary wait_for_vblank when writing vsc;
v7: adding setup once function to avoid unnecessarily write to vsc
    and set debug_ctl every time we enable or disable psr.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Credits-by: Sateesh Kavuri <sateesh.kavuri@intel.com>
Credits-by: Shobhit Kumar <shobhit.kumar@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_reg.h  |  42 +++++++++++
 drivers/gpu/drm/i915/intel_dp.c  | 149 +++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |   4 ++
 3 files changed, 195 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index dc3d6a7..31e4dbb 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1779,6 +1779,47 @@
 #define BCLRPAT(pipe) _PIPE(pipe, _BCLRPAT_A, _BCLRPAT_B)
 #define VSYNCSHIFT(trans) _TRANSCODER(trans, _VSYNCSHIFT_A, _VSYNCSHIFT_B)
 
+/* HSW eDP PSR registers */
+#define EDP_PSR_CTL				0x64800
+#define   EDP_PSR_ENABLE			(1<<31)
+#define   EDP_PSR_LINK_DISABLE			(0<<27)
+#define   EDP_PSR_LINK_STANDBY			(1<<27)
+#define   EDP_PSR_MIN_LINK_ENTRY_TIME_MASK	(3<<25)
+#define   EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES	(0<<25)
+#define   EDP_PSR_MIN_LINK_ENTRY_TIME_4_LINES	(1<<25)
+#define   EDP_PSR_MIN_LINK_ENTRY_TIME_2_LINES	(2<<25)
+#define   EDP_PSR_MIN_LINK_ENTRY_TIME_0_LINES	(3<<25)
+#define   EDP_PSR_MAX_SLEEP_TIME_SHIFT		20
+#define   EDP_PSR_SKIP_AUX_EXIT			(1<<12)
+#define   EDP_PSR_TP1_TP2_SEL			(0<<11)
+#define   EDP_PSR_TP1_TP3_SEL			(1<<11)
+#define   EDP_PSR_TP2_TP3_TIME_500us		(0<<8)
+#define   EDP_PSR_TP2_TP3_TIME_100us		(1<<8)
+#define   EDP_PSR_TP2_TP3_TIME_2500us		(2<<8)
+#define   EDP_PSR_TP2_TP3_TIME_0us		(3<<8)
+#define   EDP_PSR_TP1_TIME_500us		(0<<4)
+#define   EDP_PSR_TP1_TIME_100us		(1<<4)
+#define   EDP_PSR_TP1_TIME_2500us		(2<<4)
+#define   EDP_PSR_TP1_TIME_0us			(3<<4)
+#define   EDP_PSR_IDLE_FRAME_SHIFT		0
+
+#define EDP_PSR_AUX_CTL			0x64810
+#define EDP_PSR_AUX_DATA1		0x64814
+#define   EDP_PSR_DPCD_COMMAND		0x80060000
+#define EDP_PSR_AUX_DATA2		0x64818
+#define   EDP_PSR_DPCD_NORMAL_OPERATION	(1<<24)
+#define EDP_PSR_AUX_DATA3		0x6481c
+#define EDP_PSR_AUX_DATA4		0x64820
+#define EDP_PSR_AUX_DATA5		0x64824
+
+#define EDP_PSR_STATUS_CTL			0x64840
+#define   EDP_PSR_STATUS_STATE_MASK		(7<<29)
+
+#define EDP_PSR_DEBUG_CTL		0x64860
+#define   EDP_PSR_DEBUG_MASK_LPSP	(1<<27)
+#define   EDP_PSR_DEBUG_MASK_MEMUP	(1<<26)
+#define   EDP_PSR_DEBUG_MASK_HPD	(1<<25)
+
 /* VGA port control */
 #define ADPA			0x61100
 #define PCH_ADPA                0xe1100
@@ -2048,6 +2089,7 @@
  * (Haswell and newer) to see which VIDEO_DIP_DATA byte corresponds to each byte
  * of the infoframe structure specified by CEA-861. */
 #define   VIDEO_DIP_DATA_SIZE	32
+#define   VIDEO_DIP_VSC_DATA_SIZE	36
 #define VIDEO_DIP_CTL		0x61170
 /* Pre HSW: */
 #define   VIDEO_DIP_ENABLE		(1 << 31)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d273e36..d4b52a9 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1383,6 +1383,153 @@ static bool is_edp_psr(struct intel_dp *intel_dp)
 		intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED;
 }
 
+static bool intel_edp_is_psr_enabled(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (!IS_HASWELL(dev))
+		return false;
+
+	return I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
+}
+
+static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp,
+				    struct edp_vsc_psr *vsc_psr)
+{
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *crtc = to_intel_crtc(dig_port->base.base.crtc);
+	u32 ctl_reg = HSW_TVIDEO_DIP_CTL(crtc->config.cpu_transcoder);
+	u32 data_reg = HSW_TVIDEO_DIP_VSC_DATA(crtc->config.cpu_transcoder);
+	uint32_t *data = (uint32_t *) vsc_psr;
+	unsigned int i;
+
+	/* As per BSPec (Pipe Video Data Island Packet), we need to disable
+	   the video DIP being updated before program video DIP data buffer
+	   registers for DIP being updated. */
+	I915_WRITE(ctl_reg, ~VIDEO_DIP_ENABLE_VSC_HSW);
+	POSTING_READ(ctl_reg);
+
+	for (i = 0; i < VIDEO_DIP_VSC_DATA_SIZE; i += 4) {
+		if (i < sizeof(struct edp_vsc_psr))
+			I915_WRITE(data_reg + i, *data++);
+		else
+			I915_WRITE(data_reg + i, 0);
+	}
+
+	I915_WRITE(ctl_reg, VIDEO_DIP_ENABLE_VSC_HSW);
+	POSTING_READ(ctl_reg);
+}
+
+static void intel_edp_psr_setup(struct intel_dp *intel_dp)
+{
+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct edp_vsc_psr psr_vsc;
+
+	if (intel_dp->psr_setup_done)
+		return;
+
+	/* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */
+	memset(&psr_vsc, 0, sizeof(psr_vsc));
+	psr_vsc.sdp_header.HB0 = 0;
+	psr_vsc.sdp_header.HB1 = 0x7;
+	psr_vsc.sdp_header.HB2 = 0x2;
+	psr_vsc.sdp_header.HB3 = 0x8;
+	intel_edp_psr_write_vsc(intel_dp, &psr_vsc);
+
+	/* Avoid continuous PSR exit by masking memup and hpd */
+	I915_WRITE(EDP_PSR_DEBUG_CTL, EDP_PSR_DEBUG_MASK_MEMUP |
+		   EDP_PSR_DEBUG_MASK_HPD);
+
+	intel_dp->psr_setup_done = true;
+}
+
+static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
+{
+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t aux_clock_divider = get_aux_clock_divider(intel_dp);
+	int precharge = 0x3;
+	int msg_size = 5;       /* Header(4) + Message(1) */
+
+	/* Enable PSR in sink */
+	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT)
+		intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
+					    DP_PSR_ENABLE &
+					    ~DP_PSR_MAIN_LINK_ACTIVE);
+	else
+		intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
+					    DP_PSR_ENABLE |
+					    DP_PSR_MAIN_LINK_ACTIVE);
+
+	/* Setup AUX registers */
+	I915_WRITE(EDP_PSR_AUX_DATA1, EDP_PSR_DPCD_COMMAND);
+	I915_WRITE(EDP_PSR_AUX_DATA2, EDP_PSR_DPCD_NORMAL_OPERATION);
+	I915_WRITE(EDP_PSR_AUX_CTL,
+		   DP_AUX_CH_CTL_TIME_OUT_400us |
+		   (msg_size << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
+		   (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
+		   (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT));
+}
+
+static void intel_edp_psr_enable_source(struct intel_dp *intel_dp)
+{
+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t max_sleep_time = 0x1f;
+	uint32_t idle_frames = 1;
+	uint32_t val = 0x0;
+
+	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT) {
+		val |= EDP_PSR_LINK_STANDBY;
+		val |= EDP_PSR_TP2_TP3_TIME_0us;
+		val |= EDP_PSR_TP1_TIME_0us;
+		val |= EDP_PSR_SKIP_AUX_EXIT;
+	} else
+		val |= EDP_PSR_LINK_DISABLE;
+
+	I915_WRITE(EDP_PSR_CTL, val |
+		   EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES |
+		   max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT |
+		   idle_frames << EDP_PSR_IDLE_FRAME_SHIFT |
+		   EDP_PSR_ENABLE);
+}
+
+void intel_edp_psr_enable(struct intel_dp *intel_dp)
+{
+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+
+	if (!is_edp_psr(intel_dp) || intel_edp_is_psr_enabled(dev))
+		return;
+
+	/* Setup PSR once */
+	intel_edp_psr_setup(intel_dp);
+
+	/* Enable PSR on the panel */
+	intel_edp_psr_enable_sink(intel_dp);
+
+	/* Enable PSR on the host */
+	intel_edp_psr_enable_source(intel_dp);
+}
+
+void intel_edp_psr_disable(struct intel_dp *intel_dp)
+{
+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (!intel_edp_is_psr_enabled(dev))
+		return;
+
+	I915_WRITE(EDP_PSR_CTL, I915_READ(EDP_PSR_CTL) & ~EDP_PSR_ENABLE);
+
+	/* Wait till PSR is idle */
+	if (_wait_for((I915_READ(EDP_PSR_STATUS_CTL) &
+		       EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10))
+		DRM_ERROR("Timed out waiting for PSR Idle State\n");
+}
+
 static void intel_disable_dp(struct intel_encoder *encoder)
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
@@ -3194,6 +3341,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n",
 	     error, port_name(port));
 
+	intel_dp->psr_setup_done = false;
+
 	if (!intel_edp_init_connector(intel_dp, intel_connector)) {
 		i2c_del_adapter(&intel_dp->adapter);
 		if (is_edp(intel_dp)) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d25726d..ff36a40 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -499,6 +499,7 @@ struct intel_dp {
 	int backlight_off_delay;
 	struct delayed_work panel_vdd_work;
 	bool want_panel_vdd;
+	bool psr_setup_done;
 	struct intel_connector *attached_connector;
 };
 
@@ -834,4 +835,7 @@ extern bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
 						 enum transcoder pch_transcoder,
 						 bool enable);
 
+extern void intel_edp_psr_enable(struct intel_dp *intel_dp);
+extern void intel_edp_psr_disable(struct intel_dp *intel_dp);
+
 #endif /* __INTEL_DRV_H__ */
-- 
1.7.11.7

Re: [PATCH 04/11] drm/i915: Enable/Disable PSR

[Paulo Zanoni]

2013/7/11 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> Adding Enable and Disable PSR functionalities. This includes setting the
> PSR configuration over AUX, sending SDP VSC DIP over the eDP PIPE config,
> enabling PSR in the sink via DPCD register and finally enabling PSR on
> the host.
>
> This patch is based on initial PSR code by Sateesh Kavuri and Kumar Shobhit
> but in a different implementation.
>
> v2: * moved functions around and changed its names.
>     * removed VSC DIP unset from disable.
>     * remove FBC wa.
>     * don't mask LSPS anymore.
>     * incorporate new crtc usage after a rebase.
> v3: Make a clear separation between Sink (Panel) and Source (HW) enabling.
> v4: Fix identation and other style issues raised by checkpatch (by Paulo).
> v5: Changes according to Paulo's review:
>     static on write_vsc;
>     avoid using dp_to_dev when already calling dp_to_dig_port;
>     remove unecessary TP default time setting;
>     remove unecessary interrupts disabling;
>     remove unecessary wait_for_vblank when disabling psr;
> v6: remove unecessary wait_for_vblank when writing vsc;
> v7: adding setup once function to avoid unnecessarily write to vsc
>     and set debug_ctl every time we enable or disable psr.
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Credits-by: Sateesh Kavuri <sateesh.kavuri@intel.com>
> Credits-by: Shobhit Kumar <shobhit.kumar@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  42 +++++++++++
>  drivers/gpu/drm/i915/intel_dp.c  | 149 +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |   4 ++
>  3 files changed, 195 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index dc3d6a7..31e4dbb 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1779,6 +1779,47 @@
>  #define BCLRPAT(pipe) _PIPE(pipe, _BCLRPAT_A, _BCLRPAT_B)
>  #define VSYNCSHIFT(trans) _TRANSCODER(trans, _VSYNCSHIFT_A, _VSYNCSHIFT_B)
>
> +/* HSW eDP PSR registers */
> +#define EDP_PSR_CTL                            0x64800
> +#define   EDP_PSR_ENABLE                       (1<<31)
> +#define   EDP_PSR_LINK_DISABLE                 (0<<27)
> +#define   EDP_PSR_LINK_STANDBY                 (1<<27)
> +#define   EDP_PSR_MIN_LINK_ENTRY_TIME_MASK     (3<<25)
> +#define   EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES  (0<<25)
> +#define   EDP_PSR_MIN_LINK_ENTRY_TIME_4_LINES  (1<<25)
> +#define   EDP_PSR_MIN_LINK_ENTRY_TIME_2_LINES  (2<<25)
> +#define   EDP_PSR_MIN_LINK_ENTRY_TIME_0_LINES  (3<<25)
> +#define   EDP_PSR_MAX_SLEEP_TIME_SHIFT         20
> +#define   EDP_PSR_SKIP_AUX_EXIT                        (1<<12)
> +#define   EDP_PSR_TP1_TP2_SEL                  (0<<11)
> +#define   EDP_PSR_TP1_TP3_SEL                  (1<<11)
> +#define   EDP_PSR_TP2_TP3_TIME_500us           (0<<8)
> +#define   EDP_PSR_TP2_TP3_TIME_100us           (1<<8)
> +#define   EDP_PSR_TP2_TP3_TIME_2500us          (2<<8)
> +#define   EDP_PSR_TP2_TP3_TIME_0us             (3<<8)
> +#define   EDP_PSR_TP1_TIME_500us               (0<<4)
> +#define   EDP_PSR_TP1_TIME_100us               (1<<4)
> +#define   EDP_PSR_TP1_TIME_2500us              (2<<4)
> +#define   EDP_PSR_TP1_TIME_0us                 (3<<4)
> +#define   EDP_PSR_IDLE_FRAME_SHIFT             0
> +
> +#define EDP_PSR_AUX_CTL                        0x64810
> +#define EDP_PSR_AUX_DATA1              0x64814
> +#define   EDP_PSR_DPCD_COMMAND         0x80060000
> +#define EDP_PSR_AUX_DATA2              0x64818
> +#define   EDP_PSR_DPCD_NORMAL_OPERATION        (1<<24)
> +#define EDP_PSR_AUX_DATA3              0x6481c
> +#define EDP_PSR_AUX_DATA4              0x64820
> +#define EDP_PSR_AUX_DATA5              0x64824
> +
> +#define EDP_PSR_STATUS_CTL                     0x64840
> +#define   EDP_PSR_STATUS_STATE_MASK            (7<<29)
> +
> +#define EDP_PSR_DEBUG_CTL              0x64860
> +#define   EDP_PSR_DEBUG_MASK_LPSP      (1<<27)
> +#define   EDP_PSR_DEBUG_MASK_MEMUP     (1<<26)
> +#define   EDP_PSR_DEBUG_MASK_HPD       (1<<25)
> +
>  /* VGA port control */
>  #define ADPA                   0x61100
>  #define PCH_ADPA                0xe1100
> @@ -2048,6 +2089,7 @@
>   * (Haswell and newer) to see which VIDEO_DIP_DATA byte corresponds to each byte
>   * of the infoframe structure specified by CEA-861. */
>  #define   VIDEO_DIP_DATA_SIZE  32
> +#define   VIDEO_DIP_VSC_DATA_SIZE      36
>  #define VIDEO_DIP_CTL          0x61170
>  /* Pre HSW: */
>  #define   VIDEO_DIP_ENABLE             (1 << 31)
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index d273e36..d4b52a9 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1383,6 +1383,153 @@ static bool is_edp_psr(struct intel_dp *intel_dp)
>                 intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED;
>  }
>
> +static bool intel_edp_is_psr_enabled(struct drm_device *dev)
> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +       if (!IS_HASWELL(dev))
> +               return false;
> +
> +       return I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
> +}
> +
> +static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp,
> +                                   struct edp_vsc_psr *vsc_psr)
> +{
> +       struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +       struct drm_device *dev = dig_port->base.base.dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct intel_crtc *crtc = to_intel_crtc(dig_port->base.base.crtc);
> +       u32 ctl_reg = HSW_TVIDEO_DIP_CTL(crtc->config.cpu_transcoder);
> +       u32 data_reg = HSW_TVIDEO_DIP_VSC_DATA(crtc->config.cpu_transcoder);
> +       uint32_t *data = (uint32_t *) vsc_psr;
> +       unsigned int i;
> +
> +       /* As per BSPec (Pipe Video Data Island Packet), we need to disable
> +          the video DIP being updated before program video DIP data buffer
> +          registers for DIP being updated. */
> +       I915_WRITE(ctl_reg, ~VIDEO_DIP_ENABLE_VSC_HSW);

This should be zero.With that fixed:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>


> +       POSTING_READ(ctl_reg);
> +
> +       for (i = 0; i < VIDEO_DIP_VSC_DATA_SIZE; i += 4) {
> +               if (i < sizeof(struct edp_vsc_psr))
> +                       I915_WRITE(data_reg + i, *data++);
> +               else
> +                       I915_WRITE(data_reg + i, 0);
> +       }
> +
> +       I915_WRITE(ctl_reg, VIDEO_DIP_ENABLE_VSC_HSW);
> +       POSTING_READ(ctl_reg);
> +}
> +
> +static void intel_edp_psr_setup(struct intel_dp *intel_dp)
> +{
> +       struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct edp_vsc_psr psr_vsc;
> +
> +       if (intel_dp->psr_setup_done)
> +               return;
> +
> +       /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */
> +       memset(&psr_vsc, 0, sizeof(psr_vsc));
> +       psr_vsc.sdp_header.HB0 = 0;
> +       psr_vsc.sdp_header.HB1 = 0x7;
> +       psr_vsc.sdp_header.HB2 = 0x2;
> +       psr_vsc.sdp_header.HB3 = 0x8;
> +       intel_edp_psr_write_vsc(intel_dp, &psr_vsc);
> +
> +       /* Avoid continuous PSR exit by masking memup and hpd */
> +       I915_WRITE(EDP_PSR_DEBUG_CTL, EDP_PSR_DEBUG_MASK_MEMUP |
> +                  EDP_PSR_DEBUG_MASK_HPD);
> +
> +       intel_dp->psr_setup_done = true;
> +}
> +
> +static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
> +{
> +       struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       uint32_t aux_clock_divider = get_aux_clock_divider(intel_dp);
> +       int precharge = 0x3;
> +       int msg_size = 5;       /* Header(4) + Message(1) */
> +
> +       /* Enable PSR in sink */
> +       if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT)
> +               intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
> +                                           DP_PSR_ENABLE &
> +                                           ~DP_PSR_MAIN_LINK_ACTIVE);
> +       else
> +               intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
> +                                           DP_PSR_ENABLE |
> +                                           DP_PSR_MAIN_LINK_ACTIVE);
> +
> +       /* Setup AUX registers */
> +       I915_WRITE(EDP_PSR_AUX_DATA1, EDP_PSR_DPCD_COMMAND);
> +       I915_WRITE(EDP_PSR_AUX_DATA2, EDP_PSR_DPCD_NORMAL_OPERATION);
> +       I915_WRITE(EDP_PSR_AUX_CTL,
> +                  DP_AUX_CH_CTL_TIME_OUT_400us |
> +                  (msg_size << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
> +                  (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
> +                  (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT));
> +}
> +
> +static void intel_edp_psr_enable_source(struct intel_dp *intel_dp)
> +{
> +       struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       uint32_t max_sleep_time = 0x1f;
> +       uint32_t idle_frames = 1;
> +       uint32_t val = 0x0;
> +
> +       if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT) {
> +               val |= EDP_PSR_LINK_STANDBY;
> +               val |= EDP_PSR_TP2_TP3_TIME_0us;
> +               val |= EDP_PSR_TP1_TIME_0us;
> +               val |= EDP_PSR_SKIP_AUX_EXIT;
> +       } else
> +               val |= EDP_PSR_LINK_DISABLE;
> +
> +       I915_WRITE(EDP_PSR_CTL, val |
> +                  EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES |
> +                  max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT |
> +                  idle_frames << EDP_PSR_IDLE_FRAME_SHIFT |
> +                  EDP_PSR_ENABLE);
> +}
> +
> +void intel_edp_psr_enable(struct intel_dp *intel_dp)
> +{
> +       struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +
> +       if (!is_edp_psr(intel_dp) || intel_edp_is_psr_enabled(dev))
> +               return;
> +
> +       /* Setup PSR once */
> +       intel_edp_psr_setup(intel_dp);
> +
> +       /* Enable PSR on the panel */
> +       intel_edp_psr_enable_sink(intel_dp);
> +
> +       /* Enable PSR on the host */
> +       intel_edp_psr_enable_source(intel_dp);
> +}
> +
> +void intel_edp_psr_disable(struct intel_dp *intel_dp)
> +{
> +       struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +       if (!intel_edp_is_psr_enabled(dev))
> +               return;
> +
> +       I915_WRITE(EDP_PSR_CTL, I915_READ(EDP_PSR_CTL) & ~EDP_PSR_ENABLE);
> +
> +       /* Wait till PSR is idle */
> +       if (_wait_for((I915_READ(EDP_PSR_STATUS_CTL) &
> +                      EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10))
> +               DRM_ERROR("Timed out waiting for PSR Idle State\n");
> +}
> +
>  static void intel_disable_dp(struct intel_encoder *encoder)
>  {
>         struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> @@ -3194,6 +3341,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>         WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n",
>              error, port_name(port));
>
> +       intel_dp->psr_setup_done = false;
> +
>         if (!intel_edp_init_connector(intel_dp, intel_connector)) {
>                 i2c_del_adapter(&intel_dp->adapter);
>                 if (is_edp(intel_dp)) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d25726d..ff36a40 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -499,6 +499,7 @@ struct intel_dp {
>         int backlight_off_delay;
>         struct delayed_work panel_vdd_work;
>         bool want_panel_vdd;
> +       bool psr_setup_done;
>         struct intel_connector *attached_connector;
>  };
>
> @@ -834,4 +835,7 @@ extern bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
>                                                  enum transcoder pch_transcoder,
>                                                  bool enable);
>
> +extern void intel_edp_psr_enable(struct intel_dp *intel_dp);
> +extern void intel_edp_psr_disable(struct intel_dp *intel_dp);
> +
>  #endif /* __INTEL_DRV_H__ */
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 04/11] drm/i915: Enable/Disable PSR

[Daniel Vetter]

On Wed, Jul 17, 2013 at 02:02:58PM -0300, Paulo Zanoni wrote:
> 2013/7/11 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> > +static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp,
> > +                                   struct edp_vsc_psr *vsc_psr)
> > +{
> > +       struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > +       struct drm_device *dev = dig_port->base.base.dev;
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       struct intel_crtc *crtc = to_intel_crtc(dig_port->base.base.crtc);
> > +       u32 ctl_reg = HSW_TVIDEO_DIP_CTL(crtc->config.cpu_transcoder);
> > +       u32 data_reg = HSW_TVIDEO_DIP_VSC_DATA(crtc->config.cpu_transcoder);
> > +       uint32_t *data = (uint32_t *) vsc_psr;
> > +       unsigned int i;
> > +
> > +       /* As per BSPec (Pipe Video Data Island Packet), we need to disable
> > +          the video DIP being updated before program video DIP data buffer
> > +          registers for DIP being updated. */
> > +       I915_WRITE(ctl_reg, ~VIDEO_DIP_ENABLE_VSC_HSW);
> 
> This should be zero.With that fixed:

Fixed while applying.

> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Bikeshed: We now have two pieces of code writing DIPs, the other copy is
in intel_hdmi.c. And they don't match.

Slightly related, but: I'd really like to see our conversion to the common
infoframe helpers rsn ...

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 05/11] drm/i915: Added debugfs support for PSR Status

[Rodrigo Vivi]

Adding support for PSR Status, PSR entry counter and performance counters.
Heavily based on initial work from Shobhit.

v2: Fix PSR Status Link bits by Paulo Zanoni.
v3: Prefer seq_puts to seq_printf by Paulo Zanoni.
v4: Fix identation by Paulo Zanoni.
v5: Return earlier if it isn't Haswell in order to avoid reading non-existing
    registers - by Paulo Zanoni.

CC: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Credits-by: Shobhit Kumar <shobhit.kumar@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 95 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h     | 24 ++++++++++
 2 files changed, 119 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index d413812..fe3cd5a 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1943,6 +1943,100 @@ static int i915_dpio_info(struct seq_file *m, void *data)
 	return 0;
 }
 
+static int i915_edp_psr_status(struct seq_file *m, void *data)
+{
+	struct drm_info_node *node = m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 psrctl, psrstat, psrperf;
+
+	if (!IS_HASWELL(dev)) {
+		seq_puts(m, "PSR not supported on this platform\n");
+		return 0;
+	}
+
+	psrctl = I915_READ(EDP_PSR_CTL);
+	seq_printf(m, "PSR Enabled: %s\n",
+		   yesno(psrctl & EDP_PSR_ENABLE));
+
+	psrstat = I915_READ(EDP_PSR_STATUS_CTL);
+
+	seq_puts(m, "PSR Current State: ");
+	switch (psrstat & EDP_PSR_STATUS_STATE_MASK) {
+	case EDP_PSR_STATUS_STATE_IDLE:
+		seq_puts(m, "Reset state\n");
+		break;
+	case EDP_PSR_STATUS_STATE_SRDONACK:
+		seq_puts(m, "Wait for TG/Stream to send on frame of data after SRD conditions are met\n");
+		break;
+	case EDP_PSR_STATUS_STATE_SRDENT:
+		seq_puts(m, "SRD entry\n");
+		break;
+	case EDP_PSR_STATUS_STATE_BUFOFF:
+		seq_puts(m, "Wait for buffer turn off\n");
+		break;
+	case EDP_PSR_STATUS_STATE_BUFON:
+		seq_puts(m, "Wait for buffer turn on\n");
+		break;
+	case EDP_PSR_STATUS_STATE_AUXACK:
+		seq_puts(m, "Wait for AUX to acknowledge on SRD exit\n");
+		break;
+	case EDP_PSR_STATUS_STATE_SRDOFFACK:
+		seq_puts(m, "Wait for TG/Stream to acknowledge the SRD VDM exit\n");
+		break;
+	default:
+		seq_puts(m, "Unknown\n");
+		break;
+	}
+
+	seq_puts(m, "Link Status: ");
+	switch (psrstat & EDP_PSR_STATUS_LINK_MASK) {
+	case EDP_PSR_STATUS_LINK_FULL_OFF:
+		seq_puts(m, "Link is fully off\n");
+		break;
+	case EDP_PSR_STATUS_LINK_FULL_ON:
+		seq_puts(m, "Link is fully on\n");
+		break;
+	case EDP_PSR_STATUS_LINK_STANDBY:
+		seq_puts(m, "Link is in standby\n");
+		break;
+	default:
+		seq_puts(m, "Unknown\n");
+		break;
+	}
+
+	seq_printf(m, "PSR Entry Count: %u\n",
+		   psrstat >> EDP_PSR_STATUS_COUNT_SHIFT &
+		   EDP_PSR_STATUS_COUNT_MASK);
+
+	seq_printf(m, "Max Sleep Timer Counter: %u\n",
+		   psrstat >> EDP_PSR_STATUS_MAX_SLEEP_TIMER_SHIFT &
+		   EDP_PSR_STATUS_MAX_SLEEP_TIMER_MASK);
+
+	seq_printf(m, "Had AUX error: %s\n",
+		   yesno(psrstat & EDP_PSR_STATUS_AUX_ERROR));
+
+	seq_printf(m, "Sending AUX: %s\n",
+		   yesno(psrstat & EDP_PSR_STATUS_AUX_SENDING));
+
+	seq_printf(m, "Sending Idle: %s\n",
+		   yesno(psrstat & EDP_PSR_STATUS_SENDING_IDLE));
+
+	seq_printf(m, "Sending TP2 TP3: %s\n",
+		   yesno(psrstat & EDP_PSR_STATUS_SENDING_TP2_TP3));
+
+	seq_printf(m, "Sending TP1: %s\n",
+		   yesno(psrstat & EDP_PSR_STATUS_SENDING_TP1));
+
+	seq_printf(m, "Idle Count: %u\n",
+		   psrstat & EDP_PSR_STATUS_IDLE_MASK);
+
+	psrperf = (I915_READ(EDP_PSR_PERF_CNT)) & EDP_PSR_PERF_CNT_MASK;
+	seq_printf(m, "Performance Counter: %u\n", psrperf);
+
+	return 0;
+}
+
 static int
 i915_wedged_get(void *data, u64 *val)
 {
@@ -2372,6 +2466,7 @@ static struct drm_info_list i915_debugfs_list[] = {
 	{"i915_swizzle_info", i915_swizzle_info, 0},
 	{"i915_ppgtt_info", i915_ppgtt_info, 0},
 	{"i915_dpio", i915_dpio_info, 0},
+	{"i915_edp_psr_status", i915_edp_psr_status, 0},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 31e4dbb..b328ec6 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1814,6 +1814,30 @@
 
 #define EDP_PSR_STATUS_CTL			0x64840
 #define   EDP_PSR_STATUS_STATE_MASK		(7<<29)
+#define   EDP_PSR_STATUS_STATE_IDLE		(0<<29)
+#define   EDP_PSR_STATUS_STATE_SRDONACK		(1<<29)
+#define   EDP_PSR_STATUS_STATE_SRDENT		(2<<29)
+#define   EDP_PSR_STATUS_STATE_BUFOFF		(3<<29)
+#define   EDP_PSR_STATUS_STATE_BUFON		(4<<29)
+#define   EDP_PSR_STATUS_STATE_AUXACK		(5<<29)
+#define   EDP_PSR_STATUS_STATE_SRDOFFACK	(6<<29)
+#define   EDP_PSR_STATUS_LINK_MASK		(3<<26)
+#define   EDP_PSR_STATUS_LINK_FULL_OFF		(0<<26)
+#define   EDP_PSR_STATUS_LINK_FULL_ON		(1<<26)
+#define   EDP_PSR_STATUS_LINK_STANDBY		(2<<26)
+#define   EDP_PSR_STATUS_MAX_SLEEP_TIMER_SHIFT	20
+#define   EDP_PSR_STATUS_MAX_SLEEP_TIMER_MASK	0x1f
+#define   EDP_PSR_STATUS_COUNT_SHIFT		16
+#define   EDP_PSR_STATUS_COUNT_MASK		0xf
+#define   EDP_PSR_STATUS_AUX_ERROR		(1<<15)
+#define   EDP_PSR_STATUS_AUX_SENDING		(1<<12)
+#define   EDP_PSR_STATUS_SENDING_IDLE		(1<<9)
+#define   EDP_PSR_STATUS_SENDING_TP2_TP3	(1<<8)
+#define   EDP_PSR_STATUS_SENDING_TP1		(1<<4)
+#define   EDP_PSR_STATUS_IDLE_MASK		0xf
+
+#define EDP_PSR_PERF_CNT		0x64844
+#define   EDP_PSR_PERF_CNT_MASK		0xffffff
 
 #define EDP_PSR_DEBUG_CTL		0x64860
 #define   EDP_PSR_DEBUG_MASK_LPSP	(1<<27)
-- 
1.7.11.7

Re: [PATCH 05/11] drm/i915: Added debugfs support for PSR Status

[Chris Wilson]

On Thu, Jul 11, 2013 at 06:44:59PM -0300, Rodrigo Vivi wrote:
> Adding support for PSR Status, PSR entry counter and performance counters.
> Heavily based on initial work from Shobhit.
> 
> v2: Fix PSR Status Link bits by Paulo Zanoni.
> v3: Prefer seq_puts to seq_printf by Paulo Zanoni.
> v4: Fix identation by Paulo Zanoni.
> v5: Return earlier if it isn't Haswell in order to avoid reading non-existing
>     registers - by Paulo Zanoni.
> 
> CC: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Credits-by: Shobhit Kumar <shobhit.kumar@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 95 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h     | 24 ++++++++++
>  2 files changed, 119 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index d413812..fe3cd5a 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1943,6 +1943,100 @@ static int i915_dpio_info(struct seq_file *m, void *data)
>  	return 0;
>  }
>  
> +static int i915_edp_psr_status(struct seq_file *m, void *data)
> +{
> +	struct drm_info_node *node = m->private;
> +	struct drm_device *dev = node->minor->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 psrctl, psrstat, psrperf;
> +
> +	if (!IS_HASWELL(dev)) {

Introduce a HAS_PSR(dev)

> +		seq_puts(m, "PSR not supported on this platform\n");
> +		return 0;
> +	}
> +
> +	psrctl = I915_READ(EDP_PSR_CTL);

Missing locking.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 05/11] drm/i915: Added debugfs support for PSR Status

[Rodrigo Vivi]

On Mon, Jul 15, 2013 at 11:03 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, Jul 11, 2013 at 06:44:59PM -0300, Rodrigo Vivi wrote:
>> Adding support for PSR Status, PSR entry counter and performance counters.
>> Heavily based on initial work from Shobhit.
>>
>> v2: Fix PSR Status Link bits by Paulo Zanoni.
>> v3: Prefer seq_puts to seq_printf by Paulo Zanoni.
>> v4: Fix identation by Paulo Zanoni.
>> v5: Return earlier if it isn't Haswell in order to avoid reading non-existing
>>     registers - by Paulo Zanoni.
>>
>> CC: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> Credits-by: Shobhit Kumar <shobhit.kumar@intel.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c | 95 +++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/i915_reg.h     | 24 ++++++++++
>>  2 files changed, 119 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index d413812..fe3cd5a 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1943,6 +1943,100 @@ static int i915_dpio_info(struct seq_file *m, void *data)
>>       return 0;
>>  }
>>
>> +static int i915_edp_psr_status(struct seq_file *m, void *data)
>> +{
>> +     struct drm_info_node *node = m->private;
>> +     struct drm_device *dev = node->minor->dev;
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +     u32 psrctl, psrstat, psrperf;
>> +
>> +     if (!IS_HASWELL(dev)) {
>
> Introduce a HAS_PSR(dev)

I prefer to stay with HSW check for now because as far as I can
remember the registers on another platforms are different.
After this series is accepted I'm going to integrate psr support for
baytrail and I come up with a more elegant check.

>
>> +             seq_puts(m, "PSR not supported on this platform\n");
>> +             return 0;
>> +     }
>> +
>> +     psrctl = I915_READ(EDP_PSR_CTL);
>
> Missing locking.

what lock? that psr exit is on debug_ctl

> -Chris

Thanks,
Rodrigo.
>
> --
> Chris Wilson, Intel Open Source Technology Centre



--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Re: [PATCH 05/11] drm/i915: Added debugfs support for PSR Status

[Chris Wilson]

On Mon, Jul 15, 2013 at 05:13:27PM -0300, Rodrigo Vivi wrote:
> On Mon, Jul 15, 2013 at 11:03 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >> +             seq_puts(m, "PSR not supported on this platform\n");
> >> +             return 0;
> >> +     }
> >> +
> >> +     psrctl = I915_READ(EDP_PSR_CTL);
> >
> > Missing locking.
> 
> what lock? that psr exit is on debug_ctl

I wanted to serialise the read of EDP_PSR_CTL against the write after a
modeset. It should not be strictly necessary in this debug context.

(However, we now have very strict rules that imply concurrent access to
registers (within the same cacheline, even) is verboten.)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 06/11] drm/i915: Match all PSR mode entry conditions before enabling it.

[Rodrigo Vivi]

v2: Prefer seq_puts to seq_printf by Paulo Zanoni.
v3: small changes like avoiding calling dp_to_dig_port twice as noticed by
    Paulo Zanoni.
v4: Avoiding reading non-existent registers - noticed by Paulo
    on first psr debugfs patch.
v5: Accepting more suggestions from Paulo:
    * check sw interlace flag instead of i915_read
    * introduce PSR_S3D_ENABLED to avoid forgeting it whenever added.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 44 ++++++++++++++++++----
 drivers/gpu/drm/i915/i915_drv.h     | 13 +++++++
 drivers/gpu/drm/i915/i915_reg.h     |  7 ++++
 drivers/gpu/drm/i915/intel_dp.c     | 74 ++++++++++++++++++++++++++++++++++++-
 4 files changed, 130 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index fe3cd5a..e679968 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1948,17 +1948,47 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 	struct drm_info_node *node = m->private;
 	struct drm_device *dev = node->minor->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 psrctl, psrstat, psrperf;
+	u32 psrstat, psrperf;
 
-	if (!IS_HASWELL(dev)) {
-		seq_puts(m, "PSR not supported on this platform\n");
+	if (IS_HASWELL(dev) && I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE) {
+		seq_puts(m, "PSR enabled\n");
+	} else {
+		seq_puts(m, "PSR disabled: ");
+		switch (dev_priv->no_psr_reason) {
+		case PSR_NO_SOURCE:
+			seq_puts(m, "not supported on this platform");
+			break;
+		case PSR_NO_SINK:
+			seq_puts(m, "not supported by panel");
+			break;
+		case PSR_CRTC_NOT_ACTIVE:
+			seq_puts(m, "crtc not active");
+			break;
+		case PSR_PWR_WELL_ENABLED:
+			seq_puts(m, "power well enabled");
+			break;
+		case PSR_NOT_TILED:
+			seq_puts(m, "not tiled");
+			break;
+		case PSR_SPRITE_ENABLED:
+			seq_puts(m, "sprite enabled");
+			break;
+		case PSR_S3D_ENABLED:
+			seq_puts(m, "stereo 3d enabled");
+			break;
+		case PSR_INTERLACED_ENABLED:
+			seq_puts(m, "interlaced enabled");
+			break;
+		case PSR_HSW_NOT_DDIA:
+			seq_puts(m, "HSW ties PSR to DDI A (eDP)");
+			break;
+		default:
+			seq_puts(m, "unknown reason");
+		}
+		seq_puts(m, "\n");
 		return 0;
 	}
 
-	psrctl = I915_READ(EDP_PSR_CTL);
-	seq_printf(m, "PSR Enabled: %s\n",
-		   yesno(psrctl & EDP_PSR_ENABLE));
-
 	psrstat = I915_READ(EDP_PSR_STATUS_CTL);
 
 	seq_puts(m, "PSR Current State: ");
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 842aada..d0b9483 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -560,6 +560,17 @@ struct i915_fbc {
 	} no_fbc_reason;
 };
 
+enum no_psr_reason {
+	PSR_NO_SOURCE, /* Not supported on platform */
+	PSR_NO_SINK, /* Not supported by panel */
+	PSR_CRTC_NOT_ACTIVE,
+	PSR_PWR_WELL_ENABLED,
+	PSR_NOT_TILED,
+	PSR_SPRITE_ENABLED,
+	PSR_S3D_ENABLED,
+	PSR_INTERLACED_ENABLED,
+	PSR_HSW_NOT_DDIA,
+};
 
 enum intel_pch {
 	PCH_NONE = 0,	/* No PCH present */
@@ -1161,6 +1172,8 @@ typedef struct drm_i915_private {
 	/* Haswell power well */
 	struct i915_power_well power_well;
 
+	enum no_psr_reason no_psr_reason;
+
 	struct i915_gpu_error gpu_error;
 
 	struct drm_i915_gem_object *vlv_pctx;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b328ec6..3bca337 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4150,6 +4150,13 @@
 #define HSW_TVIDEO_DIP_VSC_DATA(trans) \
 	 _TRANSCODER(trans, HSW_VIDEO_DIP_VSC_DATA_A, HSW_VIDEO_DIP_VSC_DATA_B)
 
+#define HSW_STEREO_3D_CTL_A	0x70020
+#define   S3D_ENABLE		(1<<31)
+#define HSW_STEREO_3D_CTL_B	0x71020
+
+#define HSW_STEREO_3D_CTL(trans) \
+	_TRANSCODER(trans, HSW_STEREO_3D_CTL_A, HSW_STEREO_3D_CTL_A)
+
 #define _PCH_TRANS_HTOTAL_B          0xe1000
 #define _PCH_TRANS_HBLANK_B          0xe1004
 #define _PCH_TRANS_HSYNC_B           0xe1008
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d4b52a9..c0bd887 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1497,11 +1497,83 @@ static void intel_edp_psr_enable_source(struct intel_dp *intel_dp)
 		   EDP_PSR_ENABLE);
 }
 
+static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc = dig_port->base.base.crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_i915_gem_object *obj = to_intel_framebuffer(crtc->fb)->obj;
+	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
+
+	if (!IS_HASWELL(dev)) {
+		DRM_DEBUG_KMS("PSR not supported on this platform\n");
+		dev_priv->no_psr_reason = PSR_NO_SOURCE;
+		return false;
+	}
+
+	if ((intel_encoder->type != INTEL_OUTPUT_EDP) ||
+	    (dig_port->port != PORT_A)) {
+		DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n");
+		dev_priv->no_psr_reason = PSR_HSW_NOT_DDIA;
+		return false;
+	}
+
+	if (!is_edp_psr(intel_dp)) {
+		DRM_DEBUG_KMS("PSR not supported by this panel\n");
+		dev_priv->no_psr_reason = PSR_NO_SINK;
+		return false;
+	}
+
+	if (!intel_crtc->active || !crtc->fb || !crtc->mode.clock) {
+		DRM_DEBUG_KMS("crtc not active for PSR\n");
+		dev_priv->no_psr_reason = PSR_CRTC_NOT_ACTIVE;
+		return false;
+	}
+
+	if ((I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE) ||
+	    (I915_READ(HSW_PWR_WELL_KVMR) & HSW_PWR_WELL_ENABLE)) {
+		DRM_DEBUG_KMS("PSR condition failed: Power Well is Enabled\n");
+		dev_priv->no_psr_reason = PSR_PWR_WELL_ENABLED;
+		return false;
+	}
+
+	if (obj->tiling_mode != I915_TILING_X ||
+	    obj->fence_reg == I915_FENCE_REG_NONE) {
+		DRM_DEBUG_KMS("PSR condition failed: fb not tiled or fenced\n");
+		dev_priv->no_psr_reason = PSR_NOT_TILED;
+		return false;
+	}
+
+	if (I915_READ(SPRCTL(intel_crtc->pipe)) & SPRITE_ENABLE) {
+		DRM_DEBUG_KMS("PSR condition failed: Sprite is Enabled\n");
+		dev_priv->no_psr_reason = PSR_SPRITE_ENABLED;
+		return false;
+	}
+
+	if (I915_READ(HSW_STEREO_3D_CTL(intel_crtc->config.cpu_transcoder)) &
+	    S3D_ENABLE) {
+		DRM_DEBUG_KMS("PSR condition failed: Stereo 3D is Enabled\n");
+		dev_priv->no_psr_reason = PSR_S3D_ENABLED;
+		return false;
+	}
+
+	if (crtc->mode.flags & DRM_MODE_FLAG_INTERLACE) {
+		DRM_DEBUG_KMS("PSR condition failed: Interlaced is Enabled\n");
+		dev_priv->no_psr_reason = PSR_INTERLACED_ENABLED;
+		return false;
+	}
+
+	return true;
+}
+
 void intel_edp_psr_enable(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 
-	if (!is_edp_psr(intel_dp) || intel_edp_is_psr_enabled(dev))
+	if (!intel_edp_psr_match_conditions(intel_dp) ||
+	    intel_edp_is_psr_enabled(dev))
 		return;
 
 	/* Setup PSR once */
-- 
1.7.11.7

Re: [PATCH 06/11] drm/i915: Match all PSR mode entry conditions before enabling it.

[Chris Wilson]

On Thu, Jul 11, 2013 at 06:45:00PM -0300, Rodrigo Vivi wrote:
> v2: Prefer seq_puts to seq_printf by Paulo Zanoni.
> v3: small changes like avoiding calling dp_to_dig_port twice as noticed by
>     Paulo Zanoni.
> v4: Avoiding reading non-existent registers - noticed by Paulo
>     on first psr debugfs patch.
> v5: Accepting more suggestions from Paulo:
>     * check sw interlace flag instead of i915_read
>     * introduce PSR_S3D_ENABLED to avoid forgeting it whenever added.
> 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 44 ++++++++++++++++++----
>  drivers/gpu/drm/i915/i915_drv.h     | 13 +++++++
>  drivers/gpu/drm/i915/i915_reg.h     |  7 ++++
>  drivers/gpu/drm/i915/intel_dp.c     | 74 ++++++++++++++++++++++++++++++++++++-
>  4 files changed, 130 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index fe3cd5a..e679968 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1948,17 +1948,47 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  	struct drm_info_node *node = m->private;
>  	struct drm_device *dev = node->minor->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	u32 psrctl, psrstat, psrperf;
> +	u32 psrstat, psrperf;
>  
> -	if (!IS_HASWELL(dev)) {
> -		seq_puts(m, "PSR not supported on this platform\n");
> +	if (IS_HASWELL(dev) && I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE) {
> +		seq_puts(m, "PSR enabled\n");
> +	} else {
> +		seq_puts(m, "PSR disabled: ");
> +		switch (dev_priv->no_psr_reason) {
> +		case PSR_NO_SOURCE:

I am not a fan of using a continually expanding set of enums for
no_psr_reason (and no_fbc_reason). If we just stored a const char
*no_psr_reason, it would make like much easier for us (fewer lines and a
smaller object).

> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index d4b52a9..c0bd887 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1497,11 +1497,83 @@ static void intel_edp_psr_enable_source(struct intel_dp *intel_dp)
>  		   EDP_PSR_ENABLE);
>  }
>  
> +static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = dig_port->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_crtc *crtc = dig_port->base.base.crtc;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_i915_gem_object *obj = to_intel_framebuffer(crtc->fb)->obj;
> +	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> +
> +	if (!IS_HASWELL(dev)) {
> +		DRM_DEBUG_KMS("PSR not supported on this platform\n");
> +		dev_priv->no_psr_reason = PSR_NO_SOURCE;
> +		return false;
> +	}
> +
> +	if ((intel_encoder->type != INTEL_OUTPUT_EDP) ||
> +	    (dig_port->port != PORT_A)) {
> +		DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n");
> +		dev_priv->no_psr_reason = PSR_HSW_NOT_DDIA;
> +		return false;
> +	}
> +
> +	if (!is_edp_psr(intel_dp)) {
> +		DRM_DEBUG_KMS("PSR not supported by this panel\n");
> +		dev_priv->no_psr_reason = PSR_NO_SINK;
> +		return false;
> +	}
> +
> +	if (!intel_crtc->active || !crtc->fb || !crtc->mode.clock) {
> +		DRM_DEBUG_KMS("crtc not active for PSR\n");
> +		dev_priv->no_psr_reason = PSR_CRTC_NOT_ACTIVE;
> +		return false;
> +	}
> +
> +	if ((I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE) ||
> +	    (I915_READ(HSW_PWR_WELL_KVMR) & HSW_PWR_WELL_ENABLE)) {

Any time we resort to reading back register state is a failure in our
state tracking (and pipe_config).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 06/11] drm/i915: Match all PSR mode entry conditions before enabling it.

[Daniel Vetter]

On Mon, Jul 15, 2013 at 03:06:22PM +0100, Chris Wilson wrote:
> On Thu, Jul 11, 2013 at 06:45:00PM -0300, Rodrigo Vivi wrote:
> > v2: Prefer seq_puts to seq_printf by Paulo Zanoni.
> > v3: small changes like avoiding calling dp_to_dig_port twice as noticed by
> >     Paulo Zanoni.
> > v4: Avoiding reading non-existent registers - noticed by Paulo
> >     on first psr debugfs patch.
> > v5: Accepting more suggestions from Paulo:
> >     * check sw interlace flag instead of i915_read
> >     * introduce PSR_S3D_ENABLED to avoid forgeting it whenever added.
> > 
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 44 ++++++++++++++++++----
> >  drivers/gpu/drm/i915/i915_drv.h     | 13 +++++++
> >  drivers/gpu/drm/i915/i915_reg.h     |  7 ++++
> >  drivers/gpu/drm/i915/intel_dp.c     | 74 ++++++++++++++++++++++++++++++++++++-
> >  4 files changed, 130 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index fe3cd5a..e679968 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -1948,17 +1948,47 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> >  	struct drm_info_node *node = m->private;
> >  	struct drm_device *dev = node->minor->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	u32 psrctl, psrstat, psrperf;
> > +	u32 psrstat, psrperf;
> >  
> > -	if (!IS_HASWELL(dev)) {
> > -		seq_puts(m, "PSR not supported on this platform\n");
> > +	if (IS_HASWELL(dev) && I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE) {
> > +		seq_puts(m, "PSR enabled\n");
> > +	} else {
> > +		seq_puts(m, "PSR disabled: ");
> > +		switch (dev_priv->no_psr_reason) {
> > +		case PSR_NO_SOURCE:
> 
> I am not a fan of using a continually expanding set of enums for
> no_psr_reason (and no_fbc_reason). If we just stored a const char
> *no_psr_reason, it would make like much easier for us (fewer lines and a
> smaller object).
> 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index d4b52a9..c0bd887 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1497,11 +1497,83 @@ static void intel_edp_psr_enable_source(struct intel_dp *intel_dp)
> >  		   EDP_PSR_ENABLE);
> >  }
> >  
> > +static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > +	struct drm_device *dev = dig_port->base.base.dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct drm_crtc *crtc = dig_port->base.base.crtc;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	struct drm_i915_gem_object *obj = to_intel_framebuffer(crtc->fb)->obj;
> > +	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> > +
> > +	if (!IS_HASWELL(dev)) {
> > +		DRM_DEBUG_KMS("PSR not supported on this platform\n");
> > +		dev_priv->no_psr_reason = PSR_NO_SOURCE;
> > +		return false;
> > +	}
> > +
> > +	if ((intel_encoder->type != INTEL_OUTPUT_EDP) ||
> > +	    (dig_port->port != PORT_A)) {
> > +		DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n");
> > +		dev_priv->no_psr_reason = PSR_HSW_NOT_DDIA;
> > +		return false;
> > +	}
> > +
> > +	if (!is_edp_psr(intel_dp)) {
> > +		DRM_DEBUG_KMS("PSR not supported by this panel\n");
> > +		dev_priv->no_psr_reason = PSR_NO_SINK;
> > +		return false;
> > +	}
> > +
> > +	if (!intel_crtc->active || !crtc->fb || !crtc->mode.clock) {
> > +		DRM_DEBUG_KMS("crtc not active for PSR\n");
> > +		dev_priv->no_psr_reason = PSR_CRTC_NOT_ACTIVE;
> > +		return false;
> > +	}
> > +
> > +	if ((I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE) ||
> > +	    (I915_READ(HSW_PWR_WELL_KVMR) & HSW_PWR_WELL_ENABLE)) {
> 
> Any time we resort to reading back register state is a failure in our
> state tracking (and pipe_config).

Highly agreed, especially if it's a resource which is out of our control
like the KVMR bits. Since that's just plain broken I've removed it from
the patch.
-Daniel

> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 06/11] drm/i915: Match all PSR mode entry conditions before enabling it.

[Rodrigo Vivi]

On Thu, Jul 18, 2013 at 5:02 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Jul 15, 2013 at 03:06:22PM +0100, Chris Wilson wrote:
>> On Thu, Jul 11, 2013 at 06:45:00PM -0300, Rodrigo Vivi wrote:
>> > v2: Prefer seq_puts to seq_printf by Paulo Zanoni.
>> > v3: small changes like avoiding calling dp_to_dig_port twice as noticed by
>> >     Paulo Zanoni.
>> > v4: Avoiding reading non-existent registers - noticed by Paulo
>> >     on first psr debugfs patch.
>> > v5: Accepting more suggestions from Paulo:
>> >     * check sw interlace flag instead of i915_read
>> >     * introduce PSR_S3D_ENABLED to avoid forgeting it whenever added.
>> >
>> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_debugfs.c | 44 ++++++++++++++++++----
>> >  drivers/gpu/drm/i915/i915_drv.h     | 13 +++++++
>> >  drivers/gpu/drm/i915/i915_reg.h     |  7 ++++
>> >  drivers/gpu/drm/i915/intel_dp.c     | 74 ++++++++++++++++++++++++++++++++++++-
>> >  4 files changed, 130 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> > index fe3cd5a..e679968 100644
>> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> > @@ -1948,17 +1948,47 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>> >     struct drm_info_node *node = m->private;
>> >     struct drm_device *dev = node->minor->dev;
>> >     struct drm_i915_private *dev_priv = dev->dev_private;
>> > -   u32 psrctl, psrstat, psrperf;
>> > +   u32 psrstat, psrperf;
>> >
>> > -   if (!IS_HASWELL(dev)) {
>> > -           seq_puts(m, "PSR not supported on this platform\n");
>> > +   if (IS_HASWELL(dev) && I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE) {
>> > +           seq_puts(m, "PSR enabled\n");
>> > +   } else {
>> > +           seq_puts(m, "PSR disabled: ");
>> > +           switch (dev_priv->no_psr_reason) {
>> > +           case PSR_NO_SOURCE:
>>
>> I am not a fan of using a continually expanding set of enums for
>> no_psr_reason (and no_fbc_reason). If we just stored a const char
>> *no_psr_reason, it would make like much easier for us (fewer lines and a
>> smaller object).
>>
>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> > index d4b52a9..c0bd887 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > @@ -1497,11 +1497,83 @@ static void intel_edp_psr_enable_source(struct intel_dp *intel_dp)
>> >                EDP_PSR_ENABLE);
>> >  }
>> >
>> > +static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
>> > +{
>> > +   struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> > +   struct drm_device *dev = dig_port->base.base.dev;
>> > +   struct drm_i915_private *dev_priv = dev->dev_private;
>> > +   struct drm_crtc *crtc = dig_port->base.base.crtc;
>> > +   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> > +   struct drm_i915_gem_object *obj = to_intel_framebuffer(crtc->fb)->obj;
>> > +   struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>> > +
>> > +   if (!IS_HASWELL(dev)) {
>> > +           DRM_DEBUG_KMS("PSR not supported on this platform\n");
>> > +           dev_priv->no_psr_reason = PSR_NO_SOURCE;
>> > +           return false;
>> > +   }
>> > +
>> > +   if ((intel_encoder->type != INTEL_OUTPUT_EDP) ||
>> > +       (dig_port->port != PORT_A)) {
>> > +           DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n");
>> > +           dev_priv->no_psr_reason = PSR_HSW_NOT_DDIA;
>> > +           return false;
>> > +   }
>> > +
>> > +   if (!is_edp_psr(intel_dp)) {
>> > +           DRM_DEBUG_KMS("PSR not supported by this panel\n");
>> > +           dev_priv->no_psr_reason = PSR_NO_SINK;
>> > +           return false;
>> > +   }
>> > +
>> > +   if (!intel_crtc->active || !crtc->fb || !crtc->mode.clock) {
>> > +           DRM_DEBUG_KMS("crtc not active for PSR\n");
>> > +           dev_priv->no_psr_reason = PSR_CRTC_NOT_ACTIVE;
>> > +           return false;
>> > +   }
>> > +
>> > +   if ((I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE) ||
>> > +       (I915_READ(HSW_PWR_WELL_KVMR) & HSW_PWR_WELL_ENABLE)) {
>>
>> Any time we resort to reading back register state is a failure in our
>> state tracking (and pipe_config).
>
> Highly agreed, especially if it's a resource which is out of our control
> like the KVMR bits. Since that's just plain broken I've removed it from
> the patch.

Since Audio guys will enable power well and let it enabled all the
time, I'm wondering in mask LPSP at PSR_DEBUG_CTL.
So PSR can work even with power well enabled.
What do you think?

> -Daniel
>
>> -Chris
>>
>> --
>> Chris Wilson, Intel Open Source Technology Centre
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Re: [PATCH 06/11] drm/i915: Match all PSR mode entry conditions before enabling it.

[Daniel Vetter]

On Thu, Jul 18, 2013 at 6:36 PM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> On Thu, Jul 18, 2013 at 5:02 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Mon, Jul 15, 2013 at 03:06:22PM +0100, Chris Wilson wrote:
>>> On Thu, Jul 11, 2013 at 06:45:00PM -0300, Rodrigo Vivi wrote:
>>> > v2: Prefer seq_puts to seq_printf by Paulo Zanoni.
>>> > v3: small changes like avoiding calling dp_to_dig_port twice as noticed by
>>> >     Paulo Zanoni.
>>> > v4: Avoiding reading non-existent registers - noticed by Paulo
>>> >     on first psr debugfs patch.
>>> > v5: Accepting more suggestions from Paulo:
>>> >     * check sw interlace flag instead of i915_read
>>> >     * introduce PSR_S3D_ENABLED to avoid forgeting it whenever added.
>>> >
>>> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>>> > ---
>>> >  drivers/gpu/drm/i915/i915_debugfs.c | 44 ++++++++++++++++++----
>>> >  drivers/gpu/drm/i915/i915_drv.h     | 13 +++++++
>>> >  drivers/gpu/drm/i915/i915_reg.h     |  7 ++++
>>> >  drivers/gpu/drm/i915/intel_dp.c     | 74 ++++++++++++++++++++++++++++++++++++-
>>> >  4 files changed, 130 insertions(+), 8 deletions(-)
>>> >
>>> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>>> > index fe3cd5a..e679968 100644
>>> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> > @@ -1948,17 +1948,47 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>>> >     struct drm_info_node *node = m->private;
>>> >     struct drm_device *dev = node->minor->dev;
>>> >     struct drm_i915_private *dev_priv = dev->dev_private;
>>> > -   u32 psrctl, psrstat, psrperf;
>>> > +   u32 psrstat, psrperf;
>>> >
>>> > -   if (!IS_HASWELL(dev)) {
>>> > -           seq_puts(m, "PSR not supported on this platform\n");
>>> > +   if (IS_HASWELL(dev) && I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE) {
>>> > +           seq_puts(m, "PSR enabled\n");
>>> > +   } else {
>>> > +           seq_puts(m, "PSR disabled: ");
>>> > +           switch (dev_priv->no_psr_reason) {
>>> > +           case PSR_NO_SOURCE:
>>>
>>> I am not a fan of using a continually expanding set of enums for
>>> no_psr_reason (and no_fbc_reason). If we just stored a const char
>>> *no_psr_reason, it would make like much easier for us (fewer lines and a
>>> smaller object).
>>>
>>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> > index d4b52a9..c0bd887 100644
>>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> > @@ -1497,11 +1497,83 @@ static void intel_edp_psr_enable_source(struct intel_dp *intel_dp)
>>> >                EDP_PSR_ENABLE);
>>> >  }
>>> >
>>> > +static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
>>> > +{
>>> > +   struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>>> > +   struct drm_device *dev = dig_port->base.base.dev;
>>> > +   struct drm_i915_private *dev_priv = dev->dev_private;
>>> > +   struct drm_crtc *crtc = dig_port->base.base.crtc;
>>> > +   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>> > +   struct drm_i915_gem_object *obj = to_intel_framebuffer(crtc->fb)->obj;
>>> > +   struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>>> > +
>>> > +   if (!IS_HASWELL(dev)) {
>>> > +           DRM_DEBUG_KMS("PSR not supported on this platform\n");
>>> > +           dev_priv->no_psr_reason = PSR_NO_SOURCE;
>>> > +           return false;
>>> > +   }
>>> > +
>>> > +   if ((intel_encoder->type != INTEL_OUTPUT_EDP) ||
>>> > +       (dig_port->port != PORT_A)) {
>>> > +           DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n");
>>> > +           dev_priv->no_psr_reason = PSR_HSW_NOT_DDIA;
>>> > +           return false;
>>> > +   }
>>> > +
>>> > +   if (!is_edp_psr(intel_dp)) {
>>> > +           DRM_DEBUG_KMS("PSR not supported by this panel\n");
>>> > +           dev_priv->no_psr_reason = PSR_NO_SINK;
>>> > +           return false;
>>> > +   }
>>> > +
>>> > +   if (!intel_crtc->active || !crtc->fb || !crtc->mode.clock) {
>>> > +           DRM_DEBUG_KMS("crtc not active for PSR\n");
>>> > +           dev_priv->no_psr_reason = PSR_CRTC_NOT_ACTIVE;
>>> > +           return false;
>>> > +   }
>>> > +
>>> > +   if ((I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE) ||
>>> > +       (I915_READ(HSW_PWR_WELL_KVMR) & HSW_PWR_WELL_ENABLE)) {
>>>
>>> Any time we resort to reading back register state is a failure in our
>>> state tracking (and pipe_config).
>>
>> Highly agreed, especially if it's a resource which is out of our control
>> like the KVMR bits. Since that's just plain broken I've removed it from
>> the patch.
>
> Since Audio guys will enable power well and let it enabled all the
> time, I'm wondering in mask LPSP at PSR_DEBUG_CTL.
> So PSR can work even with power well enabled.
> What do you think?

I have honestly no idea ... have you poked Art about this?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 06/11] drm/i915: Match all PSR mode entry conditions before enabling it.

[Paulo Zanoni]

2013/7/11 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> v2: Prefer seq_puts to seq_printf by Paulo Zanoni.
> v3: small changes like avoiding calling dp_to_dig_port twice as noticed by
>     Paulo Zanoni.
> v4: Avoiding reading non-existent registers - noticed by Paulo
>     on first psr debugfs patch.
> v5: Accepting more suggestions from Paulo:
>     * check sw interlace flag instead of i915_read
>     * introduce PSR_S3D_ENABLED to avoid forgeting it whenever added.
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 44 ++++++++++++++++++----
>  drivers/gpu/drm/i915/i915_drv.h     | 13 +++++++
>  drivers/gpu/drm/i915/i915_reg.h     |  7 ++++
>  drivers/gpu/drm/i915/intel_dp.c     | 74 ++++++++++++++++++++++++++++++++++++-
>  4 files changed, 130 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index fe3cd5a..e679968 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1948,17 +1948,47 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>         struct drm_info_node *node = m->private;
>         struct drm_device *dev = node->minor->dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       u32 psrctl, psrstat, psrperf;
> +       u32 psrstat, psrperf;
>
> -       if (!IS_HASWELL(dev)) {
> -               seq_puts(m, "PSR not supported on this platform\n");
> +       if (IS_HASWELL(dev) && I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE) {
> +               seq_puts(m, "PSR enabled\n");
> +       } else {

If we're not Haswell we fall on this "else" case, then we print "PSR
disabled" and look at no_psr_reason.

With that fixed: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> +               seq_puts(m, "PSR disabled: ");
> +               switch (dev_priv->no_psr_reason) {
> +               case PSR_NO_SOURCE:
> +                       seq_puts(m, "not supported on this platform");
> +                       break;
> +               case PSR_NO_SINK:
> +                       seq_puts(m, "not supported by panel");
> +                       break;
> +               case PSR_CRTC_NOT_ACTIVE:
> +                       seq_puts(m, "crtc not active");
> +                       break;
> +               case PSR_PWR_WELL_ENABLED:
> +                       seq_puts(m, "power well enabled");
> +                       break;
> +               case PSR_NOT_TILED:
> +                       seq_puts(m, "not tiled");
> +                       break;
> +               case PSR_SPRITE_ENABLED:
> +                       seq_puts(m, "sprite enabled");
> +                       break;
> +               case PSR_S3D_ENABLED:
> +                       seq_puts(m, "stereo 3d enabled");
> +                       break;
> +               case PSR_INTERLACED_ENABLED:
> +                       seq_puts(m, "interlaced enabled");
> +                       break;
> +               case PSR_HSW_NOT_DDIA:
> +                       seq_puts(m, "HSW ties PSR to DDI A (eDP)");
> +                       break;
> +               default:
> +                       seq_puts(m, "unknown reason");
> +               }
> +               seq_puts(m, "\n");
>                 return 0;
>         }
>
> -       psrctl = I915_READ(EDP_PSR_CTL);
> -       seq_printf(m, "PSR Enabled: %s\n",
> -                  yesno(psrctl & EDP_PSR_ENABLE));
> -
>         psrstat = I915_READ(EDP_PSR_STATUS_CTL);
>
>         seq_puts(m, "PSR Current State: ");
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 842aada..d0b9483 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -560,6 +560,17 @@ struct i915_fbc {
>         } no_fbc_reason;
>  };
>
> +enum no_psr_reason {
> +       PSR_NO_SOURCE, /* Not supported on platform */
> +       PSR_NO_SINK, /* Not supported by panel */
> +       PSR_CRTC_NOT_ACTIVE,
> +       PSR_PWR_WELL_ENABLED,
> +       PSR_NOT_TILED,
> +       PSR_SPRITE_ENABLED,
> +       PSR_S3D_ENABLED,
> +       PSR_INTERLACED_ENABLED,
> +       PSR_HSW_NOT_DDIA,
> +};
>
>  enum intel_pch {
>         PCH_NONE = 0,   /* No PCH present */
> @@ -1161,6 +1172,8 @@ typedef struct drm_i915_private {
>         /* Haswell power well */
>         struct i915_power_well power_well;
>
> +       enum no_psr_reason no_psr_reason;
> +
>         struct i915_gpu_error gpu_error;
>
>         struct drm_i915_gem_object *vlv_pctx;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b328ec6..3bca337 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4150,6 +4150,13 @@
>  #define HSW_TVIDEO_DIP_VSC_DATA(trans) \
>          _TRANSCODER(trans, HSW_VIDEO_DIP_VSC_DATA_A, HSW_VIDEO_DIP_VSC_DATA_B)
>
> +#define HSW_STEREO_3D_CTL_A    0x70020
> +#define   S3D_ENABLE           (1<<31)
> +#define HSW_STEREO_3D_CTL_B    0x71020
> +
> +#define HSW_STEREO_3D_CTL(trans) \
> +       _TRANSCODER(trans, HSW_STEREO_3D_CTL_A, HSW_STEREO_3D_CTL_A)
> +
>  #define _PCH_TRANS_HTOTAL_B          0xe1000
>  #define _PCH_TRANS_HBLANK_B          0xe1004
>  #define _PCH_TRANS_HSYNC_B           0xe1008
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index d4b52a9..c0bd887 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1497,11 +1497,83 @@ static void intel_edp_psr_enable_source(struct intel_dp *intel_dp)
>                    EDP_PSR_ENABLE);
>  }
>
> +static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
> +{
> +       struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +       struct drm_device *dev = dig_port->base.base.dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct drm_crtc *crtc = dig_port->base.base.crtc;
> +       struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +       struct drm_i915_gem_object *obj = to_intel_framebuffer(crtc->fb)->obj;
> +       struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> +
> +       if (!IS_HASWELL(dev)) {
> +               DRM_DEBUG_KMS("PSR not supported on this platform\n");
> +               dev_priv->no_psr_reason = PSR_NO_SOURCE;
> +               return false;
> +       }
> +
> +       if ((intel_encoder->type != INTEL_OUTPUT_EDP) ||
> +           (dig_port->port != PORT_A)) {
> +               DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n");
> +               dev_priv->no_psr_reason = PSR_HSW_NOT_DDIA;
> +               return false;
> +       }
> +
> +       if (!is_edp_psr(intel_dp)) {
> +               DRM_DEBUG_KMS("PSR not supported by this panel\n");
> +               dev_priv->no_psr_reason = PSR_NO_SINK;
> +               return false;
> +       }
> +
> +       if (!intel_crtc->active || !crtc->fb || !crtc->mode.clock) {
> +               DRM_DEBUG_KMS("crtc not active for PSR\n");
> +               dev_priv->no_psr_reason = PSR_CRTC_NOT_ACTIVE;
> +               return false;
> +       }
> +
> +       if ((I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE) ||
> +           (I915_READ(HSW_PWR_WELL_KVMR) & HSW_PWR_WELL_ENABLE)) {
> +               DRM_DEBUG_KMS("PSR condition failed: Power Well is Enabled\n");
> +               dev_priv->no_psr_reason = PSR_PWR_WELL_ENABLED;
> +               return false;
> +       }
> +
> +       if (obj->tiling_mode != I915_TILING_X ||
> +           obj->fence_reg == I915_FENCE_REG_NONE) {
> +               DRM_DEBUG_KMS("PSR condition failed: fb not tiled or fenced\n");
> +               dev_priv->no_psr_reason = PSR_NOT_TILED;
> +               return false;
> +       }
> +
> +       if (I915_READ(SPRCTL(intel_crtc->pipe)) & SPRITE_ENABLE) {
> +               DRM_DEBUG_KMS("PSR condition failed: Sprite is Enabled\n");
> +               dev_priv->no_psr_reason = PSR_SPRITE_ENABLED;
> +               return false;
> +       }
> +
> +       if (I915_READ(HSW_STEREO_3D_CTL(intel_crtc->config.cpu_transcoder)) &
> +           S3D_ENABLE) {
> +               DRM_DEBUG_KMS("PSR condition failed: Stereo 3D is Enabled\n");
> +               dev_priv->no_psr_reason = PSR_S3D_ENABLED;
> +               return false;
> +       }
> +
> +       if (crtc->mode.flags & DRM_MODE_FLAG_INTERLACE) {
> +               DRM_DEBUG_KMS("PSR condition failed: Interlaced is Enabled\n");
> +               dev_priv->no_psr_reason = PSR_INTERLACED_ENABLED;
> +               return false;
> +       }
> +
> +       return true;
> +}
> +
>  void intel_edp_psr_enable(struct intel_dp *intel_dp)
>  {
>         struct drm_device *dev = intel_dp_to_dev(intel_dp);
>
> -       if (!is_edp_psr(intel_dp) || intel_edp_is_psr_enabled(dev))
> +       if (!intel_edp_psr_match_conditions(intel_dp) ||
> +           intel_edp_is_psr_enabled(dev))
>                 return;
>
>         /* Setup PSR once */
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH 07/11] drm/i915: add update function to disable/enable-back PSR

[Rodrigo Vivi]

Required function to disable PSR when going to console mode.
But also can be used whenever PSR mode entry conditions changed.

v2: Add it before PSR Hook. Update function not really been called yet.
v3: Fix coding style detected by checkpatch by Paulo Zanoni.
v4: do_enable must be static as Paulo noticed.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 31 ++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c0bd887..c0870a69 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1568,7 +1568,7 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
 	return true;
 }
 
-void intel_edp_psr_enable(struct intel_dp *intel_dp)
+static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 
@@ -1586,6 +1586,15 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 	intel_edp_psr_enable_source(intel_dp);
 }
 
+void intel_edp_psr_enable(struct intel_dp *intel_dp)
+{
+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+
+	if (intel_edp_psr_match_conditions(intel_dp) &&
+	    !intel_edp_is_psr_enabled(dev))
+		intel_edp_psr_do_enable(intel_dp);
+}
+
 void intel_edp_psr_disable(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
@@ -1602,6 +1611,26 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
 		DRM_ERROR("Timed out waiting for PSR Idle State\n");
 }
 
+void intel_edp_psr_update(struct drm_device *dev)
+{
+	struct intel_encoder *encoder;
+	struct intel_dp *intel_dp = NULL;
+
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
+		if (encoder->type == INTEL_OUTPUT_EDP) {
+			intel_dp = enc_to_intel_dp(&encoder->base);
+
+			if (!is_edp_psr(intel_dp))
+				return;
+
+			if (!intel_edp_psr_match_conditions(intel_dp))
+				intel_edp_psr_disable(intel_dp);
+			else
+				if (!intel_edp_is_psr_enabled(dev))
+					intel_edp_psr_do_enable(intel_dp);
+		}
+}
+
 static void intel_disable_dp(struct intel_encoder *encoder)
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ff36a40..40e955d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -837,5 +837,6 @@ extern bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
 
 extern void intel_edp_psr_enable(struct intel_dp *intel_dp);
 extern void intel_edp_psr_disable(struct intel_dp *intel_dp);
+extern void intel_edp_psr_update(struct drm_device *dev);
 
 #endif /* __INTEL_DRV_H__ */
-- 
1.7.11.7

Re: [PATCH 07/11] drm/i915: add update function to disable/enable-back PSR

[Chris Wilson]

On Thu, Jul 11, 2013 at 06:45:01PM -0300, Rodrigo Vivi wrote:
> @@ -1602,6 +1611,26 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
>  		DRM_ERROR("Timed out waiting for PSR Idle State\n");
>  }
>  
> +void intel_edp_psr_update(struct drm_device *dev)
> +{
> +	struct intel_encoder *encoder;
> +	struct intel_dp *intel_dp = NULL;
> +
> +	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
> +		if (encoder->type == INTEL_OUTPUT_EDP) {

How many eDP are you planning to allow on the system? We already have
precedence for the presumption of a single (integrated) panel on a device,
maybe we can add the logic there (i.e. stash a back pointer in this case)?

We could then also do a debugfs/i915_panel_info where we can inspect
various details normally associated with the panels (PSR status, backlight
interface, eDP vs LVDS, etc).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 07/11] drm/i915: add update function to disable/enable-back PSR

[Rodrigo Vivi]

On Mon, Jul 15, 2013 at 11:00 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, Jul 11, 2013 at 06:45:01PM -0300, Rodrigo Vivi wrote:
>> @@ -1602,6 +1611,26 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
>>               DRM_ERROR("Timed out waiting for PSR Idle State\n");
>>  }
>>
>> +void intel_edp_psr_update(struct drm_device *dev)
>> +{
>> +     struct intel_encoder *encoder;
>> +     struct intel_dp *intel_dp = NULL;
>> +
>> +     list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
>> +             if (encoder->type == INTEL_OUTPUT_EDP) {
>
> How many eDP are you planning to allow on the system? We already have
> precedence for the presumption of a single (integrated) panel on a device,
> maybe we can add the logic there (i.e. stash a back pointer in this case)?

That is a good question... I asked it myself many times when I was
trying to get intel_dp with edp from dev...
For the first version I just run the loop getting any intel_dp with
edp since we have this assumption of only one edp,
but then I thought about that convertibles with 2 panels and since in
hsw we can have edp on port D I decided to let the implementation
more generic as possible although I know we won't have this case... at
least not any time soon.

>
> We could then also do a debugfs/i915_panel_info where we can inspect
> various details normally associated with the panels (PSR status, backlight
> interface, eDP vs LVDS, etc).

I liked this idea... will have in mind for later

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre

Thanks

--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Re: [PATCH 07/11] drm/i915: add update function to disable/enable-back PSR

[Daniel Vetter]

On Mon, Jul 15, 2013 at 05:21:12PM -0300, Rodrigo Vivi wrote:
> On Mon, Jul 15, 2013 at 11:00 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Thu, Jul 11, 2013 at 06:45:01PM -0300, Rodrigo Vivi wrote:
> >> @@ -1602,6 +1611,26 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
> >>               DRM_ERROR("Timed out waiting for PSR Idle State\n");
> >>  }
> >>
> >> +void intel_edp_psr_update(struct drm_device *dev)
> >> +{
> >> +     struct intel_encoder *encoder;
> >> +     struct intel_dp *intel_dp = NULL;
> >> +
> >> +     list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
> >> +             if (encoder->type == INTEL_OUTPUT_EDP) {
> >
> > How many eDP are you planning to allow on the system? We already have
> > precedence for the presumption of a single (integrated) panel on a device,
> > maybe we can add the logic there (i.e. stash a back pointer in this case)?
> 
> That is a good question... I asked it myself many times when I was
> trying to get intel_dp with edp from dev...
> For the first version I just run the loop getting any intel_dp with
> edp since we have this assumption of only one edp,
> but then I thought about that convertibles with 2 panels and since in
> hsw we can have edp on port D I decided to let the implementation
> more generic as possible although I know we won't have this case... at
> least not any time soon.

The way I nowadays solve such a conundrum is to shovel a bit of metadata
(like psr_capable_sink) into pipe_config and let encoders fill it out
appropriately in their ->compute_config functions. Most of the "walk over
all encoders and noodle int their innards" have disappeared through
that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 07/11] drm/i915: add update function to disable/enable-back PSR

[Paulo Zanoni]

2013/7/11 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> Required function to disable PSR when going to console mode.
> But also can be used whenever PSR mode entry conditions changed.
>
> v2: Add it before PSR Hook. Update function not really been called yet.
> v3: Fix coding style detected by checkpatch by Paulo Zanoni.
> v4: do_enable must be static as Paulo noticed.
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

Even though we can improve the loops and other things with
pipe_config, this patch seems to work, so:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

We can still do the reworks later.

> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 31 ++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  2 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c0bd887..c0870a69 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1568,7 +1568,7 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
>         return true;
>  }
>
> -void intel_edp_psr_enable(struct intel_dp *intel_dp)
> +static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
>  {
>         struct drm_device *dev = intel_dp_to_dev(intel_dp);
>
> @@ -1586,6 +1586,15 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
>         intel_edp_psr_enable_source(intel_dp);
>  }
>
> +void intel_edp_psr_enable(struct intel_dp *intel_dp)
> +{
> +       struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +
> +       if (intel_edp_psr_match_conditions(intel_dp) &&
> +           !intel_edp_is_psr_enabled(dev))
> +               intel_edp_psr_do_enable(intel_dp);
> +}
> +
>  void intel_edp_psr_disable(struct intel_dp *intel_dp)
>  {
>         struct drm_device *dev = intel_dp_to_dev(intel_dp);
> @@ -1602,6 +1611,26 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
>                 DRM_ERROR("Timed out waiting for PSR Idle State\n");
>  }
>
> +void intel_edp_psr_update(struct drm_device *dev)
> +{
> +       struct intel_encoder *encoder;
> +       struct intel_dp *intel_dp = NULL;
> +
> +       list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
> +               if (encoder->type == INTEL_OUTPUT_EDP) {
> +                       intel_dp = enc_to_intel_dp(&encoder->base);
> +
> +                       if (!is_edp_psr(intel_dp))
> +                               return;
> +
> +                       if (!intel_edp_psr_match_conditions(intel_dp))
> +                               intel_edp_psr_disable(intel_dp);
> +                       else
> +                               if (!intel_edp_is_psr_enabled(dev))
> +                                       intel_edp_psr_do_enable(intel_dp);
> +               }
> +}
> +
>  static void intel_disable_dp(struct intel_encoder *encoder)
>  {
>         struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ff36a40..40e955d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -837,5 +837,6 @@ extern bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
>
>  extern void intel_edp_psr_enable(struct intel_dp *intel_dp);
>  extern void intel_edp_psr_disable(struct intel_dp *intel_dp);
> +extern void intel_edp_psr_update(struct drm_device *dev);
>
>  #endif /* __INTEL_DRV_H__ */
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH 08/11] drm/intel: add enable_psr module option and disable psr by default

[Rodrigo Vivi]

v2: prefer seq_puts to seq_printf detected by Paulo Zanoni.
v3: PSR is disabled by default. Without userspace ready it
    will cause regression for kde and xdm users

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 3 +++
 drivers/gpu/drm/i915/i915_drv.c     | 4 ++++
 drivers/gpu/drm/i915/i915_drv.h     | 2 ++
 drivers/gpu/drm/i915/intel_dp.c     | 6 ++++++
 4 files changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index e679968..5a2b621 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1961,6 +1961,9 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 		case PSR_NO_SINK:
 			seq_puts(m, "not supported by panel");
 			break;
+		case PSR_MODULE_PARAM:
+			seq_puts(m, "disabled by flag");
+			break;
 		case PSR_CRTC_NOT_ACTIVE:
 			seq_puts(m, "crtc not active");
 			break;
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b07362f..f2c018d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -118,6 +118,10 @@ module_param_named(i915_enable_ppgtt, i915_enable_ppgtt, int, 0600);
 MODULE_PARM_DESC(i915_enable_ppgtt,
 		"Enable PPGTT (default: true)");
 
+int i915_enable_psr __read_mostly = 0;
+module_param_named(enable_psr, i915_enable_psr, int, 0600);
+MODULE_PARM_DESC(enable_psr, "Enable PSR (default: false)");
+
 unsigned int i915_preliminary_hw_support __read_mostly = 0;
 module_param_named(preliminary_hw_support, i915_preliminary_hw_support, int, 0600);
 MODULE_PARM_DESC(preliminary_hw_support,
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d0b9483..1992081 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -563,6 +563,7 @@ struct i915_fbc {
 enum no_psr_reason {
 	PSR_NO_SOURCE, /* Not supported on platform */
 	PSR_NO_SINK, /* Not supported by panel */
+	PSR_MODULE_PARAM,
 	PSR_CRTC_NOT_ACTIVE,
 	PSR_PWR_WELL_ENABLED,
 	PSR_NOT_TILED,
@@ -1593,6 +1594,7 @@ extern int i915_enable_rc6 __read_mostly;
 extern int i915_enable_fbc __read_mostly;
 extern bool i915_enable_hangcheck __read_mostly;
 extern int i915_enable_ppgtt __read_mostly;
+extern int i915_enable_psr __read_mostly;
 extern unsigned int i915_preliminary_hw_support __read_mostly;
 extern int i915_disable_power_well __read_mostly;
 extern int i915_enable_ips __read_mostly;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c0870a69..c0defaf 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1526,6 +1526,12 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
 		return false;
 	}
 
+	if (!i915_enable_psr) {
+		DRM_DEBUG_KMS("PSR disable by flag\n");
+		dev_priv->no_psr_reason = PSR_MODULE_PARAM;
+		return false;
+	}
+
 	if (!intel_crtc->active || !crtc->fb || !crtc->mode.clock) {
 		DRM_DEBUG_KMS("crtc not active for PSR\n");
 		dev_priv->no_psr_reason = PSR_CRTC_NOT_ACTIVE;
-- 
1.7.11.7

Re: [PATCH 08/11] drm/intel: add enable_psr module option and disable psr by default

[Chris Wilson]

On Thu, Jul 11, 2013 at 06:45:02PM -0300, Rodrigo Vivi wrote:
> v2: prefer seq_puts to seq_printf detected by Paulo Zanoni.
> v3: PSR is disabled by default. Without userspace ready it
>     will cause regression for kde and xdm users

I think we should still aim to enable by default and disable on the
first direct access after a mmioflip.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 08/11] drm/intel: add enable_psr module option and disable psr by default

[Rodrigo Vivi]

On Mon, Jul 15, 2013 at 11:01 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, Jul 11, 2013 at 06:45:02PM -0300, Rodrigo Vivi wrote:
>> v2: prefer seq_puts to seq_printf detected by Paulo Zanoni.
>> v3: PSR is disabled by default. Without userspace ready it
>>     will cause regression for kde and xdm users
>
> I think we should still aim to enable by default and disable on the
> first direct access after a mmioflip.

unfortunately I couldn't implement a reliable way of detect it without
false positives,
so let's put this disabled for now and revert when I find a reliable way.

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Re: [PATCH 08/11] drm/intel: add enable_psr module option and disable psr by default

[Chris Wilson]

On Mon, Jul 15, 2013 at 05:23:35PM -0300, Rodrigo Vivi wrote:
> On Mon, Jul 15, 2013 at 11:01 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Thu, Jul 11, 2013 at 06:45:02PM -0300, Rodrigo Vivi wrote:
> >> v2: prefer seq_puts to seq_printf detected by Paulo Zanoni.
> >> v3: PSR is disabled by default. Without userspace ready it
> >>     will cause regression for kde and xdm users
> >
> > I think we should still aim to enable by default and disable on the
> > first direct access after a mmioflip.
> 
> unfortunately I couldn't implement a reliable way of detect it without
> false positives,

Can you give me an example of one of the false positives? The detect
front buffer writes patches we had should be good enough to only punish
legacy userspace.

> so let's put this disabled for now and revert when I find a reliable way.

That's fine, just think we're giving up too easily ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 08/11] drm/intel: add enable_psr module option and disable psr by default

[Daniel Vetter]

On Mon, Jul 15, 2013 at 11:01:13PM +0100, Chris Wilson wrote:
> On Mon, Jul 15, 2013 at 05:23:35PM -0300, Rodrigo Vivi wrote:
> > On Mon, Jul 15, 2013 at 11:01 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > On Thu, Jul 11, 2013 at 06:45:02PM -0300, Rodrigo Vivi wrote:
> > >> v2: prefer seq_puts to seq_printf detected by Paulo Zanoni.
> > >> v3: PSR is disabled by default. Without userspace ready it
> > >>     will cause regression for kde and xdm users
> > >
> > > I think we should still aim to enable by default and disable on the
> > > first direct access after a mmioflip.
> > 
> > unfortunately I couldn't implement a reliable way of detect it without
> > false positives,
> 
> Can you give me an example of one of the false positives? The detect
> front buffer writes patches we had should be good enough to only punish
> legacy userspace.
> 
> > so let's put this disabled for now and revert when I find a reliable way.
> 
> That's fine, just think we're giving up too easily ;-)

Agreed, and part of the problem is that slacker-me is sitting on a big
review of your rfc patches ;-) I really need to get around to write that
down into a nice mail from my ineligible notes ...

I hope that with good sw side frontbuffer rendering tracking we'll be able
to enable psr in more conditions, e.g. also when sprites are enabled and
similar cases.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 08/11] drm/intel: add enable_psr module option and disable psr by default

[Rodrigo Vivi]

On Mon, Jul 15, 2013 at 7:01 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, Jul 15, 2013 at 05:23:35PM -0300, Rodrigo Vivi wrote:
>> On Mon, Jul 15, 2013 at 11:01 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > On Thu, Jul 11, 2013 at 06:45:02PM -0300, Rodrigo Vivi wrote:
>> >> v2: prefer seq_puts to seq_printf detected by Paulo Zanoni.
>> >> v3: PSR is disabled by default. Without userspace ready it
>> >>     will cause regression for kde and xdm users
>> >
>> > I think we should still aim to enable by default and disable on the
>> > first direct access after a mmioflip.
>>
>> unfortunately I couldn't implement a reliable way of detect it without
>> false positives,
>
> Can you give me an example of one of the false positives? The detect
> front buffer writes patches we had should be good enough to only punish
> legacy userspace.

psr works very well on unity and gnome and disable it in the cases it
already works well is a kind of false positive...
I didn't mean false positive in terms of error in detection, but
disabling when not really needed.

>
>> so let's put this disabled for now and revert when I find a reliable way.
>
> That's fine, just think we're giving up too easily ;-)

Yes, I am! tell you more on irc ;)

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[PATCH 09/11] drm/i915: Adding global I915_PARAM for PSR ENABLED.

[Rodrigo Vivi]

This global value allows userspace know when PSR is enabled.

This will allow userspace emit more busy_ioctl when doing directly copy_area
operations through scanout allowing forced psr exit.

v2: Check for PSR enabled instead of active. (by Chris Wilson)
v3: Use existing intel_edp_is_psr_enabled function.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_dma.c  | 3 +++
 drivers/gpu/drm/i915/intel_dp.c  | 2 +-
 drivers/gpu/drm/i915/intel_drv.h | 1 +
 include/uapi/drm/i915_drm.h      | 1 +
 4 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 6ce9033..1e5dd1c 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1000,6 +1000,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
 	case I915_PARAM_HAS_EXEC_HANDLE_LUT:
 		value = 1;
 		break;
+	case I915_PARAM_PSR_ENABLED:
+		value = intel_edp_is_psr_enabled(dev);
+		break;
 	default:
 		DRM_DEBUG("Unknown parameter %d\n", param->param);
 		return -EINVAL;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c0defaf..3c9473c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1383,7 +1383,7 @@ static bool is_edp_psr(struct intel_dp *intel_dp)
 		intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED;
 }
 
-static bool intel_edp_is_psr_enabled(struct drm_device *dev)
+bool intel_edp_is_psr_enabled(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 40e955d..0f52362 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -838,5 +838,6 @@ extern bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
 extern void intel_edp_psr_enable(struct intel_dp *intel_dp);
 extern void intel_edp_psr_disable(struct intel_dp *intel_dp);
 extern void intel_edp_psr_update(struct drm_device *dev);
+extern bool intel_edp_is_psr_enabled(struct drm_device *dev);
 
 #endif /* __INTEL_DRV_H__ */
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 923ed7f..a5db73b 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -310,6 +310,7 @@ typedef struct drm_i915_irq_wait {
 #define I915_PARAM_HAS_PINNED_BATCHES	 24
 #define I915_PARAM_HAS_EXEC_NO_RELOC	 25
 #define I915_PARAM_HAS_EXEC_HANDLE_LUT   26
+#define I915_PARAM_PSR_ENABLED		 27
 
 typedef struct drm_i915_getparam {
 	int param;
-- 
1.7.11.7

Re: [PATCH 09/11] drm/i915: Adding global I915_PARAM for PSR ENABLED.

[Rodrigo Vivi]

Hi Chris,

could you please review this specific one or give you ack here?

Thanks

On Thu, Jul 11, 2013 at 6:45 PM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> This global value allows userspace know when PSR is enabled.
>
> This will allow userspace emit more busy_ioctl when doing directly copy_area
> operations through scanout allowing forced psr exit.
>
> v2: Check for PSR enabled instead of active. (by Chris Wilson)
> v3: Use existing intel_edp_is_psr_enabled function.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c  | 3 +++
>  drivers/gpu/drm/i915/intel_dp.c  | 2 +-
>  drivers/gpu/drm/i915/intel_drv.h | 1 +
>  include/uapi/drm/i915_drm.h      | 1 +
>  4 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 6ce9033..1e5dd1c 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1000,6 +1000,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
>         case I915_PARAM_HAS_EXEC_HANDLE_LUT:
>                 value = 1;
>                 break;
> +       case I915_PARAM_PSR_ENABLED:
> +               value = intel_edp_is_psr_enabled(dev);
> +               break;
>         default:
>                 DRM_DEBUG("Unknown parameter %d\n", param->param);
>                 return -EINVAL;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c0defaf..3c9473c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1383,7 +1383,7 @@ static bool is_edp_psr(struct intel_dp *intel_dp)
>                 intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED;
>  }
>
> -static bool intel_edp_is_psr_enabled(struct drm_device *dev)
> +bool intel_edp_is_psr_enabled(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 40e955d..0f52362 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -838,5 +838,6 @@ extern bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
>  extern void intel_edp_psr_enable(struct intel_dp *intel_dp);
>  extern void intel_edp_psr_disable(struct intel_dp *intel_dp);
>  extern void intel_edp_psr_update(struct drm_device *dev);
> +extern bool intel_edp_is_psr_enabled(struct drm_device *dev);
>
>  #endif /* __INTEL_DRV_H__ */
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 923ed7f..a5db73b 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -310,6 +310,7 @@ typedef struct drm_i915_irq_wait {
>  #define I915_PARAM_HAS_PINNED_BATCHES   24
>  #define I915_PARAM_HAS_EXEC_NO_RELOC    25
>  #define I915_PARAM_HAS_EXEC_HANDLE_LUT   26
> +#define I915_PARAM_PSR_ENABLED          27
>
>  typedef struct drm_i915_getparam {
>         int param;
> --
> 1.7.11.7
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Re: [PATCH 09/11] drm/i915: Adding global I915_PARAM for PSR ENABLED.

[Chris Wilson]

On Wed, Jul 17, 2013 at 02:46:52PM -0300, Rodrigo Vivi wrote:
> Hi Chris,
> 
> could you please review this specific one or give you ack here?

Didn't see anything wrong with it. The only caveat I have is that the
GETPARAM must be accurate immediately following a setcrtc. If you can
guarrantee that is true, you can have my
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
and if Daniel delays, ask him to reserve the PARAM number. 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 09/11] drm/i915: Adding global I915_PARAM for PSR ENABLED.

[Rodrigo Vivi]

On Wed, Jul 17, 2013 at 5:18 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, Jul 17, 2013 at 02:46:52PM -0300, Rodrigo Vivi wrote:
>> Hi Chris,
>>
>> could you please review this specific one or give you ack here?
>
> Didn't see anything wrong with it. The only caveat I have is that the
> GETPARAM must be accurate immediately following a setcrtc.

To be truly honest I have no idea, mainly when we alternate with fbcon
updating psr state at set_base.
Could you please also review subsequent patches in this series... 10 and 11.
I think 11 answer this question...

Another alternative would be using i915_enable_psr + i915_powersave
check instead of reading the register for current enabled status.

> If you can
> guarrantee that is true, you can have my
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> and if Daniel delays, ask him to reserve the PARAM number.
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Re: [PATCH 09/11] drm/i915: Adding global I915_PARAM for PSR ENABLED.

[Chris Wilson]

On Wed, Jul 17, 2013 at 06:01:03PM -0300, Rodrigo Vivi wrote:
> On Wed, Jul 17, 2013 at 5:18 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Wed, Jul 17, 2013 at 02:46:52PM -0300, Rodrigo Vivi wrote:
> >> Hi Chris,
> >>
> >> could you please review this specific one or give you ack here?
> >
> > Didn't see anything wrong with it. The only caveat I have is that the
> > GETPARAM must be accurate immediately following a setcrtc.
> 
> To be truly honest I have no idea, mainly when we alternate with fbcon
> updating psr state at set_base.
> Could you please also review subsequent patches in this series... 10 and 11.
> I think 11 answer this question...

If it is not clear by this point, and the changelog doesn't make it
clear, then something is missing from this patch. Hint ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 09/11] drm/i915: Adding global I915_PARAM for PSR ENABLED.

[Daniel Vetter]

On Wed, Jul 17, 2013 at 10:08:34PM +0100, Chris Wilson wrote:
> On Wed, Jul 17, 2013 at 06:01:03PM -0300, Rodrigo Vivi wrote:
> > On Wed, Jul 17, 2013 at 5:18 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > On Wed, Jul 17, 2013 at 02:46:52PM -0300, Rodrigo Vivi wrote:
> > >> Hi Chris,
> > >>
> > >> could you please review this specific one or give you ack here?
> > >
> > > Didn't see anything wrong with it. The only caveat I have is that the
> > > GETPARAM must be accurate immediately following a setcrtc.
> > 
> > To be truly honest I have no idea, mainly when we alternate with fbcon
> > updating psr state at set_base.
> > Could you please also review subsequent patches in this series... 10 and 11.
> > I think 11 answer this question...
> 
> If it is not clear by this point, and the changelog doesn't make it
> clear, then something is missing from this patch. Hint ;-)

I'll punt on userspace interface changes, at least until we've figured out
a clear picture how to do this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 09/11] drm/i915: Adding global I915_PARAM for PSR ENABLED.

[Rodrigo Vivi]

On Thu, Jul 18, 2013 at 5:24 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Jul 17, 2013 at 10:08:34PM +0100, Chris Wilson wrote:
>> On Wed, Jul 17, 2013 at 06:01:03PM -0300, Rodrigo Vivi wrote:
>> > On Wed, Jul 17, 2013 at 5:18 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > > On Wed, Jul 17, 2013 at 02:46:52PM -0300, Rodrigo Vivi wrote:
>> > >> Hi Chris,
>> > >>
>> > >> could you please review this specific one or give you ack here?
>> > >
>> > > Didn't see anything wrong with it. The only caveat I have is that the
>> > > GETPARAM must be accurate immediately following a setcrtc.
>> >
>> > To be truly honest I have no idea, mainly when we alternate with fbcon
>> > updating psr state at set_base.
>> > Could you please also review subsequent patches in this series... 10 and 11.
>> > I think 11 answer this question...
>>
>> If it is not clear by this point, and the changelog doesn't make it
>> clear, then something is missing from this patch. Hint ;-)
>
> I'll punt on userspace interface changes, at least until we've figured out
> a clear picture how to do this.

agreed

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[PATCH 10/11] drm/i915: Add functions to force psr exit

[Rodrigo Vivi]

PSR tracking engine in HSW doesn't detect automagically some directly copy area
operations through scanout so we will have to kick it manually and
reschedule it to come back to normal operation as soon as possible.

v2: Before PSR Hook. Don't force it when busy yet.
v3/v4: Solved small conflict.
v5: setup once function was already added on previous commit.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_reg.h  |  1 +
 drivers/gpu/drm/i915/intel_dp.c  | 46 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  3 +++
 3 files changed, 50 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 3bca337..dc10345 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1840,6 +1840,7 @@
 #define   EDP_PSR_PERF_CNT_MASK		0xffffff
 
 #define EDP_PSR_DEBUG_CTL		0x64860
+#define   EDP_PSR_DEBUG_FORCE_EXIT	(3<<30)
 #define   EDP_PSR_DEBUG_MASK_LPSP	(1<<27)
 #define   EDP_PSR_DEBUG_MASK_MEMUP	(1<<26)
 #define   EDP_PSR_DEBUG_MASK_HPD	(1<<25)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3c9473c..cd168e6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1393,6 +1393,48 @@ bool intel_edp_is_psr_enabled(struct drm_device *dev)
 	return I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
 }
 
+static void intel_edp_psr_delayed_normal_work(struct work_struct *__work)
+{
+	struct intel_dp *intel_dp = container_of(to_delayed_work(__work),
+						 struct intel_dp,
+						 edp_psr_delayed_normal_work);
+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&intel_dp->psr_exit_mutex);
+	I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) &
+		   ~EDP_PSR_DEBUG_FORCE_EXIT);
+	mutex_unlock(&intel_dp->psr_exit_mutex);
+}
+
+void intel_edp_psr_force_exit(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_encoder *encoder;
+	struct intel_dp *intel_dp = NULL;
+
+	if (!intel_edp_is_psr_enabled(dev))
+		return;
+
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
+		if (encoder->type == INTEL_OUTPUT_EDP)
+			intel_dp = enc_to_intel_dp(&encoder->base);
+
+	if (!intel_dp)
+		return;
+
+	if (WARN_ON(!intel_dp->psr_setup_done))
+		return;
+
+	mutex_lock(&intel_dp->psr_exit_mutex);
+	I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) |
+		   EDP_PSR_DEBUG_FORCE_EXIT);
+	mutex_unlock(&intel_dp->psr_exit_mutex);
+
+	schedule_delayed_work(&intel_dp->edp_psr_delayed_normal_work,
+			      msecs_to_jiffies(100));
+}
+
 static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp,
 				    struct edp_vsc_psr *vsc_psr)
 {
@@ -1443,6 +1485,10 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp)
 	I915_WRITE(EDP_PSR_DEBUG_CTL, EDP_PSR_DEBUG_MASK_MEMUP |
 		   EDP_PSR_DEBUG_MASK_HPD);
 
+	INIT_DELAYED_WORK(&intel_dp->edp_psr_delayed_normal_work,
+			  intel_edp_psr_delayed_normal_work);
+	mutex_init(&intel_dp->psr_exit_mutex);
+
 	intel_dp->psr_setup_done = true;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0f52362..e47f3f3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -499,6 +499,8 @@ struct intel_dp {
 	int backlight_off_delay;
 	struct delayed_work panel_vdd_work;
 	bool want_panel_vdd;
+	struct delayed_work edp_psr_delayed_normal_work;
+	struct mutex psr_exit_mutex;
 	bool psr_setup_done;
 	struct intel_connector *attached_connector;
 };
@@ -839,5 +841,6 @@ extern void intel_edp_psr_enable(struct intel_dp *intel_dp);
 extern void intel_edp_psr_disable(struct intel_dp *intel_dp);
 extern void intel_edp_psr_update(struct drm_device *dev);
 extern bool intel_edp_is_psr_enabled(struct drm_device *dev);
+extern void intel_edp_psr_force_exit(struct drm_device *dev);
 
 #endif /* __INTEL_DRV_H__ */
-- 
1.7.11.7

Re: [PATCH 10/11] drm/i915: Add functions to force psr exit

[Chris Wilson]

On Thu, Jul 11, 2013 at 06:45:04PM -0300, Rodrigo Vivi wrote:
> +static void intel_edp_psr_delayed_normal_work(struct work_struct *__work)
> +{
> +	struct intel_dp *intel_dp = container_of(to_delayed_work(__work),
> +						 struct intel_dp,
> +						 edp_psr_delayed_normal_work);
> +	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	mutex_lock(&intel_dp->psr_exit_mutex);
> +	I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) &
> +		   ~EDP_PSR_DEBUG_FORCE_EXIT);
> +	mutex_unlock(&intel_dp->psr_exit_mutex);

Note that mutex_unlock is not necessarily a write-barrier. We may
presume that it uses a locked instruction somewhere in its
implementation, but if you use a POSTING_READ all doubt is removed and
the intent made clear.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: Add functions to force psr exit

[Rodrigo Vivi]

PSR tracking engine in HSW doesn't detect automagically some directly copy area
operations through scanout so we will have to kick it manually and
reschedule it to come back to normal operation as soon as possible.

v2: Before PSR Hook. Don't force it when busy yet.
v3/v4: Solved small conflict.
v5: setup once function was already added on previous commit.
v6: Use POSTING_READ to make sure the mutex works as a write barrier as
    suggested by Chris Wilson

CC: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Shobhit Kumar <shobhit.kumar@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_reg.h  |  1 +
 drivers/gpu/drm/i915/intel_dp.c  | 48 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  3 +++
 3 files changed, 52 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 3bca337..dc10345 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1840,6 +1840,7 @@
 #define   EDP_PSR_PERF_CNT_MASK		0xffffff
 
 #define EDP_PSR_DEBUG_CTL		0x64860
+#define   EDP_PSR_DEBUG_FORCE_EXIT	(3<<30)
 #define   EDP_PSR_DEBUG_MASK_LPSP	(1<<27)
 #define   EDP_PSR_DEBUG_MASK_MEMUP	(1<<26)
 #define   EDP_PSR_DEBUG_MASK_HPD	(1<<25)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3c9473c..47e1676 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1393,6 +1393,50 @@ bool intel_edp_is_psr_enabled(struct drm_device *dev)
 	return I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
 }
 
+static void intel_edp_psr_delayed_normal_work(struct work_struct *__work)
+{
+	struct intel_dp *intel_dp = container_of(to_delayed_work(__work),
+						 struct intel_dp,
+						 edp_psr_delayed_normal_work);
+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&intel_dp->psr_exit_mutex);
+	I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) &
+		   ~EDP_PSR_DEBUG_FORCE_EXIT);
+	POSTING_READ(EDP_PSR_DEBUG_CTL);
+	mutex_unlock(&intel_dp->psr_exit_mutex);
+}
+
+void intel_edp_psr_force_exit(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_encoder *encoder;
+	struct intel_dp *intel_dp = NULL;
+
+	if (!intel_edp_is_psr_enabled(dev))
+		return;
+
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
+		if (encoder->type == INTEL_OUTPUT_EDP)
+			intel_dp = enc_to_intel_dp(&encoder->base);
+
+	if (!intel_dp)
+		return;
+
+	if (WARN_ON(!intel_dp->psr_setup_done))
+		return;
+
+	mutex_lock(&intel_dp->psr_exit_mutex);
+	I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) |
+		   EDP_PSR_DEBUG_FORCE_EXIT);
+	POSTING_READ(EDP_PSR_DEBUG_CTL);
+	mutex_unlock(&intel_dp->psr_exit_mutex);
+
+	schedule_delayed_work(&intel_dp->edp_psr_delayed_normal_work,
+			      msecs_to_jiffies(100));
+}
+
 static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp,
 				    struct edp_vsc_psr *vsc_psr)
 {
@@ -1443,6 +1487,10 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp)
 	I915_WRITE(EDP_PSR_DEBUG_CTL, EDP_PSR_DEBUG_MASK_MEMUP |
 		   EDP_PSR_DEBUG_MASK_HPD);
 
+	INIT_DELAYED_WORK(&intel_dp->edp_psr_delayed_normal_work,
+			  intel_edp_psr_delayed_normal_work);
+	mutex_init(&intel_dp->psr_exit_mutex);
+
 	intel_dp->psr_setup_done = true;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0f52362..e47f3f3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -499,6 +499,8 @@ struct intel_dp {
 	int backlight_off_delay;
 	struct delayed_work panel_vdd_work;
 	bool want_panel_vdd;
+	struct delayed_work edp_psr_delayed_normal_work;
+	struct mutex psr_exit_mutex;
 	bool psr_setup_done;
 	struct intel_connector *attached_connector;
 };
@@ -839,5 +841,6 @@ extern void intel_edp_psr_enable(struct intel_dp *intel_dp);
 extern void intel_edp_psr_disable(struct intel_dp *intel_dp);
 extern void intel_edp_psr_update(struct drm_device *dev);
 extern bool intel_edp_is_psr_enabled(struct drm_device *dev);
+extern void intel_edp_psr_force_exit(struct drm_device *dev);
 
 #endif /* __INTEL_DRV_H__ */
-- 
1.8.1.4

Re: [PATCH] drm/i915: Add functions to force psr exit

[Daniel Vetter]

On Mon, Jul 15, 2013 at 05:29:15PM -0300, Rodrigo Vivi wrote:
> PSR tracking engine in HSW doesn't detect automagically some directly copy area
> operations through scanout so we will have to kick it manually and
> reschedule it to come back to normal operation as soon as possible.
> 
> v2: Before PSR Hook. Don't force it when busy yet.
> v3/v4: Solved small conflict.
> v5: setup once function was already added on previous commit.
> v6: Use POSTING_READ to make sure the mutex works as a write barrier as
>     suggested by Chris Wilson
> 
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Shobhit Kumar <shobhit.kumar@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  1 +
>  drivers/gpu/drm/i915/intel_dp.c  | 48 ++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  3 +++
>  3 files changed, 52 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 3bca337..dc10345 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1840,6 +1840,7 @@
>  #define   EDP_PSR_PERF_CNT_MASK		0xffffff
>  
>  #define EDP_PSR_DEBUG_CTL		0x64860
> +#define   EDP_PSR_DEBUG_FORCE_EXIT	(3<<30)
>  #define   EDP_PSR_DEBUG_MASK_LPSP	(1<<27)
>  #define   EDP_PSR_DEBUG_MASK_MEMUP	(1<<26)
>  #define   EDP_PSR_DEBUG_MASK_HPD	(1<<25)
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 3c9473c..47e1676 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1393,6 +1393,50 @@ bool intel_edp_is_psr_enabled(struct drm_device *dev)
>  	return I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
>  }
>  
> +static void intel_edp_psr_delayed_normal_work(struct work_struct *__work)
> +{
> +	struct intel_dp *intel_dp = container_of(to_delayed_work(__work),
> +						 struct intel_dp,
> +						 edp_psr_delayed_normal_work);
> +	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	mutex_lock(&intel_dp->psr_exit_mutex);
> +	I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) &
> +		   ~EDP_PSR_DEBUG_FORCE_EXIT);
> +	POSTING_READ(EDP_PSR_DEBUG_CTL);
> +	mutex_unlock(&intel_dp->psr_exit_mutex);
> +}
> +
> +void intel_edp_psr_force_exit(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_encoder *encoder;
> +	struct intel_dp *intel_dp = NULL;
> +
> +	if (!intel_edp_is_psr_enabled(dev))
> +		return;
> +
> +	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
> +		if (encoder->type == INTEL_OUTPUT_EDP)
> +			intel_dp = enc_to_intel_dp(&encoder->base);
> +
> +	if (!intel_dp)
> +		return;
> +
> +	if (WARN_ON(!intel_dp->psr_setup_done))
> +		return;

If you have to dig out your data like this it usually means that it's at
the wrong spot. Imo we should track a psr_possible bit in the pipe_config
and a psr_enabled (and the locking for the runtime mutex) in the crtc
itself. Also, the psr_setup_done thing is another hint that we should move
this into the crtc.

Second I prefer if functions with tricky tie-in with existing code are
used in the same patch they're created - if it's split over two patches
review is a bit a pain. But if I read the follow-up patch correctly we
call this function from the busy_ioctl unconditionally, which will
obviously result in tons of falls postives - e.g. libdrm uses this ioctl
to manage it's buffer cache.

I think I'll punt on this patch and merge just the parts from the next one
for normal backbuffer rendering with pageflips model.
-Daniel

> +
> +	mutex_lock(&intel_dp->psr_exit_mutex);
> +	I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) |
> +		   EDP_PSR_DEBUG_FORCE_EXIT);
> +	POSTING_READ(EDP_PSR_DEBUG_CTL);
> +	mutex_unlock(&intel_dp->psr_exit_mutex);
> +
> +	schedule_delayed_work(&intel_dp->edp_psr_delayed_normal_work,
> +			      msecs_to_jiffies(100));
> +}
> +
>  static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp,
>  				    struct edp_vsc_psr *vsc_psr)
>  {
> @@ -1443,6 +1487,10 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp)
>  	I915_WRITE(EDP_PSR_DEBUG_CTL, EDP_PSR_DEBUG_MASK_MEMUP |
>  		   EDP_PSR_DEBUG_MASK_HPD);
>  
> +	INIT_DELAYED_WORK(&intel_dp->edp_psr_delayed_normal_work,
> +			  intel_edp_psr_delayed_normal_work);
> +	mutex_init(&intel_dp->psr_exit_mutex);
> +
>  	intel_dp->psr_setup_done = true;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0f52362..e47f3f3 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -499,6 +499,8 @@ struct intel_dp {
>  	int backlight_off_delay;
>  	struct delayed_work panel_vdd_work;
>  	bool want_panel_vdd;
> +	struct delayed_work edp_psr_delayed_normal_work;
> +	struct mutex psr_exit_mutex;
>  	bool psr_setup_done;
>  	struct intel_connector *attached_connector;
>  };
> @@ -839,5 +841,6 @@ extern void intel_edp_psr_enable(struct intel_dp *intel_dp);
>  extern void intel_edp_psr_disable(struct intel_dp *intel_dp);
>  extern void intel_edp_psr_update(struct drm_device *dev);
>  extern bool intel_edp_is_psr_enabled(struct drm_device *dev);
> +extern void intel_edp_psr_force_exit(struct drm_device *dev);
>  
>  #endif /* __INTEL_DRV_H__ */
> -- 
> 1.8.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Add functions to force psr exit

[Rodrigo Vivi]

On Thu, Jul 18, 2013 at 5:33 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Jul 15, 2013 at 05:29:15PM -0300, Rodrigo Vivi wrote:
>> PSR tracking engine in HSW doesn't detect automagically some directly copy area
>> operations through scanout so we will have to kick it manually and
>> reschedule it to come back to normal operation as soon as possible.
>>
>> v2: Before PSR Hook. Don't force it when busy yet.
>> v3/v4: Solved small conflict.
>> v5: setup once function was already added on previous commit.
>> v6: Use POSTING_READ to make sure the mutex works as a write barrier as
>>     suggested by Chris Wilson
>>
>> CC: Chris Wilson <chris@chris-wilson.co.uk>
>> Reviewed-by: Shobhit Kumar <shobhit.kumar@intel.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h  |  1 +
>>  drivers/gpu/drm/i915/intel_dp.c  | 48 ++++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_drv.h |  3 +++
>>  3 files changed, 52 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 3bca337..dc10345 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -1840,6 +1840,7 @@
>>  #define   EDP_PSR_PERF_CNT_MASK              0xffffff
>>
>>  #define EDP_PSR_DEBUG_CTL            0x64860
>> +#define   EDP_PSR_DEBUG_FORCE_EXIT   (3<<30)
>>  #define   EDP_PSR_DEBUG_MASK_LPSP    (1<<27)
>>  #define   EDP_PSR_DEBUG_MASK_MEMUP   (1<<26)
>>  #define   EDP_PSR_DEBUG_MASK_HPD     (1<<25)
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 3c9473c..47e1676 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1393,6 +1393,50 @@ bool intel_edp_is_psr_enabled(struct drm_device *dev)
>>       return I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
>>  }
>>
>> +static void intel_edp_psr_delayed_normal_work(struct work_struct *__work)
>> +{
>> +     struct intel_dp *intel_dp = container_of(to_delayed_work(__work),
>> +                                              struct intel_dp,
>> +                                              edp_psr_delayed_normal_work);
>> +     struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +     mutex_lock(&intel_dp->psr_exit_mutex);
>> +     I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) &
>> +                ~EDP_PSR_DEBUG_FORCE_EXIT);
>> +     POSTING_READ(EDP_PSR_DEBUG_CTL);
>> +     mutex_unlock(&intel_dp->psr_exit_mutex);
>> +}
>> +
>> +void intel_edp_psr_force_exit(struct drm_device *dev)
>> +{
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +     struct intel_encoder *encoder;
>> +     struct intel_dp *intel_dp = NULL;
>> +
>> +     if (!intel_edp_is_psr_enabled(dev))
>> +             return;
>> +
>> +     list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
>> +             if (encoder->type == INTEL_OUTPUT_EDP)
>> +                     intel_dp = enc_to_intel_dp(&encoder->base);
>> +
>> +     if (!intel_dp)
>> +             return;
>> +
>> +     if (WARN_ON(!intel_dp->psr_setup_done))
>> +             return;
>
> If you have to dig out your data like this it usually means that it's at
> the wrong spot. Imo we should track a psr_possible bit in the pipe_config
> and a psr_enabled (and the locking for the runtime mutex) in the crtc
> itself. Also, the psr_setup_done thing is another hint that we should move
> this into the crtc.

Ok, I'm going to play a bit with this and see what I can come up with.

>
> Second I prefer if functions with tricky tie-in with existing code are
> used in the same patch they're created - if it's split over two patches
> review is a bit a pain.

Agreed. I just splited out because I accepted review suggestions to
let all hooks for the last patch to avoid breaking bisects or
something like that.

> But if I read the follow-up patch correctly we
> call this function from the busy_ioctl unconditionally, which will
> obviously result in tons of falls postives - e.g. libdrm uses this ioctl
> to manage it's buffer cache.

Yes, you are right. Although I noticed that psr was still working
fine, I agree that this is too many unecessary calls :(
But couldn't find a better way to fix xdm/kde issue.
Other ideas was letting psr disabled for so long when it could be
there saving power.
But as I said, low priority on this right now... Maybe when following
your suggestions and the list you made we get it fixed properly ;)

> I think I'll punt on this patch and merge just the parts from the next one
> for normal backbuffer rendering with pageflips model.
> -Daniel

Thanks again,
Rodrigo
>
>> +
>> +     mutex_lock(&intel_dp->psr_exit_mutex);
>> +     I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) |
>> +                EDP_PSR_DEBUG_FORCE_EXIT);
>> +     POSTING_READ(EDP_PSR_DEBUG_CTL);
>> +     mutex_unlock(&intel_dp->psr_exit_mutex);
>> +
>> +     schedule_delayed_work(&intel_dp->edp_psr_delayed_normal_work,
>> +                           msecs_to_jiffies(100));
>> +}
>> +
>>  static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp,
>>                                   struct edp_vsc_psr *vsc_psr)
>>  {
>> @@ -1443,6 +1487,10 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp)
>>       I915_WRITE(EDP_PSR_DEBUG_CTL, EDP_PSR_DEBUG_MASK_MEMUP |
>>                  EDP_PSR_DEBUG_MASK_HPD);
>>
>> +     INIT_DELAYED_WORK(&intel_dp->edp_psr_delayed_normal_work,
>> +                       intel_edp_psr_delayed_normal_work);
>> +     mutex_init(&intel_dp->psr_exit_mutex);
>> +
>>       intel_dp->psr_setup_done = true;
>>  }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 0f52362..e47f3f3 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -499,6 +499,8 @@ struct intel_dp {
>>       int backlight_off_delay;
>>       struct delayed_work panel_vdd_work;
>>       bool want_panel_vdd;
>> +     struct delayed_work edp_psr_delayed_normal_work;
>> +     struct mutex psr_exit_mutex;
>>       bool psr_setup_done;
>>       struct intel_connector *attached_connector;
>>  };
>> @@ -839,5 +841,6 @@ extern void intel_edp_psr_enable(struct intel_dp *intel_dp);
>>  extern void intel_edp_psr_disable(struct intel_dp *intel_dp);
>>  extern void intel_edp_psr_update(struct drm_device *dev);
>>  extern bool intel_edp_is_psr_enabled(struct drm_device *dev);
>> +extern void intel_edp_psr_force_exit(struct drm_device *dev);
>>
>>  #endif /* __INTEL_DRV_H__ */
>> --
>> 1.8.1.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[PATCH 11/11] drm/i915: Hook PSR functionality

[Rodrigo Vivi]

PSR must be enabled after transcoder and port are running.
And it is only available for HSW.

v2: move enable/disable to intel_ddi
v3: The spec suggests PSR should be disabled even before backlight (by pzanoni)
v4: also disabling and enabling whenever panel is disabled/enabled.
v5: make it last patch to avoid breaking whenever bisecting. So calling for
    update and force exit came to this patch along with enable/disable calls.
v6: Remove unused and unecessary psr_enable/disable calls, as notice by Paulo.

CC: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_gem.c      | 2 ++
 drivers/gpu/drm/i915/intel_ddi.c     | 2 ++
 drivers/gpu/drm/i915/intel_display.c | 1 +
 3 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 46bf7e3..703bc69 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3758,6 +3758,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
 		goto unlock;
 	}
 
+	intel_edp_psr_force_exit(dev);
+
 	/* Count all active objects as busy, even if they are currently not used
 	 * by the gpu. Users of this interface expect objects to eventually
 	 * become non-busy without any further actions, therefore emit any
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 324211a..4211925 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1117,6 +1117,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
 			intel_dp_stop_link_train(intel_dp);
 
 		ironlake_edp_backlight_on(intel_dp);
+		intel_edp_psr_enable(intel_dp);
 	}
 
 	if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP) {
@@ -1147,6 +1148,7 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
 	if (type == INTEL_OUTPUT_EDP) {
 		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 
+		intel_edp_psr_disable(intel_dp);
 		ironlake_edp_backlight_off(intel_dp);
 	}
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 10a3629..eb4e49b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2272,6 +2272,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 	}
 
 	intel_update_fbc(dev);
+	intel_edp_psr_update(dev);
 	mutex_unlock(&dev->struct_mutex);
 
 	intel_crtc_update_sarea_pos(crtc, x, y);
-- 
1.7.11.7

Re: [PATCH 11/11] drm/i915: Hook PSR functionality

[Daniel Vetter]

On Thu, Jul 11, 2013 at 06:45:05PM -0300, Rodrigo Vivi wrote:
> PSR must be enabled after transcoder and port are running.
> And it is only available for HSW.
> 
> v2: move enable/disable to intel_ddi
> v3: The spec suggests PSR should be disabled even before backlight (by pzanoni)
> v4: also disabling and enabling whenever panel is disabled/enabled.
> v5: make it last patch to avoid breaking whenever bisecting. So calling for
>     update and force exit came to this patch along with enable/disable calls.
> v6: Remove unused and unecessary psr_enable/disable calls, as notice by Paulo.
> 
> CC: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

Ok, I've slurped this series in with the few bikesheds from Paulo applied
and two patches not merged:
- The userspace interface part, since I don't want to commit yet to an
  interface as long as things are unclear.
- And the invalidation part since imo that parts isn't properly thought
  through yet. See my other mail to Chris' RFC for ideas.

Now on the topic fbc, psr and frontbuffer rendering:

Now that I've appeased our dear PDT it's time to stop building castles on
sand imo. Items to pay back a bit of the technical dept:

- Untangle the "is fbc/psr" allowed mess. Imo we should add more static
  (i.e. only changes at modesets) information to the pipe config and track
  dynamic reasons for disabling fbc/psr somewhere in the crtc. Also this
  way update_fbc/psr would only need to check dynamic state and more
  important would never need to frob the basic setup (like reallocating
  the compressed buffer and similar things).

- Implement precise frontbuffer rendering tracking and disabling of
  fbc/psr for legacy userspace and rip out the hw tracking. If we have to
  add tons of workarounds (like for fbc), have performance this or just
  can't use it in a bunch of cases (sprites for psr) we might as well just
  track everything in the kernel.

- Testcases. With pipe CRC checksums we should be able to make sure that
  the frontbuffer invalidation actually happens. And if we do it all in
  the kernel the difference should be all-or-nothing, so much easier to
  test than with hw tracking where sometimes something seems to slip
  through.

- low-level improvements. I've only really reviewed the fbc code in-depth
  and discussed a few things with Art, but there's definitely a few things
  we need to do. One of the important things imo is to enable fbc
  everywhere we can to have as wide test coverage as possible for this
  feature.

I'll block future hw enabling in this area on the above list (i.e. vlv psr
or patches for -internal). I'll reconsider my stance if e.g. vlv psr is
used as a demonstration vehicle for the refactored psr tracking. But since
fbc can be used on many more machines (even desktops and apparently even
when other pipes are enabled) I think we should push that forward first
and use fbc to create solid tests and interfaces for userspace&kernel to
cooperate on frontbuffer rendering.

Cheers, Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c      | 2 ++
>  drivers/gpu/drm/i915/intel_ddi.c     | 2 ++
>  drivers/gpu/drm/i915/intel_display.c | 1 +
>  3 files changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 46bf7e3..703bc69 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3758,6 +3758,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
>  		goto unlock;
>  	}
>  
> +	intel_edp_psr_force_exit(dev);
> +
>  	/* Count all active objects as busy, even if they are currently not used
>  	 * by the gpu. Users of this interface expect objects to eventually
>  	 * become non-busy without any further actions, therefore emit any
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 324211a..4211925 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1117,6 +1117,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
>  			intel_dp_stop_link_train(intel_dp);
>  
>  		ironlake_edp_backlight_on(intel_dp);
> +		intel_edp_psr_enable(intel_dp);
>  	}
>  
>  	if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP) {
> @@ -1147,6 +1148,7 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
>  	if (type == INTEL_OUTPUT_EDP) {
>  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  
> +		intel_edp_psr_disable(intel_dp);
>  		ironlake_edp_backlight_off(intel_dp);
>  	}
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 10a3629..eb4e49b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2272,6 +2272,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>  	}
>  
>  	intel_update_fbc(dev);
> +	intel_edp_psr_update(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	intel_crtc_update_sarea_pos(crtc, x, y);
> -- 
> 1.7.11.7
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 11/11] drm/i915: Hook PSR functionality

[Rodrigo Vivi]

On Thu, Jul 18, 2013 at 6:54 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Jul 11, 2013 at 06:45:05PM -0300, Rodrigo Vivi wrote:
>> PSR must be enabled after transcoder and port are running.
>> And it is only available for HSW.
>>
>> v2: move enable/disable to intel_ddi
>> v3: The spec suggests PSR should be disabled even before backlight (by pzanoni)
>> v4: also disabling and enabling whenever panel is disabled/enabled.
>> v5: make it last patch to avoid breaking whenever bisecting. So calling for
>>     update and force exit came to this patch along with enable/disable calls.
>> v6: Remove unused and unecessary psr_enable/disable calls, as notice by Paulo.
>>
>> CC: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>
> Ok, I've slurped this series in with the few bikesheds from Paulo applied
> and two patches not merged:
> - The userspace interface part, since I don't want to commit yet to an
>   interface as long as things are unclear.
> - And the invalidation part since imo that parts isn't properly thought
>   through yet. See my other mail to Chris' RFC for ideas.

Thank you very much.

>
> Now on the topic fbc, psr and frontbuffer rendering:
>
> Now that I've appeased our dear PDT it's time to stop building castles on
> sand imo.

What about psr at Baytrail? :/

> Items to pay back a bit of the technical dept:
>
> - Untangle the "is fbc/psr" allowed mess. Imo we should add more static
>   (i.e. only changes at modesets) information to the pipe config and track
>   dynamic reasons for disabling fbc/psr somewhere in the crtc. Also this
>   way update_fbc/psr would only need to check dynamic state and more
>   important would never need to frob the basic setup (like reallocating
>   the compressed buffer and similar things).
>
> - Implement precise frontbuffer rendering tracking and disabling of
>   fbc/psr for legacy userspace and rip out the hw tracking. If we have to
>   add tons of workarounds (like for fbc), have performance this or just
>   can't use it in a bunch of cases (sprites for psr) we might as well just
>   track everything in the kernel.
>
> - Testcases. With pipe CRC checksums we should be able to make sure that
>   the frontbuffer invalidation actually happens. And if we do it all in
>   the kernel the difference should be all-or-nothing, so much easier to
>   test than with hw tracking where sometimes something seems to slip
>   through.
>
> - low-level improvements. I've only really reviewed the fbc code in-depth
>   and discussed a few things with Art, but there's definitely a few things
>   we need to do. One of the important things imo is to enable fbc
>   everywhere we can to have as wide test coverage as possible for this
>   feature.
>
> I'll block future hw enabling in this area on the above list (i.e. vlv psr
> or patches for -internal). I'll reconsider my stance if e.g. vlv psr is
> used as a demonstration vehicle for the refactored psr tracking. But since
> fbc can be used on many more machines (even desktops and apparently even
> when other pipes are enabled) I think we should push that forward first
> and use fbc to create solid tests and interfaces for userspace&kernel to
> cooperate on frontbuffer rendering.

I fully agree with you. I'm going to start playing with fbc and psr at
pipe_config, etc and check that discussion with art and try other
changes in fbc.
I'm just afraid the pressure for psr-vlv will be big as well.

Anyway, I give up on those 2 patches you didn't accepted because I
think this other things have more priority compared to xdm/kde bug.
Since it is disabled by default it won't cause troubles for kde users
and it is working well on gnome and unity that by any chance has a hsw
with edp that supports psr and use i915.enable_psr=1 parameter :/

>
> Cheers, Daniel
>
>> ---
>>  drivers/gpu/drm/i915/i915_gem.c      | 2 ++
>>  drivers/gpu/drm/i915/intel_ddi.c     | 2 ++
>>  drivers/gpu/drm/i915/intel_display.c | 1 +
>>  3 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 46bf7e3..703bc69 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -3758,6 +3758,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
>>               goto unlock;
>>       }
>>
>> +     intel_edp_psr_force_exit(dev);
>> +
>>       /* Count all active objects as busy, even if they are currently not used
>>        * by the gpu. Users of this interface expect objects to eventually
>>        * become non-busy without any further actions, therefore emit any
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 324211a..4211925 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1117,6 +1117,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
>>                       intel_dp_stop_link_train(intel_dp);
>>
>>               ironlake_edp_backlight_on(intel_dp);
>> +             intel_edp_psr_enable(intel_dp);
>>       }
>>
>>       if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP) {
>> @@ -1147,6 +1148,7 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
>>       if (type == INTEL_OUTPUT_EDP) {
>>               struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>>
>> +             intel_edp_psr_disable(intel_dp);
>>               ironlake_edp_backlight_off(intel_dp);
>>       }
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 10a3629..eb4e49b 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2272,6 +2272,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>>       }
>>
>>       intel_update_fbc(dev);
>> +     intel_edp_psr_update(dev);
>>       mutex_unlock(&dev->struct_mutex);
>>
>>       intel_crtc_update_sarea_pos(crtc, x, y);
>> --
>> 1.7.11.7
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Re: [PATCH 00/11] Enable PSR on Haswell.

[Shobhit Kumar]

On Friday 12 July 2013 03:14 AM, Rodrigo Vivi wrote:
> I'm resending full series again because after accepting most of suggestions
> and rebasing again on drm-intel-nightly most of patches got some kind of
> conflict so the full series is here again.
>
> First 3 patches on this series are already reviewed and I'd be glad if they
> were merged asap to avoid future conflicts. This patches at least allows
> people to know if they have psr panel or not.
>
> For the rest I accepted most of suggestions and explained on previous emails
> the ones I didn't accepted and why. However even the ones I didn't accepted
> I tested and verified that they caused some kind of issue.
>
> This version is working very fine for a long time in my machine. I'd appreciate if you could merge everything since now psr is disabled by default by kernel flag. So I'm 100% sure that this series won't cause any kind of regression for any user.
>
> I understand that it would be good to deliver psr enabled by default however I'm changing this default behaviour because I'm sure that PSR will cause regression without userspace (DDX) help when using kde and xdm.
>
> Thanks in advance,
> Rodrigo.
>
> Rodrigo Vivi (9):
>    drm/i915: split aux_clock_divider logic in a separated function for
>      reuse.
>    drm/i915: Enable/Disable PSR
>    drm/i915: Added debugfs support for PSR Status
>    drm/i915: Match all PSR mode entry conditions before enabling it.
>    drm/i915: add update function to disable/enable-back PSR
>    drm/intel: add enable_psr module option and disable psr by default
>    drm/i915: Adding global I915_PARAM for PSR ENABLED.
>    drm/i915: Add functions to force psr exit
>    drm/i915: Hook PSR functionality
>
> Shobhit Kumar (2):
>    drm: Added SDP and VSC structures for handling PSR for eDP
>    drm/i915: Read the EDP DPCD and PSR Capability
>
>   drivers/gpu/drm/i915/i915_debugfs.c  | 128 ++++++++++++
>   drivers/gpu/drm/i915/i915_dma.c      |   3 +
>   drivers/gpu/drm/i915/i915_drv.c      |   4 +
>   drivers/gpu/drm/i915/i915_drv.h      |  15 ++
>   drivers/gpu/drm/i915/i915_gem.c      |   2 +
>   drivers/gpu/drm/i915/i915_reg.h      |  74 +++++++
>   drivers/gpu/drm/i915/intel_ddi.c     |   2 +
>   drivers/gpu/drm/i915/intel_display.c |   1 +
>   drivers/gpu/drm/i915/intel_dp.c      | 373 ++++++++++++++++++++++++++++++++---
>   drivers/gpu/drm/i915/intel_drv.h     |  10 +
>   include/drm/drm_dp_helper.h          |  31 ++-
>   include/uapi/drm/i915_drm.h          |   1 +
>   12 files changed, 618 insertions(+), 26 deletions(-)
>

All looks good from code point of view. Not yet tested on a HSW system 
as I do not have one right now.

Reviewed-by: Shobhit Kumar <shobhit.kumar@intel.com>