[PATCH v2 0/8] drm/i915/lspcon: Work around resume failure

[PATCH v2 0/8] drm/i915/lspcon: Work around resume failure

[Imre Deak]

This is v2 of [1] addressing the review comments from Jani and Shashank.
Meanwhile I also checked that the SKL-6700 machine reported in bugzilla
(see the last patch) had the same problem as the APL where I saw this
first and the workaround seems to solve the problem there too.

[1]
https://lists.freedesktop.org/archives/intel-gfx/2016-October/109295.html

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Shashank Sharma <shashank.sharma@intel.com>

Imre Deak (8):
  drm/dp: Factor out helper to distinguish between branch and sink
    devices
  drm/i915/dp: Remove debug dependency of DPCD SW/HW revision read
  drm/i915/dp: Print only sink or branch specific OUI based on dev type
  drm/i915/dp: Print full branch/sink descriptor
  drm/i915/lspcon: Fail LSPCON probe if the start of DPCD can't be read
  drm/i915/dp: Read DP descriptor for eDP and LSPCON too
  drm/i915/lspcon: Get DDC adapter via container_of() instead of cached
    ptr
  drm/i915/lspcon: Add workaround for resuming in PCON mode

 drivers/gpu/drm/i915/intel_dp.c     | 78 ++++++++++++-------------------------
 drivers/gpu/drm/i915/intel_drv.h    | 17 +++++++-
 drivers/gpu/drm/i915/intel_lspcon.c | 57 +++++++++++++++++++++++++--
 include/drm/drm_dp_helper.h         |  6 +++
 4 files changed, 100 insertions(+), 58 deletions(-)

-- 
2.5.0

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

[PATCH v2 1/8] drm/dp: Factor out helper to distinguish between branch and sink devices

[Imre Deak]

This check is open-coded in a few places, so it makes sense to simplify
things by having a helper for it similar to the rest of DPCD feature
helpers.

v2: (Jani)
- Move the helper to drm_dp_helper.h.
- Split out this change to a separate patch.

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 11 ++++-------
 include/drm/drm_dp_helper.h     |  6 ++++++
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f30db8f..3c2293b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1459,8 +1459,7 @@ static void intel_dp_print_hw_revision(struct intel_dp *intel_dp)
 	if ((drm_debug & DRM_UT_KMS) == 0)
 		return;
 
-	if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
-	      DP_DWN_STRM_PORT_PRESENT))
+	if (!drm_dp_is_branch(intel_dp->dpcd))
 		return;
 
 	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_HW_REV, &rev, 1);
@@ -1478,8 +1477,7 @@ static void intel_dp_print_sw_revision(struct intel_dp *intel_dp)
 	if ((drm_debug & DRM_UT_KMS) == 0)
 		return;
 
-	if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
-	      DP_DWN_STRM_PORT_PRESENT))
+	if (!drm_dp_is_branch(intel_dp->dpcd))
 		return;
 
 	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_SW_REV, &rev, 2);
@@ -3615,8 +3613,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	if (!is_edp(intel_dp) && !intel_dp->sink_count)
 		return false;
 
-	if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
-	      DP_DWN_STRM_PORT_PRESENT))
+	if (!drm_dp_is_branch(intel_dp->dpcd))
 		return true; /* native DP sink */
 
 	if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
@@ -4134,7 +4131,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 		return connector_status_connected;
 
 	/* if there's no downstream port, we're done */
-	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
+	if (!drm_dp_is_branch(dpcd))
 		return connector_status_connected;
 
 	/* If we're HPD-aware, SINK_COUNT changes dynamically */
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 2a79882..55bbeb0 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -690,6 +690,12 @@ drm_dp_tps3_supported(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
 		dpcd[DP_MAX_LANE_COUNT] & DP_TPS3_SUPPORTED;
 }
 
+static inline bool
+drm_dp_is_branch(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
+{
+	return dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT;
+}
+
 /*
  * DisplayPort AUX channel
  */
-- 
2.5.0

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

[PATCH v2 2/8] drm/i915/dp: Remove debug dependency of DPCD SW/HW revision read

[Imre Deak]

Performing DPCD AUX reads based on debug settings may introduce obscure
bugs in other places that depend on the read being done (or being not
done). To reduce the uncertainty perform the reads unconditionally.

Cc: Mika Kahola <mika.kahola@intel.com>
Suggested-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3c2293b..1c1a8c8 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1456,9 +1456,6 @@ static void intel_dp_print_hw_revision(struct intel_dp *intel_dp)
 	uint8_t rev;
 	int len;
 
-	if ((drm_debug & DRM_UT_KMS) == 0)
-		return;
-
 	if (!drm_dp_is_branch(intel_dp->dpcd))
 		return;
 
@@ -1474,9 +1471,6 @@ static void intel_dp_print_sw_revision(struct intel_dp *intel_dp)
 	uint8_t rev[2];
 	int len;
 
-	if ((drm_debug & DRM_UT_KMS) == 0)
-		return;
-
 	if (!drm_dp_is_branch(intel_dp->dpcd))
 		return;
 
-- 
2.5.0

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

[PATCH v2 3/8] drm/i915/dp: Print only sink or branch specific OUI based on dev type

[Imre Deak]

There are two separate sets of DPCD registers for the DP OUI - as well as
for the device ID and HW/SW revision - based on whether the given DP
device is a branch or a sink. Currently we print both branch and sink
OUIs, for consistency print only the one that corresponds to the
probed device.

v2:
- Split out this change into a separate patch. (Jani)

Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1c1a8c8..1991250 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3624,17 +3624,17 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 static void
 intel_dp_probe_oui(struct intel_dp *intel_dp)
 {
+	bool is_branch = drm_dp_is_branch(intel_dp->dpcd);
 	u8 buf[3];
 
 	if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
 		return;
 
-	if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
-		DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
-			      buf[0], buf[1], buf[2]);
-
-	if (drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
-		DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
+	if (drm_dp_dpcd_read(&intel_dp->aux,
+			     is_branch ? DP_BRANCH_OUI : DP_SINK_OUI,
+			     buf, 3) == 3)
+		DRM_DEBUG_KMS("%s OUI: %02hx%02hx%02hx\n",
+			      is_branch ? "Branch" : "Sink",
 			      buf[0], buf[1], buf[2]);
 }
 
-- 
2.5.0

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

[PATCH v2 4/8] drm/i915/dp: Print full branch/sink descriptor

[Imre Deak]

Extend the branch/sink descriptor info with the missing device ID
field. While at it also read out all the descriptor registers in one
transfer and make the debug print more compact.

v2: (Jani)
- Cache the descriptor in intel_dp.
- Split out this change into a separate patch.

Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 61 +++++++++++++---------------------------
 drivers/gpu/drm/i915/intel_drv.h | 10 +++++++
 2 files changed, 30 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1991250..726fdf2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1451,34 +1451,33 @@ static void intel_dp_print_rates(struct intel_dp *intel_dp)
 	DRM_DEBUG_KMS("common rates: %s\n", str);
 }
 
-static void intel_dp_print_hw_revision(struct intel_dp *intel_dp)
+static bool
+__intel_dp_read_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc)
 {
-	uint8_t rev;
-	int len;
+	u32 base = drm_dp_is_branch(intel_dp->dpcd) ? DP_BRANCH_OUI :
+						      DP_SINK_OUI;
 
-	if (!drm_dp_is_branch(intel_dp->dpcd))
-		return;
-
-	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_HW_REV, &rev, 1);
-	if (len < 0)
-		return;
-
-	DRM_DEBUG_KMS("sink hw revision: %d.%d\n", (rev & 0xf0) >> 4, rev & 0xf);
+	return drm_dp_dpcd_read(&intel_dp->aux, base, desc, sizeof(*desc)) ==
+	       sizeof(*desc);
 }
 
-static void intel_dp_print_sw_revision(struct intel_dp *intel_dp)
+static bool intel_dp_read_desc(struct intel_dp *intel_dp)
 {
-	uint8_t rev[2];
-	int len;
+	struct intel_dp_desc *desc = &intel_dp->desc;
+	bool oui_sup = intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] &
+		       DP_OUI_SUPPORT;
 
-	if (!drm_dp_is_branch(intel_dp->dpcd))
-		return;
+	if (__intel_dp_read_desc(intel_dp, desc) < 0)
+		return false;
 
-	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_SW_REV, &rev, 2);
-	if (len < 0)
-		return;
+	DRM_DEBUG_KMS("DP %s: OUI %*phD%s dev-ID %.*s HW-rev %d.%d SW-rev %d.%d\n",
+		      drm_dp_is_branch(intel_dp->dpcd) ? "branch" : "sink",
+		      (int)sizeof(desc->oui), desc->oui, oui_sup ? "" : "(NS)",
+		      (int)sizeof(desc->device_id), desc->device_id,
+		      desc->hw_rev >> 4, desc->hw_rev & 0xf,
+		      desc->sw_major_rev, desc->sw_minor_rev);
 
-	DRM_DEBUG_KMS("sink sw revision: %d.%d\n", rev[0], rev[1]);
+	return true;
 }
 
 static int rate_to_index(int find, const int *rates)
@@ -3621,23 +3620,6 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	return true;
 }
 
-static void
-intel_dp_probe_oui(struct intel_dp *intel_dp)
-{
-	bool is_branch = drm_dp_is_branch(intel_dp->dpcd);
-	u8 buf[3];
-
-	if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
-		return;
-
-	if (drm_dp_dpcd_read(&intel_dp->aux,
-			     is_branch ? DP_BRANCH_OUI : DP_SINK_OUI,
-			     buf, 3) == 3)
-		DRM_DEBUG_KMS("%s OUI: %02hx%02hx%02hx\n",
-			      is_branch ? "Branch" : "Sink",
-			      buf[0], buf[1], buf[2]);
-}
-
 static bool
 intel_dp_can_mst(struct intel_dp *intel_dp)
 {
@@ -4416,10 +4398,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 
 	intel_dp_print_rates(intel_dp);
 
-	intel_dp_probe_oui(intel_dp);
-
-	intel_dp_print_hw_revision(intel_dp);
-	intel_dp_print_sw_revision(intel_dp);
+	intel_dp_read_desc(intel_dp);
 
 	intel_dp_configure_mst(intel_dp);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 16b33f5..4c9f953 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -883,6 +883,14 @@ enum link_m_n_set {
 	M2_N2
 };
 
+struct intel_dp_desc {
+	u8 oui[3];
+	u8 device_id[6];
+	u8 hw_rev;
+	u8 sw_major_rev;
+	u8 sw_minor_rev;
+} __packed;
+
 struct intel_dp {
 	i915_reg_t output_reg;
 	i915_reg_t aux_ch_ctl_reg;
@@ -905,6 +913,8 @@ struct intel_dp {
 	/* sink rates as reported by DP_SUPPORTED_LINK_RATES */
 	uint8_t num_sink_rates;
 	int sink_rates[DP_MAX_SUPPORTED_RATES];
+	/* sink or branch descriptor */
+	struct intel_dp_desc desc;
 	struct drm_dp_aux aux;
 	uint8_t train_set[4];
 	int panel_power_up_delay;
-- 
2.5.0

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

[PATCH v2 5/8] drm/i915/lspcon: Fail LSPCON probe if the start of DPCD can't be read

[Imre Deak]

All types of DP devices (eDP, DP sink, DP branch) will fail their probe
if the start of DPCD can't be read. The LSPCON PCON functionality also
depends on accessing this area, so fail the probe if the read fails.

Cc: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c     | 2 +-
 drivers/gpu/drm/i915/intel_drv.h    | 2 ++
 drivers/gpu/drm/i915/intel_lspcon.c | 5 +++++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 726fdf2..62c5512 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3495,7 +3495,7 @@ intel_dp_link_down(struct intel_dp *intel_dp)
 	intel_dp->DP = DP;
 }
 
-static bool
+bool
 intel_dp_read_dpcd(struct intel_dp *intel_dp)
 {
 	if (drm_dp_dpcd_read(&intel_dp->aux, 0x000, intel_dp->dpcd,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4c9f953..ff9d2dc 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1461,6 +1461,8 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
 	return ~((1 << lane_count) - 1) & 0xf;
 }
 
+bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
+
 /* intel_dp_aux_backlight.c */
 int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
 
diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index 632149c..23b817a 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -131,6 +131,11 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
 		}
 	}
 
+	if (!intel_dp_read_dpcd(dp)) {
+		DRM_ERROR("LSPCON DPCD read failed\n");
+		return false;
+	}
+
 	DRM_DEBUG_KMS("Success: LSPCON init\n");
 	return true;
 }
-- 
2.5.0

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

[PATCH v2 6/8] drm/i915/dp: Read DP descriptor for eDP and LSPCON too

[Imre Deak]

As for external DP sink and branch devices read and print the DP
descriptor for eDP and LSPCON devices as well to aid debugging.

v2:
- Split out this change to a separate patch. (Jani)

Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c     | 4 +++-
 drivers/gpu/drm/i915/intel_drv.h    | 1 +
 drivers/gpu/drm/i915/intel_lspcon.c | 2 ++
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 62c5512..043993f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1461,7 +1461,7 @@ __intel_dp_read_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc)
 	       sizeof(*desc);
 }
 
-static bool intel_dp_read_desc(struct intel_dp *intel_dp)
+bool intel_dp_read_desc(struct intel_dp *intel_dp)
 {
 	struct intel_dp_desc *desc = &intel_dp->desc;
 	bool oui_sup = intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] &
@@ -3519,6 +3519,8 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
 	if (!intel_dp_read_dpcd(intel_dp))
 		return false;
 
+	intel_dp_read_desc(intel_dp);
+
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11)
 		dev_priv->no_aux_handshake = intel_dp->dpcd[DP_MAX_DOWNSPREAD] &
 			DP_NO_AUX_HANDSHAKE_LINK_TRAINING;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ff9d2dc..a79cbad 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1462,6 +1462,7 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
 }
 
 bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
+bool intel_dp_read_desc(struct intel_dp *intel_dp);
 
 /* intel_dp_aux_backlight.c */
 int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index 23b817a..c5f278b 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -136,6 +136,8 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
 		return false;
 	}
 
+	intel_dp_read_desc(dp);
+
 	DRM_DEBUG_KMS("Success: LSPCON init\n");
 	return true;
 }
-- 
2.5.0

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

[PATCH v2 7/8] drm/i915/lspcon: Get DDC adapter via container_of() instead of cached ptr

[Imre Deak]

We can use the container_of() magic to get to the DDC adapter, so no
need for caching a pointer to it. We'll also need to get at the intel_dp
ptr in the following patch, so add a helper that can be used for both
purposes.

