[PATCH 1/2] drm/i915: Add enum aux_ch and clean up the aux init to use it

[PATCH 1/2] drm/i915: Add enum aux_ch and clean up the aux init to use it

[Dhinakaran Pandiyan]

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

Since we no longer have a 1:1 correspondence between ports and AUX
channels, let's give AUX channels their own enum. Makes it easier
to tell the apples from the oranges, and we get rid of the
port E AUX power domain FIXME since we now derive the power domain
from the actual AUX CH.

v2: Rebase due to AUX F

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |   8 +-
 drivers/gpu/drm/i915/intel_display.h |  11 ++
 drivers/gpu/drm/i915/intel_dp.c      | 240 +++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_drv.h     |   1 +
 4 files changed, 131 insertions(+), 129 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 1412abcb27d4..39d624083a17 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5347,8 +5347,8 @@ enum {
 #define _DPF_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64520)
 #define _DPF_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64524)
 
-#define DP_AUX_CH_CTL(port)	_MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
-#define DP_AUX_CH_DATA(port, i)	_MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
+#define DP_AUX_CH_CTL(aux_ch)	_MMIO_PORT(aux_ch, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
+#define DP_AUX_CH_DATA(aux_ch, i)	_MMIO(_PORT(aux_ch, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
 
 #define   DP_AUX_CH_CTL_SEND_BUSY	    (1 << 31)
 #define   DP_AUX_CH_CTL_DONE		    (1 << 30)
@@ -7875,8 +7875,8 @@ enum {
 #define _PCH_DPD_AUX_CH_DATA4	0xe4320
 #define _PCH_DPD_AUX_CH_DATA5	0xe4324
 
-#define PCH_DP_AUX_CH_CTL(port)		_MMIO_PORT((port) - PORT_B, _PCH_DPB_AUX_CH_CTL, _PCH_DPC_AUX_CH_CTL)
-#define PCH_DP_AUX_CH_DATA(port, i)	_MMIO(_PORT((port) - PORT_B, _PCH_DPB_AUX_CH_DATA1, _PCH_DPC_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
+#define PCH_DP_AUX_CH_CTL(aux_ch)		_MMIO_PORT((aux_ch) - AUX_CH_B, _PCH_DPB_AUX_CH_CTL, _PCH_DPC_AUX_CH_CTL)
+#define PCH_DP_AUX_CH_DATA(aux_ch, i)	_MMIO(_PORT((aux_ch) - AUX_CH_B, _PCH_DPB_AUX_CH_DATA1, _PCH_DPC_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
 
 /* CPT */
 #define  PORT_TRANS_A_SEL_CPT	0
diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
index c4042e342f50..f5733a2576e7 100644
--- a/drivers/gpu/drm/i915/intel_display.h
+++ b/drivers/gpu/drm/i915/intel_display.h
@@ -139,6 +139,17 @@ enum dpio_phy {
 
 #define I915_NUM_PHYS_VLV 2
 
+enum aux_ch {
+	AUX_CH_A,
+	AUX_CH_B,
+	AUX_CH_C,
+	AUX_CH_D,
+	_AUX_CH_E, /* does not exist */
+	AUX_CH_F,
+};
+
+#define aux_ch_name(a) ((a) + 'A')
+
 enum intel_display_power_domain {
 	POWER_DOMAIN_PIPE_A,
 	POWER_DOMAIN_PIPE_B,
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f20b25f98e5a..eeb8a026fd08 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1331,171 +1331,194 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 	return ret;
 }
 
-static enum port intel_aux_port(struct drm_i915_private *dev_priv,
-				enum port port)
+static enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
 {
+	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	enum port port = encoder->port;
 	const struct ddi_vbt_port_info *info =
 		&dev_priv->vbt.ddi_port_info[port];
-	enum port aux_port;
+	enum aux_ch aux_ch;
 
 	if (!info->alternate_aux_channel) {
+		aux_ch = (enum aux_ch) port;
+
 		DRM_DEBUG_KMS("using AUX %c for port %c (platform default)\n",
-			      port_name(port), port_name(port));
-		return port;
+			      aux_ch_name(aux_ch), port_name(port));
+		return aux_ch;
 	}
 
 	switch (info->alternate_aux_channel) {
 	case DP_AUX_A:
-		aux_port = PORT_A;
+		aux_ch = AUX_CH_A;
 		break;
 	case DP_AUX_B:
-		aux_port = PORT_B;
+		aux_ch = AUX_CH_B;
 		break;
 	case DP_AUX_C:
-		aux_port = PORT_C;
+		aux_ch = AUX_CH_C;
 		break;
 	case DP_AUX_D:
-		aux_port = PORT_D;
+		aux_ch = AUX_CH_D;
 		break;
 	case DP_AUX_F:
-		aux_port = PORT_F;
+		aux_ch = AUX_CH_F;
 		break;
 	default:
 		MISSING_CASE(info->alternate_aux_channel);
-		aux_port = PORT_A;
+		aux_ch = AUX_CH_A;
 		break;
 	}
 
 	DRM_DEBUG_KMS("using AUX %c for port %c (VBT)\n",
-		      port_name(aux_port), port_name(port));
+		      aux_ch_name(aux_ch), port_name(port));
 
-	return aux_port;
+	return aux_ch;
+}
+
+static enum intel_display_power_domain
+intel_aux_power_domain(struct intel_dp *intel_dp)
+{
+	switch (intel_dp->aux_ch) {
+	case AUX_CH_A:
+		return POWER_DOMAIN_AUX_A;
+	case AUX_CH_B:
+		return POWER_DOMAIN_AUX_B;
+	case AUX_CH_C:
+		return POWER_DOMAIN_AUX_C;
+	case AUX_CH_D:
+		return POWER_DOMAIN_AUX_D;
+	case AUX_CH_F:
+		return POWER_DOMAIN_AUX_F;
+	default:
+		MISSING_CASE(intel_dp->aux_ch);
+		return POWER_DOMAIN_AUX_A;
+	}
 }
 
 static i915_reg_t g4x_aux_ctl_reg(struct drm_i915_private *dev_priv,
-				  enum port port)
+				  enum aux_ch aux_ch)
 {
-	switch (port) {
-	case PORT_B:
-	case PORT_C:
-	case PORT_D:
-		return DP_AUX_CH_CTL(port);
+	switch (aux_ch) {
+	case AUX_CH_B:
+	case AUX_CH_C:
+	case AUX_CH_D:
+		return DP_AUX_CH_CTL(aux_ch);
 	default:
-		MISSING_CASE(port);
-		return DP_AUX_CH_CTL(PORT_B);
+		MISSING_CASE(aux_ch);
+		return DP_AUX_CH_CTL(AUX_CH_B);
 	}
 }
 
 static i915_reg_t g4x_aux_data_reg(struct drm_i915_private *dev_priv,
-				   enum port port, int index)
+				   enum aux_ch aux_ch, int index)
 {
-	switch (port) {
-	case PORT_B:
-	case PORT_C:
-	case PORT_D:
-		return DP_AUX_CH_DATA(port, index);
+	switch (aux_ch) {
+	case AUX_CH_B:
+	case AUX_CH_C:
+	case AUX_CH_D:
+		return DP_AUX_CH_DATA(aux_ch, index);
 	default:
-		MISSING_CASE(port);
-		return DP_AUX_CH_DATA(PORT_B, index);
+		MISSING_CASE(aux_ch);
+		return DP_AUX_CH_DATA(AUX_CH_B, index);
 	}
 }
 
 static i915_reg_t ilk_aux_ctl_reg(struct drm_i915_private *dev_priv,
-				  enum port port)
-{
-	switch (port) {
-	case PORT_A:
-		return DP_AUX_CH_CTL(port);
-	case PORT_B:
-	case PORT_C:
-	case PORT_D:
-		return PCH_DP_AUX_CH_CTL(port);
+				  enum aux_ch aux_ch)
+{
+	switch (aux_ch) {
+	case AUX_CH_A:
+		return DP_AUX_CH_CTL(aux_ch);
+	case AUX_CH_B:
+	case AUX_CH_C:
+	case AUX_CH_D:
+		return PCH_DP_AUX_CH_CTL(aux_ch);
 	default:
-		MISSING_CASE(port);
-		return DP_AUX_CH_CTL(PORT_A);
+		MISSING_CASE(aux_ch);
+		return DP_AUX_CH_CTL(AUX_CH_A);
 	}
 }
 
 static i915_reg_t ilk_aux_data_reg(struct drm_i915_private *dev_priv,
-				   enum port port, int index)
-{
-	switch (port) {
-	case PORT_A:
-		return DP_AUX_CH_DATA(port, index);
-	case PORT_B:
-	case PORT_C:
-	case PORT_D:
-		return PCH_DP_AUX_CH_DATA(port, index);
+				   enum aux_ch aux_ch, int index)
+{
+	switch (aux_ch) {
+	case AUX_CH_A:
+		return DP_AUX_CH_DATA(aux_ch, index);
+	case AUX_CH_B:
+	case AUX_CH_C:
+	case AUX_CH_D:
+		return PCH_DP_AUX_CH_DATA(aux_ch, index);
 	default:
-		MISSING_CASE(port);
-		return DP_AUX_CH_DATA(PORT_A, index);
+		MISSING_CASE(aux_ch);
+		return DP_AUX_CH_DATA(AUX_CH_A, index);
 	}
 }
 
 static i915_reg_t skl_aux_ctl_reg(struct drm_i915_private *dev_priv,
-				  enum port port)
-{
-	switch (port) {
-	case PORT_A:
-	case PORT_B:
-	case PORT_C:
-	case PORT_D:
-	case PORT_F:
-		return DP_AUX_CH_CTL(port);
+				  enum aux_ch aux_ch)
+{
+	switch (aux_ch) {
+	case AUX_CH_A:
+	case AUX_CH_B:
+	case AUX_CH_C:
+	case AUX_CH_D:
+	case AUX_CH_F:
+		return DP_AUX_CH_CTL(aux_ch);
 	default:
-		MISSING_CASE(port);
-		return DP_AUX_CH_CTL(PORT_A);
+		MISSING_CASE(aux_ch);
+		return DP_AUX_CH_CTL(AUX_CH_A);
 	}
 }
 
 static i915_reg_t skl_aux_data_reg(struct drm_i915_private *dev_priv,
-				   enum port port, int index)
-{
-	switch (port) {
-	case PORT_A:
-	case PORT_B:
-	case PORT_C:
-	case PORT_D:
-	case PORT_F:
-		return DP_AUX_CH_DATA(port, index);
+				   enum aux_ch aux_ch, int index)
+{
+	switch (aux_ch) {
+	case AUX_CH_A:
+	case AUX_CH_B:
+	case AUX_CH_C:
+	case AUX_CH_D:
+	case AUX_CH_F:
+		return DP_AUX_CH_DATA(aux_ch, index);
 	default:
-		MISSING_CASE(port);
-		return DP_AUX_CH_DATA(PORT_A, index);
+		MISSING_CASE(aux_ch);
+		return DP_AUX_CH_DATA(AUX_CH_A, index);
 	}
 }
 
 static i915_reg_t intel_aux_ctl_reg(struct drm_i915_private *dev_priv,
-				    enum port port)
+				    enum aux_ch aux_ch)
 {
 	if (INTEL_GEN(dev_priv) >= 9)
-		return skl_aux_ctl_reg(dev_priv, port);
+		return skl_aux_ctl_reg(dev_priv, aux_ch);
 	else if (HAS_PCH_SPLIT(dev_priv))
-		return ilk_aux_ctl_reg(dev_priv, port);
+		return ilk_aux_ctl_reg(dev_priv, aux_ch);
 	else
-		return g4x_aux_ctl_reg(dev_priv, port);
+		return g4x_aux_ctl_reg(dev_priv, aux_ch);
 }
 
 static i915_reg_t intel_aux_data_reg(struct drm_i915_private *dev_priv,
-				     enum port port, int index)
+				     enum aux_ch aux_ch, int index)
 {
 	if (INTEL_GEN(dev_priv) >= 9)
-		return skl_aux_data_reg(dev_priv, port, index);
+		return skl_aux_data_reg(dev_priv, aux_ch, index);
 	else if (HAS_PCH_SPLIT(dev_priv))
-		return ilk_aux_data_reg(dev_priv, port, index);
+		return ilk_aux_data_reg(dev_priv, aux_ch, index);
 	else
-		return g4x_aux_data_reg(dev_priv, port, index);
+		return g4x_aux_data_reg(dev_priv, aux_ch, index);
 }
 
 static void intel_aux_reg_init(struct intel_dp *intel_dp)
 {
 	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
-	enum port port = intel_aux_port(dev_priv,
-					dp_to_dig_port(intel_dp)->base.port);
+	enum aux_ch aux_ch = intel_dp->aux_ch;
 	int i;
 
-	intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port);
+	intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, aux_ch);
 	for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
-		intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i);
+		intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, aux_ch, i);
 }
 
 static void
@@ -1507,14 +1530,17 @@ intel_dp_aux_fini(struct intel_dp *intel_dp)
 static void
 intel_dp_aux_init(struct intel_dp *intel_dp)
 {
-	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	enum port port = intel_dig_port->base.port;
+	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
+
+	intel_dp->aux_ch = intel_aux_ch(intel_dp);
+	intel_dp->aux_power_domain = intel_aux_power_domain(intel_dp);
 
 	intel_aux_reg_init(intel_dp);
 	drm_dp_aux_init(&intel_dp->aux);
 
 	/* Failure to allocate our preferred name is not critical */
-	intel_dp->aux.name = kasprintf(GFP_KERNEL, "DPDDC-%c", port_name(port));
+	intel_dp->aux.name = kasprintf(GFP_KERNEL, "DPDDC-%c",
+				       port_name(encoder->port));
 	intel_dp->aux.transfer = intel_dp_aux_transfer;
 }
 
@@ -6266,42 +6292,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	return false;
 }
 
-/* Set up the hotplug pin and aux power domain. */
-static void
-intel_dp_init_connector_port_info(struct intel_digital_port *intel_dig_port)
-{
-	struct intel_encoder *encoder = &intel_dig_port->base;
-	struct intel_dp *intel_dp = &intel_dig_port->dp;
-	struct intel_encoder *intel_encoder = &intel_dig_port->base;
-	struct drm_i915_private *dev_priv = to_i915(intel_encoder->base.dev);
-
-	encoder->hpd_pin = intel_hpd_pin_default(dev_priv, encoder->port);
-
-	switch (encoder->port) {
-	case PORT_A:
-		intel_dp->aux_power_domain = POWER_DOMAIN_AUX_A;
-		break;
-	case PORT_B:
-		intel_dp->aux_power_domain = POWER_DOMAIN_AUX_B;
-		break;
-	case PORT_C:
-		intel_dp->aux_power_domain = POWER_DOMAIN_AUX_C;
-		break;
-	case PORT_D:
-		intel_dp->aux_power_domain = POWER_DOMAIN_AUX_D;
-		break;
-	case PORT_E:
-		/* FIXME: Check VBT for actual wiring of PORT E */
-		intel_dp->aux_power_domain = POWER_DOMAIN_AUX_D;
-		break;
-	case PORT_F:
-		intel_dp->aux_power_domain = POWER_DOMAIN_AUX_F;
-		break;
-	default:
-		MISSING_CASE(encoder->port);
-	}
-}
-
 static void intel_dp_modeset_retry_work_fn(struct work_struct *work)
 {
 	struct intel_connector *intel_connector;
@@ -6407,7 +6397,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 		connector->interlace_allowed = true;
 	connector->doublescan_allowed = 0;
 
-	intel_dp_init_connector_port_info(intel_dig_port);
+	intel_encoder->hpd_pin = intel_hpd_pin_default(dev_priv, port);
 
 	intel_dp_aux_init(intel_dp);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 50874f4035cf..b70ed154c4ce 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1052,6 +1052,7 @@ struct intel_dp {
 	bool detect_done;
 	bool channel_eq_status;
 	bool reset_link_params;
+	enum aux_ch aux_ch;
 	uint8_t dpcd[DP_RECEIVER_CAP_SIZE];
 	uint8_t psr_dpcd[EDP_PSR_RECEIVER_CAP_SIZE];
 	uint8_t downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
-- 
2.14.1

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

[PATCH 2/2] drm/i915/cnl: New power domain for AUX IO.

[Dhinakaran Pandiyan]

PSR on CNL requires AUX IO wells to be kept on and the existing AUX domain
for AUX-A enables DC_OFF well too. This is not required, so add a new
AUX_IO_A domain for AUX-A to allow DC states to remain enabled. Other AUX
channels re-use the existing AUX domains as they do need power well 2.

v2: Add AUX IO domain only for AUX-A
    Rebased on top of Ville's AUX series.

Cc: Imre Deak <imre.deak@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Suggested-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_display.h    |  1 +
 drivers/gpu/drm/i915/intel_dp.c         |  2 +-
 drivers/gpu/drm/i915/intel_drv.h        |  1 +
 drivers/gpu/drm/i915/intel_psr.c        | 37 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
 5 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
index f5733a2576e7..4e7418b345bc 100644
--- a/drivers/gpu/drm/i915/intel_display.h
+++ b/drivers/gpu/drm/i915/intel_display.h
@@ -186,6 +186,7 @@ enum intel_display_power_domain {
 	POWER_DOMAIN_AUX_C,
 	POWER_DOMAIN_AUX_D,
 	POWER_DOMAIN_AUX_F,
+	POWER_DOMAIN_AUX_IO_A,
 	POWER_DOMAIN_GMBUS,
 	POWER_DOMAIN_MODESET,
 	POWER_DOMAIN_GT_IRQ,
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index eeb8a026fd08..777682a925c9 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1331,7 +1331,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 	return ret;
 }
 
-static enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
+enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
 {
 	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index b70ed154c4ce..725a5b8ab611 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1684,6 +1684,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
 int intel_dp_link_required(int pixel_clock, int bpp);
 int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
 bool intel_digital_port_connected(struct intel_encoder *encoder);
+enum aux_ch intel_aux_ch(struct intel_dp *intel_dp);
 
 /* intel_dp_aux_backlight.c */
 int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 2ef374f936b9..ff77b505534c 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -56,6 +56,39 @@
 #include "intel_drv.h"
 #include "i915_drv.h"
 
+static void psr_power_get(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
+	enum intel_display_power_domain aux_domain;
+
+	if (INTEL_GEN(dev_priv) < 10)
+		return;
+
+	/* CNL HW requires corresponding AUX IOs to be powered up for PSR, AUX-A
+	 * does not require the driver to disable DC states, but the rest do.
+	 * Although PSR is enabled only on Port A currently, let's do this
+	 * correctly for other ports too.
+	 */
+	aux_domain = intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
+						    intel_dp->aux_power_domain;
+	intel_display_power_get(dev_priv, aux_domain);
+}
+
+static void psr_power_put(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
+	enum intel_display_power_domain aux_domain;
+
+	if (INTEL_GEN(dev_priv) < 10)
+		return;
+
+	aux_domain = intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
+						    intel_dp->aux_power_domain;
+	intel_display_power_put(dev_priv, aux_domain);
+}
+
 static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -459,6 +492,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
 	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
 	u32 chicken;
 
+	psr_power_get(intel_dp);
+
 	if (dev_priv->psr.psr2_support) {
 		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
 		if (dev_priv->psr.y_cord_support)
@@ -617,6 +652,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
 		else
 			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
 	}
+
+	psr_power_put(intel_dp);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index b7924feb9f27..53ea564f971e 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -130,6 +130,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
 		return "AUX_D";
 	case POWER_DOMAIN_AUX_F:
 		return "AUX_F";
+	case POWER_DOMAIN_AUX_IO_A:
+		return "AUX_IO_A";
 	case POWER_DOMAIN_GMBUS:
 		return "GMBUS";
 	case POWER_DOMAIN_INIT:
@@ -1853,6 +1855,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 	BIT_ULL(POWER_DOMAIN_INIT))
 #define CNL_DISPLAY_AUX_A_POWER_DOMAINS (		\
 	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
+	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
 	BIT_ULL(POWER_DOMAIN_INIT))
 #define CNL_DISPLAY_AUX_B_POWER_DOMAINS (		\
 	BIT_ULL(POWER_DOMAIN_AUX_B) |			\
-- 
2.14.1

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

Re: [PATCH 1/2] drm/i915: Add enum aux_ch and clean up the aux init to use it

[Pandiyan, Dhinakaran]

On Wed, 2018-02-21 at 23:28 -0800, Dhinakaran Pandiyan wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Since we no longer have a 1:1 correspondence between ports and AUX
> channels, let's give AUX channels their own enum. Makes it easier
> to tell the apples from the oranges, and we get rid of the
> port E AUX power domain FIXME since we now derive the power domain
> from the actual AUX CH.
> 
> v2: Rebase due to AUX F

Patch 2/2 has a dependency on this patch from Ville, so I have included
this patch for CI to test them together.

-DK
_______________________________________________
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 [1/2] drm/i915: Add enum aux_ch and clean up the aux init to use it

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Add enum aux_ch and clean up the aux init to use it
URL   : https://patchwork.freedesktop.org/series/38744/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
b28837aa5cbe drm/i915: Add enum aux_ch and clean up the aux init to use it
-:31: WARNING: line over 80 characters
#31: FILE: drivers/gpu/drm/i915/i915_reg.h:5350:
+#define DP_AUX_CH_CTL(aux_ch)	_MMIO_PORT(aux_ch, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)

-:32: WARNING: line over 80 characters
#32: FILE: drivers/gpu/drm/i915/i915_reg.h:5351:
+#define DP_AUX_CH_DATA(aux_ch, i)	_MMIO(_PORT(aux_ch, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */

-:42: WARNING: line over 80 characters
#42: FILE: drivers/gpu/drm/i915/i915_reg.h:7878:
+#define PCH_DP_AUX_CH_CTL(aux_ch)		_MMIO_PORT((aux_ch) - AUX_CH_B, _PCH_DPB_AUX_CH_CTL, _PCH_DPC_AUX_CH_CTL)

-:43: WARNING: line over 80 characters
#43: FILE: drivers/gpu/drm/i915/i915_reg.h:7879:
+#define PCH_DP_AUX_CH_DATA(aux_ch, i)	_MMIO(_PORT((aux_ch) - AUX_CH_B, _PCH_DPB_AUX_CH_DATA1, _PCH_DPC_AUX_CH_DATA1) + (i) * 4) /* 5 registers */

-:90: CHECK: No space is necessary after a cast
#90: FILE: drivers/gpu/drm/i915/intel_dp.c:1344:
+		aux_ch = (enum aux_ch) port;

-:349: WARNING: line over 80 characters
#349: FILE: drivers/gpu/drm/i915/intel_dp.c:1521:
+		intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, aux_ch, i);

total: 0 errors, 5 warnings, 1 checks, 393 lines checked
daf9018b0155 drm/i915/cnl: New power domain for AUX IO.
-:71: WARNING: line over 80 characters
#71: FILE: drivers/gpu/drm/i915/intel_psr.c:62:
+	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);

-:90: WARNING: line over 80 characters
#90: FILE: drivers/gpu/drm/i915/intel_psr.c:81:
+	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);

total: 0 errors, 2 warnings, 0 checks, 92 lines checked

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

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Add enum aux_ch and clean up the aux init to use it

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Add enum aux_ch and clean up the aux init to use it
URL   : https://patchwork.freedesktop.org/series/38744/
State : success

== Summary ==

Series 38744v1 series starting with [1/2] drm/i915: Add enum aux_ch and clean up the aux init to use it
https://patchwork.freedesktop.org/api/1.0/series/38744/revisions/1/mbox/

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-skl-6260u) fdo#104108
                incomplete -> PASS       (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#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
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:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:420s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:426s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:374s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:489s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:286s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:480s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:482s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:467s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:465s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:560s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:416s
fi-gdg-551       total:288  pass:180  dwarn:0   dfail:0   fail:0   skip:108 time:284s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:508s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:388s
fi-ilk-650       total:288  pass:227  dwarn:0   dfail:0   fail:1   skip:60  time:415s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:447s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:411s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:457s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:494s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:450s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:495s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:584s
fi-skl-6260u     total:246  pass:229  dwarn:0   dfail:0   fail:0   skip:16 
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:501s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:520s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:488s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:481s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:432s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:518s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:394s

42016703e66b7b572d4ab651946b715cdbff3050 drm-tip: 2018y-02m-21d-21h-26m-53s UTC integration manifest
daf9018b0155 drm/i915/cnl: New power domain for AUX IO.
b28837aa5cbe drm/i915: Add enum aux_ch and clean up the aux init to use it

== Logs ==

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

✗ Fi.CI.IGT: warning for series starting with [1/2] drm/i915: Add enum aux_ch and clean up the aux init to use it

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Add enum aux_ch and clean up the aux init to use it
URL   : https://patchwork.freedesktop.org/series/38744/
State : warning

== Summary ==

Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-c-frame-sequence:
                pass       -> FAIL       (shard-apl) fdo#103481
Test kms_chv_cursor_fail:
        Subgroup pipe-b-128x128-top-edge:
                dmesg-warn -> PASS       (shard-snb) fdo#105185
        Subgroup pipe-b-64x64-left-edge:
                pass       -> DMESG-WARN (shard-snb)
Test perf:
        Subgroup oa-exponents:
                incomplete -> PASS       (shard-apl) fdo#102254
        Subgroup buffer-fill:
                pass       -> FAIL       (shard-apl) fdo#103755
Test kms_flip:
        Subgroup modeset-vs-vblank-race-interruptible:
                fail       -> PASS       (shard-apl) fdo#103060 +1
        Subgroup 2x-plain-flip-ts-check-interruptible:
                pass       -> FAIL       (shard-hsw) fdo#100368

fdo#103481 https://bugs.freedesktop.org/show_bug.cgi?id=103481
fdo#105185 https://bugs.freedesktop.org/show_bug.cgi?id=105185
fdo#102254 https://bugs.freedesktop.org/show_bug.cgi?id=102254
fdo#103755 https://bugs.freedesktop.org/show_bug.cgi?id=103755
fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368

shard-apl        total:3369 pass:1764 dwarn:1   dfail:0   fail:14  skip:1589 time:12069s
shard-hsw        total:3464 pass:1766 dwarn:1   dfail:0   fail:3   skip:1693 time:11798s
shard-snb        total:3464 pass:1355 dwarn:2   dfail:0   fail:3   skip:2104 time:6633s
Blacklisted hosts:
shard-kbl        total:3464 pass:1960 dwarn:1   dfail:0   fail:15  skip:1488 time:9698s

== Logs ==

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

Re: [PATCH 2/2] drm/i915/cnl: New power domain for AUX IO.

[Imre Deak]

On Wed, Feb 21, 2018 at 11:28:56PM -0800, Dhinakaran Pandiyan wrote:
> PSR on CNL requires AUX IO wells to be kept on and the existing AUX domain
> for AUX-A enables DC_OFF well too. This is not required, so add a new
> AUX_IO_A domain for AUX-A to allow DC states to remain enabled. Other AUX
> channels re-use the existing AUX domains as they do need power well 2.
> 
> v2: Add AUX IO domain only for AUX-A
>     Rebased on top of Ville's AUX series.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Suggested-by: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.h    |  1 +
>  drivers/gpu/drm/i915/intel_dp.c         |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h        |  1 +
>  drivers/gpu/drm/i915/intel_psr.c        | 37 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
>  5 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> index f5733a2576e7..4e7418b345bc 100644
> --- a/drivers/gpu/drm/i915/intel_display.h
> +++ b/drivers/gpu/drm/i915/intel_display.h
> @@ -186,6 +186,7 @@ enum intel_display_power_domain {
>  	POWER_DOMAIN_AUX_C,
>  	POWER_DOMAIN_AUX_D,
>  	POWER_DOMAIN_AUX_F,
> +	POWER_DOMAIN_AUX_IO_A,
>  	POWER_DOMAIN_GMBUS,
>  	POWER_DOMAIN_MODESET,
>  	POWER_DOMAIN_GT_IRQ,
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index eeb8a026fd08..777682a925c9 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1331,7 +1331,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>  	return ret;
>  }
>  
> -static enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
>  {
>  	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index b70ed154c4ce..725a5b8ab611 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1684,6 +1684,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
>  int intel_dp_link_required(int pixel_clock, int bpp);
>  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
>  bool intel_digital_port_connected(struct intel_encoder *encoder);
> +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp);
>  
>  /* intel_dp_aux_backlight.c */
>  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 2ef374f936b9..ff77b505534c 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -56,6 +56,39 @@
>  #include "intel_drv.h"
>  #include "i915_drv.h"
>  
> +static void psr_power_get(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> +	enum intel_display_power_domain aux_domain;
> +
> +	if (INTEL_GEN(dev_priv) < 10)
> +		return;
> +
> +	/* CNL HW requires corresponding AUX IOs to be powered up for PSR, AUX-A
> +	 * does not require the driver to disable DC states, but the rest do.
> +	 * Although PSR is enabled only on Port A currently, let's do this
> +	 * correctly for other ports too.
> +	 */
> +	aux_domain = intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :

The AUX power well is port specific, while there could be an alternative
port->aux-channel mapping. So we should check the port here. 

> +						    intel_dp->aux_power_domain;
> +	intel_display_power_get(dev_priv, aux_domain);
> +}
> +
> +static void psr_power_put(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> +	enum intel_display_power_domain aux_domain;
> +
> +	if (INTEL_GEN(dev_priv) < 10)
> +		return;
> +
> +	aux_domain = intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
> +						    intel_dp->aux_power_domain;
> +	intel_display_power_put(dev_priv, aux_domain);
> +}
> +
>  static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -459,6 +492,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
>  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>  	u32 chicken;
>  
> +	psr_power_get(intel_dp);
> +
>  	if (dev_priv->psr.psr2_support) {
>  		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
>  		if (dev_priv->psr.y_cord_support)
> @@ -617,6 +652,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
>  		else
>  			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
>  	}
> +
> +	psr_power_put(intel_dp);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index b7924feb9f27..53ea564f971e 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -130,6 +130,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
>  		return "AUX_D";
>  	case POWER_DOMAIN_AUX_F:
>  		return "AUX_F";
> +	case POWER_DOMAIN_AUX_IO_A:
> +		return "AUX_IO_A";
>  	case POWER_DOMAIN_GMBUS:
>  		return "GMBUS";
>  	case POWER_DOMAIN_INIT:
> @@ -1853,6 +1855,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  #define CNL_DISPLAY_AUX_A_POWER_DOMAINS (		\
>  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  #define CNL_DISPLAY_AUX_B_POWER_DOMAINS (		\
>  	BIT_ULL(POWER_DOMAIN_AUX_B) |			\
> -- 
> 2.14.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/cnl: New power domain for AUX IO.

[Imre Deak]

On Thu, Feb 22, 2018 at 12:28:11PM +0200, Imre Deak wrote:
> On Wed, Feb 21, 2018 at 11:28:56PM -0800, Dhinakaran Pandiyan wrote:
> > PSR on CNL requires AUX IO wells to be kept on and the existing AUX domain
> > for AUX-A enables DC_OFF well too. This is not required, so add a new
> > AUX_IO_A domain for AUX-A to allow DC states to remain enabled. Other AUX
> > channels re-use the existing AUX domains as they do need power well 2.
> > 
> > v2: Add AUX IO domain only for AUX-A
> >     Rebased on top of Ville's AUX series.
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Suggested-by: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.h    |  1 +
> >  drivers/gpu/drm/i915/intel_dp.c         |  2 +-
> >  drivers/gpu/drm/i915/intel_drv.h        |  1 +
> >  drivers/gpu/drm/i915/intel_psr.c        | 37 +++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
> >  5 files changed, 43 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > index f5733a2576e7..4e7418b345bc 100644
> > --- a/drivers/gpu/drm/i915/intel_display.h
> > +++ b/drivers/gpu/drm/i915/intel_display.h
> > @@ -186,6 +186,7 @@ enum intel_display_power_domain {
> >  	POWER_DOMAIN_AUX_C,
> >  	POWER_DOMAIN_AUX_D,
> >  	POWER_DOMAIN_AUX_F,
> > +	POWER_DOMAIN_AUX_IO_A,
> >  	POWER_DOMAIN_GMBUS,
> >  	POWER_DOMAIN_MODESET,
> >  	POWER_DOMAIN_GT_IRQ,
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index eeb8a026fd08..777682a925c9 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1331,7 +1331,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> >  	return ret;
> >  }
> >  
> > -static enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> >  {
> >  	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index b70ed154c4ce..725a5b8ab611 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1684,6 +1684,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
> >  int intel_dp_link_required(int pixel_clock, int bpp);
> >  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
> >  bool intel_digital_port_connected(struct intel_encoder *encoder);
> > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp);
> >  
> >  /* intel_dp_aux_backlight.c */
> >  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 2ef374f936b9..ff77b505534c 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -56,6 +56,39 @@
> >  #include "intel_drv.h"
> >  #include "i915_drv.h"
> >  
> > +static void psr_power_get(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > +	enum intel_display_power_domain aux_domain;
> > +
> > +	if (INTEL_GEN(dev_priv) < 10)
> > +		return;
> > +
> > +	/* CNL HW requires corresponding AUX IOs to be powered up for PSR, AUX-A
> > +	 * does not require the driver to disable DC states, but the rest do.
> > +	 * Although PSR is enabled only on Port A currently, let's do this
> > +	 * correctly for other ports too.
> > +	 */
> > +	aux_domain = intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
> 
> The AUX power well is port specific, while there could be an alternative
> port->aux-channel mapping. So we should check the port here. 

At least that's how the driver worked so far .. but that was incorrect
as confirmed now by Art, and Ville's patch fixed it. So you can ignore
the above:/

It's still unclear how this works on ICL onwards at least in TBT and
USBC modes, the selection there could be port specific. Will try to
clarify that with Art.

--Imre

> 
> > +						    intel_dp->aux_power_domain;
> > +	intel_display_power_get(dev_priv, aux_domain);
> > +}
> > +
> > +static void psr_power_put(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > +	enum intel_display_power_domain aux_domain;
> > +
> > +	if (INTEL_GEN(dev_priv) < 10)
> > +		return;
> > +
> > +	aux_domain = intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
> > +						    intel_dp->aux_power_domain;
> > +	intel_display_power_put(dev_priv, aux_domain);
> > +}
> > +
> >  static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > @@ -459,6 +492,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> >  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> >  	u32 chicken;
> >  
> > +	psr_power_get(intel_dp);
> > +
> >  	if (dev_priv->psr.psr2_support) {
> >  		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> >  		if (dev_priv->psr.y_cord_support)
> > @@ -617,6 +652,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
> >  		else
> >  			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> >  	}
> > +
> > +	psr_power_put(intel_dp);
> >  }
> >  
> >  /**
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index b7924feb9f27..53ea564f971e 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -130,6 +130,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> >  		return "AUX_D";
> >  	case POWER_DOMAIN_AUX_F:
> >  		return "AUX_F";
> > +	case POWER_DOMAIN_AUX_IO_A:
> > +		return "AUX_IO_A";
> >  	case POWER_DOMAIN_GMBUS:
> >  		return "GMBUS";
> >  	case POWER_DOMAIN_INIT:
> > @@ -1853,6 +1855,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> >  	BIT_ULL(POWER_DOMAIN_INIT))
> >  #define CNL_DISPLAY_AUX_A_POWER_DOMAINS (		\
> >  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> > +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
> >  	BIT_ULL(POWER_DOMAIN_INIT))
> >  #define CNL_DISPLAY_AUX_B_POWER_DOMAINS (		\
> >  	BIT_ULL(POWER_DOMAIN_AUX_B) |			\
> > -- 
> > 2.14.1
> > 
> _______________________________________________
> 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 2/2] drm/i915/cnl: New power domain for AUX IO.

[Ville Syrjälä]

On Wed, Feb 21, 2018 at 11:28:56PM -0800, Dhinakaran Pandiyan wrote:
> PSR on CNL requires AUX IO wells to be kept on and the existing AUX domain
> for AUX-A enables DC_OFF well too. This is not required, so add a new
> AUX_IO_A domain for AUX-A to allow DC states to remain enabled. Other AUX
> channels re-use the existing AUX domains as they do need power well 2.
> 
> v2: Add AUX IO domain only for AUX-A
>     Rebased on top of Ville's AUX series.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Suggested-by: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.h    |  1 +
>  drivers/gpu/drm/i915/intel_dp.c         |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h        |  1 +
>  drivers/gpu/drm/i915/intel_psr.c        | 37 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
>  5 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> index f5733a2576e7..4e7418b345bc 100644
> --- a/drivers/gpu/drm/i915/intel_display.h
> +++ b/drivers/gpu/drm/i915/intel_display.h
> @@ -186,6 +186,7 @@ enum intel_display_power_domain {
>  	POWER_DOMAIN_AUX_C,
>  	POWER_DOMAIN_AUX_D,
>  	POWER_DOMAIN_AUX_F,
> +	POWER_DOMAIN_AUX_IO_A,
>  	POWER_DOMAIN_GMBUS,
>  	POWER_DOMAIN_MODESET,
>  	POWER_DOMAIN_GT_IRQ,
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index eeb8a026fd08..777682a925c9 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1331,7 +1331,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>  	return ret;
>  }
>  
> -static enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
>  {
>  	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index b70ed154c4ce..725a5b8ab611 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1684,6 +1684,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
>  int intel_dp_link_required(int pixel_clock, int bpp);
>  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
>  bool intel_digital_port_connected(struct intel_encoder *encoder);
> +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp);
>  
>  /* intel_dp_aux_backlight.c */
>  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 2ef374f936b9..ff77b505534c 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -56,6 +56,39 @@
>  #include "intel_drv.h"
>  #include "i915_drv.h"
>  
> +static void psr_power_get(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> +	enum intel_display_power_domain aux_domain;
> +
> +	if (INTEL_GEN(dev_priv) < 10)
> +		return;
> +
> +	/* CNL HW requires corresponding AUX IOs to be powered up for PSR, AUX-A
> +	 * does not require the driver to disable DC states, but the rest do.
> +	 * Although PSR is enabled only on Port A currently, let's do this
> +	 * correctly for other ports too.
> +	 */
> +	aux_domain = intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
> +						    intel_dp->aux_power_domain;

Could put that logic into a small helper to avoid having to duplicate it
in both get and put.

> +	intel_display_power_get(dev_priv, aux_domain);
> +}
> +
> +static void psr_power_put(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> +	enum intel_display_power_domain aux_domain;
> +
> +	if (INTEL_GEN(dev_priv) < 10)
> +		return;
> +
> +	aux_domain = intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
> +						    intel_dp->aux_power_domain;
> +	intel_display_power_put(dev_priv, aux_domain);
> +}
> +
>  static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -459,6 +492,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
>  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>  	u32 chicken;
>  
> +	psr_power_get(intel_dp);
> +
>  	if (dev_priv->psr.psr2_support) {
>  		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
>  		if (dev_priv->psr.y_cord_support)
> @@ -617,6 +652,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
>  		else
>  			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
>  	}
> +
> +	psr_power_put(intel_dp);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index b7924feb9f27..53ea564f971e 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -130,6 +130,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
>  		return "AUX_D";
>  	case POWER_DOMAIN_AUX_F:
>  		return "AUX_F";
> +	case POWER_DOMAIN_AUX_IO_A:
> +		return "AUX_IO_A";
>  	case POWER_DOMAIN_GMBUS:
>  		return "GMBUS";
>  	case POWER_DOMAIN_INIT:
> @@ -1853,6 +1855,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  #define CNL_DISPLAY_AUX_A_POWER_DOMAINS (		\
>  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  #define CNL_DISPLAY_AUX_B_POWER_DOMAINS (		\
>  	BIT_ULL(POWER_DOMAIN_AUX_B) |			\
> -- 
> 2.14.1

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

[PATCH v3] drm/i915/psr: New power domain for AUX IO.

[Dhinakaran Pandiyan]

PSR on CNL requires AUX IO wells to be kept on and the existing AUX domain
for AUX-A enables DC_OFF well too. This is not required, so add a new
AUX_IO_A domain for AUX-A to allow DC states to remain enabled. Other AUX
channels re-use the existing AUX domains as they do need power well 2.

v3: Extract aux domain selection into a function (Ville)
v2: Add AUX IO domain only for AUX-A
    Rebased on top of Ville's AUX series.

Cc: Imre Deak <imre.deak@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Suggested-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_display.h    |  1 +
 drivers/gpu/drm/i915/intel_dp.c         |  2 +-
 drivers/gpu/drm/i915/intel_drv.h        |  1 +
 drivers/gpu/drm/i915/intel_psr.c        | 38 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
 5 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
index f5733a2576e7..4e7418b345bc 100644
--- a/drivers/gpu/drm/i915/intel_display.h
+++ b/drivers/gpu/drm/i915/intel_display.h
@@ -186,6 +186,7 @@ enum intel_display_power_domain {
 	POWER_DOMAIN_AUX_C,
 	POWER_DOMAIN_AUX_D,
 	POWER_DOMAIN_AUX_F,
+	POWER_DOMAIN_AUX_IO_A,
 	POWER_DOMAIN_GMBUS,
 	POWER_DOMAIN_MODESET,
 	POWER_DOMAIN_GT_IRQ,
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 005735a7fc29..b78b9972ebae 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1331,7 +1331,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 	return ret;
 }
 
-static enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
+enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
 {
 	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5132f6a58c6d..bcd6dc9fb13d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1685,6 +1685,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
 int intel_dp_link_required(int pixel_clock, int bpp);
 int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
 bool intel_digital_port_connected(struct intel_encoder *encoder);
+enum aux_ch intel_aux_ch(struct intel_dp *intel_dp);
 
 /* intel_dp_aux_backlight.c */
 int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 2ef374f936b9..03814f7bc2d3 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -56,6 +56,40 @@
 #include "intel_drv.h"
 #include "i915_drv.h"
 
+static inline enum intel_display_power_domain
+psr_aux_domain(struct intel_dp *intel_dp)
+{
+	/* CNL HW requires corresponding AUX IOs to be powered up for PSR, AUX-A
+	 * does not require the driver to disable DC states, but the rest do.
+	 * Although PSR is enabled only on Port A currently, let's do this
+	 * correctly for other ports too.
+	 */
+	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
+					      intel_dp->aux_power_domain;
+}
+
+static void psr_power_get(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
+
+	if (INTEL_GEN(dev_priv) < 10)
+		return;
+
+	intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
+}
+
+static void psr_power_put(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
+
+	if (INTEL_GEN(dev_priv) < 10)
+		return;
+
+	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
+}
+
 static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -459,6 +493,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
 	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
 	u32 chicken;
 
+	psr_power_get(intel_dp);
+
 	if (dev_priv->psr.psr2_support) {
 		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
 		if (dev_priv->psr.y_cord_support)
@@ -617,6 +653,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
 		else
 			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
 	}
+
+	psr_power_put(intel_dp);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index b7924feb9f27..53ea564f971e 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -130,6 +130,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
 		return "AUX_D";
 	case POWER_DOMAIN_AUX_F:
 		return "AUX_F";
+	case POWER_DOMAIN_AUX_IO_A:
+		return "AUX_IO_A";
 	case POWER_DOMAIN_GMBUS:
 		return "GMBUS";
 	case POWER_DOMAIN_INIT:
@@ -1853,6 +1855,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 	BIT_ULL(POWER_DOMAIN_INIT))
 #define CNL_DISPLAY_AUX_A_POWER_DOMAINS (		\
 	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
+	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
 	BIT_ULL(POWER_DOMAIN_INIT))
 #define CNL_DISPLAY_AUX_B_POWER_DOMAINS (		\
 	BIT_ULL(POWER_DOMAIN_AUX_B) |			\
-- 
2.14.1

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

Re: [PATCH v3] drm/i915/psr: New power domain for AUX IO.

[Rodrigo Vivi]

On Thu, Feb 22, 2018 at 12:28:50PM -0800, Dhinakaran Pandiyan wrote:
> PSR on CNL requires AUX IO wells to be kept on and the existing AUX domain
> for AUX-A enables DC_OFF well too. This is not required, so add a new
> AUX_IO_A domain for AUX-A to allow DC states to remain enabled. Other AUX
> channels re-use the existing AUX domains as they do need power well 2.
> 
> v3: Extract aux domain selection into a function (Ville)
> v2: Add AUX IO domain only for AUX-A
>     Rebased on top of Ville's AUX series.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Suggested-by: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.h    |  1 +
>  drivers/gpu/drm/i915/intel_dp.c         |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h        |  1 +
>  drivers/gpu/drm/i915/intel_psr.c        | 38 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
>  5 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> index f5733a2576e7..4e7418b345bc 100644
> --- a/drivers/gpu/drm/i915/intel_display.h
> +++ b/drivers/gpu/drm/i915/intel_display.h
> @@ -186,6 +186,7 @@ enum intel_display_power_domain {
>  	POWER_DOMAIN_AUX_C,
>  	POWER_DOMAIN_AUX_D,
>  	POWER_DOMAIN_AUX_F,
> +	POWER_DOMAIN_AUX_IO_A,
>  	POWER_DOMAIN_GMBUS,
>  	POWER_DOMAIN_MODESET,
>  	POWER_DOMAIN_GT_IRQ,
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 005735a7fc29..b78b9972ebae 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1331,7 +1331,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>  	return ret;
>  }
>  
> -static enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
>  {
>  	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5132f6a58c6d..bcd6dc9fb13d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1685,6 +1685,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
>  int intel_dp_link_required(int pixel_clock, int bpp);
>  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
>  bool intel_digital_port_connected(struct intel_encoder *encoder);
> +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp);
>  
>  /* intel_dp_aux_backlight.c */
>  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 2ef374f936b9..03814f7bc2d3 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -56,6 +56,40 @@
>  #include "intel_drv.h"
>  #include "i915_drv.h"
>  
> +static inline enum intel_display_power_domain
> +psr_aux_domain(struct intel_dp *intel_dp)
> +{
> +	/* CNL HW requires corresponding AUX IOs to be powered up for PSR, AUX-A
> +	 * does not require the driver to disable DC states, but the rest do.

This phrase is strange. Specially "the rest do" part.
It seems that we need to disable DC states on other ports than A,
what it is not true.

> +	 * Although PSR is enabled only on Port A currently, let's do this
> +	 * correctly for other ports too.
> +	 */
> +	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
> +					      intel_dp->aux_power_domain;
> +}
> +
> +static void psr_power_get(struct intel_dp *intel_dp)

nip: psr_aux_io_power_get ?!

> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> +
> +	if (INTEL_GEN(dev_priv) < 10)
> +		return;
> +
> +	intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
> +}
> +
> +static void psr_power_put(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> +
> +	if (INTEL_GEN(dev_priv) < 10)
> +		return;
> +
> +	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
> +}
> +
>  static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -459,6 +493,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
>  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>  	u32 chicken;
>  
> +	psr_power_get(intel_dp);
> +
>  	if (dev_priv->psr.psr2_support) {
>  		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
>  		if (dev_priv->psr.y_cord_support)
> @@ -617,6 +653,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
>  		else
>  			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
>  	}
> +
> +	psr_power_put(intel_dp);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index b7924feb9f27..53ea564f971e 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -130,6 +130,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
>  		return "AUX_D";
>  	case POWER_DOMAIN_AUX_F:
>  		return "AUX_F";
> +	case POWER_DOMAIN_AUX_IO_A:
> +		return "AUX_IO_A";
>  	case POWER_DOMAIN_GMBUS:
>  		return "GMBUS";
>  	case POWER_DOMAIN_INIT:
> @@ -1853,6 +1855,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  #define CNL_DISPLAY_AUX_A_POWER_DOMAINS (		\
>  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  #define CNL_DISPLAY_AUX_B_POWER_DOMAINS (		\
>  	BIT_ULL(POWER_DOMAIN_AUX_B) |			\
> -- 
> 2.14.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Add enum aux_ch and clean up the aux init to use it (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Add enum aux_ch and clean up the aux init to use it (rev2)
URL   : https://patchwork.freedesktop.org/series/38744/
State : success

== Summary ==

Series 38744v2 series starting with [1/2] drm/i915: Add enum aux_ch and clean up the aux init to use it
https://patchwork.freedesktop.org/api/1.0/series/38744/revisions/2/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                pass       -> INCOMPLETE (fi-snb-2520m) fdo#103713
Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                fail       -> PASS       (fi-gdg-551) fdo#102575
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a:
                fail       -> PASS       (fi-skl-6700k2)
Test prime_vgem:
        Subgroup basic-fence-flip:
                pass       -> FAIL       (fi-ivb-3770) fdo#104008

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

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:415s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:422s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:373s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:480s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:284s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:476s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:479s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:467s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:451s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:560s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:414s
fi-gdg-551       total:288  pass:180  dwarn:0   dfail:0   fail:0   skip:108 time:283s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:508s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:386s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:404s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:452s
fi-ivb-3770      total:288  pass:254  dwarn:0   dfail:0   fail:1   skip:33  time:409s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:453s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:488s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:450s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:491s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:587s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:426s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:500s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:518s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:485s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:472s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:404s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:428s
fi-snb-2520m     total:3    pass:2    dwarn:0   dfail:0   fail:0   skip:0  
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:396s
Blacklisted hosts:
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:389s

b9f1df86d4379ec76dddc07cf6e1cc85ffb26ffd drm-tip: 2018y-02m-22d-18h-25m-51s UTC integration manifest
37c6bf3d8eab drm/i915/psr: New power domain for AUX IO.
ef0d7df2f508 drm/i915: Add enum aux_ch and clean up the aux init to use it

== Logs ==

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

Re: [PATCH v3] drm/i915/psr: New power domain for AUX IO.

[Pandiyan, Dhinakaran]

On Thu, 2018-02-22 at 12:44 -0800, Rodrigo Vivi wrote:
> On Thu, Feb 22, 2018 at 12:28:50PM -0800, Dhinakaran Pandiyan wrote:
> > PSR on CNL requires AUX IO wells to be kept on and the existing AUX domain
> > for AUX-A enables DC_OFF well too. This is not required, so add a new
> > AUX_IO_A domain for AUX-A to allow DC states to remain enabled. Other AUX
> > channels re-use the existing AUX domains as they do need power well 2.
> > 
> > v3: Extract aux domain selection into a function (Ville)
> > v2: Add AUX IO domain only for AUX-A
> >     Rebased on top of Ville's AUX series.
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Suggested-by: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.h    |  1 +
> >  drivers/gpu/drm/i915/intel_dp.c         |  2 +-
> >  drivers/gpu/drm/i915/intel_drv.h        |  1 +
> >  drivers/gpu/drm/i915/intel_psr.c        | 38 +++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
> >  5 files changed, 44 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > index f5733a2576e7..4e7418b345bc 100644
> > --- a/drivers/gpu/drm/i915/intel_display.h
> > +++ b/drivers/gpu/drm/i915/intel_display.h
> > @@ -186,6 +186,7 @@ enum intel_display_power_domain {
> >  	POWER_DOMAIN_AUX_C,
> >  	POWER_DOMAIN_AUX_D,
> >  	POWER_DOMAIN_AUX_F,
> > +	POWER_DOMAIN_AUX_IO_A,
> >  	POWER_DOMAIN_GMBUS,
> >  	POWER_DOMAIN_MODESET,
> >  	POWER_DOMAIN_GT_IRQ,
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 005735a7fc29..b78b9972ebae 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1331,7 +1331,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> >  	return ret;
> >  }
> >  
> > -static enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> >  {
> >  	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 5132f6a58c6d..bcd6dc9fb13d 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1685,6 +1685,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
> >  int intel_dp_link_required(int pixel_clock, int bpp);
> >  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
> >  bool intel_digital_port_connected(struct intel_encoder *encoder);
> > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp);
> >  
> >  /* intel_dp_aux_backlight.c */
> >  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 2ef374f936b9..03814f7bc2d3 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -56,6 +56,40 @@
> >  #include "intel_drv.h"
> >  #include "i915_drv.h"
> >  
> > +static inline enum intel_display_power_domain
> > +psr_aux_domain(struct intel_dp *intel_dp)
> > +{
> > +	/* CNL HW requires corresponding AUX IOs to be powered up for PSR, AUX-A
> > +	 * does not require the driver to disable DC states, but the rest do.
> 
> This phrase is strange. Specially "the rest do" part.
> It seems that we need to disable DC states on other ports than A,
> what it is not true.

Okay, let's back up a little bit.

AUX domain for port A enables DC_OFF and AUX-IO. AUX domains for non-A
ports enable DC_OFF, power well 2 and AUX-IO, in fact all domains that
require power well 2 enable DC_OFF too. I can't see this is in bspec but
that's what the code does currently. So, this is for AUX transfers.

which means, this comment for the previous iteration of the patch
"I mean, on CNL

POWER_DOMAIN_AUX_{B,C,D,F} are exactly the same as
POWER_DOMAIN_AUX__IO_{B,C,D,F}"

was not accurate. _AUX_IO_{B,C,D,F} domains enable only AUX_IO well and
_AUX_{B,C,D,F} domains enable power well 2, DC_OFF and AUX_IO.


For PSR, we want only the AUX_IO to be enabled for any port because the
spec says -
"PSR spontaneously sends Aux transactions.  If PSR is enabled on a port,
then the associated Aux IO must be kept powered up."

which means, this comment
"Hm, so in general (for AUX transfers) to enable AUX-A we first need to
disable DC states _except_ if we enable AUX-A for PSR where we want
DC5/6 to stay enabled." applies to all ports.

To just enable the AUX-IO well, the correct version is ->
https://patchwork.freedesktop.org/patch/205328/



-DK 
> 
> > +	 * Although PSR is enabled only on Port A currently, let's do this
> > +	 * correctly for other ports too.
> > +	 */
> > +	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
> > +					      intel_dp->aux_power_domain;
> > +}
> > +
> > +static void psr_power_get(struct intel_dp *intel_dp)
> 
> nip: psr_aux_io_power_get ?!
> 
> > +{
> > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > +
> > +	if (INTEL_GEN(dev_priv) < 10)
> > +		return;
> > +
> > +	intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
> > +}
> > +
> > +static void psr_power_put(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > +
> > +	if (INTEL_GEN(dev_priv) < 10)
> > +		return;
> > +
> > +	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
> > +}
> > +
> >  static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > @@ -459,6 +493,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> >  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> >  	u32 chicken;
> >  
> > +	psr_power_get(intel_dp);
> > +
> >  	if (dev_priv->psr.psr2_support) {
> >  		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> >  		if (dev_priv->psr.y_cord_support)
> > @@ -617,6 +653,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
> >  		else
> >  			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> >  	}
> > +
> > +	psr_power_put(intel_dp);
> >  }
> >  
> >  /**
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index b7924feb9f27..53ea564f971e 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -130,6 +130,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> >  		return "AUX_D";
> >  	case POWER_DOMAIN_AUX_F:
> >  		return "AUX_F";
> > +	case POWER_DOMAIN_AUX_IO_A:
> > +		return "AUX_IO_A";
> >  	case POWER_DOMAIN_GMBUS:
> >  		return "GMBUS";
> >  	case POWER_DOMAIN_INIT:
> > @@ -1853,6 +1855,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> >  	BIT_ULL(POWER_DOMAIN_INIT))
> >  #define CNL_DISPLAY_AUX_A_POWER_DOMAINS (		\
> >  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> > +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
> >  	BIT_ULL(POWER_DOMAIN_INIT))
> >  #define CNL_DISPLAY_AUX_B_POWER_DOMAINS (		\
> >  	BIT_ULL(POWER_DOMAIN_AUX_B) |			\
> > -- 
> > 2.14.1
> > 
> _______________________________________________
> 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 v3] drm/i915/psr: New power domain for AUX IO.

[Ville Syrjälä]

On Thu, Feb 22, 2018 at 09:33:10PM +0000, Pandiyan, Dhinakaran wrote:
> 
> 
> 
> On Thu, 2018-02-22 at 12:44 -0800, Rodrigo Vivi wrote:
> > On Thu, Feb 22, 2018 at 12:28:50PM -0800, Dhinakaran Pandiyan wrote:
> > > PSR on CNL requires AUX IO wells to be kept on and the existing AUX domain
> > > for AUX-A enables DC_OFF well too. This is not required, so add a new
> > > AUX_IO_A domain for AUX-A to allow DC states to remain enabled. Other AUX
> > > channels re-use the existing AUX domains as they do need power well 2.
> > > 
> > > v3: Extract aux domain selection into a function (Ville)
> > > v2: Add AUX IO domain only for AUX-A
> > >     Rebased on top of Ville's AUX series.
> > > 
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Suggested-by: Imre Deak <imre.deak@intel.com>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.h    |  1 +
> > >  drivers/gpu/drm/i915/intel_dp.c         |  2 +-
> > >  drivers/gpu/drm/i915/intel_drv.h        |  1 +
> > >  drivers/gpu/drm/i915/intel_psr.c        | 38 +++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
> > >  5 files changed, 44 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > index f5733a2576e7..4e7418b345bc 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > @@ -186,6 +186,7 @@ enum intel_display_power_domain {
> > >  	POWER_DOMAIN_AUX_C,
> > >  	POWER_DOMAIN_AUX_D,
> > >  	POWER_DOMAIN_AUX_F,
> > > +	POWER_DOMAIN_AUX_IO_A,
> > >  	POWER_DOMAIN_GMBUS,
> > >  	POWER_DOMAIN_MODESET,
> > >  	POWER_DOMAIN_GT_IRQ,
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 005735a7fc29..b78b9972ebae 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1331,7 +1331,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> > >  	return ret;
> > >  }
> > >  
> > > -static enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > >  {
> > >  	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 5132f6a58c6d..bcd6dc9fb13d 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1685,6 +1685,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
> > >  int intel_dp_link_required(int pixel_clock, int bpp);
> > >  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
> > >  bool intel_digital_port_connected(struct intel_encoder *encoder);
> > > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp);
> > >  
> > >  /* intel_dp_aux_backlight.c */
> > >  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > index 2ef374f936b9..03814f7bc2d3 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -56,6 +56,40 @@
> > >  #include "intel_drv.h"
> > >  #include "i915_drv.h"
> > >  
> > > +static inline enum intel_display_power_domain
> > > +psr_aux_domain(struct intel_dp *intel_dp)
> > > +{
> > > +	/* CNL HW requires corresponding AUX IOs to be powered up for PSR, AUX-A
> > > +	 * does not require the driver to disable DC states, but the rest do.
> > 
> > This phrase is strange. Specially "the rest do" part.
> > It seems that we need to disable DC states on other ports than A,
> > what it is not true.
> 
> Okay, let's back up a little bit.
> 
> AUX domain for port A enables DC_OFF and AUX-IO. AUX domains for non-A
> ports enable DC_OFF, power well 2 and AUX-IO, in fact all domains that
> require power well 2 enable DC_OFF too. I can't see this is in bspec but
> that's what the code does currently. So, this is for AUX transfers.
> 
> which means, this comment for the previous iteration of the patch
> "I mean, on CNL
> 
> POWER_DOMAIN_AUX_{B,C,D,F} are exactly the same as
> POWER_DOMAIN_AUX__IO_{B,C,D,F}"
> 
> was not accurate. _AUX_IO_{B,C,D,F} domains enable only AUX_IO well and
> _AUX_{B,C,D,F} domains enable power well 2, DC_OFF and AUX_IO.
> 
> 
> For PSR, we want only the AUX_IO to be enabled for any port because the
> spec says -
> "PSR spontaneously sends Aux transactions.  If PSR is enabled on a port,
> then the associated Aux IO must be kept powered up."
> 
> which means, this comment
> "Hm, so in general (for AUX transfers) to enable AUX-A we first need to
> disable DC states _except_ if we enable AUX-A for PSR where we want
> DC5/6 to stay enabled." applies to all ports.
> 
> To just enable the AUX-IO well, the correct version is ->
> https://patchwork.freedesktop.org/patch/205328/

While that is true I don't think it actually matters in the end. As
long as transcoder A-C is enabled DC will be off and PG2 will be
enabled. So I suppose we can (ab)use that fact to save a few power
domain bits and only define AUX_IO_A. The comment should probably
be reworded to say as much.

> 
> 
> 
> -DK 
> > 
> > > +	 * Although PSR is enabled only on Port A currently, let's do this
> > > +	 * correctly for other ports too.
> > > +	 */
> > > +	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
> > > +					      intel_dp->aux_power_domain;
> > > +}
> > > +
> > > +static void psr_power_get(struct intel_dp *intel_dp)
> > 
> > nip: psr_aux_io_power_get ?!
> > 
> > > +{
> > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > > +
> > > +	if (INTEL_GEN(dev_priv) < 10)
> > > +		return;
> > > +
> > > +	intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
> > > +}
> > > +
> > > +static void psr_power_put(struct intel_dp *intel_dp)
> > > +{
> > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > > +
> > > +	if (INTEL_GEN(dev_priv) < 10)
> > > +		return;
> > > +
> > > +	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
> > > +}
> > > +
> > >  static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > @@ -459,6 +493,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> > >  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > >  	u32 chicken;
> > >  
> > > +	psr_power_get(intel_dp);
> > > +
> > >  	if (dev_priv->psr.psr2_support) {
> > >  		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> > >  		if (dev_priv->psr.y_cord_support)
> > > @@ -617,6 +653,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
> > >  		else
> > >  			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > >  	}
> > > +
> > > +	psr_power_put(intel_dp);
> > >  }
> > >  
> > >  /**
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > index b7924feb9f27..53ea564f971e 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > @@ -130,6 +130,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> > >  		return "AUX_D";
> > >  	case POWER_DOMAIN_AUX_F:
> > >  		return "AUX_F";
> > > +	case POWER_DOMAIN_AUX_IO_A:
> > > +		return "AUX_IO_A";
> > >  	case POWER_DOMAIN_GMBUS:
> > >  		return "GMBUS";
> > >  	case POWER_DOMAIN_INIT:
> > > @@ -1853,6 +1855,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > >  #define CNL_DISPLAY_AUX_A_POWER_DOMAINS (		\
> > >  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> > > +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
> > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > >  #define CNL_DISPLAY_AUX_B_POWER_DOMAINS (		\
> > >  	BIT_ULL(POWER_DOMAIN_AUX_B) |			\
> > > -- 
> > > 2.14.1
> > > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v3] drm/i915/psr: New power domain for AUX IO.

[Imre Deak]

On Thu, Feb 22, 2018 at 11:33:10PM +0200, Pandiyan, Dhinakaran wrote:
> 
> 
> 
> On Thu, 2018-02-22 at 12:44 -0800, Rodrigo Vivi wrote:
> > On Thu, Feb 22, 2018 at 12:28:50PM -0800, Dhinakaran Pandiyan wrote:
> > > PSR on CNL requires AUX IO wells to be kept on and the existing AUX domain
> > > for AUX-A enables DC_OFF well too. This is not required, so add a new
> > > AUX_IO_A domain for AUX-A to allow DC states to remain enabled. Other AUX
> > > channels re-use the existing AUX domains as they do need power well 2.
> > > 
> > > v3: Extract aux domain selection into a function (Ville)
> > > v2: Add AUX IO domain only for AUX-A
> > >     Rebased on top of Ville's AUX series.
> > > 
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Suggested-by: Imre Deak <imre.deak@intel.com>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.h    |  1 +
> > >  drivers/gpu/drm/i915/intel_dp.c         |  2 +-
> > >  drivers/gpu/drm/i915/intel_drv.h        |  1 +
> > >  drivers/gpu/drm/i915/intel_psr.c        | 38 +++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
> > >  5 files changed, 44 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > index f5733a2576e7..4e7418b345bc 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > @@ -186,6 +186,7 @@ enum intel_display_power_domain {
> > >  	POWER_DOMAIN_AUX_C,
> > >  	POWER_DOMAIN_AUX_D,
> > >  	POWER_DOMAIN_AUX_F,
> > > +	POWER_DOMAIN_AUX_IO_A,
> > >  	POWER_DOMAIN_GMBUS,
> > >  	POWER_DOMAIN_MODESET,
> > >  	POWER_DOMAIN_GT_IRQ,
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 005735a7fc29..b78b9972ebae 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1331,7 +1331,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> > >  	return ret;
> > >  }
> > >  
> > > -static enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > >  {
> > >  	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 5132f6a58c6d..bcd6dc9fb13d 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1685,6 +1685,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
> > >  int intel_dp_link_required(int pixel_clock, int bpp);
> > >  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
> > >  bool intel_digital_port_connected(struct intel_encoder *encoder);
> > > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp);
> > >  
> > >  /* intel_dp_aux_backlight.c */
> > >  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > index 2ef374f936b9..03814f7bc2d3 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -56,6 +56,40 @@
> > >  #include "intel_drv.h"
> > >  #include "i915_drv.h"
> > >  
> > > +static inline enum intel_display_power_domain
> > > +psr_aux_domain(struct intel_dp *intel_dp)
> > > +{
> > > +	/* CNL HW requires corresponding AUX IOs to be powered up for PSR, AUX-A
> > > +	 * does not require the driver to disable DC states, but the rest do.
> > 
> > This phrase is strange. Specially "the rest do" part.
> > It seems that we need to disable DC states on other ports than A,
> > what it is not true.
> 
> Okay, let's back up a little bit.
> 
> AUX domain for port A enables DC_OFF and AUX-IO. AUX domains for non-A
> ports enable DC_OFF, power well 2 and AUX-IO, in fact all domains that
> require power well 2 enable DC_OFF too. I can't see this is in bspec but
> that's what the code does currently. So, this is for AUX transfers.

To enable DC5/6 power well 2 needs to be disabled; I see this is still
missing from the BSpec DC5/6 enable sequence, though I asked already for
an update there before. Will ask again.

Non-A AUX channels are contained in power well 2, so we have to disable
DC states and enable power well 2 for non-A AUX channels.

> 
> which means, this comment for the previous iteration of the patch
> "I mean, on CNL
> 
> POWER_DOMAIN_AUX_{B,C,D,F} are exactly the same as
> POWER_DOMAIN_AUX__IO_{B,C,D,F}"
> 
> was not accurate. _AUX_IO_{B,C,D,F} domains enable only AUX_IO well and
> _AUX_{B,C,D,F} domains enable power well 2, DC_OFF and AUX_IO.
> 
> 
> For PSR, we want only the AUX_IO to be enabled for any port because the
> spec says -
> "PSR spontaneously sends Aux transactions.  If PSR is enabled on a port,
> then the associated Aux IO must be kept powered up."
> 
> which means, this comment
> "Hm, so in general (for AUX transfers) to enable AUX-A we first need to
> disable DC states _except_ if we enable AUX-A for PSR where we want
> DC5/6 to stay enabled." applies to all ports.

It doesn't make sense and we can't enable DC5/6 for non-A AUX channels,
since those are contained in power well 2, which won't be powered on/off
dynamically by DMC.

--Imre

> 
> To just enable the AUX-IO well, the correct version is ->
> https://patchwork.freedesktop.org/patch/205328/
> 
> 
> 
> -DK 
> > 
> > > +	 * Although PSR is enabled only on Port A currently, let's do this
> > > +	 * correctly for other ports too.
> > > +	 */
> > > +	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
> > > +					      intel_dp->aux_power_domain;
> > > +}
> > > +
> > > +static void psr_power_get(struct intel_dp *intel_dp)
> > 
> > nip: psr_aux_io_power_get ?!
> > 
> > > +{
> > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > > +
> > > +	if (INTEL_GEN(dev_priv) < 10)
> > > +		return;
> > > +
> > > +	intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
> > > +}
> > > +
> > > +static void psr_power_put(struct intel_dp *intel_dp)
> > > +{
> > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > > +
> > > +	if (INTEL_GEN(dev_priv) < 10)
> > > +		return;
> > > +
> > > +	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
> > > +}
> > > +
> > >  static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > @@ -459,6 +493,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> > >  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > >  	u32 chicken;
> > >  
> > > +	psr_power_get(intel_dp);
> > > +
> > >  	if (dev_priv->psr.psr2_support) {
> > >  		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> > >  		if (dev_priv->psr.y_cord_support)
> > > @@ -617,6 +653,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
> > >  		else
> > >  			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > >  	}
> > > +
> > > +	psr_power_put(intel_dp);
> > >  }
> > >  
> > >  /**
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > index b7924feb9f27..53ea564f971e 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > @@ -130,6 +130,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> > >  		return "AUX_D";
> > >  	case POWER_DOMAIN_AUX_F:
> > >  		return "AUX_F";
> > > +	case POWER_DOMAIN_AUX_IO_A:
> > > +		return "AUX_IO_A";
> > >  	case POWER_DOMAIN_GMBUS:
> > >  		return "GMBUS";
> > >  	case POWER_DOMAIN_INIT:
> > > @@ -1853,6 +1855,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > >  #define CNL_DISPLAY_AUX_A_POWER_DOMAINS (		\
> > >  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> > > +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
> > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > >  #define CNL_DISPLAY_AUX_B_POWER_DOMAINS (		\
> > >  	BIT_ULL(POWER_DOMAIN_AUX_B) |			\
> > > -- 
> > > 2.14.1
> > > 
> > _______________________________________________
> > 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 v3] drm/i915/psr: New power domain for AUX IO.

[Imre Deak]

On Thu, Feb 22, 2018 at 11:53:30PM +0200, Imre Deak wrote:
> On Thu, Feb 22, 2018 at 11:33:10PM +0200, Pandiyan, Dhinakaran wrote:
> > 
> > 
> > 
> > On Thu, 2018-02-22 at 12:44 -0800, Rodrigo Vivi wrote:
> > > On Thu, Feb 22, 2018 at 12:28:50PM -0800, Dhinakaran Pandiyan wrote:
> > > > PSR on CNL requires AUX IO wells to be kept on and the existing AUX domain
> > > > for AUX-A enables DC_OFF well too. This is not required, so add a new
> > > > AUX_IO_A domain for AUX-A to allow DC states to remain enabled. Other AUX
> > > > channels re-use the existing AUX domains as they do need power well 2.
> > > > 
> > > > v3: Extract aux domain selection into a function (Ville)
> > > > v2: Add AUX IO domain only for AUX-A
> > > >     Rebased on top of Ville's AUX series.
> > > > 
> > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Suggested-by: Imre Deak <imre.deak@intel.com>
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.h    |  1 +
> > > >  drivers/gpu/drm/i915/intel_dp.c         |  2 +-
> > > >  drivers/gpu/drm/i915/intel_drv.h        |  1 +
> > > >  drivers/gpu/drm/i915/intel_psr.c        | 38 +++++++++++++++++++++++++++++++++
> > > >  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
> > > >  5 files changed, 44 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > > index f5733a2576e7..4e7418b345bc 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > > @@ -186,6 +186,7 @@ enum intel_display_power_domain {
> > > >  	POWER_DOMAIN_AUX_C,
> > > >  	POWER_DOMAIN_AUX_D,
> > > >  	POWER_DOMAIN_AUX_F,
> > > > +	POWER_DOMAIN_AUX_IO_A,
> > > >  	POWER_DOMAIN_GMBUS,
> > > >  	POWER_DOMAIN_MODESET,
> > > >  	POWER_DOMAIN_GT_IRQ,
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 005735a7fc29..b78b9972ebae 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -1331,7 +1331,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> > > >  	return ret;
> > > >  }
> > > >  
> > > > -static enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > > > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > > >  {
> > > >  	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > > >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 5132f6a58c6d..bcd6dc9fb13d 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1685,6 +1685,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
> > > >  int intel_dp_link_required(int pixel_clock, int bpp);
> > > >  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
> > > >  bool intel_digital_port_connected(struct intel_encoder *encoder);
> > > > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp);
> > > >  
> > > >  /* intel_dp_aux_backlight.c */
> > > >  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 2ef374f936b9..03814f7bc2d3 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -56,6 +56,40 @@
> > > >  #include "intel_drv.h"
> > > >  #include "i915_drv.h"
> > > >  
> > > > +static inline enum intel_display_power_domain
> > > > +psr_aux_domain(struct intel_dp *intel_dp)
> > > > +{
> > > > +	/* CNL HW requires corresponding AUX IOs to be powered up for PSR, AUX-A
> > > > +	 * does not require the driver to disable DC states, but the rest do.
> > > 
> > > This phrase is strange. Specially "the rest do" part.
> > > It seems that we need to disable DC states on other ports than A,
> > > what it is not true.
> > 
> > Okay, let's back up a little bit.
> > 
> > AUX domain for port A enables DC_OFF and AUX-IO. AUX domains for non-A
> > ports enable DC_OFF, power well 2 and AUX-IO, in fact all domains that
> > require power well 2 enable DC_OFF too. I can't see this is in bspec but
> > that's what the code does currently. So, this is for AUX transfers.
> 
> To enable DC5/6 power well 2 needs to be disabled; I see this is still
> missing from the BSpec DC5/6 enable sequence, though I asked already for
> an update there before. Will ask again.

Actually it's at "Mode Set[SKL+]/Sequences for Display C5 and C6":

"""
...
2. Configure display engine to have power wells above power well 1
disabled, following the appropriate mode set disable sequences for any
ports using power wells above power well 1. This can be done earlier if
desired.
...
4. Set DC_STATE_EN Dynamic DC State Enable = "Enable up to DC5" for DC5
or "Enable up to DC6" for DC6.

"""

> 
> Non-A AUX channels are contained in power well 2, so we have to disable
> DC states and enable power well 2 for non-A AUX channels.
> 
> > 
> > which means, this comment for the previous iteration of the patch
> > "I mean, on CNL
> > 
> > POWER_DOMAIN_AUX_{B,C,D,F} are exactly the same as
> > POWER_DOMAIN_AUX__IO_{B,C,D,F}"
> > 
> > was not accurate. _AUX_IO_{B,C,D,F} domains enable only AUX_IO well and
> > _AUX_{B,C,D,F} domains enable power well 2, DC_OFF and AUX_IO.
> > 
> > 
> > For PSR, we want only the AUX_IO to be enabled for any port because the
> > spec says -
> > "PSR spontaneously sends Aux transactions.  If PSR is enabled on a port,
> > then the associated Aux IO must be kept powered up."
> > 
> > which means, this comment
> > "Hm, so in general (for AUX transfers) to enable AUX-A we first need to
> > disable DC states _except_ if we enable AUX-A for PSR where we want
> > DC5/6 to stay enabled." applies to all ports.
> 
> It doesn't make sense and we can't enable DC5/6 for non-A AUX channels,
> since those are contained in power well 2, which won't be powered on/off
> dynamically by DMC.
> 
> --Imre
> 
> > 
> > To just enable the AUX-IO well, the correct version is ->
> > https://patchwork.freedesktop.org/patch/205328/
> > 
> > 
> > 
> > -DK 
> > > 
> > > > +	 * Although PSR is enabled only on Port A currently, let's do this
> > > > +	 * correctly for other ports too.
> > > > +	 */
> > > > +	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
> > > > +					      intel_dp->aux_power_domain;
> > > > +}
> > > > +
> > > > +static void psr_power_get(struct intel_dp *intel_dp)
> > > 
> > > nip: psr_aux_io_power_get ?!
> > > 
> > > > +{
> > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > > > +
> > > > +	if (INTEL_GEN(dev_priv) < 10)
> > > > +		return;
> > > > +
> > > > +	intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
> > > > +}
> > > > +
> > > > +static void psr_power_put(struct intel_dp *intel_dp)
> > > > +{
> > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > > > +
> > > > +	if (INTEL_GEN(dev_priv) < 10)
> > > > +		return;
> > > > +
> > > > +	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
> > > > +}
> > > > +
> > > >  static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > > @@ -459,6 +493,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> > > >  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > > >  	u32 chicken;
> > > >  
> > > > +	psr_power_get(intel_dp);
> > > > +
> > > >  	if (dev_priv->psr.psr2_support) {
> > > >  		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> > > >  		if (dev_priv->psr.y_cord_support)
> > > > @@ -617,6 +653,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
> > > >  		else
> > > >  			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > > >  	}
> > > > +
> > > > +	psr_power_put(intel_dp);
> > > >  }
> > > >  
> > > >  /**
> > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > index b7924feb9f27..53ea564f971e 100644
> > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > @@ -130,6 +130,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> > > >  		return "AUX_D";
> > > >  	case POWER_DOMAIN_AUX_F:
> > > >  		return "AUX_F";
> > > > +	case POWER_DOMAIN_AUX_IO_A:
> > > > +		return "AUX_IO_A";
> > > >  	case POWER_DOMAIN_GMBUS:
> > > >  		return "GMBUS";
> > > >  	case POWER_DOMAIN_INIT:
> > > > @@ -1853,6 +1855,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> > > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > > >  #define CNL_DISPLAY_AUX_A_POWER_DOMAINS (		\
> > > >  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> > > > +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
> > > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > > >  #define CNL_DISPLAY_AUX_B_POWER_DOMAINS (		\
> > > >  	BIT_ULL(POWER_DOMAIN_AUX_B) |			\
> > > > -- 
> > > > 2.14.1
> > > > 
> > > _______________________________________________
> > > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/psr: New power domain for AUX IO.

[Pandiyan, Dhinakaran]

On Thu, 2018-02-22 at 23:47 +0200, Ville Syrjälä wrote:
> On Thu, Feb 22, 2018 at 09:33:10PM +0000, Pandiyan, Dhinakaran wrote:
> > 
> > 
> > 
> > On Thu, 2018-02-22 at 12:44 -0800, Rodrigo Vivi wrote:
> > > On Thu, Feb 22, 2018 at 12:28:50PM -0800, Dhinakaran Pandiyan wrote:
> > > > PSR on CNL requires AUX IO wells to be kept on and the existing AUX domain
> > > > for AUX-A enables DC_OFF well too. This is not required, so add a new
> > > > AUX_IO_A domain for AUX-A to allow DC states to remain enabled. Other AUX
> > > > channels re-use the existing AUX domains as they do need power well 2.
> > > > 
> > > > v3: Extract aux domain selection into a function (Ville)
> > > > v2: Add AUX IO domain only for AUX-A
> > > >     Rebased on top of Ville's AUX series.
> > > > 
> > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Suggested-by: Imre Deak <imre.deak@intel.com>
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.h    |  1 +
> > > >  drivers/gpu/drm/i915/intel_dp.c         |  2 +-
> > > >  drivers/gpu/drm/i915/intel_drv.h        |  1 +
> > > >  drivers/gpu/drm/i915/intel_psr.c        | 38 +++++++++++++++++++++++++++++++++
> > > >  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
> > > >  5 files changed, 44 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > > index f5733a2576e7..4e7418b345bc 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > > @@ -186,6 +186,7 @@ enum intel_display_power_domain {
> > > >  	POWER_DOMAIN_AUX_C,
> > > >  	POWER_DOMAIN_AUX_D,
> > > >  	POWER_DOMAIN_AUX_F,
> > > > +	POWER_DOMAIN_AUX_IO_A,
> > > >  	POWER_DOMAIN_GMBUS,
> > > >  	POWER_DOMAIN_MODESET,
> > > >  	POWER_DOMAIN_GT_IRQ,
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 005735a7fc29..b78b9972ebae 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -1331,7 +1331,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> > > >  	return ret;
> > > >  }
> > > >  
> > > > -static enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > > > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > > >  {
> > > >  	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > > >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 5132f6a58c6d..bcd6dc9fb13d 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1685,6 +1685,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
> > > >  int intel_dp_link_required(int pixel_clock, int bpp);
> > > >  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
> > > >  bool intel_digital_port_connected(struct intel_encoder *encoder);
> > > > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp);
> > > >  
> > > >  /* intel_dp_aux_backlight.c */
> > > >  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 2ef374f936b9..03814f7bc2d3 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -56,6 +56,40 @@
> > > >  #include "intel_drv.h"
> > > >  #include "i915_drv.h"
> > > >  
> > > > +static inline enum intel_display_power_domain
> > > > +psr_aux_domain(struct intel_dp *intel_dp)
> > > > +{
> > > > +	/* CNL HW requires corresponding AUX IOs to be powered up for PSR, AUX-A
> > > > +	 * does not require the driver to disable DC states, but the rest do.
> > > 
> > > This phrase is strange. Specially "the rest do" part.
> > > It seems that we need to disable DC states on other ports than A,
> > > what it is not true.
> > 
> > Okay, let's back up a little bit.
> > 
> > AUX domain for port A enables DC_OFF and AUX-IO. AUX domains for non-A
> > ports enable DC_OFF, power well 2 and AUX-IO, in fact all domains that
> > require power well 2 enable DC_OFF too. I can't see this is in bspec but
> > that's what the code does currently. So, this is for AUX transfers.
> > 
> > which means, this comment for the previous iteration of the patch
> > "I mean, on CNL
> > 
> > POWER_DOMAIN_AUX_{B,C,D,F} are exactly the same as
> > POWER_DOMAIN_AUX__IO_{B,C,D,F}"
> > 
> > was not accurate. _AUX_IO_{B,C,D,F} domains enable only AUX_IO well and
> > _AUX_{B,C,D,F} domains enable power well 2, DC_OFF and AUX_IO.
> > 
> > 
> > For PSR, we want only the AUX_IO to be enabled for any port because the
> > spec says -
> > "PSR spontaneously sends Aux transactions.  If PSR is enabled on a port,
> > then the associated Aux IO must be kept powered up."
> > 
> > which means, this comment
> > "Hm, so in general (for AUX transfers) to enable AUX-A we first need to
> > disable DC states _except_ if we enable AUX-A for PSR where we want
> > DC5/6 to stay enabled." applies to all ports.
> > 
> > To just enable the AUX-IO well, the correct version is ->
> > https://patchwork.freedesktop.org/patch/205328/
> 
> While that is true I don't think it actually matters in the end. As
> long as transcoder A-C is enabled DC will be off and PG2 will be
> enabled. So I suppose we can (ab)use that fact to save a few power
> domain bits and only define AUX_IO_A. The comment should probably
> be reworded to say as much.
> 

"CNL HW requires corresponding AUX IOs to be powered up for PSR.
However, for non-A AUX ports the corresponding non-EDP transcoders
would have already enabled power well 2 and DC_OFF. This means we can 
acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference instead of a
specific AUX_IO reference without powering up any extra wells."

Sound good?

> > 
> > 
> > 
> > -DK 
> > > 
> > > > +	 * Although PSR is enabled only on Port A currently, let's do this
> > > > +	 * correctly for other ports too.
> > > > +	 */
> > > > +	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
> > > > +					      intel_dp->aux_power_domain;
> > > > +}
> > > > +
> > > > +static void psr_power_get(struct intel_dp *intel_dp)
> > > 
> > > nip: psr_aux_io_power_get ?!
> > > 
> > > > +{
> > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > > > +
> > > > +	if (INTEL_GEN(dev_priv) < 10)
> > > > +		return;
> > > > +
> > > > +	intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
> > > > +}
> > > > +
> > > > +static void psr_power_put(struct intel_dp *intel_dp)
> > > > +{
> > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > > > +
> > > > +	if (INTEL_GEN(dev_priv) < 10)
> > > > +		return;
> > > > +
> > > > +	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
> > > > +}
> > > > +
> > > >  static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > > @@ -459,6 +493,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> > > >  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > > >  	u32 chicken;
> > > >  
> > > > +	psr_power_get(intel_dp);
> > > > +
> > > >  	if (dev_priv->psr.psr2_support) {
> > > >  		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> > > >  		if (dev_priv->psr.y_cord_support)
> > > > @@ -617,6 +653,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
> > > >  		else
> > > >  			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > > >  	}
> > > > +
> > > > +	psr_power_put(intel_dp);
> > > >  }
> > > >  
> > > >  /**
> > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > index b7924feb9f27..53ea564f971e 100644
> > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > @@ -130,6 +130,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> > > >  		return "AUX_D";
> > > >  	case POWER_DOMAIN_AUX_F:
> > > >  		return "AUX_F";
> > > > +	case POWER_DOMAIN_AUX_IO_A:
> > > > +		return "AUX_IO_A";
> > > >  	case POWER_DOMAIN_GMBUS:
> > > >  		return "GMBUS";
> > > >  	case POWER_DOMAIN_INIT:
> > > > @@ -1853,6 +1855,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> > > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > > >  #define CNL_DISPLAY_AUX_A_POWER_DOMAINS (		\
> > > >  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> > > > +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
> > > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > > >  #define CNL_DISPLAY_AUX_B_POWER_DOMAINS (		\
> > > >  	BIT_ULL(POWER_DOMAIN_AUX_B) |			\
> > > > -- 
> > > > 2.14.1
> > > > 
> > > _______________________________________________
> > > 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 v3] drm/i915/psr: New power domain for AUX IO.

[Pandiyan, Dhinakaran]

On Fri, 2018-02-23 at 00:05 +0200, Imre Deak wrote:
> On Thu, Feb 22, 2018 at 11:53:30PM +0200, Imre Deak wrote:
> > On Thu, Feb 22, 2018 at 11:33:10PM +0200, Pandiyan, Dhinakaran wrote:
> > > 
> > > 
> > > 
> > > On Thu, 2018-02-22 at 12:44 -0800, Rodrigo Vivi wrote:
> > > > On Thu, Feb 22, 2018 at 12:28:50PM -0800, Dhinakaran Pandiyan wrote:
> > > > > PSR on CNL requires AUX IO wells to be kept on and the existing AUX domain
> > > > > for AUX-A enables DC_OFF well too. This is not required, so add a new
> > > > > AUX_IO_A domain for AUX-A to allow DC states to remain enabled. Other AUX
> > > > > channels re-use the existing AUX domains as they do need power well 2.
> > > > > 
> > > > > v3: Extract aux domain selection into a function (Ville)
> > > > > v2: Add AUX IO domain only for AUX-A
> > > > >     Rebased on top of Ville's AUX series.
> > > > > 
> > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Suggested-by: Imre Deak <imre.deak@intel.com>
> > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_display.h    |  1 +
> > > > >  drivers/gpu/drm/i915/intel_dp.c         |  2 +-
> > > > >  drivers/gpu/drm/i915/intel_drv.h        |  1 +
> > > > >  drivers/gpu/drm/i915/intel_psr.c        | 38 +++++++++++++++++++++++++++++++++
> > > > >  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
> > > > >  5 files changed, 44 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > > > index f5733a2576e7..4e7418b345bc 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > > > @@ -186,6 +186,7 @@ enum intel_display_power_domain {
> > > > >  	POWER_DOMAIN_AUX_C,
> > > > >  	POWER_DOMAIN_AUX_D,
> > > > >  	POWER_DOMAIN_AUX_F,
> > > > > +	POWER_DOMAIN_AUX_IO_A,
> > > > >  	POWER_DOMAIN_GMBUS,
> > > > >  	POWER_DOMAIN_MODESET,
> > > > >  	POWER_DOMAIN_GT_IRQ,
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index 005735a7fc29..b78b9972ebae 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -1331,7 +1331,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> > > > >  	return ret;
> > > > >  }
> > > > >  
> > > > > -static enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > > > > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > > > >  {
> > > > >  	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > > > >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > > index 5132f6a58c6d..bcd6dc9fb13d 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -1685,6 +1685,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
> > > > >  int intel_dp_link_required(int pixel_clock, int bpp);
> > > > >  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
> > > > >  bool intel_digital_port_connected(struct intel_encoder *encoder);
> > > > > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp);
> > > > >  
> > > > >  /* intel_dp_aux_backlight.c */
> > > > >  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > > > index 2ef374f936b9..03814f7bc2d3 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > @@ -56,6 +56,40 @@
> > > > >  #include "intel_drv.h"
> > > > >  #include "i915_drv.h"
> > > > >  
> > > > > +static inline enum intel_display_power_domain
> > > > > +psr_aux_domain(struct intel_dp *intel_dp)
> > > > > +{
> > > > > +	/* CNL HW requires corresponding AUX IOs to be powered up for PSR, AUX-A
> > > > > +	 * does not require the driver to disable DC states, but the rest do.
> > > > 
> > > > This phrase is strange. Specially "the rest do" part.
> > > > It seems that we need to disable DC states on other ports than A,
> > > > what it is not true.
> > > 
> > > Okay, let's back up a little bit.
> > > 
> > > AUX domain for port A enables DC_OFF and AUX-IO. AUX domains for non-A
> > > ports enable DC_OFF, power well 2 and AUX-IO, in fact all domains that
> > > require power well 2 enable DC_OFF too. I can't see this is in bspec but
> > > that's what the code does currently. So, this is for AUX transfers.
> > 
> > To enable DC5/6 power well 2 needs to be disabled; I see this is still
> > missing from the BSpec DC5/6 enable sequence, though I asked already for
> > an update there before. Will ask again.
> 
> Actually it's at "Mode Set[SKL+]/Sequences for Display C5 and C6":
> 
> """
> ...
> 2. Configure display engine to have power wells above power well 1
> disabled, following the appropriate mode set disable sequences for any
> ports using power wells above power well 1. This can be done earlier if
> desired.
> ...
> 4. Set DC_STATE_EN Dynamic DC State Enable = "Enable up to DC5" for DC5
> or "Enable up to DC6" for DC6.
> 
> """
> 
Thanks, finding this information would have been much easier if it was
all in one place. We still have to assume the converse from the enable
step. I wonder if enabling power well 2 is sufficient to bring out the
hardware from DC6 without having to explicitly disable it.


> > 
> > Non-A AUX channels are contained in power well 2, so we have to disable
> > DC states and enable power well 2 for non-A AUX channels.
> > 
> > > 
> > > which means, this comment for the previous iteration of the patch
> > > "I mean, on CNL
> > > 
> > > POWER_DOMAIN_AUX_{B,C,D,F} are exactly the same as
> > > POWER_DOMAIN_AUX__IO_{B,C,D,F}"
> > > 
> > > was not accurate. _AUX_IO_{B,C,D,F} domains enable only AUX_IO well and
> > > _AUX_{B,C,D,F} domains enable power well 2, DC_OFF and AUX_IO.
> > > 
> > > 
> > > For PSR, we want only the AUX_IO to be enabled for any port because the
> > > spec says -
> > > "PSR spontaneously sends Aux transactions.  If PSR is enabled on a port,
> > > then the associated Aux IO must be kept powered up."
> > > 
> > > which means, this comment
> > > "Hm, so in general (for AUX transfers) to enable AUX-A we first need to
> > > disable DC states _except_ if we enable AUX-A for PSR where we want
> > > DC5/6 to stay enabled." applies to all ports.
> > 
> > It doesn't make sense and we can't enable DC5/6 for non-A AUX channels,
> > since those are contained in power well 2, which won't be powered on/off
> > dynamically by DMC.

Is the AUX-IO contained in power well 2? The AUX_CTL registers are. 

If the hardware is sending AUX transactions on AUX_A when PSR is
enabled, does it mean it is also powering up power well 1 or just the
AUX-IO?


> > 
> > --Imre
> > 
> > > 
> > > To just enable the AUX-IO well, the correct version is ->
> > > https://patchwork.freedesktop.org/patch/205328/
> > > 
> > > 
> > > 
> > > -DK 
> > > > 
> > > > > +	 * Although PSR is enabled only on Port A currently, let's do this
> > > > > +	 * correctly for other ports too.
> > > > > +	 */
> > > > > +	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
> > > > > +					      intel_dp->aux_power_domain;
> > > > > +}
> > > > > +
> > > > > +static void psr_power_get(struct intel_dp *intel_dp)
> > > > 
> > > > nip: psr_aux_io_power_get ?!
> > > > 
> > > > > +{
> > > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > > > > +
> > > > > +	if (INTEL_GEN(dev_priv) < 10)
> > > > > +		return;
> > > > > +
> > > > > +	intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
> > > > > +}
> > > > > +
> > > > > +static void psr_power_put(struct intel_dp *intel_dp)
> > > > > +{
> > > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > > > > +
> > > > > +	if (INTEL_GEN(dev_priv) < 10)
> > > > > +		return;
> > > > > +
> > > > > +	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
> > > > > +}
> > > > > +
> > > > >  static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
> > > > >  {
> > > > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > > > @@ -459,6 +493,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> > > > >  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > > > >  	u32 chicken;
> > > > >  
> > > > > +	psr_power_get(intel_dp);
> > > > > +
> > > > >  	if (dev_priv->psr.psr2_support) {
> > > > >  		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> > > > >  		if (dev_priv->psr.y_cord_support)
> > > > > @@ -617,6 +653,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
> > > > >  		else
> > > > >  			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > > > >  	}
> > > > > +
> > > > > +	psr_power_put(intel_dp);
> > > > >  }
> > > > >  
> > > > >  /**
> > > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > index b7924feb9f27..53ea564f971e 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > @@ -130,6 +130,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> > > > >  		return "AUX_D";
> > > > >  	case POWER_DOMAIN_AUX_F:
> > > > >  		return "AUX_F";
> > > > > +	case POWER_DOMAIN_AUX_IO_A:
> > > > > +		return "AUX_IO_A";
> > > > >  	case POWER_DOMAIN_GMBUS:
> > > > >  		return "GMBUS";
> > > > >  	case POWER_DOMAIN_INIT:
> > > > > @@ -1853,6 +1855,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> > > > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > > > >  #define CNL_DISPLAY_AUX_A_POWER_DOMAINS (		\
> > > > >  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> > > > > +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
> > > > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > > > >  #define CNL_DISPLAY_AUX_B_POWER_DOMAINS (		\
> > > > >  	BIT_ULL(POWER_DOMAIN_AUX_B) |			\
> > > > > -- 
> > > > > 2.14.1
> > > > > 
> > > > _______________________________________________
> > > > 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
> _______________________________________________
> 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 v3] drm/i915/psr: New power domain for AUX IO.

[Imre Deak]

On Fri, Feb 23, 2018 at 12:30:14AM +0200, Pandiyan, Dhinakaran wrote:
> On Fri, 2018-02-23 at 00:05 +0200, Imre Deak wrote:
> > On Thu, Feb 22, 2018 at 11:53:30PM +0200, Imre Deak wrote:
> > > On Thu, Feb 22, 2018 at 11:33:10PM +0200, Pandiyan, Dhinakaran wrote:
> > > > 
> > > > 
> > > > 
> > > > On Thu, 2018-02-22 at 12:44 -0800, Rodrigo Vivi wrote:
> > > > > On Thu, Feb 22, 2018 at 12:28:50PM -0800, Dhinakaran Pandiyan wrote:
> > > > > > PSR on CNL requires AUX IO wells to be kept on and the existing AUX domain
> > > > > > for AUX-A enables DC_OFF well too. This is not required, so add a new
> > > > > > AUX_IO_A domain for AUX-A to allow DC states to remain enabled. Other AUX
> > > > > > channels re-use the existing AUX domains as they do need power well 2.
> > > > > > 
> > > > > > v3: Extract aux domain selection into a function (Ville)
> > > > > > v2: Add AUX IO domain only for AUX-A
> > > > > >     Rebased on top of Ville's AUX series.
> > > > > > 
> > > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Suggested-by: Imre Deak <imre.deak@intel.com>
> > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_display.h    |  1 +
> > > > > >  drivers/gpu/drm/i915/intel_dp.c         |  2 +-
> > > > > >  drivers/gpu/drm/i915/intel_drv.h        |  1 +
> > > > > >  drivers/gpu/drm/i915/intel_psr.c        | 38 +++++++++++++++++++++++++++++++++
> > > > > >  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
> > > > > >  5 files changed, 44 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > > > > index f5733a2576e7..4e7418b345bc 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > > > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > > > > @@ -186,6 +186,7 @@ enum intel_display_power_domain {
> > > > > >  	POWER_DOMAIN_AUX_C,
> > > > > >  	POWER_DOMAIN_AUX_D,
> > > > > >  	POWER_DOMAIN_AUX_F,
> > > > > > +	POWER_DOMAIN_AUX_IO_A,
> > > > > >  	POWER_DOMAIN_GMBUS,
> > > > > >  	POWER_DOMAIN_MODESET,
> > > > > >  	POWER_DOMAIN_GT_IRQ,
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > index 005735a7fc29..b78b9972ebae 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > @@ -1331,7 +1331,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> > > > > >  	return ret;
> > > > > >  }
> > > > > >  
> > > > > > -static enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > > > > > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > > > > >  {
> > > > > >  	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > > > > >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > index 5132f6a58c6d..bcd6dc9fb13d 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > @@ -1685,6 +1685,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
> > > > > >  int intel_dp_link_required(int pixel_clock, int bpp);
> > > > > >  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
> > > > > >  bool intel_digital_port_connected(struct intel_encoder *encoder);
> > > > > > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp);
> > > > > >  
> > > > > >  /* intel_dp_aux_backlight.c */
> > > > > >  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > index 2ef374f936b9..03814f7bc2d3 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > @@ -56,6 +56,40 @@
> > > > > >  #include "intel_drv.h"
> > > > > >  #include "i915_drv.h"
> > > > > >  
> > > > > > +static inline enum intel_display_power_domain
> > > > > > +psr_aux_domain(struct intel_dp *intel_dp)
> > > > > > +{
> > > > > > +	/* CNL HW requires corresponding AUX IOs to be powered up for PSR, AUX-A
> > > > > > +	 * does not require the driver to disable DC states, but the rest do.
> > > > > 
> > > > > This phrase is strange. Specially "the rest do" part.
> > > > > It seems that we need to disable DC states on other ports than A,
> > > > > what it is not true.
> > > > 
> > > > Okay, let's back up a little bit.
> > > > 
> > > > AUX domain for port A enables DC_OFF and AUX-IO. AUX domains for non-A
> > > > ports enable DC_OFF, power well 2 and AUX-IO, in fact all domains that
> > > > require power well 2 enable DC_OFF too. I can't see this is in bspec but
> > > > that's what the code does currently. So, this is for AUX transfers.
> > > 
> > > To enable DC5/6 power well 2 needs to be disabled; I see this is still
> > > missing from the BSpec DC5/6 enable sequence, though I asked already for
> > > an update there before. Will ask again.
> > 
> > Actually it's at "Mode Set[SKL+]/Sequences for Display C5 and C6":
> > 
> > """
> > ...
> > 2. Configure display engine to have power wells above power well 1
> > disabled, following the appropriate mode set disable sequences for any
> > ports using power wells above power well 1. This can be done earlier if
> > desired.
> > ...
> > 4. Set DC_STATE_EN Dynamic DC State Enable = "Enable up to DC5" for DC5
> > or "Enable up to DC6" for DC6.
> > 
> > """
> > 
> Thanks, finding this information would have been much easier if it was
> all in one place. We still have to assume the converse from the enable
> step. I wonder if enabling power well 2 is sufficient to bring out the
> hardware from DC6 without having to explicitly disable it.

Yep, that's not that clear, but I guess in any caes we can do that always
manually. Should file an update request for this.

> > > Non-A AUX channels are contained in power well 2, so we have to disable
> > > DC states and enable power well 2 for non-A AUX channels.
> > > 
> > > > 
> > > > which means, this comment for the previous iteration of the patch
> > > > "I mean, on CNL
> > > > 
> > > > POWER_DOMAIN_AUX_{B,C,D,F} are exactly the same as
> > > > POWER_DOMAIN_AUX__IO_{B,C,D,F}"
> > > > 
> > > > was not accurate. _AUX_IO_{B,C,D,F} domains enable only AUX_IO well and
> > > > _AUX_{B,C,D,F} domains enable power well 2, DC_OFF and AUX_IO.
> > > > 
> > > > 
> > > > For PSR, we want only the AUX_IO to be enabled for any port because the
> > > > spec says -
> > > > "PSR spontaneously sends Aux transactions.  If PSR is enabled on a port,
> > > > then the associated Aux IO must be kept powered up."
> > > > 
> > > > which means, this comment
> > > > "Hm, so in general (for AUX transfers) to enable AUX-A we first need to
> > > > disable DC states _except_ if we enable AUX-A for PSR where we want
> > > > DC5/6 to stay enabled." applies to all ports.
> > > 
> > > It doesn't make sense and we can't enable DC5/6 for non-A AUX channels,
> > > since those are contained in power well 2, which won't be powered on/off
> > > dynamically by DMC.
> 
> Is the AUX-IO contained in power well 2? The AUX_CTL registers are.

Not sure if the two things are divided but I would assume the HW/FW
needs the same registers to perform an AUX transfer as what the driver
programs. (Hence the need for the HW mutex.)

> If the hardware is sending AUX transactions on AUX_A when PSR is
> enabled, does it mean it is also powering up power well 1 or just the
> AUX-IO?

Again, I'd say it needs the same power as what's needed in case the
driver does AUX transfers. So yes, power well 1 would be powered on.

--Imre

> 
> 
> > > 
> > > --Imre
> > > 
> > > > 
> > > > To just enable the AUX-IO well, the correct version is ->
> > > > https://patchwork.freedesktop.org/patch/205328/
> > > > 
> > > > 
> > > > 
> > > > -DK 
> > > > > 
> > > > > > +	 * Although PSR is enabled only on Port A currently, let's do this
> > > > > > +	 * correctly for other ports too.
> > > > > > +	 */
> > > > > > +	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
> > > > > > +					      intel_dp->aux_power_domain;
> > > > > > +}
> > > > > > +
> > > > > > +static void psr_power_get(struct intel_dp *intel_dp)
> > > > > 
> > > > > nip: psr_aux_io_power_get ?!
> > > > > 
> > > > > > +{
> > > > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > > > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > > > > > +
> > > > > > +	if (INTEL_GEN(dev_priv) < 10)
> > > > > > +		return;
> > > > > > +
> > > > > > +	intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
> > > > > > +}
> > > > > > +
> > > > > > +static void psr_power_put(struct intel_dp *intel_dp)
> > > > > > +{
> > > > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > > > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > > > > > +
> > > > > > +	if (INTEL_GEN(dev_priv) < 10)
> > > > > > +		return;
> > > > > > +
> > > > > > +	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
> > > > > > +}
> > > > > > +
> > > > > >  static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
> > > > > >  {
> > > > > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > > > > @@ -459,6 +493,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> > > > > >  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > > > > >  	u32 chicken;
> > > > > >  
> > > > > > +	psr_power_get(intel_dp);
> > > > > > +
> > > > > >  	if (dev_priv->psr.psr2_support) {
> > > > > >  		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> > > > > >  		if (dev_priv->psr.y_cord_support)
> > > > > > @@ -617,6 +653,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
> > > > > >  		else
> > > > > >  			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > > > > >  	}
> > > > > > +
> > > > > > +	psr_power_put(intel_dp);
> > > > > >  }
> > > > > >  
> > > > > >  /**
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > > index b7924feb9f27..53ea564f971e 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > > @@ -130,6 +130,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> > > > > >  		return "AUX_D";
> > > > > >  	case POWER_DOMAIN_AUX_F:
> > > > > >  		return "AUX_F";
> > > > > > +	case POWER_DOMAIN_AUX_IO_A:
> > > > > > +		return "AUX_IO_A";
> > > > > >  	case POWER_DOMAIN_GMBUS:
> > > > > >  		return "GMBUS";
> > > > > >  	case POWER_DOMAIN_INIT:
> > > > > > @@ -1853,6 +1855,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> > > > > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > > > > >  #define CNL_DISPLAY_AUX_A_POWER_DOMAINS (		\
> > > > > >  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> > > > > > +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
> > > > > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > > > > >  #define CNL_DISPLAY_AUX_B_POWER_DOMAINS (		\
> > > > > >  	BIT_ULL(POWER_DOMAIN_AUX_B) |			\
> > > > > > -- 
> > > > > > 2.14.1
> > > > > > 
> > > > > _______________________________________________
> > > > > 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
> > _______________________________________________
> > 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 v3] drm/i915/psr: New power domain for AUX IO.

[Rodrigo Vivi]

On Thu, Feb 22, 2018 at 10:10:24PM +0000, Pandiyan, Dhinakaran wrote:
> 
> 
> On Thu, 2018-02-22 at 23:47 +0200, Ville Syrjälä wrote:
> > On Thu, Feb 22, 2018 at 09:33:10PM +0000, Pandiyan, Dhinakaran wrote:
> > > 
> > > 
> > > 
> > > On Thu, 2018-02-22 at 12:44 -0800, Rodrigo Vivi wrote:
> > > > On Thu, Feb 22, 2018 at 12:28:50PM -0800, Dhinakaran Pandiyan wrote:
> > > > > PSR on CNL requires AUX IO wells to be kept on and the existing AUX domain
> > > > > for AUX-A enables DC_OFF well too. This is not required, so add a new
> > > > > AUX_IO_A domain for AUX-A to allow DC states to remain enabled. Other AUX
> > > > > channels re-use the existing AUX domains as they do need power well 2.
> > > > > 
> > > > > v3: Extract aux domain selection into a function (Ville)
> > > > > v2: Add AUX IO domain only for AUX-A
> > > > >     Rebased on top of Ville's AUX series.
> > > > > 
> > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Suggested-by: Imre Deak <imre.deak@intel.com>
> > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_display.h    |  1 +
> > > > >  drivers/gpu/drm/i915/intel_dp.c         |  2 +-
> > > > >  drivers/gpu/drm/i915/intel_drv.h        |  1 +
> > > > >  drivers/gpu/drm/i915/intel_psr.c        | 38 +++++++++++++++++++++++++++++++++
> > > > >  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
> > > > >  5 files changed, 44 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > > > index f5733a2576e7..4e7418b345bc 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > > > @@ -186,6 +186,7 @@ enum intel_display_power_domain {
> > > > >  	POWER_DOMAIN_AUX_C,
> > > > >  	POWER_DOMAIN_AUX_D,
> > > > >  	POWER_DOMAIN_AUX_F,
> > > > > +	POWER_DOMAIN_AUX_IO_A,
> > > > >  	POWER_DOMAIN_GMBUS,
> > > > >  	POWER_DOMAIN_MODESET,
> > > > >  	POWER_DOMAIN_GT_IRQ,
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index 005735a7fc29..b78b9972ebae 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -1331,7 +1331,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> > > > >  	return ret;
> > > > >  }
> > > > >  
> > > > > -static enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > > > > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > > > >  {
> > > > >  	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > > > >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > > index 5132f6a58c6d..bcd6dc9fb13d 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -1685,6 +1685,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
> > > > >  int intel_dp_link_required(int pixel_clock, int bpp);
> > > > >  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
> > > > >  bool intel_digital_port_connected(struct intel_encoder *encoder);
> > > > > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp);
> > > > >  
> > > > >  /* intel_dp_aux_backlight.c */
> > > > >  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > > > index 2ef374f936b9..03814f7bc2d3 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > @@ -56,6 +56,40 @@
> > > > >  #include "intel_drv.h"
> > > > >  #include "i915_drv.h"
> > > > >  
> > > > > +static inline enum intel_display_power_domain
> > > > > +psr_aux_domain(struct intel_dp *intel_dp)
> > > > > +{
> > > > > +	/* CNL HW requires corresponding AUX IOs to be powered up for PSR, AUX-A
> > > > > +	 * does not require the driver to disable DC states, but the rest do.
> > > > 
> > > > This phrase is strange. Specially "the rest do" part.
> > > > It seems that we need to disable DC states on other ports than A,
> > > > what it is not true.
> > > 
> > > Okay, let's back up a little bit.
> > > 
> > > AUX domain for port A enables DC_OFF and AUX-IO. AUX domains for non-A
> > > ports enable DC_OFF, power well 2 and AUX-IO, in fact all domains that
> > > require power well 2 enable DC_OFF too. I can't see this is in bspec but
> > > that's what the code does currently. So, this is for AUX transfers.
> > > 
> > > which means, this comment for the previous iteration of the patch
> > > "I mean, on CNL
> > > 
> > > POWER_DOMAIN_AUX_{B,C,D,F} are exactly the same as
> > > POWER_DOMAIN_AUX__IO_{B,C,D,F}"
> > > 
> > > was not accurate. _AUX_IO_{B,C,D,F} domains enable only AUX_IO well and
> > > _AUX_{B,C,D,F} domains enable power well 2, DC_OFF and AUX_IO.
> > > 
> > > 
> > > For PSR, we want only the AUX_IO to be enabled for any port because the
> > > spec says -
> > > "PSR spontaneously sends Aux transactions.  If PSR is enabled on a port,
> > > then the associated Aux IO must be kept powered up."
> > > 
> > > which means, this comment
> > > "Hm, so in general (for AUX transfers) to enable AUX-A we first need to
> > > disable DC states _except_ if we enable AUX-A for PSR where we want
> > > DC5/6 to stay enabled." applies to all ports.
> > > 
> > > To just enable the AUX-IO well, the correct version is ->
> > > https://patchwork.freedesktop.org/patch/205328/
> > 
> > While that is true I don't think it actually matters in the end. As
> > long as transcoder A-C is enabled DC will be off and PG2 will be
> > enabled. So I suppose we can (ab)use that fact to save a few power
> > domain bits and only define AUX_IO_A. The comment should probably
> > be reworded to say as much.
> > 
> 
> "CNL HW requires corresponding AUX IOs to be powered up for PSR.
> However, for non-A AUX ports the corresponding non-EDP transcoders
> would have already enabled power well 2 and DC_OFF. This means we can 
> acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference instead of a
> specific AUX_IO reference without powering up any extra wells."
> 
> Sound good?

Oh ok... thanks for explaining it to me in person as well...
I was really missing that AUX B to F were already part for PG2 hence
part of DC_OFF...

Maybe the first version was less confusing actually... but we can
go with this optmization.

If you want to go with this version:

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

and ville can merge when merging the aux_ch one.

> 
> > > 
> > > 
> > > 
> > > -DK 
> > > > 
> > > > > +	 * Although PSR is enabled only on Port A currently, let's do this
> > > > > +	 * correctly for other ports too.
> > > > > +	 */
> > > > > +	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
> > > > > +					      intel_dp->aux_power_domain;
> > > > > +}
> > > > > +
> > > > > +static void psr_power_get(struct intel_dp *intel_dp)
> > > > 
> > > > nip: psr_aux_io_power_get ?!
> > > > 
> > > > > +{
> > > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > > > > +
> > > > > +	if (INTEL_GEN(dev_priv) < 10)
> > > > > +		return;
> > > > > +
> > > > > +	intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
> > > > > +}
> > > > > +
> > > > > +static void psr_power_put(struct intel_dp *intel_dp)
> > > > > +{
> > > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > > > > +
> > > > > +	if (INTEL_GEN(dev_priv) < 10)
> > > > > +		return;
> > > > > +
> > > > > +	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
> > > > > +}
> > > > > +
> > > > >  static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
> > > > >  {
> > > > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > > > @@ -459,6 +493,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> > > > >  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > > > >  	u32 chicken;
> > > > >  
> > > > > +	psr_power_get(intel_dp);
> > > > > +
> > > > >  	if (dev_priv->psr.psr2_support) {
> > > > >  		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> > > > >  		if (dev_priv->psr.y_cord_support)
> > > > > @@ -617,6 +653,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
> > > > >  		else
> > > > >  			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > > > >  	}
> > > > > +
> > > > > +	psr_power_put(intel_dp);
> > > > >  }
> > > > >  
> > > > >  /**
> > > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > index b7924feb9f27..53ea564f971e 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > @@ -130,6 +130,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> > > > >  		return "AUX_D";
> > > > >  	case POWER_DOMAIN_AUX_F:
> > > > >  		return "AUX_F";
> > > > > +	case POWER_DOMAIN_AUX_IO_A:
> > > > > +		return "AUX_IO_A";
> > > > >  	case POWER_DOMAIN_GMBUS:
> > > > >  		return "GMBUS";
> > > > >  	case POWER_DOMAIN_INIT:
> > > > > @@ -1853,6 +1855,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> > > > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > > > >  #define CNL_DISPLAY_AUX_A_POWER_DOMAINS (		\
> > > > >  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> > > > > +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
> > > > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > > > >  #define CNL_DISPLAY_AUX_B_POWER_DOMAINS (		\
> > > > >  	BIT_ULL(POWER_DOMAIN_AUX_B) |			\
> > > > > -- 
> > > > > 2.14.1
> > > > > 
> > > > _______________________________________________
> > > > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/psr: New power domain for AUX IO.

[Pandiyan, Dhinakaran]

On Fri, 2018-02-23 at 00:42 +0200, Imre Deak wrote:
> On Fri, Feb 23, 2018 at 12:30:14AM +0200, Pandiyan, Dhinakaran wrote:
> > On Fri, 2018-02-23 at 00:05 +0200, Imre Deak wrote:
> > > On Thu, Feb 22, 2018 at 11:53:30PM +0200, Imre Deak wrote:
> > > > On Thu, Feb 22, 2018 at 11:33:10PM +0200, Pandiyan, Dhinakaran wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > On Thu, 2018-02-22 at 12:44 -0800, Rodrigo Vivi wrote:
> > > > > > On Thu, Feb 22, 2018 at 12:28:50PM -0800, Dhinakaran Pandiyan wrote:
> > > > > > > PSR on CNL requires AUX IO wells to be kept on and the existing AUX domain
> > > > > > > for AUX-A enables DC_OFF well too. This is not required, so add a new
> > > > > > > AUX_IO_A domain for AUX-A to allow DC states to remain enabled. Other AUX
> > > > > > > channels re-use the existing AUX domains as they do need power well 2.
> > > > > > > 
> > > > > > > v3: Extract aux domain selection into a function (Ville)
> > > > > > > v2: Add AUX IO domain only for AUX-A
> > > > > > >     Rebased on top of Ville's AUX series.
> > > > > > > 
> > > > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > Suggested-by: Imre Deak <imre.deak@intel.com>
> > > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/intel_display.h    |  1 +
> > > > > > >  drivers/gpu/drm/i915/intel_dp.c         |  2 +-
> > > > > > >  drivers/gpu/drm/i915/intel_drv.h        |  1 +
> > > > > > >  drivers/gpu/drm/i915/intel_psr.c        | 38 +++++++++++++++++++++++++++++++++
> > > > > > >  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
> > > > > > >  5 files changed, 44 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > > > > > index f5733a2576e7..4e7418b345bc 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > > > > > @@ -186,6 +186,7 @@ enum intel_display_power_domain {
> > > > > > >  	POWER_DOMAIN_AUX_C,
> > > > > > >  	POWER_DOMAIN_AUX_D,
> > > > > > >  	POWER_DOMAIN_AUX_F,
> > > > > > > +	POWER_DOMAIN_AUX_IO_A,
> > > > > > >  	POWER_DOMAIN_GMBUS,
> > > > > > >  	POWER_DOMAIN_MODESET,
> > > > > > >  	POWER_DOMAIN_GT_IRQ,
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > index 005735a7fc29..b78b9972ebae 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > @@ -1331,7 +1331,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> > > > > > >  	return ret;
> > > > > > >  }
> > > > > > >  
> > > > > > > -static enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > > > > > > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > > > > > >  {
> > > > > > >  	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > > > > > >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > index 5132f6a58c6d..bcd6dc9fb13d 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > @@ -1685,6 +1685,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
> > > > > > >  int intel_dp_link_required(int pixel_clock, int bpp);
> > > > > > >  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
> > > > > > >  bool intel_digital_port_connected(struct intel_encoder *encoder);
> > > > > > > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp);
> > > > > > >  
> > > > > > >  /* intel_dp_aux_backlight.c */
> > > > > > >  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > > index 2ef374f936b9..03814f7bc2d3 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > > @@ -56,6 +56,40 @@
> > > > > > >  #include "intel_drv.h"
> > > > > > >  #include "i915_drv.h"
> > > > > > >  
> > > > > > > +static inline enum intel_display_power_domain
> > > > > > > +psr_aux_domain(struct intel_dp *intel_dp)
> > > > > > > +{
> > > > > > > +	/* CNL HW requires corresponding AUX IOs to be powered up for PSR, AUX-A
> > > > > > > +	 * does not require the driver to disable DC states, but the rest do.
> > > > > > 
> > > > > > This phrase is strange. Specially "the rest do" part.
> > > > > > It seems that we need to disable DC states on other ports than A,
> > > > > > what it is not true.
> > > > > 
> > > > > Okay, let's back up a little bit.
> > > > > 
> > > > > AUX domain for port A enables DC_OFF and AUX-IO. AUX domains for non-A
> > > > > ports enable DC_OFF, power well 2 and AUX-IO, in fact all domains that
> > > > > require power well 2 enable DC_OFF too. I can't see this is in bspec but
> > > > > that's what the code does currently. So, this is for AUX transfers.
> > > > 
> > > > To enable DC5/6 power well 2 needs to be disabled; I see this is still
> > > > missing from the BSpec DC5/6 enable sequence, though I asked already for
> > > > an update there before. Will ask again.
> > > 
> > > Actually it's at "Mode Set[SKL+]/Sequences for Display C5 and C6":
> > > 
> > > """
> > > ...
> > > 2. Configure display engine to have power wells above power well 1
> > > disabled, following the appropriate mode set disable sequences for any
> > > ports using power wells above power well 1. This can be done earlier if
> > > desired.
> > > ...
> > > 4. Set DC_STATE_EN Dynamic DC State Enable = "Enable up to DC5" for DC5
> > > or "Enable up to DC6" for DC6.
> > > 
> > > """
> > > 
> > Thanks, finding this information would have been much easier if it was
> > all in one place. We still have to assume the converse from the enable
> > step. I wonder if enabling power well 2 is sufficient to bring out the
> > hardware from DC6 without having to explicitly disable it.
> 
> Yep, that's not that clear, but I guess in any caes we can do that always
> manually. Should file an update request for this.
> 
> > > > Non-A AUX channels are contained in power well 2, so we have to disable
> > > > DC states and enable power well 2 for non-A AUX channels.
> > > > 
> > > > > 
> > > > > which means, this comment for the previous iteration of the patch
> > > > > "I mean, on CNL
> > > > > 
> > > > > POWER_DOMAIN_AUX_{B,C,D,F} are exactly the same as
> > > > > POWER_DOMAIN_AUX__IO_{B,C,D,F}"
> > > > > 
> > > > > was not accurate. _AUX_IO_{B,C,D,F} domains enable only AUX_IO well and
> > > > > _AUX_{B,C,D,F} domains enable power well 2, DC_OFF and AUX_IO.
> > > > > 
> > > > > 
> > > > > For PSR, we want only the AUX_IO to be enabled for any port because the
> > > > > spec says -
> > > > > "PSR spontaneously sends Aux transactions.  If PSR is enabled on a port,
> > > > > then the associated Aux IO must be kept powered up."
> > > > > 
> > > > > which means, this comment
> > > > > "Hm, so in general (for AUX transfers) to enable AUX-A we first need to
> > > > > disable DC states _except_ if we enable AUX-A for PSR where we want
> > > > > DC5/6 to stay enabled." applies to all ports.
> > > > 
> > > > It doesn't make sense and we can't enable DC5/6 for non-A AUX channels,
> > > > since those are contained in power well 2, which won't be powered on/off
> > > > dynamically by DMC.
> > 
> > Is the AUX-IO contained in power well 2? The AUX_CTL registers are.
> 
> Not sure if the two things are divided but I would assume the HW/FW
> needs the same registers to perform an AUX transfer as what the driver
> programs. (Hence the need for the HW mutex.)
> 
That's a fair guess but kinda defeats the point of having a separate
power well for AUX IO, doesn't it? We should perhaps check this with
Art.


> > If the hardware is sending AUX transactions on AUX_A when PSR is
> > enabled, does it mean it is also powering up power well 1 or just the
> > AUX-IO?
> 
> Again, I'd say it needs the same power as what's needed in case the
> driver does AUX transfers. So yes, power well 1 would be powered on.
> 
> --Imre
> 
> > 
> > 
> > > > 
> > > > --Imre
> > > > 
> > > > > 
> > > > > To just enable the AUX-IO well, the correct version is ->
> > > > > https://patchwork.freedesktop.org/patch/205328/
> > > > > 
> > > > > 
> > > > > 
> > > > > -DK 
> > > > > > 
> > > > > > > +	 * Although PSR is enabled only on Port A currently, let's do this
> > > > > > > +	 * correctly for other ports too.
> > > > > > > +	 */
> > > > > > > +	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
> > > > > > > +					      intel_dp->aux_power_domain;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void psr_power_get(struct intel_dp *intel_dp)
> > > > > > 
> > > > > > nip: psr_aux_io_power_get ?!
> > > > > > 
> > > > > > > +{
> > > > > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > > > > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > > > > > > +
> > > > > > > +	if (INTEL_GEN(dev_priv) < 10)
> > > > > > > +		return;
> > > > > > > +
> > > > > > > +	intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void psr_power_put(struct intel_dp *intel_dp)
> > > > > > > +{
> > > > > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > > > > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > > > > > > +
> > > > > > > +	if (INTEL_GEN(dev_priv) < 10)
> > > > > > > +		return;
> > > > > > > +
> > > > > > > +	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
> > > > > > > +}
> > > > > > > +
> > > > > > >  static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
> > > > > > >  {
> > > > > > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > > > > > @@ -459,6 +493,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> > > > > > >  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > > > > > >  	u32 chicken;
> > > > > > >  
> > > > > > > +	psr_power_get(intel_dp);
> > > > > > > +
> > > > > > >  	if (dev_priv->psr.psr2_support) {
> > > > > > >  		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> > > > > > >  		if (dev_priv->psr.y_cord_support)
> > > > > > > @@ -617,6 +653,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
> > > > > > >  		else
> > > > > > >  			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > > > > > >  	}
> > > > > > > +
> > > > > > > +	psr_power_put(intel_dp);
> > > > > > >  }
> > > > > > >  
> > > > > > >  /**
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > > > index b7924feb9f27..53ea564f971e 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > > > @@ -130,6 +130,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> > > > > > >  		return "AUX_D";
> > > > > > >  	case POWER_DOMAIN_AUX_F:
> > > > > > >  		return "AUX_F";
> > > > > > > +	case POWER_DOMAIN_AUX_IO_A:
> > > > > > > +		return "AUX_IO_A";
> > > > > > >  	case POWER_DOMAIN_GMBUS:
> > > > > > >  		return "GMBUS";
> > > > > > >  	case POWER_DOMAIN_INIT:
> > > > > > > @@ -1853,6 +1855,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> > > > > > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > > > > > >  #define CNL_DISPLAY_AUX_A_POWER_DOMAINS (		\
> > > > > > >  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> > > > > > > +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
> > > > > > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > > > > > >  #define CNL_DISPLAY_AUX_B_POWER_DOMAINS (		\
> > > > > > >  	BIT_ULL(POWER_DOMAIN_AUX_B) |			\
> > > > > > > -- 
> > > > > > > 2.14.1
> > > > > > > 
> > > > > > _______________________________________________
> > > > > > 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
> > > _______________________________________________
> > > 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 v3] drm/i915/psr: New power domain for AUX IO.

[Imre Deak]

On Fri, Feb 23, 2018 at 12:58:44AM +0200, Pandiyan, Dhinakaran wrote:
> On Fri, 2018-02-23 at 00:42 +0200, Imre Deak wrote:
> > On Fri, Feb 23, 2018 at 12:30:14AM +0200, Pandiyan, Dhinakaran wrote:
> > > On Fri, 2018-02-23 at 00:05 +0200, Imre Deak wrote:
> > > > On Thu, Feb 22, 2018 at 11:53:30PM +0200, Imre Deak wrote:
> > > > > On Thu, Feb 22, 2018 at 11:33:10PM +0200, Pandiyan, Dhinakaran wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Thu, 2018-02-22 at 12:44 -0800, Rodrigo Vivi wrote:
> > > > > > > On Thu, Feb 22, 2018 at 12:28:50PM -0800, Dhinakaran Pandiyan wrote:
> > > > > > > > PSR on CNL requires AUX IO wells to be kept on and the existing AUX domain
> > > > > > > > for AUX-A enables DC_OFF well too. This is not required, so add a new
> > > > > > > > AUX_IO_A domain for AUX-A to allow DC states to remain enabled. Other AUX
> > > > > > > > channels re-use the existing AUX domains as they do need power well 2.
> > > > > > > > 
> > > > > > > > v3: Extract aux domain selection into a function (Ville)
> > > > > > > > v2: Add AUX IO domain only for AUX-A
> > > > > > > >     Rebased on top of Ville's AUX series.
> > > > > > > > 
> > > > > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > Suggested-by: Imre Deak <imre.deak@intel.com>
> > > > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/i915/intel_display.h    |  1 +
> > > > > > > >  drivers/gpu/drm/i915/intel_dp.c         |  2 +-
> > > > > > > >  drivers/gpu/drm/i915/intel_drv.h        |  1 +
> > > > > > > >  drivers/gpu/drm/i915/intel_psr.c        | 38 +++++++++++++++++++++++++++++++++
> > > > > > > >  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
> > > > > > > >  5 files changed, 44 insertions(+), 1 deletion(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > > > > > > index f5733a2576e7..4e7418b345bc 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > > > > > > @@ -186,6 +186,7 @@ enum intel_display_power_domain {
> > > > > > > >  	POWER_DOMAIN_AUX_C,
> > > > > > > >  	POWER_DOMAIN_AUX_D,
> > > > > > > >  	POWER_DOMAIN_AUX_F,
> > > > > > > > +	POWER_DOMAIN_AUX_IO_A,
> > > > > > > >  	POWER_DOMAIN_GMBUS,
> > > > > > > >  	POWER_DOMAIN_MODESET,
> > > > > > > >  	POWER_DOMAIN_GT_IRQ,
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > > index 005735a7fc29..b78b9972ebae 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > > @@ -1331,7 +1331,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> > > > > > > >  	return ret;
> > > > > > > >  }
> > > > > > > >  
> > > > > > > > -static enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > > > > > > > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > > > > > > >  {
> > > > > > > >  	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > > > > > > >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > > index 5132f6a58c6d..bcd6dc9fb13d 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > > @@ -1685,6 +1685,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
> > > > > > > >  int intel_dp_link_required(int pixel_clock, int bpp);
> > > > > > > >  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
> > > > > > > >  bool intel_digital_port_connected(struct intel_encoder *encoder);
> > > > > > > > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp);
> > > > > > > >  
> > > > > > > >  /* intel_dp_aux_backlight.c */
> > > > > > > >  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > > > index 2ef374f936b9..03814f7bc2d3 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > > > @@ -56,6 +56,40 @@
> > > > > > > >  #include "intel_drv.h"
> > > > > > > >  #include "i915_drv.h"
> > > > > > > >  
> > > > > > > > +static inline enum intel_display_power_domain
> > > > > > > > +psr_aux_domain(struct intel_dp *intel_dp)
> > > > > > > > +{
> > > > > > > > +	/* CNL HW requires corresponding AUX IOs to be powered up for PSR, AUX-A
> > > > > > > > +	 * does not require the driver to disable DC states, but the rest do.
> > > > > > > 
> > > > > > > This phrase is strange. Specially "the rest do" part.
> > > > > > > It seems that we need to disable DC states on other ports than A,
> > > > > > > what it is not true.
> > > > > > 
> > > > > > Okay, let's back up a little bit.
> > > > > > 
> > > > > > AUX domain for port A enables DC_OFF and AUX-IO. AUX domains for non-A
> > > > > > ports enable DC_OFF, power well 2 and AUX-IO, in fact all domains that
> > > > > > require power well 2 enable DC_OFF too. I can't see this is in bspec but
> > > > > > that's what the code does currently. So, this is for AUX transfers.
> > > > > 
> > > > > To enable DC5/6 power well 2 needs to be disabled; I see this is still
> > > > > missing from the BSpec DC5/6 enable sequence, though I asked already for
> > > > > an update there before. Will ask again.
> > > > 
> > > > Actually it's at "Mode Set[SKL+]/Sequences for Display C5 and C6":
> > > > 
> > > > """
> > > > ...
> > > > 2. Configure display engine to have power wells above power well 1
> > > > disabled, following the appropriate mode set disable sequences for any
> > > > ports using power wells above power well 1. This can be done earlier if
> > > > desired.
> > > > ...
> > > > 4. Set DC_STATE_EN Dynamic DC State Enable = "Enable up to DC5" for DC5
> > > > or "Enable up to DC6" for DC6.
> > > > 
> > > > """
> > > > 
> > > Thanks, finding this information would have been much easier if it was
> > > all in one place. We still have to assume the converse from the enable
> > > step. I wonder if enabling power well 2 is sufficient to bring out the
> > > hardware from DC6 without having to explicitly disable it.
> > 
> > Yep, that's not that clear, but I guess in any caes we can do that always
> > manually. Should file an update request for this.
> > 
> > > > > Non-A AUX channels are contained in power well 2, so we have to disable
> > > > > DC states and enable power well 2 for non-A AUX channels.
> > > > > 
> > > > > > 
> > > > > > which means, this comment for the previous iteration of the patch
> > > > > > "I mean, on CNL
> > > > > > 
> > > > > > POWER_DOMAIN_AUX_{B,C,D,F} are exactly the same as
> > > > > > POWER_DOMAIN_AUX__IO_{B,C,D,F}"
> > > > > > 
> > > > > > was not accurate. _AUX_IO_{B,C,D,F} domains enable only AUX_IO well and
> > > > > > _AUX_{B,C,D,F} domains enable power well 2, DC_OFF and AUX_IO.
> > > > > > 
> > > > > > 
> > > > > > For PSR, we want only the AUX_IO to be enabled for any port because the
> > > > > > spec says -
> > > > > > "PSR spontaneously sends Aux transactions.  If PSR is enabled on a port,
> > > > > > then the associated Aux IO must be kept powered up."
> > > > > > 
> > > > > > which means, this comment
> > > > > > "Hm, so in general (for AUX transfers) to enable AUX-A we first need to
> > > > > > disable DC states _except_ if we enable AUX-A for PSR where we want
> > > > > > DC5/6 to stay enabled." applies to all ports.
> > > > > 
> > > > > It doesn't make sense and we can't enable DC5/6 for non-A AUX channels,
> > > > > since those are contained in power well 2, which won't be powered on/off
> > > > > dynamically by DMC.
> > > 
> > > Is the AUX-IO contained in power well 2? The AUX_CTL registers are.
> > 
> > Not sure if the two things are divided but I would assume the HW/FW
> > needs the same registers to perform an AUX transfer as what the driver
> > programs. (Hence the need for the HW mutex.)
> > 
> That's a fair guess but kinda defeats the point of having a separate
> power well for AUX IO, doesn't it?

No, if you have also other functionality backed by PW#1/2. Then by
powering off the AUX-IO power well when AUX transfers are not needed
(like when you have only the main link on) saves power even if PW#1/2 is
enabled.

> We should perhaps check this with Art.

Sure, I agree it's not completely clear.

> > > If the hardware is sending AUX transactions on AUX_A when PSR is
> > > enabled, does it mean it is also powering up power well 1 or just the
> > > AUX-IO?
> > 
> > Again, I'd say it needs the same power as what's needed in case the
> > driver does AUX transfers. So yes, power well 1 would be powered on.
> > 
> > --Imre
> > 
> > > 
> > > 
> > > > > 
> > > > > --Imre
> > > > > 
> > > > > > 
> > > > > > To just enable the AUX-IO well, the correct version is ->
> > > > > > https://patchwork.freedesktop.org/patch/205328/
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > -DK 
> > > > > > > 
> > > > > > > > +	 * Although PSR is enabled only on Port A currently, let's do this
> > > > > > > > +	 * correctly for other ports too.
> > > > > > > > +	 */
> > > > > > > > +	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
> > > > > > > > +					      intel_dp->aux_power_domain;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static void psr_power_get(struct intel_dp *intel_dp)
> > > > > > > 
> > > > > > > nip: psr_aux_io_power_get ?!
> > > > > > > 
> > > > > > > > +{
> > > > > > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > > > > > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > > > > > > > +
> > > > > > > > +	if (INTEL_GEN(dev_priv) < 10)
> > > > > > > > +		return;
> > > > > > > > +
> > > > > > > > +	intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static void psr_power_put(struct intel_dp *intel_dp)
> > > > > > > > +{
> > > > > > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > > > > > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > > > > > > > +
> > > > > > > > +	if (INTEL_GEN(dev_priv) < 10)
> > > > > > > > +		return;
> > > > > > > > +
> > > > > > > > +	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
> > > > > > > >  {
> > > > > > > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > > > > > > @@ -459,6 +493,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> > > > > > > >  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > > > > > > >  	u32 chicken;
> > > > > > > >  
> > > > > > > > +	psr_power_get(intel_dp);
> > > > > > > > +
> > > > > > > >  	if (dev_priv->psr.psr2_support) {
> > > > > > > >  		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> > > > > > > >  		if (dev_priv->psr.y_cord_support)
> > > > > > > > @@ -617,6 +653,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
> > > > > > > >  		else
> > > > > > > >  			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > > > > > > >  	}
> > > > > > > > +
> > > > > > > > +	psr_power_put(intel_dp);
> > > > > > > >  }
> > > > > > > >  
> > > > > > > >  /**
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > > > > index b7924feb9f27..53ea564f971e 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > > > > @@ -130,6 +130,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> > > > > > > >  		return "AUX_D";
> > > > > > > >  	case POWER_DOMAIN_AUX_F:
> > > > > > > >  		return "AUX_F";
> > > > > > > > +	case POWER_DOMAIN_AUX_IO_A:
> > > > > > > > +		return "AUX_IO_A";
> > > > > > > >  	case POWER_DOMAIN_GMBUS:
> > > > > > > >  		return "GMBUS";
> > > > > > > >  	case POWER_DOMAIN_INIT:
> > > > > > > > @@ -1853,6 +1855,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> > > > > > > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > > > > > > >  #define CNL_DISPLAY_AUX_A_POWER_DOMAINS (		\
> > > > > > > >  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> > > > > > > > +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
> > > > > > > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > > > > > > >  #define CNL_DISPLAY_AUX_B_POWER_DOMAINS (		\
> > > > > > > >  	BIT_ULL(POWER_DOMAIN_AUX_B) |			\
> > > > > > > > -- 
> > > > > > > > 2.14.1
> > > > > > > > 
> > > > > > > _______________________________________________
> > > > > > > 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
> > > > _______________________________________________
> > > > 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

[PATCH v4] drm/i915/psr: New power domain for AUX IO.

[Dhinakaran Pandiyan]

PSR on CNL requires AUX IO wells to be kept on and the existing AUX domain
for AUX-A enables DC_OFF well too. This is not required, so add a new
AUX_IO_A domain for AUX-A to allow DC states to remain enabled. Other AUX
channels re-use the existing AUX domains.

v4: Reword comment (Rodrigo and Ville)
    Rename _get and _put functions to include aux_io substring(Rodrigo)
    Remove unnecessary diff that got included.
v3: Extract aux domain selection into a function (Ville)
v2: Add AUX IO domain only for AUX-A
    Rebased on top of Ville's AUX series.

Cc: Imre Deak <imre.deak@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Suggested-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_display.h    |  1 +
 drivers/gpu/drm/i915/intel_psr.c        | 41 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
 3 files changed, 45 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
index f5733a2576e7..4e7418b345bc 100644
--- a/drivers/gpu/drm/i915/intel_display.h
+++ b/drivers/gpu/drm/i915/intel_display.h
@@ -186,6 +186,7 @@ enum intel_display_power_domain {
 	POWER_DOMAIN_AUX_C,
 	POWER_DOMAIN_AUX_D,
 	POWER_DOMAIN_AUX_F,
+	POWER_DOMAIN_AUX_IO_A,
 	POWER_DOMAIN_GMBUS,
 	POWER_DOMAIN_MODESET,
 	POWER_DOMAIN_GT_IRQ,
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 2ef374f936b9..04430d4c99c9 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -56,6 +56,43 @@
 #include "intel_drv.h"
 #include "i915_drv.h"
 
+static inline enum intel_display_power_domain
+psr_aux_domain(struct intel_dp *intel_dp)
+{
+	/* CNL HW requires corresponding AUX IOs to be powered up for PSR.
+	 * However, for non-A AUX ports the corresponding non-EDP transcoders
+	 * would have already enabled power well 2 and DC_OFF. This means we can
+	 * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference instead of a
+	 * specific AUX_IO reference without powering up any extra wells.
+	 * Note that PSR is enabled only on Port A even though this function
+	 * returns the correct domain for other ports too.
+	 */
+	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
+					      intel_dp->aux_power_domain;
+}
+
+static void psr_aux_io_power_get(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
+
+	if (INTEL_GEN(dev_priv) < 10)
+		return;
+
+	intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
+}
+
+static void psr_aux_io_power_put(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
+
+	if (INTEL_GEN(dev_priv) < 10)
+		return;
+
+	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
+}
+
 static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -459,6 +496,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
 	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)
@@ -617,6 +656,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
 		else
 			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
 	}
+
+	psr_aux_io_power_put(intel_dp);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index b7924feb9f27..53ea564f971e 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -130,6 +130,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
 		return "AUX_D";
 	case POWER_DOMAIN_AUX_F:
 		return "AUX_F";
+	case POWER_DOMAIN_AUX_IO_A:
+		return "AUX_IO_A";
 	case POWER_DOMAIN_GMBUS:
 		return "GMBUS";
 	case POWER_DOMAIN_INIT:
@@ -1853,6 +1855,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 	BIT_ULL(POWER_DOMAIN_INIT))
 #define CNL_DISPLAY_AUX_A_POWER_DOMAINS (		\
 	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
+	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
 	BIT_ULL(POWER_DOMAIN_INIT))
 #define CNL_DISPLAY_AUX_B_POWER_DOMAINS (		\
 	BIT_ULL(POWER_DOMAIN_AUX_B) |			\
-- 
2.14.1

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

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Add enum aux_ch and clean up the aux init to use it (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Add enum aux_ch and clean up the aux init to use it (rev3)
URL   : https://patchwork.freedesktop.org/series/38744/
State : success

== Summary ==

Series 38744v3 series starting with [1/2] drm/i915: Add enum aux_ch and clean up the aux init to use it
https://patchwork.freedesktop.org/api/1.0/series/38744/revisions/3/mbox/

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:421s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:425s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:373s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:479s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:283s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:478s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:479s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:463s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:455s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:565s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:408s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:290s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:503s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:386s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:406s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:446s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:414s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:452s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:495s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:448s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:493s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:588s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:426s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:495s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:520s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:480s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:467s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:407s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:425s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:518s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:394s
Blacklisted hosts:
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:392s

f57b51c21b8307ba66b5212ceb886e52f506fa5e drm-tip: 2018y-02m-22d-23h-14m-05s UTC integration manifest
8d2603ac2831 drm/i915/psr: New power domain for AUX IO.
72f2fe3467be drm/i915: Add enum aux_ch and clean up the aux init to use it

== Logs ==

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

✗ Fi.CI.IGT: failure for series starting with [1/2] drm/i915: Add enum aux_ch and clean up the aux init to use it (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Add enum aux_ch and clean up the aux init to use it (rev2)
URL   : https://patchwork.freedesktop.org/series/38744/
State : failure

== Summary ==

Test gem_eio:
        Subgroup in-flight-contexts:
                pass       -> INCOMPLETE (shard-apl) fdo#104945
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-primscrn-indfb-msflip-blt:
                fail       -> PASS       (shard-apl) fdo#103167
Test kms_flip_tiling:
        Subgroup flip-to-yf-tiled:
                fail       -> PASS       (shard-apl) fdo#103822
Test perf:
        Subgroup buffer-fill:
                pass       -> FAIL       (shard-apl) fdo#103755
Test kms_plane:
        Subgroup plane-panning-bottom-right-suspend-pipe-b-planes:
                pass       -> INCOMPLETE (shard-hsw) fdo#103540
Test kms_vblank:
        Subgroup pipe-b-ts-continuation-suspend:
                pass       -> FAIL       (shard-hsw)
Test kms_flip:
        Subgroup 2x-wf_vblank-ts-check:
                fail       -> PASS       (shard-hsw) fdo#100368 +1

fdo#104945 https://bugs.freedesktop.org/show_bug.cgi?id=104945
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
fdo#103755 https://bugs.freedesktop.org/show_bug.cgi?id=103755
fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368

shard-apl        total:3428 pass:1798 dwarn:1   dfail:0   fail:13  skip:1615 time:12113s
shard-hsw        total:3448 pass:1759 dwarn:1   dfail:0   fail:3   skip:1683 time:11171s
shard-snb        total:3465 pass:1358 dwarn:1   dfail:0   fail:2   skip:2104 time:6585s

== Logs ==

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

✓ Fi.CI.IGT: success for series starting with [1/2] drm/i915: Add enum aux_ch and clean up the aux init to use it (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Add enum aux_ch and clean up the aux init to use it (rev3)
URL   : https://patchwork.freedesktop.org/series/38744/
State : success

== Summary ==

Test kms_flip:
        Subgroup 2x-plain-flip-fb-recreate-interruptible:
                fail       -> PASS       (shard-hsw) fdo#100368 +1
        Subgroup modeset-vs-vblank-race:
                pass       -> FAIL       (shard-hsw) fdo#103060 +1
        Subgroup 2x-flip-vs-expired-vblank-interruptible:
                pass       -> FAIL       (shard-hsw) fdo#102887
Test gem_eio:
        Subgroup in-flight:
                pass       -> INCOMPLETE (shard-apl) fdo#104945 +1
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-primscrn-spr-indfb-fullscreen:
                pass       -> FAIL       (shard-snb) fdo#101623

fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
fdo#104945 https://bugs.freedesktop.org/show_bug.cgi?id=104945
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623

shard-apl        total:3236 pass:1699 dwarn:1   dfail:0   fail:12  skip:1521 time:11339s
shard-hsw        total:3465 pass:1766 dwarn:1   dfail:0   fail:4   skip:1693 time:11719s
shard-snb        total:3465 pass:1357 dwarn:1   dfail:0   fail:3   skip:2104 time:6600s
Blacklisted hosts:
shard-kbl        total:3422 pass:1944 dwarn:1   dfail:0   fail:14  skip:1462 time:9352s

== Logs ==

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