[PATCH 1/6] drm/i915/psr: Nuke aux_frame_sync

[PATCH 1/6] drm/i915/psr: Nuke aux_frame_sync

[José Roberto de Souza]

PSR2 selective update requires aux frame sync(even though we don't
support it in i915) and do not makes sense active PSR2 to only do
full screen updates aka PSR1.
Having aux_frame_sync flag could cause it be set to true even when
the PSR1 is being used, see intel_psr2_config_valid().

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 -
 drivers/gpu/drm/i915/intel_psr.c | 10 +++++-----
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e27ba8fb64e6..8a584273f897 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -602,7 +602,6 @@ struct i915_psr {
 	struct delayed_work work;
 	unsigned busy_frontbuffer_bits;
 	bool psr2_support;
-	bool aux_frame_sync;
 	bool link_standby;
 	bool y_cord_support;
 	bool colorimetry_support;
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 317cb4a12693..9811f5f0bc75 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -144,9 +144,9 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
 				      DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
 				      &frame_sync_cap) != 1)
 			frame_sync_cap = 0;
-		dev_priv->psr.aux_frame_sync = frame_sync_cap & DP_AUX_FRAME_SYNC_CAP;
+		frame_sync_cap = (frame_sync_cap & DP_AUX_FRAME_SYNC_CAP);
 		/* PSR2 needs frame sync as well */
-		dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
+		dev_priv->psr.psr2_support = frame_sync_cap;
 		DRM_DEBUG_KMS("PSR2 %s on sink",
 			      dev_priv->psr.psr2_support ? "supported" : "not supported");
 
@@ -269,7 +269,7 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
 	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
 
 	/* Enable AUX frame sync at sink */
-	if (dev_priv->psr.aux_frame_sync)
+	if (dev_priv->psr.psr2_support)
 		drm_dp_dpcd_writeb(&intel_dp->aux,
 				DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
 				DP_AUX_FRAME_SYNC_ENABLE);
@@ -714,7 +714,7 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
 		i915_reg_t psr_status;
 		u32 psr_status_mask;
 
-		if (dev_priv->psr.aux_frame_sync)
+		if (dev_priv->psr.psr2_support)
 			drm_dp_dpcd_writeb(&intel_dp->aux,
 					DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
 					0);