Cc: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h    |  1 -
 drivers/gpu/drm/i915/intel_lspcon.c | 15 +++++++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a79cbad..45f55b5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -974,7 +974,6 @@ struct intel_dp {
 struct intel_lspcon {
 	bool active;
 	enum drm_lspcon_mode mode;
-	struct drm_dp_aux *aux;
 };
 
 struct intel_digital_port {
diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index c5f278b..3dc5a0b 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -27,10 +27,18 @@
 #include <drm/drm_dp_dual_mode_helper.h>
 #include "intel_drv.h"
 
+static struct intel_dp *lspcon_to_intel_dp(struct intel_lspcon *lspcon)
+{
+	struct intel_digital_port *dig_port =
+		container_of(lspcon, struct intel_digital_port, lspcon);
+
+	return &dig_port->dp;
+}
+
 static enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon)
 {
 	enum drm_lspcon_mode current_mode = DRM_LSPCON_MODE_INVALID;
-	struct i2c_adapter *adapter = &lspcon->aux->ddc;
+	struct i2c_adapter *adapter = &lspcon_to_intel_dp(lspcon)->aux.ddc;
 
 	if (drm_lspcon_get_mode(adapter, &current_mode))
 		DRM_ERROR("Error reading LSPCON mode\n");
@@ -45,7 +53,7 @@ static int lspcon_change_mode(struct intel_lspcon *lspcon,
 {
 	int err;
 	enum drm_lspcon_mode current_mode;
-	struct i2c_adapter *adapter = &lspcon->aux->ddc;
+	struct i2c_adapter *adapter = &lspcon_to_intel_dp(lspcon)->aux.ddc;
 
 	err = drm_lspcon_get_mode(adapter, &current_mode);
 	if (err) {
@@ -72,7 +80,7 @@ static int lspcon_change_mode(struct intel_lspcon *lspcon,
 static bool lspcon_probe(struct intel_lspcon *lspcon)
 {
 	enum drm_dp_dual_mode_type adaptor_type;
-	struct i2c_adapter *adapter = &lspcon->aux->ddc;
+	struct i2c_adapter *adapter = &lspcon_to_intel_dp(lspcon)->aux.ddc;
 
 	/* Lets probe the adaptor and check its type */
 	adaptor_type = drm_dp_dual_mode_detect(adapter);
@@ -111,7 +119,6 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
 
 	lspcon->active = false;
 	lspcon->mode = DRM_LSPCON_MODE_INVALID;
-	lspcon->aux = &dp->aux;
 
 	if (!lspcon_probe(lspcon)) {
 		DRM_ERROR("Failed to probe lspcon\n");
-- 
2.5.0

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

[PATCH v2 8/8] drm/i915/lspcon: Add workaround for resuming in PCON mode

[Imre Deak]

On my APL the LSPCON firmware resumes in PCON mode as opposed to the
expected LS mode. It also appears to be in a state where AUX DPCD reads
will succeed but return garbage recovering only after a few hundreds of
milliseconds. After the recovery time DPCD reads will result in the
correct values and things will continue to work. If I2C over AUX is
attempted during this recovery time (implying an AUX write transaction)
the firmware won't recover and will stay in this broken state.

As a workaround check if the firmware is in PCON state after resume and
if so wait until the correct DPCD values are returned. For this we
compare the branch descriptor with the one we cached during init time.
If the firmware was in the LS state, we skip the w/a and continue as
before.

v2:
- Use the DP descriptor value cached in intel_dp. (Jani)
- Get to intel_dp using container_of(), instead of a cached ptr.
  (Shashank)
- Use usleep_range() instead of msleep().

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98353
Cc: Shashank Sharma <shashank.sharma@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c     |  2 +-
 drivers/gpu/drm/i915/intel_drv.h    |  3 +++
 drivers/gpu/drm/i915/intel_lspcon.c | 37 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 043993f..e9be955 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1451,7 +1451,7 @@ static void intel_dp_print_rates(struct intel_dp *intel_dp)
 	DRM_DEBUG_KMS("common rates: %s\n", str);
 }
 
-static bool
+bool
 __intel_dp_read_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc)
 {
 	u32 base = drm_dp_is_branch(intel_dp->dpcd) ? DP_BRANCH_OUI :
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 45f55b5..f2b6c59 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -974,6 +974,7 @@ struct intel_dp {
 struct intel_lspcon {
 	bool active;
 	enum drm_lspcon_mode mode;
+	bool desc_valid;
 };
 
 struct intel_digital_port {
@@ -1461,6 +1462,8 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
 }
 
 bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
+bool __intel_dp_read_desc(struct intel_dp *intel_dp,
+			  struct intel_dp_desc *desc);
 bool intel_dp_read_desc(struct intel_dp *intel_dp);
 
 /* intel_dp_aux_backlight.c */
diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index 3dc5a0b..daa5234 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -97,8 +97,43 @@ static bool lspcon_probe(struct intel_lspcon *lspcon)
 	return true;
 }
 
+static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon)
+{
+	struct intel_dp *intel_dp = lspcon_to_intel_dp(lspcon);
+	unsigned long start = jiffies;
+
+	if (!lspcon->desc_valid)
+		return;
+
+	while (1) {
+		struct intel_dp_desc desc;
+
+		/*
+		 * The w/a only applies in PCON mode and we don't expect any
+		 * AUX errors.
+		 */
+		if (!__intel_dp_read_desc(intel_dp, &desc))
+			return;
+
+		if (!memcmp(&intel_dp->desc, &desc, sizeof(desc))) {
+			DRM_DEBUG_KMS("LSPCON recovering in PCON mode after %u ms\n",
+				      jiffies_to_msecs(jiffies - start));
+			return;
+		}
+
+		if (time_after(jiffies, start + msecs_to_jiffies(1000)))
+			break;
+
+		usleep_range(10000, 15000);
+	}
+
+	DRM_DEBUG_KMS("LSPCON DP descriptor mismatch after resume\n");
+}
+
 void lspcon_resume(struct intel_lspcon *lspcon)
 {
+	lspcon_resume_in_pcon_wa(lspcon);
+
 	if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON, true))
 		DRM_ERROR("LSPCON resume failed\n");
 	else
@@ -143,7 +178,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
 		return false;
 	}
 
-	intel_dp_read_desc(dp);
+	lspcon->desc_valid = intel_dp_read_desc(dp);
 
 	DRM_DEBUG_KMS("Success: LSPCON init\n");
 	return true;
-- 
2.5.0

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

Re: [PATCH v2 1/8] drm/dp: Factor out helper to distinguish between branch and sink devices

[Jani Nikula]

On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> This check is open-coded in a few places, so it makes sense to simplify
> things by having a helper for it similar to the rest of DPCD feature
> helpers.
>
> v2: (Jani)
> - Move the helper to drm_dp_helper.h.
> - Split out this change to a separate patch.
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Imre Deak <imre.deak@intel.com>

DP 1.4 has changed the name of "branch" devices, but I think this is
good.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> ---
>  drivers/gpu/drm/i915/intel_dp.c | 11 ++++-------
>  include/drm/drm_dp_helper.h     |  6 ++++++
>  2 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f30db8f..3c2293b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1459,8 +1459,7 @@ static void intel_dp_print_hw_revision(struct intel_dp *intel_dp)
>  	if ((drm_debug & DRM_UT_KMS) == 0)
>  		return;
>  
> -	if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> -	      DP_DWN_STRM_PORT_PRESENT))
> +	if (!drm_dp_is_branch(intel_dp->dpcd))
>  		return;
>  
>  	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_HW_REV, &rev, 1);
> @@ -1478,8 +1477,7 @@ static void intel_dp_print_sw_revision(struct intel_dp *intel_dp)
>  	if ((drm_debug & DRM_UT_KMS) == 0)
>  		return;
>  
> -	if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> -	      DP_DWN_STRM_PORT_PRESENT))
> +	if (!drm_dp_is_branch(intel_dp->dpcd))
>  		return;
>  
>  	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_SW_REV, &rev, 2);
> @@ -3615,8 +3613,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	if (!is_edp(intel_dp) && !intel_dp->sink_count)
>  		return false;
>  
> -	if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> -	      DP_DWN_STRM_PORT_PRESENT))
> +	if (!drm_dp_is_branch(intel_dp->dpcd))
>  		return true; /* native DP sink */
>  
>  	if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
> @@ -4134,7 +4131,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>  		return connector_status_connected;
>  
>  	/* if there's no downstream port, we're done */
> -	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
> +	if (!drm_dp_is_branch(dpcd))
>  		return connector_status_connected;
>  
>  	/* If we're HPD-aware, SINK_COUNT changes dynamically */
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 2a79882..55bbeb0 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -690,6 +690,12 @@ drm_dp_tps3_supported(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
>  		dpcd[DP_MAX_LANE_COUNT] & DP_TPS3_SUPPORTED;
>  }
>  
> +static inline bool
> +drm_dp_is_branch(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> +{
> +	return dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT;
> +}
> +
>  /*
>   * DisplayPort AUX channel
>   */

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/8] drm/i915/dp: Remove debug dependency of DPCD SW/HW revision read

[Jani Nikula]

On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> Performing DPCD AUX reads based on debug settings may introduce obscure
> bugs in other places that depend on the read being done (or being not
> done). To reduce the uncertainty perform the reads unconditionally.
>
> Cc: Mika Kahola <mika.kahola@intel.com>
> Suggested-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 3c2293b..1c1a8c8 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1456,9 +1456,6 @@ static void intel_dp_print_hw_revision(struct intel_dp *intel_dp)
>  	uint8_t rev;
>  	int len;
>  
> -	if ((drm_debug & DRM_UT_KMS) == 0)
> -		return;
> -
>  	if (!drm_dp_is_branch(intel_dp->dpcd))
>  		return;
>  
> @@ -1474,9 +1471,6 @@ static void intel_dp_print_sw_revision(struct intel_dp *intel_dp)
>  	uint8_t rev[2];
>  	int len;
>  
> -	if ((drm_debug & DRM_UT_KMS) == 0)
> -		return;
> -
>  	if (!drm_dp_is_branch(intel_dp->dpcd))
>  		return;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/8] drm/i915/dp: Print only sink or branch specific OUI based on dev type

[Jani Nikula]

On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> There are two separate sets of DPCD registers for the DP OUI - as well as
> for the device ID and HW/SW revision - based on whether the given DP
> device is a branch or a sink. Currently we print both branch and sink
> OUIs, for consistency print only the one that corresponds to the
> probed device.
>
> v2:
> - Split out this change into a separate patch. (Jani)
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> ---
>  drivers/gpu/drm/i915/intel_dp.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 1c1a8c8..1991250 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3624,17 +3624,17 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  static void
>  intel_dp_probe_oui(struct intel_dp *intel_dp)
>  {
> +	bool is_branch = drm_dp_is_branch(intel_dp->dpcd);
>  	u8 buf[3];
>  
>  	if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
>  		return;
>  
> -	if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
> -		DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
> -			      buf[0], buf[1], buf[2]);
> -
> -	if (drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
> -		DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
> +	if (drm_dp_dpcd_read(&intel_dp->aux,
> +			     is_branch ? DP_BRANCH_OUI : DP_SINK_OUI,
> +			     buf, 3) == 3)
> +		DRM_DEBUG_KMS("%s OUI: %02hx%02hx%02hx\n",
> +			      is_branch ? "Branch" : "Sink",
>  			      buf[0], buf[1], buf[2]);
>  }

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for drm/i915/lspcon: Work around resume failure

[Patchwork]

== Series Details ==

Series: drm/i915/lspcon: Work around resume failure
URL   : https://patchwork.freedesktop.org/series/14280/
State : warning

== Summary ==

Series 14280v1 drm/i915/lspcon: Work around resume failure
https://patchwork.freedesktop.org/api/1.0/series/14280/revisions/1/mbox/

Test drv_module_reload_basic:
                pass       -> DMESG-WARN (fi-skl-6700hq)
Test gem_exec_suspend:
        Subgroup basic-s3:
                dmesg-warn -> PASS       (fi-skl-6700hq)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                dmesg-warn -> PASS       (fi-skl-6700hq)
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-skl-6700hq)
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> PASS       (fi-skl-6700hq)

fi-bdw-5557u     total:246  pass:231  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:246  pass:204  dwarn:0   dfail:0   fail:0   skip:42 
fi-bxt-t5700     total:246  pass:216  dwarn:0   dfail:0   fail:0   skip:30 
fi-byt-j1900     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-byt-n2820     total:246  pass:211  dwarn:0   dfail:0   fail:0   skip:35 
fi-hsw-4770      total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-hsw-4770r     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-ilk-650       total:246  pass:185  dwarn:0   dfail:0   fail:1   skip:60 
fi-ivb-3520m     total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25 
fi-ivb-3770      total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25 
fi-skl-6260u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:246  pass:222  dwarn:1   dfail:0   fail:0   skip:23 
fi-skl-6700k     total:246  pass:222  dwarn:1   dfail:0   fail:0   skip:23 
fi-skl-6770hq    total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:246  pass:210  dwarn:0   dfail:0   fail:0   skip:36 
fi-snb-2600      total:246  pass:209  dwarn:0   dfail:0   fail:0   skip:37 

Results at /Patchwork_2801/

102441087e47f4892b88c4f6e308e3edf89eeda1 drm-intel-nightly: 2016y-10m-24d-14h-28m-17s UTC integration manifest
eaa672c drm/i915/lspcon: Add workaround for resuming in PCON mode
eb04604 drm/i915/lspcon: Get DDC adapter via container_of() instead of cached ptr
0ca993d drm/i915/dp: Read DP descriptor for eDP and LSPCON too
0144d102 drm/i915/lspcon: Fail LSPCON probe if the start of DPCD can't be read
7568e07 drm/i915/dp: Print full branch/sink descriptor
3b4f6e9 drm/i915/dp: Print only sink or branch specific OUI based on dev type
b1439bc drm/i915/dp: Remove debug dependency of DPCD SW/HW revision read
2beca1a drm/dp: Factor out helper to distinguish between branch and sink devices

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

Re: ✗ Fi.CI.BAT: warning for drm/i915/lspcon: Work around resume failure

[Imre Deak]

On ma, 2016-10-24 at 17:16 +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915/lspcon: Work around resume failure
> URL   : https://patchwork.freedesktop.org/series/14280/
> State : warning
> 
> == Summary ==
> 
> Series 14280v1 drm/i915/lspcon: Work around resume failure
> https://patchwork.freedesktop.org/api/1.0/series/14280/revisions/1/mb
> ox/
> 
> Test drv_module_reload_basic:
>                 pass       -> DMESG-WARN (fi-skl-6700hq)

This one is a result of the previous CI run where LSPCON failed after
S3. After a warm-reboot LSPCON is still in a broken state, and even the
initial LSPCON probe will fail.

A power cycle between CI runs would prevent the above.

> Test gem_exec_suspend:
>         Subgroup basic-s3:
>                 dmesg-warn -> PASS       (fi-skl-6700hq)
> Test kms_pipe_crc_basic:
>         Subgroup suspend-read-crc-pipe-a:
>                 dmesg-warn -> PASS       (fi-skl-6700hq)
>         Subgroup suspend-read-crc-pipe-b:
>                 dmesg-warn -> PASS       (fi-skl-6700hq)
>         Subgroup suspend-read-crc-pipe-c:
>                 dmesg-warn -> PASS       (fi-skl-6700hq)
> 
> fi-bdw-
> 5557u     total:246  pass:231  dwarn:0   dfail:0   fail:0   skip:15 
> fi-bsw-
> n3050     total:246  pass:204  dwarn:0   dfail:0   fail:0   skip:42 
> fi-bxt-
> t5700     total:246  pass:216  dwarn:0   dfail:0   fail:0   skip:30 
> fi-byt-
> j1900     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
> fi-byt-
> n2820     total:246  pass:211  dwarn:0   dfail:0   fail:0   skip:35 
> fi-hsw-
> 4770      total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
> fi-hsw-
> 4770r     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
> fi-ilk-
> 650       total:246  pass:185  dwarn:0   dfail:0   fail:1   skip:60 
> fi-ivb-
> 3520m     total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25 
> fi-ivb-
> 3770      total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25 
> fi-skl-
> 6260u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
> fi-skl-
> 6700hq    total:246  pass:222  dwarn:1   dfail:0   fail:0   skip:23 
> fi-skl-
> 6700k     total:246  pass:222  dwarn:1   dfail:0   fail:0   skip:23 
> fi-skl-
> 6770hq    total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
> fi-snb-
> 2520m     total:246  pass:210  dwarn:0   dfail:0   fail:0   skip:36 
> fi-snb-
> 2600      total:246  pass:209  dwarn:0   dfail:0   fail:0   skip:37 
> 
> Results at /Patchwork_2801/
> 
> 102441087e47f4892b88c4f6e308e3edf89eeda1 drm-intel-nightly: 2016y-
> 10m-24d-14h-28m-17s UTC integration manifest
> eaa672c drm/i915/lspcon: Add workaround for resuming in PCON mode
> eb04604 drm/i915/lspcon: Get DDC adapter via container_of() instead
> of cached ptr
> 0ca993d drm/i915/dp: Read DP descriptor for eDP and LSPCON too
> 0144d102 drm/i915/lspcon: Fail LSPCON probe if the start of DPCD
> can't be read
> 7568e07 drm/i915/dp: Print full branch/sink descriptor
> 3b4f6e9 drm/i915/dp: Print only sink or branch specific OUI based on
> dev type
> b1439bc drm/i915/dp: Remove debug dependency of DPCD SW/HW revision
> read
> 2beca1a drm/dp: Factor out helper to distinguish between branch and
> sink devices
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/8] drm/i915/dp: Print full branch/sink descriptor

[Jani Nikula]

On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> Extend the branch/sink descriptor info with the missing device ID
> field. While at it also read out all the descriptor registers in one
> transfer and make the debug print more compact.
>
> v2: (Jani)
> - Cache the descriptor in intel_dp.
> - Split out this change into a separate patch.
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 61 +++++++++++++---------------------------
>  drivers/gpu/drm/i915/intel_drv.h | 10 +++++++
>  2 files changed, 30 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 1991250..726fdf2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1451,34 +1451,33 @@ static void intel_dp_print_rates(struct intel_dp *intel_dp)
>  	DRM_DEBUG_KMS("common rates: %s\n", str);
>  }
>  
> -static void intel_dp_print_hw_revision(struct intel_dp *intel_dp)
> +static bool
> +__intel_dp_read_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc)
>  {
> -	uint8_t rev;
> -	int len;
> +	u32 base = drm_dp_is_branch(intel_dp->dpcd) ? DP_BRANCH_OUI :
> +						      DP_SINK_OUI;
>  
> -	if (!drm_dp_is_branch(intel_dp->dpcd))
> -		return;
> -
> -	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_HW_REV, &rev, 1);
> -	if (len < 0)
> -		return;
> -
> -	DRM_DEBUG_KMS("sink hw revision: %d.%d\n", (rev & 0xf0) >> 4, rev & 0xf);
> +	return drm_dp_dpcd_read(&intel_dp->aux, base, desc, sizeof(*desc)) ==
> +	       sizeof(*desc);
>  }
>  
> -static void intel_dp_print_sw_revision(struct intel_dp *intel_dp)
> +static bool intel_dp_read_desc(struct intel_dp *intel_dp)
>  {
> -	uint8_t rev[2];
> -	int len;
> +	struct intel_dp_desc *desc = &intel_dp->desc;
> +	bool oui_sup = intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] &
> +		       DP_OUI_SUPPORT;
>  
> -	if (!drm_dp_is_branch(intel_dp->dpcd))
> -		return;
> +	if (__intel_dp_read_desc(intel_dp, desc) < 0)

The bool returned from __intel_dp_read_desc will never be less than 0...

There's no point in reading the desc if oui_sup is false, is there? All
of desc should read all zeros in that case, not just OUI.

I guess the desc should be reset to zeros at detect or somewhere. I'm
sure we have other stuff we fail to reset too.

> +		return false;
>  
> -	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_SW_REV, &rev, 2);
> -	if (len < 0)
> -		return;
> +	DRM_DEBUG_KMS("DP %s: OUI %*phD%s dev-ID %.*s HW-rev %d.%d SW-rev %d.%d\n",

Maybe some form of %*pE would be more defensive for device id? See
Documentation/printk-formats.txt.

> +		      drm_dp_is_branch(intel_dp->dpcd) ? "branch" : "sink",
> +		      (int)sizeof(desc->oui), desc->oui, oui_sup ? "" : "(NS)",
> +		      (int)sizeof(desc->device_id), desc->device_id,
> +		      desc->hw_rev >> 4, desc->hw_rev & 0xf,
> +		      desc->sw_major_rev, desc->sw_minor_rev);
>  
> -	DRM_DEBUG_KMS("sink sw revision: %d.%d\n", rev[0], rev[1]);
> +	return true;
>  }
>  
>  static int rate_to_index(int find, const int *rates)
> @@ -3621,23 +3620,6 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	return true;
>  }
>  
> -static void
> -intel_dp_probe_oui(struct intel_dp *intel_dp)
> -{
> -	bool is_branch = drm_dp_is_branch(intel_dp->dpcd);
> -	u8 buf[3];
> -
> -	if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
> -		return;
> -
> -	if (drm_dp_dpcd_read(&intel_dp->aux,
> -			     is_branch ? DP_BRANCH_OUI : DP_SINK_OUI,
> -			     buf, 3) == 3)
> -		DRM_DEBUG_KMS("%s OUI: %02hx%02hx%02hx\n",
> -			      is_branch ? "Branch" : "Sink",
> -			      buf[0], buf[1], buf[2]);
> -}
> -
>  static bool
>  intel_dp_can_mst(struct intel_dp *intel_dp)
>  {
> @@ -4416,10 +4398,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>  
>  	intel_dp_print_rates(intel_dp);
>  
> -	intel_dp_probe_oui(intel_dp);
> -
> -	intel_dp_print_hw_revision(intel_dp);
> -	intel_dp_print_sw_revision(intel_dp);
> +	intel_dp_read_desc(intel_dp);
>  
>  	intel_dp_configure_mst(intel_dp);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 16b33f5..4c9f953 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -883,6 +883,14 @@ enum link_m_n_set {
>  	M2_N2
>  };
>  
> +struct intel_dp_desc {
> +	u8 oui[3];
> +	u8 device_id[6];
> +	u8 hw_rev;
> +	u8 sw_major_rev;
> +	u8 sw_minor_rev;
> +} __packed;
> +
>  struct intel_dp {
>  	i915_reg_t output_reg;
>  	i915_reg_t aux_ch_ctl_reg;
> @@ -905,6 +913,8 @@ struct intel_dp {
>  	/* sink rates as reported by DP_SUPPORTED_LINK_RATES */
>  	uint8_t num_sink_rates;
>  	int sink_rates[DP_MAX_SUPPORTED_RATES];
> +	/* sink or branch descriptor */
> +	struct intel_dp_desc desc;
>  	struct drm_dp_aux aux;
>  	uint8_t train_set[4];
>  	int panel_power_up_delay;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 5/8] drm/i915/lspcon: Fail LSPCON probe if the start of DPCD can't be read

[Jani Nikula]

On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> All types of DP devices (eDP, DP sink, DP branch) will fail their probe
> if the start of DPCD can't be read. The LSPCON PCON functionality also
> depends on accessing this area, so fail the probe if the read fails.
>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> ---
>  drivers/gpu/drm/i915/intel_dp.c     | 2 +-
>  drivers/gpu/drm/i915/intel_drv.h    | 2 ++
>  drivers/gpu/drm/i915/intel_lspcon.c | 5 +++++
>  3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 726fdf2..62c5512 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3495,7 +3495,7 @@ intel_dp_link_down(struct intel_dp *intel_dp)
>  	intel_dp->DP = DP;
>  }
>  
> -static bool
> +bool
>  intel_dp_read_dpcd(struct intel_dp *intel_dp)
>  {
>  	if (drm_dp_dpcd_read(&intel_dp->aux, 0x000, intel_dp->dpcd,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4c9f953..ff9d2dc 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1461,6 +1461,8 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
>  	return ~((1 << lane_count) - 1) & 0xf;
>  }
>  
> +bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
> +
>  /* intel_dp_aux_backlight.c */
>  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
>  
> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> index 632149c..23b817a 100644
> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> @@ -131,6 +131,11 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>  		}
>  	}
>  
> +	if (!intel_dp_read_dpcd(dp)) {
> +		DRM_ERROR("LSPCON DPCD read failed\n");
> +		return false;
> +	}
> +
>  	DRM_DEBUG_KMS("Success: LSPCON init\n");
>  	return true;
>  }

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 6/8] drm/i915/dp: Read DP descriptor for eDP and LSPCON too

[Jani Nikula]

On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> As for external DP sink and branch devices read and print the DP
> descriptor for eDP and LSPCON devices as well to aid debugging.
>
> v2:
> - Split out this change to a separate patch. (Jani)
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> ---
>  drivers/gpu/drm/i915/intel_dp.c     | 4 +++-
>  drivers/gpu/drm/i915/intel_drv.h    | 1 +
>  drivers/gpu/drm/i915/intel_lspcon.c | 2 ++
>  3 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 62c5512..043993f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1461,7 +1461,7 @@ __intel_dp_read_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc)
>  	       sizeof(*desc);
>  }
>  
> -static bool intel_dp_read_desc(struct intel_dp *intel_dp)
> +bool intel_dp_read_desc(struct intel_dp *intel_dp)
>  {
>  	struct intel_dp_desc *desc = &intel_dp->desc;
>  	bool oui_sup = intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] &
> @@ -3519,6 +3519,8 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
>  	if (!intel_dp_read_dpcd(intel_dp))
>  		return false;
>  
> +	intel_dp_read_desc(intel_dp);
> +
>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11)
>  		dev_priv->no_aux_handshake = intel_dp->dpcd[DP_MAX_DOWNSPREAD] &
>  			DP_NO_AUX_HANDSHAKE_LINK_TRAINING;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ff9d2dc..a79cbad 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1462,6 +1462,7 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
>  }
>  
>  bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
> +bool intel_dp_read_desc(struct intel_dp *intel_dp);
>  
>  /* intel_dp_aux_backlight.c */
>  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> index 23b817a..c5f278b 100644
> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> @@ -136,6 +136,8 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>  		return false;
>  	}
>  
> +	intel_dp_read_desc(dp);
> +
>  	DRM_DEBUG_KMS("Success: LSPCON init\n");
>  	return true;
>  }

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 7/8] drm/i915/lspcon: Get DDC adapter via container_of() instead of cached ptr

[Jani Nikula]

On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> We can use the container_of() magic to get to the DDC adapter, so no
> need for caching a pointer to it. We'll also need to get at the intel_dp
> ptr in the following patch, so add a helper that can be used for both
> purposes.
>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

SPOT FTW.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> ---
>  drivers/gpu/drm/i915/intel_drv.h    |  1 -
>  drivers/gpu/drm/i915/intel_lspcon.c | 15 +++++++++++----
>  2 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a79cbad..45f55b5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -974,7 +974,6 @@ struct intel_dp {
>  struct intel_lspcon {
>  	bool active;
>  	enum drm_lspcon_mode mode;
> -	struct drm_dp_aux *aux;
>  };
>  
>  struct intel_digital_port {
> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> index c5f278b..3dc5a0b 100644
> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> @@ -27,10 +27,18 @@
>  #include <drm/drm_dp_dual_mode_helper.h>
>  #include "intel_drv.h"
>  
> +static struct intel_dp *lspcon_to_intel_dp(struct intel_lspcon *lspcon)
> +{
> +	struct intel_digital_port *dig_port =
> +		container_of(lspcon, struct intel_digital_port, lspcon);
> +
> +	return &dig_port->dp;
> +}
> +
>  static enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon)
>  {
>  	enum drm_lspcon_mode current_mode = DRM_LSPCON_MODE_INVALID;
> -	struct i2c_adapter *adapter = &lspcon->aux->ddc;
> +	struct i2c_adapter *adapter = &lspcon_to_intel_dp(lspcon)->aux.ddc;
>  
>  	if (drm_lspcon_get_mode(adapter, &current_mode))
>  		DRM_ERROR("Error reading LSPCON mode\n");
> @@ -45,7 +53,7 @@ static int lspcon_change_mode(struct intel_lspcon *lspcon,
>  {
>  	int err;
>  	enum drm_lspcon_mode current_mode;
> -	struct i2c_adapter *adapter = &lspcon->aux->ddc;
> +	struct i2c_adapter *adapter = &lspcon_to_intel_dp(lspcon)->aux.ddc;
>  
>  	err = drm_lspcon_get_mode(adapter, &current_mode);
>  	if (err) {
> @@ -72,7 +80,7 @@ static int lspcon_change_mode(struct intel_lspcon *lspcon,
>  static bool lspcon_probe(struct intel_lspcon *lspcon)
>  {
>  	enum drm_dp_dual_mode_type adaptor_type;
> -	struct i2c_adapter *adapter = &lspcon->aux->ddc;
> +	struct i2c_adapter *adapter = &lspcon_to_intel_dp(lspcon)->aux.ddc;
>  
>  	/* Lets probe the adaptor and check its type */
>  	adaptor_type = drm_dp_dual_mode_detect(adapter);
> @@ -111,7 +119,6 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>  
>  	lspcon->active = false;
>  	lspcon->mode = DRM_LSPCON_MODE_INVALID;
> -	lspcon->aux = &dp->aux;
>  
>  	if (!lspcon_probe(lspcon)) {
>  		DRM_ERROR("Failed to probe lspcon\n");

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/8] drm/i915/dp: Print full branch/sink descriptor

[Imre Deak]

On Mon, 2016-10-24 at 21:14 +0300, Jani Nikula wrote:
> On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> > Extend the branch/sink descriptor info with the missing device ID
> > field. While at it also read out all the descriptor registers in one
> > transfer and make the debug print more compact.
> > 
> > v2: (Jani)
> > - Cache the descriptor in intel_dp.
> > - Split out this change into a separate patch.
> > 
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c  | 61 +++++++++++++---------------------------
> >  drivers/gpu/drm/i915/intel_drv.h | 10 +++++++
> >  2 files changed, 30 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 1991250..726fdf2 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1451,34 +1451,33 @@ static void intel_dp_print_rates(struct intel_dp *intel_dp)
> >  	DRM_DEBUG_KMS("common rates: %s\n", str);
> >  }
> >  
> > -static void intel_dp_print_hw_revision(struct intel_dp *intel_dp)
> > +static bool
> > +__intel_dp_read_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc)
> >  {
> > -	uint8_t rev;
> > -	int len;
> > +	u32 base = drm_dp_is_branch(intel_dp->dpcd) ? DP_BRANCH_OUI :
> > +						      DP_SINK_OUI;
> >  
> > -	if (!drm_dp_is_branch(intel_dp->dpcd))
> > -		return;
> > -
> > -	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_HW_REV, &rev, 1);
> > -	if (len < 0)
> > -		return;
> > -
> > -	DRM_DEBUG_KMS("sink hw revision: %d.%d\n", (rev & 0xf0) >> 4, rev & 0xf);
> > +	return drm_dp_dpcd_read(&intel_dp->aux, base, desc, sizeof(*desc)) ==
> > +	       sizeof(*desc);
> >  }
> >  
> > -static void intel_dp_print_sw_revision(struct intel_dp *intel_dp)
> > +static bool intel_dp_read_desc(struct intel_dp *intel_dp)
> >  {
> > -	uint8_t rev[2];
> > -	int len;
> > +	struct intel_dp_desc *desc = &intel_dp->desc;
> > +	bool oui_sup = intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] &
> > +		       DP_OUI_SUPPORT;
> >  
> > -	if (!drm_dp_is_branch(intel_dp->dpcd))
> > -		return;
> > +	if (__intel_dp_read_desc(intel_dp, desc) < 0)
> 
> The bool returned from __intel_dp_read_desc will never be less than 0...

Yep, that's a typo, will fix it.

> There's no point in reading the desc if oui_sup is false, is there? All of desc should read all zeros in that case, not just OUI.

The spec is not too clear about this yes, but as I read it oui_sup
applies only to the OUI reg, device ID and the revisions can be
still valid. The LSPCON FW has oui_sup cleared, but returns valid
device ID and revision values.

> I guess the desc should be reset to zeros at detect or somewhere. I'm
> sure we have other stuff we fail to reset too.

Yes, like the dpcd array. desc will be used only for debug printing atm
where we always re-read it first, and for LSPCON where it's constant,
so if that's fine for you I'd leave this for later.

> 
> > +		return false;
> >  
> > -	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_SW_REV, &rev, 2);
> > -	if (len < 0)
> > -		return;
> > +	DRM_DEBUG_KMS("DP %s: OUI %*phD%s dev-ID %.*s HW-rev %d.%d SW-rev %d.%d\n",
> 
> Maybe some form of %*pE would be more defensive for device id? See
> Documentation/printk-formats.txt.

Yes, can change that to '%*pE'.

> > +		      drm_dp_is_branch(intel_dp->dpcd) ? "branch" : "sink",
> > +		      (int)sizeof(desc->oui), desc->oui, oui_sup ? "" : "(NS)",
> > +		      (int)sizeof(desc->device_id), desc->device_id,
> > +		      desc->hw_rev >> 4, desc->hw_rev & 0xf,
> > +		      desc->sw_major_rev, desc->sw_minor_rev);
> >  
> > -	DRM_DEBUG_KMS("sink sw revision: %d.%d\n", rev[0], rev[1]);
> > +	return true;
> >  }
> >  
> >  static int rate_to_index(int find, const int *rates)
> > @@ -3621,23 +3620,6 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> >  	return true;
> >  }
> >  
> > -static void
> > -intel_dp_probe_oui(struct intel_dp *intel_dp)
> > -{
> > -	bool is_branch = drm_dp_is_branch(intel_dp->dpcd);
> > -	u8 buf[3];
> > -
> > -	if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
> > -		return;
> > -
> > -	if (drm_dp_dpcd_read(&intel_dp->aux,
> > -			     is_branch ? DP_BRANCH_OUI : DP_SINK_OUI,
> > -			     buf, 3) == 3)
> > -		DRM_DEBUG_KMS("%s OUI: %02hx%02hx%02hx\n",
> > -			      is_branch ? "Branch" : "Sink",
> > -			      buf[0], buf[1], buf[2]);
> > -}
> > -
> >  static bool
> >  intel_dp_can_mst(struct intel_dp *intel_dp)
> >  {
> > @@ -4416,10 +4398,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> >  
> >  	intel_dp_print_rates(intel_dp);
> >  
> > -	intel_dp_probe_oui(intel_dp);
> > -
> > -	intel_dp_print_hw_revision(intel_dp);
> > -	intel_dp_print_sw_revision(intel_dp);
> > +	intel_dp_read_desc(intel_dp);
> >  
> >  	intel_dp_configure_mst(intel_dp);
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 16b33f5..4c9f953 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -883,6 +883,14 @@ enum link_m_n_set {
> >  	M2_N2
> >  };
> >  
> > +struct intel_dp_desc {
> > +	u8 oui[3];
> > +	u8 device_id[6];
> > +	u8 hw_rev;
> > +	u8 sw_major_rev;
> > +	u8 sw_minor_rev;
> > +} __packed;
> > +
> >  struct intel_dp {
> >  	i915_reg_t output_reg;
> >  	i915_reg_t aux_ch_ctl_reg;
> > @@ -905,6 +913,8 @@ struct intel_dp {
> >  	/* sink rates as reported by DP_SUPPORTED_LINK_RATES */
> >  	uint8_t num_sink_rates;
> >  	int sink_rates[DP_MAX_SUPPORTED_RATES];
> > +	/* sink or branch descriptor */
> > +	struct intel_dp_desc desc;
> >  	struct drm_dp_aux aux;
> >  	uint8_t train_set[4];
> >  	int panel_power_up_delay;
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: warning for drm/i915/lspcon: Work around resume failure

[Jani Nikula]

On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> On ma, 2016-10-24 at 17:16 +0000, Patchwork wrote:
>> == Series Details ==
>> 
>> Series: drm/i915/lspcon: Work around resume failure
>> URL   : https://patchwork.freedesktop.org/series/14280/
>> State : warning
>> 
>> == Summary ==
>> 
>> Series 14280v1 drm/i915/lspcon: Work around resume failure
>> https://patchwork.freedesktop.org/api/1.0/series/14280/revisions/1/mb
>> ox/
>> 
>> Test drv_module_reload_basic:
>>                 pass       -> DMESG-WARN (fi-skl-6700hq)
>
> This one is a result of the previous CI run where LSPCON failed after
> S3. After a warm-reboot LSPCON is still in a broken state, and even the
> initial LSPCON probe will fail.
>
> A power cycle between CI runs would prevent the above.

Err, what. So patch 8/8 doesn't fix that problem? What fixes that
then?!?

BR,
Jani

>
>> Test gem_exec_suspend:
>>         Subgroup basic-s3:
>>                 dmesg-warn -> PASS       (fi-skl-6700hq)
>> Test kms_pipe_crc_basic:
>>         Subgroup suspend-read-crc-pipe-a:
>>                 dmesg-warn -> PASS       (fi-skl-6700hq)
>>         Subgroup suspend-read-crc-pipe-b:
>>                 dmesg-warn -> PASS       (fi-skl-6700hq)
>>         Subgroup suspend-read-crc-pipe-c:
>>                 dmesg-warn -> PASS       (fi-skl-6700hq)
>> 
>> fi-bdw-
>> 5557u     total:246  pass:231  dwarn:0   dfail:0   fail:0   skip:15 
>> fi-bsw-
>> n3050     total:246  pass:204  dwarn:0   dfail:0   fail:0   skip:42 
>> fi-bxt-
>> t5700     total:246  pass:216  dwarn:0   dfail:0   fail:0   skip:30 
>> fi-byt-
>> j1900     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
>> fi-byt-
>> n2820     total:246  pass:211  dwarn:0   dfail:0   fail:0   skip:35 
>> fi-hsw-
>> 4770      total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
>> fi-hsw-
>> 4770r     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
>> fi-ilk-
>> 650       total:246  pass:185  dwarn:0   dfail:0   fail:1   skip:60 
>> fi-ivb-
>> 3520m     total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25 
>> fi-ivb-
>> 3770      total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25 
>> fi-skl-
>> 6260u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
>> fi-skl-
>> 6700hq    total:246  pass:222  dwarn:1   dfail:0   fail:0   skip:23 
>> fi-skl-
>> 6700k     total:246  pass:222  dwarn:1   dfail:0   fail:0   skip:23 
>> fi-skl-
>> 6770hq    total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
>> fi-snb-
>> 2520m     total:246  pass:210  dwarn:0   dfail:0   fail:0   skip:36 
>> fi-snb-
>> 2600      total:246  pass:209  dwarn:0   dfail:0   fail:0   skip:37 
>> 
>> Results at /Patchwork_2801/
>> 
>> 102441087e47f4892b88c4f6e308e3edf89eeda1 drm-intel-nightly: 2016y-
>> 10m-24d-14h-28m-17s UTC integration manifest
>> eaa672c drm/i915/lspcon: Add workaround for resuming in PCON mode
>> eb04604 drm/i915/lspcon: Get DDC adapter via container_of() instead
>> of cached ptr
>> 0ca993d drm/i915/dp: Read DP descriptor for eDP and LSPCON too
>> 0144d102 drm/i915/lspcon: Fail LSPCON probe if the start of DPCD
>> can't be read
>> 7568e07 drm/i915/dp: Print full branch/sink descriptor
>> 3b4f6e9 drm/i915/dp: Print only sink or branch specific OUI based on
>> dev type
>> b1439bc drm/i915/dp: Remove debug dependency of DPCD SW/HW revision
>> read
>> 2beca1a drm/dp: Factor out helper to distinguish between branch and
>> sink devices
>> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: warning for drm/i915/lspcon: Work around resume failure

[Imre Deak]

On Mon, 2016-10-24 at 21:58 +0300, Jani Nikula wrote:
> On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> > On ma, 2016-10-24 at 17:16 +0000, Patchwork wrote:
> > > == Series Details ==
> > > 
> > > Series: drm/i915/lspcon: Work around resume failure
> > > URL   : https://patchwork.freedesktop.org/series/14280/
> > > State : warning
> > > 
> > > == Summary ==
> > > 
> > > Series 14280v1 drm/i915/lspcon: Work around resume failure
> > > https://patchwork.freedesktop.org/api/1.0/series/14280/revisions/1/mb
> > > ox/
> > > 
> > > Test drv_module_reload_basic:
> > >                 pass       -> DMESG-WARN (fi-skl-6700hq)
> > 
> > This one is a result of the previous CI run where LSPCON failed after
> > S3. After a warm-reboot LSPCON is still in a broken state, and even the
> > initial LSPCON probe will fail.
> > 
> > A power cycle between CI runs would prevent the above.
> 
> Err, what. So patch 8/8 doesn't fix that problem? What fixes that
> then?!?

It does fix the problem, which happens after S3. The above fail is due to the
previous CI run which left LSPCON in a broken state after an S3 test. A subsequent
warm-reset on that machine won't recover LSPCON and the load time LSPCON probe will
fail. I couldn't reproduce this problem with the workaround applied.

--Imre

> 
> BR,
> Jani
> 
> > 
> > > Test gem_exec_suspend:
> > >         Subgroup basic-s3:
> > >                 dmesg-warn -> PASS       (fi-skl-6700hq)
> > > Test kms_pipe_crc_basic:
> > >         Subgroup suspend-read-crc-pipe-a:
> > >                 dmesg-warn -> PASS       (fi-skl-6700hq)
> > >         Subgroup suspend-read-crc-pipe-b:
> > >                 dmesg-warn -> PASS       (fi-skl-6700hq)
> > >         Subgroup suspend-read-crc-pipe-c:
> > >                 dmesg-warn -> PASS       (fi-skl-6700hq)
> > > 
> > > fi-bdw-
> > > 5557u     total:246  pass:231  dwarn:0   dfail:0   fail:0   skip:15 
> > > fi-bsw-
> > > n3050     total:246  pass:204  dwarn:0   dfail:0   fail:0   skip:42 
> > > fi-bxt-
> > > t5700     total:246  pass:216  dwarn:0   dfail:0   fail:0   skip:30 
> > > fi-byt-
> > > j1900     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
> > > fi-byt-
> > > n2820     total:246  pass:211  dwarn:0   dfail:0   fail:0   skip:35 
> > > fi-hsw-
> > > 4770      total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
> > > fi-hsw-
> > > 4770r     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
> > > fi-ilk-
> > > 650       total:246  pass:185  dwarn:0   dfail:0   fail:1   skip:60 
> > > fi-ivb-
> > > 3520m     total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25 
> > > fi-ivb-
> > > 3770      total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25 
> > > fi-skl-
> > > 6260u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
> > > fi-skl-
> > > 6700hq    total:246  pass:222  dwarn:1   dfail:0   fail:0   skip:23 
> > > fi-skl-
> > > 6700k     total:246  pass:222  dwarn:1   dfail:0   fail:0   skip:23 
> > > fi-skl-
> > > 6770hq    total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
> > > fi-snb-
> > > 2520m     total:246  pass:210  dwarn:0   dfail:0   fail:0   skip:36 
> > > fi-snb-
> > > 2600      total:246  pass:209  dwarn:0   dfail:0   fail:0   skip:37 
> > > 
> > > Results at /Patchwork_2801/
> > > 
> > > 102441087e47f4892b88c4f6e308e3edf89eeda1 drm-intel-nightly: 2016y-
> > > 10m-24d-14h-28m-17s UTC integration manifest
> > > eaa672c drm/i915/lspcon: Add workaround for resuming in PCON mode
> > > eb04604 drm/i915/lspcon: Get DDC adapter via container_of() instead
> > > of cached ptr
> > > 0ca993d drm/i915/dp: Read DP descriptor for eDP and LSPCON too
> > > 0144d102 drm/i915/lspcon: Fail LSPCON probe if the start of DPCD
> > > can't be read
> > > 7568e07 drm/i915/dp: Print full branch/sink descriptor
> > > 3b4f6e9 drm/i915/dp: Print only sink or branch specific OUI based on
> > > dev type
> > > b1439bc drm/i915/dp: Remove debug dependency of DPCD SW/HW revision
> > > read
> > > 2beca1a drm/dp: Factor out helper to distinguish between branch and
> > > sink devices
> > > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/8] drm/i915/dp: Print full branch/sink descriptor

[Jani Nikula]

On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> On Mon, 2016-10-24 at 21:14 +0300, Jani Nikula wrote:
>> On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
>> > Extend the branch/sink descriptor info with the missing device ID
>> > field. While at it also read out all the descriptor registers in one
>> > transfer and make the debug print more compact.
>> > 
>> > v2: (Jani)
>> > - Cache the descriptor in intel_dp.
>> > - Split out this change into a separate patch.
>> > 
>> > Cc: Jani Nikula <jani.nikula@intel.com>
>> > Signed-off-by: Imre Deak <imre.deak@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_dp.c  | 61 +++++++++++++---------------------------
>> >  drivers/gpu/drm/i915/intel_drv.h | 10 +++++++
>> >  2 files changed, 30 insertions(+), 41 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> > index 1991250..726fdf2 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > @@ -1451,34 +1451,33 @@ static void intel_dp_print_rates(struct intel_dp *intel_dp)
>> >  	DRM_DEBUG_KMS("common rates: %s\n", str);
>> >  }
>> >  
>> > -static void intel_dp_print_hw_revision(struct intel_dp *intel_dp)
>> > +static bool
>> > +__intel_dp_read_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc)
>> >  {
>> > -	uint8_t rev;
>> > -	int len;
>> > +	u32 base = drm_dp_is_branch(intel_dp->dpcd) ? DP_BRANCH_OUI :
>> > +						      DP_SINK_OUI;
>> >  
>> > -	if (!drm_dp_is_branch(intel_dp->dpcd))
>> > -		return;
>> > -
>> > -	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_HW_REV, &rev, 1);
>> > -	if (len < 0)
>> > -		return;
>> > -
>> > -	DRM_DEBUG_KMS("sink hw revision: %d.%d\n", (rev & 0xf0) >> 4, rev & 0xf);
>> > +	return drm_dp_dpcd_read(&intel_dp->aux, base, desc, sizeof(*desc)) ==
>> > +	       sizeof(*desc);
>> >  }
>> >  
>> > -static void intel_dp_print_sw_revision(struct intel_dp *intel_dp)
>> > +static bool intel_dp_read_desc(struct intel_dp *intel_dp)
>> >  {
>> > -	uint8_t rev[2];
>> > -	int len;
>> > +	struct intel_dp_desc *desc = &intel_dp->desc;
>> > +	bool oui_sup = intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] &
>> > +		       DP_OUI_SUPPORT;
>> >  
>> > -	if (!drm_dp_is_branch(intel_dp->dpcd))
>> > -		return;
>> > +	if (__intel_dp_read_desc(intel_dp, desc) < 0)
>> 
>> The bool returned from __intel_dp_read_desc will never be less than 0...
>
> Yep, that's a typo, will fix it.
>
>> There's no point in reading the desc if oui_sup is false, is there? All of desc should read all zeros in that case, not just OUI.
>
> The spec is not too clear about this yes, but as I read it oui_sup
> applies only to the OUI reg, device ID and the revisions can be
> still valid.

For address 00007h:

"""
Bit 7 = OUI Support
0 = OUI not supported
1 = OUI supported (OUI and Device Identification mandatory for DP 1.2)
00400h to 00402h for a Sink device, plus 00403h-0040Bh for Device
Identification
00500h to 00502h for a Branch device, plus 00503h-0050Bh for Device
Identification
"""

Based on that I'd say the bit covers device id and revisions too. Why
would the device id and revision offset be mentioned here otherwise?

> The LSPCON FW has oui_sup cleared, but returns valid device ID and
> revision values.

I think I'll cry a little.

>> I guess the desc should be reset to zeros at detect or somewhere. I'm
>> sure we have other stuff we fail to reset too.
>
> Yes, like the dpcd array. desc will be used only for debug printing atm
> where we always re-read it first, and for LSPCON where it's constant,
> so if that's fine for you I'd leave this for later.
>
>> 
>> > +		return false;
>> >  
>> > -	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_SW_REV, &rev, 2);
>> > -	if (len < 0)
>> > -		return;
>> > +	DRM_DEBUG_KMS("DP %s: OUI %*phD%s dev-ID %.*s HW-rev %d.%d SW-rev %d.%d\n",
>> 
>> Maybe some form of %*pE would be more defensive for device id? See
>> Documentation/printk-formats.txt.
>
> Yes, can change that to '%*pE'.
>
>> > +		      drm_dp_is_branch(intel_dp->dpcd) ? "branch" : "sink",
>> > +		      (int)sizeof(desc->oui), desc->oui, oui_sup ? "" : "(NS)",
>> > +		      (int)sizeof(desc->device_id), desc->device_id,
>> > +		      desc->hw_rev >> 4, desc->hw_rev & 0xf,
>> > +		      desc->sw_major_rev, desc->sw_minor_rev);
>> >  
>> > -	DRM_DEBUG_KMS("sink sw revision: %d.%d\n", rev[0], rev[1]);
>> > +	return true;
>> >  }
>> >  
>> >  static int rate_to_index(int find, const int *rates)
>> > @@ -3621,23 +3620,6 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>> >  	return true;
>> >  }
>> >  
>> > -static void
>> > -intel_dp_probe_oui(struct intel_dp *intel_dp)
>> > -{
>> > -	bool is_branch = drm_dp_is_branch(intel_dp->dpcd);
>> > -	u8 buf[3];
>> > -
>> > -	if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
>> > -		return;
>> > -
>> > -	if (drm_dp_dpcd_read(&intel_dp->aux,
>> > -			     is_branch ? DP_BRANCH_OUI : DP_SINK_OUI,
>> > -			     buf, 3) == 3)
>> > -		DRM_DEBUG_KMS("%s OUI: %02hx%02hx%02hx\n",
>> > -			      is_branch ? "Branch" : "Sink",
>> > -			      buf[0], buf[1], buf[2]);
>> > -}
>> > -
>> >  static bool
>> >  intel_dp_can_mst(struct intel_dp *intel_dp)
>> >  {
>> > @@ -4416,10 +4398,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>> >  
>> >  	intel_dp_print_rates(intel_dp);
>> >  
>> > -	intel_dp_probe_oui(intel_dp);
>> > -
>> > -	intel_dp_print_hw_revision(intel_dp);
>> > -	intel_dp_print_sw_revision(intel_dp);
>> > +	intel_dp_read_desc(intel_dp);
>> >  
>> >  	intel_dp_configure_mst(intel_dp);
>> >  
>> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> > index 16b33f5..4c9f953 100644
>> > --- a/drivers/gpu/drm/i915/intel_drv.h
>> > +++ b/drivers/gpu/drm/i915/intel_drv.h
>> > @@ -883,6 +883,14 @@ enum link_m_n_set {
>> >  	M2_N2
>> >  };
>> >  
>> > +struct intel_dp_desc {
>> > +	u8 oui[3];
>> > +	u8 device_id[6];
>> > +	u8 hw_rev;
>> > +	u8 sw_major_rev;
>> > +	u8 sw_minor_rev;
>> > +} __packed;
>> > +
>> >  struct intel_dp {
>> >  	i915_reg_t output_reg;
>> >  	i915_reg_t aux_ch_ctl_reg;
>> > @@ -905,6 +913,8 @@ struct intel_dp {
>> >  	/* sink rates as reported by DP_SUPPORTED_LINK_RATES */
>> >  	uint8_t num_sink_rates;
>> >  	int sink_rates[DP_MAX_SUPPORTED_RATES];
>> > +	/* sink or branch descriptor */
>> > +	struct intel_dp_desc desc;
>> >  	struct drm_dp_aux aux;
>> >  	uint8_t train_set[4];
>> >  	int panel_power_up_delay;
>> 

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/8] drm/i915/dp: Print full branch/sink descriptor

[Imre Deak]

On Mon, 2016-10-24 at 22:10 +0300, Jani Nikula wrote:
> On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> > On Mon, 2016-10-24 at 21:14 +0300, Jani Nikula wrote:
> > > On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> > > > Extend the branch/sink descriptor info with the missing device
> > > > ID
> > > > field. While at it also read out all the descriptor registers
> > > > in one
> > > > transfer and make the debug print more compact.
> > > > 
> > > > v2: (Jani)
> > > > - Cache the descriptor in intel_dp.
> > > > - Split out this change into a separate patch.
> > > > 
> > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c  | 61 +++++++++++++-----------
> > > > ----------------
> > > >  drivers/gpu/drm/i915/intel_drv.h | 10 +++++++
> > > >  2 files changed, 30 insertions(+), 41 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 1991250..726fdf2 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -1451,34 +1451,33 @@ static void intel_dp_print_rates(struct
> > > > intel_dp *intel_dp)
> > > >  	DRM_DEBUG_KMS("common rates: %s\n", str);
> > > >  }
> > > >  
> > > > -static void intel_dp_print_hw_revision(struct intel_dp
> > > > *intel_dp)
> > > > +static bool
> > > > +__intel_dp_read_desc(struct intel_dp *intel_dp, struct
> > > > intel_dp_desc *desc)
> > > >  {
> > > > -	uint8_t rev;
> > > > -	int len;
> > > > +	u32 base = drm_dp_is_branch(intel_dp->dpcd) ?
> > > > DP_BRANCH_OUI :
> > > > +						      DP_SINK_
> > > > OUI;
> > > >  
> > > > -	if (!drm_dp_is_branch(intel_dp->dpcd))
> > > > -		return;
> > > > -
> > > > -	len = drm_dp_dpcd_read(&intel_dp->aux,
> > > > DP_BRANCH_HW_REV, &rev, 1);
> > > > -	if (len < 0)
> > > > -		return;
> > > > -
> > > > -	DRM_DEBUG_KMS("sink hw revision: %d.%d\n", (rev &
> > > > 0xf0) >> 4, rev & 0xf);
> > > > +	return drm_dp_dpcd_read(&intel_dp->aux, base, desc,
> > > > sizeof(*desc)) ==
> > > > +	       sizeof(*desc);
> > > >  }
> > > >  
> > > > -static void intel_dp_print_sw_revision(struct intel_dp
> > > > *intel_dp)
> > > > +static bool intel_dp_read_desc(struct intel_dp *intel_dp)
> > > >  {
> > > > -	uint8_t rev[2];
> > > > -	int len;
> > > > +	struct intel_dp_desc *desc = &intel_dp->desc;
> > > > +	bool oui_sup = intel_dp-
> > > > >dpcd[DP_DOWN_STREAM_PORT_COUNT] &
> > > > +		       DP_OUI_SUPPORT;
> > > >  
> > > > -	if (!drm_dp_is_branch(intel_dp->dpcd))
> > > > -		return;
> > > > +	if (__intel_dp_read_desc(intel_dp, desc) < 0)
> > > 
> > > The bool returned from __intel_dp_read_desc will never be less
> > > than 0...
> > 
> > Yep, that's a typo, will fix it.
> > 
> > > There's no point in reading the desc if oui_sup is false, is
> > > there? All of desc should read all zeros in that case, not just
> > > OUI.
> > 
> > The spec is not too clear about this yes, but as I read it oui_sup
> > applies only to the OUI reg, device ID and the revisions can be
> > still valid.
> 
> For address 00007h:
> 
> """
> Bit 7 = OUI Support
> 0 = OUI not supported
> 1 = OUI supported (OUI and Device Identification mandatory for DP
> 1.2)
> 00400h to 00402h for a Sink device, plus 00403h-0040Bh for Device
> Identification
> 00500h to 00502h for a Branch device, plus 00503h-0050Bh for Device
> Identification
> """
> 
> Based on that I'd say the bit covers device id and revisions too. Why
> would the device id and revision offset be mentioned here otherwise?

As a reference to what 'Device Identification' above means? In any case
if 'OUI Support' applies to the whole descriptor referring separately
to OUI and Device Identification right afterwards is confusing to me.

Could we just read it out regardless of the flag and depend on the read
failing or the values being zeroed if it's not "supported"?

> 
> > The LSPCON FW has oui_sup cleared, but returns valid device ID and
> > revision values.
> 
> I think I'll cry a little.
> 
> > > I guess the desc should be reset to zeros at detect or somewhere.
> > > I'm
> > > sure we have other stuff we fail to reset too.
> > 
> > Yes, like the dpcd array. desc will be used only for debug printing
> > atm
> > where we always re-read it first, and for LSPCON where it's
> > constant,
> > so if that's fine for you I'd leave this for later.
> > 
> > > 
> > > > +		return false;
> > > >  
> > > > -	len = drm_dp_dpcd_read(&intel_dp->aux,
> > > > DP_BRANCH_SW_REV, &rev, 2);
> > > > -	if (len < 0)
> > > > -		return;
> > > > +	DRM_DEBUG_KMS("DP %s: OUI %*phD%s dev-ID %.*s HW-rev
> > > > %d.%d SW-rev %d.%d\n",
> > > 
> > > Maybe some form of %*pE would be more defensive for device id?
> > > See
> > > Documentation/printk-formats.txt.
> > 
> > Yes, can change that to '%*pE'.
> > 
> > > > +		      drm_dp_is_branch(intel_dp->dpcd) ?
> > > > "branch" : "sink",
> > > > +		      (int)sizeof(desc->oui), desc->oui,
> > > > oui_sup ? "" : "(NS)",
> > > > +		      (int)sizeof(desc->device_id), desc-
> > > > >device_id,
> > > > +		      desc->hw_rev >> 4, desc->hw_rev & 0xf,
> > > > +		      desc->sw_major_rev, desc->sw_minor_rev);
> > > >  
> > > > -	DRM_DEBUG_KMS("sink sw revision: %d.%d\n", rev[0],
> > > > rev[1]);
> > > > +	return true;
> > > >  }
> > > >  
> > > >  static int rate_to_index(int find, const int *rates)
> > > > @@ -3621,23 +3620,6 @@ intel_dp_get_dpcd(struct intel_dp
> > > > *intel_dp)
> > > >  	return true;
> > > >  }
> > > >  
> > > > -static void
> > > > -intel_dp_probe_oui(struct intel_dp *intel_dp)
> > > > -{
> > > > -	bool is_branch = drm_dp_is_branch(intel_dp->dpcd);
> > > > -	u8 buf[3];
> > > > -
> > > > -	if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] &
> > > > DP_OUI_SUPPORT))
> > > > -		return;
> > > > -
> > > > -	if (drm_dp_dpcd_read(&intel_dp->aux,
> > > > -			     is_branch ? DP_BRANCH_OUI :
> > > > DP_SINK_OUI,
> > > > -			     buf, 3) == 3)
> > > > -		DRM_DEBUG_KMS("%s OUI: %02hx%02hx%02hx\n",
> > > > -			      is_branch ? "Branch" : "Sink",
> > > > -			      buf[0], buf[1], buf[2]);
> > > > -}
> > > > -
> > > >  static bool
> > > >  intel_dp_can_mst(struct intel_dp *intel_dp)
> > > >  {
> > > > @@ -4416,10 +4398,7 @@ intel_dp_long_pulse(struct
> > > > intel_connector *intel_connector)
> > > >  
> > > >  	intel_dp_print_rates(intel_dp);
> > > >  
> > > > -	intel_dp_probe_oui(intel_dp);
> > > > -
> > > > -	intel_dp_print_hw_revision(intel_dp);
> > > > -	intel_dp_print_sw_revision(intel_dp);
> > > > +	intel_dp_read_desc(intel_dp);
> > > >  
> > > >  	intel_dp_configure_mst(intel_dp);
> > > >  
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 16b33f5..4c9f953 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -883,6 +883,14 @@ enum link_m_n_set {
> > > >  	M2_N2
> > > >  };
> > > >  
> > > > +struct intel_dp_desc {
> > > > +	u8 oui[3];
> > > > +	u8 device_id[6];
> > > > +	u8 hw_rev;
> > > > +	u8 sw_major_rev;
> > > > +	u8 sw_minor_rev;
> > > > +} __packed;
> > > > +
> > > >  struct intel_dp {
> > > >  	i915_reg_t output_reg;
> > > >  	i915_reg_t aux_ch_ctl_reg;
> > > > @@ -905,6 +913,8 @@ struct intel_dp {
> > > >  	/* sink rates as reported by DP_SUPPORTED_LINK_RATES
> > > > */
> > > >  	uint8_t num_sink_rates;
> > > >  	int sink_rates[DP_MAX_SUPPORTED_RATES];
> > > > +	/* sink or branch descriptor */
> > > > +	struct intel_dp_desc desc;
> > > >  	struct drm_dp_aux aux;
> > > >  	uint8_t train_set[4];
> > > >  	int panel_power_up_delay;
> > > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: warning for drm/i915/lspcon: Work around resume failure

[Saarinen, Jani]

> == Series Details ==
> 
> Series: drm/i915/lspcon: Work around resume failure
> URL   : https://patchwork.freedesktop.org/series/14280/
> State : warning
> 
> == Summary ==
> 
> Series 14280v1 drm/i915/lspcon: Work around resume failure
> https://patchwork.freedesktop.org/api/1.0/series/14280/revisions/1/mbox/
> 
> Test drv_module_reload_basic:
>                 pass       -> DMESG-WARN (fi-skl-6700hq)
[   27.826226] [drm:lspcon_init [i915]] *ERROR* Failed to probe lspcon
[   27.826261] [drm:intel_ddi_init [i915]] *ERROR* LSPCON init failed on port B
[   27.846057] [Firmware Bug]: ACPI(PEGP) defines _DOD but not _DOS

> Test gem_exec_suspend:
>         Subgroup basic-s3:
>                 dmesg-warn -> PASS       (fi-skl-6700hq)
> Test kms_pipe_crc_basic:
>         Subgroup suspend-read-crc-pipe-a:
>                 dmesg-warn -> PASS       (fi-skl-6700hq)
>         Subgroup suspend-read-crc-pipe-b:
>                 dmesg-warn -> PASS       (fi-skl-6700hq)
>         Subgroup suspend-read-crc-pipe-c:
>                 dmesg-warn -> PASS       (fi-skl-6700hq)
> 
> fi-bdw-5557u     total:246  pass:231  dwarn:0   dfail:0   fail:0   skip:15
> fi-bsw-n3050     total:246  pass:204  dwarn:0   dfail:0   fail:0   skip:42
> fi-bxt-t5700     total:246  pass:216  dwarn:0   dfail:0   fail:0   skip:30
> fi-byt-j1900     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31
> fi-byt-n2820     total:246  pass:211  dwarn:0   dfail:0   fail:0   skip:35
> fi-hsw-4770      total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22
> fi-hsw-4770r     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22
> fi-ilk-650       total:246  pass:185  dwarn:0   dfail:0   fail:1   skip:60
> fi-ivb-3520m     total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25
> fi-ivb-3770      total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25
> fi-skl-6260u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14
> fi-skl-6700hq    total:246  pass:222  dwarn:1   dfail:0   fail:0   skip:23
> fi-skl-6700k     total:246  pass:222  dwarn:1   dfail:0   fail:0   skip:23
> fi-skl-6770hq    total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14
> fi-snb-2520m     total:246  pass:210  dwarn:0   dfail:0   fail:0   skip:36
> fi-snb-2600      total:246  pass:209  dwarn:0   dfail:0   fail:0   skip:37
> 
> Results at /Patchwork_2801/
> 
> 102441087e47f4892b88c4f6e308e3edf89eeda1 drm-intel-nightly: 2016y-10m-
> 24d-14h-28m-17s UTC integration manifest eaa672c drm/i915/lspcon: Add
> workaround for resuming in PCON mode
> eb04604 drm/i915/lspcon: Get DDC adapter via container_of() instead of cached
> ptr 0ca993d drm/i915/dp: Read DP descriptor for eDP and LSPCON too
> 0144d102 drm/i915/lspcon: Fail LSPCON probe if the start of DPCD can't be read
> 7568e07 drm/i915/dp: Print full branch/sink descriptor
> 3b4f6e9 drm/i915/dp: Print only sink or branch specific OUI based on dev type
> b1439bc drm/i915/dp: Remove debug dependency of DPCD SW/HW revision
> read 2beca1a drm/dp: Factor out helper to distinguish between branch and sink
> devices


Jani Saarinen
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo



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

Re: ✗ Fi.CI.BAT: warning for drm/i915/lspcon: Work around resume failure

[Saarinen, Jani]

> > == Summary ==
> >
> > Series 14280v1 drm/i915/lspcon: Work around resume failure
> > https://patchwork.freedesktop.org/api/1.0/series/14280/revisions/1/mb
> > ox/
> >
> > Test drv_module_reload_basic:
> >                 pass       -> DMESG-WARN (fi-skl-6700hq)
> 
> This one is a result of the previous CI run where LSPCON failed after S3. After a
> warm-reboot LSPCON is still in a broken state, and even the initial LSPCON probe
> will fail.
> 
> A power cycle between CI runs would prevent the above.
OK, thanks.

> 
> > Test gem_exec_suspend:
> >         Subgroup basic-s3:
> >                 dmesg-warn -> PASS       (fi-skl-6700hq) Test
> > kms_pipe_crc_basic:
> >         Subgroup suspend-read-crc-pipe-a:
> >                 dmesg-warn -> PASS       (fi-skl-6700hq)
> >         Subgroup suspend-read-crc-pipe-b:
> >                 dmesg-warn -> PASS       (fi-skl-6700hq)
> >         Subgroup suspend-read-crc-pipe-c:
> >                 dmesg-warn -> PASS       (fi-skl-6700hq)
> >


Jani Saarinen
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo



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

Re: [Intel-gfx] [PATCH v2 1/8] drm/dp: Factor out helper to distinguish between branch and sink devices

[Daniel Vetter]

On Mon, Oct 24, 2016 at 08:10:46PM +0300, Jani Nikula wrote:
> On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> > This check is open-coded in a few places, so it makes sense to simplify
> > things by having a helper for it similar to the rest of DPCD feature
> > helpers.
> >
> > v2: (Jani)
> > - Move the helper to drm_dp_helper.h.
> > - Split out this change to a separate patch.
> >
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> DP 1.4 has changed the name of "branch" devices, but I think this is
> good.
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Applied to drm-misc, thanks.
-Daniel
> 
> 
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 11 ++++-------
> >  include/drm/drm_dp_helper.h     |  6 ++++++
> >  2 files changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index f30db8f..3c2293b 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1459,8 +1459,7 @@ static void intel_dp_print_hw_revision(struct intel_dp *intel_dp)
> >  	if ((drm_debug & DRM_UT_KMS) == 0)
> >  		return;
> >  
> > -	if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> > -	      DP_DWN_STRM_PORT_PRESENT))
> > +	if (!drm_dp_is_branch(intel_dp->dpcd))
> >  		return;
> >  
> >  	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_HW_REV, &rev, 1);
> > @@ -1478,8 +1477,7 @@ static void intel_dp_print_sw_revision(struct intel_dp *intel_dp)
> >  	if ((drm_debug & DRM_UT_KMS) == 0)
> >  		return;
> >  
> > -	if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> > -	      DP_DWN_STRM_PORT_PRESENT))
> > +	if (!drm_dp_is_branch(intel_dp->dpcd))
> >  		return;
> >  
> >  	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_SW_REV, &rev, 2);
> > @@ -3615,8 +3613,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> >  	if (!is_edp(intel_dp) && !intel_dp->sink_count)
> >  		return false;
> >  
> > -	if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> > -	      DP_DWN_STRM_PORT_PRESENT))
> > +	if (!drm_dp_is_branch(intel_dp->dpcd))
> >  		return true; /* native DP sink */
> >  
> >  	if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
> > @@ -4134,7 +4131,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
> >  		return connector_status_connected;
> >  
> >  	/* if there's no downstream port, we're done */
> > -	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
> > +	if (!drm_dp_is_branch(dpcd))
> >  		return connector_status_connected;
> >  
> >  	/* If we're HPD-aware, SINK_COUNT changes dynamically */
> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > index 2a79882..55bbeb0 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -690,6 +690,12 @@ drm_dp_tps3_supported(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> >  		dpcd[DP_MAX_LANE_COUNT] & DP_TPS3_SUPPORTED;
> >  }
> >  
> > +static inline bool
> > +drm_dp_is_branch(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> > +{
> > +	return dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT;
> > +}
> > +
> >  /*
> >   * DisplayPort AUX channel
> >   */
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 1/8] drm/dp: Factor out helper to distinguish between branch and sink devices

[Jani Nikula]

On Tue, 25 Oct 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Oct 24, 2016 at 08:10:46PM +0300, Jani Nikula wrote:
>> On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
>> > This check is open-coded in a few places, so it makes sense to simplify
>> > things by having a helper for it similar to the rest of DPCD feature
>> > helpers.
>> >
>> > v2: (Jani)
>> > - Move the helper to drm_dp_helper.h.
>> > - Split out this change to a separate patch.
>> >
>> > Cc: Jani Nikula <jani.nikula@intel.com>
>> > Cc: dri-devel@lists.freedesktop.org
>> > Signed-off-by: Imre Deak <imre.deak@intel.com>
>> 
>> DP 1.4 has changed the name of "branch" devices, but I think this is
>> good.
>> 
>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>
> Applied to drm-misc, thanks.

Thanks, but... that actually makes this series harder to land. We can't
move forward with the i915 changes before a backmerge to dinq with this
helper included.

BR,
Jani.

> -Daniel
>> 
>> 
>> > ---
>> >  drivers/gpu/drm/i915/intel_dp.c | 11 ++++-------
>> >  include/drm/drm_dp_helper.h     |  6 ++++++
>> >  2 files changed, 10 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> > index f30db8f..3c2293b 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > @@ -1459,8 +1459,7 @@ static void intel_dp_print_hw_revision(struct intel_dp *intel_dp)
>> >  	if ((drm_debug & DRM_UT_KMS) == 0)
>> >  		return;
>> >  
>> > -	if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
>> > -	      DP_DWN_STRM_PORT_PRESENT))
>> > +	if (!drm_dp_is_branch(intel_dp->dpcd))
>> >  		return;
>> >  
>> >  	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_HW_REV, &rev, 1);
>> > @@ -1478,8 +1477,7 @@ static void intel_dp_print_sw_revision(struct intel_dp *intel_dp)
>> >  	if ((drm_debug & DRM_UT_KMS) == 0)
>> >  		return;
>> >  
>> > -	if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
>> > -	      DP_DWN_STRM_PORT_PRESENT))
>> > +	if (!drm_dp_is_branch(intel_dp->dpcd))
>> >  		return;
>> >  
>> >  	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_SW_REV, &rev, 2);
>> > @@ -3615,8 +3613,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>> >  	if (!is_edp(intel_dp) && !intel_dp->sink_count)
>> >  		return false;
>> >  
>> > -	if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
>> > -	      DP_DWN_STRM_PORT_PRESENT))
>> > +	if (!drm_dp_is_branch(intel_dp->dpcd))
>> >  		return true; /* native DP sink */
>> >  
>> >  	if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
>> > @@ -4134,7 +4131,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>> >  		return connector_status_connected;
>> >  
>> >  	/* if there's no downstream port, we're done */
>> > -	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
>> > +	if (!drm_dp_is_branch(dpcd))
>> >  		return connector_status_connected;
>> >  
>> >  	/* If we're HPD-aware, SINK_COUNT changes dynamically */
>> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>> > index 2a79882..55bbeb0 100644
>> > --- a/include/drm/drm_dp_helper.h
>> > +++ b/include/drm/drm_dp_helper.h
>> > @@ -690,6 +690,12 @@ drm_dp_tps3_supported(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
>> >  		dpcd[DP_MAX_LANE_COUNT] & DP_TPS3_SUPPORTED;
>> >  }
>> >  
>> > +static inline bool
>> > +drm_dp_is_branch(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
>> > +{
>> > +	return dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT;
>> > +}
>> > +
>> >  /*
>> >   * DisplayPort AUX channel
>> >   */
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/8] drm/dp: Factor out helper to distinguish between branch and sink devices

[Daniel Vetter]

On Tue, Oct 25, 2016 at 10:46:44AM +0300, Jani Nikula wrote:
> On Tue, 25 Oct 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Mon, Oct 24, 2016 at 08:10:46PM +0300, Jani Nikula wrote:
> >> On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> >> > This check is open-coded in a few places, so it makes sense to simplify
> >> > things by having a helper for it similar to the rest of DPCD feature
> >> > helpers.
> >> >
> >> > v2: (Jani)
> >> > - Move the helper to drm_dp_helper.h.
> >> > - Split out this change to a separate patch.
> >> >
> >> > Cc: Jani Nikula <jani.nikula@intel.com>
> >> > Cc: dri-devel@lists.freedesktop.org
> >> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> >> 
> >> DP 1.4 has changed the name of "branch" devices, but I think this is
> >> good.
> >> 
> >> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> >
> > Applied to drm-misc, thanks.
> 
> Thanks, but... that actually makes this series harder to land. We can't
> move forward with the i915 changes before a backmerge to dinq with this
> helper included.

Oh garbage, I thought it was 1/1. Should wait for coffee to kick in first.
I guess just apply it to drm-intel also, and we'll deal with the fallout
(I can't rebase and all that).
-Daniel

> 
> BR,
> Jani.
> 
> > -Daniel
> >> 
> >> 
> >> > ---
> >> >  drivers/gpu/drm/i915/intel_dp.c | 11 ++++-------
> >> >  include/drm/drm_dp_helper.h     |  6 ++++++
> >> >  2 files changed, 10 insertions(+), 7 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> > index f30db8f..3c2293b 100644
> >> > --- a/drivers/gpu/drm/i915/intel_dp.c
> >> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> > @@ -1459,8 +1459,7 @@ static void intel_dp_print_hw_revision(struct intel_dp *intel_dp)
> >> >  	if ((drm_debug & DRM_UT_KMS) == 0)
> >> >  		return;
> >> >  
> >> > -	if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> >> > -	      DP_DWN_STRM_PORT_PRESENT))
> >> > +	if (!drm_dp_is_branch(intel_dp->dpcd))
> >> >  		return;
> >> >  
> >> >  	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_HW_REV, &rev, 1);
> >> > @@ -1478,8 +1477,7 @@ static void intel_dp_print_sw_revision(struct intel_dp *intel_dp)
> >> >  	if ((drm_debug & DRM_UT_KMS) == 0)
> >> >  		return;
> >> >  
> >> > -	if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> >> > -	      DP_DWN_STRM_PORT_PRESENT))
> >> > +	if (!drm_dp_is_branch(intel_dp->dpcd))
> >> >  		return;
> >> >  
> >> >  	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_SW_REV, &rev, 2);
> >> > @@ -3615,8 +3613,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> >> >  	if (!is_edp(intel_dp) && !intel_dp->sink_count)
> >> >  		return false;
> >> >  
> >> > -	if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> >> > -	      DP_DWN_STRM_PORT_PRESENT))
> >> > +	if (!drm_dp_is_branch(intel_dp->dpcd))
> >> >  		return true; /* native DP sink */
> >> >  
> >> >  	if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
> >> > @@ -4134,7 +4131,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
> >> >  		return connector_status_connected;
> >> >  
> >> >  	/* if there's no downstream port, we're done */
> >> > -	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
> >> > +	if (!drm_dp_is_branch(dpcd))
> >> >  		return connector_status_connected;
> >> >  
> >> >  	/* If we're HPD-aware, SINK_COUNT changes dynamically */
> >> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> >> > index 2a79882..55bbeb0 100644
> >> > --- a/include/drm/drm_dp_helper.h
> >> > +++ b/include/drm/drm_dp_helper.h
> >> > @@ -690,6 +690,12 @@ drm_dp_tps3_supported(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> >> >  		dpcd[DP_MAX_LANE_COUNT] & DP_TPS3_SUPPORTED;
> >> >  }
> >> >  
> >> > +static inline bool
> >> > +drm_dp_is_branch(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> >> > +{
> >> > +	return dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT;
> >> > +}
> >> > +
> >> >  /*
> >> >   * DisplayPort AUX channel
> >> >   */
> >> 
> >> -- 
> >> Jani Nikula, Intel Open Source Technology Center
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/8] drm/i915/dp: Print full branch/sink descriptor

[Jani Nikula]

On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> On Mon, 2016-10-24 at 22:10 +0300, Jani Nikula wrote:
>> On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
>> > On Mon, 2016-10-24 at 21:14 +0300, Jani Nikula wrote:
>> > > On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
>> > > > Extend the branch/sink descriptor info with the missing device
>> > > > ID
>> > > > field. While at it also read out all the descriptor registers
>> > > > in one
>> > > > transfer and make the debug print more compact.
>> > > > 
>> > > > v2: (Jani)
>> > > > - Cache the descriptor in intel_dp.
>> > > > - Split out this change into a separate patch.
>> > > > 
>> > > > Cc: Jani Nikula <jani.nikula@intel.com>
>> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
>> > > > ---
>> > > >  drivers/gpu/drm/i915/intel_dp.c  | 61 +++++++++++++-----------
>> > > > ----------------
>> > > >  drivers/gpu/drm/i915/intel_drv.h | 10 +++++++
>> > > >  2 files changed, 30 insertions(+), 41 deletions(-)
>> > > > 
>> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> > > > b/drivers/gpu/drm/i915/intel_dp.c
>> > > > index 1991250..726fdf2 100644
>> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > > > @@ -1451,34 +1451,33 @@ static void intel_dp_print_rates(struct
>> > > > intel_dp *intel_dp)
>> > > >  	DRM_DEBUG_KMS("common rates: %s\n", str);
>> > > >  }
>> > > >  
>> > > > -static void intel_dp_print_hw_revision(struct intel_dp
>> > > > *intel_dp)
>> > > > +static bool
>> > > > +__intel_dp_read_desc(struct intel_dp *intel_dp, struct
>> > > > intel_dp_desc *desc)
>> > > >  {
>> > > > -	uint8_t rev;
>> > > > -	int len;
>> > > > +	u32 base = drm_dp_is_branch(intel_dp->dpcd) ?
>> > > > DP_BRANCH_OUI :
>> > > > +						      DP_SINK_
>> > > > OUI;
>> > > >  
>> > > > -	if (!drm_dp_is_branch(intel_dp->dpcd))
>> > > > -		return;
>> > > > -
>> > > > -	len = drm_dp_dpcd_read(&intel_dp->aux,
>> > > > DP_BRANCH_HW_REV, &rev, 1);
>> > > > -	if (len < 0)
>> > > > -		return;
>> > > > -
>> > > > -	DRM_DEBUG_KMS("sink hw revision: %d.%d\n", (rev &
>> > > > 0xf0) >> 4, rev & 0xf);
>> > > > +	return drm_dp_dpcd_read(&intel_dp->aux, base, desc,
>> > > > sizeof(*desc)) ==
>> > > > +	       sizeof(*desc);
>> > > >  }
>> > > >  
>> > > > -static void intel_dp_print_sw_revision(struct intel_dp
>> > > > *intel_dp)
>> > > > +static bool intel_dp_read_desc(struct intel_dp *intel_dp)
>> > > >  {
>> > > > -	uint8_t rev[2];
>> > > > -	int len;
>> > > > +	struct intel_dp_desc *desc = &intel_dp->desc;
>> > > > +	bool oui_sup = intel_dp-
>> > > > >dpcd[DP_DOWN_STREAM_PORT_COUNT] &
>> > > > +		       DP_OUI_SUPPORT;
>> > > >  
>> > > > -	if (!drm_dp_is_branch(intel_dp->dpcd))
>> > > > -		return;
>> > > > +	if (__intel_dp_read_desc(intel_dp, desc) < 0)
>> > > 
>> > > The bool returned from __intel_dp_read_desc will never be less
>> > > than 0...
>> > 
>> > Yep, that's a typo, will fix it.
>> > 
>> > > There's no point in reading the desc if oui_sup is false, is
>> > > there? All of desc should read all zeros in that case, not just
>> > > OUI.
>> > 
>> > The spec is not too clear about this yes, but as I read it oui_sup
>> > applies only to the OUI reg, device ID and the revisions can be
>> > still valid.
>> 
>> For address 00007h:
>> 
>> """
>> Bit 7 = OUI Support
>> 0 = OUI not supported
>> 1 = OUI supported (OUI and Device Identification mandatory for DP
>> 1.2)
>> 00400h to 00402h for a Sink device, plus 00403h-0040Bh for Device
>> Identification
>> 00500h to 00502h for a Branch device, plus 00503h-0050Bh for Device
>> Identification
>> """
>> 
>> Based on that I'd say the bit covers device id and revisions too. Why
>> would the device id and revision offset be mentioned here otherwise?
>
> As a reference to what 'Device Identification' above means? In any case
> if 'OUI Support' applies to the whole descriptor referring separately
> to OUI and Device Identification right afterwards is confusing to me.

What you call "desc" here means DPCD 0x400..0x40b for sinks, or
0x500..0x50b for branches. That's OUI, an ascii device identification
string, and hw/sw revisions.

What's the DPCD revision that LSPCON reports? OUI support is mandatory
for DPCD 1.2 and 1.4.

> Could we just read it out regardless of the flag and depend on the read
> failing or the values being zeroed if it's not "supported"?

Please tell me again why we use the OUI for this purpose instead of just
intel_dp->dpcd?

BR,
Jani.


>
>> 
>> > The LSPCON FW has oui_sup cleared, but returns valid device ID and
>> > revision values.
>> 
>> I think I'll cry a little.
>> 
>> > > I guess the desc should be reset to zeros at detect or somewhere.
>> > > I'm
>> > > sure we have other stuff we fail to reset too.
>> > 
>> > Yes, like the dpcd array. desc will be used only for debug printing
>> > atm
>> > where we always re-read it first, and for LSPCON where it's
>> > constant,
>> > so if that's fine for you I'd leave this for later.
>> > 
>> > > 
>> > > > +		return false;
>> > > >  
>> > > > -	len = drm_dp_dpcd_read(&intel_dp->aux,
>> > > > DP_BRANCH_SW_REV, &rev, 2);
>> > > > -	if (len < 0)
>> > > > -		return;
>> > > > +	DRM_DEBUG_KMS("DP %s: OUI %*phD%s dev-ID %.*s HW-rev
>> > > > %d.%d SW-rev %d.%d\n",
>> > > 
>> > > Maybe some form of %*pE would be more defensive for device id?
>> > > See
>> > > Documentation/printk-formats.txt.
>> > 
>> > Yes, can change that to '%*pE'.
>> > 
>> > > > +		      drm_dp_is_branch(intel_dp->dpcd) ?
>> > > > "branch" : "sink",
>> > > > +		      (int)sizeof(desc->oui), desc->oui,
>> > > > oui_sup ? "" : "(NS)",
>> > > > +		      (int)sizeof(desc->device_id), desc-
>> > > > >device_id,
>> > > > +		      desc->hw_rev >> 4, desc->hw_rev & 0xf,
>> > > > +		      desc->sw_major_rev, desc->sw_minor_rev);
>> > > >  
>> > > > -	DRM_DEBUG_KMS("sink sw revision: %d.%d\n", rev[0],
>> > > > rev[1]);
>> > > > +	return true;
>> > > >  }
>> > > >  
>> > > >  static int rate_to_index(int find, const int *rates)
>> > > > @@ -3621,23 +3620,6 @@ intel_dp_get_dpcd(struct intel_dp
>> > > > *intel_dp)
>> > > >  	return true;
>> > > >  }
>> > > >  
>> > > > -static void
>> > > > -intel_dp_probe_oui(struct intel_dp *intel_dp)
>> > > > -{
>> > > > -	bool is_branch = drm_dp_is_branch(intel_dp->dpcd);
>> > > > -	u8 buf[3];
>> > > > -
>> > > > -	if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] &
>> > > > DP_OUI_SUPPORT))
>> > > > -		return;
>> > > > -
>> > > > -	if (drm_dp_dpcd_read(&intel_dp->aux,
>> > > > -			     is_branch ? DP_BRANCH_OUI :
>> > > > DP_SINK_OUI,
>> > > > -			     buf, 3) == 3)
>> > > > -		DRM_DEBUG_KMS("%s OUI: %02hx%02hx%02hx\n",
>> > > > -			      is_branch ? "Branch" : "Sink",
>> > > > -			      buf[0], buf[1], buf[2]);
>> > > > -}
>> > > > -
>> > > >  static bool
>> > > >  intel_dp_can_mst(struct intel_dp *intel_dp)
>> > > >  {
>> > > > @@ -4416,10 +4398,7 @@ intel_dp_long_pulse(struct
>> > > > intel_connector *intel_connector)
>> > > >  
>> > > >  	intel_dp_print_rates(intel_dp);
>> > > >  
>> > > > -	intel_dp_probe_oui(intel_dp);
>> > > > -
>> > > > -	intel_dp_print_hw_revision(intel_dp);
>> > > > -	intel_dp_print_sw_revision(intel_dp);
>> > > > +	intel_dp_read_desc(intel_dp);
>> > > >  
>> > > >  	intel_dp_configure_mst(intel_dp);
>> > > >  
>> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> > > > b/drivers/gpu/drm/i915/intel_drv.h
>> > > > index 16b33f5..4c9f953 100644
>> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
>> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
>> > > > @@ -883,6 +883,14 @@ enum link_m_n_set {
>> > > >  	M2_N2
>> > > >  };
>> > > >  
>> > > > +struct intel_dp_desc {
>> > > > +	u8 oui[3];
>> > > > +	u8 device_id[6];
>> > > > +	u8 hw_rev;
>> > > > +	u8 sw_major_rev;
>> > > > +	u8 sw_minor_rev;
>> > > > +} __packed;
>> > > > +
>> > > >  struct intel_dp {
>> > > >  	i915_reg_t output_reg;
>> > > >  	i915_reg_t aux_ch_ctl_reg;
>> > > > @@ -905,6 +913,8 @@ struct intel_dp {
>> > > >  	/* sink rates as reported by DP_SUPPORTED_LINK_RATES
>> > > > */
>> > > >  	uint8_t num_sink_rates;
>> > > >  	int sink_rates[DP_MAX_SUPPORTED_RATES];
>> > > > +	/* sink or branch descriptor */
>> > > > +	struct intel_dp_desc desc;
>> > > >  	struct drm_dp_aux aux;
>> > > >  	uint8_t train_set[4];
>> > > >  	int panel_power_up_delay;
>> > > 
>> 

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/8] drm/i915/dp: Print full branch/sink descriptor

[Imre Deak]

On ti, 2016-10-25 at 12:28 +0300, Jani Nikula wrote:
> On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> > On Mon, 2016-10-24 at 22:10 +0300, Jani Nikula wrote:
> > > On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> > > > On Mon, 2016-10-24 at 21:14 +0300, Jani Nikula wrote:
> > > > > On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> > > > > > Extend the branch/sink descriptor info with the missing device
> > > > > > ID
> > > > > > field. While at it also read out all the descriptor registers
> > > > > > in one
> > > > > > transfer and make the debug print more compact.
> > > > > > 
> > > > > > v2: (Jani)
> > > > > > - Cache the descriptor in intel_dp.
> > > > > > - Split out this change into a separate patch.
> > > > > > 
> > > > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_dp.c  | 61 +++++++++++++-----------
> > > > > > ----------------
> > > > > >  drivers/gpu/drm/i915/intel_drv.h | 10 +++++++
> > > > > >  2 files changed, 30 insertions(+), 41 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > index 1991250..726fdf2 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > @@ -1451,34 +1451,33 @@ static void intel_dp_print_rates(struct
> > > > > > intel_dp *intel_dp)
> > > > > >  	DRM_DEBUG_KMS("common rates: %s\n", str);
> > > > > >  }
> > > > > >  
> > > > > > -static void intel_dp_print_hw_revision(struct intel_dp
> > > > > > *intel_dp)
> > > > > > +static bool
> > > > > > +__intel_dp_read_desc(struct intel_dp *intel_dp, struct
> > > > > > intel_dp_desc *desc)
> > > > > >  {
> > > > > > -	uint8_t rev;
> > > > > > -	int len;
> > > > > > +	u32 base = drm_dp_is_branch(intel_dp->dpcd) ?
> > > > > > DP_BRANCH_OUI :
> > > > > > +						      DP_SINK_
> > > > > > OUI;
> > > > > >  
> > > > > > -	if (!drm_dp_is_branch(intel_dp->dpcd))
> > > > > > -		return;
> > > > > > -
> > > > > > -	len = drm_dp_dpcd_read(&intel_dp->aux,
> > > > > > DP_BRANCH_HW_REV, &rev, 1);
> > > > > > -	if (len < 0)
> > > > > > -		return;
> > > > > > -
> > > > > > -	DRM_DEBUG_KMS("sink hw revision: %d.%d\n", (rev &
> > > > > > 0xf0) >> 4, rev & 0xf);
> > > > > > +	return drm_dp_dpcd_read(&intel_dp->aux, base, desc,
> > > > > > sizeof(*desc)) ==
> > > > > > +	       sizeof(*desc);
> > > > > >  }
> > > > > >  
> > > > > > -static void intel_dp_print_sw_revision(struct intel_dp
> > > > > > *intel_dp)
> > > > > > +static bool intel_dp_read_desc(struct intel_dp *intel_dp)
> > > > > >  {
> > > > > > -	uint8_t rev[2];
> > > > > > -	int len;
> > > > > > +	struct intel_dp_desc *desc = &intel_dp->desc;
> > > > > > +	bool oui_sup = intel_dp-
> > > > > > > dpcd[DP_DOWN_STREAM_PORT_COUNT] &
> > > > > > +		       DP_OUI_SUPPORT;
> > > > > >  
> > > > > > -	if (!drm_dp_is_branch(intel_dp->dpcd))
> > > > > > -		return;
> > > > > > +	if (__intel_dp_read_desc(intel_dp, desc) < 0)
> > > > > 
> > > > > The bool returned from __intel_dp_read_desc will never be less
> > > > > than 0...
> > > > 
> > > > Yep, that's a typo, will fix it.
> > > > 
> > > > > There's no point in reading the desc if oui_sup is false, is
> > > > > there? All of desc should read all zeros in that case, not just
> > > > > OUI.
> > > > 
> > > > The spec is not too clear about this yes, but as I read it oui_sup
> > > > applies only to the OUI reg, device ID and the revisions can be
> > > > still valid.
> > > 
> > > For address 00007h:
> > > 
> > > """
> > > Bit 7 = OUI Support
> > > 0 = OUI not supported
> > > 1 = OUI supported (OUI and Device Identification mandatory for DP
> > > 1.2)
> > > 00400h to 00402h for a Sink device, plus 00403h-0040Bh for Device
> > > Identification
> > > 00500h to 00502h for a Branch device, plus 00503h-0050Bh for Device
> > > Identification
> > > """
> > > 
> > > Based on that I'd say the bit covers device id and revisions too. Why
> > > would the device id and revision offset be mentioned here otherwise?
> > 
> > As a reference to what 'Device Identification' above means? In any case
> > if 'OUI Support' applies to the whole descriptor referring separately
> > to OUI and Device Identification right afterwards is confusing to me.
> 
> What you call "desc" here means DPCD 0x400..0x40b for sinks, or
> 0x500..0x50b for branches. That's OUI, an ascii device identification
> string, and hw/sw revisions.

Yes, I was talking about the specification.

> What's the DPCD revision that LSPCON reports? OUI support is mandatory
> for DPCD 1.2 and 1.4.

1.2

> > Could we just read it out regardless of the flag and depend on the read
> > failing or the values being zeroed if it's not "supported"?
> 
> Please tell me again why we use the OUI for this purpose instead of just
> intel_dp->dpcd?

For identifying if correct AUX data is returned in patch 8? Because imo
an ID will mismatch with bigger likelihood than the start of DPCD, if
for example an incorrect read will result in zeroes.

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

Re: [PATCH v2 4/8] drm/i915/dp: Print full branch/sink descriptor

[Jani Nikula]

On Tue, 25 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> On ti, 2016-10-25 at 12:28 +0300, Jani Nikula wrote:
>> On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
>> > On Mon, 2016-10-24 at 22:10 +0300, Jani Nikula wrote:
>> > > On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
>> > > > On Mon, 2016-10-24 at 21:14 +0300, Jani Nikula wrote:
>> > > > > On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
>> > > > > > Extend the branch/sink descriptor info with the missing device
>> > > > > > ID
>> > > > > > field. While at it also read out all the descriptor registers
>> > > > > > in one
>> > > > > > transfer and make the debug print more compact.
>> > > > > > 
>> > > > > > v2: (Jani)
>> > > > > > - Cache the descriptor in intel_dp.
>> > > > > > - Split out this change into a separate patch.
>> > > > > > 
>> > > > > > Cc: Jani Nikula <jani.nikula@intel.com>
>> > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
>> > > > > > ---
>> > > > > >  drivers/gpu/drm/i915/intel_dp.c  | 61 +++++++++++++-----------
>> > > > > > ----------------
>> > > > > >  drivers/gpu/drm/i915/intel_drv.h | 10 +++++++
>> > > > > >  2 files changed, 30 insertions(+), 41 deletions(-)
>> > > > > > 
>> > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> > > > > > b/drivers/gpu/drm/i915/intel_dp.c
>> > > > > > index 1991250..726fdf2 100644
>> > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > > > > > @@ -1451,34 +1451,33 @@ static void intel_dp_print_rates(struct
>> > > > > > intel_dp *intel_dp)
>> > > > > >  	DRM_DEBUG_KMS("common rates: %s\n", str);
>> > > > > >  }
>> > > > > >  
>> > > > > > -static void intel_dp_print_hw_revision(struct intel_dp
>> > > > > > *intel_dp)
>> > > > > > +static bool
>> > > > > > +__intel_dp_read_desc(struct intel_dp *intel_dp, struct
>> > > > > > intel_dp_desc *desc)
>> > > > > >  {
>> > > > > > -	uint8_t rev;
>> > > > > > -	int len;
>> > > > > > +	u32 base = drm_dp_is_branch(intel_dp->dpcd) ?
>> > > > > > DP_BRANCH_OUI :
>> > > > > > +						      DP_SINK_
>> > > > > > OUI;
>> > > > > >  
>> > > > > > -	if (!drm_dp_is_branch(intel_dp->dpcd))
>> > > > > > -		return;
>> > > > > > -
>> > > > > > -	len = drm_dp_dpcd_read(&intel_dp->aux,
>> > > > > > DP_BRANCH_HW_REV, &rev, 1);
>> > > > > > -	if (len < 0)
>> > > > > > -		return;
>> > > > > > -
>> > > > > > -	DRM_DEBUG_KMS("sink hw revision: %d.%d\n", (rev &
>> > > > > > 0xf0) >> 4, rev & 0xf);
>> > > > > > +	return drm_dp_dpcd_read(&intel_dp->aux, base, desc,
>> > > > > > sizeof(*desc)) ==
>> > > > > > +	       sizeof(*desc);
>> > > > > >  }
>> > > > > >  
>> > > > > > -static void intel_dp_print_sw_revision(struct intel_dp
>> > > > > > *intel_dp)
>> > > > > > +static bool intel_dp_read_desc(struct intel_dp *intel_dp)
>> > > > > >  {
>> > > > > > -	uint8_t rev[2];
>> > > > > > -	int len;
>> > > > > > +	struct intel_dp_desc *desc = &intel_dp->desc;
>> > > > > > +	bool oui_sup = intel_dp-
>> > > > > > > dpcd[DP_DOWN_STREAM_PORT_COUNT] &
>> > > > > > +		       DP_OUI_SUPPORT;
>> > > > > >  
>> > > > > > -	if (!drm_dp_is_branch(intel_dp->dpcd))
>> > > > > > -		return;
>> > > > > > +	if (__intel_dp_read_desc(intel_dp, desc) < 0)
>> > > > > 
>> > > > > The bool returned from __intel_dp_read_desc will never be less
>> > > > > than 0...
>> > > > 
>> > > > Yep, that's a typo, will fix it.
>> > > > 
>> > > > > There's no point in reading the desc if oui_sup is false, is
>> > > > > there? All of desc should read all zeros in that case, not just
>> > > > > OUI.
>> > > > 
>> > > > The spec is not too clear about this yes, but as I read it oui_sup
>> > > > applies only to the OUI reg, device ID and the revisions can be
>> > > > still valid.
>> > > 
>> > > For address 00007h:
>> > > 
>> > > """
>> > > Bit 7 = OUI Support
>> > > 0 = OUI not supported
>> > > 1 = OUI supported (OUI and Device Identification mandatory for DP
>> > > 1.2)
>> > > 00400h to 00402h for a Sink device, plus 00403h-0040Bh for Device
>> > > Identification
>> > > 00500h to 00502h for a Branch device, plus 00503h-0050Bh for Device
>> > > Identification
>> > > """
>> > > 
>> > > Based on that I'd say the bit covers device id and revisions too. Why
>> > > would the device id and revision offset be mentioned here otherwise?
>> > 
>> > As a reference to what 'Device Identification' above means? In any case
>> > if 'OUI Support' applies to the whole descriptor referring separately
>> > to OUI and Device Identification right afterwards is confusing to me.
>> 
>> What you call "desc" here means DPCD 0x400..0x40b for sinks, or
>> 0x500..0x50b for branches. That's OUI, an ascii device identification
>> string, and hw/sw revisions.
>
> Yes, I was talking about the specification.
>
>> What's the DPCD revision that LSPCON reports? OUI support is mandatory
>> for DPCD 1.2 and 1.4.
>
> 1.2
>
>> > Could we just read it out regardless of the flag and depend on the read
>> > failing or the values being zeroed if it's not "supported"?
>> 
>> Please tell me again why we use the OUI for this purpose instead of just
>> intel_dp->dpcd?
>
> For identifying if correct AUX data is returned in patch 8? Because imo
> an ID will mismatch with bigger likelihood than the start of DPCD, if
> for example an incorrect read will result in zeroes.

If the sink doesn't support some DPCD stuff, such as the OUI, it's
supposed to return all zeros... And this bugger is supposed to support
OUI because it's DP 1.2 but it doesn't, except it does anyway. *rolls
eyes*.

J.


>
> --Imre

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 4/8] drm/i915/dp: Print full branch/sink descriptor

[Imre Deak]

Extend the branch/sink descriptor info with the missing device ID
field. While at it also read out all the descriptor registers in one
transfer and make the debug print more compact.

v2: (Jani)
- Cache the descriptor in intel_dp.
- Split out this change into a separate patch.
v3: (Jani)
- Fix return value check of __intel_dp_read_desc().
- Use %pE instead of %s to print the device ID.

Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 63 ++++++++++++++--------------------------
 drivers/gpu/drm/i915/intel_drv.h | 10 +++++++
 2 files changed, 32 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1991250..57260a2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1451,34 +1451,35 @@ static void intel_dp_print_rates(struct intel_dp *intel_dp)
 	DRM_DEBUG_KMS("common rates: %s\n", str);
 }
 
-static void intel_dp_print_hw_revision(struct intel_dp *intel_dp)
+static bool
+__intel_dp_read_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc)
 {
-	uint8_t rev;
-	int len;
+	u32 base = drm_dp_is_branch(intel_dp->dpcd) ? DP_BRANCH_OUI :
+						      DP_SINK_OUI;
 
-	if (!drm_dp_is_branch(intel_dp->dpcd))
-		return;
-
-	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_HW_REV, &rev, 1);
-	if (len < 0)
-		return;
-
-	DRM_DEBUG_KMS("sink hw revision: %d.%d\n", (rev & 0xf0) >> 4, rev & 0xf);
+	return drm_dp_dpcd_read(&intel_dp->aux, base, desc, sizeof(*desc)) ==
+	       sizeof(*desc);
 }
 
-static void intel_dp_print_sw_revision(struct intel_dp *intel_dp)
+static bool intel_dp_read_desc(struct intel_dp *intel_dp)
 {
-	uint8_t rev[2];
-	int len;
+	struct intel_dp_desc *desc = &intel_dp->desc;
+	bool oui_sup = intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] &
+		       DP_OUI_SUPPORT;
+	int dev_id_len;
 
-	if (!drm_dp_is_branch(intel_dp->dpcd))
-		return;
+	if (!__intel_dp_read_desc(intel_dp, desc))
+		return false;
 
-	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_SW_REV, &rev, 2);
-	if (len < 0)
-		return;
+	dev_id_len = strnlen(desc->device_id, sizeof(desc->device_id));
+	DRM_DEBUG_KMS("DP %s: OUI %*phD%s dev-ID %*pE HW-rev %d.%d SW-rev %d.%d\n",
+		      drm_dp_is_branch(intel_dp->dpcd) ? "branch" : "sink",
+		      (int)sizeof(desc->oui), desc->oui, oui_sup ? "" : "(NS)",
+		      dev_id_len, desc->device_id,
+		      desc->hw_rev >> 4, desc->hw_rev & 0xf,
+		      desc->sw_major_rev, desc->sw_minor_rev);
 
-	DRM_DEBUG_KMS("sink sw revision: %d.%d\n", rev[0], rev[1]);
+	return true;
 }
 
 static int rate_to_index(int find, const int *rates)
@@ -3621,23 +3622,6 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	return true;
 }
 
-static void
-intel_dp_probe_oui(struct intel_dp *intel_dp)
-{
-	bool is_branch = drm_dp_is_branch(intel_dp->dpcd);
-	u8 buf[3];
-
-	if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
-		return;
-
-	if (drm_dp_dpcd_read(&intel_dp->aux,
-			     is_branch ? DP_BRANCH_OUI : DP_SINK_OUI,
-			     buf, 3) == 3)
-		DRM_DEBUG_KMS("%s OUI: %02hx%02hx%02hx\n",
-			      is_branch ? "Branch" : "Sink",
-			      buf[0], buf[1], buf[2]);
-}
-
 static bool
 intel_dp_can_mst(struct intel_dp *intel_dp)
 {
@@ -4416,10 +4400,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 
 	intel_dp_print_rates(intel_dp);
 
-	intel_dp_probe_oui(intel_dp);
-
-	intel_dp_print_hw_revision(intel_dp);
-	intel_dp_print_sw_revision(intel_dp);
+	intel_dp_read_desc(intel_dp);
 
 	intel_dp_configure_mst(intel_dp);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 16b33f5..4c9f953 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -883,6 +883,14 @@ enum link_m_n_set {
 	M2_N2
 };
 
+struct intel_dp_desc {
+	u8 oui[3];
+	u8 device_id[6];
+	u8 hw_rev;
+	u8 sw_major_rev;
+	u8 sw_minor_rev;
+} __packed;
+
 struct intel_dp {
 	i915_reg_t output_reg;
 	i915_reg_t aux_ch_ctl_reg;
@@ -905,6 +913,8 @@ struct intel_dp {
 	/* sink rates as reported by DP_SUPPORTED_LINK_RATES */
 	uint8_t num_sink_rates;
 	int sink_rates[DP_MAX_SUPPORTED_RATES];
+	/* sink or branch descriptor */
+	struct intel_dp_desc desc;
 	struct drm_dp_aux aux;
 	uint8_t train_set[4];
 	int panel_power_up_delay;
-- 
2.5.0

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

✗ Fi.CI.BAT: warning for drm/i915/lspcon: Work around resume failure (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/lspcon: Work around resume failure (rev2)
URL   : https://patchwork.freedesktop.org/series/14280/
State : warning

== Summary ==

Series 14280v2 drm/i915/lspcon: Work around resume failure
https://patchwork.freedesktop.org/api/1.0/series/14280/revisions/2/mbox/

Test drv_module_reload_basic:
                dmesg-warn -> PASS       (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-skl-6700hq)
Test gem_exec_suspend:
        Subgroup basic-s3:
                dmesg-warn -> PASS       (fi-skl-6770hq)
                dmesg-warn -> PASS       (fi-skl-6700hq)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> SKIP       (fi-byt-n2820)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                fail       -> PASS       (fi-skl-6770hq)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                dmesg-warn -> PASS       (fi-skl-6700hq)
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-skl-6770hq)
                dmesg-warn -> PASS       (fi-skl-6700hq)
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> PASS       (fi-skl-6700hq)

fi-bdw-5557u     total:246  pass:231  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:246  pass:204  dwarn:0   dfail:0   fail:0   skip:42 
fi-bxt-t5700     total:246  pass:216  dwarn:0   dfail:0   fail:0   skip:30 
fi-byt-j1900     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-byt-n2820     total:246  pass:210  dwarn:0   dfail:0   fail:0   skip:36 
fi-hsw-4770      total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-hsw-4770r     total:246  pass:223  dwarn:0   dfail:0   fail:0   skip:23 
fi-ilk-650       total:246  pass:185  dwarn:0   dfail:0   fail:0   skip:61 
fi-ivb-3520m     total:246  pass:220  dwarn:0   dfail:0   fail:0   skip:26 
fi-ivb-3770      total:246  pass:220  dwarn:0   dfail:0   fail:0   skip:26 
fi-kbl-7200u     total:246  pass:222  dwarn:0   dfail:0   fail:0   skip:24 
fi-skl-6260u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:246  pass:222  dwarn:1   dfail:0   fail:0   skip:23 
fi-skl-6700k     total:246  pass:222  dwarn:1   dfail:0   fail:0   skip:23 
fi-skl-6770hq    total:246  pass:231  dwarn:1   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:246  pass:209  dwarn:0   dfail:0   fail:0   skip:37 
fi-snb-2600      total:246  pass:208  dwarn:0   dfail:0   fail:0   skip:38 

3ce99d02d8f2b7d1414d20d5972f8e277b33693e drm-intel-nightly: 2016y-10m-25d-13h-55m-46s UTC integration manifest
ab50b11 drm/i915/lspcon: Add workaround for resuming in PCON mode
607efb9 drm/i915/lspcon: Get DDC adapter via container_of() instead of cached ptr
e281e36 drm/i915/dp: Read DP descriptor for eDP and LSPCON too
742a374 drm/i915/lspcon: Fail LSPCON probe if the start of DPCD can't be read
e3a2cc0 drm/i915/dp: Print full branch/sink descriptor
d6131ca drm/i915/dp: Print only sink or branch specific OUI based on dev type
3c3c2a2 drm/i915/dp: Remove debug dependency of DPCD SW/HW revision read

Full results at https://intel-gfx-ci.01.org/CI/Patchwork_2810/

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2810/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 4/8] drm/i915/dp: Print full branch/sink descriptor

[Jani Nikula]

On Tue, 25 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> Extend the branch/sink descriptor info with the missing device ID
> field. While at it also read out all the descriptor registers in one
> transfer and make the debug print more compact.
>
> v2: (Jani)
> - Cache the descriptor in intel_dp.
> - Split out this change into a separate patch.
> v3: (Jani)
> - Fix return value check of __intel_dp_read_desc().
> - Use %pE instead of %s to print the device ID.
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 63 ++++++++++++++--------------------------
>  drivers/gpu/drm/i915/intel_drv.h | 10 +++++++
>  2 files changed, 32 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 1991250..57260a2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1451,34 +1451,35 @@ static void intel_dp_print_rates(struct intel_dp *intel_dp)
>  	DRM_DEBUG_KMS("common rates: %s\n", str);
>  }
>  
> -static void intel_dp_print_hw_revision(struct intel_dp *intel_dp)
> +static bool
> +__intel_dp_read_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc)
>  {
> -	uint8_t rev;
> -	int len;
> +	u32 base = drm_dp_is_branch(intel_dp->dpcd) ? DP_BRANCH_OUI :
> +						      DP_SINK_OUI;
>  
> -	if (!drm_dp_is_branch(intel_dp->dpcd))
> -		return;
> -
> -	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_HW_REV, &rev, 1);
> -	if (len < 0)
> -		return;
> -
> -	DRM_DEBUG_KMS("sink hw revision: %d.%d\n", (rev & 0xf0) >> 4, rev & 0xf);
> +	return drm_dp_dpcd_read(&intel_dp->aux, base, desc, sizeof(*desc)) ==
> +	       sizeof(*desc);
>  }
>  
> -static void intel_dp_print_sw_revision(struct intel_dp *intel_dp)
> +static bool intel_dp_read_desc(struct intel_dp *intel_dp)
>  {
> -	uint8_t rev[2];
> -	int len;
> +	struct intel_dp_desc *desc = &intel_dp->desc;
> +	bool oui_sup = intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] &
> +		       DP_OUI_SUPPORT;
> +	int dev_id_len;
>  
> -	if (!drm_dp_is_branch(intel_dp->dpcd))
> -		return;
> +	if (!__intel_dp_read_desc(intel_dp, desc))
> +		return false;
>  
> -	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_SW_REV, &rev, 2);
> -	if (len < 0)
> -		return;
> +	dev_id_len = strnlen(desc->device_id, sizeof(desc->device_id));
> +	DRM_DEBUG_KMS("DP %s: OUI %*phD%s dev-ID %*pE HW-rev %d.%d SW-rev %d.%d\n",
> +		      drm_dp_is_branch(intel_dp->dpcd) ? "branch" : "sink",
> +		      (int)sizeof(desc->oui), desc->oui, oui_sup ? "" : "(NS)",
> +		      dev_id_len, desc->device_id,
> +		      desc->hw_rev >> 4, desc->hw_rev & 0xf,
> +		      desc->sw_major_rev, desc->sw_minor_rev);
>  
> -	DRM_DEBUG_KMS("sink sw revision: %d.%d\n", rev[0], rev[1]);
> +	return true;
>  }
>  
>  static int rate_to_index(int find, const int *rates)
> @@ -3621,23 +3622,6 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	return true;
>  }
>  
> -static void
> -intel_dp_probe_oui(struct intel_dp *intel_dp)
> -{
> -	bool is_branch = drm_dp_is_branch(intel_dp->dpcd);
> -	u8 buf[3];
> -
> -	if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
> -		return;
> -
> -	if (drm_dp_dpcd_read(&intel_dp->aux,
> -			     is_branch ? DP_BRANCH_OUI : DP_SINK_OUI,
> -			     buf, 3) == 3)
> -		DRM_DEBUG_KMS("%s OUI: %02hx%02hx%02hx\n",
> -			      is_branch ? "Branch" : "Sink",
> -			      buf[0], buf[1], buf[2]);
> -}
> -
>  static bool
>  intel_dp_can_mst(struct intel_dp *intel_dp)
>  {
> @@ -4416,10 +4400,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>  
>  	intel_dp_print_rates(intel_dp);
>  
> -	intel_dp_probe_oui(intel_dp);
> -
> -	intel_dp_print_hw_revision(intel_dp);
> -	intel_dp_print_sw_revision(intel_dp);
> +	intel_dp_read_desc(intel_dp);
>  
>  	intel_dp_configure_mst(intel_dp);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 16b33f5..4c9f953 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -883,6 +883,14 @@ enum link_m_n_set {
>  	M2_N2
>  };
>  
> +struct intel_dp_desc {
> +	u8 oui[3];
> +	u8 device_id[6];
> +	u8 hw_rev;
> +	u8 sw_major_rev;
> +	u8 sw_minor_rev;
> +} __packed;
> +
>  struct intel_dp {
>  	i915_reg_t output_reg;
>  	i915_reg_t aux_ch_ctl_reg;
> @@ -905,6 +913,8 @@ struct intel_dp {
>  	/* sink rates as reported by DP_SUPPORTED_LINK_RATES */
>  	uint8_t num_sink_rates;
>  	int sink_rates[DP_MAX_SUPPORTED_RATES];
> +	/* sink or branch descriptor */
> +	struct intel_dp_desc desc;
>  	struct drm_dp_aux aux;
>  	uint8_t train_set[4];
>  	int panel_power_up_delay;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 8/8] drm/i915/lspcon: Add workaround for resuming in PCON mode

[Jani Nikula]

On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> On my APL the LSPCON firmware resumes in PCON mode as opposed to the
> expected LS mode. It also appears to be in a state where AUX DPCD reads
> will succeed but return garbage recovering only after a few hundreds of
> milliseconds. After the recovery time DPCD reads will result in the
> correct values and things will continue to work. If I2C over AUX is
> attempted during this recovery time (implying an AUX write transaction)
> the firmware won't recover and will stay in this broken state.
>
> As a workaround check if the firmware is in PCON state after resume and
> if so wait until the correct DPCD values are returned. For this we
> compare the branch descriptor with the one we cached during init time.
> If the firmware was in the LS state, we skip the w/a and continue as
> before.
>
> v2:
> - Use the DP descriptor value cached in intel_dp. (Jani)
> - Get to intel_dp using container_of(), instead of a cached ptr.
>   (Shashank)
> - Use usleep_range() instead of msleep().
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98353
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

I don't think we properly understand what's going on with the device. In
the mean time, limp on with this if this fixes stuff.

Acked-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dp.c     |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h    |  3 +++
>  drivers/gpu/drm/i915/intel_lspcon.c | 37 ++++++++++++++++++++++++++++++++++++-
>  3 files changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 043993f..e9be955 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1451,7 +1451,7 @@ static void intel_dp_print_rates(struct intel_dp *intel_dp)
>  	DRM_DEBUG_KMS("common rates: %s\n", str);
>  }
>  
> -static bool
> +bool
>  __intel_dp_read_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc)
>  {
>  	u32 base = drm_dp_is_branch(intel_dp->dpcd) ? DP_BRANCH_OUI :
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 45f55b5..f2b6c59 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -974,6 +974,7 @@ struct intel_dp {
>  struct intel_lspcon {
>  	bool active;
>  	enum drm_lspcon_mode mode;
> +	bool desc_valid;
>  };
>  
>  struct intel_digital_port {
> @@ -1461,6 +1462,8 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
>  }
>  
>  bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
> +bool __intel_dp_read_desc(struct intel_dp *intel_dp,
> +			  struct intel_dp_desc *desc);
>  bool intel_dp_read_desc(struct intel_dp *intel_dp);
>  
>  /* intel_dp_aux_backlight.c */
> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> index 3dc5a0b..daa5234 100644
> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> @@ -97,8 +97,43 @@ static bool lspcon_probe(struct intel_lspcon *lspcon)
>  	return true;
>  }
>  
> +static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon)
> +{
> +	struct intel_dp *intel_dp = lspcon_to_intel_dp(lspcon);
> +	unsigned long start = jiffies;
> +
> +	if (!lspcon->desc_valid)
> +		return;
> +
> +	while (1) {
> +		struct intel_dp_desc desc;
> +
> +		/*
> +		 * The w/a only applies in PCON mode and we don't expect any
> +		 * AUX errors.
> +		 */
> +		if (!__intel_dp_read_desc(intel_dp, &desc))
> +			return;
> +
> +		if (!memcmp(&intel_dp->desc, &desc, sizeof(desc))) {
> +			DRM_DEBUG_KMS("LSPCON recovering in PCON mode after %u ms\n",
> +				      jiffies_to_msecs(jiffies - start));
> +			return;
> +		}
> +
> +		if (time_after(jiffies, start + msecs_to_jiffies(1000)))
> +			break;
> +
> +		usleep_range(10000, 15000);
> +	}
> +
> +	DRM_DEBUG_KMS("LSPCON DP descriptor mismatch after resume\n");
> +}
> +
>  void lspcon_resume(struct intel_lspcon *lspcon)
>  {
> +	lspcon_resume_in_pcon_wa(lspcon);
> +
>  	if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON, true))
>  		DRM_ERROR("LSPCON resume failed\n");
>  	else
> @@ -143,7 +178,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>  		return false;
>  	}
>  
> -	intel_dp_read_desc(dp);
> +	lspcon->desc_valid = intel_dp_read_desc(dp);
>  
>  	DRM_DEBUG_KMS("Success: LSPCON init\n");
>  	return true;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: warning for drm/i915/lspcon: Work around resume failure (rev2)

[Imre Deak]

On Tue, 2016-10-25 at 14:46 +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915/lspcon: Work around resume failure (rev2)
> URL   : https://patchwork.freedesktop.org/series/14280/
> State : warning
> 
> == Summary ==
> 
> Series 14280v2 drm/i915/lspcon: Work around resume failure
> https://patchwork.freedesktop.org/api/1.0/series/14280/revisions/2/mb
> ox/
> 
> Test drv_module_reload_basic:
>                 dmesg-warn -> PASS       (fi-skl-6770hq)
>                 pass       -> DMESG-WARN (fi-skl-6700hq)

This is due to the bug's persistence over warm reboot. Since I pushed
this w/a there's been two DRM_CI runs without these errors on this
machine (whereas before either this or one of the S3 tests always
failed).

> Test gem_exec_suspend:
>         Subgroup basic-s3:
>                 dmesg-warn -> PASS       (fi-skl-6770hq)
>                 dmesg-warn -> PASS       (fi-skl-6700hq)
> Test kms_flip:
>         Subgroup basic-flip-vs-wf_vblank:
>                 pass       -> SKIP       (fi-byt-n2820)
> Test kms_frontbuffer_tracking:
>         Subgroup basic:
>                 fail       -> PASS       (fi-skl-6770hq)
> Test kms_pipe_crc_basic:
>         Subgroup suspend-read-crc-pipe-a:
>                 dmesg-warn -> PASS       (fi-skl-6700hq)
>         Subgroup suspend-read-crc-pipe-b:
>                 pass       -> DMESG-WARN (fi-skl-6770hq)

*ERROR* failed to inform PCU about cdclk change
https://bugs.freedesktop.org/show_bug.cgi?id=97929

Thanks for the review, I pushed the patchset to -dinq.

>                 dmesg-warn -> PASS       (fi-skl-6700hq)
>         Subgroup suspend-read-crc-pipe-c:
>                 dmesg-warn -> PASS       (fi-skl-6700hq)
> 
> fi-bdw-
> 5557u     total:246  pass:231  dwarn:0   dfail:0   fail:0   skip:15 
> fi-bsw-
> n3050     total:246  pass:204  dwarn:0   dfail:0   fail:0   skip:42 
> fi-bxt-
> t5700     total:246  pass:216  dwarn:0   dfail:0   fail:0   skip:30 
> fi-byt-
> j1900     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
> fi-byt-
> n2820     total:246  pass:210  dwarn:0   dfail:0   fail:0   skip:36 
> fi-hsw-
> 4770      total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
> fi-hsw-
> 4770r     total:246  pass:223  dwarn:0   dfail:0   fail:0   skip:23 
> fi-ilk-
> 650       total:246  pass:185  dwarn:0   dfail:0   fail:0   skip:61 
> fi-ivb-
> 3520m     total:246  pass:220  dwarn:0   dfail:0   fail:0   skip:26 
> fi-ivb-
> 3770      total:246  pass:220  dwarn:0   dfail:0   fail:0   skip:26 
> fi-kbl-
> 7200u     total:246  pass:222  dwarn:0   dfail:0   fail:0   skip:24 
> fi-skl-
> 6260u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
> fi-skl-
> 6700hq    total:246  pass:222  dwarn:1   dfail:0   fail:0   skip:23 
> fi-skl-
> 6700k     total:246  pass:222  dwarn:1   dfail:0   fail:0   skip:23 
> fi-skl-
> 6770hq    total:246  pass:231  dwarn:1   dfail:0   fail:0   skip:14 
> fi-snb-
> 2520m     total:246  pass:209  dwarn:0   dfail:0   fail:0   skip:37 
> fi-snb-
> 2600      total:246  pass:208  dwarn:0   dfail:0   fail:0   skip:38 
> 
> 3ce99d02d8f2b7d1414d20d5972f8e277b33693e drm-intel-nightly: 2016y-
> 10m-25d-13h-55m-46s UTC integration manifest
> ab50b11 drm/i915/lspcon: Add workaround for resuming in PCON mode
> 607efb9 drm/i915/lspcon: Get DDC adapter via container_of() instead
> of cached ptr
> e281e36 drm/i915/dp: Read DP descriptor for eDP and LSPCON too
> 742a374 drm/i915/lspcon: Fail LSPCON probe if the start of DPCD can't
> be read
> e3a2cc0 drm/i915/dp: Print full branch/sink descriptor
> d6131ca drm/i915/dp: Print only sink or branch specific OUI based on
> dev type
> 3c3c2a2 drm/i915/dp: Remove debug dependency of DPCD SW/HW revision
> read
> 
> Full results at https://intel-gfx-ci.01.org/CI/Patchwork_2810/
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2810/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx