[PATCH v2 1/5] drm/i915/psr: Nuke aux_frame_sync

[PATCH v2 1/5] 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>
Reviewed-by: 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 v2 2/5] 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>
---

v2: Changes in comment requested by Rodrigo Vivi

 drivers/gpu/drm/i915/i915_drv.h  |  1 -
 drivers/gpu/drm/i915/intel_psr.c | 56 +++++++++++++++++++++-------------------
 2 files changed, 29 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..5593d1f3049a 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,39 @@ 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");
+
+		/*
+		 * All panels that supports PSR version 03h (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.
+		 *
+		 * To support PSR version 02h and PSR version 03h without
+		 * Y-coordinate requirement panels we would need to enable
+		 * GTC first.
+		 */
+		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 +215,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 +491,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 +590,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 v2 3/5] drm/i915/psr/cnl: Enable Y-coordinate support in source

[José Roberto de Souza]

From: "Souza, Jose" <jose.souza@intel.com>

For Geminilake and Cannonlake+ the Y-coordinate support must be
enabled in PSR2_CTL too.

Spec: 7713

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---

v2: This is specific to Geminilake and Cannonlake+

 drivers/gpu/drm/i915/i915_reg.h  | 2 ++
 drivers/gpu/drm/i915/intel_psr.c | 6 ++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 1e000f3004cb..bac54f744913 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3869,6 +3869,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) /* GLK and CNL+ */
+#define   EDP_Y_COORDINATE_ENABLE	(1<<25) /* GLK and CNL+ */
 #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 5593d1f3049a..c5eeb13cbcfd 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -417,8 +417,10 @@ 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;
+	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
+		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 v2 4/5] 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 5378863e3238..4366adcad56d 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 c5eeb13cbcfd..aa4e03f65386 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -161,15 +161,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 =
@@ -210,7 +210,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;
@@ -282,12 +282,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);
@@ -455,7 +455,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);
@@ -475,7 +475,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)) {
@@ -574,7 +574,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);
@@ -595,7 +595,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);
@@ -648,7 +648,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);
@@ -718,12 +718,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;
 
@@ -747,7 +747,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);
@@ -807,7 +807,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,
@@ -866,11 +866,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);
@@ -1039,7 +1039,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 v2 5/5] drm/i915/psr: Simply PSR computed state

[José Roberto de Souza]

Having has_psr and has_psr2 can be ambiguous and also uses one more
byte than needed(not taking in care struct alignment).

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>
---

v2: Grouped the 2 bools into one u8 as suggested by Dhinakaran Pandiyan.

 drivers/gpu/drm/i915/intel_drv.h |  3 +--
 drivers/gpu/drm/i915/intel_psr.c | 14 ++++++++------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a215aa78b0be..a7383235f90a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -807,8 +807,7 @@ struct intel_crtc_state {
 	struct intel_link_m_n dp_m2_n2;
 	bool has_drrs;
 
-	bool has_psr;
-	bool has_psr2;
+	u8 psr;
 
 	/*
 	 * 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 aa4e03f65386..78b5c0c88261 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -563,9 +563,11 @@ 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" : "");
+	if (intel_psr2_config_valid(intel_dp, crtc_state))
+		crtc_state->psr = DP_PSR2_IS_SUPPORTED;
+	else
+		crtc_state->psr = DP_PSR_IS_SUPPORTED;
+	DRM_DEBUG_KMS("Enabling PSR%d\n", crtc_state->psr);
 }
 
 static void intel_psr_activate(struct intel_dp *intel_dp)
@@ -635,7 +637,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->psr)
 		return;
 
 	if (WARN_ON(!CAN_PSR(dev_priv)))
@@ -648,7 +650,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->psr == DP_PSR2_IS_SUPPORTED);
 	dev_priv->psr.busy_frontbuffer_bits = 0;
 
 	dev_priv->psr.setup_vsc(intel_dp, crtc_state);
@@ -770,7 +772,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->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

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

[Rodrigo Vivi]

On Fri, Mar 16, 2018 at 04:04:58PM -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>

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

> ---
> 
> v2: Changes in comment requested by Rodrigo Vivi
> 
>  drivers/gpu/drm/i915/i915_drv.h  |  1 -
>  drivers/gpu/drm/i915/intel_psr.c | 56 +++++++++++++++++++++-------------------
>  2 files changed, 29 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..5593d1f3049a 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,39 @@ 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");
> +
> +		/*
> +		 * All panels that supports PSR version 03h (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.
> +		 *
> +		 * To support PSR version 02h and PSR version 03h without
> +		 * Y-coordinate requirement panels we would need to enable
> +		 * GTC first.
> +		 */
> +		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 +215,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 +491,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 +590,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 v2 4/5] drm/i915/psr: Do not override PSR2 sink support