@@ -862,7 +862,7 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
 		return;
 
 	if (HAS_DDI(dev_priv)) {
-		if (dev_priv->psr.aux_frame_sync)
+		if (dev_priv->psr.psr2_support)
 			drm_dp_dpcd_writeb(&intel_dp->aux,
 					DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
 					0);
-- 
2.16.2

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

[PATCH 2/6] drm/i915/psr: Tie PSR2 support to Y coordinate requirement in intel_psr_init_dpcd()

[José Roberto de Souza]

Move to only one place the sink requirements that the actual driver
needs to enable PSR2.

Also intel_psr2_config_valid() is called every time the crtc config
is computed, wasting some time every time it was checking for
Y coordinate requirement.

This allow us to nuke y_cord_support and some of VSC setup code that
was handling a scenario that would never happen(PSR2 without Y
coordinate).

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 -
 drivers/gpu/drm/i915/intel_psr.c | 55 ++++++++++++++++++++--------------------
 2 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8a584273f897..d4bc8d18f56c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -603,7 +603,6 @@ struct i915_psr {
 	unsigned busy_frontbuffer_bits;
 	bool psr2_support;
 	bool link_standby;
-	bool y_cord_support;
 	bool colorimetry_support;
 	bool alpm;
 	bool has_hw_tracking;
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 9811f5f0bc75..62d97d5576d1 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -93,7 +93,7 @@ static void psr_aux_io_power_put(struct intel_dp *intel_dp)
 	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
 }
 
-static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
+static bool intel_dp_get_y_coord_required(struct intel_dp *intel_dp)
 {
 	uint8_t psr_caps = 0;
 
@@ -137,22 +137,38 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
 
 	if (INTEL_GEN(dev_priv) >= 9 &&
 	    (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) {
-		uint8_t frame_sync_cap;
+		uint8_t frame_sync_cap, y_coord_req;
 
 		dev_priv->psr.sink_support = true;
+
+		/* PSR2 needs frame sync to do selective updates */
 		if (drm_dp_dpcd_readb(&intel_dp->aux,
 				      DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
 				      &frame_sync_cap) != 1)
 			frame_sync_cap = 0;
 		frame_sync_cap = (frame_sync_cap & DP_AUX_FRAME_SYNC_CAP);
-		/* PSR2 needs frame sync as well */
-		dev_priv->psr.psr2_support = frame_sync_cap;
-		DRM_DEBUG_KMS("PSR2 %s on sink",
-			      dev_priv->psr.psr2_support ? "supported" : "not supported");
+
+		/*
+		 * FIXME: Enable PSR2 only for y-coordinate PSR2 panels
+		 * After GTC implementation, remove this restriction.
+		 *
+		 * All panels that supports PSR version 3 that is
+		 * PSR2 + Y-coordinate support can handle Y coordinates in VSC
+		 * but we are only sure that it is going to be used when
+		 * required by the panel. This way panel is capable to do
+		 * selective update even without a valid aux frame sync.
+		 */
+		y_coord_req = intel_dp_get_y_coord_required(intel_dp);
+
+		dev_priv->psr.psr2_support = frame_sync_cap && y_coord_req;
+		if (dev_priv->psr.psr2_support)
+			DRM_DEBUG_KMS("PSR2 supported on sink\n");
+		else
+			DRM_DEBUG_KMS("PSR2 not supported on sink"
+				      "(frame sync: %d Y-coord required: %d)\n",
+				      frame_sync_cap, y_coord_req);
 
 		if (dev_priv->psr.psr2_support) {
-			dev_priv->psr.y_cord_support =
-				intel_dp_get_y_cord_status(intel_dp);
 			dev_priv->psr.colorimetry_support =
 				intel_dp_get_colorimetry_status(intel_dp);
 			dev_priv->psr.alpm =
@@ -198,16 +214,12 @@ static void hsw_psr_setup_vsc(struct intel_dp *intel_dp,
 		memset(&psr_vsc, 0, sizeof(psr_vsc));
 		psr_vsc.sdp_header.HB0 = 0;
 		psr_vsc.sdp_header.HB1 = 0x7;
-		if (dev_priv->psr.colorimetry_support &&
-		    dev_priv->psr.y_cord_support) {
+		if (dev_priv->psr.colorimetry_support) {
 			psr_vsc.sdp_header.HB2 = 0x5;
 			psr_vsc.sdp_header.HB3 = 0x13;
-		} else if (dev_priv->psr.y_cord_support) {
+		} else {
 			psr_vsc.sdp_header.HB2 = 0x4;
 			psr_vsc.sdp_header.HB3 = 0xe;
-		} else {
-			psr_vsc.sdp_header.HB2 = 0x3;
-			psr_vsc.sdp_header.HB3 = 0xc;
 		}
 	} else {
 		/* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */
@@ -478,15 +490,6 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
 		return false;
 	}
 
-	/*
-	 * FIXME:enable psr2 only for y-cordinate psr2 panels
-	 * After gtc implementation , remove this restriction.
-	 */
-	if (!dev_priv->psr.y_cord_support) {
-		DRM_DEBUG_KMS("PSR2 not enabled, panel does not support Y coordinate\n");
-		return false;
-	}
-
 	return true;
 }
 
@@ -586,14 +589,12 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
 	struct drm_device *dev = dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
-	u32 chicken;
 
 	psr_aux_io_power_get(intel_dp);
 
 	if (dev_priv->psr.psr2_support) {
-		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
-		if (dev_priv->psr.y_cord_support)
-			chicken |= PSR2_ADD_VERTICAL_LINE_COUNT;
+		u32 chicken = PSR2_VSC_ENABLE_PROG_HEADER
+			      | PSR2_ADD_VERTICAL_LINE_COUNT;
 		I915_WRITE(CHICKEN_TRANS(cpu_transcoder), chicken);
 
 		I915_WRITE(EDP_PSR_DEBUG,
-- 
2.16.2

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

[PATCH 3/6] drm/i915/psr: Enable Y-coordinate support in source

[José Roberto de Souza]

We are requiring that sink requires Y-coordinate but we are not
sending it in the main-link.
Even if hardware tracking isn't good enough it will not cause
any more issues enabling it.

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index a15db41a208a..e9fc1722c0fb 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4132,6 +4132,8 @@ enum {
 #define EDP_PSR2_CTL			_MMIO(0x6f900)
 #define   EDP_PSR2_ENABLE		(1<<31)
 #define   EDP_SU_TRACK_ENABLE		(1<<30)
+#define   EDP_Y_COORDINATE_VALID	(1<<26)
+#define   EDP_Y_COORDINATE_ENABLE	(1<<25)
 #define   EDP_MAX_SU_DISABLE_TIME(t)	((t)<<20)
 #define   EDP_MAX_SU_DISABLE_TIME_MASK	(0x1f<<20)
 #define   EDP_PSR2_TP2_TIME_500		(0<<8)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 62d97d5576d1..c9da1390a727 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -416,8 +416,8 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
 	/* FIXME: selective update is probably totally broken because it doesn't
 	 * mesh at all with our frontbuffer tracking. And the hw alone isn't
 	 * good enough. */
-	val |= EDP_PSR2_ENABLE |
-		EDP_SU_TRACK_ENABLE;
+	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
+	val |= EDP_Y_COORDINATE_VALID | EDP_Y_COORDINATE_ENABLE;
 
 	if (drm_dp_dpcd_readb(&intel_dp->aux,
 				DP_SYNCHRONIZATION_LATENCY_IN_SINK,
-- 
2.16.2

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

[PATCH 4/6] drm/i915/psr: Do not override PSR2 sink support

[José Roberto de Souza]

Sink can support our PSR2 requirements but userspace can request
a resolution that PSR2 hardware do not support, in this case it
was overwritten the PSR2 sink support.
Adding another flag here, this way if requested resolution changed
to a value that PSR2 hardware can handle, PSR2 can be enabled.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 972014b2497d..aa5918583df8 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2571,7 +2571,7 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 		   yesno(work_busy(&dev_priv->psr.work.work)));
 
 	if (HAS_DDI(dev_priv)) {
-		if (dev_priv->psr.psr2_support)
+		if (dev_priv->psr.psr2_enabled)
 			enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
 		else
 			enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
@@ -2619,7 +2619,7 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 
 		seq_printf(m, "Performance_Counter: %u\n", psrperf);
 	}
-	if (dev_priv->psr.psr2_support) {
+	if (dev_priv->psr.psr2_enabled) {
 		u32 psr2 = I915_READ(EDP_PSR2_STATUS);
 
 		seq_printf(m, "EDP_PSR2_STATUS: %x [%s]\n",
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d4bc8d18f56c..d4475196f78a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -601,11 +601,12 @@ struct i915_psr {
 	bool active;
 	struct delayed_work work;
 	unsigned busy_frontbuffer_bits;
-	bool psr2_support;
+	bool sink_psr2_support;
 	bool link_standby;
 	bool colorimetry_support;
 	bool alpm;
 	bool has_hw_tracking;
+	bool psr2_enabled;
 
 	void (*enable_source)(struct intel_dp *,
 			      const struct intel_crtc_state *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index c9da1390a727..4cb613855c20 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -160,15 +160,15 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
 		 */
 		y_coord_req = intel_dp_get_y_coord_required(intel_dp);
 
-		dev_priv->psr.psr2_support = frame_sync_cap && y_coord_req;
-		if (dev_priv->psr.psr2_support)
+		dev_priv->psr.sink_psr2_support = frame_sync_cap && y_coord_req;
+		if (dev_priv->psr.sink_psr2_support)
 			DRM_DEBUG_KMS("PSR2 supported on sink\n");
 		else
 			DRM_DEBUG_KMS("PSR2 not supported on sink"
 				      "(frame sync: %d Y-coord required: %d)\n",
 				      frame_sync_cap, y_coord_req);
 
-		if (dev_priv->psr.psr2_support) {
+		if (dev_priv->psr.sink_psr2_support) {
 			dev_priv->psr.colorimetry_support =
 				intel_dp_get_colorimetry_status(intel_dp);
 			dev_priv->psr.alpm =
@@ -209,7 +209,7 @@ static void hsw_psr_setup_vsc(struct intel_dp *intel_dp,
 	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
 	struct edp_vsc_psr psr_vsc;
 
-	if (dev_priv->psr.psr2_support) {
+	if (dev_priv->psr.psr2_enabled) {
 		/* Prepare VSC Header for SU as per EDP 1.4 spec, Table 6.11 */
 		memset(&psr_vsc, 0, sizeof(psr_vsc));
 		psr_vsc.sdp_header.HB0 = 0;
@@ -281,12 +281,12 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
 	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
 
 	/* Enable AUX frame sync at sink */
-	if (dev_priv->psr.psr2_support)
+	if (dev_priv->psr.psr2_enabled)
 		drm_dp_dpcd_writeb(&intel_dp->aux,
 				DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
 				DP_AUX_FRAME_SYNC_ENABLE);
 	/* Enable ALPM at sink for psr2 */
-	if (dev_priv->psr.psr2_support && dev_priv->psr.alpm)
+	if (dev_priv->psr.psr2_enabled && dev_priv->psr.alpm)
 		drm_dp_dpcd_writeb(&intel_dp->aux,
 				DP_RECEIVER_ALPM_CONFIG,
 				DP_ALPM_ENABLE);
@@ -452,7 +452,7 @@ static void hsw_psr_activate(struct intel_dp *intel_dp)
 	 */
 
 	/* psr1 and psr2 are mutually exclusive.*/
-	if (dev_priv->psr.psr2_support)
+	if (dev_priv->psr.psr2_enabled)
 		hsw_activate_psr2(intel_dp);
 	else
 		hsw_activate_psr1(intel_dp);
@@ -472,7 +472,7 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
 	 * dynamically during PSR enable, and extracted from sink
 	 * caps during eDP detection.
 	 */
-	if (!dev_priv->psr.psr2_support)
+	if (!dev_priv->psr.sink_psr2_support)
 		return false;
 
 	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
@@ -571,7 +571,7 @@ static void intel_psr_activate(struct intel_dp *intel_dp)
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
-	if (dev_priv->psr.psr2_support)
+	if (dev_priv->psr.psr2_enabled)
 		WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
 	else
 		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
@@ -592,7 +592,7 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
 
 	psr_aux_io_power_get(intel_dp);
 
-	if (dev_priv->psr.psr2_support) {
+	if (dev_priv->psr.psr2_enabled) {
 		u32 chicken = PSR2_VSC_ENABLE_PROG_HEADER
 			      | PSR2_ADD_VERTICAL_LINE_COUNT;
 		I915_WRITE(CHICKEN_TRANS(cpu_transcoder), chicken);
@@ -645,7 +645,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,
 		goto unlock;
 	}
 
-	dev_priv->psr.psr2_support = crtc_state->has_psr2;
+	dev_priv->psr.psr2_enabled = crtc_state->has_psr2;
 	dev_priv->psr.busy_frontbuffer_bits = 0;
 
 	dev_priv->psr.setup_vsc(intel_dp, crtc_state);
@@ -715,12 +715,12 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
 		i915_reg_t psr_status;
 		u32 psr_status_mask;
 
-		if (dev_priv->psr.psr2_support)
+		if (dev_priv->psr.psr2_enabled)
 			drm_dp_dpcd_writeb(&intel_dp->aux,
 					DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
 					0);
 
-		if (dev_priv->psr.psr2_support) {
+		if (dev_priv->psr.psr2_enabled) {
 			psr_status = EDP_PSR2_STATUS;
 			psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
 
@@ -744,7 +744,7 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
 
 		dev_priv->psr.active = false;
 	} else {
-		if (dev_priv->psr.psr2_support)
+		if (dev_priv->psr.psr2_enabled)
 			WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
 		else
 			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
@@ -804,7 +804,7 @@ static void intel_psr_work(struct work_struct *work)
 	 * and be ready for re-enable.
 	 */
 	if (HAS_DDI(dev_priv)) {
-		if (dev_priv->psr.psr2_support) {
+		if (dev_priv->psr.psr2_enabled) {
 			if (intel_wait_for_register(dev_priv,
 						    EDP_PSR2_STATUS,
 						    EDP_PSR2_STATUS_STATE_MASK,
@@ -863,11 +863,11 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
 		return;
 
 	if (HAS_DDI(dev_priv)) {
-		if (dev_priv->psr.psr2_support)
+		if (dev_priv->psr.psr2_enabled)
 			drm_dp_dpcd_writeb(&intel_dp->aux,
 					DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
 					0);
-		if (dev_priv->psr.psr2_support) {
+		if (dev_priv->psr.psr2_enabled) {
 			val = I915_READ(EDP_PSR2_CTL);
 			WARN_ON(!(val & EDP_PSR2_ENABLE));
 			I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
@@ -1036,7 +1036,7 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
 
 	/* By definition flush = invalidate + flush */
 	if (frontbuffer_bits) {
-		if (dev_priv->psr.psr2_support ||
+		if (dev_priv->psr.psr2_enabled ||
 		    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
 			intel_psr_exit(dev_priv);
 		} else {
-- 
2.16.2

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

[PATCH 5/6] drm/i915/psr: Rename intel_crtc_state has_psr to can_psr

[José Roberto de Souza]

This value is a match of hardware and sink has PSR + if it can be
enabled by the requested state, see intel_psr_compute_config().

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h |  4 ++--
 drivers/gpu/drm/i915/intel_psr.c | 12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a215aa78b0be..cccaf84415ab 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -807,8 +807,8 @@ struct intel_crtc_state {
 	struct intel_link_m_n dp_m2_n2;
 	bool has_drrs;
 
-	bool has_psr;
-	bool has_psr2;
+	bool can_psr;
+	bool can_psr2;
 
 	/*
 	 * Frequence the dpll for the port should run at. Differs from the
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 4cb613855c20..d622e37894d4 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -560,9 +560,9 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
 		return;
 	}
 
-	crtc_state->has_psr = true;
-	crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp, crtc_state);
-	DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : "");
+	crtc_state->can_psr = true;
+	crtc_state->can_psr2 = intel_psr2_config_valid(intel_dp, crtc_state);
+	DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->can_psr2 ? "2" : "");
 }
 
 static void intel_psr_activate(struct intel_dp *intel_dp)
@@ -632,7 +632,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
-	if (!crtc_state->has_psr)
+	if (!crtc_state->can_psr)
 		return;
 
 	if (WARN_ON(!CAN_PSR(dev_priv)))
@@ -645,7 +645,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,
 		goto unlock;
 	}
 
-	dev_priv->psr.psr2_enabled = crtc_state->has_psr2;
+	dev_priv->psr.psr2_enabled = crtc_state->can_psr2;
 	dev_priv->psr.busy_frontbuffer_bits = 0;
 
 	dev_priv->psr.setup_vsc(intel_dp, crtc_state);
@@ -767,7 +767,7 @@ void intel_psr_disable(struct intel_dp *intel_dp,
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
-	if (!old_crtc_state->has_psr)
+	if (!old_crtc_state->can_psr)
 		return;
 
 	if (WARN_ON(!CAN_PSR(dev_priv)))
-- 
2.16.2

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

[PATCH 6/6] drm/i915/psr: Enable aux frame sync in source

[José Roberto de Souza]

Even with GTC not enabled lets send the aux frame sync.
Hardware is going to send dummy values but this way we can get rid of
this workarround in PSR exit: 'drm/i915/psr: disable aux_frame_sync
on psr2 exit'.
Also moving the line disabling aux frame sync in sink to after report
that PSR2 has exit to avoid.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 15 ++++++---------
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index e9fc1722c0fb..5a2364656aa5 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4132,6 +4132,7 @@ enum {
 #define EDP_PSR2_CTL			_MMIO(0x6f900)
 #define   EDP_PSR2_ENABLE		(1<<31)
 #define   EDP_SU_TRACK_ENABLE		(1<<30)
+#define   EDP_AUX_FRAME_SYNC_ENABLE	(1<<27)
 #define   EDP_Y_COORDINATE_VALID	(1<<26)
 #define   EDP_Y_COORDINATE_ENABLE	(1<<25)
 #define   EDP_MAX_SU_DISABLE_TIME(t)	((t)<<20)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index d622e37894d4..7aab66b5bc91 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -418,6 +418,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
 	 * good enough. */
 	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
 	val |= EDP_Y_COORDINATE_VALID | EDP_Y_COORDINATE_ENABLE;
+	val |= EDP_AUX_FRAME_SYNC_ENABLE;
 
 	if (drm_dp_dpcd_readb(&intel_dp->aux,
 				DP_SYNCHRONIZATION_LATENCY_IN_SINK,
@@ -715,11 +716,6 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
 		i915_reg_t psr_status;
 		u32 psr_status_mask;
 
-		if (dev_priv->psr.psr2_enabled)
-			drm_dp_dpcd_writeb(&intel_dp->aux,
-					DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
-					0);
-
 		if (dev_priv->psr.psr2_enabled) {
 			psr_status = EDP_PSR2_STATUS;
 			psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
@@ -742,6 +738,11 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
 					    2000))
 			DRM_ERROR("Timed out waiting for PSR Idle State\n");
 
+		if (dev_priv->psr.psr2_enabled)
+			drm_dp_dpcd_writeb(&intel_dp->aux,
+					   DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
+					   0);
+
 		dev_priv->psr.active = false;
 	} else {
 		if (dev_priv->psr.psr2_enabled)
@@ -863,10 +864,6 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
 		return;
 
 	if (HAS_DDI(dev_priv)) {
-		if (dev_priv->psr.psr2_enabled)
-			drm_dp_dpcd_writeb(&intel_dp->aux,
-					DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
-					0);
 		if (dev_priv->psr.psr2_enabled) {
 			val = I915_READ(EDP_PSR2_CTL);
 			WARN_ON(!(val & EDP_PSR2_ENABLE));
-- 
2.16.2

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

✗ Fi.CI.BAT: failure for series starting with [1/6] drm/i915/psr: Nuke aux_frame_sync

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm/i915/psr: Nuke aux_frame_sync
URL   : https://patchwork.freedesktop.org/series/39990/
State : failure

== Summary ==

Series 39990v1 series starting with [1/6] drm/i915/psr: Nuke aux_frame_sync
https://patchwork.freedesktop.org/api/1.0/series/39990/revisions/1/mbox/

---- Possible new issues:

Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> FAIL       (fi-skl-6770hq)

---- Known issues:

Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                fail       -> PASS       (fi-gdg-551) fdo#102575
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                pass       -> INCOMPLETE (fi-bxt-dsi) fdo#103927
Test prime_vgem:
        Subgroup basic-fence-flip:
                pass       -> FAIL       (fi-ilk-650) fdo#104008

fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008

fi-bdw-5557u     total:285  pass:264  dwarn:0   dfail:0   fail:0   skip:21  time:432s
fi-bdw-gvtdvm    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:441s
fi-blb-e6850     total:285  pass:220  dwarn:1   dfail:0   fail:0   skip:64  time:381s
fi-bsw-n3050     total:285  pass:239  dwarn:0   dfail:0   fail:0   skip:46  time:539s
fi-bwr-2160      total:285  pass:180  dwarn:0   dfail:0   fail:0   skip:105 time:299s
fi-bxt-dsi       total:243  pass:216  dwarn:0   dfail:0   fail:0   skip:26 
fi-byt-j1900     total:285  pass:250  dwarn:0   dfail:0   fail:0   skip:35  time:516s
fi-byt-n2820     total:285  pass:246  dwarn:0   dfail:0   fail:0   skip:39  time:506s
fi-cfl-8700k     total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:408s
fi-cfl-s2        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:589s
fi-cfl-u         total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:513s
fi-cnl-y3        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:592s
fi-elk-e7500     total:285  pass:226  dwarn:0   dfail:0   fail:0   skip:59  time:427s
fi-gdg-551       total:285  pass:177  dwarn:0   dfail:0   fail:0   skip:108 time:317s
fi-glk-1         total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:538s
fi-hsw-4770      total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:407s
fi-ilk-650       total:285  pass:224  dwarn:0   dfail:0   fail:1   skip:60  time:425s
fi-ivb-3520m     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:475s
fi-ivb-3770      total:285  pass:252  dwarn:0   dfail:0   fail:0   skip:33  time:429s
fi-kbl-7500u     total:285  pass:260  dwarn:1   dfail:0   fail:0   skip:24  time:476s
fi-kbl-7567u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:468s
fi-kbl-r         total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:515s
fi-pnv-d510      total:285  pass:219  dwarn:1   dfail:0   fail:0   skip:65  time:654s
fi-skl-6260u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:442s
fi-skl-6600u     total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:527s
fi-skl-6700hq    total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:544s
fi-skl-6700k2    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:497s
fi-skl-6770hq    total:285  pass:264  dwarn:0   dfail:0   fail:1   skip:20  time:479s
fi-skl-guc       total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:429s
fi-skl-gvtdvm    total:285  pass:262  dwarn:0   dfail:0   fail:0   skip:23  time:448s
fi-snb-2520m     total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:578s
fi-snb-2600      total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:409s
Blacklisted hosts:
fi-cnl-drrs      total:285  pass:254  dwarn:3   dfail:0   fail:0   skip:28  time:533s

178cfb9373cc2bdfcb6ca73e03369d2c37cc4b58 drm-tip: 2018y-03m-14d-21h-38m-09s UTC integration manifest
f82d67643405 drm/i915/psr: Enable aux frame sync in source
5adb06aa65d7 drm/i915/psr: Rename intel_crtc_state has_psr to can_psr
d4117b9b7931 drm/i915/psr: Do not override PSR2 sink support
dd78cdfeb5ba drm/i915/psr: Enable Y-coordinate support in source
0496ade25ff9 drm/i915/psr: Tie PSR2 support to Y coordinate requirement in intel_psr_init_dpcd()
742aaf18896d drm/i915/psr: Nuke aux_frame_sync

== Logs ==

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

Re: [PATCH 3/6] drm/i915/psr: Enable Y-coordinate support in source

[Rodrigo Vivi]

drm/i915/cnl:....

On Wed, Mar 14, 2018 at 03:36:14PM -0700, José Roberto de Souza wrote:
> We are requiring that sink requires Y-coordinate but we are not
> sending it in the main-link.

Also add on CNL here

> Even if hardware tracking isn't good enough it will not cause
> any more issues enabling it.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  | 2 ++
>  drivers/gpu/drm/i915/intel_psr.c | 4 ++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index a15db41a208a..e9fc1722c0fb 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4132,6 +4132,8 @@ enum {
>  #define EDP_PSR2_CTL			_MMIO(0x6f900)
>  #define   EDP_PSR2_ENABLE		(1<<31)
>  #define   EDP_SU_TRACK_ENABLE		(1<<30)
> +#define   EDP_Y_COORDINATE_VALID	(1<<26)
> +#define   EDP_Y_COORDINATE_ENABLE	(1<<25)

probably good add CNL_ prefix on these bits...

>  #define   EDP_MAX_SU_DISABLE_TIME(t)	((t)<<20)
>  #define   EDP_MAX_SU_DISABLE_TIME_MASK	(0x1f<<20)
>  #define   EDP_PSR2_TP2_TIME_500		(0<<8)
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 62d97d5576d1..c9da1390a727 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -416,8 +416,8 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>  	/* FIXME: selective update is probably totally broken because it doesn't
>  	 * mesh at all with our frontbuffer tracking. And the hw alone isn't
>  	 * good enough. */
> -	val |= EDP_PSR2_ENABLE |
> -		EDP_SU_TRACK_ENABLE;
> +	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
> +	val |= EDP_Y_COORDINATE_VALID | EDP_Y_COORDINATE_ENABLE;

if (INTEL_GEN(dev_priv) >= 10)
	val |= EDP_Y_COORDINATE_VALID | EDP_Y_COORDINATE_ENABLE;

since those bits were reserved before CNL.

With those changes:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

>  
>  	if (drm_dp_dpcd_readb(&intel_dp->aux,
>  				DP_SYNCHRONIZATION_LATENCY_IN_SINK,
> -- 
> 2.16.2
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm/i915/psr: Nuke aux_frame_sync

[Rodrigo Vivi]

On Wed, Mar 14, 2018 at 03:36:12PM -0700, José Roberto de Souza wrote:
> PSR2 selective update requires aux frame sync(even though we don't
> support it in i915) and do not makes sense active PSR2 to only do
> full screen updates aka PSR1.
> Having aux_frame_sync flag could cause it be set to true even when
> the PSR1 is being used, see intel_psr2_config_valid().
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 -
>  drivers/gpu/drm/i915/intel_psr.c | 10 +++++-----
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e27ba8fb64e6..8a584273f897 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -602,7 +602,6 @@ struct i915_psr {
>  	struct delayed_work work;
>  	unsigned busy_frontbuffer_bits;
>  	bool psr2_support;
> -	bool aux_frame_sync;
>  	bool link_standby;
>  	bool y_cord_support;
>  	bool colorimetry_support;
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 317cb4a12693..9811f5f0bc75 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -144,9 +144,9 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>  				      DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
>  				      &frame_sync_cap) != 1)
>  			frame_sync_cap = 0;
> -		dev_priv->psr.aux_frame_sync = frame_sync_cap & DP_AUX_FRAME_SYNC_CAP;
> +		frame_sync_cap = (frame_sync_cap & DP_AUX_FRAME_SYNC_CAP);
>  		/* PSR2 needs frame sync as well */
> -		dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
> +		dev_priv->psr.psr2_support = frame_sync_cap;
>  		DRM_DEBUG_KMS("PSR2 %s on sink",
>  			      dev_priv->psr.psr2_support ? "supported" : "not supported");
>  
> @@ -269,7 +269,7 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
>  	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
>  
>  	/* Enable AUX frame sync at sink */
> -	if (dev_priv->psr.aux_frame_sync)
> +	if (dev_priv->psr.psr2_support)
>  		drm_dp_dpcd_writeb(&intel_dp->aux,
>  				DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
>  				DP_AUX_FRAME_SYNC_ENABLE);
> @@ -714,7 +714,7 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
>  		i915_reg_t psr_status;
>  		u32 psr_status_mask;
>  
> -		if (dev_priv->psr.aux_frame_sync)
> +		if (dev_priv->psr.psr2_support)
>  			drm_dp_dpcd_writeb(&intel_dp->aux,
>  					DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
>  					0);
> @@ -862,7 +862,7 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
>  		return;
>  
>  	if (HAS_DDI(dev_priv)) {
> -		if (dev_priv->psr.aux_frame_sync)
> +		if (dev_priv->psr.psr2_support)
>  			drm_dp_dpcd_writeb(&intel_dp->aux,
>  					DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
>  					0);
> -- 
> 2.16.2
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915/psr: Enable aux frame sync in source

[Rodrigo Vivi]

On Wed, Mar 14, 2018 at 03:36:17PM -0700, José Roberto de Souza wrote:
> Even with GTC not enabled lets send the aux frame sync.
> Hardware is going to send dummy values but this way we can get rid of
> this workarround in PSR exit: 'drm/i915/psr: disable aux_frame_sync
> on psr2 exit'.
> Also moving the line disabling aux frame sync in sink to after report
> that PSR2 has exit to avoid.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 15 ++++++---------
>  2 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index e9fc1722c0fb..5a2364656aa5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4132,6 +4132,7 @@ enum {
>  #define EDP_PSR2_CTL			_MMIO(0x6f900)
>  #define   EDP_PSR2_ENABLE		(1<<31)
>  #define   EDP_SU_TRACK_ENABLE		(1<<30)
> +#define   EDP_AUX_FRAME_SYNC_ENABLE	(1<<27)

This shows reserved here for me. I believe we should just drop this patch.

>  #define   EDP_Y_COORDINATE_VALID	(1<<26)
>  #define   EDP_Y_COORDINATE_ENABLE	(1<<25)
>  #define   EDP_MAX_SU_DISABLE_TIME(t)	((t)<<20)
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index d622e37894d4..7aab66b5bc91 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -418,6 +418,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>  	 * good enough. */
>  	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
>  	val |= EDP_Y_COORDINATE_VALID | EDP_Y_COORDINATE_ENABLE;
> +	val |= EDP_AUX_FRAME_SYNC_ENABLE;
>  
>  	if (drm_dp_dpcd_readb(&intel_dp->aux,
>  				DP_SYNCHRONIZATION_LATENCY_IN_SINK,
> @@ -715,11 +716,6 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
>  		i915_reg_t psr_status;
>  		u32 psr_status_mask;
>  
> -		if (dev_priv->psr.psr2_enabled)
> -			drm_dp_dpcd_writeb(&intel_dp->aux,
> -					DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
> -					0);
> -
>  		if (dev_priv->psr.psr2_enabled) {
>  			psr_status = EDP_PSR2_STATUS;
>  			psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> @@ -742,6 +738,11 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
>  					    2000))
>  			DRM_ERROR("Timed out waiting for PSR Idle State\n");
>  
> +		if (dev_priv->psr.psr2_enabled)
> +			drm_dp_dpcd_writeb(&intel_dp->aux,
> +					   DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
> +					   0);
> +
>  		dev_priv->psr.active = false;
>  	} else {
>  		if (dev_priv->psr.psr2_enabled)
> @@ -863,10 +864,6 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
>  		return;
>  
>  	if (HAS_DDI(dev_priv)) {
> -		if (dev_priv->psr.psr2_enabled)
> -			drm_dp_dpcd_writeb(&intel_dp->aux,
> -					DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
> -					0);
>  		if (dev_priv->psr.psr2_enabled) {
>  			val = I915_READ(EDP_PSR2_CTL);
>  			WARN_ON(!(val & EDP_PSR2_ENABLE));
> -- 
> 2.16.2
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/6] drm/i915/psr: Tie PSR2 support to Y coordinate requirement in intel_psr_init_dpcd()

[Rodrigo Vivi]

On Wed, Mar 14, 2018 at 03:36:13PM -0700, José Roberto de Souza wrote:
> Move to only one place the sink requirements that the actual driver
> needs to enable PSR2.
> 
> Also intel_psr2_config_valid() is called every time the crtc config
> is computed, wasting some time every time it was checking for
> Y coordinate requirement.
> 
> This allow us to nuke y_cord_support and some of VSC setup code that
> was handling a scenario that would never happen(PSR2 without Y
> coordinate).
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 -
>  drivers/gpu/drm/i915/intel_psr.c | 55 ++++++++++++++++++++--------------------
>  2 files changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8a584273f897..d4bc8d18f56c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -603,7 +603,6 @@ struct i915_psr {
>  	unsigned busy_frontbuffer_bits;
>  	bool psr2_support;
>  	bool link_standby;
> -	bool y_cord_support;
>  	bool colorimetry_support;
>  	bool alpm;
>  	bool has_hw_tracking;
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 9811f5f0bc75..62d97d5576d1 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -93,7 +93,7 @@ static void psr_aux_io_power_put(struct intel_dp *intel_dp)
>  	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
>  }
>  
> -static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
> +static bool intel_dp_get_y_coord_required(struct intel_dp *intel_dp)
>  {
>  	uint8_t psr_caps = 0;
>  
> @@ -137,22 +137,38 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>  
>  	if (INTEL_GEN(dev_priv) >= 9 &&
>  	    (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) {
> -		uint8_t frame_sync_cap;
> +		uint8_t frame_sync_cap, y_coord_req;
>  
>  		dev_priv->psr.sink_support = true;
> +
> +		/* PSR2 needs frame sync to do selective updates */
>  		if (drm_dp_dpcd_readb(&intel_dp->aux,
>  				      DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
>  				      &frame_sync_cap) != 1)
>  			frame_sync_cap = 0;
>  		frame_sync_cap = (frame_sync_cap & DP_AUX_FRAME_SYNC_CAP);
> -		/* PSR2 needs frame sync as well */
> -		dev_priv->psr.psr2_support = frame_sync_cap;
> -		DRM_DEBUG_KMS("PSR2 %s on sink",
> -			      dev_priv->psr.psr2_support ? "supported" : "not supported");
> +
> +		/*
> +		 * FIXME: Enable PSR2 only for y-coordinate PSR2 panels
> +		 * After GTC implementation, remove this restriction.

I believe we should remove the FIXME and just comment out that non y-coordinate
would need GTC that we currently don't implement.

later if we change our minds and decided to add that support we revisit here
but without a FIXME poluting code for something we don't plan to support for now.

> +		 *
> +		 * All panels that supports PSR version 3 that is

PSRv3? is there really something formal about that?

> +		 * PSR2 + Y-coordinate support can handle Y coordinates in VSC
> +		 * but we are only sure that it is going to be used when
> +		 * required by the panel. This way panel is capable to do
> +		 * selective update even without a valid aux frame sync.
> +		 */
> +		y_coord_req = intel_dp_get_y_coord_required(intel_dp);
> +
> +		dev_priv->psr.psr2_support = frame_sync_cap && y_coord_req;
> +		if (dev_priv->psr.psr2_support)
> +			DRM_DEBUG_KMS("PSR2 supported on sink\n");
> +		else
> +			DRM_DEBUG_KMS("PSR2 not supported on sink"
> +				      "(frame sync: %d Y-coord required: %d)\n",
> +				      frame_sync_cap, y_coord_req);
>  
>  		if (dev_priv->psr.psr2_support) {
> -			dev_priv->psr.y_cord_support =
> -				intel_dp_get_y_cord_status(intel_dp);
>  			dev_priv->psr.colorimetry_support =
>  				intel_dp_get_colorimetry_status(intel_dp);
>  			dev_priv->psr.alpm =
> @@ -198,16 +214,12 @@ static void hsw_psr_setup_vsc(struct intel_dp *intel_dp,
>  		memset(&psr_vsc, 0, sizeof(psr_vsc));
>  		psr_vsc.sdp_header.HB0 = 0;
>  		psr_vsc.sdp_header.HB1 = 0x7;
> -		if (dev_priv->psr.colorimetry_support &&
> -		    dev_priv->psr.y_cord_support) {
> +		if (dev_priv->psr.colorimetry_support) {
>  			psr_vsc.sdp_header.HB2 = 0x5;
>  			psr_vsc.sdp_header.HB3 = 0x13;
> -		} else if (dev_priv->psr.y_cord_support) {
> +		} else {
>  			psr_vsc.sdp_header.HB2 = 0x4;
>  			psr_vsc.sdp_header.HB3 = 0xe;
> -		} else {
> -			psr_vsc.sdp_header.HB2 = 0x3;
> -			psr_vsc.sdp_header.HB3 = 0xc;
>  		}
>  	} else {
>  		/* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */
> @@ -478,15 +490,6 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
>  		return false;
>  	}
>  
> -	/*
> -	 * FIXME:enable psr2 only for y-cordinate psr2 panels
> -	 * After gtc implementation , remove this restriction.
> -	 */
> -	if (!dev_priv->psr.y_cord_support) {
> -		DRM_DEBUG_KMS("PSR2 not enabled, panel does not support Y coordinate\n");
> -		return false;
> -	}
> -
>  	return true;
>  }
>  
> @@ -586,14 +589,12 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
>  	struct drm_device *dev = dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> -	u32 chicken;
>  
>  	psr_aux_io_power_get(intel_dp);
>  
>  	if (dev_priv->psr.psr2_support) {
> -		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> -		if (dev_priv->psr.y_cord_support)
> -			chicken |= PSR2_ADD_VERTICAL_LINE_COUNT;
> +		u32 chicken = PSR2_VSC_ENABLE_PROG_HEADER
> +			      | PSR2_ADD_VERTICAL_LINE_COUNT;
>  		I915_WRITE(CHICKEN_TRANS(cpu_transcoder), chicken);
>  
>  		I915_WRITE(EDP_PSR_DEBUG,
> -- 
> 2.16.2
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/6] drm/i915/psr: Tie PSR2 support to Y coordinate requirement in intel_psr_init_dpcd()

[Souza, Jose]

On Thu, 2018-03-15 at 17:35 -0700, Rodrigo Vivi wrote:
> On Wed, Mar 14, 2018 at 03:36:13PM -0700, José Roberto de Souza
> wrote:
> > Move to only one place the sink requirements that the actual driver
> > needs to enable PSR2.
> > 
> > Also intel_psr2_config_valid() is called every time the crtc config
> > is computed, wasting some time every time it was checking for
> > Y coordinate requirement.
> > 
> > This allow us to nuke y_cord_support and some of VSC setup code
> > that
> > was handling a scenario that would never happen(PSR2 without Y
> > coordinate).
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |  1 -
> >  drivers/gpu/drm/i915/intel_psr.c | 55 ++++++++++++++++++++------
> > --------------
> >  2 files changed, 28 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 8a584273f897..d4bc8d18f56c 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -603,7 +603,6 @@ struct i915_psr {
> >  	unsigned busy_frontbuffer_bits;
> >  	bool psr2_support;
> >  	bool link_standby;
> > -	bool y_cord_support;
> >  	bool colorimetry_support;
> >  	bool alpm;
> >  	bool has_hw_tracking;
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 9811f5f0bc75..62d97d5576d1 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -93,7 +93,7 @@ static void psr_aux_io_power_put(struct intel_dp
> > *intel_dp)
> >  	intel_display_power_put(dev_priv,
> > psr_aux_domain(intel_dp));
> >  }
> >  
> > -static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
> > +static bool intel_dp_get_y_coord_required(struct intel_dp
> > *intel_dp)
> >  {
> >  	uint8_t psr_caps = 0;
> >  
> > @@ -137,22 +137,38 @@ void intel_psr_init_dpcd(struct intel_dp
> > *intel_dp)
> >  
> >  	if (INTEL_GEN(dev_priv) >= 9 &&
> >  	    (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) {
> > -		uint8_t frame_sync_cap;
> > +		uint8_t frame_sync_cap, y_coord_req;
> >  
> >  		dev_priv->psr.sink_support = true;
> > +
> > +		/* PSR2 needs frame sync to do selective updates
> > */
> >  		if (drm_dp_dpcd_readb(&intel_dp->aux,
> >  				      DP_SINK_DEVICE_AUX_FRAME_SYN
> > C_CAP,
> >  				      &frame_sync_cap) != 1)
> >  			frame_sync_cap = 0;
> >  		frame_sync_cap = (frame_sync_cap &
> > DP_AUX_FRAME_SYNC_CAP);
> > -		/* PSR2 needs frame sync as well */
> > -		dev_priv->psr.psr2_support = frame_sync_cap;
> > -		DRM_DEBUG_KMS("PSR2 %s on sink",
> > -			      dev_priv->psr.psr2_support ?
> > "supported" : "not supported");
> > +
> > +		/*
> > +		 * FIXME: Enable PSR2 only for y-coordinate PSR2
> > panels
> > +		 * After GTC implementation, remove this
> > restriction.
> 
> I believe we should remove the FIXME and just comment out that non y-
> coordinate
> would need GTC that we currently don't implement.
> 
> later if we change our minds and decided to add that support we
> revisit here
> but without a FIXME poluting code for something we don't plan to
> support for now.
> 
> > +		 *
> > +		 * All panels that supports PSR version 3 that is
> 
> PSRv3? is there really something formal about that?

Yes, from eDP spec:

PSR Support and Version

00h = Panel Self Refresh Capability is not supported (default).
If PSR/PSR2 is supported, the SET_POWER_CAPABLE bit in the
EDP_GENERAL_CAPABILITY_1 register (DPCD Address 00701h, bit 7) must
be set to 1.
01h = Panel Self Refresh capability is supported, PSR version is 01h
(PSR).
02h = Panel Self Refresh with Selective Update (SU) capability is
supported,
PSR version is 02h (PSR2). Y-coordinates for SU are not supported.
Supported
by eDP v1.4 (and higher).
03h = Panel Self Refresh with SU capability and Y-coordinate supported,
PSR
version is 03h (PSR2 + Y-coordinate). Supported by eDP v1.4a (and
higher).

> 
> > +		 * PSR2 + Y-coordinate support can handle Y
> > coordinates in VSC
> > +		 * but we are only sure that it is going to be
> > used when
> > +		 * required by the panel. This way panel is
> > capable to do
> > +		 * selective update even without a valid aux frame
> > sync.
> > +		 */
> > +		y_coord_req =
> > intel_dp_get_y_coord_required(intel_dp);
> > +
> > +		dev_priv->psr.psr2_support = frame_sync_cap &&
> > y_coord_req;
> > +		if (dev_priv->psr.psr2_support)
> > +			DRM_DEBUG_KMS("PSR2 supported on sink\n");
> > +		else
> > +			DRM_DEBUG_KMS("PSR2 not supported on sink"
> > +				      "(frame sync: %d Y-coord
> > required: %d)\n",
> > +				      frame_sync_cap,
> > y_coord_req);
> >  
> >  		if (dev_priv->psr.psr2_support) {
> > -			dev_priv->psr.y_cord_support =
> > -				intel_dp_get_y_cord_status(intel_d
> > p);
> >  			dev_priv->psr.colorimetry_support =
> >  				intel_dp_get_colorimetry_status(in
> > tel_dp);
> >  			dev_priv->psr.alpm =
> > @@ -198,16 +214,12 @@ static void hsw_psr_setup_vsc(struct intel_dp
> > *intel_dp,
> >  		memset(&psr_vsc, 0, sizeof(psr_vsc));
> >  		psr_vsc.sdp_header.HB0 = 0;
> >  		psr_vsc.sdp_header.HB1 = 0x7;
> > -		if (dev_priv->psr.colorimetry_support &&
> > -		    dev_priv->psr.y_cord_support) {
> > +		if (dev_priv->psr.colorimetry_support) {
> >  			psr_vsc.sdp_header.HB2 = 0x5;
> >  			psr_vsc.sdp_header.HB3 = 0x13;
> > -		} else if (dev_priv->psr.y_cord_support) {
> > +		} else {
> >  			psr_vsc.sdp_header.HB2 = 0x4;
> >  			psr_vsc.sdp_header.HB3 = 0xe;
> > -		} else {
> > -			psr_vsc.sdp_header.HB2 = 0x3;
> > -			psr_vsc.sdp_header.HB3 = 0xc;
> >  		}
> >  	} else {
> >  		/* Prepare VSC packet as per EDP 1.3 spec, Table
> > 3.10 */
> > @@ -478,15 +490,6 @@ static bool intel_psr2_config_valid(struct
> > intel_dp *intel_dp,
> >  		return false;
> >  	}
> >  
> > -	/*
> > -	 * FIXME:enable psr2 only for y-cordinate psr2 panels
> > -	 * After gtc implementation , remove this restriction.
> > -	 */
> > -	if (!dev_priv->psr.y_cord_support) {
> > -		DRM_DEBUG_KMS("PSR2 not enabled, panel does not
> > support Y coordinate\n");
> > -		return false;
> > -	}
> > -
> >  	return true;
> >  }
> >  
> > @@ -586,14 +589,12 @@ static void hsw_psr_enable_source(struct
> > intel_dp *intel_dp,
> >  	struct drm_device *dev = dig_port->base.base.dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	enum transcoder cpu_transcoder = crtc_state-
> > >cpu_transcoder;
> > -	u32 chicken;
> >  
> >  	psr_aux_io_power_get(intel_dp);
> >  
> >  	if (dev_priv->psr.psr2_support) {
> > -		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> > -		if (dev_priv->psr.y_cord_support)
> > -			chicken |= PSR2_ADD_VERTICAL_LINE_COUNT;
> > +		u32 chicken = PSR2_VSC_ENABLE_PROG_HEADER
> > +			      | PSR2_ADD_VERTICAL_LINE_COUNT;
> >  		I915_WRITE(CHICKEN_TRANS(cpu_transcoder),
> > chicken);
> >  
> >  		I915_WRITE(EDP_PSR_DEBUG,
> > -- 
> > 2.16.2
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/6] drm/i915/psr: Tie PSR2 support to Y coordinate requirement in intel_psr_init_dpcd()

[Rodrigo Vivi]

On Thu, Mar 15, 2018 at 06:04:27PM -0700, Souza, Jose wrote:
> On Thu, 2018-03-15 at 17:35 -0700, Rodrigo Vivi wrote:
> > On Wed, Mar 14, 2018 at 03:36:13PM -0700, José Roberto de Souza
> > wrote:
> > > Move to only one place the sink requirements that the actual driver
> > > needs to enable PSR2.
> > > 
> > > Also intel_psr2_config_valid() is called every time the crtc config
> > > is computed, wasting some time every time it was checking for
> > > Y coordinate requirement.
> > > 
> > > This allow us to nuke y_cord_support and some of VSC setup code
> > > that
> > > was handling a scenario that would never happen(PSR2 without Y
> > > coordinate).
> > > 
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h  |  1 -
> > >  drivers/gpu/drm/i915/intel_psr.c | 55 ++++++++++++++++++++------
> > > --------------
> > >  2 files changed, 28 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 8a584273f897..d4bc8d18f56c 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -603,7 +603,6 @@ struct i915_psr {
> > >  	unsigned busy_frontbuffer_bits;
> > >  	bool psr2_support;
> > >  	bool link_standby;
> > > -	bool y_cord_support;
> > >  	bool colorimetry_support;
> > >  	bool alpm;
> > >  	bool has_hw_tracking;
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 9811f5f0bc75..62d97d5576d1 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -93,7 +93,7 @@ static void psr_aux_io_power_put(struct intel_dp
> > > *intel_dp)
> > >  	intel_display_power_put(dev_priv,
> > > psr_aux_domain(intel_dp));
> > >  }
> > >  
> > > -static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
> > > +static bool intel_dp_get_y_coord_required(struct intel_dp
> > > *intel_dp)
> > >  {
> > >  	uint8_t psr_caps = 0;
> > >  
> > > @@ -137,22 +137,38 @@ void intel_psr_init_dpcd(struct intel_dp
> > > *intel_dp)
> > >  
> > >  	if (INTEL_GEN(dev_priv) >= 9 &&
> > >  	    (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) {
> > > -		uint8_t frame_sync_cap;
> > > +		uint8_t frame_sync_cap, y_coord_req;
> > >  
> > >  		dev_priv->psr.sink_support = true;
> > > +
> > > +		/* PSR2 needs frame sync to do selective updates
> > > */
> > >  		if (drm_dp_dpcd_readb(&intel_dp->aux,
> > >  				      DP_SINK_DEVICE_AUX_FRAME_SYN
> > > C_CAP,
> > >  				      &frame_sync_cap) != 1)
> > >  			frame_sync_cap = 0;
> > >  		frame_sync_cap = (frame_sync_cap &
> > > DP_AUX_FRAME_SYNC_CAP);
> > > -		/* PSR2 needs frame sync as well */
> > > -		dev_priv->psr.psr2_support = frame_sync_cap;
> > > -		DRM_DEBUG_KMS("PSR2 %s on sink",
> > > -			      dev_priv->psr.psr2_support ?
> > > "supported" : "not supported");
> > > +
> > > +		/*
> > > +		 * FIXME: Enable PSR2 only for y-coordinate PSR2
> > > panels
> > > +		 * After GTC implementation, remove this
> > > restriction.
> > 
> > I believe we should remove the FIXME and just comment out that non y-
> > coordinate
> > would need GTC that we currently don't implement.
> > 
> > later if we change our minds and decided to add that support we
> > revisit here
> > but without a FIXME poluting code for something we don't plan to
> > support for now.
> > 
> > > +		 *
> > > +		 * All panels that supports PSR version 3 that is
> > 
> > PSRv3? is there really something formal about that?
> 
> Yes, from eDP spec:
> 
> PSR Support and Version
> 
> 00h = Panel Self Refresh Capability is not supported (default).
> If PSR/PSR2 is supported, the SET_POWER_CAPABLE bit in the
> EDP_GENERAL_CAPABILITY_1 register (DPCD Address 00701h, bit 7) must
> be set to 1.
> 01h = Panel Self Refresh capability is supported, PSR version is 01h
> (PSR).
> 02h = Panel Self Refresh with Selective Update (SU) capability is
> supported,
> PSR version is 02h (PSR2). Y-coordinates for SU are not supported.
> Supported
> by eDP v1.4 (and higher).
> 03h = Panel Self Refresh with SU capability and Y-coordinate supported,
> PSR
> version is 03h (PSR2 + Y-coordinate). Supported by eDP v1.4a (and
> higher).

Oh cool! now I understand the comment.
Could we add the comment like in spec ie
s/PSR version 3 that is PSR2 + Y-coordinate/PSR version 03h (PSR2 + Y-coordinate)/
?
just to avoid giving impression of a PSR3 support and someone
freaking out thinking that we would need much more changes ;)

> 
> > 
> > > +		 * PSR2 + Y-coordinate support can handle Y
> > > coordinates in VSC
> > > +		 * but we are only sure that it is going to be
> > > used when
> > > +		 * required by the panel. This way panel is
> > > capable to do
> > > +		 * selective update even without a valid aux frame
> > > sync.
> > > +		 */
> > > +		y_coord_req =
> > > intel_dp_get_y_coord_required(intel_dp);
> > > +
> > > +		dev_priv->psr.psr2_support = frame_sync_cap &&
> > > y_coord_req;
> > > +		if (dev_priv->psr.psr2_support)
> > > +			DRM_DEBUG_KMS("PSR2 supported on sink\n");
> > > +		else
> > > +			DRM_DEBUG_KMS("PSR2 not supported on sink"
> > > +				      "(frame sync: %d Y-coord
> > > required: %d)\n",
> > > +				      frame_sync_cap,
> > > y_coord_req);
> > >  
> > >  		if (dev_priv->psr.psr2_support) {
> > > -			dev_priv->psr.y_cord_support =
> > > -				intel_dp_get_y_cord_status(intel_d
> > > p);
> > >  			dev_priv->psr.colorimetry_support =
> > >  				intel_dp_get_colorimetry_status(in
> > > tel_dp);
> > >  			dev_priv->psr.alpm =
> > > @@ -198,16 +214,12 @@ static void hsw_psr_setup_vsc(struct intel_dp
> > > *intel_dp,
> > >  		memset(&psr_vsc, 0, sizeof(psr_vsc));
> > >  		psr_vsc.sdp_header.HB0 = 0;
> > >  		psr_vsc.sdp_header.HB1 = 0x7;
> > > -		if (dev_priv->psr.colorimetry_support &&
> > > -		    dev_priv->psr.y_cord_support) {
> > > +		if (dev_priv->psr.colorimetry_support) {
> > >  			psr_vsc.sdp_header.HB2 = 0x5;
> > >  			psr_vsc.sdp_header.HB3 = 0x13;
> > > -		} else if (dev_priv->psr.y_cord_support) {
> > > +		} else {
> > >  			psr_vsc.sdp_header.HB2 = 0x4;
> > >  			psr_vsc.sdp_header.HB3 = 0xe;
> > > -		} else {
> > > -			psr_vsc.sdp_header.HB2 = 0x3;
> > > -			psr_vsc.sdp_header.HB3 = 0xc;
> > >  		}
> > >  	} else {
> > >  		/* Prepare VSC packet as per EDP 1.3 spec, Table
> > > 3.10 */
> > > @@ -478,15 +490,6 @@ static bool intel_psr2_config_valid(struct
> > > intel_dp *intel_dp,
> > >  		return false;
> > >  	}
> > >  
> > > -	/*
> > > -	 * FIXME:enable psr2 only for y-cordinate psr2 panels
> > > -	 * After gtc implementation , remove this restriction.
> > > -	 */
> > > -	if (!dev_priv->psr.y_cord_support) {
> > > -		DRM_DEBUG_KMS("PSR2 not enabled, panel does not
> > > support Y coordinate\n");
> > > -		return false;
> > > -	}
> > > -
> > >  	return true;
> > >  }
> > >  
> > > @@ -586,14 +589,12 @@ static void hsw_psr_enable_source(struct
> > > intel_dp *intel_dp,
> > >  	struct drm_device *dev = dig_port->base.base.dev;
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  	enum transcoder cpu_transcoder = crtc_state-
> > > >cpu_transcoder;
> > > -	u32 chicken;
> > >  
> > >  	psr_aux_io_power_get(intel_dp);
> > >  
> > >  	if (dev_priv->psr.psr2_support) {
> > > -		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> > > -		if (dev_priv->psr.y_cord_support)
> > > -			chicken |= PSR2_ADD_VERTICAL_LINE_COUNT;
> > > +		u32 chicken = PSR2_VSC_ENABLE_PROG_HEADER
> > > +			      | PSR2_ADD_VERTICAL_LINE_COUNT;
> > >  		I915_WRITE(CHICKEN_TRANS(cpu_transcoder),
> > > chicken);
> > >  
> > >  		I915_WRITE(EDP_PSR_DEBUG,
> > > -- 
> > > 2.16.2
> > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/6] drm/i915/psr: Enable Y-coordinate support in source

[Pandiyan, Dhinakaran]

On Thu, 2018-03-15 at 17:28 -0700, Rodrigo Vivi wrote:
> drm/i915/cnl:....
> 
> On Wed, Mar 14, 2018 at 03:36:14PM -0700, José Roberto de Souza wrote:
> > We are requiring that sink requires Y-coordinate but we are not
> > sending it in the main-link.
> 
> Also add on CNL here
> 
> > Even if hardware tracking isn't good enough it will not cause
> > any more issues enabling it.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h  | 2 ++
> >  drivers/gpu/drm/i915/intel_psr.c | 4 ++--
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index a15db41a208a..e9fc1722c0fb 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4132,6 +4132,8 @@ enum {
> >  #define EDP_PSR2_CTL			_MMIO(0x6f900)
> >  #define   EDP_PSR2_ENABLE		(1<<31)
> >  #define   EDP_SU_TRACK_ENABLE		(1<<30)
> > +#define   EDP_Y_COORDINATE_VALID	(1<<26)
> > +#define   EDP_Y_COORDINATE_ENABLE	(1<<25)
> 
> probably good add CNL_ prefix on these bits...
> 
> >  #define   EDP_MAX_SU_DISABLE_TIME(t)	((t)<<20)
> >  #define   EDP_MAX_SU_DISABLE_TIME_MASK	(0x1f<<20)
> >  #define   EDP_PSR2_TP2_TIME_500		(0<<8)
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 62d97d5576d1..c9da1390a727 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -416,8 +416,8 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
> >  	/* FIXME: selective update is probably totally broken because it doesn't
> >  	 * mesh at all with our frontbuffer tracking. And the hw alone isn't
> >  	 * good enough. */
> > -	val |= EDP_PSR2_ENABLE |
> > -		EDP_SU_TRACK_ENABLE;
> > +	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
> > +	val |= EDP_Y_COORDINATE_VALID | EDP_Y_COORDINATE_ENABLE;
> 
> if (INTEL_GEN(dev_priv) >= 10)
> 	val |= EDP_Y_COORDINATE_VALID | EDP_Y_COORDINATE_ENABLE;
> 
> since those bits were reserved before CNL.
> 

How does this work on pre-CNL platforms without the enable bit?

> With those changes:
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> >  
> >  	if (drm_dp_dpcd_readb(&intel_dp->aux,
> >  				DP_SYNCHRONIZATION_LATENCY_IN_SINK,
> > -- 
> > 2.16.2
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915/psr: Enable aux frame sync in source

[Pandiyan, Dhinakaran]

On Wed, 2018-03-14 at 15:36 -0700, José Roberto de Souza wrote:
> Even with GTC not enabled lets send the aux frame sync.

If this was never enabled on the source side, why do we place a
requirement on the sink to support aux frame sync?



> Hardware is going to send dummy values but this way we can get rid of
> this workarround in PSR exit: 'drm/i915/psr: disable aux_frame_sync
> on psr2 exit'.
> Also moving the line disabling aux frame sync in sink to after report
> that PSR2 has exit to avoid.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 15 ++++++---------
>  2 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index e9fc1722c0fb..5a2364656aa5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4132,6 +4132,7 @@ enum {
>  #define EDP_PSR2_CTL			_MMIO(0x6f900)
>  #define   EDP_PSR2_ENABLE		(1<<31)
>  #define   EDP_SU_TRACK_ENABLE		(1<<30)
> +#define   EDP_AUX_FRAME_SYNC_ENABLE	(1<<27)
>  #define   EDP_Y_COORDINATE_VALID	(1<<26)
>  #define   EDP_Y_COORDINATE_ENABLE	(1<<25)
>  #define   EDP_MAX_SU_DISABLE_TIME(t)	((t)<<20)
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index d622e37894d4..7aab66b5bc91 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -418,6 +418,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>  	 * good enough. */
>  	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
>  	val |= EDP_Y_COORDINATE_VALID | EDP_Y_COORDINATE_ENABLE;
> +	val |= EDP_AUX_FRAME_SYNC_ENABLE;
>  
>  	if (drm_dp_dpcd_readb(&intel_dp->aux,
>  				DP_SYNCHRONIZATION_LATENCY_IN_SINK,
> @@ -715,11 +716,6 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
>  		i915_reg_t psr_status;
>  		u32 psr_status_mask;
>  
> -		if (dev_priv->psr.psr2_enabled)
> -			drm_dp_dpcd_writeb(&intel_dp->aux,
> -					DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
> -					0);
> -
>  		if (dev_priv->psr.psr2_enabled) {
>  			psr_status = EDP_PSR2_STATUS;
>  			psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> @@ -742,6 +738,11 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
>  					    2000))
>  			DRM_ERROR("Timed out waiting for PSR Idle State\n");
>  
> +		if (dev_priv->psr.psr2_enabled)
> +			drm_dp_dpcd_writeb(&intel_dp->aux,
> +					   DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
> +					   0);
> +
>  		dev_priv->psr.active = false;
>  	} else {
>  		if (dev_priv->psr.psr2_enabled)
> @@ -863,10 +864,6 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
>  		return;
>  
>  	if (HAS_DDI(dev_priv)) {
> -		if (dev_priv->psr.psr2_enabled)
> -			drm_dp_dpcd_writeb(&intel_dp->aux,
> -					DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
> -					0);
>  		if (dev_priv->psr.psr2_enabled) {
>  			val = I915_READ(EDP_PSR2_CTL);
>  			WARN_ON(!(val & EDP_PSR2_ENABLE));
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/6] drm/i915/psr: Rename intel_crtc_state has_psr to can_psr

[Pandiyan, Dhinakaran]

On Wed, 2018-03-14 at 15:36 -0700, José Roberto de Souza wrote:
> This value is a match of hardware and sink has PSR + if it can be
> enabled by the requested state, see intel_psr_compute_config().
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  4 ++--
>  drivers/gpu/drm/i915/intel_psr.c | 12 ++++++------
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a215aa78b0be..cccaf84415ab 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -807,8 +807,8 @@ struct intel_crtc_state {
>  	struct intel_link_m_n dp_m2_n2;
>  	bool has_drrs;
>  
> -	bool has_psr;
> -	bool has_psr2;
> +	bool can_psr;
> +	bool can_psr2;


I am not convinced by this change, the computed state either has PSR1 or
PSR2, "can" connotes ambiguity in my opinion.


I was thinking of converting this to an u8 psr = [0,1,2] to mean no PSR,
PSR1 and PSR2 respectively. We can do away with the has/can confusion :)

Using bool does save us 6 bits though depending on how the structure is
packed. 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/6] drm/i915/psr: Rename intel_crtc_state has_psr to can_psr

[Souza, Jose]

On Thu, 2018-03-15 at 19:09 -0700, Pandiyan, Dhinakaran wrote:
> 
> 
> On Wed, 2018-03-14 at 15:36 -0700, José Roberto de Souza wrote:
> > This value is a match of hardware and sink has PSR + if it can be
> > enabled by the requested state, see intel_psr_compute_config().
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h |  4 ++--
> >  drivers/gpu/drm/i915/intel_psr.c | 12 ++++++------
> >  2 files changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index a215aa78b0be..cccaf84415ab 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -807,8 +807,8 @@ struct intel_crtc_state {
> >  	struct intel_link_m_n dp_m2_n2;
> >  	bool has_drrs;
> >  
> > -	bool has_psr;
> > -	bool has_psr2;
> > +	bool can_psr;
> > +	bool can_psr2;
> 
> 
> I am not convinced by this change, the computed state either has PSR1
> or
> PSR2, "can" connotes ambiguity in my opinion.

Computed state can have PSR1 and PSR2 set.

> 
> 
> I was thinking of converting this to an u8 psr = [0,1,2] to mean no
> PSR,
> PSR1 and PSR2 respectively. We can do away with the has/can confusion
> :)
> 
> Using bool does save us 6 bits though depending on how the structure
> is
> packed. 

I almost did this but I just renamed to do less changes. But cool I
will rename it to psr and have 0, 1 or 2 values.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx