All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v7 0/5] drm/i915: DP branch devices
@ 2016-08-08 13:00 Mika Kahola
  2016-08-08 13:00 ` [PATCH v7 1/5] drm: Read DP branch device HW revision Mika Kahola
                   ` (5 more replies)
  0 siblings, 6 replies; 20+ messages in thread
From: Mika Kahola @ 2016-08-08 13:00 UTC (permalink / raw)
  To: dri-devel; +Cc: daniel.vetter, intel-gfx

Prep work for DP branch device handling

This series of patches reads DPCD register 0x80h for receiver
capabilities for DP branch devices. The branch device types are
converters for the following standards

 - DP to VGA
 - DP to DVI
 - DP to HDMI
 - DP++ dual mode
 - Wireless WiGig
 
DPCD register defines max pixel rate for VGA dongles. This
check is carried out during mode validation. 

[1] git://github.com/mkahola/drm-intel-mika.git dp_branch_device

v2: DPCD register read outs moved to drm (Ville, Daniel)
v3: Max pixel rate computation moved to drm (Daniel)
v4: Use of drm_dp_helper routines to collect data (Ville)
v5: Remove duplicate code and unnecessary functions from drm_dp_helper (Ville)
v6: Rebase and i915_debugfs cleanup
v7: Structure cleanups and initial step to move DP debugging info to drm_dp_helpers

Mika Kahola (5):
  drm: Read DP branch device HW revision
  drm: Read DP branch device SW revision
  drm/i915: Check pixel rate for DP to VGA dongle
  drm: Update bits per component for display info
  drm: Add DP branch device info on debugfs

 drivers/gpu/drm/drm_dp_helper.c     | 158 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_debugfs.c |   2 +
 drivers/gpu/drm/i915/intel_dp.c     |  28 ++++++-
 drivers/gpu/drm/i915/intel_drv.h    |   1 +
 include/drm/drm_dp_helper.h         |  20 +++++
 5 files changed, 208 insertions(+), 1 deletion(-)

-- 
1.9.1

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

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

* [PATCH v7 1/5] drm: Read DP branch device HW revision
  2016-08-08 13:00 [PATCH v7 0/5] drm/i915: DP branch devices Mika Kahola
@ 2016-08-08 13:00 ` Mika Kahola
  2016-08-11  7:10   ` Ville Syrjälä
  2016-08-08 13:00 ` [PATCH v7 2/5] drm: Read DP branch device SW revision Mika Kahola
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 20+ messages in thread
From: Mika Kahola @ 2016-08-08 13:00 UTC (permalink / raw)
  To: dri-devel; +Cc: daniel.vetter, intel-gfx

HW revision is mandatory field for DisplayPort branch
devices. This is defined in DPCD register field 0x509.

v2: move drm_dp_ds_revision structure to be part of
    drm_dp_link structure (Daniel)

Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
 drivers/gpu/drm/drm_dp_helper.c  | 27 +++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 include/drm/drm_dp_helper.h      |  9 +++++++++
 3 files changed, 37 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 75b2873..5fecdc1 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -514,6 +514,33 @@ int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
 EXPORT_SYMBOL(drm_dp_downstream_max_bpc);
 
 /**
+ * drm_dp_downstream_hw_rev() - read DP branch device HW revision
+ * @aux: DisplayPort AUX channel
+ *
+ * Returns 0 on succes or negative error code on failure
+ */
+int drm_dp_downstream_hw_rev(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
+			     struct drm_dp_aux *aux, struct drm_dp_link *link)
+{
+	uint8_t tmp;
+	int err;
+
+	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
+		return -EINVAL;
+
+	err = drm_dp_dpcd_read(aux, DP_BRANCH_HW_REV, &tmp, 1);
+
+	if (err < 0)
+		return err;
+
+	link->ds_hw_rev.major = (tmp & 0xf0) >> 4;
+	link->ds_hw_rev.minor = tmp & 0xf;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_dp_downstream_hw_rev);
+
+/**
  * drm_dp_downstream_id() - identify branch device
  * @aux: DisplayPort AUX channel
  *
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e74d851..a6eccf5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -865,6 +865,7 @@ struct intel_dp {
 	uint8_t num_sink_rates;
 	int sink_rates[DP_MAX_SUPPORTED_RATES];
 	struct drm_dp_aux aux;
+	struct drm_dp_link link;
 	uint8_t train_set[4];
 	int panel_power_up_delay;
 	int panel_power_down_delay;
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 8e1fe58..1127948 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -446,6 +446,7 @@
 #define DP_SINK_OUI			    0x400
 #define DP_BRANCH_OUI			    0x500
 #define DP_BRANCH_ID                        0x503
+#define DP_BRANCH_HW_REV                    0x509
 
 #define DP_SET_POWER                        0x600
 # define DP_SET_POWER_D0                    0x1
@@ -803,11 +804,17 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
  */
 #define DP_LINK_CAP_ENHANCED_FRAMING (1 << 0)
 
+struct drm_dp_ds_revision {
+	int major;
+	int minor;
+};
+
 struct drm_dp_link {
 	unsigned char revision;
 	unsigned int rate;
 	unsigned int num_lanes;
 	unsigned long capabilities;
+	struct drm_dp_ds_revision ds_hw_rev;
 };
 
 int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link);
@@ -819,6 +826,8 @@ int drm_dp_downstream_max_clock(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
 int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
 			      const u8 port_cap[4]);
 int drm_dp_downstream_id(struct drm_dp_aux *aux, char id[6]);
+int drm_dp_downstream_hw_rev(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
+			     struct drm_dp_aux *aux, struct drm_dp_link *link);
 
 void drm_dp_aux_init(struct drm_dp_aux *aux);
 int drm_dp_aux_register(struct drm_dp_aux *aux);
-- 
1.9.1

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

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

* [PATCH v7 2/5] drm: Read DP branch device SW revision
  2016-08-08 13:00 [PATCH v7 0/5] drm/i915: DP branch devices Mika Kahola
  2016-08-08 13:00 ` [PATCH v7 1/5] drm: Read DP branch device HW revision Mika Kahola
@ 2016-08-08 13:00 ` Mika Kahola
  2016-08-08 13:00 ` [PATCH v7 3/5] drm/i915: Check pixel rate for DP to VGA dongle Mika Kahola
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 20+ messages in thread
From: Mika Kahola @ 2016-08-08 13:00 UTC (permalink / raw)
  To: dri-devel; +Cc: daniel.vetter, intel-gfx

SW revision is mandatory field for DisplayPort branch
devices. This is defined in DPCD register field 0x50A.

v2: move drm_dp_ds_revision structure to be part of
    drm_dp_link structure (Daniel)

Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 27 +++++++++++++++++++++++++++
 include/drm/drm_dp_helper.h     |  5 +++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 5fecdc1..1f36016 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -541,6 +541,33 @@ int drm_dp_downstream_hw_rev(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
 EXPORT_SYMBOL(drm_dp_downstream_hw_rev);
 
 /**
+ * drm_dp_downstream_sw_rev() - read DP branch device SW revision
+ * @aux: DisplayPort AUX channel
+ *
+ * Returns SW revision on success or negative error code on failure
+ */
+int drm_dp_downstream_sw_rev(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
+			     struct drm_dp_aux *aux, struct drm_dp_link *link)
+{
+	uint8_t tmp[2];
+	int err;
+
+	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
+		return -EINVAL;
+
+	err = drm_dp_dpcd_read(aux, DP_BRANCH_SW_REV, tmp, 2);
+
+	if (err < 0)
+		return err;
+
+	link->ds_sw_rev.major = tmp[0];
+	link->ds_sw_rev.minor = tmp[1];
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_dp_downstream_sw_rev);
+
+/**
  * drm_dp_downstream_id() - identify branch device
  * @aux: DisplayPort AUX channel
  *
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 1127948..45366aa 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -447,6 +447,7 @@
 #define DP_BRANCH_OUI			    0x500
 #define DP_BRANCH_ID                        0x503
 #define DP_BRANCH_HW_REV                    0x509
+#define DP_BRANCH_SW_REV                    0x50A
 
 #define DP_SET_POWER                        0x600
 # define DP_SET_POWER_D0                    0x1
@@ -815,6 +816,7 @@ struct drm_dp_link {
 	unsigned int num_lanes;
 	unsigned long capabilities;
 	struct drm_dp_ds_revision ds_hw_rev;
+	struct drm_dp_ds_revision ds_sw_rev;
 };
 
 int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link);
@@ -829,6 +831,9 @@ int drm_dp_downstream_id(struct drm_dp_aux *aux, char id[6]);
 int drm_dp_downstream_hw_rev(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
 			     struct drm_dp_aux *aux, struct drm_dp_link *link);
 
+int drm_dp_downstream_sw_rev(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
+			     struct drm_dp_aux *aux, struct drm_dp_link *link);
+
 void drm_dp_aux_init(struct drm_dp_aux *aux);
 int drm_dp_aux_register(struct drm_dp_aux *aux);
 void drm_dp_aux_unregister(struct drm_dp_aux *aux);
-- 
1.9.1

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

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

* [PATCH v7 3/5] drm/i915: Check pixel rate for DP to VGA dongle
  2016-08-08 13:00 [PATCH v7 0/5] drm/i915: DP branch devices Mika Kahola
  2016-08-08 13:00 ` [PATCH v7 1/5] drm: Read DP branch device HW revision Mika Kahola
  2016-08-08 13:00 ` [PATCH v7 2/5] drm: Read DP branch device SW revision Mika Kahola
@ 2016-08-08 13:00 ` Mika Kahola
  2016-08-11  7:18   ` Ville Syrjälä
  2016-08-08 13:00 ` [PATCH v7 4/5] drm: Update bits per component for display info Mika Kahola
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 20+ messages in thread
From: Mika Kahola @ 2016-08-08 13:00 UTC (permalink / raw)
  To: dri-devel; +Cc: daniel.vetter, intel-gfx

Filter out a mode that exceeds the max pixel rate setting
for DP to VGA dongle. This is defined in DPCD register 0x81
if detailed cap info i.e. info field is 4 bytes long and
it is available for DP downstream port.

The register defines the pixel rate divided by 8 in MP/s.

v2: DPCD read outs and computation moved to drm (Ville, Daniel)
v3: Sink pixel rate computation moved to drm_dp_max_sink_dotclock()
    function (Daniel)
v4: Use of drm_dp_helper.c routines to compute max pixel clock (Ville)
v5: Use of intel_dp->downstream_ports to read out port capabilities.
    Code restructuring (Ville)
v6: Move DP branch device check to drm_dp_helper.c (Daniel)

Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 21b04c3..e990c8b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -190,6 +190,26 @@ intel_dp_max_data_rate(int max_link_clock, int max_lanes)
 	return (max_link_clock * max_lanes * 8) / 10;
 }
 
+static int
+intel_dp_downstream_max_dotclock(struct intel_dp *intel_dp, int dotclk)
+{
+	int ds_dotclk;
+	int type;
+
+	ds_dotclk = drm_dp_downstream_max_clock(intel_dp->dpcd,
+						intel_dp->downstream_ports);
+
+	if (ds_dotclk == 0)
+		return dotclk;
+
+	type = intel_dp->downstream_ports[0] & DP_DS_PORT_TYPE_MASK;
+
+	if (type != DP_DS_PORT_TYPE_VGA)
+		return dotclk;
+
+	return min(dotclk, ds_dotclk);
+}
+
 static enum drm_mode_status
 intel_dp_mode_valid(struct drm_connector *connector,
 		    struct drm_display_mode *mode)
@@ -199,7 +219,10 @@ intel_dp_mode_valid(struct drm_connector *connector,
 	struct drm_display_mode *fixed_mode = intel_connector->panel.fixed_mode;
 	int target_clock = mode->clock;
 	int max_rate, mode_rate, max_lanes, max_link_clock;
-	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
+	int max_dotclk;
+
+	max_dotclk = intel_dp_downstream_max_dotclock(intel_dp,
+						      to_i915(connector->dev)->max_dotclk_freq);
 
 	if (is_edp(intel_dp) && fixed_mode) {
 		if (mode->hdisplay > fixed_mode->hdisplay)
-- 
1.9.1

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

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

* [PATCH v7 4/5] drm: Update bits per component for display info
  2016-08-08 13:00 [PATCH v7 0/5] drm/i915: DP branch devices Mika Kahola
                   ` (2 preceding siblings ...)
  2016-08-08 13:00 ` [PATCH v7 3/5] drm/i915: Check pixel rate for DP to VGA dongle Mika Kahola
@ 2016-08-08 13:00 ` Mika Kahola
  2016-08-11  7:22   ` Ville Syrjälä
  2016-08-08 13:00 ` [PATCH v7 5/5] drm: Add DP branch device info on debugfs Mika Kahola
  2016-08-08 13:10 ` ✗ Ro.CI.BAT: failure for drm/i915: DP branch devices (rev7) Patchwork
  5 siblings, 1 reply; 20+ messages in thread
From: Mika Kahola @ 2016-08-08 13:00 UTC (permalink / raw)
  To: dri-devel; +Cc: daniel.vetter, intel-gfx

DisplayPort branch device may define max supported bits per
component. Update display info based on this value if bpc
is defined.

v2: cleanup to match the drm_dp_helper.c patches introduced
    earlier in this series
v3: Fill bpc for connector's display info in separate
    drm_dp_helper function (Daniel)

Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 24 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c |  3 +++
 include/drm/drm_dp_helper.h     |  4 ++++
 3 files changed, 31 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 1f36016..a2c46ed 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -514,6 +514,30 @@ int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
 EXPORT_SYMBOL(drm_dp_downstream_max_bpc);
 
 /**
+ * drm_dp_downstream_update_max_bpc() - extract branch device max
+ *                                      bits per component and update
+ *                                      connector max bpc
+ * @dpcd: DisplayPort configuration data
+ * @port_cap: port capabilities
+ * @connector: Connector info
+ * Returns current max bpc on success
+ */
+int drm_dp_downstream_update_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
+				     const u8 port_cap[4],
+				     struct drm_connector *connector)
+{
+	if (dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT) {
+		int bpc = drm_dp_downstream_max_bpc(dpcd, port_cap);
+
+		if (bpc > 0)
+			connector->display_info.bpc = bpc;
+	}
+
+	return connector->display_info.bpc;
+}
+EXPORT_SYMBOL(drm_dp_downstream_update_max_bpc);
+
+/**
  * drm_dp_downstream_hw_rev() - read DP branch device HW revision
  * @aux: DisplayPort AUX channel
  *
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e990c8b..763e2f5 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3985,6 +3985,9 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 	uint8_t *dpcd = intel_dp->dpcd;
 	uint8_t type;
 
+	drm_dp_downstream_update_max_bpc(dpcd, intel_dp->downstream_ports,
+					 &intel_dp->attached_connector->base);
+
 	if (!intel_dp_get_dpcd(intel_dp))
 		return connector_status_disconnected;
 
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 45366aa..5491a9b 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -26,6 +26,7 @@
 #include <linux/types.h>
 #include <linux/i2c.h>
 #include <linux/delay.h>
+#include <drm/drm_crtc.h>
 
 /*
  * Unless otherwise noted, all values are from the DP 1.1a spec.  Note that
@@ -827,6 +828,9 @@ int drm_dp_downstream_max_clock(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
 				const u8 port_cap[4]);
 int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
 			      const u8 port_cap[4]);
+int drm_dp_downstream_update_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
+				     const u8 port_cap[4],
+				     struct drm_connector *connector);
 int drm_dp_downstream_id(struct drm_dp_aux *aux, char id[6]);
 int drm_dp_downstream_hw_rev(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
 			     struct drm_dp_aux *aux, struct drm_dp_link *link);
-- 
1.9.1

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

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

* [PATCH v7 5/5] drm: Add DP branch device info on debugfs
  2016-08-08 13:00 [PATCH v7 0/5] drm/i915: DP branch devices Mika Kahola
                   ` (3 preceding siblings ...)
  2016-08-08 13:00 ` [PATCH v7 4/5] drm: Update bits per component for display info Mika Kahola
@ 2016-08-08 13:00 ` Mika Kahola
  2016-08-08 13:10 ` ✗ Ro.CI.BAT: failure for drm/i915: DP branch devices (rev7) Patchwork
  5 siblings, 0 replies; 20+ messages in thread
From: Mika Kahola @ 2016-08-08 13:00 UTC (permalink / raw)
  To: dri-devel; +Cc: daniel.vetter, intel-gfx

Read DisplayPort branch device info from through debugfs
interface.

v2: use drm_dp_helper routines to collect data
v3: cleanup to match the drm_dp_helper.c patches introduced
    earlier in this series
v4: move DP branch device info to function 'intel_dp_branch_device_info()'
v5: initial step to move debugging info from intel_dp. to drm_dp_helper.c (Daniel)

Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
 drivers/gpu/drm/drm_dp_helper.c     | 80 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_debugfs.c |  2 +
 include/drm/drm_dp_helper.h         |  2 +
 3 files changed, 84 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index a2c46ed..47bbb5e 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -603,6 +603,86 @@ int drm_dp_downstream_id(struct drm_dp_aux *aux, char id[6])
 }
 EXPORT_SYMBOL(drm_dp_downstream_id);
 
+/**
+ * drm_dp_downstream_debug() - debug DP branch devices
+ * @m: pointer for debugfs file
+ * @dpcd: DisplayPort configuration data
+ * @port_cap: port capabilities
+ * @aux: DisplayPort AUX channel
+ *
+ */
+void drm_dp_downstream_debug(struct seq_file *m, const u8 dpcd[DP_RECEIVER_CAP_SIZE],
+			     const u8 port_cap[4], struct drm_dp_aux *aux)
+{
+	struct drm_dp_link link;
+	bool detailed_cap_info = dpcd[DP_DOWNSTREAMPORT_PRESENT] &
+		DP_DETAILED_CAP_INFO_AVAILABLE;
+	int clk;
+	int bpc;
+	char id[6];
+	int type = port_cap[0] & DP_DS_PORT_TYPE_MASK;
+	bool branch_device = dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT;
+
+	seq_printf(m, "\tDP branch device present: %s\n", branch_device ? "yes" : "no");
+
+	if (!branch_device)
+		return;
+
+	switch (type) {
+	case DP_DS_PORT_TYPE_DP:
+		seq_printf(m, "\t\tType: DisplayPort\n");
+		break;
+	case DP_DS_PORT_TYPE_VGA:
+		seq_printf(m, "\t\tType: VGA\n");
+		break;
+	case DP_DS_PORT_TYPE_DVI:
+		seq_printf(m, "\t\tType: DVI\n");
+		break;
+	case DP_DS_PORT_TYPE_HDMI:
+		seq_printf(m, "\t\tType: HDMI\n");
+		break;
+	case DP_DS_PORT_TYPE_NON_EDID:
+		seq_printf(m, "\t\tType: others without EDID support\n");
+		break;
+	case DP_DS_PORT_TYPE_DP_DUALMODE:
+		seq_printf(m, "\t\tType: DP++\n");
+		break;
+	case DP_DS_PORT_TYPE_WIRELESS:
+		seq_printf(m, "\t\tType: Wireless\n");
+		break;
+	default:
+		seq_printf(m, "\t\tType: N/A\n");
+	}
+
+	drm_dp_downstream_id(aux, id);
+	seq_printf(m, "\t\tID: %s\n", id);
+
+	if (drm_dp_downstream_hw_rev(dpcd, aux, &link) == 0)
+		seq_printf(m, "\t\tHW revision: %.2d.%.2d\n", link.ds_hw_rev.major,
+			   link.ds_hw_rev.minor);
+
+	if (drm_dp_downstream_sw_rev(dpcd, aux, &link) == 0)
+		seq_printf(m, "\t\tSW revision: %.2d.%.2d\n", link.ds_sw_rev.major,
+			   link.ds_sw_rev.minor);
+
+	if (detailed_cap_info) {
+		clk = drm_dp_downstream_max_clock(dpcd, port_cap);
+
+		if (clk > 0) {
+			if (type == DP_DS_PORT_TYPE_VGA)
+				seq_printf(m, "\t\tMax dot clock: %d kHz\n", clk);
+			else
+				seq_printf(m, "\t\tMax TMDS clock: %d kHz\n", clk);
+		}
+
+		bpc = drm_dp_downstream_max_bpc(dpcd, port_cap);
+
+		if (bpc > 0)
+			seq_printf(m, "\t\tMax bpc: %d\n", bpc);
+	}
+}
+EXPORT_SYMBOL(drm_dp_downstream_debug);
+
 /*
  * I2C-over-AUX implementation
  */
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 531ca02..3747f67 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2959,6 +2959,8 @@ static void intel_dp_info(struct seq_file *m,
 	seq_printf(m, "\taudio support: %s\n", yesno(intel_dp->has_audio));
 	if (intel_connector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
 		intel_panel_info(m, &intel_connector->panel);
+	drm_dp_downstream_debug(m, intel_dp->dpcd, intel_dp->downstream_ports,
+				&intel_dp->aux);
 }
 
 static void intel_hdmi_info(struct seq_file *m,
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 5491a9b..85123ac 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -837,6 +837,8 @@ int drm_dp_downstream_hw_rev(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
 
 int drm_dp_downstream_sw_rev(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
 			     struct drm_dp_aux *aux, struct drm_dp_link *link);
+void drm_dp_downstream_debug(struct seq_file *m, const u8 dpcd[DP_RECEIVER_CAP_SIZE],
+			     const u8 port_cap[4], struct drm_dp_aux *aux);
 
 void drm_dp_aux_init(struct drm_dp_aux *aux);
 int drm_dp_aux_register(struct drm_dp_aux *aux);
-- 
1.9.1

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

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

* ✗ Ro.CI.BAT: failure for drm/i915: DP branch devices (rev7)
  2016-08-08 13:00 [PATCH v7 0/5] drm/i915: DP branch devices Mika Kahola
                   ` (4 preceding siblings ...)
  2016-08-08 13:00 ` [PATCH v7 5/5] drm: Add DP branch device info on debugfs Mika Kahola
@ 2016-08-08 13:10 ` Patchwork
  5 siblings, 0 replies; 20+ messages in thread
From: Patchwork @ 2016-08-08 13:10 UTC (permalink / raw)
  To: Mika Kahola; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: DP branch devices (rev7)
URL   : https://patchwork.freedesktop.org/series/6658/
State : failure

== Summary ==

Applying: drm: Read DP branch device HW revision
fatal: sha1 information is lacking or useless (drivers/gpu/drm/drm_dp_helper.c).
error: could not build fake ancestor
Patch failed at 0001 drm: Read DP branch device HW revision
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

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

* Re: [PATCH v7 1/5] drm: Read DP branch device HW revision
  2016-08-08 13:00 ` [PATCH v7 1/5] drm: Read DP branch device HW revision Mika Kahola
@ 2016-08-11  7:10   ` Ville Syrjälä
  2016-08-11  8:14     ` Mika Kahola
  0 siblings, 1 reply; 20+ messages in thread
From: Ville Syrjälä @ 2016-08-11  7:10 UTC (permalink / raw)
  To: Mika Kahola; +Cc: daniel.vetter, intel-gfx, jim.bride, dri-devel

On Mon, Aug 08, 2016 at 04:00:26PM +0300, Mika Kahola wrote:
> HW revision is mandatory field for DisplayPort branch
> devices. This is defined in DPCD register field 0x509.

But what do we want to do with it? Me, I don't see a point in parsing a
bunch of stuff from the DPCD unless there's a real use case for it.
/dev/drm_dp_aux will provide all the debug aid we need if we need to
check random pieces of data from the DPCD, so even exposing this sort
of stuff via debugfs is IMO pretty pointless.

If there's a good reason for a debug print, then I think we could parse
something and dump it out so that it's always in the dmesg. But so far
I'm not aware of any bug that would have required to deal with different
hw revisions of things and whatnot.

> 
> v2: move drm_dp_ds_revision structure to be part of
>     drm_dp_link structure (Daniel)
> 
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c  | 27 +++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  include/drm/drm_dp_helper.h      |  9 +++++++++
>  3 files changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 75b2873..5fecdc1 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -514,6 +514,33 @@ int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
>  EXPORT_SYMBOL(drm_dp_downstream_max_bpc);
>  
>  /**
> + * drm_dp_downstream_hw_rev() - read DP branch device HW revision
> + * @aux: DisplayPort AUX channel
> + *
> + * Returns 0 on succes or negative error code on failure
> + */
> +int drm_dp_downstream_hw_rev(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> +			     struct drm_dp_aux *aux, struct drm_dp_link *link)
> +{
> +	uint8_t tmp;
> +	int err;
> +
> +	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
> +		return -EINVAL;
> +
> +	err = drm_dp_dpcd_read(aux, DP_BRANCH_HW_REV, &tmp, 1);
> +
> +	if (err < 0)
> +		return err;
> +
> +	link->ds_hw_rev.major = (tmp & 0xf0) >> 4;
> +	link->ds_hw_rev.minor = tmp & 0xf;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_dp_downstream_hw_rev);
> +
> +/**
>   * drm_dp_downstream_id() - identify branch device
>   * @aux: DisplayPort AUX channel
>   *
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e74d851..a6eccf5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -865,6 +865,7 @@ struct intel_dp {
>  	uint8_t num_sink_rates;
>  	int sink_rates[DP_MAX_SUPPORTED_RATES];
>  	struct drm_dp_aux aux;
> +	struct drm_dp_link link;
>  	uint8_t train_set[4];
>  	int panel_power_up_delay;
>  	int panel_power_down_delay;
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 8e1fe58..1127948 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -446,6 +446,7 @@
>  #define DP_SINK_OUI			    0x400
>  #define DP_BRANCH_OUI			    0x500
>  #define DP_BRANCH_ID                        0x503
> +#define DP_BRANCH_HW_REV                    0x509
>  
>  #define DP_SET_POWER                        0x600
>  # define DP_SET_POWER_D0                    0x1
> @@ -803,11 +804,17 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
>   */
>  #define DP_LINK_CAP_ENHANCED_FRAMING (1 << 0)
>  
> +struct drm_dp_ds_revision {
> +	int major;
> +	int minor;
> +};
> +
>  struct drm_dp_link {
>  	unsigned char revision;
>  	unsigned int rate;
>  	unsigned int num_lanes;
>  	unsigned long capabilities;
> +	struct drm_dp_ds_revision ds_hw_rev;
>  };
>  
>  int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link);
> @@ -819,6 +826,8 @@ int drm_dp_downstream_max_clock(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
>  int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
>  			      const u8 port_cap[4]);
>  int drm_dp_downstream_id(struct drm_dp_aux *aux, char id[6]);
> +int drm_dp_downstream_hw_rev(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> +			     struct drm_dp_aux *aux, struct drm_dp_link *link);
>  
>  void drm_dp_aux_init(struct drm_dp_aux *aux);
>  int drm_dp_aux_register(struct drm_dp_aux *aux);
> -- 
> 1.9.1

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

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

* Re: [PATCH v7 3/5] drm/i915: Check pixel rate for DP to VGA dongle
  2016-08-08 13:00 ` [PATCH v7 3/5] drm/i915: Check pixel rate for DP to VGA dongle Mika Kahola
@ 2016-08-11  7:18   ` Ville Syrjälä
  2016-08-11  9:43     ` Mika Kahola
  0 siblings, 1 reply; 20+ messages in thread
From: Ville Syrjälä @ 2016-08-11  7:18 UTC (permalink / raw)
  To: Mika Kahola; +Cc: daniel.vetter, intel-gfx, jim.bride, dri-devel

On Mon, Aug 08, 2016 at 04:00:28PM +0300, Mika Kahola wrote:
> Filter out a mode that exceeds the max pixel rate setting
> for DP to VGA dongle. This is defined in DPCD register 0x81
> if detailed cap info i.e. info field is 4 bytes long and
> it is available for DP downstream port.
> 
> The register defines the pixel rate divided by 8 in MP/s.
> 
> v2: DPCD read outs and computation moved to drm (Ville, Daniel)
> v3: Sink pixel rate computation moved to drm_dp_max_sink_dotclock()
>     function (Daniel)
> v4: Use of drm_dp_helper.c routines to compute max pixel clock (Ville)
> v5: Use of intel_dp->downstream_ports to read out port capabilities.
>     Code restructuring (Ville)
> v6: Move DP branch device check to drm_dp_helper.c (Daniel)
> 
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 21b04c3..e990c8b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -190,6 +190,26 @@ intel_dp_max_data_rate(int max_link_clock, int max_lanes)
>  	return (max_link_clock * max_lanes * 8) / 10;
>  }
>  
> +static int
> +intel_dp_downstream_max_dotclock(struct intel_dp *intel_dp, int dotclk)
> +{

I would just

{
	int max_dotclk = dev_priv->max_dotclk_freq;

	ds_max_dotclk = ...;
	if (ds_dotclk != 0)
		max_dotclk = min(max_dotclk, ds_max_dotclk);

	return max_dotclk;
}

> +	int ds_dotclk;
> +	int type;
> +
> +	ds_dotclk = drm_dp_downstream_max_clock(intel_dp->dpcd,
> +						intel_dp->downstream_ports);
> +
> +	if (ds_dotclk == 0)
> +		return dotclk;
> +
> +	type = intel_dp->downstream_ports[0] & DP_DS_PORT_TYPE_MASK;
> +	
> +	if (type != DP_DS_PORT_TYPE_VGA)
> +		return dotclk;

Why isn't drm_dp_downstream_max_clock() handling all of it already?
Why are we even checking for !=VGA?

> +
> +	return min(dotclk, ds_dotclk);
> +}
> +
>  static enum drm_mode_status
>  intel_dp_mode_valid(struct drm_connector *connector,
>  		    struct drm_display_mode *mode)
> @@ -199,7 +219,10 @@ intel_dp_mode_valid(struct drm_connector *connector,
>  	struct drm_display_mode *fixed_mode = intel_connector->panel.fixed_mode;
>  	int target_clock = mode->clock;
>  	int max_rate, mode_rate, max_lanes, max_link_clock;
> -	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
> +	int max_dotclk;
> +
> +	max_dotclk = intel_dp_downstream_max_dotclock(intel_dp,
> +						      to_i915(connector->dev)->max_dotclk_freq);
>  
>  	if (is_edp(intel_dp) && fixed_mode) {
>  		if (mode->hdisplay > fixed_mode->hdisplay)
> -- 
> 1.9.1

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

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

* Re: [PATCH v7 4/5] drm: Update bits per component for display info
  2016-08-08 13:00 ` [PATCH v7 4/5] drm: Update bits per component for display info Mika Kahola
@ 2016-08-11  7:22   ` Ville Syrjälä
  2016-08-11  8:53     ` Daniel Vetter
  0 siblings, 1 reply; 20+ messages in thread
From: Ville Syrjälä @ 2016-08-11  7:22 UTC (permalink / raw)
  To: Mika Kahola; +Cc: daniel.vetter, intel-gfx, dri-devel

On Mon, Aug 08, 2016 at 04:00:29PM +0300, Mika Kahola wrote:
> DisplayPort branch device may define max supported bits per
> component. Update display info based on this value if bpc
> is defined.
> 
> v2: cleanup to match the drm_dp_helper.c patches introduced
>     earlier in this series
> v3: Fill bpc for connector's display info in separate
>     drm_dp_helper function (Daniel)
> 
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 24 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c |  3 +++
>  include/drm/drm_dp_helper.h     |  4 ++++
>  3 files changed, 31 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 1f36016..a2c46ed 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -514,6 +514,30 @@ int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
>  EXPORT_SYMBOL(drm_dp_downstream_max_bpc);
>  
>  /**
> + * drm_dp_downstream_update_max_bpc() - extract branch device max
> + *                                      bits per component and update
> + *                                      connector max bpc
> + * @dpcd: DisplayPort configuration data
> + * @port_cap: port capabilities
> + * @connector: Connector info
> + * Returns current max bpc on success
> + */
> +int drm_dp_downstream_update_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> +				     const u8 port_cap[4],
> +				     struct drm_connector *connector)
> +{
> +	if (dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT) {
> +		int bpc = drm_dp_downstream_max_bpc(dpcd, port_cap);
> +
> +		if (bpc > 0)
> +			connector->display_info.bpc = bpc;
> +	}
> +
> +	return connector->display_info.bpc;
> +}
> +EXPORT_SYMBOL(drm_dp_downstream_update_max_bpc);
> +
> +/**
>   * drm_dp_downstream_hw_rev() - read DP branch device HW revision
>   * @aux: DisplayPort AUX channel
>   *
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e990c8b..763e2f5 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3985,6 +3985,9 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>  	uint8_t *dpcd = intel_dp->dpcd;
>  	uint8_t type;
>  
> +	drm_dp_downstream_update_max_bpc(dpcd, intel_dp->downstream_ports,
> +					 &intel_dp->attached_connector->base);
> +

And it will be promptly overwritten by the EDID parser. Like I said
before, touching display_info from elsewhere is a bad idea right now.
We need to lay out some real rules on how the EDID parser vs. DP
helpers must be called if we are to share that information between
the two.

For now, I would just utilize the DP helper in the compute_config
to limit the bpc.

>  	if (!intel_dp_get_dpcd(intel_dp))
>  		return connector_status_disconnected;
>  
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 45366aa..5491a9b 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -26,6 +26,7 @@
>  #include <linux/types.h>
>  #include <linux/i2c.h>
>  #include <linux/delay.h>
> +#include <drm/drm_crtc.h>
>  
>  /*
>   * Unless otherwise noted, all values are from the DP 1.1a spec.  Note that
> @@ -827,6 +828,9 @@ int drm_dp_downstream_max_clock(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
>  				const u8 port_cap[4]);
>  int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
>  			      const u8 port_cap[4]);
> +int drm_dp_downstream_update_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> +				     const u8 port_cap[4],
> +				     struct drm_connector *connector);
>  int drm_dp_downstream_id(struct drm_dp_aux *aux, char id[6]);
>  int drm_dp_downstream_hw_rev(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
>  			     struct drm_dp_aux *aux, struct drm_dp_link *link);
> -- 
> 1.9.1

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

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

* Re: [PATCH v7 1/5] drm: Read DP branch device HW revision
  2016-08-11  7:10   ` Ville Syrjälä
@ 2016-08-11  8:14     ` Mika Kahola
  2016-08-11  8:22       ` Ville Syrjälä
  0 siblings, 1 reply; 20+ messages in thread
From: Mika Kahola @ 2016-08-11  8:14 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: daniel.vetter, intel-gfx, dri-devel

On Thu, 2016-08-11 at 10:10 +0300, Ville Syrjälä wrote:
> On Mon, Aug 08, 2016 at 04:00:26PM +0300, Mika Kahola wrote:
> > HW revision is mandatory field for DisplayPort branch
> > devices. This is defined in DPCD register field 0x509.
> 
> But what do we want to do with it? Me, I don't see a point in parsing a
> bunch of stuff from the DPCD unless there's a real use case for it.
> /dev/drm_dp_aux will provide all the debug aid we need if we need to
> check random pieces of data from the DPCD, so even exposing this sort
> of stuff via debugfs is IMO pretty pointless.
> 
> If there's a good reason for a debug print, then I think we could parse
> something and dump it out so that it's always in the dmesg. But so far
> I'm not aware of any bug that would have required to deal with different
> hw revisions of things and whatnot.
> 
Digging up the HW and SW revisions are just for debugging purposes. My
idea here was that it would be handy if this information would be
available if we face a problem let's say with VGA dongle for example. It
is true that at the moment we don't have a single bug that would require
HW/SW revision information but maybe in the future we have one.

So, if we don't want to have this stuff in drm_dp_helpers and/or debugfs
I could make a change and print this info on dmesg when DP branch device
is plugged in.

Cheers,
Mika

> > 
> > v2: move drm_dp_ds_revision structure to be part of
> >     drm_dp_link structure (Daniel)
> > 
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> >  drivers/gpu/drm/drm_dp_helper.c  | 27 +++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> >  include/drm/drm_dp_helper.h      |  9 +++++++++
> >  3 files changed, 37 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > index 75b2873..5fecdc1 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -514,6 +514,33 @@ int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> >  EXPORT_SYMBOL(drm_dp_downstream_max_bpc);
> >  
> >  /**
> > + * drm_dp_downstream_hw_rev() - read DP branch device HW revision
> > + * @aux: DisplayPort AUX channel
> > + *
> > + * Returns 0 on succes or negative error code on failure
> > + */
> > +int drm_dp_downstream_hw_rev(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> > +			     struct drm_dp_aux *aux, struct drm_dp_link *link)
> > +{
> > +	uint8_t tmp;
> > +	int err;
> > +
> > +	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
> > +		return -EINVAL;
> > +
> > +	err = drm_dp_dpcd_read(aux, DP_BRANCH_HW_REV, &tmp, 1);
> > +
> > +	if (err < 0)
> > +		return err;
> > +
> > +	link->ds_hw_rev.major = (tmp & 0xf0) >> 4;
> > +	link->ds_hw_rev.minor = tmp & 0xf;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_dp_downstream_hw_rev);
> > +
> > +/**
> >   * drm_dp_downstream_id() - identify branch device
> >   * @aux: DisplayPort AUX channel
> >   *
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index e74d851..a6eccf5 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -865,6 +865,7 @@ struct intel_dp {
> >  	uint8_t num_sink_rates;
> >  	int sink_rates[DP_MAX_SUPPORTED_RATES];
> >  	struct drm_dp_aux aux;
> > +	struct drm_dp_link link;
> >  	uint8_t train_set[4];
> >  	int panel_power_up_delay;
> >  	int panel_power_down_delay;
> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > index 8e1fe58..1127948 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -446,6 +446,7 @@
> >  #define DP_SINK_OUI			    0x400
> >  #define DP_BRANCH_OUI			    0x500
> >  #define DP_BRANCH_ID                        0x503
> > +#define DP_BRANCH_HW_REV                    0x509
> >  
> >  #define DP_SET_POWER                        0x600
> >  # define DP_SET_POWER_D0                    0x1
> > @@ -803,11 +804,17 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
> >   */
> >  #define DP_LINK_CAP_ENHANCED_FRAMING (1 << 0)
> >  
> > +struct drm_dp_ds_revision {
> > +	int major;
> > +	int minor;
> > +};
> > +
> >  struct drm_dp_link {
> >  	unsigned char revision;
> >  	unsigned int rate;
> >  	unsigned int num_lanes;
> >  	unsigned long capabilities;
> > +	struct drm_dp_ds_revision ds_hw_rev;
> >  };
> >  
> >  int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link);
> > @@ -819,6 +826,8 @@ int drm_dp_downstream_max_clock(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> >  int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> >  			      const u8 port_cap[4]);
> >  int drm_dp_downstream_id(struct drm_dp_aux *aux, char id[6]);
> > +int drm_dp_downstream_hw_rev(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> > +			     struct drm_dp_aux *aux, struct drm_dp_link *link);
> >  
> >  void drm_dp_aux_init(struct drm_dp_aux *aux);
> >  int drm_dp_aux_register(struct drm_dp_aux *aux);
> > -- 
> > 1.9.1
> 


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

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

* Re: [PATCH v7 1/5] drm: Read DP branch device HW revision
  2016-08-11  8:14     ` Mika Kahola
@ 2016-08-11  8:22       ` Ville Syrjälä
  2016-08-11 17:21         ` Jim Bride
  0 siblings, 1 reply; 20+ messages in thread
From: Ville Syrjälä @ 2016-08-11  8:22 UTC (permalink / raw)
  To: Mika Kahola; +Cc: daniel.vetter, intel-gfx, dri-devel

On Thu, Aug 11, 2016 at 11:14:43AM +0300, Mika Kahola wrote:
> On Thu, 2016-08-11 at 10:10 +0300, Ville Syrjälä wrote:
> > On Mon, Aug 08, 2016 at 04:00:26PM +0300, Mika Kahola wrote:
> > > HW revision is mandatory field for DisplayPort branch
> > > devices. This is defined in DPCD register field 0x509.
> > 
> > But what do we want to do with it? Me, I don't see a point in parsing a
> > bunch of stuff from the DPCD unless there's a real use case for it.
> > /dev/drm_dp_aux will provide all the debug aid we need if we need to
> > check random pieces of data from the DPCD, so even exposing this sort
> > of stuff via debugfs is IMO pretty pointless.
> > 
> > If there's a good reason for a debug print, then I think we could parse
> > something and dump it out so that it's always in the dmesg. But so far
> > I'm not aware of any bug that would have required to deal with different
> > hw revisions of things and whatnot.
> > 
> Digging up the HW and SW revisions are just for debugging purposes. My
> idea here was that it would be handy if this information would be
> available if we face a problem let's say with VGA dongle for example. It
> is true that at the moment we don't have a single bug that would require
> HW/SW revision information but maybe in the future we have one.
> 
> So, if we don't want to have this stuff in drm_dp_helpers and/or debugfs
> I could make a change and print this info on dmesg when DP branch device
> is plugged in.

Perhaps. We do print out the OUI as well, so there is some precedent.

> 
> Cheers,
> Mika
> 
> > > 
> > > v2: move drm_dp_ds_revision structure to be part of
> > >     drm_dp_link structure (Daniel)
> > > 
> > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_dp_helper.c  | 27 +++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> > >  include/drm/drm_dp_helper.h      |  9 +++++++++
> > >  3 files changed, 37 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > > index 75b2873..5fecdc1 100644
> > > --- a/drivers/gpu/drm/drm_dp_helper.c
> > > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > > @@ -514,6 +514,33 @@ int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> > >  EXPORT_SYMBOL(drm_dp_downstream_max_bpc);
> > >  
> > >  /**
> > > + * drm_dp_downstream_hw_rev() - read DP branch device HW revision
> > > + * @aux: DisplayPort AUX channel
> > > + *
> > > + * Returns 0 on succes or negative error code on failure
> > > + */
> > > +int drm_dp_downstream_hw_rev(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> > > +			     struct drm_dp_aux *aux, struct drm_dp_link *link)
> > > +{
> > > +	uint8_t tmp;
> > > +	int err;
> > > +
> > > +	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
> > > +		return -EINVAL;
> > > +
> > > +	err = drm_dp_dpcd_read(aux, DP_BRANCH_HW_REV, &tmp, 1);
> > > +
> > > +	if (err < 0)
> > > +		return err;
> > > +
> > > +	link->ds_hw_rev.major = (tmp & 0xf0) >> 4;
> > > +	link->ds_hw_rev.minor = tmp & 0xf;
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL(drm_dp_downstream_hw_rev);
> > > +
> > > +/**
> > >   * drm_dp_downstream_id() - identify branch device
> > >   * @aux: DisplayPort AUX channel
> > >   *
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index e74d851..a6eccf5 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -865,6 +865,7 @@ struct intel_dp {
> > >  	uint8_t num_sink_rates;
> > >  	int sink_rates[DP_MAX_SUPPORTED_RATES];
> > >  	struct drm_dp_aux aux;
> > > +	struct drm_dp_link link;
> > >  	uint8_t train_set[4];
> > >  	int panel_power_up_delay;
> > >  	int panel_power_down_delay;
> > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > > index 8e1fe58..1127948 100644
> > > --- a/include/drm/drm_dp_helper.h
> > > +++ b/include/drm/drm_dp_helper.h
> > > @@ -446,6 +446,7 @@
> > >  #define DP_SINK_OUI			    0x400
> > >  #define DP_BRANCH_OUI			    0x500
> > >  #define DP_BRANCH_ID                        0x503
> > > +#define DP_BRANCH_HW_REV                    0x509
> > >  
> > >  #define DP_SET_POWER                        0x600
> > >  # define DP_SET_POWER_D0                    0x1
> > > @@ -803,11 +804,17 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
> > >   */
> > >  #define DP_LINK_CAP_ENHANCED_FRAMING (1 << 0)
> > >  
> > > +struct drm_dp_ds_revision {
> > > +	int major;
> > > +	int minor;
> > > +};
> > > +
> > >  struct drm_dp_link {
> > >  	unsigned char revision;
> > >  	unsigned int rate;
> > >  	unsigned int num_lanes;
> > >  	unsigned long capabilities;
> > > +	struct drm_dp_ds_revision ds_hw_rev;
> > >  };
> > >  
> > >  int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link);
> > > @@ -819,6 +826,8 @@ int drm_dp_downstream_max_clock(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> > >  int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> > >  			      const u8 port_cap[4]);
> > >  int drm_dp_downstream_id(struct drm_dp_aux *aux, char id[6]);
> > > +int drm_dp_downstream_hw_rev(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> > > +			     struct drm_dp_aux *aux, struct drm_dp_link *link);
> > >  
> > >  void drm_dp_aux_init(struct drm_dp_aux *aux);
> > >  int drm_dp_aux_register(struct drm_dp_aux *aux);
> > > -- 
> > > 1.9.1
> > 
> 

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

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

* Re: [PATCH v7 4/5] drm: Update bits per component for display info
  2016-08-11  7:22   ` Ville Syrjälä
@ 2016-08-11  8:53     ` Daniel Vetter
  0 siblings, 0 replies; 20+ messages in thread
From: Daniel Vetter @ 2016-08-11  8:53 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: daniel.vetter, intel-gfx, dri-devel

On Thu, Aug 11, 2016 at 10:22:27AM +0300, Ville Syrjälä wrote:
> On Mon, Aug 08, 2016 at 04:00:29PM +0300, Mika Kahola wrote:
> > DisplayPort branch device may define max supported bits per
> > component. Update display info based on this value if bpc
> > is defined.
> > 
> > v2: cleanup to match the drm_dp_helper.c patches introduced
> >     earlier in this series
> > v3: Fill bpc for connector's display info in separate
> >     drm_dp_helper function (Daniel)
> > 
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> >  drivers/gpu/drm/drm_dp_helper.c | 24 ++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_dp.c |  3 +++
> >  include/drm/drm_dp_helper.h     |  4 ++++
> >  3 files changed, 31 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > index 1f36016..a2c46ed 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -514,6 +514,30 @@ int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> >  EXPORT_SYMBOL(drm_dp_downstream_max_bpc);
> >  
> >  /**
> > + * drm_dp_downstream_update_max_bpc() - extract branch device max
> > + *                                      bits per component and update
> > + *                                      connector max bpc
> > + * @dpcd: DisplayPort configuration data
> > + * @port_cap: port capabilities
> > + * @connector: Connector info
> > + * Returns current max bpc on success
> > + */
> > +int drm_dp_downstream_update_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> > +				     const u8 port_cap[4],
> > +				     struct drm_connector *connector)
> > +{
> > +	if (dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT) {
> > +		int bpc = drm_dp_downstream_max_bpc(dpcd, port_cap);
> > +
> > +		if (bpc > 0)
> > +			connector->display_info.bpc = bpc;
> > +	}
> > +
> > +	return connector->display_info.bpc;
> > +}
> > +EXPORT_SYMBOL(drm_dp_downstream_update_max_bpc);
> > +
> > +/**
> >   * drm_dp_downstream_hw_rev() - read DP branch device HW revision
> >   * @aux: DisplayPort AUX channel
> >   *
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index e990c8b..763e2f5 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3985,6 +3985,9 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
> >  	uint8_t *dpcd = intel_dp->dpcd;
> >  	uint8_t type;
> >  
> > +	drm_dp_downstream_update_max_bpc(dpcd, intel_dp->downstream_ports,
> > +					 &intel_dp->attached_connector->base);
> > +
> 
> And it will be promptly overwritten by the EDID parser. Like I said
> before, touching display_info from elsewhere is a bad idea right now.
> We need to lay out some real rules on how the EDID parser vs. DP
> helpers must be called if we are to share that information between
> the two.
> 
> For now, I would just utilize the DP helper in the compute_config
> to limit the bpc.

I think we need one dp helper which does _all_ the sink detection, and in
the right order. Edid reading dpcd decoding, all that. And then stuff is
all into relevant structures like display_info, but also some new (or well
I think we have parts of it already) dp_sink struct. Otherwise this will
stay fragile, especially once other drivers start using it.
-Daniel

> 
> >  	if (!intel_dp_get_dpcd(intel_dp))
> >  		return connector_status_disconnected;
> >  
> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > index 45366aa..5491a9b 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -26,6 +26,7 @@
> >  #include <linux/types.h>
> >  #include <linux/i2c.h>
> >  #include <linux/delay.h>
> > +#include <drm/drm_crtc.h>
> >  
> >  /*
> >   * Unless otherwise noted, all values are from the DP 1.1a spec.  Note that
> > @@ -827,6 +828,9 @@ int drm_dp_downstream_max_clock(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> >  				const u8 port_cap[4]);
> >  int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> >  			      const u8 port_cap[4]);
> > +int drm_dp_downstream_update_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> > +				     const u8 port_cap[4],
> > +				     struct drm_connector *connector);
> >  int drm_dp_downstream_id(struct drm_dp_aux *aux, char id[6]);
> >  int drm_dp_downstream_hw_rev(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> >  			     struct drm_dp_aux *aux, struct drm_dp_link *link);
> > -- 
> > 1.9.1
> 
> -- 
> Ville Syrjälä
> Intel OTC

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

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

* Re: [PATCH v7 3/5] drm/i915: Check pixel rate for DP to VGA dongle
  2016-08-11  7:18   ` Ville Syrjälä
@ 2016-08-11  9:43     ` Mika Kahola
  2016-08-11 10:51       ` Ville Syrjälä
  0 siblings, 1 reply; 20+ messages in thread
From: Mika Kahola @ 2016-08-11  9:43 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: daniel.vetter, intel-gfx, dri-devel

On Thu, 2016-08-11 at 10:18 +0300, Ville Syrjälä wrote:
> On Mon, Aug 08, 2016 at 04:00:28PM +0300, Mika Kahola wrote:
> > Filter out a mode that exceeds the max pixel rate setting
> > for DP to VGA dongle. This is defined in DPCD register 0x81
> > if detailed cap info i.e. info field is 4 bytes long and
> > it is available for DP downstream port.
> > 
> > The register defines the pixel rate divided by 8 in MP/s.
> > 
> > v2: DPCD read outs and computation moved to drm (Ville, Daniel)
> > v3: Sink pixel rate computation moved to drm_dp_max_sink_dotclock()
> >     function (Daniel)
> > v4: Use of drm_dp_helper.c routines to compute max pixel clock (Ville)
> > v5: Use of intel_dp->downstream_ports to read out port capabilities.
> >     Code restructuring (Ville)
> > v6: Move DP branch device check to drm_dp_helper.c (Daniel)
> > 
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 25 ++++++++++++++++++++++++-
> >  1 file changed, 24 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 21b04c3..e990c8b 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -190,6 +190,26 @@ intel_dp_max_data_rate(int max_link_clock, int max_lanes)
> >  	return (max_link_clock * max_lanes * 8) / 10;
> >  }
> >  
> > +static int
> > +intel_dp_downstream_max_dotclock(struct intel_dp *intel_dp, int dotclk)
> > +{
> 
> I would just
> 
> {
> 	int max_dotclk = dev_priv->max_dotclk_freq;
> 
> 	ds_max_dotclk = ...;
> 	if (ds_dotclk != 0)
> 		max_dotclk = min(max_dotclk, ds_max_dotclk);
> 
> 	return max_dotclk;
> }
> 
> > +	int ds_dotclk;
> > +	int type;
> > +
> > +	ds_dotclk = drm_dp_downstream_max_clock(intel_dp->dpcd,
> > +						intel_dp->downstream_ports);
> > +
> > +	if (ds_dotclk == 0)
> > +		return dotclk;
> > +
> > +	type = intel_dp->downstream_ports[0] & DP_DS_PORT_TYPE_MASK;
> > +	
> > +	if (type != DP_DS_PORT_TYPE_VGA)
> > +		return dotclk;
> 
> Why isn't drm_dp_downstream_max_clock() handling all of it already?
> Why are we even checking for !=VGA?
The routine drm_dp_downstream_max_clock returns the clock rate which can
be dotclock (VGA only) or TMDS clock (for DVI, HDMI, DP++). Here, we
need to have a check for this as we are only interested to update VGA
dotclock value.

Cheers,
Mika
> 
> > +
> > +	return min(dotclk, ds_dotclk);
> > +}
> > +
> >  static enum drm_mode_status
> >  intel_dp_mode_valid(struct drm_connector *connector,
> >  		    struct drm_display_mode *mode)
> > @@ -199,7 +219,10 @@ intel_dp_mode_valid(struct drm_connector *connector,
> >  	struct drm_display_mode *fixed_mode = intel_connector->panel.fixed_mode;
> >  	int target_clock = mode->clock;
> >  	int max_rate, mode_rate, max_lanes, max_link_clock;
> > -	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
> > +	int max_dotclk;
> > +
> > +	max_dotclk = intel_dp_downstream_max_dotclock(intel_dp,
> > +						      to_i915(connector->dev)->max_dotclk_freq);
> >  
> >  	if (is_edp(intel_dp) && fixed_mode) {
> >  		if (mode->hdisplay > fixed_mode->hdisplay)
> > -- 
> > 1.9.1
> 


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

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

* Re: [PATCH v7 3/5] drm/i915: Check pixel rate for DP to VGA dongle
  2016-08-11  9:43     ` Mika Kahola
@ 2016-08-11 10:51       ` Ville Syrjälä
  2016-08-11 10:56         ` Daniel Vetter
  0 siblings, 1 reply; 20+ messages in thread
From: Ville Syrjälä @ 2016-08-11 10:51 UTC (permalink / raw)
  To: Mika Kahola; +Cc: daniel.vetter, intel-gfx, jim.bride, dri-devel

On Thu, Aug 11, 2016 at 12:43:39PM +0300, Mika Kahola wrote:
> On Thu, 2016-08-11 at 10:18 +0300, Ville Syrjälä wrote:
> > On Mon, Aug 08, 2016 at 04:00:28PM +0300, Mika Kahola wrote:
> > > Filter out a mode that exceeds the max pixel rate setting
> > > for DP to VGA dongle. This is defined in DPCD register 0x81
> > > if detailed cap info i.e. info field is 4 bytes long and
> > > it is available for DP downstream port.
> > > 
> > > The register defines the pixel rate divided by 8 in MP/s.
> > > 
> > > v2: DPCD read outs and computation moved to drm (Ville, Daniel)
> > > v3: Sink pixel rate computation moved to drm_dp_max_sink_dotclock()
> > >     function (Daniel)
> > > v4: Use of drm_dp_helper.c routines to compute max pixel clock (Ville)
> > > v5: Use of intel_dp->downstream_ports to read out port capabilities.
> > >     Code restructuring (Ville)
> > > v6: Move DP branch device check to drm_dp_helper.c (Daniel)
> > > 
> > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 25 ++++++++++++++++++++++++-
> > >  1 file changed, 24 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 21b04c3..e990c8b 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -190,6 +190,26 @@ intel_dp_max_data_rate(int max_link_clock, int max_lanes)
> > >  	return (max_link_clock * max_lanes * 8) / 10;
> > >  }
> > >  
> > > +static int
> > > +intel_dp_downstream_max_dotclock(struct intel_dp *intel_dp, int dotclk)
> > > +{
> > 
> > I would just
> > 
> > {
> > 	int max_dotclk = dev_priv->max_dotclk_freq;
> > 
> > 	ds_max_dotclk = ...;
> > 	if (ds_dotclk != 0)
> > 		max_dotclk = min(max_dotclk, ds_max_dotclk);
> > 
> > 	return max_dotclk;
> > }
> > 
> > > +	int ds_dotclk;
> > > +	int type;
> > > +
> > > +	ds_dotclk = drm_dp_downstream_max_clock(intel_dp->dpcd,
> > > +						intel_dp->downstream_ports);
> > > +
> > > +	if (ds_dotclk == 0)
> > > +		return dotclk;
> > > +
> > > +	type = intel_dp->downstream_ports[0] & DP_DS_PORT_TYPE_MASK;
> > > +	
> > > +	if (type != DP_DS_PORT_TYPE_VGA)
> > > +		return dotclk;
> > 
> > Why isn't drm_dp_downstream_max_clock() handling all of it already?
> > Why are we even checking for !=VGA?
> The routine drm_dp_downstream_max_clock returns the clock rate which can
> be dotclock (VGA only) or TMDS clock (for DVI, HDMI, DP++). Here, we
> need to have a check for this as we are only interested to update VGA
> dotclock value.

We should handle it all. Actually I'm not even sure how we're supposed
to deal with the downstream port max TMDS clock since for HDMI that
depends on the bpc, but since this is about a DP->HDMI conversion, I
don't know if we have to take the downstream port max TMDS clock into
account when choosing the bpc over the DP link as well. I suppose that's
possible if the dongle can't change change the bpc, and instead just
passes things through. I think this is one of those places where the
DP spec is way too unclear. But for DP->VGA there is no clock going out
the other end, so it must be just about the limits of the DP input or
the DAC.

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

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

* Re: [PATCH v7 3/5] drm/i915: Check pixel rate for DP to VGA dongle
  2016-08-11 10:51       ` Ville Syrjälä
@ 2016-08-11 10:56         ` Daniel Vetter
  2016-08-11 12:26           ` Mika Kahola
  0 siblings, 1 reply; 20+ messages in thread
From: Daniel Vetter @ 2016-08-11 10:56 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: daniel.vetter, intel-gfx, dri-devel

On Thu, Aug 11, 2016 at 01:51:42PM +0300, Ville Syrjälä wrote:
> On Thu, Aug 11, 2016 at 12:43:39PM +0300, Mika Kahola wrote:
> > On Thu, 2016-08-11 at 10:18 +0300, Ville Syrjälä wrote:
> > > On Mon, Aug 08, 2016 at 04:00:28PM +0300, Mika Kahola wrote:
> > > > Filter out a mode that exceeds the max pixel rate setting
> > > > for DP to VGA dongle. This is defined in DPCD register 0x81
> > > > if detailed cap info i.e. info field is 4 bytes long and
> > > > it is available for DP downstream port.
> > > > 
> > > > The register defines the pixel rate divided by 8 in MP/s.
> > > > 
> > > > v2: DPCD read outs and computation moved to drm (Ville, Daniel)
> > > > v3: Sink pixel rate computation moved to drm_dp_max_sink_dotclock()
> > > >     function (Daniel)
> > > > v4: Use of drm_dp_helper.c routines to compute max pixel clock (Ville)
> > > > v5: Use of intel_dp->downstream_ports to read out port capabilities.
> > > >     Code restructuring (Ville)
> > > > v6: Move DP branch device check to drm_dp_helper.c (Daniel)
> > > > 
> > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c | 25 ++++++++++++++++++++++++-
> > > >  1 file changed, 24 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 21b04c3..e990c8b 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -190,6 +190,26 @@ intel_dp_max_data_rate(int max_link_clock, int max_lanes)
> > > >  	return (max_link_clock * max_lanes * 8) / 10;
> > > >  }
> > > >  
> > > > +static int
> > > > +intel_dp_downstream_max_dotclock(struct intel_dp *intel_dp, int dotclk)
> > > > +{
> > > 
> > > I would just
> > > 
> > > {
> > > 	int max_dotclk = dev_priv->max_dotclk_freq;
> > > 
> > > 	ds_max_dotclk = ...;
> > > 	if (ds_dotclk != 0)
> > > 		max_dotclk = min(max_dotclk, ds_max_dotclk);
> > > 
> > > 	return max_dotclk;
> > > }
> > > 
> > > > +	int ds_dotclk;
> > > > +	int type;
> > > > +
> > > > +	ds_dotclk = drm_dp_downstream_max_clock(intel_dp->dpcd,
> > > > +						intel_dp->downstream_ports);
> > > > +
> > > > +	if (ds_dotclk == 0)
> > > > +		return dotclk;
> > > > +
> > > > +	type = intel_dp->downstream_ports[0] & DP_DS_PORT_TYPE_MASK;
> > > > +	
> > > > +	if (type != DP_DS_PORT_TYPE_VGA)
> > > > +		return dotclk;
> > > 
> > > Why isn't drm_dp_downstream_max_clock() handling all of it already?
> > > Why are we even checking for !=VGA?
> > The routine drm_dp_downstream_max_clock returns the clock rate which can
> > be dotclock (VGA only) or TMDS clock (for DVI, HDMI, DP++). Here, we
> > need to have a check for this as we are only interested to update VGA
> > dotclock value.
> 
> We should handle it all. Actually I'm not even sure how we're supposed
> to deal with the downstream port max TMDS clock since for HDMI that
> depends on the bpc, but since this is about a DP->HDMI conversion, I
> don't know if we have to take the downstream port max TMDS clock into
> account when choosing the bpc over the DP link as well. I suppose that's
> possible if the dongle can't change change the bpc, and instead just
> passes things through. I think this is one of those places where the
> DP spec is way too unclear. But for DP->VGA there is no clock going out
> the other end, so it must be just about the limits of the DP input or
> the DAC.

I guess we should defensively assume that the tmds clock limit is both for
the input and the output signal, worst case, for the dp->hdmi dongle?
Except when it's a passive level-shifter only one ofc.
-Daniel
-- 
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

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

* Re: [PATCH v7 3/5] drm/i915: Check pixel rate for DP to VGA dongle
  2016-08-11 10:56         ` Daniel Vetter
@ 2016-08-11 12:26           ` Mika Kahola
  2016-08-11 14:23             ` Ville Syrjälä
  0 siblings, 1 reply; 20+ messages in thread
From: Mika Kahola @ 2016-08-11 12:26 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: daniel.vetter, intel-gfx, dri-devel

On Thu, 2016-08-11 at 12:56 +0200, Daniel Vetter wrote:
> On Thu, Aug 11, 2016 at 01:51:42PM +0300, Ville Syrjälä wrote:
> > On Thu, Aug 11, 2016 at 12:43:39PM +0300, Mika Kahola wrote:
> > > On Thu, 2016-08-11 at 10:18 +0300, Ville Syrjälä wrote:
> > > > On Mon, Aug 08, 2016 at 04:00:28PM +0300, Mika Kahola wrote:
> > > > > Filter out a mode that exceeds the max pixel rate setting
> > > > > for DP to VGA dongle. This is defined in DPCD register 0x81
> > > > > if detailed cap info i.e. info field is 4 bytes long and
> > > > > it is available for DP downstream port.
> > > > > 
> > > > > The register defines the pixel rate divided by 8 in MP/s.
> > > > > 
> > > > > v2: DPCD read outs and computation moved to drm (Ville, Daniel)
> > > > > v3: Sink pixel rate computation moved to drm_dp_max_sink_dotclock()
> > > > >     function (Daniel)
> > > > > v4: Use of drm_dp_helper.c routines to compute max pixel clock (Ville)
> > > > > v5: Use of intel_dp->downstream_ports to read out port capabilities.
> > > > >     Code restructuring (Ville)
> > > > > v6: Move DP branch device check to drm_dp_helper.c (Daniel)
> > > > > 
> > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_dp.c | 25 ++++++++++++++++++++++++-
> > > > >  1 file changed, 24 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index 21b04c3..e990c8b 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -190,6 +190,26 @@ intel_dp_max_data_rate(int max_link_clock, int max_lanes)
> > > > >  	return (max_link_clock * max_lanes * 8) / 10;
> > > > >  }
> > > > >  
> > > > > +static int
> > > > > +intel_dp_downstream_max_dotclock(struct intel_dp *intel_dp, int dotclk)
> > > > > +{
> > > > 
> > > > I would just
> > > > 
> > > > {
> > > > 	int max_dotclk = dev_priv->max_dotclk_freq;
> > > > 
> > > > 	ds_max_dotclk = ...;
> > > > 	if (ds_dotclk != 0)
> > > > 		max_dotclk = min(max_dotclk, ds_max_dotclk);
> > > > 
> > > > 	return max_dotclk;
> > > > }
> > > > 
> > > > > +	int ds_dotclk;
> > > > > +	int type;
> > > > > +
> > > > > +	ds_dotclk = drm_dp_downstream_max_clock(intel_dp->dpcd,
> > > > > +						intel_dp->downstream_ports);
> > > > > +
> > > > > +	if (ds_dotclk == 0)
> > > > > +		return dotclk;
> > > > > +
> > > > > +	type = intel_dp->downstream_ports[0] & DP_DS_PORT_TYPE_MASK;
> > > > > +	
> > > > > +	if (type != DP_DS_PORT_TYPE_VGA)
> > > > > +		return dotclk;
> > > > 
> > > > Why isn't drm_dp_downstream_max_clock() handling all of it already?
> > > > Why are we even checking for !=VGA?
> > > The routine drm_dp_downstream_max_clock returns the clock rate which can
> > > be dotclock (VGA only) or TMDS clock (for DVI, HDMI, DP++). Here, we
> > > need to have a check for this as we are only interested to update VGA
> > > dotclock value.
> > 
> > We should handle it all. Actually I'm not even sure how we're supposed
> > to deal with the downstream port max TMDS clock since for HDMI that
> > depends on the bpc, but since this is about a DP->HDMI conversion, I
> > don't know if we have to take the downstream port max TMDS clock into
> > account when choosing the bpc over the DP link as well. I suppose that's
> > possible if the dongle can't change change the bpc, and instead just
> > passes things through. I think this is one of those places where the
> > DP spec is way too unclear. But for DP->VGA there is no clock going out
> > the other end, so it must be just about the limits of the DP input or
> > the DAC.
> 
> I guess we should defensively assume that the tmds clock limit is both for
> the input and the output signal, worst case, for the dp->hdmi dongle?
> Except when it's a passive level-shifter only one ofc.
> -Daniel
So, we should respect dongles both tmds and bpc values. I thought that
tmds clock checks was already covered by Ville's DP dual mode patch
series?


-- 
Mika Kahola - Intel OTC

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

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

* Re: [PATCH v7 3/5] drm/i915: Check pixel rate for DP to VGA dongle
  2016-08-11 12:26           ` Mika Kahola
@ 2016-08-11 14:23             ` Ville Syrjälä
  0 siblings, 0 replies; 20+ messages in thread
From: Ville Syrjälä @ 2016-08-11 14:23 UTC (permalink / raw)
  To: Mika Kahola; +Cc: daniel.vetter, intel-gfx, jim.bride, dri-devel

On Thu, Aug 11, 2016 at 03:26:42PM +0300, Mika Kahola wrote:
> On Thu, 2016-08-11 at 12:56 +0200, Daniel Vetter wrote:
> > On Thu, Aug 11, 2016 at 01:51:42PM +0300, Ville Syrjälä wrote:
> > > On Thu, Aug 11, 2016 at 12:43:39PM +0300, Mika Kahola wrote:
> > > > On Thu, 2016-08-11 at 10:18 +0300, Ville Syrjälä wrote:
> > > > > On Mon, Aug 08, 2016 at 04:00:28PM +0300, Mika Kahola wrote:
> > > > > > Filter out a mode that exceeds the max pixel rate setting
> > > > > > for DP to VGA dongle. This is defined in DPCD register 0x81
> > > > > > if detailed cap info i.e. info field is 4 bytes long and
> > > > > > it is available for DP downstream port.
> > > > > > 
> > > > > > The register defines the pixel rate divided by 8 in MP/s.
> > > > > > 
> > > > > > v2: DPCD read outs and computation moved to drm (Ville, Daniel)
> > > > > > v3: Sink pixel rate computation moved to drm_dp_max_sink_dotclock()
> > > > > >     function (Daniel)
> > > > > > v4: Use of drm_dp_helper.c routines to compute max pixel clock (Ville)
> > > > > > v5: Use of intel_dp->downstream_ports to read out port capabilities.
> > > > > >     Code restructuring (Ville)
> > > > > > v6: Move DP branch device check to drm_dp_helper.c (Daniel)
> > > > > > 
> > > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_dp.c | 25 ++++++++++++++++++++++++-
> > > > > >  1 file changed, 24 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > index 21b04c3..e990c8b 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > @@ -190,6 +190,26 @@ intel_dp_max_data_rate(int max_link_clock, int max_lanes)
> > > > > >  	return (max_link_clock * max_lanes * 8) / 10;
> > > > > >  }
> > > > > >  
> > > > > > +static int
> > > > > > +intel_dp_downstream_max_dotclock(struct intel_dp *intel_dp, int dotclk)
> > > > > > +{
> > > > > 
> > > > > I would just
> > > > > 
> > > > > {
> > > > > 	int max_dotclk = dev_priv->max_dotclk_freq;
> > > > > 
> > > > > 	ds_max_dotclk = ...;
> > > > > 	if (ds_dotclk != 0)
> > > > > 		max_dotclk = min(max_dotclk, ds_max_dotclk);
> > > > > 
> > > > > 	return max_dotclk;
> > > > > }
> > > > > 
> > > > > > +	int ds_dotclk;
> > > > > > +	int type;
> > > > > > +
> > > > > > +	ds_dotclk = drm_dp_downstream_max_clock(intel_dp->dpcd,
> > > > > > +						intel_dp->downstream_ports);
> > > > > > +
> > > > > > +	if (ds_dotclk == 0)
> > > > > > +		return dotclk;
> > > > > > +
> > > > > > +	type = intel_dp->downstream_ports[0] & DP_DS_PORT_TYPE_MASK;
> > > > > > +	
> > > > > > +	if (type != DP_DS_PORT_TYPE_VGA)
> > > > > > +		return dotclk;
> > > > > 
> > > > > Why isn't drm_dp_downstream_max_clock() handling all of it already?
> > > > > Why are we even checking for !=VGA?
> > > > The routine drm_dp_downstream_max_clock returns the clock rate which can
> > > > be dotclock (VGA only) or TMDS clock (for DVI, HDMI, DP++). Here, we
> > > > need to have a check for this as we are only interested to update VGA
> > > > dotclock value.
> > > 
> > > We should handle it all. Actually I'm not even sure how we're supposed
> > > to deal with the downstream port max TMDS clock since for HDMI that
> > > depends on the bpc, but since this is about a DP->HDMI conversion, I
> > > don't know if we have to take the downstream port max TMDS clock into
> > > account when choosing the bpc over the DP link as well. I suppose that's
> > > possible if the dongle can't change change the bpc, and instead just
> > > passes things through. I think this is one of those places where the
> > > DP spec is way too unclear. But for DP->VGA there is no clock going out
> > > the other end, so it must be just about the limits of the DP input or
> > > the DAC.
> > 
> > I guess we should defensively assume that the tmds clock limit is both for
> > the input and the output signal, worst case, for the dp->hdmi dongle?
> > Except when it's a passive level-shifter only one ofc.
> > -Daniel
> So, we should respect dongles both tmds and bpc values. I thought that
> tmds clock checks was already covered by Ville's DP dual mode patch
> series?

That doesn't deal with active DP->HDMI converters, which is what this
DPCD stuff will have to deal with.

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

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

* Re: [PATCH v7 1/5] drm: Read DP branch device HW revision
  2016-08-11  8:22       ` Ville Syrjälä
@ 2016-08-11 17:21         ` Jim Bride
  2016-08-11 17:46           ` Ville Syrjälä
  0 siblings, 1 reply; 20+ messages in thread
From: Jim Bride @ 2016-08-11 17:21 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: daniel.vetter, intel-gfx, dri-devel

On Thu, Aug 11, 2016 at 11:22:15AM +0300, Ville Syrjälä wrote:
> On Thu, Aug 11, 2016 at 11:14:43AM +0300, Mika Kahola wrote:
> > On Thu, 2016-08-11 at 10:10 +0300, Ville Syrjälä wrote:
> > > On Mon, Aug 08, 2016 at 04:00:26PM +0300, Mika Kahola wrote:
> > > > HW revision is mandatory field for DisplayPort branch
> > > > devices. This is defined in DPCD register field 0x509.
> > > 
> > > But what do we want to do with it? Me, I don't see a point in parsing a
> > > bunch of stuff from the DPCD unless there's a real use case for it.
> > > /dev/drm_dp_aux will provide all the debug aid we need if we need to
> > > check random pieces of data from the DPCD, so even exposing this sort
> > > of stuff via debugfs is IMO pretty pointless.
> > > 
> > > If there's a good reason for a debug print, then I think we could parse
> > > something and dump it out so that it's always in the dmesg. But so far
> > > I'm not aware of any bug that would have required to deal with different
> > > hw revisions of things and whatnot.
> > > 
> > Digging up the HW and SW revisions are just for debugging purposes. My
> > idea here was that it would be handy if this information would be
> > available if we face a problem let's say with VGA dongle for example. It
> > is true that at the moment we don't have a single bug that would require
> > HW/SW revision information but maybe in the future we have one.
> > 
> > So, if we don't want to have this stuff in drm_dp_helpers and/or debugfs
> > I could make a change and print this info on dmesg when DP branch device
> > is plugged in.
> 
> Perhaps. We do print out the OUI as well, so there is some precedent.

Even if we don't dump stuff out to dmesg, I'd like to see this information
in the mst_info debugfs file or someplace similar.  As has been pointed out,
having this information available in the event that we need to work with a
vendor on a problem would be a help.

Jim


> 
> > 
> > Cheers,
> > Mika
> > 
> > > > 
> > > > v2: move drm_dp_ds_revision structure to be part of
> > > >     drm_dp_link structure (Daniel)
> > > > 
> > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_dp_helper.c  | 27 +++++++++++++++++++++++++++
> > > >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> > > >  include/drm/drm_dp_helper.h      |  9 +++++++++
> > > >  3 files changed, 37 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > > > index 75b2873..5fecdc1 100644
> > > > --- a/drivers/gpu/drm/drm_dp_helper.c
> > > > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > > > @@ -514,6 +514,33 @@ int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> > > >  EXPORT_SYMBOL(drm_dp_downstream_max_bpc);
> > > >  
> > > >  /**
> > > > + * drm_dp_downstream_hw_rev() - read DP branch device HW revision
> > > > + * @aux: DisplayPort AUX channel
> > > > + *
> > > > + * Returns 0 on succes or negative error code on failure
> > > > + */
> > > > +int drm_dp_downstream_hw_rev(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> > > > +			     struct drm_dp_aux *aux, struct drm_dp_link *link)
> > > > +{
> > > > +	uint8_t tmp;
> > > > +	int err;
> > > > +
> > > > +	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
> > > > +		return -EINVAL;
> > > > +
> > > > +	err = drm_dp_dpcd_read(aux, DP_BRANCH_HW_REV, &tmp, 1);
> > > > +
> > > > +	if (err < 0)
> > > > +		return err;
> > > > +
> > > > +	link->ds_hw_rev.major = (tmp & 0xf0) >> 4;
> > > > +	link->ds_hw_rev.minor = tmp & 0xf;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(drm_dp_downstream_hw_rev);
> > > > +
> > > > +/**
> > > >   * drm_dp_downstream_id() - identify branch device
> > > >   * @aux: DisplayPort AUX channel
> > > >   *
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > index e74d851..a6eccf5 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -865,6 +865,7 @@ struct intel_dp {
> > > >  	uint8_t num_sink_rates;
> > > >  	int sink_rates[DP_MAX_SUPPORTED_RATES];
> > > >  	struct drm_dp_aux aux;
> > > > +	struct drm_dp_link link;
> > > >  	uint8_t train_set[4];
> > > >  	int panel_power_up_delay;
> > > >  	int panel_power_down_delay;
> > > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > > > index 8e1fe58..1127948 100644
> > > > --- a/include/drm/drm_dp_helper.h
> > > > +++ b/include/drm/drm_dp_helper.h
> > > > @@ -446,6 +446,7 @@
> > > >  #define DP_SINK_OUI			    0x400
> > > >  #define DP_BRANCH_OUI			    0x500
> > > >  #define DP_BRANCH_ID                        0x503
> > > > +#define DP_BRANCH_HW_REV                    0x509
> > > >  
> > > >  #define DP_SET_POWER                        0x600
> > > >  # define DP_SET_POWER_D0                    0x1
> > > > @@ -803,11 +804,17 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
> > > >   */
> > > >  #define DP_LINK_CAP_ENHANCED_FRAMING (1 << 0)
> > > >  
> > > > +struct drm_dp_ds_revision {
> > > > +	int major;
> > > > +	int minor;
> > > > +};
> > > > +
> > > >  struct drm_dp_link {
> > > >  	unsigned char revision;
> > > >  	unsigned int rate;
> > > >  	unsigned int num_lanes;
> > > >  	unsigned long capabilities;
> > > > +	struct drm_dp_ds_revision ds_hw_rev;
> > > >  };
> > > >  
> > > >  int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link);
> > > > @@ -819,6 +826,8 @@ int drm_dp_downstream_max_clock(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> > > >  int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> > > >  			      const u8 port_cap[4]);
> > > >  int drm_dp_downstream_id(struct drm_dp_aux *aux, char id[6]);
> > > > +int drm_dp_downstream_hw_rev(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> > > > +			     struct drm_dp_aux *aux, struct drm_dp_link *link);
> > > >  
> > > >  void drm_dp_aux_init(struct drm_dp_aux *aux);
> > > >  int drm_dp_aux_register(struct drm_dp_aux *aux);
> > > > -- 
> > > > 1.9.1
> > > 
> > 
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH v7 1/5] drm: Read DP branch device HW revision
  2016-08-11 17:21         ` Jim Bride
@ 2016-08-11 17:46           ` Ville Syrjälä
  0 siblings, 0 replies; 20+ messages in thread
From: Ville Syrjälä @ 2016-08-11 17:46 UTC (permalink / raw)
  To: Jim Bride; +Cc: daniel.vetter, intel-gfx, dri-devel

On Thu, Aug 11, 2016 at 10:21:36AM -0700, Jim Bride wrote:
> On Thu, Aug 11, 2016 at 11:22:15AM +0300, Ville Syrjälä wrote:
> > On Thu, Aug 11, 2016 at 11:14:43AM +0300, Mika Kahola wrote:
> > > On Thu, 2016-08-11 at 10:10 +0300, Ville Syrjälä wrote:
> > > > On Mon, Aug 08, 2016 at 04:00:26PM +0300, Mika Kahola wrote:
> > > > > HW revision is mandatory field for DisplayPort branch
> > > > > devices. This is defined in DPCD register field 0x509.
> > > > 
> > > > But what do we want to do with it? Me, I don't see a point in parsing a
> > > > bunch of stuff from the DPCD unless there's a real use case for it.
> > > > /dev/drm_dp_aux will provide all the debug aid we need if we need to
> > > > check random pieces of data from the DPCD, so even exposing this sort
> > > > of stuff via debugfs is IMO pretty pointless.
> > > > 
> > > > If there's a good reason for a debug print, then I think we could parse
> > > > something and dump it out so that it's always in the dmesg. But so far
> > > > I'm not aware of any bug that would have required to deal with different
> > > > hw revisions of things and whatnot.
> > > > 
> > > Digging up the HW and SW revisions are just for debugging purposes. My
> > > idea here was that it would be handy if this information would be
> > > available if we face a problem let's say with VGA dongle for example. It
> > > is true that at the moment we don't have a single bug that would require
> > > HW/SW revision information but maybe in the future we have one.
> > > 
> > > So, if we don't want to have this stuff in drm_dp_helpers and/or debugfs
> > > I could make a change and print this info on dmesg when DP branch device
> > > is plugged in.
> > 
> > Perhaps. We do print out the OUI as well, so there is some precedent.
> 
> Even if we don't dump stuff out to dmesg, I'd like to see this information
> in the mst_info debugfs file or someplace similar.  As has been pointed out,
> having this information available in the event that we need to work with a
> vendor on a problem would be a help.

Just dump the whole thing via /dev/drm_dp_aux<n>. The problem with
getting parsed data for debugging is that the parser itself might be
buggy or just incomplete. So IMO it's always much better to get the
raw data. This way you can be sure that it's not affected by the parser,
and that you get all of the data and don't have to back asking for more all
the time.

I really hate it when someone attaches a parsed VBT dump to a bug for
instance. It's pretty much useless.

I've been waiting for someone to step up and write a userspace tool for
pretty printing the entire DPCD dumps. So far no one has volunteered.

> Jim
> 
> 
> > 
> > > 
> > > Cheers,
> > > Mika
> > > 
> > > > > 
> > > > > v2: move drm_dp_ds_revision structure to be part of
> > > > >     drm_dp_link structure (Daniel)
> > > > > 
> > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_dp_helper.c  | 27 +++++++++++++++++++++++++++
> > > > >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> > > > >  include/drm/drm_dp_helper.h      |  9 +++++++++
> > > > >  3 files changed, 37 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > > > > index 75b2873..5fecdc1 100644
> > > > > --- a/drivers/gpu/drm/drm_dp_helper.c
> > > > > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > > > > @@ -514,6 +514,33 @@ int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> > > > >  EXPORT_SYMBOL(drm_dp_downstream_max_bpc);
> > > > >  
> > > > >  /**
> > > > > + * drm_dp_downstream_hw_rev() - read DP branch device HW revision
> > > > > + * @aux: DisplayPort AUX channel
> > > > > + *
> > > > > + * Returns 0 on succes or negative error code on failure
> > > > > + */
> > > > > +int drm_dp_downstream_hw_rev(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> > > > > +			     struct drm_dp_aux *aux, struct drm_dp_link *link)
> > > > > +{
> > > > > +	uint8_t tmp;
> > > > > +	int err;
> > > > > +
> > > > > +	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	err = drm_dp_dpcd_read(aux, DP_BRANCH_HW_REV, &tmp, 1);
> > > > > +
> > > > > +	if (err < 0)
> > > > > +		return err;
> > > > > +
> > > > > +	link->ds_hw_rev.major = (tmp & 0xf0) >> 4;
> > > > > +	link->ds_hw_rev.minor = tmp & 0xf;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_dp_downstream_hw_rev);
> > > > > +
> > > > > +/**
> > > > >   * drm_dp_downstream_id() - identify branch device
> > > > >   * @aux: DisplayPort AUX channel
> > > > >   *
> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > > index e74d851..a6eccf5 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -865,6 +865,7 @@ struct intel_dp {
> > > > >  	uint8_t num_sink_rates;
> > > > >  	int sink_rates[DP_MAX_SUPPORTED_RATES];
> > > > >  	struct drm_dp_aux aux;
> > > > > +	struct drm_dp_link link;
> > > > >  	uint8_t train_set[4];
> > > > >  	int panel_power_up_delay;
> > > > >  	int panel_power_down_delay;
> > > > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > > > > index 8e1fe58..1127948 100644
> > > > > --- a/include/drm/drm_dp_helper.h
> > > > > +++ b/include/drm/drm_dp_helper.h
> > > > > @@ -446,6 +446,7 @@
> > > > >  #define DP_SINK_OUI			    0x400
> > > > >  #define DP_BRANCH_OUI			    0x500
> > > > >  #define DP_BRANCH_ID                        0x503
> > > > > +#define DP_BRANCH_HW_REV                    0x509
> > > > >  
> > > > >  #define DP_SET_POWER                        0x600
> > > > >  # define DP_SET_POWER_D0                    0x1
> > > > > @@ -803,11 +804,17 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
> > > > >   */
> > > > >  #define DP_LINK_CAP_ENHANCED_FRAMING (1 << 0)
> > > > >  
> > > > > +struct drm_dp_ds_revision {
> > > > > +	int major;
> > > > > +	int minor;
> > > > > +};
> > > > > +
> > > > >  struct drm_dp_link {
> > > > >  	unsigned char revision;
> > > > >  	unsigned int rate;
> > > > >  	unsigned int num_lanes;
> > > > >  	unsigned long capabilities;
> > > > > +	struct drm_dp_ds_revision ds_hw_rev;
> > > > >  };
> > > > >  
> > > > >  int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link);
> > > > > @@ -819,6 +826,8 @@ int drm_dp_downstream_max_clock(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> > > > >  int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> > > > >  			      const u8 port_cap[4]);
> > > > >  int drm_dp_downstream_id(struct drm_dp_aux *aux, char id[6]);
> > > > > +int drm_dp_downstream_hw_rev(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> > > > > +			     struct drm_dp_aux *aux, struct drm_dp_link *link);
> > > > >  
> > > > >  void drm_dp_aux_init(struct drm_dp_aux *aux);
> > > > >  int drm_dp_aux_register(struct drm_dp_aux *aux);
> > > > > -- 
> > > > > 1.9.1
> > > > 
> > > 
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC

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

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

end of thread, other threads:[~2016-08-11 17:46 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-08 13:00 [PATCH v7 0/5] drm/i915: DP branch devices Mika Kahola
2016-08-08 13:00 ` [PATCH v7 1/5] drm: Read DP branch device HW revision Mika Kahola
2016-08-11  7:10   ` Ville Syrjälä
2016-08-11  8:14     ` Mika Kahola
2016-08-11  8:22       ` Ville Syrjälä
2016-08-11 17:21         ` Jim Bride
2016-08-11 17:46           ` Ville Syrjälä
2016-08-08 13:00 ` [PATCH v7 2/5] drm: Read DP branch device SW revision Mika Kahola
2016-08-08 13:00 ` [PATCH v7 3/5] drm/i915: Check pixel rate for DP to VGA dongle Mika Kahola
2016-08-11  7:18   ` Ville Syrjälä
2016-08-11  9:43     ` Mika Kahola
2016-08-11 10:51       ` Ville Syrjälä
2016-08-11 10:56         ` Daniel Vetter
2016-08-11 12:26           ` Mika Kahola
2016-08-11 14:23             ` Ville Syrjälä
2016-08-08 13:00 ` [PATCH v7 4/5] drm: Update bits per component for display info Mika Kahola
2016-08-11  7:22   ` Ville Syrjälä
2016-08-11  8:53     ` Daniel Vetter
2016-08-08 13:00 ` [PATCH v7 5/5] drm: Add DP branch device info on debugfs Mika Kahola
2016-08-08 13:10 ` ✗ Ro.CI.BAT: failure for drm/i915: DP branch devices (rev7) Patchwork

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