[Rodrigo Vivi]

On Fri, Mar 16, 2018 at 04:05:00PM -0700, José Roberto de Souza wrote:
> 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>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@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 5378863e3238..4366adcad56d 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 c5eeb13cbcfd..aa4e03f65386 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -161,15 +161,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 =
> @@ -210,7 +210,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;
> @@ -282,12 +282,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);
> @@ -455,7 +455,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);
> @@ -475,7 +475,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)) {
> @@ -574,7 +574,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);
> @@ -595,7 +595,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);
> @@ -648,7 +648,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);
> @@ -718,12 +718,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;
>  
> @@ -747,7 +747,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);
> @@ -807,7 +807,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,
> @@ -866,11 +866,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);
> @@ -1039,7 +1039,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

Re: [PATCH v2 5/5] drm/i915/psr: Simply PSR computed state

[Rodrigo Vivi]

On Fri, Mar 16, 2018 at 04:05:01PM -0700, José Roberto de Souza wrote:
> Having has_psr and has_psr2 can be ambiguous and also uses one more
> byte than needed(not taking in care struct alignment).
> 
> 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>
> ---
> 
> v2: Grouped the 2 bools into one u8 as suggested by Dhinakaran Pandiyan.
> 
>  drivers/gpu/drm/i915/intel_drv.h |  3 +--
>  drivers/gpu/drm/i915/intel_psr.c | 14 ++++++++------
>  2 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a215aa78b0be..a7383235f90a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -807,8 +807,7 @@ struct intel_crtc_state {
>  	struct intel_link_m_n dp_m2_n2;
>  	bool has_drrs;
>  
> -	bool has_psr;
> -	bool has_psr2;
> +	u8 psr;
>  
>  	/*
>  	 * 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 aa4e03f65386..78b5c0c88261 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -563,9 +563,11 @@ 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" : "");
> +	if (intel_psr2_config_valid(intel_dp, crtc_state))
> +		crtc_state->psr = DP_PSR2_IS_SUPPORTED;
> +	else
> +		crtc_state->psr = DP_PSR_IS_SUPPORTED;
> +	DRM_DEBUG_KMS("Enabling PSR%d\n", crtc_state->psr);

Could we still continue writing "PSR" instead of "PSR1" ?

otherwise patch lgtm...

>  }
>  
>  static void intel_psr_activate(struct intel_dp *intel_dp)
> @@ -635,7 +637,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->psr)
>  		return;
>  
>  	if (WARN_ON(!CAN_PSR(dev_priv)))
> @@ -648,7 +650,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->psr == DP_PSR2_IS_SUPPORTED);
>  	dev_priv->psr.busy_frontbuffer_bits = 0;
>  
>  	dev_priv->psr.setup_vsc(intel_dp, crtc_state);
> @@ -770,7 +772,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->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

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

[Pandiyan, Dhinakaran]

On Fri, 2018-03-16 at 16:04 -0700, José Roberto de Souza wrote:
> PSR2 selective update requires aux frame sync(even though we don't
> support it in i915)

Any chance I could get you to fix this line? 

i915 does not support aux frame sync ^
PSR2 panel needs to support aux frame sync (see below)

I find this contradictory, please explain what you believe is going on.


>  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>
> Reviewed-by: 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);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for series starting with [v2,1/5] drm/i915/psr: Nuke aux_frame_sync

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/5] drm/i915/psr: Nuke aux_frame_sync
URL   : https://patchwork.freedesktop.org/series/40138/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
ed472e09578b drm/i915/psr: Nuke aux_frame_sync
7106739a3800 drm/i915/psr: Tie PSR2 support to Y coordinate requirement in intel_psr_init_dpcd()
ff9a3483c3d6 drm/i915/psr/cnl: Enable Y-coordinate support in source
-:26: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#26: FILE: drivers/gpu/drm/i915/i915_reg.h:3872:
+#define   EDP_Y_COORDINATE_VALID	(1<<26) /* GLK and CNL+ */
                                 	  ^

-:27: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#27: FILE: drivers/gpu/drm/i915/i915_reg.h:3873:
+#define   EDP_Y_COORDINATE_ENABLE	(1<<25) /* GLK and CNL+ */
                                  	  ^

total: 0 errors, 0 warnings, 2 checks, 20 lines checked
37e021ca152b drm/i915/psr: Do not override PSR2 sink support
a737ca97244b drm/i915/psr: Simply PSR computed state

_______________________________________________
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 [v2,1/5] drm/i915/psr: Nuke aux_frame_sync

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/5] drm/i915/psr: Nuke aux_frame_sync
URL   : https://patchwork.freedesktop.org/series/40138/
State : failure

== Summary ==

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

---- Possible new issues:

Test gem_sync:
        Subgroup basic-many-each:
                pass       -> INCOMPLETE (fi-snb-2600)

---- Known issues:

Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> FAIL       (fi-skl-6770hq) fdo#100368
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                incomplete -> PASS       (fi-snb-2520m) fdo#103713
        Subgroup suspend-read-crc-pipe-c:
                pass       -> INCOMPLETE (fi-bxt-dsi) fdo#103927

fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927

fi-bdw-5557u     total:285  pass:264  dwarn:0   dfail:0   fail:0   skip:21  time:430s
fi-bdw-gvtdvm    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:444s
fi-blb-e6850     total:285  pass:220  dwarn:1   dfail:0   fail:0   skip:64  time:379s
fi-bsw-n3050     total:285  pass:239  dwarn:0   dfail:0   fail:0   skip:46  time:535s
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-bxt-j4205     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:510s
fi-byt-j1900     total:285  pass:250  dwarn:0   dfail:0   fail:0   skip:35  time:519s
fi-byt-n2820     total:285  pass:246  dwarn:0   dfail:0   fail:0   skip:39  time:503s
fi-cfl-8700k     total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:409s
fi-cfl-s2        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:582s
fi-cfl-u         total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:512s
fi-cnl-drrs      total:285  pass:254  dwarn:3   dfail:0   fail:0   skip:28  time:539s
fi-cnl-y3        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:584s
fi-elk-e7500     total:285  pass:225  dwarn:1   dfail:0   fail:0   skip:59  time:425s
fi-gdg-551       total:285  pass:176  dwarn:0   dfail:0   fail:1   skip:108 time:319s
fi-glk-1         total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:535s
fi-hsw-4770      total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:402s
fi-ilk-650       total:285  pass:225  dwarn:0   dfail:0   fail:0   skip:60  time:417s
fi-ivb-3520m     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:473s
fi-ivb-3770      total:285  pass:252  dwarn:0   dfail:0   fail:0   skip:33  time:430s
fi-kbl-7500u     total:285  pass:260  dwarn:1   dfail:0   fail:0   skip:24  time:474s
fi-kbl-7567u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:467s
fi-kbl-r         total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:516s
fi-pnv-d510      total:285  pass:219  dwarn:1   dfail:0   fail:0   skip:65  time:649s
fi-skl-6260u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:445s
fi-skl-6600u     total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:531s
fi-skl-6700hq    total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:539s
fi-skl-6700k2    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:508s
fi-skl-6770hq    total:285  pass:264  dwarn:0   dfail:0   fail:1   skip:20  time:476s
fi-skl-guc       total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:427s
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:556s
fi-snb-2600      total:149  pass:132  dwarn:0   dfail:0   fail:0   skip:16 

639c78d2805bec2022435a8184db27a3b9767fe7 drm-tip: 2018y-03m-16d-23h-05m-00s UTC integration manifest
a737ca97244b drm/i915/psr: Simply PSR computed state
37e021ca152b drm/i915/psr: Do not override PSR2 sink support
ff9a3483c3d6 drm/i915/psr/cnl: Enable Y-coordinate support in source
7106739a3800 drm/i915/psr: Tie PSR2 support to Y coordinate requirement in intel_psr_init_dpcd()
ed472e09578b drm/i915/psr: Nuke aux_frame_sync

== Logs ==

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

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

[Pandiyan, Dhinakaran]

On Fri, 2018-03-16 at 16:04 -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>
> ---
> 
> v2: Changes in comment requested by Rodrigo Vivi
> 
>  drivers/gpu/drm/i915/i915_drv.h  |  1 -
>  drivers/gpu/drm/i915/intel_psr.c | 56 +++++++++++++++++++++-------------------
>  2 files changed, 29 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..5593d1f3049a 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,39 @@ 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");
> +
> +		/*
> +		 * All panels that supports PSR version 03h (PSR2 +
> +		 * Y-coordinate) 

nit: Perhaps check for PSR version 03h as a minimum requirement for
PSR2, a bit redundant but is useful documentation in code.



> 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.
> +		 *
> +		 * To support PSR version 02h and PSR version 03h without
> +		 * Y-coordinate requirement panels we would need to enable
> +		 * GTC first.
> +		 */
> +		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 +215,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 +491,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 +590,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,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Pandiyan, Dhinakaran]

On Fri, 2018-03-16 at 16:25 -0700, Rodrigo Vivi wrote:
> On Fri, Mar 16, 2018 at 04:04:58PM -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>
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> > ---
> > 
> > v2: Changes in comment requested by Rodrigo Vivi
> > 
> >  drivers/gpu/drm/i915/i915_drv.h  |  1 -
> >  drivers/gpu/drm/i915/intel_psr.c | 56 +++++++++++++++++++++-------------------
> >  2 files changed, 29 insertions(+), 28 deletions(-)
> > 

> >  
> > @@ -586,14 +590,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;


I think I answered my own question.

Please check the bits on these register. Seems like these bits were
replaced by

PSR2_CTL[EDP_Y_COORDINATE_VALID | EDP_Y_COORDINATE_ENABLE] bits on gen10


If I am correct, this register should be set only for gen-9. It'd be
nice to fix PSR2 while making these changes :)




> >  		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 v2 3/5] drm/i915/psr/cnl: Enable Y-coordinate support in source

[Pandiyan, Dhinakaran]

On Fri, 2018-03-16 at 16:04 -0700, José Roberto de Souza wrote:
> From: "Souza, Jose" <jose.souza@intel.com>
> 
> For Geminilake and Cannonlake+ the Y-coordinate support must be
> enabled in PSR2_CTL too.
> 
> Spec: 7713
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> 
> v2: This is specific to Geminilake and Cannonlake+
> 
>  drivers/gpu/drm/i915/i915_reg.h  | 2 ++
>  drivers/gpu/drm/i915/intel_psr.c | 6 ++++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 1e000f3004cb..bac54f744913 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3869,6 +3869,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) /* GLK and CNL+ */
> +#define   EDP_Y_COORDINATE_ENABLE	(1<<25) /* GLK and CNL+ */
>  #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 5593d1f3049a..c5eeb13cbcfd 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -417,8 +417,10 @@ 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;
> +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
> +		val |= EDP_Y_COORDINATE_VALID | EDP_Y_COORDINATE_ENABLE;
> +	}

The corresponding bits for gen-9 are set in hsw_psr_enable_source.


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

Re: [PATCH v2 5/5] drm/i915/psr: Simply PSR computed state

[Pandiyan, Dhinakaran]

On Fri, 2018-03-16 at 16:30 -0700, Rodrigo Vivi wrote:
> On Fri, Mar 16, 2018 at 04:05:01PM -0700, José Roberto de Souza wrote:
> > Having has_psr and has_psr2 can be ambiguous and also uses one more
> > byte than needed(not taking in care struct alignment).
> > 
> > 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>
> > ---
> > 
> > v2: Grouped the 2 bools into one u8 as suggested by Dhinakaran Pandiyan.
> > 
> >  drivers/gpu/drm/i915/intel_drv.h |  3 +--
> >  drivers/gpu/drm/i915/intel_psr.c | 14 ++++++++------
> >  2 files changed, 9 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index a215aa78b0be..a7383235f90a 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -807,8 +807,7 @@ struct intel_crtc_state {
> >  	struct intel_link_m_n dp_m2_n2;
> >  	bool has_drrs;
> >  
> > -	bool has_psr;
> > -	bool has_psr2;
> > +	u8 psr;

/* 0 = disabled, 1 = PSR1, 2 = 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 aa4e03f65386..78b5c0c88261 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -563,9 +563,11 @@ 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" : "");
> > +	if (intel_psr2_config_valid(intel_dp, crtc_state))
> > +		crtc_state->psr = DP_PSR2_IS_SUPPORTED;

We can avoid the dependency on an unrelated macro definition if you
explicitly set it to 1 or 2.

> > +	else
> > +		crtc_state->psr = DP_PSR_IS_SUPPORTED;
> > +	DRM_DEBUG_KMS("Enabling PSR%d\n", crtc_state->psr);

Also I think you should initialize ->psr = 0
> 
> Could we still continue writing "PSR" instead of "PSR1" ?
> 
> otherwise patch lgtm...
> 
> >  }
> >  
> >  static void intel_psr_activate(struct intel_dp *intel_dp)
> > @@ -635,7 +637,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->psr)
> >  		return;
> >  
> >  	if (WARN_ON(!CAN_PSR(dev_priv)))
> > @@ -648,7 +650,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->psr == DP_PSR2_IS_SUPPORTED);
> >  	dev_priv->psr.busy_frontbuffer_bits = 0;
> >  
> >  	dev_priv->psr.setup_vsc(intel_dp, crtc_state);
> > @@ -770,7 +772,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->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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 5/5] drm/i915/psr: Simply PSR computed state

[Souza, Jose]

On Fri, 2018-03-16 at 17:38 -0700, Pandiyan, Dhinakaran wrote:
> 
> 
> On Fri, 2018-03-16 at 16:30 -0700, Rodrigo Vivi wrote:
> > On Fri, Mar 16, 2018 at 04:05:01PM -0700, José Roberto de Souza
> > wrote:
> > > Having has_psr and has_psr2 can be ambiguous and also uses one
> > > more
> > > byte than needed(not taking in care struct alignment).
> > > 
> > > 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>
> > > ---
> > > 
> > > v2: Grouped the 2 bools into one u8 as suggested by Dhinakaran
> > > Pandiyan.
> > > 
> > >  drivers/gpu/drm/i915/intel_drv.h |  3 +--
> > >  drivers/gpu/drm/i915/intel_psr.c | 14 ++++++++------
> > >  2 files changed, 9 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index a215aa78b0be..a7383235f90a 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -807,8 +807,7 @@ struct intel_crtc_state {
> > >  	struct intel_link_m_n dp_m2_n2;
> > >  	bool has_drrs;
> > >  
> > > -	bool has_psr;
> > > -	bool has_psr2;
> > > +	u8 psr;
> 
> /* 0 = disabled, 1 = PSR1, 2 = 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 aa4e03f65386..78b5c0c88261 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -563,9 +563,11 @@ 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" : "");
> > > +	if (intel_psr2_config_valid(intel_dp, crtc_state))
> > > +		crtc_state->psr = DP_PSR2_IS_SUPPORTED;
> 
> We can avoid the dependency on an unrelated macro definition if you
> explicitly set it to 1 or 2.

I don't like the idea of using magic numbers, what do you think about
add enum?

> 
> > > +	else
> > > +		crtc_state->psr = DP_PSR_IS_SUPPORTED;
> > > +	DRM_DEBUG_KMS("Enabling PSR%d\n", crtc_state->psr);
> 
> Also I think you should initialize ->psr = 0
> > 
> > Could we still continue writing "PSR" instead of "PSR1" ?
> > 
> > otherwise patch lgtm...
> > 
> > >  }
> > >  
> > >  static void intel_psr_activate(struct intel_dp *intel_dp)
> > > @@ -635,7 +637,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->psr)
> > >  		return;
> > >  
> > >  	if (WARN_ON(!CAN_PSR(dev_priv)))
> > > @@ -648,7 +650,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->psr ==
> > > DP_PSR2_IS_SUPPORTED);
> > >  	dev_priv->psr.busy_frontbuffer_bits = 0;
> > >  
> > >  	dev_priv->psr.setup_vsc(intel_dp, crtc_state);
> > > @@ -770,7 +772,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->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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx