All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] drm/i915: Add enum aux_ch and clean up the aux init to use it
@ 2018-02-20 17:05 Ville Syrjala
  2018-02-20 17:05 ` [PATCH 2/3] drm/i915: Nuke aux regs from intel_dp Ville Syrjala
                   ` (6 more replies)
  0 siblings, 7 replies; 20+ messages in thread
From: Ville Syrjala @ 2018-02-20 17:05 UTC (permalink / raw)
  To: intel-gfx

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>
---
 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 898064e8bea7..7f6a7f592fe6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1049,6 +1049,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.13.6

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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 2/3] drm/i915: Nuke aux regs from intel_dp
  2018-02-20 17:05 [PATCH 1/3] drm/i915: Add enum aux_ch and clean up the aux init to use it Ville Syrjala
@ 2018-02-20 17:05 ` Ville Syrjala
  2018-02-20 17:30   ` Chris Wilson
  2018-02-20 19:00   ` [PATCH v2 " Ville Syrjala
  2018-02-20 17:05 ` [PATCH 3/3] drm/i915: Collect aux ch vfunc setup into intel_dp_aux_init() Ville Syrjala
                   ` (5 subsequent siblings)
  6 siblings, 2 replies; 20+ messages in thread
From: Ville Syrjala @ 2018-02-20 17:05 UTC (permalink / raw)
  To: intel-gfx

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

Just store function pointers that give us the correct register offsets
instead of storing the register offsets themselves. Slightly less
efficient perhaps but saves a few bytes and better matches how we do
things elsewhere.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 81 +++++++++++++++++++---------------------
 drivers/gpu/drm/i915/intel_drv.h |  5 ++-
 2 files changed, 41 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index eeb8a026fd08..ea414766f82b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -936,7 +936,7 @@ static uint32_t
 intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
 {
 	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
-	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg;
+	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
 	uint32_t status;
 	bool done;
 
@@ -1089,7 +1089,7 @@ intel_dp_aux_ch(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);
-	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg;
+	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
 	uint32_t aux_clock_divider;
 	int i, ret, recv_bytes;
 	uint32_t status;
@@ -1154,7 +1154,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 		for (try = 0; try < 5; try++) {
 			/* Load the send data into the aux channel data registers */
 			for (i = 0; i < send_bytes; i += 4)
-				I915_WRITE(intel_dp->aux_ch_data_reg[i >> 2],
+				I915_WRITE(intel_dp->aux_ch_data_reg(intel_dp, i >> 2),
 					   intel_dp_pack_aux(send + i,
 							     send_bytes - i));
 
@@ -1239,7 +1239,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 		recv_bytes = recv_size;
 
 	for (i = 0; i < recv_bytes; i += 4)
-		intel_dp_unpack_aux(I915_READ(intel_dp->aux_ch_data_reg[i >> 2]),
+		intel_dp_unpack_aux(I915_READ(intel_dp->aux_ch_data_reg(intel_dp, i >> 2)),
 				    recv + i, recv_bytes - i);
 
 	ret = recv_bytes;
@@ -1396,9 +1396,11 @@ intel_aux_power_domain(struct intel_dp *intel_dp)
 	}
 }
 
-static i915_reg_t g4x_aux_ctl_reg(struct drm_i915_private *dev_priv,
-				  enum aux_ch aux_ch)
+static i915_reg_t g4x_aux_ctl_reg(struct intel_dp *intel_dp)
 {
+	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
+	enum aux_ch aux_ch = intel_dp->aux_ch;
+
 	switch (aux_ch) {
 	case AUX_CH_B:
 	case AUX_CH_C:
@@ -1410,9 +1412,11 @@ static i915_reg_t g4x_aux_ctl_reg(struct drm_i915_private *dev_priv,
 	}
 }
 
-static i915_reg_t g4x_aux_data_reg(struct drm_i915_private *dev_priv,
-				   enum aux_ch aux_ch, int index)
+static i915_reg_t g4x_aux_data_reg(struct intel_dp *intel_dp, int index)
 {
+	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
+	enum aux_ch aux_ch = intel_dp->aux_ch;
+
 	switch (aux_ch) {
 	case AUX_CH_B:
 	case AUX_CH_C:
@@ -1424,9 +1428,11 @@ static i915_reg_t g4x_aux_data_reg(struct drm_i915_private *dev_priv,
 	}
 }
 
-static i915_reg_t ilk_aux_ctl_reg(struct drm_i915_private *dev_priv,
-				  enum aux_ch aux_ch)
+static i915_reg_t ilk_aux_ctl_reg(struct intel_dp *intel_dp)
 {
+	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
+	enum aux_ch aux_ch = intel_dp->aux_ch;
+
 	switch (aux_ch) {
 	case AUX_CH_A:
 		return DP_AUX_CH_CTL(aux_ch);
@@ -1440,9 +1446,11 @@ static i915_reg_t ilk_aux_ctl_reg(struct drm_i915_private *dev_priv,
 	}
 }
 
-static i915_reg_t ilk_aux_data_reg(struct drm_i915_private *dev_priv,
-				   enum aux_ch aux_ch, int index)
+static i915_reg_t ilk_aux_data_reg(struct intel_dp *intel_dp, int index)
 {
+	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
+	enum aux_ch aux_ch = intel_dp->aux_ch;
+
 	switch (aux_ch) {
 	case AUX_CH_A:
 		return DP_AUX_CH_DATA(aux_ch, index);
@@ -1456,9 +1464,11 @@ static i915_reg_t ilk_aux_data_reg(struct drm_i915_private *dev_priv,
 	}
 }
 
-static i915_reg_t skl_aux_ctl_reg(struct drm_i915_private *dev_priv,
-				  enum aux_ch aux_ch)
+static i915_reg_t skl_aux_ctl_reg(struct intel_dp *intel_dp)
 {
+	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
+	enum aux_ch aux_ch = intel_dp->aux_ch;
+
 	switch (aux_ch) {
 	case AUX_CH_A:
 	case AUX_CH_B:
@@ -1472,9 +1482,11 @@ static i915_reg_t skl_aux_ctl_reg(struct drm_i915_private *dev_priv,
 	}
 }
 
-static i915_reg_t skl_aux_data_reg(struct drm_i915_private *dev_priv,
-				   enum aux_ch aux_ch, int index)
+static i915_reg_t skl_aux_data_reg(struct intel_dp *intel_dp, int index)
 {
+	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
+	enum aux_ch aux_ch = intel_dp->aux_ch;
+
 	switch (aux_ch) {
 	case AUX_CH_A:
 	case AUX_CH_B:
@@ -1488,37 +1500,20 @@ static i915_reg_t skl_aux_data_reg(struct drm_i915_private *dev_priv,
 	}
 }
 
-static i915_reg_t intel_aux_ctl_reg(struct drm_i915_private *dev_priv,
-				    enum aux_ch aux_ch)
-{
-	if (INTEL_GEN(dev_priv) >= 9)
-		return skl_aux_ctl_reg(dev_priv, aux_ch);
-	else if (HAS_PCH_SPLIT(dev_priv))
-		return ilk_aux_ctl_reg(dev_priv, aux_ch);
-	else
-		return g4x_aux_ctl_reg(dev_priv, aux_ch);
-}
-
-static i915_reg_t intel_aux_data_reg(struct drm_i915_private *dev_priv,
-				     enum aux_ch aux_ch, int index)
-{
-	if (INTEL_GEN(dev_priv) >= 9)
-		return skl_aux_data_reg(dev_priv, aux_ch, index);
-	else if (HAS_PCH_SPLIT(dev_priv))
-		return ilk_aux_data_reg(dev_priv, aux_ch, index);
-	else
-		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 aux_ch aux_ch = intel_dp->aux_ch;
-	int i;
 
-	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, aux_ch, i);
+	if (INTEL_GEN(dev_priv) >= 9) {
+		intel_dp->aux_ch_ctl_reg = skl_aux_ctl_reg;
+		intel_dp->aux_ch_data_reg = skl_aux_data_reg;
+	} else if (HAS_PCH_SPLIT(dev_priv)) {
+		intel_dp->aux_ch_ctl_reg = ilk_aux_ctl_reg;
+		intel_dp->aux_ch_data_reg = ilk_aux_data_reg;
+	} else {
+		intel_dp->aux_ch_ctl_reg = g4x_aux_ctl_reg;
+		intel_dp->aux_ch_data_reg = g4x_aux_data_reg;
+	}
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7f6a7f592fe6..a72820fe5262 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1038,8 +1038,6 @@ struct intel_dp_compliance {
 
 struct intel_dp {
 	i915_reg_t output_reg;
-	i915_reg_t aux_ch_ctl_reg;
-	i915_reg_t aux_ch_data_reg[5];
 	uint32_t DP;
 	int link_rate;
 	uint8_t lane_count;
@@ -1124,6 +1122,9 @@ struct intel_dp {
 				     int send_bytes,
 				     uint32_t aux_clock_divider);
 
+	i915_reg_t (*aux_ch_ctl_reg)(struct intel_dp *dp);
+	i915_reg_t (*aux_ch_data_reg)(struct intel_dp *dp, int index);
+
 	/* This is called before a link training is starterd */
 	void (*prepare_link_retrain)(struct intel_dp *intel_dp);
 
-- 
2.13.6

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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 3/3] drm/i915: Collect aux ch vfunc setup into intel_dp_aux_init()
  2018-02-20 17:05 [PATCH 1/3] drm/i915: Add enum aux_ch and clean up the aux init to use it Ville Syrjala
  2018-02-20 17:05 ` [PATCH 2/3] drm/i915: Nuke aux regs from intel_dp Ville Syrjala
@ 2018-02-20 17:05 ` Ville Syrjala
  2018-02-20 17:25   ` Chris Wilson
  2018-02-20 19:35   ` Rodrigo Vivi
  2018-02-20 17:28 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/i915: Add enum aux_ch and clean up the aux init to use it Patchwork
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 20+ messages in thread
From: Ville Syrjala @ 2018-02-20 17:05 UTC (permalink / raw)
  To: intel-gfx

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

Collect all the aux ch vfunc assignments into intel_dp_aux_init()
instead of having it spread around.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 53 +++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ea414766f82b..005735a7fc29 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1500,9 +1500,20 @@ static i915_reg_t skl_aux_data_reg(struct intel_dp *intel_dp, int index)
 	}
 }
 
-static void intel_aux_reg_init(struct intel_dp *intel_dp)
+static void
+intel_dp_aux_fini(struct intel_dp *intel_dp)
+{
+	kfree(intel_dp->aux.name);
+}
+
+static void
+intel_dp_aux_init(struct intel_dp *intel_dp)
 {
 	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
+	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);
 
 	if (INTEL_GEN(dev_priv) >= 9) {
 		intel_dp->aux_ch_ctl_reg = skl_aux_ctl_reg;
@@ -1514,23 +1525,21 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
 		intel_dp->aux_ch_ctl_reg = g4x_aux_ctl_reg;
 		intel_dp->aux_ch_data_reg = g4x_aux_data_reg;
 	}
-}
 
-static void
-intel_dp_aux_fini(struct intel_dp *intel_dp)
-{
-	kfree(intel_dp->aux.name);
-}
-
-static void
-intel_dp_aux_init(struct intel_dp *intel_dp)
-{
-	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
+	if (INTEL_GEN(dev_priv) >= 9)
+		intel_dp->get_aux_clock_divider = skl_get_aux_clock_divider;
+	else if (IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
+		intel_dp->get_aux_clock_divider = hsw_get_aux_clock_divider;
+	else if (HAS_PCH_SPLIT(dev_priv))
+		intel_dp->get_aux_clock_divider = ilk_get_aux_clock_divider;
+	else
+		intel_dp->get_aux_clock_divider = g4x_get_aux_clock_divider;
 
-	intel_dp->aux_ch = intel_aux_ch(intel_dp);
-	intel_dp->aux_power_domain = intel_aux_power_domain(intel_dp);
+	if (INTEL_GEN(dev_priv) >= 9)
+		intel_dp->get_aux_send_ctl = skl_get_aux_send_ctl;
+	else
+		intel_dp->get_aux_send_ctl = g4x_get_aux_send_ctl;
 
-	intel_aux_reg_init(intel_dp);
 	drm_dp_aux_init(&intel_dp->aux);
 
 	/* Failure to allocate our preferred name is not critical */
@@ -6338,20 +6347,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	intel_dp->active_pipe = INVALID_PIPE;
 
 	/* intel_dp vfuncs */
-	if (INTEL_GEN(dev_priv) >= 9)
-		intel_dp->get_aux_clock_divider = skl_get_aux_clock_divider;
-	else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
-		intel_dp->get_aux_clock_divider = hsw_get_aux_clock_divider;
-	else if (HAS_PCH_SPLIT(dev_priv))
-		intel_dp->get_aux_clock_divider = ilk_get_aux_clock_divider;
-	else
-		intel_dp->get_aux_clock_divider = g4x_get_aux_clock_divider;
-
-	if (INTEL_GEN(dev_priv) >= 9)
-		intel_dp->get_aux_send_ctl = skl_get_aux_send_ctl;
-	else
-		intel_dp->get_aux_send_ctl = g4x_get_aux_send_ctl;
-
 	if (HAS_DDI(dev_priv))
 		intel_dp->prepare_link_retrain = intel_ddi_prepare_link_retrain;
 
-- 
2.13.6

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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] drm/i915: Collect aux ch vfunc setup into intel_dp_aux_init()
  2018-02-20 17:05 ` [PATCH 3/3] drm/i915: Collect aux ch vfunc setup into intel_dp_aux_init() Ville Syrjala
@ 2018-02-20 17:25   ` Chris Wilson
  2018-02-20 19:35   ` Rodrigo Vivi
  1 sibling, 0 replies; 20+ messages in thread
From: Chris Wilson @ 2018-02-20 17:25 UTC (permalink / raw)
  To: Ville Syrjala, intel-gfx

Quoting Ville Syrjala (2018-02-20 17:05:24)
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Collect all the aux ch vfunc assignments into intel_dp_aux_init()
> instead of having it spread around.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 20+ messages in thread

* ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/i915: Add enum aux_ch and clean up the aux init to use it
  2018-02-20 17:05 [PATCH 1/3] drm/i915: Add enum aux_ch and clean up the aux init to use it Ville Syrjala
  2018-02-20 17:05 ` [PATCH 2/3] drm/i915: Nuke aux regs from intel_dp Ville Syrjala
  2018-02-20 17:05 ` [PATCH 3/3] drm/i915: Collect aux ch vfunc setup into intel_dp_aux_init() Ville Syrjala
@ 2018-02-20 17:28 ` Patchwork
  2018-02-20 17:43 ` ✓ Fi.CI.BAT: success " Patchwork
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 20+ messages in thread
From: Patchwork @ 2018-02-20 17:28 UTC (permalink / raw)
  To: Ville Syrjala; +Cc: intel-gfx

== Series Details ==

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

== Summary ==

$ dim checkpatch origin/drm-tip
b454295ca54f drm/i915: Add enum aux_ch and clean up the aux init to use it
-:29: WARNING: line over 80 characters
#29: 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)

-:30: WARNING: line over 80 characters
#30: 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 */

-:40: WARNING: line over 80 characters
#40: 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)

-:41: WARNING: line over 80 characters
#41: 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 */

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

-:347: WARNING: line over 80 characters
#347: 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
51b5f8ecc90b drm/i915: Nuke aux regs from intel_dp
-:43: WARNING: line over 80 characters
#43: FILE: drivers/gpu/drm/i915/intel_dp.c:1157:
+				I915_WRITE(intel_dp->aux_ch_data_reg(intel_dp, i >> 2),

-:52: WARNING: line over 80 characters
#52: FILE: drivers/gpu/drm/i915/intel_dp.c:1242:
+		intel_dp_unpack_aux(I915_READ(intel_dp->aux_ch_data_reg(intel_dp, i >> 2)),

total: 0 errors, 2 warnings, 0 checks, 174 lines checked
76545e043355 drm/i915: Collect aux ch vfunc setup into intel_dp_aux_init()

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] drm/i915: Nuke aux regs from intel_dp
  2018-02-20 17:05 ` [PATCH 2/3] drm/i915: Nuke aux regs from intel_dp Ville Syrjala
@ 2018-02-20 17:30   ` Chris Wilson
  2018-02-20 17:49     ` Ville Syrjälä
  2018-02-20 19:00   ` [PATCH v2 " Ville Syrjala
  1 sibling, 1 reply; 20+ messages in thread
From: Chris Wilson @ 2018-02-20 17:30 UTC (permalink / raw)
  To: Ville Syrjala, intel-gfx

Quoting Ville Syrjala (2018-02-20 17:05:23)
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Just store function pointers that give us the correct register offsets
> instead of storing the register offsets themselves. Slightly less
> efficient perhaps but saves a few bytes and better matches how we do
> things elsewhere.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 81 +++++++++++++++++++---------------------
>  drivers/gpu/drm/i915/intel_drv.h |  5 ++-
>  2 files changed, 41 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index eeb8a026fd08..ea414766f82b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -936,7 +936,7 @@ static uint32_t
>  intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
>  {
>         struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> -       i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg;
> +       i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
>         uint32_t status;
>         bool done;
>  
> @@ -1089,7 +1089,7 @@ intel_dp_aux_ch(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);
> -       i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg;
> +       i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
>         uint32_t aux_clock_divider;
>         int i, ret, recv_bytes;
>         uint32_t status;
> @@ -1154,7 +1154,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>                 for (try = 0; try < 5; try++) {
>                         /* Load the send data into the aux channel data registers */
>                         for (i = 0; i < send_bytes; i += 4)
> -                               I915_WRITE(intel_dp->aux_ch_data_reg[i >> 2],
> +                               I915_WRITE(intel_dp->aux_ch_data_reg(intel_dp, i >> 2),
>                                            intel_dp_pack_aux(send + i,
>                                                              send_bytes - i));
>  
> @@ -1239,7 +1239,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>                 recv_bytes = recv_size;
>  
>         for (i = 0; i < recv_bytes; i += 4)
> -               intel_dp_unpack_aux(I915_READ(intel_dp->aux_ch_data_reg[i >> 2]),
> +               intel_dp_unpack_aux(I915_READ(intel_dp->aux_ch_data_reg(intel_dp, i >> 2)),
>                                     recv + i, recv_bytes - i);

I might have created data_reg[5] locals for intel_dp_aux_ch(), and even
consider intel_dp->aux_ch_data_regs fill the entire reg[5] array in one
pass.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 20+ messages in thread

* ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Add enum aux_ch and clean up the aux init to use it
  2018-02-20 17:05 [PATCH 1/3] drm/i915: Add enum aux_ch and clean up the aux init to use it Ville Syrjala
                   ` (2 preceding siblings ...)
  2018-02-20 17:28 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/i915: Add enum aux_ch and clean up the aux init to use it Patchwork
@ 2018-02-20 17:43 ` Patchwork
  2018-02-20 17:47 ` [PATCH 1/3] " Chris Wilson
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 20+ messages in thread
From: Patchwork @ 2018-02-20 17:43 UTC (permalink / raw)
  To: Ville Syrjala; +Cc: intel-gfx

== Series Details ==

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

== Summary ==

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

Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                pass       -> FAIL       (fi-gdg-551) fdo#102575
Test gem_ringfill:
        Subgroup basic-default-hang:
                dmesg-warn -> INCOMPLETE (fi-pnv-d510) fdo#101600
Test kms_chamelium:
        Subgroup dp-edid-read:
                pass       -> FAIL       (fi-kbl-7500u) fdo#102505
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                incomplete -> PASS       (fi-snb-2520m) fdo#103713
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#101600 https://bugs.freedesktop.org/show_bug.cgi?id=101600
fdo#102505 https://bugs.freedesktop.org/show_bug.cgi?id=102505
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
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:426s
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:475s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:285s
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:469s
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:179  dwarn:0   dfail:0   fail:1   skip:108 time:281s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:505s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:386s
fi-ilk-650       total:288  pass:227  dwarn:0   dfail:0   fail:1   skip:60  time:413s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:460s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:408s
fi-kbl-7500u     total:288  pass:262  dwarn:1   dfail:0   fail:1   skip:24  time:446s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:485s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:443s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:493s
fi-pnv-d510      total:146  pass:113  dwarn:0   dfail:0   fail:0   skip:32 
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:428s
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:521s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:489s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:480s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:406s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:431s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:507s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:392s

6697b76215fde36745a73b95453febbdb8c63991 drm-tip: 2018y-02m-20d-16h-40m-48s UTC integration manifest
76545e043355 drm/i915: Collect aux ch vfunc setup into intel_dp_aux_init()
51b5f8ecc90b drm/i915: Nuke aux regs from intel_dp
b454295ca54f 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_8083/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/3] drm/i915: Add enum aux_ch and clean up the aux init to use it
  2018-02-20 17:05 [PATCH 1/3] drm/i915: Add enum aux_ch and clean up the aux init to use it Ville Syrjala
                   ` (3 preceding siblings ...)
  2018-02-20 17:43 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-02-20 17:47 ` Chris Wilson
  2018-02-20 19:31 ` Rodrigo Vivi
  2018-02-22  6:12 ` Pandiyan, Dhinakaran
  6 siblings, 0 replies; 20+ messages in thread
From: Chris Wilson @ 2018-02-20 17:47 UTC (permalink / raw)
  To: Ville Syrjala, intel-gfx

Quoting Ville Syrjala (2018-02-20 17:05:22)
> 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>

The translation looks fine; nothing stood out and utterly confused me.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Should definitely seek a second opinion on the new enum vs abusing port.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] drm/i915: Nuke aux regs from intel_dp
  2018-02-20 17:30   ` Chris Wilson
@ 2018-02-20 17:49     ` Ville Syrjälä
  2018-02-20 17:57       ` Chris Wilson
  0 siblings, 1 reply; 20+ messages in thread
From: Ville Syrjälä @ 2018-02-20 17:49 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Tue, Feb 20, 2018 at 05:30:51PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjala (2018-02-20 17:05:23)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Just store function pointers that give us the correct register offsets
> > instead of storing the register offsets themselves. Slightly less
> > efficient perhaps but saves a few bytes and better matches how we do
> > things elsewhere.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c  | 81 +++++++++++++++++++---------------------
> >  drivers/gpu/drm/i915/intel_drv.h |  5 ++-
> >  2 files changed, 41 insertions(+), 45 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index eeb8a026fd08..ea414766f82b 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -936,7 +936,7 @@ static uint32_t
> >  intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
> >  {
> >         struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> > -       i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg;
> > +       i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
> >         uint32_t status;
> >         bool done;
> >  
> > @@ -1089,7 +1089,7 @@ intel_dp_aux_ch(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);
> > -       i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg;
> > +       i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
> >         uint32_t aux_clock_divider;
> >         int i, ret, recv_bytes;
> >         uint32_t status;
> > @@ -1154,7 +1154,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >                 for (try = 0; try < 5; try++) {
> >                         /* Load the send data into the aux channel data registers */
> >                         for (i = 0; i < send_bytes; i += 4)
> > -                               I915_WRITE(intel_dp->aux_ch_data_reg[i >> 2],
> > +                               I915_WRITE(intel_dp->aux_ch_data_reg(intel_dp, i >> 2),
> >                                            intel_dp_pack_aux(send + i,
> >                                                              send_bytes - i));
> >  
> > @@ -1239,7 +1239,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >                 recv_bytes = recv_size;
> >  
> >         for (i = 0; i < recv_bytes; i += 4)
> > -               intel_dp_unpack_aux(I915_READ(intel_dp->aux_ch_data_reg[i >> 2]),
> > +               intel_dp_unpack_aux(I915_READ(intel_dp->aux_ch_data_reg(intel_dp, i >> 2)),
> >                                     recv + i, recv_bytes - i);
> 
> I might have created data_reg[5] locals for intel_dp_aux_ch(), and even
> consider intel_dp->aux_ch_data_regs fill the entire reg[5] array in one
> pass.

Hmm. We do use a 'ch_ctl' local for the ctl reg so something
like that wouldn't feel totally out of place.

This is the only caller so not sure if there's much point in having the
vfuncs populate the entire array.

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 005735a7fc29..787c0aa45163 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1089,7 +1089,7 @@ intel_dp_aux_ch(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);
-	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
+	i915_reg_t ch_ctl, ch_data[5];
 	uint32_t aux_clock_divider;
 	int i, ret, recv_bytes;
 	uint32_t status;
@@ -1097,6 +1097,10 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 	bool has_aux_irq = HAS_AUX_IRQ(dev_priv);
 	bool vdd;
 
+	ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
+	for (i = 0; i < ARRAY_SIZE(ch_data); i++)
+		     ch_data[i] = intel_dp->aux_ch_data_reg(intel_dp, i);
+
 	pps_lock(intel_dp);
 
 	/*
@@ -1154,7 +1158,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 		for (try = 0; try < 5; try++) {
 			/* Load the send data into the aux channel data registers */
 			for (i = 0; i < send_bytes; i += 4)
-				I915_WRITE(intel_dp->aux_ch_data_reg(intel_dp, i >> 2),
+				I915_WRITE(ch_data[i >> 2],
 					   intel_dp_pack_aux(send + i,
 							     send_bytes - i));
 
@@ -1239,7 +1243,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 		recv_bytes = recv_size;
 
 	for (i = 0; i < recv_bytes; i += 4)
-		intel_dp_unpack_aux(I915_READ(intel_dp->aux_ch_data_reg(intel_dp, i >> 2)),
+		intel_dp_unpack_aux(I915_READ(ch_data[i >> 2]),
 				    recv + i, recv_bytes - i);
 
 	ret = recv_bytes;
-- 
2.13.6

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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] drm/i915: Nuke aux regs from intel_dp
  2018-02-20 17:49     ` Ville Syrjälä
@ 2018-02-20 17:57       ` Chris Wilson
  0 siblings, 0 replies; 20+ messages in thread
From: Chris Wilson @ 2018-02-20 17:57 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

Quoting Ville Syrjälä (2018-02-20 17:49:43)
> On Tue, Feb 20, 2018 at 05:30:51PM +0000, Chris Wilson wrote:
> > Quoting Ville Syrjala (2018-02-20 17:05:23)
> > > @@ -1089,7 +1089,7 @@ intel_dp_aux_ch(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);
> > > -       i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg;
> > > +       i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
> > >         uint32_t aux_clock_divider;
> > >         int i, ret, recv_bytes;
> > >         uint32_t status;
> > > @@ -1154,7 +1154,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >                 for (try = 0; try < 5; try++) {
> > >                         /* Load the send data into the aux channel data registers */
> > >                         for (i = 0; i < send_bytes; i += 4)
> > > -                               I915_WRITE(intel_dp->aux_ch_data_reg[i >> 2],
> > > +                               I915_WRITE(intel_dp->aux_ch_data_reg(intel_dp, i >> 2),
> > >                                            intel_dp_pack_aux(send + i,
> > >                                                              send_bytes - i));
> > >  
> > > @@ -1239,7 +1239,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >                 recv_bytes = recv_size;
> > >  
> > >         for (i = 0; i < recv_bytes; i += 4)
> > > -               intel_dp_unpack_aux(I915_READ(intel_dp->aux_ch_data_reg[i >> 2]),
> > > +               intel_dp_unpack_aux(I915_READ(intel_dp->aux_ch_data_reg(intel_dp, i >> 2)),
> > >                                     recv + i, recv_bytes - i);
> > 
> > I might have created data_reg[5] locals for intel_dp_aux_ch(), and even
> > consider intel_dp->aux_ch_data_regs fill the entire reg[5] array in one
> > pass.
> 
> Hmm. We do use a 'ch_ctl' local for the ctl reg so something
> like that wouldn't feel totally out of place.
> 
> This is the only caller so not sure if there's much point in having the
> vfuncs populate the entire array.
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 005735a7fc29..787c0aa45163 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1089,7 +1089,7 @@ intel_dp_aux_ch(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);
> -       i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
> +       i915_reg_t ch_ctl, ch_data[5];
>         uint32_t aux_clock_divider;
>         int i, ret, recv_bytes;
>         uint32_t status;
> @@ -1097,6 +1097,10 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>         bool has_aux_irq = HAS_AUX_IRQ(dev_priv);
>         bool vdd;
>  
> +       ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
> +       for (i = 0; i < ARRAY_SIZE(ch_data); i++)
> +                    ch_data[i] = intel_dp->aux_ch_data_reg(intel_dp, i);

This has my vote.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v2 2/3] drm/i915: Nuke aux regs from intel_dp
  2018-02-20 17:05 ` [PATCH 2/3] drm/i915: Nuke aux regs from intel_dp Ville Syrjala
  2018-02-20 17:30   ` Chris Wilson
@ 2018-02-20 19:00   ` Ville Syrjala
  2018-02-22  7:16     ` Pandiyan, Dhinakaran
  1 sibling, 1 reply; 20+ messages in thread
From: Ville Syrjala @ 2018-02-20 19:00 UTC (permalink / raw)
  To: intel-gfx

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

Just store function pointers that give us the correct register offsets
instead of storing the register offsets themselves. Slightly less
efficient perhaps but saves a few bytes and better matches how we do
things elsewhere.

v2: Keep a local array of data registers (Chris)

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 85 ++++++++++++++++++++--------------------
 drivers/gpu/drm/i915/intel_drv.h |  5 ++-
 2 files changed, 45 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index eeb8a026fd08..b0c273b5b2a9 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -936,7 +936,7 @@ static uint32_t
 intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
 {
 	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
-	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg;
+	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
 	uint32_t status;
 	bool done;
 
@@ -1089,7 +1089,7 @@ intel_dp_aux_ch(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);
-	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg;
+	i915_reg_t ch_ctl, ch_data[5];
 	uint32_t aux_clock_divider;
 	int i, ret, recv_bytes;
 	uint32_t status;
@@ -1097,6 +1097,10 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 	bool has_aux_irq = HAS_AUX_IRQ(dev_priv);
 	bool vdd;
 
+	ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
+	for (i = 0; i < ARRAY_SIZE(ch_data); i++)
+		     ch_data[i] = intel_dp->aux_ch_data_reg(intel_dp, i);
+
 	pps_lock(intel_dp);
 
 	/*
@@ -1154,7 +1158,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 		for (try = 0; try < 5; try++) {
 			/* Load the send data into the aux channel data registers */
 			for (i = 0; i < send_bytes; i += 4)
-				I915_WRITE(intel_dp->aux_ch_data_reg[i >> 2],
+				I915_WRITE(ch_data[i >> 2],
 					   intel_dp_pack_aux(send + i,
 							     send_bytes - i));
 
@@ -1239,7 +1243,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 		recv_bytes = recv_size;
 
 	for (i = 0; i < recv_bytes; i += 4)
-		intel_dp_unpack_aux(I915_READ(intel_dp->aux_ch_data_reg[i >> 2]),
+		intel_dp_unpack_aux(I915_READ(ch_data[i >> 2]),
 				    recv + i, recv_bytes - i);
 
 	ret = recv_bytes;
@@ -1396,9 +1400,11 @@ intel_aux_power_domain(struct intel_dp *intel_dp)
 	}
 }
 
-static i915_reg_t g4x_aux_ctl_reg(struct drm_i915_private *dev_priv,
-				  enum aux_ch aux_ch)
+static i915_reg_t g4x_aux_ctl_reg(struct intel_dp *intel_dp)
 {
+	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
+	enum aux_ch aux_ch = intel_dp->aux_ch;
+
 	switch (aux_ch) {
 	case AUX_CH_B:
 	case AUX_CH_C:
@@ -1410,9 +1416,11 @@ static i915_reg_t g4x_aux_ctl_reg(struct drm_i915_private *dev_priv,
 	}
 }
 
-static i915_reg_t g4x_aux_data_reg(struct drm_i915_private *dev_priv,
-				   enum aux_ch aux_ch, int index)
+static i915_reg_t g4x_aux_data_reg(struct intel_dp *intel_dp, int index)
 {
+	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
+	enum aux_ch aux_ch = intel_dp->aux_ch;
+
 	switch (aux_ch) {
 	case AUX_CH_B:
 	case AUX_CH_C:
@@ -1424,9 +1432,11 @@ static i915_reg_t g4x_aux_data_reg(struct drm_i915_private *dev_priv,
 	}
 }
 
-static i915_reg_t ilk_aux_ctl_reg(struct drm_i915_private *dev_priv,
-				  enum aux_ch aux_ch)
+static i915_reg_t ilk_aux_ctl_reg(struct intel_dp *intel_dp)
 {
+	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
+	enum aux_ch aux_ch = intel_dp->aux_ch;
+
 	switch (aux_ch) {
 	case AUX_CH_A:
 		return DP_AUX_CH_CTL(aux_ch);
@@ -1440,9 +1450,11 @@ static i915_reg_t ilk_aux_ctl_reg(struct drm_i915_private *dev_priv,
 	}
 }
 
-static i915_reg_t ilk_aux_data_reg(struct drm_i915_private *dev_priv,
-				   enum aux_ch aux_ch, int index)
+static i915_reg_t ilk_aux_data_reg(struct intel_dp *intel_dp, int index)
 {
+	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
+	enum aux_ch aux_ch = intel_dp->aux_ch;
+
 	switch (aux_ch) {
 	case AUX_CH_A:
 		return DP_AUX_CH_DATA(aux_ch, index);
@@ -1456,9 +1468,11 @@ static i915_reg_t ilk_aux_data_reg(struct drm_i915_private *dev_priv,
 	}
 }
 
-static i915_reg_t skl_aux_ctl_reg(struct drm_i915_private *dev_priv,
-				  enum aux_ch aux_ch)
+static i915_reg_t skl_aux_ctl_reg(struct intel_dp *intel_dp)
 {
+	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
+	enum aux_ch aux_ch = intel_dp->aux_ch;
+
 	switch (aux_ch) {
 	case AUX_CH_A:
 	case AUX_CH_B:
@@ -1472,9 +1486,11 @@ static i915_reg_t skl_aux_ctl_reg(struct drm_i915_private *dev_priv,
 	}
 }
 
-static i915_reg_t skl_aux_data_reg(struct drm_i915_private *dev_priv,
-				   enum aux_ch aux_ch, int index)
+static i915_reg_t skl_aux_data_reg(struct intel_dp *intel_dp, int index)
 {
+	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
+	enum aux_ch aux_ch = intel_dp->aux_ch;
+
 	switch (aux_ch) {
 	case AUX_CH_A:
 	case AUX_CH_B:
@@ -1488,37 +1504,20 @@ static i915_reg_t skl_aux_data_reg(struct drm_i915_private *dev_priv,
 	}
 }
 
-static i915_reg_t intel_aux_ctl_reg(struct drm_i915_private *dev_priv,
-				    enum aux_ch aux_ch)
-{
-	if (INTEL_GEN(dev_priv) >= 9)
-		return skl_aux_ctl_reg(dev_priv, aux_ch);
-	else if (HAS_PCH_SPLIT(dev_priv))
-		return ilk_aux_ctl_reg(dev_priv, aux_ch);
-	else
-		return g4x_aux_ctl_reg(dev_priv, aux_ch);
-}
-
-static i915_reg_t intel_aux_data_reg(struct drm_i915_private *dev_priv,
-				     enum aux_ch aux_ch, int index)
-{
-	if (INTEL_GEN(dev_priv) >= 9)
-		return skl_aux_data_reg(dev_priv, aux_ch, index);
-	else if (HAS_PCH_SPLIT(dev_priv))
-		return ilk_aux_data_reg(dev_priv, aux_ch, index);
-	else
-		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 aux_ch aux_ch = intel_dp->aux_ch;
-	int i;
 
-	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, aux_ch, i);
+	if (INTEL_GEN(dev_priv) >= 9) {
+		intel_dp->aux_ch_ctl_reg = skl_aux_ctl_reg;
+		intel_dp->aux_ch_data_reg = skl_aux_data_reg;
+	} else if (HAS_PCH_SPLIT(dev_priv)) {
+		intel_dp->aux_ch_ctl_reg = ilk_aux_ctl_reg;
+		intel_dp->aux_ch_data_reg = ilk_aux_data_reg;
+	} else {
+		intel_dp->aux_ch_ctl_reg = g4x_aux_ctl_reg;
+		intel_dp->aux_ch_data_reg = g4x_aux_data_reg;
+	}
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7f6a7f592fe6..a72820fe5262 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1038,8 +1038,6 @@ struct intel_dp_compliance {
 
 struct intel_dp {
 	i915_reg_t output_reg;
-	i915_reg_t aux_ch_ctl_reg;
-	i915_reg_t aux_ch_data_reg[5];
 	uint32_t DP;
 	int link_rate;
 	uint8_t lane_count;
@@ -1124,6 +1122,9 @@ struct intel_dp {
 				     int send_bytes,
 				     uint32_t aux_clock_divider);
 
+	i915_reg_t (*aux_ch_ctl_reg)(struct intel_dp *dp);
+	i915_reg_t (*aux_ch_data_reg)(struct intel_dp *dp, int index);
+
 	/* This is called before a link training is starterd */
 	void (*prepare_link_retrain)(struct intel_dp *intel_dp);
 
-- 
2.13.6

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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/3] drm/i915: Add enum aux_ch and clean up the aux init to use it
  2018-02-20 17:05 [PATCH 1/3] drm/i915: Add enum aux_ch and clean up the aux init to use it Ville Syrjala
                   ` (4 preceding siblings ...)
  2018-02-20 17:47 ` [PATCH 1/3] " Chris Wilson
@ 2018-02-20 19:31 ` Rodrigo Vivi
  2018-02-22  7:25   ` Pandiyan, Dhinakaran
  2018-02-22 16:29   ` Ville Syrjälä
  2018-02-22  6:12 ` Pandiyan, Dhinakaran
  6 siblings, 2 replies; 20+ messages in thread
From: Rodrigo Vivi @ 2018-02-20 19:31 UTC (permalink / raw)
  To: Ville Syrjala; +Cc: intel-gfx

On Tue, Feb 20, 2018 at 07:05:22PM +0200, Ville Syrjala 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.


Beautiful clean-up. I had this in a todo list years ago and
after staying on the bottom for so long I had removed it from there...
> 
> v2: Rebase due to AUX F

sorry and thanks! :)

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I wondered if at least aux_power_domain part could be in a
separated patch because by the end I was a bit confused...
But in the end I could put all pieces together and it made sense
again. So one patch for easy back porting seems better.

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 898064e8bea7..7f6a7f592fe6 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1049,6 +1049,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.13.6
> 
> _______________________________________________
> 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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] drm/i915: Collect aux ch vfunc setup into intel_dp_aux_init()
  2018-02-20 17:05 ` [PATCH 3/3] drm/i915: Collect aux ch vfunc setup into intel_dp_aux_init() Ville Syrjala
  2018-02-20 17:25   ` Chris Wilson
@ 2018-02-20 19:35   ` Rodrigo Vivi
  1 sibling, 0 replies; 20+ messages in thread
From: Rodrigo Vivi @ 2018-02-20 19:35 UTC (permalink / raw)
  To: Ville Syrjala; +Cc: intel-gfx

On Tue, Feb 20, 2018 at 07:05:24PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Collect all the aux ch vfunc assignments into intel_dp_aux_init()
> instead of having it spread around.

Makes sense and seems cleaner and right.

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

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


> ---
>  drivers/gpu/drm/i915/intel_dp.c | 53 +++++++++++++++++++----------------------
>  1 file changed, 24 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index ea414766f82b..005735a7fc29 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1500,9 +1500,20 @@ static i915_reg_t skl_aux_data_reg(struct intel_dp *intel_dp, int index)
>  	}
>  }
>  
> -static void intel_aux_reg_init(struct intel_dp *intel_dp)
> +static void
> +intel_dp_aux_fini(struct intel_dp *intel_dp)
> +{
> +	kfree(intel_dp->aux.name);
> +}
> +
> +static void
> +intel_dp_aux_init(struct intel_dp *intel_dp)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> +	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);
>  
>  	if (INTEL_GEN(dev_priv) >= 9) {
>  		intel_dp->aux_ch_ctl_reg = skl_aux_ctl_reg;
> @@ -1514,23 +1525,21 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
>  		intel_dp->aux_ch_ctl_reg = g4x_aux_ctl_reg;
>  		intel_dp->aux_ch_data_reg = g4x_aux_data_reg;
>  	}
> -}
>  
> -static void
> -intel_dp_aux_fini(struct intel_dp *intel_dp)
> -{
> -	kfree(intel_dp->aux.name);
> -}
> -
> -static void
> -intel_dp_aux_init(struct intel_dp *intel_dp)
> -{
> -	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> +	if (INTEL_GEN(dev_priv) >= 9)
> +		intel_dp->get_aux_clock_divider = skl_get_aux_clock_divider;
> +	else if (IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
> +		intel_dp->get_aux_clock_divider = hsw_get_aux_clock_divider;
> +	else if (HAS_PCH_SPLIT(dev_priv))
> +		intel_dp->get_aux_clock_divider = ilk_get_aux_clock_divider;
> +	else
> +		intel_dp->get_aux_clock_divider = g4x_get_aux_clock_divider;
>  
> -	intel_dp->aux_ch = intel_aux_ch(intel_dp);
> -	intel_dp->aux_power_domain = intel_aux_power_domain(intel_dp);
> +	if (INTEL_GEN(dev_priv) >= 9)
> +		intel_dp->get_aux_send_ctl = skl_get_aux_send_ctl;
> +	else
> +		intel_dp->get_aux_send_ctl = g4x_get_aux_send_ctl;
>  
> -	intel_aux_reg_init(intel_dp);
>  	drm_dp_aux_init(&intel_dp->aux);
>  
>  	/* Failure to allocate our preferred name is not critical */
> @@ -6338,20 +6347,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  	intel_dp->active_pipe = INVALID_PIPE;
>  
>  	/* intel_dp vfuncs */
> -	if (INTEL_GEN(dev_priv) >= 9)
> -		intel_dp->get_aux_clock_divider = skl_get_aux_clock_divider;
> -	else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> -		intel_dp->get_aux_clock_divider = hsw_get_aux_clock_divider;
> -	else if (HAS_PCH_SPLIT(dev_priv))
> -		intel_dp->get_aux_clock_divider = ilk_get_aux_clock_divider;
> -	else
> -		intel_dp->get_aux_clock_divider = g4x_get_aux_clock_divider;
> -
> -	if (INTEL_GEN(dev_priv) >= 9)
> -		intel_dp->get_aux_send_ctl = skl_get_aux_send_ctl;
> -	else
> -		intel_dp->get_aux_send_ctl = g4x_get_aux_send_ctl;
> -
>  	if (HAS_DDI(dev_priv))
>  		intel_dp->prepare_link_retrain = intel_ddi_prepare_link_retrain;
>  
> -- 
> 2.13.6
> 
> _______________________________________________
> 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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/3] drm/i915: Add enum aux_ch and clean up the aux init to use it
  2018-02-20 17:05 [PATCH 1/3] drm/i915: Add enum aux_ch and clean up the aux init to use it Ville Syrjala
                   ` (5 preceding siblings ...)
  2018-02-20 19:31 ` Rodrigo Vivi
@ 2018-02-22  6:12 ` Pandiyan, Dhinakaran
  6 siblings, 0 replies; 20+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-02-22  6:12 UTC (permalink / raw)
  To: ville.syrjala; +Cc: intel-gfx




On Tue, 2018-02-20 at 19:05 +0200, Ville Syrjala 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.

Neat, this solves the problem I noticed while adding cnl aux io power
domain. Thanks.

> 
> v2: Rebase due to AUX F
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/3] drm/i915: Nuke aux regs from intel_dp
  2018-02-20 19:00   ` [PATCH v2 " Ville Syrjala
@ 2018-02-22  7:16     ` Pandiyan, Dhinakaran
  2018-02-22 12:43       ` Ville Syrjälä
  0 siblings, 1 reply; 20+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-02-22  7:16 UTC (permalink / raw)
  To: ville.syrjala; +Cc: intel-gfx


On Tue, 2018-02-20 at 21:00 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Just store function pointers that give us the correct register offsets
> instead of storing the register offsets themselves. Slightly less
> efficient perhaps but saves a few bytes and better matches how we do
> things elsewhere.
> 
> v2: Keep a local array of data registers (Chris)
> 
Intriguing, why bother storing register offsets in one go if it's okay
to make these additional function calls for every aux transaction.

What am I missing here?



> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 85 ++++++++++++++++++++--------------------
>  drivers/gpu/drm/i915/intel_drv.h |  5 ++-
>  2 files changed, 45 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index eeb8a026fd08..b0c273b5b2a9 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -936,7 +936,7 @@ static uint32_t
>  intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> -	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg;
> +	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
>  	uint32_t status;
>  	bool done;
>  
> @@ -1089,7 +1089,7 @@ intel_dp_aux_ch(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);
> -	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg;
> +	i915_reg_t ch_ctl, ch_data[5];
>  	uint32_t aux_clock_divider;
>  	int i, ret, recv_bytes;
>  	uint32_t status;
> @@ -1097,6 +1097,10 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  	bool has_aux_irq = HAS_AUX_IRQ(dev_priv);
>  	bool vdd;
>  
> +	ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
> +	for (i = 0; i < ARRAY_SIZE(ch_data); i++)
> +		     ch_data[i] = intel_dp->aux_ch_data_reg(intel_dp, i);
> +
>  	pps_lock(intel_dp);
>  
>  	/*
> @@ -1154,7 +1158,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  		for (try = 0; try < 5; try++) {
>  			/* Load the send data into the aux channel data registers */
>  			for (i = 0; i < send_bytes; i += 4)
> -				I915_WRITE(intel_dp->aux_ch_data_reg[i >> 2],
> +				I915_WRITE(ch_data[i >> 2],
>  					   intel_dp_pack_aux(send + i,
>  							     send_bytes - i));
>  
> @@ -1239,7 +1243,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  		recv_bytes = recv_size;
>  
>  	for (i = 0; i < recv_bytes; i += 4)
> -		intel_dp_unpack_aux(I915_READ(intel_dp->aux_ch_data_reg[i >> 2]),
> +		intel_dp_unpack_aux(I915_READ(ch_data[i >> 2]),
>  				    recv + i, recv_bytes - i);
>  
>  	ret = recv_bytes;
> @@ -1396,9 +1400,11 @@ intel_aux_power_domain(struct intel_dp *intel_dp)
>  	}
>  }
>  
> -static i915_reg_t g4x_aux_ctl_reg(struct drm_i915_private *dev_priv,
> -				  enum aux_ch aux_ch)
> +static i915_reg_t g4x_aux_ctl_reg(struct intel_dp *intel_dp)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> +	enum aux_ch aux_ch = intel_dp->aux_ch;
> +
>  	switch (aux_ch) {
>  	case AUX_CH_B:
>  	case AUX_CH_C:
> @@ -1410,9 +1416,11 @@ static i915_reg_t g4x_aux_ctl_reg(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> -static i915_reg_t g4x_aux_data_reg(struct drm_i915_private *dev_priv,
> -				   enum aux_ch aux_ch, int index)
> +static i915_reg_t g4x_aux_data_reg(struct intel_dp *intel_dp, int index)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> +	enum aux_ch aux_ch = intel_dp->aux_ch;
> +
>  	switch (aux_ch) {
>  	case AUX_CH_B:
>  	case AUX_CH_C:
> @@ -1424,9 +1432,11 @@ static i915_reg_t g4x_aux_data_reg(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> -static i915_reg_t ilk_aux_ctl_reg(struct drm_i915_private *dev_priv,
> -				  enum aux_ch aux_ch)
> +static i915_reg_t ilk_aux_ctl_reg(struct intel_dp *intel_dp)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> +	enum aux_ch aux_ch = intel_dp->aux_ch;
> +
>  	switch (aux_ch) {
>  	case AUX_CH_A:
>  		return DP_AUX_CH_CTL(aux_ch);
> @@ -1440,9 +1450,11 @@ static i915_reg_t ilk_aux_ctl_reg(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> -static i915_reg_t ilk_aux_data_reg(struct drm_i915_private *dev_priv,
> -				   enum aux_ch aux_ch, int index)
> +static i915_reg_t ilk_aux_data_reg(struct intel_dp *intel_dp, int index)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> +	enum aux_ch aux_ch = intel_dp->aux_ch;
> +
>  	switch (aux_ch) {
>  	case AUX_CH_A:
>  		return DP_AUX_CH_DATA(aux_ch, index);
> @@ -1456,9 +1468,11 @@ static i915_reg_t ilk_aux_data_reg(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> -static i915_reg_t skl_aux_ctl_reg(struct drm_i915_private *dev_priv,
> -				  enum aux_ch aux_ch)
> +static i915_reg_t skl_aux_ctl_reg(struct intel_dp *intel_dp)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> +	enum aux_ch aux_ch = intel_dp->aux_ch;
> +
>  	switch (aux_ch) {
>  	case AUX_CH_A:
>  	case AUX_CH_B:
> @@ -1472,9 +1486,11 @@ static i915_reg_t skl_aux_ctl_reg(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> -static i915_reg_t skl_aux_data_reg(struct drm_i915_private *dev_priv,
> -				   enum aux_ch aux_ch, int index)
> +static i915_reg_t skl_aux_data_reg(struct intel_dp *intel_dp, int index)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> +	enum aux_ch aux_ch = intel_dp->aux_ch;
> +
>  	switch (aux_ch) {
>  	case AUX_CH_A:
>  	case AUX_CH_B:
> @@ -1488,37 +1504,20 @@ static i915_reg_t skl_aux_data_reg(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> -static i915_reg_t intel_aux_ctl_reg(struct drm_i915_private *dev_priv,
> -				    enum aux_ch aux_ch)
> -{
> -	if (INTEL_GEN(dev_priv) >= 9)
> -		return skl_aux_ctl_reg(dev_priv, aux_ch);
> -	else if (HAS_PCH_SPLIT(dev_priv))
> -		return ilk_aux_ctl_reg(dev_priv, aux_ch);
> -	else
> -		return g4x_aux_ctl_reg(dev_priv, aux_ch);
> -}
> -
> -static i915_reg_t intel_aux_data_reg(struct drm_i915_private *dev_priv,
> -				     enum aux_ch aux_ch, int index)
> -{
> -	if (INTEL_GEN(dev_priv) >= 9)
> -		return skl_aux_data_reg(dev_priv, aux_ch, index);
> -	else if (HAS_PCH_SPLIT(dev_priv))
> -		return ilk_aux_data_reg(dev_priv, aux_ch, index);
> -	else
> -		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 aux_ch aux_ch = intel_dp->aux_ch;
> -	int i;
>  
> -	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, aux_ch, i);
> +	if (INTEL_GEN(dev_priv) >= 9) {
> +		intel_dp->aux_ch_ctl_reg = skl_aux_ctl_reg;
> +		intel_dp->aux_ch_data_reg = skl_aux_data_reg;
> +	} else if (HAS_PCH_SPLIT(dev_priv)) {
> +		intel_dp->aux_ch_ctl_reg = ilk_aux_ctl_reg;
> +		intel_dp->aux_ch_data_reg = ilk_aux_data_reg;
> +	} else {
> +		intel_dp->aux_ch_ctl_reg = g4x_aux_ctl_reg;
> +		intel_dp->aux_ch_data_reg = g4x_aux_data_reg;
> +	}
>  }
>  
>  static void
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 7f6a7f592fe6..a72820fe5262 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1038,8 +1038,6 @@ struct intel_dp_compliance {
>  
>  struct intel_dp {
>  	i915_reg_t output_reg;
> -	i915_reg_t aux_ch_ctl_reg;
> -	i915_reg_t aux_ch_data_reg[5];
>  	uint32_t DP;
>  	int link_rate;
>  	uint8_t lane_count;
> @@ -1124,6 +1122,9 @@ struct intel_dp {
>  				     int send_bytes,
>  				     uint32_t aux_clock_divider);
>  
> +	i915_reg_t (*aux_ch_ctl_reg)(struct intel_dp *dp);
> +	i915_reg_t (*aux_ch_data_reg)(struct intel_dp *dp, int index);
> +
>  	/* This is called before a link training is starterd */
>  	void (*prepare_link_retrain)(struct intel_dp *intel_dp);
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/3] drm/i915: Add enum aux_ch and clean up the aux init to use it
  2018-02-20 19:31 ` Rodrigo Vivi
@ 2018-02-22  7:25   ` Pandiyan, Dhinakaran
  2018-02-22 12:48     ` Ville Syrjälä
  2018-02-22 16:29   ` Ville Syrjälä
  1 sibling, 1 reply; 20+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-02-22  7:25 UTC (permalink / raw)
  To: Vivi, Rodrigo; +Cc: intel-gfx




On Tue, 2018-02-20 at 11:31 -0800, Rodrigo Vivi wrote:
> On Tue, Feb 20, 2018 at 07:05:22PM +0200, Ville Syrjala 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.
> 
> 
> Beautiful clean-up. I had this in a todo list years ago and
> after staying on the bottom for so long I had removed it from there...
> > 
> > v2: Rebase due to AUX F
> 
> sorry and thanks! :)
> 
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I wondered if at least aux_power_domain part could be in a
> separated patch because by the end I was a bit confused...
> But in the end I could put all pieces together and it made sense
> again. So one patch for easy back porting seems better.
> 
> 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);

Shouldn't intel_dp->aux_ch change too if we are switching to fallback
default registers?

Patch 3/3 assigns intel_dp->aux_ch and then assigns control and data
registers.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/3] drm/i915: Nuke aux regs from intel_dp
  2018-02-22  7:16     ` Pandiyan, Dhinakaran
@ 2018-02-22 12:43       ` Ville Syrjälä
  2018-02-22 20:38         ` Pandiyan, Dhinakaran
  0 siblings, 1 reply; 20+ messages in thread
From: Ville Syrjälä @ 2018-02-22 12:43 UTC (permalink / raw)
  To: Pandiyan, Dhinakaran; +Cc: intel-gfx

On Thu, Feb 22, 2018 at 07:16:07AM +0000, Pandiyan, Dhinakaran wrote:
> 
> On Tue, 2018-02-20 at 21:00 +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Just store function pointers that give us the correct register offsets
> > instead of storing the register offsets themselves. Slightly less
> > efficient perhaps but saves a few bytes and better matches how we do
> > things elsewhere.
> > 
> > v2: Keep a local array of data registers (Chris)
> > 
> Intriguing, why bother storing register offsets in one go if it's okay
> to make these additional function calls for every aux transaction.
> 
> What am I missing here?

Slight optimization to do them only once per aux transfer instead of
potentially multiple times on account of the retry loops.

> 
> 
> 
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c  | 85 ++++++++++++++++++++--------------------
> >  drivers/gpu/drm/i915/intel_drv.h |  5 ++-
> >  2 files changed, 45 insertions(+), 45 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index eeb8a026fd08..b0c273b5b2a9 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -936,7 +936,7 @@ static uint32_t
> >  intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> > -	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg;
> > +	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
> >  	uint32_t status;
> >  	bool done;
> >  
> > @@ -1089,7 +1089,7 @@ intel_dp_aux_ch(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);
> > -	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg;
> > +	i915_reg_t ch_ctl, ch_data[5];
> >  	uint32_t aux_clock_divider;
> >  	int i, ret, recv_bytes;
> >  	uint32_t status;
> > @@ -1097,6 +1097,10 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >  	bool has_aux_irq = HAS_AUX_IRQ(dev_priv);
> >  	bool vdd;
> >  
> > +	ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
> > +	for (i = 0; i < ARRAY_SIZE(ch_data); i++)
> > +		     ch_data[i] = intel_dp->aux_ch_data_reg(intel_dp, i);
> > +
> >  	pps_lock(intel_dp);
> >  
> >  	/*
> > @@ -1154,7 +1158,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >  		for (try = 0; try < 5; try++) {
> >  			/* Load the send data into the aux channel data registers */
> >  			for (i = 0; i < send_bytes; i += 4)
> > -				I915_WRITE(intel_dp->aux_ch_data_reg[i >> 2],
> > +				I915_WRITE(ch_data[i >> 2],
> >  					   intel_dp_pack_aux(send + i,
> >  							     send_bytes - i));
> >  
> > @@ -1239,7 +1243,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >  		recv_bytes = recv_size;
> >  
> >  	for (i = 0; i < recv_bytes; i += 4)
> > -		intel_dp_unpack_aux(I915_READ(intel_dp->aux_ch_data_reg[i >> 2]),
> > +		intel_dp_unpack_aux(I915_READ(ch_data[i >> 2]),
> >  				    recv + i, recv_bytes - i);
> >  
> >  	ret = recv_bytes;
> > @@ -1396,9 +1400,11 @@ intel_aux_power_domain(struct intel_dp *intel_dp)
> >  	}
> >  }
> >  
> > -static i915_reg_t g4x_aux_ctl_reg(struct drm_i915_private *dev_priv,
> > -				  enum aux_ch aux_ch)
> > +static i915_reg_t g4x_aux_ctl_reg(struct intel_dp *intel_dp)
> >  {
> > +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> > +	enum aux_ch aux_ch = intel_dp->aux_ch;
> > +
> >  	switch (aux_ch) {
> >  	case AUX_CH_B:
> >  	case AUX_CH_C:
> > @@ -1410,9 +1416,11 @@ static i915_reg_t g4x_aux_ctl_reg(struct drm_i915_private *dev_priv,
> >  	}
> >  }
> >  
> > -static i915_reg_t g4x_aux_data_reg(struct drm_i915_private *dev_priv,
> > -				   enum aux_ch aux_ch, int index)
> > +static i915_reg_t g4x_aux_data_reg(struct intel_dp *intel_dp, int index)
> >  {
> > +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> > +	enum aux_ch aux_ch = intel_dp->aux_ch;
> > +
> >  	switch (aux_ch) {
> >  	case AUX_CH_B:
> >  	case AUX_CH_C:
> > @@ -1424,9 +1432,11 @@ static i915_reg_t g4x_aux_data_reg(struct drm_i915_private *dev_priv,
> >  	}
> >  }
> >  
> > -static i915_reg_t ilk_aux_ctl_reg(struct drm_i915_private *dev_priv,
> > -				  enum aux_ch aux_ch)
> > +static i915_reg_t ilk_aux_ctl_reg(struct intel_dp *intel_dp)
> >  {
> > +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> > +	enum aux_ch aux_ch = intel_dp->aux_ch;
> > +
> >  	switch (aux_ch) {
> >  	case AUX_CH_A:
> >  		return DP_AUX_CH_CTL(aux_ch);
> > @@ -1440,9 +1450,11 @@ static i915_reg_t ilk_aux_ctl_reg(struct drm_i915_private *dev_priv,
> >  	}
> >  }
> >  
> > -static i915_reg_t ilk_aux_data_reg(struct drm_i915_private *dev_priv,
> > -				   enum aux_ch aux_ch, int index)
> > +static i915_reg_t ilk_aux_data_reg(struct intel_dp *intel_dp, int index)
> >  {
> > +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> > +	enum aux_ch aux_ch = intel_dp->aux_ch;
> > +
> >  	switch (aux_ch) {
> >  	case AUX_CH_A:
> >  		return DP_AUX_CH_DATA(aux_ch, index);
> > @@ -1456,9 +1468,11 @@ static i915_reg_t ilk_aux_data_reg(struct drm_i915_private *dev_priv,
> >  	}
> >  }
> >  
> > -static i915_reg_t skl_aux_ctl_reg(struct drm_i915_private *dev_priv,
> > -				  enum aux_ch aux_ch)
> > +static i915_reg_t skl_aux_ctl_reg(struct intel_dp *intel_dp)
> >  {
> > +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> > +	enum aux_ch aux_ch = intel_dp->aux_ch;
> > +
> >  	switch (aux_ch) {
> >  	case AUX_CH_A:
> >  	case AUX_CH_B:
> > @@ -1472,9 +1486,11 @@ static i915_reg_t skl_aux_ctl_reg(struct drm_i915_private *dev_priv,
> >  	}
> >  }
> >  
> > -static i915_reg_t skl_aux_data_reg(struct drm_i915_private *dev_priv,
> > -				   enum aux_ch aux_ch, int index)
> > +static i915_reg_t skl_aux_data_reg(struct intel_dp *intel_dp, int index)
> >  {
> > +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> > +	enum aux_ch aux_ch = intel_dp->aux_ch;
> > +
> >  	switch (aux_ch) {
> >  	case AUX_CH_A:
> >  	case AUX_CH_B:
> > @@ -1488,37 +1504,20 @@ static i915_reg_t skl_aux_data_reg(struct drm_i915_private *dev_priv,
> >  	}
> >  }
> >  
> > -static i915_reg_t intel_aux_ctl_reg(struct drm_i915_private *dev_priv,
> > -				    enum aux_ch aux_ch)
> > -{
> > -	if (INTEL_GEN(dev_priv) >= 9)
> > -		return skl_aux_ctl_reg(dev_priv, aux_ch);
> > -	else if (HAS_PCH_SPLIT(dev_priv))
> > -		return ilk_aux_ctl_reg(dev_priv, aux_ch);
> > -	else
> > -		return g4x_aux_ctl_reg(dev_priv, aux_ch);
> > -}
> > -
> > -static i915_reg_t intel_aux_data_reg(struct drm_i915_private *dev_priv,
> > -				     enum aux_ch aux_ch, int index)
> > -{
> > -	if (INTEL_GEN(dev_priv) >= 9)
> > -		return skl_aux_data_reg(dev_priv, aux_ch, index);
> > -	else if (HAS_PCH_SPLIT(dev_priv))
> > -		return ilk_aux_data_reg(dev_priv, aux_ch, index);
> > -	else
> > -		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 aux_ch aux_ch = intel_dp->aux_ch;
> > -	int i;
> >  
> > -	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, aux_ch, i);
> > +	if (INTEL_GEN(dev_priv) >= 9) {
> > +		intel_dp->aux_ch_ctl_reg = skl_aux_ctl_reg;
> > +		intel_dp->aux_ch_data_reg = skl_aux_data_reg;
> > +	} else if (HAS_PCH_SPLIT(dev_priv)) {
> > +		intel_dp->aux_ch_ctl_reg = ilk_aux_ctl_reg;
> > +		intel_dp->aux_ch_data_reg = ilk_aux_data_reg;
> > +	} else {
> > +		intel_dp->aux_ch_ctl_reg = g4x_aux_ctl_reg;
> > +		intel_dp->aux_ch_data_reg = g4x_aux_data_reg;
> > +	}
> >  }
> >  
> >  static void
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 7f6a7f592fe6..a72820fe5262 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1038,8 +1038,6 @@ struct intel_dp_compliance {
> >  
> >  struct intel_dp {
> >  	i915_reg_t output_reg;
> > -	i915_reg_t aux_ch_ctl_reg;
> > -	i915_reg_t aux_ch_data_reg[5];
> >  	uint32_t DP;
> >  	int link_rate;
> >  	uint8_t lane_count;
> > @@ -1124,6 +1122,9 @@ struct intel_dp {
> >  				     int send_bytes,
> >  				     uint32_t aux_clock_divider);
> >  
> > +	i915_reg_t (*aux_ch_ctl_reg)(struct intel_dp *dp);
> > +	i915_reg_t (*aux_ch_data_reg)(struct intel_dp *dp, int index);
> > +
> >  	/* This is called before a link training is starterd */
> >  	void (*prepare_link_retrain)(struct intel_dp *intel_dp);
> >  

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/3] drm/i915: Add enum aux_ch and clean up the aux init to use it
  2018-02-22  7:25   ` Pandiyan, Dhinakaran
@ 2018-02-22 12:48     ` Ville Syrjälä
  0 siblings, 0 replies; 20+ messages in thread
From: Ville Syrjälä @ 2018-02-22 12:48 UTC (permalink / raw)
  To: Pandiyan, Dhinakaran; +Cc: intel-gfx, Vivi, Rodrigo

On Thu, Feb 22, 2018 at 07:25:22AM +0000, Pandiyan, Dhinakaran wrote:
> 
> 
> 
> On Tue, 2018-02-20 at 11:31 -0800, Rodrigo Vivi wrote:
> > On Tue, Feb 20, 2018 at 07:05:22PM +0200, Ville Syrjala 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.
> > 
> > 
> > Beautiful clean-up. I had this in a todo list years ago and
> > after staying on the bottom for so long I had removed it from there...
> > > 
> > > v2: Rebase due to AUX F
> > 
> > sorry and thanks! :)
> > 
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > I wondered if at least aux_power_domain part could be in a
> > separated patch because by the end I was a bit confused...
> > But in the end I could put all pieces together and it made sense
> > again. So one patch for easy back porting seems better.
> > 
> > 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);
> 
> Shouldn't intel_dp->aux_ch change too if we are switching to fallback
> default registers?

The default case is there just to appease the compiler really.
Whatever we do here is most likely going to be wrong anyway so
there's little point in trying to make it polished in any way.

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/3] drm/i915: Add enum aux_ch and clean up the aux init to use it
  2018-02-20 19:31 ` Rodrigo Vivi
  2018-02-22  7:25   ` Pandiyan, Dhinakaran
@ 2018-02-22 16:29   ` Ville Syrjälä
  1 sibling, 0 replies; 20+ messages in thread
From: Ville Syrjälä @ 2018-02-22 16:29 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: intel-gfx

On Tue, Feb 20, 2018 at 11:31:09AM -0800, Rodrigo Vivi wrote:
> On Tue, Feb 20, 2018 at 07:05:22PM +0200, Ville Syrjala 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.
> 
> 
> Beautiful clean-up. I had this in a todo list years ago and
> after staying on the bottom for so long I had removed it from there...
> > 
> > v2: Rebase due to AUX F
> 
> sorry and thanks! :)
> 
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I wondered if at least aux_power_domain part could be in a
> separated patch because by the end I was a bit confused...
> But in the end I could put all pieces together and it made sense
> again. So one patch for easy back porting seems better.

I suppose we might want to backport just the power_domain fix.
So might be worth extracting that into a separate patch. Should
be a simple matter of s/encoder->port/intel_dp_aux_port()/

> 
> 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 898064e8bea7..7f6a7f592fe6 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1049,6 +1049,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.13.6
> > 
> > _______________________________________________
> > 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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/3] drm/i915: Nuke aux regs from intel_dp
  2018-02-22 12:43       ` Ville Syrjälä
@ 2018-02-22 20:38         ` Pandiyan, Dhinakaran
  0 siblings, 0 replies; 20+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-02-22 20:38 UTC (permalink / raw)
  To: ville.syrjala; +Cc: intel-gfx




On Thu, 2018-02-22 at 14:43 +0200, Ville Syrjälä wrote:
> On Thu, Feb 22, 2018 at 07:16:07AM +0000, Pandiyan, Dhinakaran wrote:
> > 
> > On Tue, 2018-02-20 at 21:00 +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Just store function pointers that give us the correct register offsets
> > > instead of storing the register offsets themselves. Slightly less
> > > efficient perhaps but saves a few bytes and better matches how we do
> > > things elsewhere.
> > > 
> > > v2: Keep a local array of data registers (Chris)
> > > 
> > Intriguing, why bother storing register offsets in one go if it's okay
> > to make these additional function calls for every aux transaction.
> > 
> > What am I missing here?
> 
> Slight optimization to do them only once per aux transfer instead of
> potentially multiple times on account of the retry loops.
> 

Yeah, that optimization intrigued me because the additional function
calls this patch introduces make the existing code suboptimal. :)
That's enough bikeshed I suppose, thanks for your answer.


> > 
> > 
> > 
> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c  | 85 ++++++++++++++++++++--------------------
> > >  drivers/gpu/drm/i915/intel_drv.h |  5 ++-
> > >  2 files changed, 45 insertions(+), 45 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index eeb8a026fd08..b0c273b5b2a9 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -936,7 +936,7 @@ static uint32_t
> > >  intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> > > -	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg;
> > > +	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
> > >  	uint32_t status;
> > >  	bool done;
> > >  
> > > @@ -1089,7 +1089,7 @@ intel_dp_aux_ch(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);
> > > -	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg;
> > > +	i915_reg_t ch_ctl, ch_data[5];
> > >  	uint32_t aux_clock_divider;
> > >  	int i, ret, recv_bytes;
> > >  	uint32_t status;
> > > @@ -1097,6 +1097,10 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >  	bool has_aux_irq = HAS_AUX_IRQ(dev_priv);
> > >  	bool vdd;
> > >  
> > > +	ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
> > > +	for (i = 0; i < ARRAY_SIZE(ch_data); i++)
> > > +		     ch_data[i] = intel_dp->aux_ch_data_reg(intel_dp, i);
> > > +
> > >  	pps_lock(intel_dp);
> > >  
> > >  	/*
> > > @@ -1154,7 +1158,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >  		for (try = 0; try < 5; try++) {
> > >  			/* Load the send data into the aux channel data registers */
> > >  			for (i = 0; i < send_bytes; i += 4)
> > > -				I915_WRITE(intel_dp->aux_ch_data_reg[i >> 2],
> > > +				I915_WRITE(ch_data[i >> 2],
> > >  					   intel_dp_pack_aux(send + i,
> > >  							     send_bytes - i));
> > >  
> > > @@ -1239,7 +1243,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >  		recv_bytes = recv_size;
> > >  
> > >  	for (i = 0; i < recv_bytes; i += 4)
> > > -		intel_dp_unpack_aux(I915_READ(intel_dp->aux_ch_data_reg[i >> 2]),
> > > +		intel_dp_unpack_aux(I915_READ(ch_data[i >> 2]),
> > >  				    recv + i, recv_bytes - i);
> > >  
> > >  	ret = recv_bytes;
> > > @@ -1396,9 +1400,11 @@ intel_aux_power_domain(struct intel_dp *intel_dp)
> > >  	}
> > >  }
> > >  
> > > -static i915_reg_t g4x_aux_ctl_reg(struct drm_i915_private *dev_priv,
> > > -				  enum aux_ch aux_ch)
> > > +static i915_reg_t g4x_aux_ctl_reg(struct intel_dp *intel_dp)
> > >  {
> > > +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> > > +	enum aux_ch aux_ch = intel_dp->aux_ch;
> > > +
> > >  	switch (aux_ch) {
> > >  	case AUX_CH_B:
> > >  	case AUX_CH_C:
> > > @@ -1410,9 +1416,11 @@ static i915_reg_t g4x_aux_ctl_reg(struct drm_i915_private *dev_priv,
> > >  	}
> > >  }
> > >  
> > > -static i915_reg_t g4x_aux_data_reg(struct drm_i915_private *dev_priv,
> > > -				   enum aux_ch aux_ch, int index)
> > > +static i915_reg_t g4x_aux_data_reg(struct intel_dp *intel_dp, int index)
> > >  {
> > > +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> > > +	enum aux_ch aux_ch = intel_dp->aux_ch;
> > > +
> > >  	switch (aux_ch) {
> > >  	case AUX_CH_B:
> > >  	case AUX_CH_C:
> > > @@ -1424,9 +1432,11 @@ static i915_reg_t g4x_aux_data_reg(struct drm_i915_private *dev_priv,
> > >  	}
> > >  }
> > >  
> > > -static i915_reg_t ilk_aux_ctl_reg(struct drm_i915_private *dev_priv,
> > > -				  enum aux_ch aux_ch)
> > > +static i915_reg_t ilk_aux_ctl_reg(struct intel_dp *intel_dp)
> > >  {
> > > +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> > > +	enum aux_ch aux_ch = intel_dp->aux_ch;
> > > +
> > >  	switch (aux_ch) {
> > >  	case AUX_CH_A:
> > >  		return DP_AUX_CH_CTL(aux_ch);
> > > @@ -1440,9 +1450,11 @@ static i915_reg_t ilk_aux_ctl_reg(struct drm_i915_private *dev_priv,
> > >  	}
> > >  }
> > >  
> > > -static i915_reg_t ilk_aux_data_reg(struct drm_i915_private *dev_priv,
> > > -				   enum aux_ch aux_ch, int index)
> > > +static i915_reg_t ilk_aux_data_reg(struct intel_dp *intel_dp, int index)
> > >  {
> > > +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> > > +	enum aux_ch aux_ch = intel_dp->aux_ch;
> > > +
> > >  	switch (aux_ch) {
> > >  	case AUX_CH_A:
> > >  		return DP_AUX_CH_DATA(aux_ch, index);
> > > @@ -1456,9 +1468,11 @@ static i915_reg_t ilk_aux_data_reg(struct drm_i915_private *dev_priv,
> > >  	}
> > >  }
> > >  
> > > -static i915_reg_t skl_aux_ctl_reg(struct drm_i915_private *dev_priv,
> > > -				  enum aux_ch aux_ch)
> > > +static i915_reg_t skl_aux_ctl_reg(struct intel_dp *intel_dp)
> > >  {
> > > +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> > > +	enum aux_ch aux_ch = intel_dp->aux_ch;
> > > +
> > >  	switch (aux_ch) {
> > >  	case AUX_CH_A:
> > >  	case AUX_CH_B:
> > > @@ -1472,9 +1486,11 @@ static i915_reg_t skl_aux_ctl_reg(struct drm_i915_private *dev_priv,
> > >  	}
> > >  }
> > >  
> > > -static i915_reg_t skl_aux_data_reg(struct drm_i915_private *dev_priv,
> > > -				   enum aux_ch aux_ch, int index)
> > > +static i915_reg_t skl_aux_data_reg(struct intel_dp *intel_dp, int index)
> > >  {
> > > +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> > > +	enum aux_ch aux_ch = intel_dp->aux_ch;
> > > +
> > >  	switch (aux_ch) {
> > >  	case AUX_CH_A:
> > >  	case AUX_CH_B:
> > > @@ -1488,37 +1504,20 @@ static i915_reg_t skl_aux_data_reg(struct drm_i915_private *dev_priv,
> > >  	}
> > >  }
> > >  
> > > -static i915_reg_t intel_aux_ctl_reg(struct drm_i915_private *dev_priv,
> > > -				    enum aux_ch aux_ch)
> > > -{
> > > -	if (INTEL_GEN(dev_priv) >= 9)
> > > -		return skl_aux_ctl_reg(dev_priv, aux_ch);
> > > -	else if (HAS_PCH_SPLIT(dev_priv))
> > > -		return ilk_aux_ctl_reg(dev_priv, aux_ch);
> > > -	else
> > > -		return g4x_aux_ctl_reg(dev_priv, aux_ch);
> > > -}
> > > -
> > > -static i915_reg_t intel_aux_data_reg(struct drm_i915_private *dev_priv,
> > > -				     enum aux_ch aux_ch, int index)
> > > -{
> > > -	if (INTEL_GEN(dev_priv) >= 9)
> > > -		return skl_aux_data_reg(dev_priv, aux_ch, index);
> > > -	else if (HAS_PCH_SPLIT(dev_priv))
> > > -		return ilk_aux_data_reg(dev_priv, aux_ch, index);
> > > -	else
> > > -		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 aux_ch aux_ch = intel_dp->aux_ch;
> > > -	int i;
> > >  
> > > -	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, aux_ch, i);
> > > +	if (INTEL_GEN(dev_priv) >= 9) {
> > > +		intel_dp->aux_ch_ctl_reg = skl_aux_ctl_reg;
> > > +		intel_dp->aux_ch_data_reg = skl_aux_data_reg;
> > > +	} else if (HAS_PCH_SPLIT(dev_priv)) {
> > > +		intel_dp->aux_ch_ctl_reg = ilk_aux_ctl_reg;
> > > +		intel_dp->aux_ch_data_reg = ilk_aux_data_reg;
> > > +	} else {
> > > +		intel_dp->aux_ch_ctl_reg = g4x_aux_ctl_reg;
> > > +		intel_dp->aux_ch_data_reg = g4x_aux_data_reg;
> > > +	}
> > >  }
> > >  
> > >  static void
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 7f6a7f592fe6..a72820fe5262 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1038,8 +1038,6 @@ struct intel_dp_compliance {
> > >  
> > >  struct intel_dp {
> > >  	i915_reg_t output_reg;
> > > -	i915_reg_t aux_ch_ctl_reg;
> > > -	i915_reg_t aux_ch_data_reg[5];
> > >  	uint32_t DP;
> > >  	int link_rate;
> > >  	uint8_t lane_count;
> > > @@ -1124,6 +1122,9 @@ struct intel_dp {
> > >  				     int send_bytes,
> > >  				     uint32_t aux_clock_divider);
> > >  
> > > +	i915_reg_t (*aux_ch_ctl_reg)(struct intel_dp *dp);
> > > +	i915_reg_t (*aux_ch_data_reg)(struct intel_dp *dp, int index);
> > > +
> > >  	/* This is called before a link training is starterd */
> > >  	void (*prepare_link_retrain)(struct intel_dp *intel_dp);
> > >  
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2018-02-22 20:38 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-20 17:05 [PATCH 1/3] drm/i915: Add enum aux_ch and clean up the aux init to use it Ville Syrjala
2018-02-20 17:05 ` [PATCH 2/3] drm/i915: Nuke aux regs from intel_dp Ville Syrjala
2018-02-20 17:30   ` Chris Wilson
2018-02-20 17:49     ` Ville Syrjälä
2018-02-20 17:57       ` Chris Wilson
2018-02-20 19:00   ` [PATCH v2 " Ville Syrjala
2018-02-22  7:16     ` Pandiyan, Dhinakaran
2018-02-22 12:43       ` Ville Syrjälä
2018-02-22 20:38         ` Pandiyan, Dhinakaran
2018-02-20 17:05 ` [PATCH 3/3] drm/i915: Collect aux ch vfunc setup into intel_dp_aux_init() Ville Syrjala
2018-02-20 17:25   ` Chris Wilson
2018-02-20 19:35   ` Rodrigo Vivi
2018-02-20 17:28 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/i915: Add enum aux_ch and clean up the aux init to use it Patchwork
2018-02-20 17:43 ` ✓ Fi.CI.BAT: success " Patchwork
2018-02-20 17:47 ` [PATCH 1/3] " Chris Wilson
2018-02-20 19:31 ` Rodrigo Vivi
2018-02-22  7:25   ` Pandiyan, Dhinakaran
2018-02-22 12:48     ` Ville Syrjälä
2018-02-22 16:29   ` Ville Syrjälä
2018-02-22  6:12 ` Pandiyan, Dhinakaran

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.