[PATCH 0/4] drm: DP++ adaptor support

[PATCH 0/4] drm: DP++ adaptor support

[ville.syrjala]

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

While looking at a regression caused by i915's use of 12bpc HDMI mode,
I ended up reading the DP dual mode spec, and that lead to this patch
series.

I intentionally made the basics of the helper look somewhat like
Thierry's HDMI 2.0 SCDC stuff [1], except with a few less bugs :P

The entire series is available here:
git://github.com/vsyrjala/linux.git dp_dual_mode_2

[1] https://lists.freedesktop.org/archives/dri-devel/2015-September/090929.html

Ville Syrjälä (4):
  drm: Add helper for DP++ adaptors
  drm/i915: Respect DP++ adaptor TMDS clock limit
  drm/i915: Enable/disable TMDS output buffers in DP++ adaptor as needed
  drm/i915: Determine DP++ type 1 DVI adaptor presence based on VBT

 drivers/gpu/drm/Makefile                  |   2 +-
 drivers/gpu/drm/drm_dp_dual_mode_helper.c | 324 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_bios.h         |   3 +
 drivers/gpu/drm/i915/intel_dp.c           |  28 +++
 drivers/gpu/drm/i915/intel_drv.h          |   5 +
 drivers/gpu/drm/i915/intel_hdmi.c         | 107 +++++++++-
 include/drm/drm_dp_dual_mode_helper.h     |  80 ++++++++
 7 files changed, 540 insertions(+), 9 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_dp_dual_mode_helper.c
 create mode 100644 include/drm/drm_dp_dual_mode_helper.h

-- 
2.4.10

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

[PATCH 1/4] drm: Add helper for DP++ adaptors

[ville.syrjala]

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

Add a helper which aids in he identification of DP dual mode (aka. DP++)
adaptors. There are several types of adaptors specified:
type 1 DVI, type 1 HDMI, type 2 DVI, type 2 HDMI

Type 1 adaptors have a max TMDS clock limit of 165MHz, type 2 adaptors
may go as high as 300MHz and they provide a register informing the
source device what the actual limit is. Supposedly also type 1 adaptors
may optionally implement this register. This TMDS clock limit is the
main reason why we need to identify these adaptors.

Type 1 adaptors provide access to their internal registers and the sink
DDC bus through I2C. Type 2 adaptors provide this access both via I2C
and I2C-over-AUX. A type 2 source device may choose to implement either
or both of these methods. If a source device implements only the
I2C-over-AUX method, then the driver will obviously need specific
support for such adaptors since the port is driven like an HDMI port,
but DDC communication happes over the AUX channel.

This helper should be enough to identify the adaptor type (some
type 1 DVI adaptors may be a slight exception) and the maximum TMDS
clock limit. Another feature that may be available is control over
the TMDS output buffers on the adaptor, possibly allowing for some
power saving when the TMDS link is down.

Other user controllable features that may be available in the adaptors
are downstream i2c bus speed control when using i2c-over-aux, and
some control over the CEC pin. I chose not to provide any helper
functions for those since I have no use for them in i915 at this time.
The rest of the registers in the adaptor are mostly just information,
eg. IEEE OUI, hardware and firmware revision, etc.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/Makefile                  |   2 +-
 drivers/gpu/drm/drm_dp_dual_mode_helper.c | 328 ++++++++++++++++++++++++++++++
 include/drm/drm_dp_dual_mode_helper.h     |  80 ++++++++
 3 files changed, 409 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/drm_dp_dual_mode_helper.c
 create mode 100644 include/drm/drm_dp_dual_mode_helper.h

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 6eb94fc561dc..22228ef50f36 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -23,7 +23,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
 
 drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
 		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
-		drm_kms_helper_common.o
+		drm_kms_helper_common.o drm_dp_dual_mode_helper.o
 
 drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
new file mode 100644
index 000000000000..bfe511c09568
--- /dev/null
+++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
@@ -0,0 +1,328 @@
+/*
+ * Copyright © 2016 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include <linux/errno.h>
+#include <linux/export.h>
+#include <linux/i2c.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <drm/drm_dp_dual_mode_helper.h>
+#include <drm/drmP.h>
+
+/**
+ * DOC: DP dual mode (aka. DP++) adaptor helpers
+ *
+ * Helper functions to deal with DP dual mode adaptors.
+ *
+ * Type 1:
+ * Adaptor registers (if any) and the sink DDC bus may be accessed via I2C.
+ *
+ * Type 2:
+ * Adaptor registers and sink DDC bus can be accessed either via I2C or
+ * I2C-over-AUX. Source devices may choose to implement either one or
+ * both of these access methods.
+ */
+
+#define DP_DUAL_MODE_SLAVE_ADDRESS 0x40
+
+/**
+ * drm_dp_dual_mode_read - Read from the DP dual mode adaptor register(s)
+ * adapter: I2C adapter for the DDC bus
+ * offset: register offset
+ * buffer: buffer for return data
+ * size: sizo of the buffer
+ *
+ * Reads @size bytes from the DP dual mode adaptor registers
+ * starting at @offset.
+ *
+ * Returns:
+ * 0 on success, negative error code on failure
+ */
+ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
+			      u8 offset, void *buffer, size_t size)
+{
+	struct i2c_msg msgs[] = {
+		{
+			.addr = DP_DUAL_MODE_SLAVE_ADDRESS,
+			.flags = 0,
+			.len = 1,
+			.buf = &offset,
+		},
+		{
+			.addr = DP_DUAL_MODE_SLAVE_ADDRESS,
+			.flags = I2C_M_RD,
+			.len = size,
+			.buf = buffer,
+		},
+	};
+	int ret;
+
+	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
+	if (ret < 0)
+		return ret;
+	if (ret != ARRAY_SIZE(msgs))
+		return -EPROTO;
+
+	return 0;
+}
+
+/**
+ * drm_dp_dual_mode_write - Write to the DP dual mode adaptor register(s)
+ * adapter: I2C adapter for the DDC bus
+ * offset: register offset
+ * buffer: buffer for write data
+ * size: sizo of the buffer
+ *
+ * Writes @size bytes to the DP dual mode adaptor registers
+ * starting at @offset.
+ *
+ * Returns:
+ * 0 on success, negative error code on failure
+ */
+ssize_t drm_dp_dual_mode_write(struct i2c_adapter *adapter,
+			       u8 offset, const void *buffer, size_t size)
+{
+	struct i2c_msg msg = {
+		.addr = DP_DUAL_MODE_SLAVE_ADDRESS,
+		.flags = 0,
+		.len = 1 + size,
+		.buf = NULL,
+	};
+	void *data;
+	int ret;
+
+	data = kmalloc(msg.len, GFP_TEMPORARY);
+	if (!data)
+		return -ENOMEM;
+
+	msg.buf = data;
+
+	memcpy(data, &offset, 1);
+	memcpy(data + 1, buffer, size);
+
+	ret = i2c_transfer(adapter, &msg, 1);
+
+	kfree(data);
+
+	if (ret < 0)
+		return ret;
+	if (ret != 1)
+		return -EPROTO;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_dp_dual_mode_write);
+
+static bool is_hdmi_adaptor(const char hdmi_id[DP_DUAL_MODE_HDMI_ID_LEN])
+{
+	static const char dp_dual_mode_hdmi_id[DP_DUAL_MODE_HDMI_ID_LEN] =
+		"DP-HDMI ADAPTOR\x04";
+
+	return memcmp(hdmi_id, dp_dual_mode_hdmi_id,
+		      sizeof(dp_dual_mode_hdmi_id)) == 0;
+}
+
+/**
+ * drm_dp_dual_mode_detect - Identyfy the DP dual mode adaptor
+ * adapter: I2C adapter for the DDC bus
+ *
+ * Attempt to identify the type of the DP dual mode adaptor used.
+ *
+ * Note that when the answer is @DRM_DP_DUAL_MODE_NONE it's not
+ * certain whether we're dealing with a native HDMI port or
+ * a type 1 DVI dual mode adaptor. The driver will have to use
+ * some other hardware/driver specific mechanism to make that
+ * distinction.
+ *
+ * Returns:
+ * The type of the DP dual mode adaptor used
+ */
+enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct i2c_adapter *adapter)
+{
+	char hdmi_id[DP_DUAL_MODE_HDMI_ID_LEN] = {};
+	uint8_t adaptor_id = 0x00;
+	ssize_t ret;
+
+	/*
+	 * Let's see if the adaptor is there the by reading the
+	 * HDMI ID registers.
+	 *
+	 * Note that type 1 DVI adaptors are not required to implemnt
+	 * any registers, and that presents a problem for detection.
+	 * If the i2c transfer is nacked, we may or may not be dealing
+	 * with a type 1 DVI adaptor. Some other mechanism of detecting
+	 * the presence of the adaptor is required. One way would be
+	 * to check the state of the CONFIG1 pin, Another method would
+	 * simply require the driver to know whether the port is a DP++
+	 * port or a native HDMI port. Both of these methods are entirely
+	 * hardware/driver specific so we can't deal with them here.
+	 */
+	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_HDMI_ID,
+				    hdmi_id, sizeof(hdmi_id));
+	if (ret)
+		return DRM_DP_DUAL_MODE_NONE;
+
+	/*
+	 * Sigh. Some (maybe all?) type 1 adaptors are broken and ack
+	 * the offset but ignore it, and instead they just always return
+	 * data from the start of the HDMI ID buffer. So for a broken
+	 * type 1 HDMI adaptor a single byte read will always give us
+	 * 0x44, and for a type 1 DVI adaptor it should give 0x00
+	 * (assuming it implements any registers). Fortunately neither
+	 * of those values will match the type 2 signature of the
+	 * DP_DUAL_MODE_ADAPTOR_ID register so we can proceed with
+	 * the type 2 adaptor detection safely even in the presence
+	 * of broken type 1 adaptors.
+	 */
+	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_ADAPTOR_ID,
+				    &adaptor_id, sizeof(adaptor_id));
+	if (ret || (adaptor_id != (DP_DUAL_MODE_TYPE_TYPE2 |
+				   DP_DUAL_MODE_REV_TYPE2))) {
+		if (is_hdmi_adaptor(hdmi_id))
+			return DRM_DP_DUAL_MODE_TYPE1_HDMI;
+		else
+			return DRM_DP_DUAL_MODE_TYPE1_DVI;
+	} else {
+		if (is_hdmi_adaptor(hdmi_id))
+			return DRM_DP_DUAL_MODE_TYPE2_HDMI;
+		else
+			return DRM_DP_DUAL_MODE_TYPE2_DVI;
+	}
+}
+EXPORT_SYMBOL(drm_dp_dual_mode_detect);
+
+/**
+ * drm_dp_dual_mode_max_tmds_clock - Max TMDS clock for DP dual mode adaptor
+ * adapter: I2C adapter for the DDC bus
+ *
+ * Determine the max TMDS clock the adaptor supports based on the
+ * DP_DUAL_MODE_MAX_TMDS_CLOCK register. The register is mandatory for
+ * type 2 adaptors, optional for type 1 adaptors. Hoever, as some type 1
+ * adaptors are broken (see comments in drm_dp_dual_mode_detect() for the
+ * details) one probably shouldn't use this with type 1 adaptors at all.
+ * Type 1 adaptors should anyway be always limited to 165 MHz.
+ *
+ * Returns:
+ * Maximum supported TMDS clock rate for the DP dual mode adaptor in kHz.
+ */
+int drm_dp_dual_mode_max_tmds_clock(struct i2c_adapter *adapter)
+{
+	uint8_t max_tmds_clock;
+	ssize_t ret;
+
+	/*
+	 * Type 1 adaptors are limited to 165MHz
+	 * Type 2 adaptors can tells us their limit
+	 */
+	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_MAX_TMDS_CLOCK,
+				    &max_tmds_clock, sizeof(max_tmds_clock));
+	if (ret)
+		return 165000;
+
+	return max_tmds_clock * 5000 / 2;
+}
+EXPORT_SYMBOL(drm_dp_dual_mode_max_tmds_clock);
+
+/**
+ * drm_dp_dual_mode_get_tmds_output - Get the state of the TMDS output buffers in the DP dual mode adaptor
+ * adapter: I2C adapter for the DDC bus
+ * enabled: current state of the TMDS output buffers
+ *
+ * Get the state of the TMDS output buffers in the adaptor.
+ * DP_DUAL_MODE_TMDS_OEN register is mandatory for type 2 adaptors,
+ * optionals for type 1 adaptors. Hoever, as some type 1 adaptors are
+ * broken (see comments in drm_dp_dual_mode_detect() for the details)
+ * one probably shouldn't use this with type 1 adaptors at all.
+ *
+ * Returns:
+ * 0 on success, negative error code on failure
+ */
+int drm_dp_dual_mode_get_tmds_output(struct i2c_adapter *adapter, bool *enabled)
+{
+	uint8_t tmds_oen;
+	ssize_t ret;
+
+	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_TMDS_OEN,
+				    &tmds_oen, sizeof(tmds_oen));
+	if (ret)
+		return ret;
+
+	*enabled = !(tmds_oen & DP_DUAL_MODE_TMDS_DISABLE);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_dp_dual_mode_get_tmds_output);
+
+/**
+ * drm_dp_dual_mode_set_tmds_output - Enable/disable TMDS output buffers in the DP dual mode adaptor
+ * adapter: I2C adapter for the DDC bus
+ * enable: enable (as opposed to disable) the TMDS output buffers
+ *
+ * Set the state of the TMDS output buffers in the adaptor.
+ * DP_DUAL_MODE_TMDS_OEN register is mandatory for type 2 adaptors,
+ * optionals for type 1 adaptors. Hoever, as some type 1 adaptors are
+ * broken (see comments in drm_dp_dual_mode_detect() for the details)
+ * one probably shouldn't use this with type 1 adaptors at all.
+ *
+ * Returns:
+ * 0 on success, negative error code on failure
+ */
+int drm_dp_dual_mode_set_tmds_output(struct i2c_adapter *adapter, bool enable)
+{
+	uint8_t tmds_oen = enable ? 0 : DP_DUAL_MODE_TMDS_DISABLE;
+	ssize_t ret;
+
+	ret = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_TMDS_OEN,
+				     &tmds_oen, sizeof(tmds_oen));
+	if (ret)
+		return ret;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_dp_dual_mode_set_tmds_output);
+
+/**
+ * drm_dp_get_dual_mode_type_name - Get the name of the DP dual mode adaptor type as a string
+ * type: DP dual mode adaptor type
+ *
+ * Returns:
+ * String representation of the DP dual mode adaptor type
+ */
+const char *drm_dp_get_dual_mode_type_name(enum drm_dp_dual_mode_type type)
+{
+	switch (type) {
+	case DRM_DP_DUAL_MODE_NONE:
+		return "none";
+	case DRM_DP_DUAL_MODE_TYPE1_DVI:
+		return "type 1 DVI";
+	case DRM_DP_DUAL_MODE_TYPE1_HDMI:
+		return "type 1 HDMI";
+	case DRM_DP_DUAL_MODE_TYPE2_DVI:
+		return "type 2 DVI";
+	case DRM_DP_DUAL_MODE_TYPE2_HDMI:
+		return "type 2 HDMI";
+	default:
+		return "unknown";
+	};
+
+}
+EXPORT_SYMBOL(drm_dp_get_dual_mode_type_name);
diff --git a/include/drm/drm_dp_dual_mode_helper.h b/include/drm/drm_dp_dual_mode_helper.h
new file mode 100644
index 000000000000..a2f6b4587f5f
--- /dev/null
+++ b/include/drm/drm_dp_dual_mode_helper.h
@@ -0,0 +1,80 @@
+/*
+ * Copyright © 2016 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#ifndef DRM_DP_DUAL_MODE_HELPER_H
+#define DRM_DP_DUAL_MODE_HELPER_H
+
+#include <linux/types.h>
+
+/*
+ * Optional for type 1 DVI adaptors
+ * Mandatory for type 1 HDMI and type 2 adators
+ */
+#define DP_DUAL_MODE_HDMI_ID 0x00 /* 00-0f */
+#define  DP_DUAL_MODE_HDMI_ID_LEN 16
+/*
+ * Optional for type 1 adaptors
+ * Mandatory for type 2 adators
+ */
+#define DP_DUAL_MODE_ADAPTOR_ID 0x10
+#define  DP_DUAL_MODE_REV_MASK 0x07
+#define  DP_DUAL_MODE_REV_TYPE2 0x00
+#define  DP_DUAL_MODE_TYPE_MASK 0xf0
+#define  DP_DUAL_MODE_TYPE_TYPE2 0xa0
+#define DP_DUAL_MODE_IEEE_OUI 0x11 /* 11-13*/
+#define  DP_DUAL_IEEE_OUI_LEN 3
+#define DP_DUAL_DEVICE_ID 0x14 /* 14-19 */
+#define  DP_DUAL_DEVICE_ID_LEN 6
+#define DP_DUAL_MODE_HARDWARE_REV 0x1a
+#define DP_DUAL_MODE_FIRMWARE_MAJOR_REV 0x1b
+#define DP_DUAL_MODE_FIRMWARE_MINOR_REV 0x1c
+#define DP_DUAL_MODE_MAX_TMDS_CLOCK 0x1d
+#define DP_DUAL_MODE_I2C_SPEED_CAP 0x1e
+#define DP_DUAL_MODE_TMDS_OEN 0x20
+#define  DP_DUAL_MODE_TMDS_DISABLE 0x01
+#define DP_DUAL_MODE_HDMI_PIN_CTRL 0x21
+#define  DP_DUAL_MODE_CEC_ENABLE 0x01
+#define DP_DUAL_MODE_I2C_SPEED_CTRL 0x22
+#define DP_DUAL_MODE_LAST_RESERVED 0xff
+
+struct i2c_adapter;
+
+ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
+			      u8 offset, void *buffer, size_t size);
+ssize_t drm_dp_dual_mode_write(struct i2c_adapter *adapter,
+			       u8 offset, const void *buffer, size_t size);
+
+enum drm_dp_dual_mode_type {
+	DRM_DP_DUAL_MODE_NONE,
+	DRM_DP_DUAL_MODE_TYPE1_DVI,
+	DRM_DP_DUAL_MODE_TYPE1_HDMI,
+	DRM_DP_DUAL_MODE_TYPE2_DVI,
+	DRM_DP_DUAL_MODE_TYPE2_HDMI,
+};
+
+enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct i2c_adapter *adapter);
+int drm_dp_dual_mode_max_tmds_clock(struct i2c_adapter *adapter);
+int drm_dp_dual_mode_get_tmds_output(struct i2c_adapter *adapter, bool *enabled);
+int drm_dp_dual_mode_set_tmds_output(struct i2c_adapter *adapter, bool enable);
+const char *drm_dp_get_dual_mode_type_name(enum drm_dp_dual_mode_type type);
+
+#endif
-- 
2.4.10

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm: Add helper for DP++ adaptors

[Zanoni, Paulo R]

Em Ter, 2016-02-23 às 18:46 +0200, ville.syrjala@linux.intel.com
escreveu:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Add a helper which aids in he identification of DP dual mode (aka.
> DP++)
> adaptors. There are several types of adaptors specified:
> type 1 DVI, type 1 HDMI, type 2 DVI, type 2 HDMI
> 
> Type 1 adaptors have a max TMDS clock limit of 165MHz, type 2
> adaptors
> may go as high as 300MHz and they provide a register informing the
> source device what the actual limit is. Supposedly also type 1
> adaptors
> may optionally implement this register. This TMDS clock limit is the
> main reason why we need to identify these adaptors.
> 
> Type 1 adaptors provide access to their internal registers and the
> sink
> DDC bus through I2C. Type 2 adaptors provide this access both via I2C
> and I2C-over-AUX. A type 2 source device may choose to implement
> either
> or both of these methods. If a source device implements only the
> I2C-over-AUX method, then the driver will obviously need specific
> support for such adaptors since the port is driven like an HDMI port,
> but DDC communication happes over the AUX channel.
> 
> This helper should be enough to identify the adaptor type (some
> type 1 DVI adaptors may be a slight exception) and the maximum TMDS
> clock limit. Another feature that may be available is control over
> the TMDS output buffers on the adaptor, possibly allowing for some
> power saving when the TMDS link is down.
> 
> Other user controllable features that may be available in the
> adaptors
> are downstream i2c bus speed control when using i2c-over-aux, and
> some control over the CEC pin. I chose not to provide any helper
> functions for those since I have no use for them in i915 at this
> time.
> The rest of the registers in the adaptor are mostly just information,
> eg. IEEE OUI, hardware and firmware revision, etc.

Please run a spell checker and do some proof-reading both in the commit
message and in the code comments. Multiple instances of "sizo",
"Hoever", "adator", "Identyfy", etc. I also spotted some typos in the
next commits.



> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/Makefile                  |   2 +-
>  drivers/gpu/drm/drm_dp_dual_mode_helper.c | 328
> ++++++++++++++++++++++++++++++
>  include/drm/drm_dp_dual_mode_helper.h     |  80 ++++++++
>  3 files changed, 409 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/drm_dp_dual_mode_helper.c
>  create mode 100644 include/drm/drm_dp_dual_mode_helper.h
> 
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 6eb94fc561dc..22228ef50f36 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -23,7 +23,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
>  
>  drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o
> drm_probe_helper.o \
>  		drm_plane_helper.o drm_dp_mst_topology.o
> drm_atomic_helper.o \
> -		drm_kms_helper_common.o
> +		drm_kms_helper_common.o drm_dp_dual_mode_helper.o
>  
>  drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> new file mode 100644
> index 000000000000..bfe511c09568
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> @@ -0,0 +1,328 @@
> +/*
> + * Copyright © 2016 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person
> obtaining a
> + * copy of this software and associated documentation files (the
> "Software"),
> + * to deal in the Software without restriction, including without
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom
> the
> + * Software is furnished to do so, subject to the following
> conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
> EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM,
> DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
> USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/export.h>
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <drm/drm_dp_dual_mode_helper.h>
> +#include <drm/drmP.h>
> +
> +/**
> + * DOC: DP dual mode (aka. DP++) adaptor helpers
> + *
> + * Helper functions to deal with DP dual mode adaptors.
> + *
> + * Type 1:
> + * Adaptor registers (if any) and the sink DDC bus may be accessed
> via I2C.
> + *
> + * Type 2:
> + * Adaptor registers and sink DDC bus can be accessed either via I2C
> or
> + * I2C-over-AUX. Source devices may choose to implement either one
> or
> + * both of these access methods.
> + */
> +
> +#define DP_DUAL_MODE_SLAVE_ADDRESS 0x40
> +
> +/**
> + * drm_dp_dual_mode_read - Read from the DP dual mode adaptor
> register(s)
> + * adapter: I2C adapter for the DDC bus
> + * offset: register offset
> + * buffer: buffer for return data
> + * size: sizo of the buffer
> + *
> + * Reads @size bytes from the DP dual mode adaptor registers
> + * starting at @offset.
> + *
> + * Returns:
> + * 0 on success, negative error code on failure
> + */
> +ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
> +			      u8 offset, void *buffer, size_t size)
> +{
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr = DP_DUAL_MODE_SLAVE_ADDRESS,
> +			.flags = 0,
> +			.len = 1,
> +			.buf = &offset,
> +		},
> +		{
> +			.addr = DP_DUAL_MODE_SLAVE_ADDRESS,
> +			.flags = I2C_M_RD,
> +			.len = size,
> +			.buf = buffer,
> +		},
> +	};
> +	int ret;
> +
> +	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
> +	if (ret < 0)
> +		return ret;
> +	if (ret != ARRAY_SIZE(msgs))
> +		return -EPROTO;

No retries needed here or below? (asking this due to the comment in
drm_edid.c about retries)

> +
> +	return 0;
> +}
> +
> +/**
> + * drm_dp_dual_mode_write - Write to the DP dual mode adaptor
> register(s)
> + * adapter: I2C adapter for the DDC bus
> + * offset: register offset
> + * buffer: buffer for write data
> + * size: sizo of the buffer
> + *
> + * Writes @size bytes to the DP dual mode adaptor registers
> + * starting at @offset.
> + *
> + * Returns:
> + * 0 on success, negative error code on failure
> + */
> +ssize_t drm_dp_dual_mode_write(struct i2c_adapter *adapter,
> +			       u8 offset, const void *buffer, size_t
> size)
> +{
> +	struct i2c_msg msg = {
> +		.addr = DP_DUAL_MODE_SLAVE_ADDRESS,
> +		.flags = 0,
> +		.len = 1 + size,
> +		.buf = NULL,
> +	};
> +	void *data;
> +	int ret;
> +
> +	data = kmalloc(msg.len, GFP_TEMPORARY);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	msg.buf = data;
> +
> +	memcpy(data, &offset, 1);
> +	memcpy(data + 1, buffer, size);
> +
> +	ret = i2c_transfer(adapter, &msg, 1);
> +
> +	kfree(data);
> +
> +	if (ret < 0)
> +		return ret;
> +	if (ret != 1)
> +		return -EPROTO;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_dp_dual_mode_write);
> +
> +static bool is_hdmi_adaptor(const char
> hdmi_id[DP_DUAL_MODE_HDMI_ID_LEN])
> +{
> +	static const char
> dp_dual_mode_hdmi_id[DP_DUAL_MODE_HDMI_ID_LEN] =
> +		"DP-HDMI ADAPTOR\x04";
> +
> +	return memcmp(hdmi_id, dp_dual_mode_hdmi_id,
> +		      sizeof(dp_dual_mode_hdmi_id)) == 0;
> +}
> +
> +/**
> + * drm_dp_dual_mode_detect - Identyfy the DP dual mode adaptor
> + * adapter: I2C adapter for the DDC bus
> + *
> + * Attempt to identify the type of the DP dual mode adaptor used.
> + *
> + * Note that when the answer is @DRM_DP_DUAL_MODE_NONE it's not

So how about adding a new or just renaming it to
DRM_DP_DUAL_MODE_UNKNOWN? Let's not assume everybody is going to read
the docs.

> + * certain whether we're dealing with a native HDMI port or
> + * a type 1 DVI dual mode adaptor. The driver will have to use
> + * some other hardware/driver specific mechanism to make that
> + * distinction.
> + *
> + * Returns:
> + * The type of the DP dual mode adaptor used
> + */
> +enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct
> i2c_adapter *adapter)
> +{
> +	char hdmi_id[DP_DUAL_MODE_HDMI_ID_LEN] = {};
> +	uint8_t adaptor_id = 0x00;
> +	ssize_t ret;
> +
> +	/*
> +	 * Let's see if the adaptor is there the by reading the
> +	 * HDMI ID registers.
> +	 *
> +	 * Note that type 1 DVI adaptors are not required to
> implemnt
> +	 * any registers, and that presents a problem for detection.
> +	 * If the i2c transfer is nacked, we may or may not be
> dealing
> +	 * with a type 1 DVI adaptor. Some other mechanism of
> detecting
> +	 * the presence of the adaptor is required. One way would be
> +	 * to check the state of the CONFIG1 pin, Another method
> would
> +	 * simply require the driver to know whether the port is a
> DP++
> +	 * port or a native HDMI port. Both of these methods are
> entirely
> +	 * hardware/driver specific so we can't deal with them here.
> +	 */
> +	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_HDMI_ID,
> +				    hdmi_id, sizeof(hdmi_id));
> +	if (ret)
> +		return DRM_DP_DUAL_MODE_NONE;
> +
> +	/*
> +	 * Sigh. Some (maybe all?) type 1 adaptors are broken and
> ack
> +	 * the offset but ignore it, and instead they just always
> return
> +	 * data from the start of the HDMI ID buffer. So for a
> broken
> +	 * type 1 HDMI adaptor a single byte read will always give
> us
> +	 * 0x44, and for a type 1 DVI adaptor it should give 0x00
> +	 * (assuming it implements any registers).

So shouldn't we just try to read from 0x00 to 0x1F in a single shot?
Would this work for your specific adaptor? It would be interesting to
know.


>  Fortunately neither
> +	 * of those values will match the type 2 signature of the
> +	 * DP_DUAL_MODE_ADAPTOR_ID register so we can proceed with
> +	 * the type 2 adaptor detection safely even in the presence
> +	 * of broken type 1 adaptors.
> +	 */
> +	ret = drm_dp_dual_mode_read(adapter,
> DP_DUAL_MODE_ADAPTOR_ID,
> +				    &adaptor_id,
> sizeof(adaptor_id));
> +	if (ret || (adaptor_id != (DP_DUAL_MODE_TYPE_TYPE2 |
> +				   DP_DUAL_MODE_REV_TYPE2))) {
> +		if (is_hdmi_adaptor(hdmi_id))
> +			return DRM_DP_DUAL_MODE_TYPE1_HDMI;
> +		else
> +			return DRM_DP_DUAL_MODE_TYPE1_DVI;
> +	} else {
> +		if (is_hdmi_adaptor(hdmi_id))
> +			return DRM_DP_DUAL_MODE_TYPE2_HDMI;
> +		else
> +			return DRM_DP_DUAL_MODE_TYPE2_DVI;
> +	}
> +}
> +EXPORT_SYMBOL(drm_dp_dual_mode_detect);
> +
> +/**
> + * drm_dp_dual_mode_max_tmds_clock - Max TMDS clock for DP dual mode
> adaptor
> + * adapter: I2C adapter for the DDC bus
> + *
> + * Determine the max TMDS clock the adaptor supports based on the
> + * DP_DUAL_MODE_MAX_TMDS_CLOCK register. The register is mandatory
> for
> + * type 2 adaptors, optional for type 1 adaptors. Hoever, as some
> type 1
> + * adaptors are broken (see comments in drm_dp_dual_mode_detect()
> for the
> + * details) one probably shouldn't use this with type 1 adaptors at
> all.
> + * Type 1 adaptors should anyway be always limited to 165 MHz.

I always assume programmers are as bad as possible, so I wonder if we
should just also call drm_dp_dual_mode_detect() here. Also try the
trick of reading the whole thing at once instead of the specific
address. Same goes for the other functions. I also see that this would
make patches 2-4 different, so this is just an idea, not a requirement.

> + *
> + * Returns:
> + * Maximum supported TMDS clock rate for the DP dual mode adaptor in
> kHz.
> + */
> +int drm_dp_dual_mode_max_tmds_clock(struct i2c_adapter *adapter)
> +{
> +	uint8_t max_tmds_clock;
> +	ssize_t ret;
> +
> +	/*
> +	 * Type 1 adaptors are limited to 165MHz
> 
> +	 * Type 2 adaptors can tells us their limit

"can tells"

> +	 */
> +	ret = drm_dp_dual_mode_read(adapter,
> DP_DUAL_MODE_MAX_TMDS_CLOCK,
> +				    &max_tmds_clock,
> sizeof(max_tmds_clock));
> +	if (ret)

Maybe also "if (ret || max_tmds_clock == 0 || max_tmds_clock == 0xFF)"?


> +		return 165000;
> +
> +	return max_tmds_clock * 5000 / 2;
> +}
> +EXPORT_SYMBOL(drm_dp_dual_mode_max_tmds_clock);
> +
> +/**
> + * drm_dp_dual_mode_get_tmds_output - Get the state of the TMDS
> output buffers in the DP dual mode adaptor
> + * adapter: I2C adapter for the DDC bus
> + * enabled: current state of the TMDS output buffers
> + *
> + * Get the state of the TMDS output buffers in the adaptor.
> + * DP_DUAL_MODE_TMDS_OEN register is mandatory for type 2 adaptors,
> + * optionals for type 1 adaptors. Hoever, as some type 1 adaptors
> are
> + * broken (see comments in drm_dp_dual_mode_detect() for the
> details)
> + * one probably shouldn't use this with type 1 adaptors at all.
> + *
> + * Returns:
> + * 0 on success, negative error code on failure
> + */
> +int drm_dp_dual_mode_get_tmds_output(struct i2c_adapter *adapter,
> bool *enabled)
> +{
> +	uint8_t tmds_oen;
> +	ssize_t ret;
> +
> +	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_TMDS_OEN,
> +				    &tmds_oen, sizeof(tmds_oen));
> +	if (ret)
> +		return ret;
> +
> +	*enabled = !(tmds_oen & DP_DUAL_MODE_TMDS_DISABLE);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_dp_dual_mode_get_tmds_output);
> +
> +/**
> + * drm_dp_dual_mode_set_tmds_output - Enable/disable TMDS output
> buffers in the DP dual mode adaptor
> + * adapter: I2C adapter for the DDC bus
> + * enable: enable (as opposed to disable) the TMDS output buffers
> + *
> + * Set the state of the TMDS output buffers in the adaptor.
> + * DP_DUAL_MODE_TMDS_OEN register is mandatory for type 2 adaptors,
> + * optionals for type 1 adaptors. Hoever, as some type 1 adaptors
> are
> + * broken (see comments in drm_dp_dual_mode_detect() for the
> details)
> + * one probably shouldn't use this with type 1 adaptors at all.
> + *
> + * Returns:
> + * 0 on success, negative error code on failure
> + */
> +int drm_dp_dual_mode_set_tmds_output(struct i2c_adapter *adapter,
> bool enable)
> +{
> +	uint8_t tmds_oen = enable ? 0 : DP_DUAL_MODE_TMDS_DISABLE;
> +	ssize_t ret;
> +
> +	ret = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_TMDS_OEN,
> +				     &tmds_oen, sizeof(tmds_oen));
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_dp_dual_mode_set_tmds_output);
> +
> +/**
> + * drm_dp_get_dual_mode_type_name - Get the name of the DP dual mode
> adaptor type as a string
> + * type: DP dual mode adaptor type
> + *
> + * Returns:
> + * String representation of the DP dual mode adaptor type
> + */
> +const char *drm_dp_get_dual_mode_type_name(enum
> drm_dp_dual_mode_type type)
> +{
> +	switch (type) {
> +	case DRM_DP_DUAL_MODE_NONE:
> +		return "none";
> +	case DRM_DP_DUAL_MODE_TYPE1_DVI:
> +		return "type 1 DVI";
> +	case DRM_DP_DUAL_MODE_TYPE1_HDMI:
> +		return "type 1 HDMI";
> +	case DRM_DP_DUAL_MODE_TYPE2_DVI:
> +		return "type 2 DVI";
> +	case DRM_DP_DUAL_MODE_TYPE2_HDMI:
> +		return "type 2 HDMI";
> +	default:
> +		return "unknown";

Please at least WARN_ON(1)?

And, if you implement my suggestion of adding an actual unknown value
to the enum, don't forget to change the default case to return
something that's not "unknown".

Most of the things I wrote above are just ideas, not requirements, and
having this patch as-is looks better than not having it, so if you at
least fix the spelling errors:

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> +	};
> +
> +}
> +EXPORT_SYMBOL(drm_dp_get_dual_mode_type_name);
> diff --git a/include/drm/drm_dp_dual_mode_helper.h
> b/include/drm/drm_dp_dual_mode_helper.h
> new file mode 100644
> index 000000000000..a2f6b4587f5f
> --- /dev/null
> +++ b/include/drm/drm_dp_dual_mode_helper.h
> @@ -0,0 +1,80 @@
> +/*
> + * Copyright © 2016 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person
> obtaining a
> + * copy of this software and associated documentation files (the
> "Software"),
> + * to deal in the Software without restriction, including without
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom
> the
> + * Software is furnished to do so, subject to the following
> conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
> EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM,
> DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
> USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifndef DRM_DP_DUAL_MODE_HELPER_H
> +#define DRM_DP_DUAL_MODE_HELPER_H
> +
> +#include <linux/types.h>
> +
> +/*
> + * Optional for type 1 DVI adaptors
> + * Mandatory for type 1 HDMI and type 2 adators
> + */
> +#define DP_DUAL_MODE_HDMI_ID 0x00 /* 00-0f */
> +#define  DP_DUAL_MODE_HDMI_ID_LEN 16
> +/*
> + * Optional for type 1 adaptors
> + * Mandatory for type 2 adators
> + */
> +#define DP_DUAL_MODE_ADAPTOR_ID 0x10
> +#define  DP_DUAL_MODE_REV_MASK 0x07
> +#define  DP_DUAL_MODE_REV_TYPE2 0x00
> +#define  DP_DUAL_MODE_TYPE_MASK 0xf0
> +#define  DP_DUAL_MODE_TYPE_TYPE2 0xa0
> +#define DP_DUAL_MODE_IEEE_OUI 0x11 /* 11-13*/
> +#define  DP_DUAL_IEEE_OUI_LEN 3
> +#define DP_DUAL_DEVICE_ID 0x14 /* 14-19 */
> +#define  DP_DUAL_DEVICE_ID_LEN 6
> +#define DP_DUAL_MODE_HARDWARE_REV 0x1a
> +#define DP_DUAL_MODE_FIRMWARE_MAJOR_REV 0x1b
> +#define DP_DUAL_MODE_FIRMWARE_MINOR_REV 0x1c
> +#define DP_DUAL_MODE_MAX_TMDS_CLOCK 0x1d
> +#define DP_DUAL_MODE_I2C_SPEED_CAP 0x1e
> +#define DP_DUAL_MODE_TMDS_OEN 0x20
> +#define  DP_DUAL_MODE_TMDS_DISABLE 0x01
> +#define DP_DUAL_MODE_HDMI_PIN_CTRL 0x21
> +#define  DP_DUAL_MODE_CEC_ENABLE 0x01
> +#define DP_DUAL_MODE_I2C_SPEED_CTRL 0x22
> +#define DP_DUAL_MODE_LAST_RESERVED 0xff
> +
> +struct i2c_adapter;
> +
> +ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
> +			      u8 offset, void *buffer, size_t size);
> +ssize_t drm_dp_dual_mode_write(struct i2c_adapter *adapter,
> +			       u8 offset, const void *buffer, size_t
> size);
> +
> +enum drm_dp_dual_mode_type {
> +	DRM_DP_DUAL_MODE_NONE,
> +	DRM_DP_DUAL_MODE_TYPE1_DVI,
> +	DRM_DP_DUAL_MODE_TYPE1_HDMI,
> +	DRM_DP_DUAL_MODE_TYPE2_DVI,
> +	DRM_DP_DUAL_MODE_TYPE2_HDMI,
> +};
> +
> +enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct
> i2c_adapter *adapter);
> +int drm_dp_dual_mode_max_tmds_clock(struct i2c_adapter *adapter);
> +int drm_dp_dual_mode_get_tmds_output(struct i2c_adapter *adapter,
> bool *enabled);
> +int drm_dp_dual_mode_set_tmds_output(struct i2c_adapter *adapter,
> bool enable);
> +const char *drm_dp_get_dual_mode_type_name(enum
> drm_dp_dual_mode_type type);
> +
> +#endif
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm: Add helper for DP++ adaptors

[Ville Syrjälä]

On Thu, Mar 31, 2016 at 07:25:36PM +0000, Zanoni, Paulo R wrote:
> Em Ter, 2016-02-23 às 18:46 +0200, ville.syrjala@linux.intel.com
> escreveu:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Add a helper which aids in he identification of DP dual mode (aka.
> > DP++)
> > adaptors. There are several types of adaptors specified:
> > type 1 DVI, type 1 HDMI, type 2 DVI, type 2 HDMI
> > 
> > Type 1 adaptors have a max TMDS clock limit of 165MHz, type 2
> > adaptors
> > may go as high as 300MHz and they provide a register informing the
> > source device what the actual limit is. Supposedly also type 1
> > adaptors
> > may optionally implement this register. This TMDS clock limit is the
> > main reason why we need to identify these adaptors.
> > 
> > Type 1 adaptors provide access to their internal registers and the
> > sink
> > DDC bus through I2C. Type 2 adaptors provide this access both via I2C
> > and I2C-over-AUX. A type 2 source device may choose to implement
> > either
> > or both of these methods. If a source device implements only the
> > I2C-over-AUX method, then the driver will obviously need specific
> > support for such adaptors since the port is driven like an HDMI port,
> > but DDC communication happes over the AUX channel.
> > 
> > This helper should be enough to identify the adaptor type (some
> > type 1 DVI adaptors may be a slight exception) and the maximum TMDS
> > clock limit. Another feature that may be available is control over
> > the TMDS output buffers on the adaptor, possibly allowing for some
> > power saving when the TMDS link is down.
> > 
> > Other user controllable features that may be available in the
> > adaptors
> > are downstream i2c bus speed control when using i2c-over-aux, and
> > some control over the CEC pin. I chose not to provide any helper
> > functions for those since I have no use for them in i915 at this
> > time.
> > The rest of the registers in the adaptor are mostly just information,
> > eg. IEEE OUI, hardware and firmware revision, etc.
> 
> Please run a spell checker and do some proof-reading both in the commit
> message and in the code comments. Multiple instances of "sizo",
> "Hoever", "adator", "Identyfy", etc. I also spotted some typos in the
> next commits.
> 
> 
> 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/Makefile                  |   2 +-
> >  drivers/gpu/drm/drm_dp_dual_mode_helper.c | 328
> > ++++++++++++++++++++++++++++++
> >  include/drm/drm_dp_dual_mode_helper.h     |  80 ++++++++
> >  3 files changed, 409 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/drm_dp_dual_mode_helper.c
> >  create mode 100644 include/drm/drm_dp_dual_mode_helper.h
> > 
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index 6eb94fc561dc..22228ef50f36 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -23,7 +23,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
> >  
> >  drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o
> > drm_probe_helper.o \
> >  		drm_plane_helper.o drm_dp_mst_topology.o
> > drm_atomic_helper.o \
> > -		drm_kms_helper_common.o
> > +		drm_kms_helper_common.o drm_dp_dual_mode_helper.o
> >  
> >  drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> >  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> > diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> > b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> > new file mode 100644
> > index 000000000000..bfe511c09568
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> > @@ -0,0 +1,328 @@
> > +/*
> > + * Copyright © 2016 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> > obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > + * to deal in the Software without restriction, including without
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute,
> > sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom
> > the
> > + * Software is furnished to do so, subject to the following
> > conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be
> > included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
> > EVENT SHALL
> > + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM,
> > DAMAGES OR
> > + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> > OTHERWISE,
> > + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
> > USE OR
> > + * OTHER DEALINGS IN THE SOFTWARE.
> > + */
> > +
> > +#include <linux/errno.h>
> > +#include <linux/export.h>
> > +#include <linux/i2c.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +#include <drm/drm_dp_dual_mode_helper.h>
> > +#include <drm/drmP.h>
> > +
> > +/**
> > + * DOC: DP dual mode (aka. DP++) adaptor helpers
> > + *
> > + * Helper functions to deal with DP dual mode adaptors.
> > + *
> > + * Type 1:
> > + * Adaptor registers (if any) and the sink DDC bus may be accessed
> > via I2C.
> > + *
> > + * Type 2:
> > + * Adaptor registers and sink DDC bus can be accessed either via I2C
> > or
> > + * I2C-over-AUX. Source devices may choose to implement either one
> > or
> > + * both of these access methods.
> > + */
> > +
> > +#define DP_DUAL_MODE_SLAVE_ADDRESS 0x40
> > +
> > +/**
> > + * drm_dp_dual_mode_read - Read from the DP dual mode adaptor
> > register(s)
> > + * adapter: I2C adapter for the DDC bus
> > + * offset: register offset
> > + * buffer: buffer for return data
> > + * size: sizo of the buffer
> > + *
> > + * Reads @size bytes from the DP dual mode adaptor registers
> > + * starting at @offset.
> > + *
> > + * Returns:
> > + * 0 on success, negative error code on failure
> > + */
> > +ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
> > +			      u8 offset, void *buffer, size_t size)
> > +{
> > +	struct i2c_msg msgs[] = {
> > +		{
> > +			.addr = DP_DUAL_MODE_SLAVE_ADDRESS,
> > +			.flags = 0,
> > +			.len = 1,
> > +			.buf = &offset,
> > +		},
> > +		{
> > +			.addr = DP_DUAL_MODE_SLAVE_ADDRESS,
> > +			.flags = I2C_M_RD,
> > +			.len = size,
> > +			.buf = buffer,
> > +		},
> > +	};
> > +	int ret;
> > +
> > +	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
> > +	if (ret < 0)
> > +		return ret;
> > +	if (ret != ARRAY_SIZE(msgs))
> > +		return -EPROTO;
> 
> No retries needed here or below? (asking this due to the comment in
> drm_edid.c about retries)

The EDID retries are mostly cargo culted. I suspect you don't really
need them unless the connector is about to fall off, or the wiring
is made of cotton.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * drm_dp_dual_mode_write - Write to the DP dual mode adaptor
> > register(s)
> > + * adapter: I2C adapter for the DDC bus
> > + * offset: register offset
> > + * buffer: buffer for write data
> > + * size: sizo of the buffer
> > + *
> > + * Writes @size bytes to the DP dual mode adaptor registers
> > + * starting at @offset.
> > + *
> > + * Returns:
> > + * 0 on success, negative error code on failure
> > + */
> > +ssize_t drm_dp_dual_mode_write(struct i2c_adapter *adapter,
> > +			       u8 offset, const void *buffer, size_t
> > size)
> > +{
> > +	struct i2c_msg msg = {
> > +		.addr = DP_DUAL_MODE_SLAVE_ADDRESS,
> > +		.flags = 0,
> > +		.len = 1 + size,
> > +		.buf = NULL,
> > +	};
> > +	void *data;
> > +	int ret;
> > +
> > +	data = kmalloc(msg.len, GFP_TEMPORARY);
> > +	if (!data)
> > +		return -ENOMEM;
> > +
> > +	msg.buf = data;
> > +
> > +	memcpy(data, &offset, 1);
> > +	memcpy(data + 1, buffer, size);
> > +
> > +	ret = i2c_transfer(adapter, &msg, 1);
> > +
> > +	kfree(data);
> > +
> > +	if (ret < 0)
> > +		return ret;
> > +	if (ret != 1)
> > +		return -EPROTO;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_dp_dual_mode_write);
> > +
> > +static bool is_hdmi_adaptor(const char
> > hdmi_id[DP_DUAL_MODE_HDMI_ID_LEN])
> > +{
> > +	static const char
> > dp_dual_mode_hdmi_id[DP_DUAL_MODE_HDMI_ID_LEN] =
> > +		"DP-HDMI ADAPTOR\x04";
> > +
> > +	return memcmp(hdmi_id, dp_dual_mode_hdmi_id,
> > +		      sizeof(dp_dual_mode_hdmi_id)) == 0;
> > +}
> > +
> > +/**
> > + * drm_dp_dual_mode_detect - Identyfy the DP dual mode adaptor
> > + * adapter: I2C adapter for the DDC bus
> > + *
> > + * Attempt to identify the type of the DP dual mode adaptor used.
> > + *
> > + * Note that when the answer is @DRM_DP_DUAL_MODE_NONE it's not
> 
> So how about adding a new or just renaming it to
> DRM_DP_DUAL_MODE_UNKNOWN? Let's not assume everybody is going to read
> the docs.

I suppose adding an extra enum value can't hurt.

> 
> > + * certain whether we're dealing with a native HDMI port or
> > + * a type 1 DVI dual mode adaptor. The driver will have to use
> > + * some other hardware/driver specific mechanism to make that
> > + * distinction.
> > + *
> > + * Returns:
> > + * The type of the DP dual mode adaptor used
> > + */
> > +enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct
> > i2c_adapter *adapter)
> > +{
> > +	char hdmi_id[DP_DUAL_MODE_HDMI_ID_LEN] = {};
> > +	uint8_t adaptor_id = 0x00;
> > +	ssize_t ret;
> > +
> > +	/*
> > +	 * Let's see if the adaptor is there the by reading the
> > +	 * HDMI ID registers.
> > +	 *
> > +	 * Note that type 1 DVI adaptors are not required to
> > implemnt
> > +	 * any registers, and that presents a problem for detection.
> > +	 * If the i2c transfer is nacked, we may or may not be
> > dealing
> > +	 * with a type 1 DVI adaptor. Some other mechanism of
> > detecting
> > +	 * the presence of the adaptor is required. One way would be
> > +	 * to check the state of the CONFIG1 pin, Another method
> > would
> > +	 * simply require the driver to know whether the port is a
> > DP++
> > +	 * port or a native HDMI port. Both of these methods are
> > entirely
> > +	 * hardware/driver specific so we can't deal with them here.
> > +	 */
> > +	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_HDMI_ID,
> > +				    hdmi_id, sizeof(hdmi_id));
> > +	if (ret)
> > +		return DRM_DP_DUAL_MODE_NONE;
> > +
> > +	/*
> > +	 * Sigh. Some (maybe all?) type 1 adaptors are broken and
> > ack
> > +	 * the offset but ignore it, and instead they just always
> > return
> > +	 * data from the start of the HDMI ID buffer. So for a
> > broken
> > +	 * type 1 HDMI adaptor a single byte read will always give
> > us
> > +	 * 0x44, and for a type 1 DVI adaptor it should give 0x00
> > +	 * (assuming it implements any registers).
> 
> So shouldn't we just try to read from 0x00 to 0x1F in a single shot?
> Would this work for your specific adaptor? It would be interesting to
> know.

I think I did consider that after giving up on another strange detection
scheme that would have perhaps allowed me to detect decent type 1
adaptors. IIRC the adaptors I tested returned 0x00 or 0xff for the
0x10-0x20 range when doing a single read, but I suspect they might as
well just wrap after 0x0f since the spec says they don't need to implement
anything beyond 0x00-0x0f. While all of those results would work just as
well as reading the single byte, I decided that there's no point in doing
that since I had not seem a single sane type 1 adaptor. They're clearly
all just bottom of the barrel hardware so seems unlikely anyone would
go to the effort of implementing one decently. So in the end I just
figured that minimizing the amount of data we have to read is the sane
thing to do.

> 
> 
> >  Fortunately neither
> > +	 * of those values will match the type 2 signature of the
> > +	 * DP_DUAL_MODE_ADAPTOR_ID register so we can proceed with
> > +	 * the type 2 adaptor detection safely even in the presence
> > +	 * of broken type 1 adaptors.
> > +	 */
> > +	ret = drm_dp_dual_mode_read(adapter,
> > DP_DUAL_MODE_ADAPTOR_ID,
> > +				    &adaptor_id,
> > sizeof(adaptor_id));
> > +	if (ret || (adaptor_id != (DP_DUAL_MODE_TYPE_TYPE2 |
> > +				   DP_DUAL_MODE_REV_TYPE2))) {
> > +		if (is_hdmi_adaptor(hdmi_id))
> > +			return DRM_DP_DUAL_MODE_TYPE1_HDMI;
> > +		else
> > +			return DRM_DP_DUAL_MODE_TYPE1_DVI;
> > +	} else {
> > +		if (is_hdmi_adaptor(hdmi_id))
> > +			return DRM_DP_DUAL_MODE_TYPE2_HDMI;
> > +		else
> > +			return DRM_DP_DUAL_MODE_TYPE2_DVI;
> > +	}
> > +}
> > +EXPORT_SYMBOL(drm_dp_dual_mode_detect);
> > +
> > +/**
> > + * drm_dp_dual_mode_max_tmds_clock - Max TMDS clock for DP dual mode
> > adaptor
> > + * adapter: I2C adapter for the DDC bus
> > + *
> > + * Determine the max TMDS clock the adaptor supports based on the
> > + * DP_DUAL_MODE_MAX_TMDS_CLOCK register. The register is mandatory
> > for
> > + * type 2 adaptors, optional for type 1 adaptors. Hoever, as some
> > type 1
> > + * adaptors are broken (see comments in drm_dp_dual_mode_detect()
> > for the
> > + * details) one probably shouldn't use this with type 1 adaptors at
> > all.
> > + * Type 1 adaptors should anyway be always limited to 165 MHz.
> 
> I always assume programmers are as bad as possible, so I wonder if we
> should just also call drm_dp_dual_mode_detect() here.

This crossed my mind but I didn't want to slow down things needlessly.

I did in fact consider that we might want to raise the abstraction
level a bit and introduce some kind of dp_dual_mode object that keeps
around the relevant information/state and would allow drivers to avoid
the type checks and whatnot. Unfortunately those cursed type 1 DVI
adaptors would still require the driver to step in if the detection
fails to find anything, so we'd need a callback into the driver or
something.

And so, since the driver anyway has to deal with the type 1 DVI adaptors,
I decided that trying to hide these sort of details inside the helper
is pretty much pointless.

> Also try the
> trick of reading the whole thing at once instead of the specific
> address. Same goes for the other functions. I also see that this would
> make patches 2-4 different, so this is just an idea, not a requirement.

As I suspect some adaptors might just wrap around at 0xf, doing a
big read doesn't feel entirely safe here.

> 
> > + *
> > + * Returns:
> > + * Maximum supported TMDS clock rate for the DP dual mode adaptor in
> > kHz.
> > + */
> > +int drm_dp_dual_mode_max_tmds_clock(struct i2c_adapter *adapter)
> > +{
> > +	uint8_t max_tmds_clock;
> > +	ssize_t ret;
> > +
> > +	/*
> > +	 * Type 1 adaptors are limited to 165MHz
> > 
> > +	 * Type 2 adaptors can tells us their limit
> 
> "can tells"
> 
> > +	 */
> > +	ret = drm_dp_dual_mode_read(adapter,
> > DP_DUAL_MODE_MAX_TMDS_CLOCK,
> > +				    &max_tmds_clock,
> > sizeof(max_tmds_clock));
> > +	if (ret)
> 
> Maybe also "if (ret || max_tmds_clock == 0 || max_tmds_clock == 0xFF)"?

That would seem sensible indeed.

> 
> 
> > +		return 165000;
> > +
> > +	return max_tmds_clock * 5000 / 2;
> > +}
> > +EXPORT_SYMBOL(drm_dp_dual_mode_max_tmds_clock);
> > +
> > +/**
> > + * drm_dp_dual_mode_get_tmds_output - Get the state of the TMDS
> > output buffers in the DP dual mode adaptor
> > + * adapter: I2C adapter for the DDC bus
> > + * enabled: current state of the TMDS output buffers
> > + *
> > + * Get the state of the TMDS output buffers in the adaptor.
> > + * DP_DUAL_MODE_TMDS_OEN register is mandatory for type 2 adaptors,
> > + * optionals for type 1 adaptors. Hoever, as some type 1 adaptors
> > are
> > + * broken (see comments in drm_dp_dual_mode_detect() for the
> > details)
> > + * one probably shouldn't use this with type 1 adaptors at all.
> > + *
> > + * Returns:
> > + * 0 on success, negative error code on failure
> > + */
> > +int drm_dp_dual_mode_get_tmds_output(struct i2c_adapter *adapter,
> > bool *enabled)
> > +{
> > +	uint8_t tmds_oen;
> > +	ssize_t ret;
> > +
> > +	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_TMDS_OEN,
> > +				    &tmds_oen, sizeof(tmds_oen));
> > +	if (ret)
> > +		return ret;
> > +
> > +	*enabled = !(tmds_oen & DP_DUAL_MODE_TMDS_DISABLE);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_dp_dual_mode_get_tmds_output);
> > +
> > +/**
> > + * drm_dp_dual_mode_set_tmds_output - Enable/disable TMDS output
> > buffers in the DP dual mode adaptor
> > + * adapter: I2C adapter for the DDC bus
> > + * enable: enable (as opposed to disable) the TMDS output buffers
> > + *
> > + * Set the state of the TMDS output buffers in the adaptor.
> > + * DP_DUAL_MODE_TMDS_OEN register is mandatory for type 2 adaptors,
> > + * optionals for type 1 adaptors. Hoever, as some type 1 adaptors
> > are
> > + * broken (see comments in drm_dp_dual_mode_detect() for the
> > details)
> > + * one probably shouldn't use this with type 1 adaptors at all.
> > + *
> > + * Returns:
> > + * 0 on success, negative error code on failure
> > + */
> > +int drm_dp_dual_mode_set_tmds_output(struct i2c_adapter *adapter,
> > bool enable)
> > +{
> > +	uint8_t tmds_oen = enable ? 0 : DP_DUAL_MODE_TMDS_DISABLE;
> > +	ssize_t ret;
> > +
> > +	ret = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_TMDS_OEN,
> > +				     &tmds_oen, sizeof(tmds_oen));
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_dp_dual_mode_set_tmds_output);
> > +
> > +/**
> > + * drm_dp_get_dual_mode_type_name - Get the name of the DP dual mode
> > adaptor type as a string
> > + * type: DP dual mode adaptor type
> > + *
> > + * Returns:
> > + * String representation of the DP dual mode adaptor type
> > + */
> > +const char *drm_dp_get_dual_mode_type_name(enum
> > drm_dp_dual_mode_type type)
> > +{
> > +	switch (type) {
> > +	case DRM_DP_DUAL_MODE_NONE:
> > +		return "none";
> > +	case DRM_DP_DUAL_MODE_TYPE1_DVI:
> > +		return "type 1 DVI";
> > +	case DRM_DP_DUAL_MODE_TYPE1_HDMI:
> > +		return "type 1 HDMI";
> > +	case DRM_DP_DUAL_MODE_TYPE2_DVI:
> > +		return "type 2 DVI";
> > +	case DRM_DP_DUAL_MODE_TYPE2_HDMI:
> > +		return "type 2 HDMI";
> > +	default:
> > +		return "unknown";
> 
> Please at least WARN_ON(1)?

We don't generally warn in these case (in the drm core).

> 
> And, if you implement my suggestion of adding an actual unknown value
> to the enum, don't forget to change the default case to return
> something that's not "unknown".
> 
> Most of the things I wrote above are just ideas, not requirements, and
> having this patch as-is looks better than not having it, so if you at
> least fix the spelling errors:
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> > +	};
> > +
> > +}
> > +EXPORT_SYMBOL(drm_dp_get_dual_mode_type_name);
> > diff --git a/include/drm/drm_dp_dual_mode_helper.h
> > b/include/drm/drm_dp_dual_mode_helper.h
> > new file mode 100644
> > index 000000000000..a2f6b4587f5f
> > --- /dev/null
> > +++ b/include/drm/drm_dp_dual_mode_helper.h
> > @@ -0,0 +1,80 @@
> > +/*
> > + * Copyright © 2016 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> > obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > + * to deal in the Software without restriction, including without
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute,
> > sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom
> > the
> > + * Software is furnished to do so, subject to the following
> > conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be
> > included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
> > EVENT SHALL
> > + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM,
> > DAMAGES OR
> > + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> > OTHERWISE,
> > + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
> > USE OR
> > + * OTHER DEALINGS IN THE SOFTWARE.
> > + */
> > +
> > +#ifndef DRM_DP_DUAL_MODE_HELPER_H
> > +#define DRM_DP_DUAL_MODE_HELPER_H
> > +
> > +#include <linux/types.h>
> > +
> > +/*
> > + * Optional for type 1 DVI adaptors
> > + * Mandatory for type 1 HDMI and type 2 adators
> > + */
> > +#define DP_DUAL_MODE_HDMI_ID 0x00 /* 00-0f */
> > +#define  DP_DUAL_MODE_HDMI_ID_LEN 16
> > +/*
> > + * Optional for type 1 adaptors
> > + * Mandatory for type 2 adators
> > + */
> > +#define DP_DUAL_MODE_ADAPTOR_ID 0x10
> > +#define  DP_DUAL_MODE_REV_MASK 0x07
> > +#define  DP_DUAL_MODE_REV_TYPE2 0x00
> > +#define  DP_DUAL_MODE_TYPE_MASK 0xf0
> > +#define  DP_DUAL_MODE_TYPE_TYPE2 0xa0
> > +#define DP_DUAL_MODE_IEEE_OUI 0x11 /* 11-13*/
> > +#define  DP_DUAL_IEEE_OUI_LEN 3
> > +#define DP_DUAL_DEVICE_ID 0x14 /* 14-19 */
> > +#define  DP_DUAL_DEVICE_ID_LEN 6
> > +#define DP_DUAL_MODE_HARDWARE_REV 0x1a
> > +#define DP_DUAL_MODE_FIRMWARE_MAJOR_REV 0x1b
> > +#define DP_DUAL_MODE_FIRMWARE_MINOR_REV 0x1c
> > +#define DP_DUAL_MODE_MAX_TMDS_CLOCK 0x1d
> > +#define DP_DUAL_MODE_I2C_SPEED_CAP 0x1e
> > +#define DP_DUAL_MODE_TMDS_OEN 0x20
> > +#define  DP_DUAL_MODE_TMDS_DISABLE 0x01
> > +#define DP_DUAL_MODE_HDMI_PIN_CTRL 0x21
> > +#define  DP_DUAL_MODE_CEC_ENABLE 0x01
> > +#define DP_DUAL_MODE_I2C_SPEED_CTRL 0x22
> > +#define DP_DUAL_MODE_LAST_RESERVED 0xff
> > +
> > +struct i2c_adapter;
> > +
> > +ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
> > +			      u8 offset, void *buffer, size_t size);
> > +ssize_t drm_dp_dual_mode_write(struct i2c_adapter *adapter,
> > +			       u8 offset, const void *buffer, size_t
> > size);
> > +
> > +enum drm_dp_dual_mode_type {
> > +	DRM_DP_DUAL_MODE_NONE,
> > +	DRM_DP_DUAL_MODE_TYPE1_DVI,
> > +	DRM_DP_DUAL_MODE_TYPE1_HDMI,
> > +	DRM_DP_DUAL_MODE_TYPE2_DVI,
> > +	DRM_DP_DUAL_MODE_TYPE2_HDMI,
> > +};
> > +
> > +enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct
> > i2c_adapter *adapter);
> > +int drm_dp_dual_mode_max_tmds_clock(struct i2c_adapter *adapter);
> > +int drm_dp_dual_mode_get_tmds_output(struct i2c_adapter *adapter,
> > bool *enabled);
> > +int drm_dp_dual_mode_set_tmds_output(struct i2c_adapter *adapter,
> > bool enable);
> > +const char *drm_dp_get_dual_mode_type_name(enum
> > drm_dp_dual_mode_type type);
> > +
> > +#endif

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

[PATCH 2/4] drm/i915: Respect DP++ adaptor TMDS clock limit

[ville.syrjala]

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

Try to detect the max TMDS clock limit for the DP++ adaptor (if any)
and take it into account when checking the port clock.

Note that as with the sink (HDMI vs. DVI) TMDS clock limit we'll ignore
the adaptor TMDS clock limit in the modeset path, in case users are
already "overclocking" their TMDS links. One subtle change here is that
we'll have to respect the adaptor TMDS clock limit when we decide whether
to do 12bpc or 8bpc, otherwise we might end up picking 12bpc and
accidentally driving the TMDS link out of spec even when the user chose
a mode that fits wihting the limits at 8bpc. This means you can't
"overclock" your DP++ dongle at 12bpc anymore, but you can continue to
do so at 8bpc.

Note that for simplicity we'll use the I2C access method for all dual
mode adaptors including type 2. Otherwise we'd have to start mixing
DP AUX and HDMI together. In the future we may need to do that if we
come across any board designs that don't hook up the DDC pins to the
DP++ connectors. Such boards would obviously only work with type 2
dual mode adaptors, and not type 1.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h  |  3 ++
 drivers/gpu/drm/i915/intel_hdmi.c | 65 ++++++++++++++++++++++++++++++++++-----
 2 files changed, 60 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4852049c9ab3..944eacbfb6a9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -704,6 +704,9 @@ struct cxsr_latency {
 struct intel_hdmi {
 	i915_reg_t hdmi_reg;
 	int ddc_bus;
+	struct {
+		int max_tmds_clock;
+	} dp_dual_mode;
 	bool limited_color_range;
 	bool color_range_auto;
 	bool has_hdmi_sink;
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 80b44c054087..1e8cfdb18dc7 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -34,6 +34,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_dp_dual_mode_helper.h>
 #include "intel_drv.h"
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
@@ -1168,27 +1169,42 @@ static void pch_post_disable_hdmi(struct intel_encoder *encoder)
 	intel_disable_hdmi(encoder);
 }
 
-static int hdmi_port_clock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
+static int intel_hdmi_source_max_tmds_clock(struct drm_i915_private *dev_priv)
 {
-	struct drm_device *dev = intel_hdmi_to_dev(hdmi);
-
-	if ((respect_dvi_limit && !hdmi->has_hdmi_sink) || IS_G4X(dev))
+	if (IS_G4X(dev_priv))
 		return 165000;
-	else if (IS_HASWELL(dev) || INTEL_INFO(dev)->gen >= 8)
+	else if (IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8)
 		return 300000;
 	else
 		return 225000;
 }
 
+static int hdmi_port_clock_limit(struct intel_hdmi *hdmi,
+				 bool respect_downstream_limits)
+{
+	struct drm_device *dev = intel_hdmi_to_dev(hdmi);
+	int max_tmds_clock = intel_hdmi_source_max_tmds_clock(to_i915(dev));
+
+	if (respect_downstream_limits) {
+		if (hdmi->dp_dual_mode.max_tmds_clock)
+			max_tmds_clock = min(max_tmds_clock,
+					     hdmi->dp_dual_mode.max_tmds_clock);
+		if (!hdmi->has_hdmi_sink)
+			max_tmds_clock = min(max_tmds_clock, 165000);
+	}
+
+	return max_tmds_clock;
+}
+
 static enum drm_mode_status
 hdmi_port_clock_valid(struct intel_hdmi *hdmi,
-		      int clock, bool respect_dvi_limit)
+		      int clock, bool respect_downstream_limits)
 {
 	struct drm_device *dev = intel_hdmi_to_dev(hdmi);
 
 	if (clock < 25000)
 		return MODE_CLOCK_LOW;
-	if (clock > hdmi_port_clock_limit(hdmi, respect_dvi_limit))
+	if (clock > hdmi_port_clock_limit(hdmi, respect_downstream_limits))
 		return MODE_CLOCK_HIGH;
 
 	/* BXT DPLL can't generate 223-240 MHz */
@@ -1312,7 +1328,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
 	 * within limits.
 	 */
 	if (pipe_config->pipe_bpp > 8*3 && pipe_config->has_hdmi_sink &&
-	    hdmi_port_clock_valid(intel_hdmi, clock_12bpc, false) == MODE_OK &&
+	    hdmi_port_clock_valid(intel_hdmi, clock_12bpc, true) == MODE_OK &&
 	    hdmi_12bpc_possible(pipe_config)) {
 		DRM_DEBUG_KMS("picking bpc to 12 for HDMI output\n");
 		desired_bpp = 12*3;
@@ -1352,10 +1368,41 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
 	intel_hdmi->has_audio = false;
 	intel_hdmi->rgb_quant_range_selectable = false;
 
+	intel_hdmi->dp_dual_mode.max_tmds_clock = 0;
+
 	kfree(to_intel_connector(connector)->detect_edid);
 	to_intel_connector(connector)->detect_edid = NULL;
 }
 
+static void
+intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector)
+{
+	struct drm_i915_private *dev_priv = to_i915(connector->dev);
+	struct intel_hdmi *hdmi = intel_attached_hdmi(connector);
+	struct i2c_adapter *adapter =
+		intel_gmbus_get_adapter(dev_priv, hdmi->ddc_bus);
+	enum drm_dp_dual_mode_type type = drm_dp_dual_mode_detect(adapter);
+
+	if (type == DRM_DP_DUAL_MODE_NONE)
+		return;
+
+	/*
+	 * Type 1 adaptors can be broken unfortunately, so let's
+	 * not trust anything they say beyond the type identification.
+	 */
+	if (type == DRM_DP_DUAL_MODE_TYPE2_DVI ||
+	    type == DRM_DP_DUAL_MODE_TYPE2_HDMI) {
+		hdmi->dp_dual_mode.max_tmds_clock =
+			drm_dp_dual_mode_max_tmds_clock(adapter);
+	} else {
+		hdmi->dp_dual_mode.max_tmds_clock = 165000;
+	}
+
+	DRM_DEBUG_KMS("DP dual mode adaptor (%s) detected (max TMDS clock: %d kHz)\n",
+		      drm_dp_get_dual_mode_type_name(type),
+		      hdmi->dp_dual_mode.max_tmds_clock);
+}
+
 static bool
 intel_hdmi_set_edid(struct drm_connector *connector, bool force)
 {
@@ -1371,6 +1418,8 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force)
 				    intel_gmbus_get_adapter(dev_priv,
 				    intel_hdmi->ddc_bus));
 
+		intel_hdmi_dp_dual_mode_detect(connector);
+
 		intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
 	}
 
-- 
2.4.10

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm/i915: Respect DP++ adaptor TMDS clock limit

[Ville Syrjälä]

On Tue, Feb 23, 2016 at 06:46:26PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Try to detect the max TMDS clock limit for the DP++ adaptor (if any)
> and take it into account when checking the port clock.
> 
> Note that as with the sink (HDMI vs. DVI) TMDS clock limit we'll ignore
> the adaptor TMDS clock limit in the modeset path, in case users are
> already "overclocking" their TMDS links. One subtle change here is that
> we'll have to respect the adaptor TMDS clock limit when we decide whether
> to do 12bpc or 8bpc, otherwise we might end up picking 12bpc and
> accidentally driving the TMDS link out of spec even when the user chose
> a mode that fits wihting the limits at 8bpc. This means you can't
> "overclock" your DP++ dongle at 12bpc anymore, but you can continue to
> do so at 8bpc.
> 
> Note that for simplicity we'll use the I2C access method for all dual
> mode adaptors including type 2. Otherwise we'd have to start mixing
> DP AUX and HDMI together. In the future we may need to do that if we
> come across any board designs that don't hook up the DDC pins to the
> DP++ connectors. Such boards would obviously only work with type 2
> dual mode adaptors, and not type 1.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reported-by: Tore Anderson <tore@fud.no>
Tested-by: Tore Anderson <tore@fud.no>
Fixes: 7a0baa623446 ("Revert "drm/i915: Disable 12bpc hdmi for now"")
References: https://lists.freedesktop.org/archives/intel-gfx/2016-February/088336.html

> ---
>  drivers/gpu/drm/i915/intel_drv.h  |  3 ++
>  drivers/gpu/drm/i915/intel_hdmi.c | 65 ++++++++++++++++++++++++++++++++++-----
>  2 files changed, 60 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4852049c9ab3..944eacbfb6a9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -704,6 +704,9 @@ struct cxsr_latency {
>  struct intel_hdmi {
>  	i915_reg_t hdmi_reg;
>  	int ddc_bus;
> +	struct {
> +		int max_tmds_clock;
> +	} dp_dual_mode;
>  	bool limited_color_range;
>  	bool color_range_auto;
>  	bool has_hdmi_sink;
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 80b44c054087..1e8cfdb18dc7 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -34,6 +34,7 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_dp_dual_mode_helper.h>
>  #include "intel_drv.h"
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
> @@ -1168,27 +1169,42 @@ static void pch_post_disable_hdmi(struct intel_encoder *encoder)
>  	intel_disable_hdmi(encoder);
>  }
>  
> -static int hdmi_port_clock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
> +static int intel_hdmi_source_max_tmds_clock(struct drm_i915_private *dev_priv)
>  {
> -	struct drm_device *dev = intel_hdmi_to_dev(hdmi);
> -
> -	if ((respect_dvi_limit && !hdmi->has_hdmi_sink) || IS_G4X(dev))
> +	if (IS_G4X(dev_priv))
>  		return 165000;
> -	else if (IS_HASWELL(dev) || INTEL_INFO(dev)->gen >= 8)
> +	else if (IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8)
>  		return 300000;
>  	else
>  		return 225000;
>  }
>  
> +static int hdmi_port_clock_limit(struct intel_hdmi *hdmi,
> +				 bool respect_downstream_limits)
> +{
> +	struct drm_device *dev = intel_hdmi_to_dev(hdmi);
> +	int max_tmds_clock = intel_hdmi_source_max_tmds_clock(to_i915(dev));
> +
> +	if (respect_downstream_limits) {
> +		if (hdmi->dp_dual_mode.max_tmds_clock)
> +			max_tmds_clock = min(max_tmds_clock,
> +					     hdmi->dp_dual_mode.max_tmds_clock);
> +		if (!hdmi->has_hdmi_sink)
> +			max_tmds_clock = min(max_tmds_clock, 165000);
> +	}
> +
> +	return max_tmds_clock;
> +}
> +
>  static enum drm_mode_status
>  hdmi_port_clock_valid(struct intel_hdmi *hdmi,
> -		      int clock, bool respect_dvi_limit)
> +		      int clock, bool respect_downstream_limits)
>  {
>  	struct drm_device *dev = intel_hdmi_to_dev(hdmi);
>  
>  	if (clock < 25000)
>  		return MODE_CLOCK_LOW;
> -	if (clock > hdmi_port_clock_limit(hdmi, respect_dvi_limit))
> +	if (clock > hdmi_port_clock_limit(hdmi, respect_downstream_limits))
>  		return MODE_CLOCK_HIGH;
>  
>  	/* BXT DPLL can't generate 223-240 MHz */
> @@ -1312,7 +1328,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  	 * within limits.
>  	 */
>  	if (pipe_config->pipe_bpp > 8*3 && pipe_config->has_hdmi_sink &&
> -	    hdmi_port_clock_valid(intel_hdmi, clock_12bpc, false) == MODE_OK &&
> +	    hdmi_port_clock_valid(intel_hdmi, clock_12bpc, true) == MODE_OK &&
>  	    hdmi_12bpc_possible(pipe_config)) {
>  		DRM_DEBUG_KMS("picking bpc to 12 for HDMI output\n");
>  		desired_bpp = 12*3;
> @@ -1352,10 +1368,41 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
>  	intel_hdmi->has_audio = false;
>  	intel_hdmi->rgb_quant_range_selectable = false;
>  
> +	intel_hdmi->dp_dual_mode.max_tmds_clock = 0;
> +
>  	kfree(to_intel_connector(connector)->detect_edid);
>  	to_intel_connector(connector)->detect_edid = NULL;
>  }
>  
> +static void
> +intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(connector->dev);
> +	struct intel_hdmi *hdmi = intel_attached_hdmi(connector);
> +	struct i2c_adapter *adapter =
> +		intel_gmbus_get_adapter(dev_priv, hdmi->ddc_bus);
> +	enum drm_dp_dual_mode_type type = drm_dp_dual_mode_detect(adapter);
> +
> +	if (type == DRM_DP_DUAL_MODE_NONE)
> +		return;
> +
> +	/*
> +	 * Type 1 adaptors can be broken unfortunately, so let's
> +	 * not trust anything they say beyond the type identification.
> +	 */
> +	if (type == DRM_DP_DUAL_MODE_TYPE2_DVI ||
> +	    type == DRM_DP_DUAL_MODE_TYPE2_HDMI) {
> +		hdmi->dp_dual_mode.max_tmds_clock =
> +			drm_dp_dual_mode_max_tmds_clock(adapter);
> +	} else {
> +		hdmi->dp_dual_mode.max_tmds_clock = 165000;
> +	}
> +
> +	DRM_DEBUG_KMS("DP dual mode adaptor (%s) detected (max TMDS clock: %d kHz)\n",
> +		      drm_dp_get_dual_mode_type_name(type),
> +		      hdmi->dp_dual_mode.max_tmds_clock);
> +}
> +
>  static bool
>  intel_hdmi_set_edid(struct drm_connector *connector, bool force)
>  {
> @@ -1371,6 +1418,8 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force)
>  				    intel_gmbus_get_adapter(dev_priv,
>  				    intel_hdmi->ddc_bus));
>  
> +		intel_hdmi_dp_dual_mode_detect(connector);
> +
>  		intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
>  	}
>  
> -- 
> 2.4.10
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/4] drm/i915: Respect DP++ adaptor TMDS clock limit

[Daniel Vetter]

On Tue, Feb 23, 2016 at 06:46:26PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Try to detect the max TMDS clock limit for the DP++ adaptor (if any)
> and take it into account when checking the port clock.
> 
> Note that as with the sink (HDMI vs. DVI) TMDS clock limit we'll ignore
> the adaptor TMDS clock limit in the modeset path, in case users are
> already "overclocking" their TMDS links. One subtle change here is that
> we'll have to respect the adaptor TMDS clock limit when we decide whether
> to do 12bpc or 8bpc, otherwise we might end up picking 12bpc and
> accidentally driving the TMDS link out of spec even when the user chose
> a mode that fits wihting the limits at 8bpc. This means you can't
> "overclock" your DP++ dongle at 12bpc anymore, but you can continue to
> do so at 8bpc.
> 
> Note that for simplicity we'll use the I2C access method for all dual
> mode adaptors including type 2. Otherwise we'd have to start mixing
> DP AUX and HDMI together. In the future we may need to do that if we
> come across any board designs that don't hook up the DDC pins to the
> DP++ connectors. Such boards would obviously only work with type 2
> dual mode adaptors, and not type 1.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

And on this patch and patch 1:

Cc: drm-intel-fixes@lists.freedesktop.org

I'll review patch 1 too if someone pities me with the spec ;-)

Thanks, Daniel

> ---
>  drivers/gpu/drm/i915/intel_drv.h  |  3 ++
>  drivers/gpu/drm/i915/intel_hdmi.c | 65 ++++++++++++++++++++++++++++++++++-----
>  2 files changed, 60 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4852049c9ab3..944eacbfb6a9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -704,6 +704,9 @@ struct cxsr_latency {
>  struct intel_hdmi {
>  	i915_reg_t hdmi_reg;
>  	int ddc_bus;
> +	struct {
> +		int max_tmds_clock;
> +	} dp_dual_mode;
>  	bool limited_color_range;
>  	bool color_range_auto;
>  	bool has_hdmi_sink;
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 80b44c054087..1e8cfdb18dc7 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -34,6 +34,7 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_dp_dual_mode_helper.h>
>  #include "intel_drv.h"
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
> @@ -1168,27 +1169,42 @@ static void pch_post_disable_hdmi(struct intel_encoder *encoder)
>  	intel_disable_hdmi(encoder);
>  }
>  
> -static int hdmi_port_clock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
> +static int intel_hdmi_source_max_tmds_clock(struct drm_i915_private *dev_priv)
>  {
> -	struct drm_device *dev = intel_hdmi_to_dev(hdmi);
> -
> -	if ((respect_dvi_limit && !hdmi->has_hdmi_sink) || IS_G4X(dev))
> +	if (IS_G4X(dev_priv))
>  		return 165000;
> -	else if (IS_HASWELL(dev) || INTEL_INFO(dev)->gen >= 8)
> +	else if (IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8)
>  		return 300000;
>  	else
>  		return 225000;
>  }
>  
> +static int hdmi_port_clock_limit(struct intel_hdmi *hdmi,
> +				 bool respect_downstream_limits)
> +{
> +	struct drm_device *dev = intel_hdmi_to_dev(hdmi);
> +	int max_tmds_clock = intel_hdmi_source_max_tmds_clock(to_i915(dev));
> +
> +	if (respect_downstream_limits) {
> +		if (hdmi->dp_dual_mode.max_tmds_clock)
> +			max_tmds_clock = min(max_tmds_clock,
> +					     hdmi->dp_dual_mode.max_tmds_clock);
> +		if (!hdmi->has_hdmi_sink)
> +			max_tmds_clock = min(max_tmds_clock, 165000);
> +	}
> +
> +	return max_tmds_clock;
> +}
> +
>  static enum drm_mode_status
>  hdmi_port_clock_valid(struct intel_hdmi *hdmi,
> -		      int clock, bool respect_dvi_limit)
> +		      int clock, bool respect_downstream_limits)
>  {
>  	struct drm_device *dev = intel_hdmi_to_dev(hdmi);
>  
>  	if (clock < 25000)
>  		return MODE_CLOCK_LOW;
> -	if (clock > hdmi_port_clock_limit(hdmi, respect_dvi_limit))
> +	if (clock > hdmi_port_clock_limit(hdmi, respect_downstream_limits))
>  		return MODE_CLOCK_HIGH;
>  
>  	/* BXT DPLL can't generate 223-240 MHz */
> @@ -1312,7 +1328,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  	 * within limits.
>  	 */
>  	if (pipe_config->pipe_bpp > 8*3 && pipe_config->has_hdmi_sink &&
> -	    hdmi_port_clock_valid(intel_hdmi, clock_12bpc, false) == MODE_OK &&
> +	    hdmi_port_clock_valid(intel_hdmi, clock_12bpc, true) == MODE_OK &&
>  	    hdmi_12bpc_possible(pipe_config)) {
>  		DRM_DEBUG_KMS("picking bpc to 12 for HDMI output\n");
>  		desired_bpp = 12*3;
> @@ -1352,10 +1368,41 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
>  	intel_hdmi->has_audio = false;
>  	intel_hdmi->rgb_quant_range_selectable = false;
>  
> +	intel_hdmi->dp_dual_mode.max_tmds_clock = 0;
> +
>  	kfree(to_intel_connector(connector)->detect_edid);
>  	to_intel_connector(connector)->detect_edid = NULL;
>  }
>  
> +static void
> +intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(connector->dev);
> +	struct intel_hdmi *hdmi = intel_attached_hdmi(connector);
> +	struct i2c_adapter *adapter =
> +		intel_gmbus_get_adapter(dev_priv, hdmi->ddc_bus);
> +	enum drm_dp_dual_mode_type type = drm_dp_dual_mode_detect(adapter);
> +
> +	if (type == DRM_DP_DUAL_MODE_NONE)
> +		return;
> +
> +	/*
> +	 * Type 1 adaptors can be broken unfortunately, so let's
> +	 * not trust anything they say beyond the type identification.
> +	 */
> +	if (type == DRM_DP_DUAL_MODE_TYPE2_DVI ||
> +	    type == DRM_DP_DUAL_MODE_TYPE2_HDMI) {
> +		hdmi->dp_dual_mode.max_tmds_clock =
> +			drm_dp_dual_mode_max_tmds_clock(adapter);
> +	} else {
> +		hdmi->dp_dual_mode.max_tmds_clock = 165000;
> +	}
> +
> +	DRM_DEBUG_KMS("DP dual mode adaptor (%s) detected (max TMDS clock: %d kHz)\n",
> +		      drm_dp_get_dual_mode_type_name(type),
> +		      hdmi->dp_dual_mode.max_tmds_clock);
> +}
> +
>  static bool
>  intel_hdmi_set_edid(struct drm_connector *connector, bool force)
>  {
> @@ -1371,6 +1418,8 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force)
>  				    intel_gmbus_get_adapter(dev_priv,
>  				    intel_hdmi->ddc_bus));
>  
> +		intel_hdmi_dp_dual_mode_detect(connector);
> +
>  		intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
>  	}
>  
> -- 
> 2.4.10
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[PATCH 3/4] drm/i915: Enable/disable TMDS output buffers in DP++ adaptor as needed

[ville.syrjala]

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

To save a bit of power, let's try to turn off the TMDS output buffers
in DP++ adaptors when we're not driving the port.

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

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 944eacbfb6a9..3ca29a181e64 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -706,6 +706,7 @@ struct intel_hdmi {
 	int ddc_bus;
 	struct {
 		int max_tmds_clock;
+		bool tmds_output_control;
 	} dp_dual_mode;
 	bool limited_color_range;
 	bool color_range_auto;
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 1e8cfdb18dc7..660a65f48fd8 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -846,6 +846,13 @@ static void intel_hdmi_prepare(struct intel_encoder *encoder)
 	const struct drm_display_mode *adjusted_mode = &crtc->config->base.adjusted_mode;
 	u32 hdmi_val;
 
+	if (intel_hdmi->dp_dual_mode.tmds_output_control) {
+		struct i2c_adapter *adapter =
+			intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus);
+
+		drm_dp_dual_mode_set_tmds_output(adapter, true);
+	}
+
 	hdmi_val = SDVO_ENCODING_HDMI;
 	if (!HAS_PCH_SPLIT(dev) && crtc->config->limited_color_range)
 		hdmi_val |= HDMI_COLOR_RANGE_16_235;
@@ -1144,6 +1151,13 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
 	}
 
 	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
+
+	if (intel_hdmi->dp_dual_mode.tmds_output_control) {
+		struct i2c_adapter *adapter =
+			intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus);
+
+		drm_dp_dual_mode_set_tmds_output(adapter, false);
+	}
 }
 
 static void g4x_disable_hdmi(struct intel_encoder *encoder)
@@ -1369,6 +1383,7 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
 	intel_hdmi->rgb_quant_range_selectable = false;
 
 	intel_hdmi->dp_dual_mode.max_tmds_clock = 0;
+	intel_hdmi->dp_dual_mode.tmds_output_control = false;
 
 	kfree(to_intel_connector(connector)->detect_edid);
 	to_intel_connector(connector)->detect_edid = NULL;
@@ -1392,15 +1407,23 @@ intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector)
 	 */
 	if (type == DRM_DP_DUAL_MODE_TYPE2_DVI ||
 	    type == DRM_DP_DUAL_MODE_TYPE2_HDMI) {
+		bool tmds_enabled;
+
 		hdmi->dp_dual_mode.max_tmds_clock =
 			drm_dp_dual_mode_max_tmds_clock(adapter);
+
+		hdmi->dp_dual_mode.tmds_output_control =
+			drm_dp_dual_mode_get_tmds_output(adapter, &tmds_enabled) == 0 &&
+			drm_dp_dual_mode_set_tmds_output(adapter, tmds_enabled) == 0;
 	} else {
 		hdmi->dp_dual_mode.max_tmds_clock = 165000;
+		hdmi->dp_dual_mode.tmds_output_control = false;
 	}
 
-	DRM_DEBUG_KMS("DP dual mode adaptor (%s) detected (max TMDS clock: %d kHz)\n",
+	DRM_DEBUG_KMS("DP dual mode adaptor (%s) detected (max TMDS clock: %d kHz, TMDS OE# control: %s)\n",
 		      drm_dp_get_dual_mode_type_name(type),
-		      hdmi->dp_dual_mode.max_tmds_clock);
+		      hdmi->dp_dual_mode.max_tmds_clock,
+		      yesno(hdmi->dp_dual_mode.tmds_output_control));
 }
 
 static bool
-- 
2.4.10

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

[PATCH v2 3/4] drm/i915: Enable/disable TMDS output buffers in DP++ adaptor as needed

[ville.syrjala]

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

To save a bit of power, let's try to turn off the TMDS output buffers
in DP++ adaptors when we're not driving the port.

v2: Let's not forget DDI, toss in a debug message while at it

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c  | 12 ++++++++++++
 drivers/gpu/drm/i915/intel_drv.h  |  2 ++
 drivers/gpu/drm/i915/intel_hdmi.c | 31 +++++++++++++++++++++++++++++--
 3 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 21a9b83f3bfc..458c41788b80 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2301,6 +2301,12 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
 	enum port port = intel_ddi_get_encoder_port(intel_encoder);
 	int type = intel_encoder->type;
 
+	if (type == INTEL_OUTPUT_HDMI) {
+		struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
+
+		intel_dp_dual_mode_set_tmds_output(intel_hdmi, true);
+	}
+
 	intel_prepare_ddi_buffer(intel_encoder);
 
 	if (type == INTEL_OUTPUT_EDP) {
@@ -2367,6 +2373,12 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
 					DPLL_CTRL2_DDI_CLK_OFF(port)));
 	else if (INTEL_INFO(dev)->gen < 9)
 		I915_WRITE(PORT_CLK_SEL(port), PORT_CLK_SEL_NONE);
+
+	if (type == INTEL_OUTPUT_HDMI) {
+		struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
+
+		intel_dp_dual_mode_set_tmds_output(intel_hdmi, false);
+	}
 }
 
 static void intel_enable_ddi(struct intel_encoder *intel_encoder)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 944eacbfb6a9..2e4fa4337c59 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -706,6 +706,7 @@ struct intel_hdmi {
 	int ddc_bus;
 	struct {
 		int max_tmds_clock;
+		bool tmds_output_control;
 	} dp_dual_mode;
 	bool limited_color_range;
 	bool color_range_auto;
@@ -1355,6 +1356,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);
 bool intel_hdmi_compute_config(struct intel_encoder *encoder,
 			       struct intel_crtc_state *pipe_config);
+void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
 
 
 /* intel_lvds.c */
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 1e8cfdb18dc7..4b528a669f8e 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -837,6 +837,20 @@ static void hsw_set_infoframes(struct drm_encoder *encoder,
 	intel_hdmi_set_hdmi_infoframe(encoder, adjusted_mode);
 }
 
+void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable)
+{
+	if (hdmi->dp_dual_mode.tmds_output_control) {
+		struct drm_i915_private *dev_priv = to_i915(intel_hdmi_to_dev(hdmi));
+		struct i2c_adapter *adapter =
+			intel_gmbus_get_adapter(dev_priv, hdmi->ddc_bus);
+
+		DRM_DEBUG_KMS("%s DP dual mode adaptor TMDS output\n",
+			      enable ? "Enabling" : "Disabling");
+
+		drm_dp_dual_mode_set_tmds_output(adapter, enable);
+	}
+}
+
 static void intel_hdmi_prepare(struct intel_encoder *encoder)
 {
 	struct drm_device *dev = encoder->base.dev;
@@ -846,6 +860,8 @@ static void intel_hdmi_prepare(struct intel_encoder *encoder)
 	const struct drm_display_mode *adjusted_mode = &crtc->config->base.adjusted_mode;
 	u32 hdmi_val;
 
+	intel_dp_dual_mode_set_tmds_output(intel_hdmi, true);
+
 	hdmi_val = SDVO_ENCODING_HDMI;
 	if (!HAS_PCH_SPLIT(dev) && crtc->config->limited_color_range)
 		hdmi_val |= HDMI_COLOR_RANGE_16_235;
@@ -1144,6 +1160,8 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
 	}
 
 	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
+
+	intel_dp_dual_mode_set_tmds_output(intel_hdmi, false);
 }
 
 static void g4x_disable_hdmi(struct intel_encoder *encoder)
@@ -1369,6 +1387,7 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
 	intel_hdmi->rgb_quant_range_selectable = false;
 
 	intel_hdmi->dp_dual_mode.max_tmds_clock = 0;
+	intel_hdmi->dp_dual_mode.tmds_output_control = false;
 
 	kfree(to_intel_connector(connector)->detect_edid);
 	to_intel_connector(connector)->detect_edid = NULL;
@@ -1392,15 +1411,23 @@ intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector)
 	 */
 	if (type == DRM_DP_DUAL_MODE_TYPE2_DVI ||
 	    type == DRM_DP_DUAL_MODE_TYPE2_HDMI) {
+		bool tmds_enabled;
+
 		hdmi->dp_dual_mode.max_tmds_clock =
 			drm_dp_dual_mode_max_tmds_clock(adapter);
+
+		hdmi->dp_dual_mode.tmds_output_control =
+			drm_dp_dual_mode_get_tmds_output(adapter, &tmds_enabled) == 0 &&
+			drm_dp_dual_mode_set_tmds_output(adapter, tmds_enabled) == 0;
 	} else {
 		hdmi->dp_dual_mode.max_tmds_clock = 165000;
+		hdmi->dp_dual_mode.tmds_output_control = false;
 	}
 
-	DRM_DEBUG_KMS("DP dual mode adaptor (%s) detected (max TMDS clock: %d kHz)\n",
+	DRM_DEBUG_KMS("DP dual mode adaptor (%s) detected (max TMDS clock: %d kHz, TMDS OE# control: %s)\n",
 		      drm_dp_get_dual_mode_type_name(type),
-		      hdmi->dp_dual_mode.max_tmds_clock);
+		      hdmi->dp_dual_mode.max_tmds_clock,
+		      yesno(hdmi->dp_dual_mode.tmds_output_control));
 }
 
 static bool
-- 
2.4.10

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/4] drm/i915: Enable/disable TMDS output buffers in DP++ adaptor as needed

[Zanoni, Paulo R]

Em Qui, 2016-02-25 às 14:51 +0200, ville.syrjala@linux.intel.com
escreveu:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> To save a bit of power, let's try to turn off the TMDS output buffers
> in DP++ adaptors when we're not driving the port.
> 
> v2: Let's not forget DDI, toss in a debug message while at it
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c  | 12 ++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h  |  2 ++
>  drivers/gpu/drm/i915/intel_hdmi.c | 31
> +++++++++++++++++++++++++++++--
>  3 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 21a9b83f3bfc..458c41788b80 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2301,6 +2301,12 @@ static void intel_ddi_pre_enable(struct
> intel_encoder *intel_encoder)
>  	enum port port = intel_ddi_get_encoder_port(intel_encoder);
>  	int type = intel_encoder->type;
>  
> +	if (type == INTEL_OUTPUT_HDMI) {
> +		struct intel_hdmi *intel_hdmi =
> enc_to_intel_hdmi(encoder);
> +
> +		intel_dp_dual_mode_set_tmds_output(intel_hdmi,
> true);
> +	}
> +
>  	intel_prepare_ddi_buffer(intel_encoder);
>  
>  	if (type == INTEL_OUTPUT_EDP) {
> @@ -2367,6 +2373,12 @@ static void intel_ddi_post_disable(struct
> intel_encoder *intel_encoder)
>  					DPLL_CTRL2_DDI_CLK_OFF(port)
> ));
>  	else if (INTEL_INFO(dev)->gen < 9)
>  		I915_WRITE(PORT_CLK_SEL(port), PORT_CLK_SEL_NONE);
> +
> +	if (type == INTEL_OUTPUT_HDMI) {
> +		struct intel_hdmi *intel_hdmi =
> enc_to_intel_hdmi(encoder);
> +
> +		intel_dp_dual_mode_set_tmds_output(intel_hdmi,
> false);
> +	}
>  }
>  
>  static void intel_enable_ddi(struct intel_encoder *intel_encoder)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 944eacbfb6a9..2e4fa4337c59 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -706,6 +706,7 @@ struct intel_hdmi {
>  	int ddc_bus;
>  	struct {
>  		int max_tmds_clock;
> +		bool tmds_output_control;
>  	} dp_dual_mode;
>  	bool limited_color_range;
>  	bool color_range_auto;
> @@ -1355,6 +1356,7 @@ void intel_hdmi_init_connector(struct
> intel_digital_port *intel_dig_port,
>  struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);
>  bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  			       struct intel_crtc_state
> *pipe_config);
> +void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi,
> bool enable);
>  
>  
>  /* intel_lvds.c */
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> b/drivers/gpu/drm/i915/intel_hdmi.c
> index 1e8cfdb18dc7..4b528a669f8e 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -837,6 +837,20 @@ static void hsw_set_infoframes(struct
> drm_encoder *encoder,
>  	intel_hdmi_set_hdmi_infoframe(encoder, adjusted_mode);
>  }
>  
> +void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi,
> bool enable)
> +{
> +	if (hdmi->dp_dual_mode.tmds_output_control) {

Or you could just do an early return here and save an indentation level
:)

> +		struct drm_i915_private *dev_priv =
> to_i915(intel_hdmi_to_dev(hdmi));
> +		struct i2c_adapter *adapter =
> +			intel_gmbus_get_adapter(dev_priv, hdmi-
> >ddc_bus);
> +
> +		DRM_DEBUG_KMS("%s DP dual mode adaptor TMDS
> output\n",
> +			      enable ? "Enabling" : "Disabling");
> +
> +		drm_dp_dual_mode_set_tmds_output(adapter, enable);
> +	}
> +}
> +
>  static void intel_hdmi_prepare(struct intel_encoder *encoder)
>  {
>  	struct drm_device *dev = encoder->base.dev;
> @@ -846,6 +860,8 @@ static void intel_hdmi_prepare(struct
> intel_encoder *encoder)
>  	const struct drm_display_mode *adjusted_mode = &crtc-
> >config->base.adjusted_mode;
>  	u32 hdmi_val;
>  
> +	intel_dp_dual_mode_set_tmds_output(intel_hdmi, true);
> +
>  	hdmi_val = SDVO_ENCODING_HDMI;
>  	if (!HAS_PCH_SPLIT(dev) && crtc->config-
> >limited_color_range)
>  		hdmi_val |= HDMI_COLOR_RANGE_16_235;
> @@ -1144,6 +1160,8 @@ static void intel_disable_hdmi(struct
> intel_encoder *encoder)
>  	}
>  
>  	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
> +
> +	intel_dp_dual_mode_set_tmds_output(intel_hdmi, false);
>  }
>  
>  static void g4x_disable_hdmi(struct intel_encoder *encoder)
> @@ -1369,6 +1387,7 @@ intel_hdmi_unset_edid(struct drm_connector
> *connector)
>  	intel_hdmi->rgb_quant_range_selectable = false;
>  
>  	intel_hdmi->dp_dual_mode.max_tmds_clock = 0;
> +	intel_hdmi->dp_dual_mode.tmds_output_control = false;
>  
>  	kfree(to_intel_connector(connector)->detect_edid);
>  	to_intel_connector(connector)->detect_edid = NULL;
> @@ -1392,15 +1411,23 @@ intel_hdmi_dp_dual_mode_detect(struct
> drm_connector *connector)
>  	 */
>  	if (type == DRM_DP_DUAL_MODE_TYPE2_DVI ||
>  	    type == DRM_DP_DUAL_MODE_TYPE2_HDMI) {
> +		bool tmds_enabled;
> +
>  		hdmi->dp_dual_mode.max_tmds_clock =
>  			drm_dp_dual_mode_max_tmds_clock(adapter);
> +
> +		hdmi->dp_dual_mode.tmds_output_control =
> +			drm_dp_dual_mode_get_tmds_output(adapter,
> &tmds_enabled) == 0 &&
> +			drm_dp_dual_mode_set_tmds_output(adapter,
> tmds_enabled) == 0;
>  	} else {
>  		hdmi->dp_dual_mode.max_tmds_clock = 165000;
> +		hdmi->dp_dual_mode.tmds_output_control = false;

While type 1 adaptors are not required to implement the register, what
if they do? We could potentially keep TMDS disabled forever. Maybe my
suggestions on p1 would help a little bit here?

Anyway, what we have here is already better than the previous state, so
if no rebase is required:

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>  	}
>  
> -	DRM_DEBUG_KMS("DP dual mode adaptor (%s) detected (max TMDS
> clock: %d kHz)\n",
> +	DRM_DEBUG_KMS("DP dual mode adaptor (%s) detected (max TMDS
> clock: %d kHz, TMDS OE# control: %s)\n",
>  		      drm_dp_get_dual_mode_type_name(type),
> -		      hdmi->dp_dual_mode.max_tmds_clock);
> +		      hdmi->dp_dual_mode.max_tmds_clock,
> +		      yesno(hdmi-
> >dp_dual_mode.tmds_output_control));
>  }
>  
>  static bool
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/4] drm/i915: Enable/disable TMDS output buffers in DP++ adaptor as needed

[Ville Syrjälä]

On Thu, Mar 31, 2016 at 07:26:25PM +0000, Zanoni, Paulo R wrote:
> Em Qui, 2016-02-25 às 14:51 +0200, ville.syrjala@linux.intel.com
> escreveu:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > To save a bit of power, let's try to turn off the TMDS output buffers
> > in DP++ adaptors when we're not driving the port.
> > 
> > v2: Let's not forget DDI, toss in a debug message while at it
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c  | 12 ++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h  |  2 ++
> >  drivers/gpu/drm/i915/intel_hdmi.c | 31
> > +++++++++++++++++++++++++++++--
> >  3 files changed, 43 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index 21a9b83f3bfc..458c41788b80 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -2301,6 +2301,12 @@ static void intel_ddi_pre_enable(struct
> > intel_encoder *intel_encoder)
> >  	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> >  	int type = intel_encoder->type;
> >  
> > +	if (type == INTEL_OUTPUT_HDMI) {
> > +		struct intel_hdmi *intel_hdmi =
> > enc_to_intel_hdmi(encoder);
> > +
> > +		intel_dp_dual_mode_set_tmds_output(intel_hdmi,
> > true);
> > +	}
> > +
> >  	intel_prepare_ddi_buffer(intel_encoder);
> >  
> >  	if (type == INTEL_OUTPUT_EDP) {
> > @@ -2367,6 +2373,12 @@ static void intel_ddi_post_disable(struct
> > intel_encoder *intel_encoder)
> >  					DPLL_CTRL2_DDI_CLK_OFF(port)
> > ));
> >  	else if (INTEL_INFO(dev)->gen < 9)
> >  		I915_WRITE(PORT_CLK_SEL(port), PORT_CLK_SEL_NONE);
> > +
> > +	if (type == INTEL_OUTPUT_HDMI) {
> > +		struct intel_hdmi *intel_hdmi =
> > enc_to_intel_hdmi(encoder);
> > +
> > +		intel_dp_dual_mode_set_tmds_output(intel_hdmi,
> > false);
> > +	}
> >  }
> >  
> >  static void intel_enable_ddi(struct intel_encoder *intel_encoder)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 944eacbfb6a9..2e4fa4337c59 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -706,6 +706,7 @@ struct intel_hdmi {
> >  	int ddc_bus;
> >  	struct {
> >  		int max_tmds_clock;
> > +		bool tmds_output_control;
> >  	} dp_dual_mode;
> >  	bool limited_color_range;
> >  	bool color_range_auto;
> > @@ -1355,6 +1356,7 @@ void intel_hdmi_init_connector(struct
> > intel_digital_port *intel_dig_port,
> >  struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);
> >  bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> >  			       struct intel_crtc_state
> > *pipe_config);
> > +void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi,
> > bool enable);
> >  
> >  
> >  /* intel_lvds.c */
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 1e8cfdb18dc7..4b528a669f8e 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -837,6 +837,20 @@ static void hsw_set_infoframes(struct
> > drm_encoder *encoder,
> >  	intel_hdmi_set_hdmi_infoframe(encoder, adjusted_mode);
> >  }
> >  
> > +void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi,
> > bool enable)
> > +{
> > +	if (hdmi->dp_dual_mode.tmds_output_control) {
> 
> Or you could just do an early return here and save an indentation level
> :)

Indeed. I had the code inlined originally so the extra indentation
made sense, and the I just copypasted it to this separate function,
and in my haste forgot to flatten it.

> 
> > +		struct drm_i915_private *dev_priv =
> > to_i915(intel_hdmi_to_dev(hdmi));
> > +		struct i2c_adapter *adapter =
> > +			intel_gmbus_get_adapter(dev_priv, hdmi-
> > >ddc_bus);
> > +
> > +		DRM_DEBUG_KMS("%s DP dual mode adaptor TMDS
> > output\n",
> > +			      enable ? "Enabling" : "Disabling");
> > +
> > +		drm_dp_dual_mode_set_tmds_output(adapter, enable);
> > +	}
> > +}
> > +
> >  static void intel_hdmi_prepare(struct intel_encoder *encoder)
> >  {
> >  	struct drm_device *dev = encoder->base.dev;
> > @@ -846,6 +860,8 @@ static void intel_hdmi_prepare(struct
> > intel_encoder *encoder)
> >  	const struct drm_display_mode *adjusted_mode = &crtc-
> > >config->base.adjusted_mode;
> >  	u32 hdmi_val;
> >  
> > +	intel_dp_dual_mode_set_tmds_output(intel_hdmi, true);
> > +
> >  	hdmi_val = SDVO_ENCODING_HDMI;
> >  	if (!HAS_PCH_SPLIT(dev) && crtc->config-
> > >limited_color_range)
> >  		hdmi_val |= HDMI_COLOR_RANGE_16_235;
> > @@ -1144,6 +1160,8 @@ static void intel_disable_hdmi(struct
> > intel_encoder *encoder)
> >  	}
> >  
> >  	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
> > +
> > +	intel_dp_dual_mode_set_tmds_output(intel_hdmi, false);
> >  }
> >  
> >  static void g4x_disable_hdmi(struct intel_encoder *encoder)
> > @@ -1369,6 +1387,7 @@ intel_hdmi_unset_edid(struct drm_connector
> > *connector)
> >  	intel_hdmi->rgb_quant_range_selectable = false;
> >  
> >  	intel_hdmi->dp_dual_mode.max_tmds_clock = 0;
> > +	intel_hdmi->dp_dual_mode.tmds_output_control = false;
> >  
> >  	kfree(to_intel_connector(connector)->detect_edid);
> >  	to_intel_connector(connector)->detect_edid = NULL;
> > @@ -1392,15 +1411,23 @@ intel_hdmi_dp_dual_mode_detect(struct
> > drm_connector *connector)
> >  	 */
> >  	if (type == DRM_DP_DUAL_MODE_TYPE2_DVI ||
> >  	    type == DRM_DP_DUAL_MODE_TYPE2_HDMI) {
> > +		bool tmds_enabled;
> > +
> >  		hdmi->dp_dual_mode.max_tmds_clock =
> >  			drm_dp_dual_mode_max_tmds_clock(adapter);
> > +
> > +		hdmi->dp_dual_mode.tmds_output_control =
> > +			drm_dp_dual_mode_get_tmds_output(adapter,
> > &tmds_enabled) == 0 &&
> > +			drm_dp_dual_mode_set_tmds_output(adapter,
> > tmds_enabled) == 0;
> >  	} else {
> >  		hdmi->dp_dual_mode.max_tmds_clock = 165000;
> > +		hdmi->dp_dual_mode.tmds_output_control = false;
> 
> While type 1 adaptors are not required to implement the register, what
> if they do?

I suppose we could just always do the i2c write and if the adaptor
supports it everything is peachy, and if it doesn't, well, then I
suppose the write just goes to /dev/null and nothing should happen.

> We could potentially keep TMDS disabled forever.

That would be an entirely broken adaptor since it would not work at all
with source devices that are not dual mode aware.

> Maybe my
> suggestions on p1 would help a little bit here?
> 
> Anyway, what we have here is already better than the previous state, so
> if no rebase is required:
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> >  	}
> >  
> > -	DRM_DEBUG_KMS("DP dual mode adaptor (%s) detected (max TMDS
> > clock: %d kHz)\n",
> > +	DRM_DEBUG_KMS("DP dual mode adaptor (%s) detected (max TMDS
> > clock: %d kHz, TMDS OE# control: %s)\n",
> >  		      drm_dp_get_dual_mode_type_name(type),
> > -		      hdmi->dp_dual_mode.max_tmds_clock);
> > +		      hdmi->dp_dual_mode.max_tmds_clock,
> > +		      yesno(hdmi-
> > >dp_dual_mode.tmds_output_control));
> >  }
> >  
> >  static bool

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

[PATCH 4/4] drm/i915: Determine DP++ type 1 DVI adaptor presence based on VBT

[ville.syrjala]

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

DP dual mode type 1 DVI adaptors aren't required to implement any
registers, so it's a bit hard to detect them. The best way would
be to check the state of the CONFIG1 pin, but we have no way to
do that. So as a last resort, check the VBT to see if the HDMI
port is in fact a dual mode capable DP port.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_bios.h |  3 +++
 drivers/gpu/drm/i915/intel_dp.c   | 28 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_hdmi.c | 23 +++++++++++++++++++++--
 4 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index 350d4e0f75a4..50d1659efe47 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -730,6 +730,7 @@ struct bdb_psr {
 #define	 DEVICE_TYPE_INT_TV	0x1009
 #define	 DEVICE_TYPE_HDMI	0x60D2
 #define	 DEVICE_TYPE_DP		0x68C6
+#define	 DEVICE_TYPE_DP_DUAL_MODE	0x60D6
 #define	 DEVICE_TYPE_eDP	0x78C6
 
 #define  DEVICE_TYPE_CLASS_EXTENSION	(1 << 15)
@@ -764,6 +765,8 @@ struct bdb_psr {
 	 DEVICE_TYPE_DISPLAYPORT_OUTPUT | \
 	 DEVICE_TYPE_ANALOG_OUTPUT)
 
+#define DEVICE_TYPE_DP_DUAL_MODE_BITS ~DEVICE_TYPE_NOT_HDMI_OUTPUT
+
 /* define the DVO port for HDMI output type */
 #define		DVO_B		1
 #define		DVO_C		2
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index cbc06596659a..f3edacf517ac 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5104,6 +5104,34 @@ bool intel_dp_is_edp(struct drm_device *dev, enum port port)
 	return false;
 }
 
+bool intel_dp_is_dual_mode(struct drm_i915_private *dev_priv, enum port port)
+{
+	const union child_device_config *p_child;
+	int i;
+	static const short port_mapping[] = {
+		[PORT_B] = DVO_PORT_DPB,
+		[PORT_C] = DVO_PORT_DPC,
+		[PORT_D] = DVO_PORT_DPD,
+		[PORT_E] = DVO_PORT_DPE,
+	};
+
+	if (port == PORT_A || port >= ARRAY_SIZE(port_mapping))
+		return false;
+
+	if (!dev_priv->vbt.child_dev_num)
+		return false;
+
+	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
+		p_child = &dev_priv->vbt.child_dev[i];
+
+		if (p_child->common.dvo_port == port_mapping[port] &&
+		    (p_child->common.device_type & DEVICE_TYPE_DP_DUAL_MODE_BITS) ==
+		    (DEVICE_TYPE_DP_DUAL_MODE & DEVICE_TYPE_DP_DUAL_MODE_BITS))
+			return true;
+	}
+	return false;
+}
+
 void
 intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connector)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3ca29a181e64..c7d1ea4dbe42 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1248,6 +1248,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
 bool intel_dp_compute_config(struct intel_encoder *encoder,
 			     struct intel_crtc_state *pipe_config);
 bool intel_dp_is_edp(struct drm_device *dev, enum port port);
+bool intel_dp_is_dual_mode(struct drm_i915_private *dev_priv, enum port port);
 enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port,
 				  bool long_hpd);
 void intel_edp_backlight_on(struct intel_dp *intel_dp);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 660a65f48fd8..1476f3afb7e2 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1390,14 +1390,33 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
 }
 
 static void
-intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector)
+intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector, bool has_edid)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
 	struct intel_hdmi *hdmi = intel_attached_hdmi(connector);
+	enum port port = hdmi_to_dig_port(hdmi)->port;
 	struct i2c_adapter *adapter =
 		intel_gmbus_get_adapter(dev_priv, hdmi->ddc_bus);
 	enum drm_dp_dual_mode_type type = drm_dp_dual_mode_detect(adapter);
 
+	/*
+	 * Type 1 DVI adaptors are not required to implement any
+	 * registers, so we can't always detect their presence.
+	 * Ideally we should be able to check the state of the
+	 * CONFIG1 pin, but no such luck on our hardware.
+	 *
+	 * The only method left to us is to check the VBT to see
+	 * if the port is a dual mode capable DP port. But let's
+	 * only do that when we sucesfully read the EDID, to avoid
+	 * confusing log messages about DP dual mode adaptors when
+	 * there's nothing connected to the port.
+	 */
+	if (type == DRM_DP_DUAL_MODE_NONE && has_edid &&
+	    intel_dp_is_dual_mode(dev_priv, port)) {
+		DRM_DEBUG_KMS("Assuming DP dual mode adaptor presence based on VBT\n");
+		type = DRM_DP_DUAL_MODE_TYPE1_DVI;
+	}
+
 	if (type == DRM_DP_DUAL_MODE_NONE)
 		return;
 
@@ -1441,7 +1460,7 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force)
 				    intel_gmbus_get_adapter(dev_priv,
 				    intel_hdmi->ddc_bus));
 
-		intel_hdmi_dp_dual_mode_detect(connector);
+		intel_hdmi_dp_dual_mode_detect(connector, edid != NULL);
 
 		intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
 	}
-- 
2.4.10

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

Re: [PATCH 4/4] drm/i915: Determine DP++ type 1 DVI adaptor presence based on VBT

[Zanoni, Paulo R]

Em Ter, 2016-02-23 às 18:46 +0200, ville.syrjala@linux.intel.com
escreveu:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> DP dual mode type 1 DVI adaptors aren't required to implement any
> registers, so it's a bit hard to detect them. The best way would
> be to check the state of the CONFIG1 pin, but we have no way to
> do that. So as a last resort, check the VBT to see if the HDMI
> port is in fact a dual mode capable DP port.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_bios.h |  3 +++
>  drivers/gpu/drm/i915/intel_dp.c   | 28 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h  |  1 +
>  drivers/gpu/drm/i915/intel_hdmi.c | 23 +++++++++++++++++++++--
>  4 files changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.h
> b/drivers/gpu/drm/i915/intel_bios.h
> index 350d4e0f75a4..50d1659efe47 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -730,6 +730,7 @@ struct bdb_psr {
>  #define	 DEVICE_TYPE_INT_TV	0x1009
>  #define	 DEVICE_TYPE_HDMI	0x60D2
>  #define	 DEVICE_TYPE_DP		0x68C6
> +#define	 DEVICE_TYPE_DP_DUAL_MODE	0x60D6
>  #define	 DEVICE_TYPE_eDP	0x78C6
>  
>  #define  DEVICE_TYPE_CLASS_EXTENSION	(1 << 15)
> @@ -764,6 +765,8 @@ struct bdb_psr {
>  	 DEVICE_TYPE_DISPLAYPORT_OUTPUT | \
>  	 DEVICE_TYPE_ANALOG_OUTPUT)
>  
> +#define DEVICE_TYPE_DP_DUAL_MODE_BITS ~DEVICE_TYPE_NOT_HDMI_OUTPUT
> +
>  /* define the DVO port for HDMI output type */
>  #define		DVO_B		1
>  #define		DVO_C		2
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> index cbc06596659a..f3edacf517ac 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5104,6 +5104,34 @@ bool intel_dp_is_edp(struct drm_device *dev,
> enum port port)
>  	return false;
>  }
>  
> +bool intel_dp_is_dual_mode(struct drm_i915_private *dev_priv, enum
> port port)
> +{
> +	const union child_device_config *p_child;
> +	int i;
> +	static const short port_mapping[] = {
> +		[PORT_B] = DVO_PORT_DPB,
> +		[PORT_C] = DVO_PORT_DPC,
> +		[PORT_D] = DVO_PORT_DPD,
> +		[PORT_E] = DVO_PORT_DPE,
> +	};
> +
> +	if (port == PORT_A || port >= ARRAY_SIZE(port_mapping))
> +		return false;
> +
> +	if (!dev_priv->vbt.child_dev_num)
> +		return false;
> +
> +	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
> +		p_child = &dev_priv->vbt.child_dev[i];
> +
> +		if (p_child->common.dvo_port == port_mapping[port]
> &&
> +		    (p_child->common.device_type &
> DEVICE_TYPE_DP_DUAL_MODE_BITS) ==
> +		    (DEVICE_TYPE_DP_DUAL_MODE &
> DEVICE_TYPE_DP_DUAL_MODE_BITS))
> +			return true;
> +	}

Some thoughts:

This is going to be implemented for all VBT versions. Since there's no
real history about anything before version 155, is this really what we
want? A huge part of the "we don't trust the VBT" culture we have on
our team is because of those old versions being completely unreliable.
If this is the case, we could make this implementation just be a small
patch in parse_ddi_port(). I'm kinda afraid we may somehow break old
machines yet again.

- Instead of creating these complicated bit masks, why don't we just
specifically check "if bit 2 and bit 4 are enabled, we're using an
adaptor"? Much simpler IMHO.

- Jani's recent patch suggests you may want to move this function to
intel_bios.c in order to avoid including intel_vbt_defs.h from
intel_hdmi.c. Anyway, you'll have to rebase.

> +	return false;
> +}
> +
>  void
>  intel_dp_add_properties(struct intel_dp *intel_dp, struct
> drm_connector *connector)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 3ca29a181e64..c7d1ea4dbe42 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1248,6 +1248,7 @@ int intel_dp_sink_crc(struct intel_dp
> *intel_dp, u8 *crc);
>  bool intel_dp_compute_config(struct intel_encoder *encoder,
>  			     struct intel_crtc_state *pipe_config);
>  bool intel_dp_is_edp(struct drm_device *dev, enum port port);
> +bool intel_dp_is_dual_mode(struct drm_i915_private *dev_priv, enum
> port port);
>  enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port
> *intel_dig_port,
>  				  bool long_hpd);
>  void intel_edp_backlight_on(struct intel_dp *intel_dp);
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> b/drivers/gpu/drm/i915/intel_hdmi.c
> index 660a65f48fd8..1476f3afb7e2 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1390,14 +1390,33 @@ intel_hdmi_unset_edid(struct drm_connector
> *connector)
>  }
>  
>  static void
> -intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector)
> +intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector, bool
> has_edid)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector->dev);
>  	struct intel_hdmi *hdmi = intel_attached_hdmi(connector);
> +	enum port port = hdmi_to_dig_port(hdmi)->port;
>  	struct i2c_adapter *adapter =
>  		intel_gmbus_get_adapter(dev_priv, hdmi->ddc_bus);
>  	enum drm_dp_dual_mode_type type =
> drm_dp_dual_mode_detect(adapter);
>  
> +	/*
> +	 * Type 1 DVI adaptors are not required to implement any
> +	 * registers, so we can't always detect their presence.
> +	 * Ideally we should be able to check the state of the
> +	 * CONFIG1 pin, but no such luck on our hardware.
> +	 *
> +	 * The only method left to us is to check the VBT to see
> +	 * if the port is a dual mode capable DP port. But let's
> +	 * only do that when we sucesfully read the EDID, to avoid
> +	 * confusing log messages about DP dual mode adaptors when
> +	 * there's nothing connected to the port.
> +	 */
> +	if (type == DRM_DP_DUAL_MODE_NONE && has_edid &&
> +	    intel_dp_is_dual_mode(dev_priv, port)) {
> +		DRM_DEBUG_KMS("Assuming DP dual mode adaptor
> presence based on VBT\n");
> +		type = DRM_DP_DUAL_MODE_TYPE1_DVI;
> +	}
> +
>  	if (type == DRM_DP_DUAL_MODE_NONE)
>  		return;
>  
> @@ -1441,7 +1460,7 @@ intel_hdmi_set_edid(struct drm_connector
> *connector, bool force)
>  				    intel_gmbus_get_adapter(dev_priv
> ,
>  				    intel_hdmi->ddc_bus));
>  
> -		intel_hdmi_dp_dual_mode_detect(connector);
> +		intel_hdmi_dp_dual_mode_detect(connector, edid !=
> NULL);
>  
>  		intel_display_power_put(dev_priv,
> POWER_DOMAIN_GMBUS);
>  	}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Determine DP++ type 1 DVI adaptor presence based on VBT

[Jani Nikula]

On Fri, 01 Apr 2016, "Zanoni, Paulo R" <paulo.r.zanoni@intel.com> wrote:
> Em Ter, 2016-02-23 às 18:46 +0200, ville.syrjala@linux.intel.com
> escreveu:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> 
>> DP dual mode type 1 DVI adaptors aren't required to implement any
>> registers, so it's a bit hard to detect them. The best way would
>> be to check the state of the CONFIG1 pin, but we have no way to
>> do that. So as a last resort, check the VBT to see if the HDMI
>> port is in fact a dual mode capable DP port.
>> 
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_bios.h |  3 +++
>>  drivers/gpu/drm/i915/intel_dp.c   | 28 ++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_drv.h  |  1 +
>>  drivers/gpu/drm/i915/intel_hdmi.c | 23 +++++++++++++++++++++--
>>  4 files changed, 53 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_bios.h
>> b/drivers/gpu/drm/i915/intel_bios.h
>> index 350d4e0f75a4..50d1659efe47 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.h
>> +++ b/drivers/gpu/drm/i915/intel_bios.h
>> @@ -730,6 +730,7 @@ struct bdb_psr {
>>  #define	 DEVICE_TYPE_INT_TV	0x1009
>>  #define	 DEVICE_TYPE_HDMI	0x60D2
>>  #define	 DEVICE_TYPE_DP		0x68C6
>> +#define	 DEVICE_TYPE_DP_DUAL_MODE	0x60D6
>>  #define	 DEVICE_TYPE_eDP	0x78C6
>>  
>>  #define  DEVICE_TYPE_CLASS_EXTENSION	(1 << 15)
>> @@ -764,6 +765,8 @@ struct bdb_psr {
>>  	 DEVICE_TYPE_DISPLAYPORT_OUTPUT | \
>>  	 DEVICE_TYPE_ANALOG_OUTPUT)
>>  
>> +#define DEVICE_TYPE_DP_DUAL_MODE_BITS ~DEVICE_TYPE_NOT_HDMI_OUTPUT
>> +
>>  /* define the DVO port for HDMI output type */
>>  #define		DVO_B		1
>>  #define		DVO_C		2
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> b/drivers/gpu/drm/i915/intel_dp.c
>> index cbc06596659a..f3edacf517ac 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -5104,6 +5104,34 @@ bool intel_dp_is_edp(struct drm_device *dev,
>> enum port port)
>>  	return false;
>>  }
>>  
>> +bool intel_dp_is_dual_mode(struct drm_i915_private *dev_priv, enum
>> port port)
>> +{
>> +	const union child_device_config *p_child;
>> +	int i;
>> +	static const short port_mapping[] = {
>> +		[PORT_B] = DVO_PORT_DPB,
>> +		[PORT_C] = DVO_PORT_DPC,
>> +		[PORT_D] = DVO_PORT_DPD,
>> +		[PORT_E] = DVO_PORT_DPE,
>> +	};
>> +
>> +	if (port == PORT_A || port >= ARRAY_SIZE(port_mapping))
>> +		return false;
>> +
>> +	if (!dev_priv->vbt.child_dev_num)
>> +		return false;
>> +
>> +	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
>> +		p_child = &dev_priv->vbt.child_dev[i];
>> +
>> +		if (p_child->common.dvo_port == port_mapping[port]
>> &&
>> +		    (p_child->common.device_type &
>> DEVICE_TYPE_DP_DUAL_MODE_BITS) ==
>> +		    (DEVICE_TYPE_DP_DUAL_MODE &
>> DEVICE_TYPE_DP_DUAL_MODE_BITS))
>> +			return true;
>> +	}
>
> Some thoughts:
>
> This is going to be implemented for all VBT versions. Since there's no
> real history about anything before version 155, is this really what we
> want? A huge part of the "we don't trust the VBT" culture we have on
> our team is because of those old versions being completely unreliable.
> If this is the case, we could make this implementation just be a small
> patch in parse_ddi_port(). I'm kinda afraid we may somehow break old
> machines yet again.
>
> - Instead of creating these complicated bit masks, why don't we just
> specifically check "if bit 2 and bit 4 are enabled, we're using an
> adaptor"? Much simpler IMHO.
>
> - Jani's recent patch suggests you may want to move this function to
> intel_bios.c in order to avoid including intel_vbt_defs.h from
> intel_hdmi.c. Anyway, you'll have to rebase.

Yup, it should become intel_bios_is_dp_dual_mode() or something along
those lines in intel_bios.c. intel_vbt_defs.h is now private data to
intel_bios.c. intel_bios.h should be kept minimal.

My long term plan is to make most of dev_priv->vbt opaque pointers you
can query using helpers provided by intel_bios.c. In the future, this
should make it easier to change how we obtain and store the data. This
should also make it easier to handle setups without VBT or with limited
VBT.

BR,
Jani.

>
>> +	return false;
>> +}
>> +
>>  void
>>  intel_dp_add_properties(struct intel_dp *intel_dp, struct
>> drm_connector *connector)
>>  {
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 3ca29a181e64..c7d1ea4dbe42 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1248,6 +1248,7 @@ int intel_dp_sink_crc(struct intel_dp
>> *intel_dp, u8 *crc);
>>  bool intel_dp_compute_config(struct intel_encoder *encoder,
>>  			     struct intel_crtc_state *pipe_config);
>>  bool intel_dp_is_edp(struct drm_device *dev, enum port port);
>> +bool intel_dp_is_dual_mode(struct drm_i915_private *dev_priv, enum
>> port port);
>>  enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port
>> *intel_dig_port,
>>  				  bool long_hpd);
>>  void intel_edp_backlight_on(struct intel_dp *intel_dp);
>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
>> b/drivers/gpu/drm/i915/intel_hdmi.c
>> index 660a65f48fd8..1476f3afb7e2 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -1390,14 +1390,33 @@ intel_hdmi_unset_edid(struct drm_connector
>> *connector)
>>  }
>>  
>>  static void
>> -intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector)
>> +intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector, bool
>> has_edid)
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(connector->dev);
>>  	struct intel_hdmi *hdmi = intel_attached_hdmi(connector);
>> +	enum port port = hdmi_to_dig_port(hdmi)->port;
>>  	struct i2c_adapter *adapter =
>>  		intel_gmbus_get_adapter(dev_priv, hdmi->ddc_bus);
>>  	enum drm_dp_dual_mode_type type =
>> drm_dp_dual_mode_detect(adapter);
>>  
>> +	/*
>> +	 * Type 1 DVI adaptors are not required to implement any
>> +	 * registers, so we can't always detect their presence.
>> +	 * Ideally we should be able to check the state of the
>> +	 * CONFIG1 pin, but no such luck on our hardware.
>> +	 *
>> +	 * The only method left to us is to check the VBT to see
>> +	 * if the port is a dual mode capable DP port. But let's
>> +	 * only do that when we sucesfully read the EDID, to avoid
>> +	 * confusing log messages about DP dual mode adaptors when
>> +	 * there's nothing connected to the port.
>> +	 */
>> +	if (type == DRM_DP_DUAL_MODE_NONE && has_edid &&
>> +	    intel_dp_is_dual_mode(dev_priv, port)) {
>> +		DRM_DEBUG_KMS("Assuming DP dual mode adaptor
>> presence based on VBT\n");
>> +		type = DRM_DP_DUAL_MODE_TYPE1_DVI;
>> +	}
>> +
>>  	if (type == DRM_DP_DUAL_MODE_NONE)
>>  		return;
>>  
>> @@ -1441,7 +1460,7 @@ intel_hdmi_set_edid(struct drm_connector
>> *connector, bool force)
>>  				    intel_gmbus_get_adapter(dev_priv
>> ,
>>  				    intel_hdmi->ddc_bus));
>>  
>> -		intel_hdmi_dp_dual_mode_detect(connector);
>> +		intel_hdmi_dp_dual_mode_detect(connector, edid !=
>> NULL);
>>  
>>  		intel_display_power_put(dev_priv,
>> POWER_DOMAIN_GMBUS);
>>  	}

-- 
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: [Intel-gfx] [PATCH 4/4] drm/i915: Determine DP++ type 1 DVI adaptor presence based on VBT

[Ville Syrjälä]

On Thu, Mar 31, 2016 at 10:06:37PM +0000, Zanoni, Paulo R wrote:
> Em Ter, 2016-02-23 às 18:46 +0200, ville.syrjala@linux.intel.com
> escreveu:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > DP dual mode type 1 DVI adaptors aren't required to implement any
> > registers, so it's a bit hard to detect them. The best way would
> > be to check the state of the CONFIG1 pin, but we have no way to
> > do that. So as a last resort, check the VBT to see if the HDMI
> > port is in fact a dual mode capable DP port.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_bios.h |  3 +++
> >  drivers/gpu/drm/i915/intel_dp.c   | 28 ++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h  |  1 +
> >  drivers/gpu/drm/i915/intel_hdmi.c | 23 +++++++++++++++++++++--
> >  4 files changed, 53 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_bios.h
> > b/drivers/gpu/drm/i915/intel_bios.h
> > index 350d4e0f75a4..50d1659efe47 100644
> > --- a/drivers/gpu/drm/i915/intel_bios.h
> > +++ b/drivers/gpu/drm/i915/intel_bios.h
> > @@ -730,6 +730,7 @@ struct bdb_psr {
> >  #define	 DEVICE_TYPE_INT_TV	0x1009
> >  #define	 DEVICE_TYPE_HDMI	0x60D2
> >  #define	 DEVICE_TYPE_DP		0x68C6
> > +#define	 DEVICE_TYPE_DP_DUAL_MODE	0x60D6
> >  #define	 DEVICE_TYPE_eDP	0x78C6
> >  
> >  #define  DEVICE_TYPE_CLASS_EXTENSION	(1 << 15)
> > @@ -764,6 +765,8 @@ struct bdb_psr {
> >  	 DEVICE_TYPE_DISPLAYPORT_OUTPUT | \
> >  	 DEVICE_TYPE_ANALOG_OUTPUT)
> >  
> > +#define DEVICE_TYPE_DP_DUAL_MODE_BITS ~DEVICE_TYPE_NOT_HDMI_OUTPUT
> > +
> >  /* define the DVO port for HDMI output type */
> >  #define		DVO_B		1
> >  #define		DVO_C		2
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index cbc06596659a..f3edacf517ac 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -5104,6 +5104,34 @@ bool intel_dp_is_edp(struct drm_device *dev,
> > enum port port)
> >  	return false;
> >  }
> >  
> > +bool intel_dp_is_dual_mode(struct drm_i915_private *dev_priv, enum
> > port port)
> > +{
> > +	const union child_device_config *p_child;
> > +	int i;
> > +	static const short port_mapping[] = {
> > +		[PORT_B] = DVO_PORT_DPB,
> > +		[PORT_C] = DVO_PORT_DPC,
> > +		[PORT_D] = DVO_PORT_DPD,
> > +		[PORT_E] = DVO_PORT_DPE,
> > +	};
> > +
> > +	if (port == PORT_A || port >= ARRAY_SIZE(port_mapping))
> > +		return false;
> > +
> > +	if (!dev_priv->vbt.child_dev_num)
> > +		return false;
> > +
> > +	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
> > +		p_child = &dev_priv->vbt.child_dev[i];
> > +
> > +		if (p_child->common.dvo_port == port_mapping[port]
> > &&
> > +		    (p_child->common.device_type &
> > DEVICE_TYPE_DP_DUAL_MODE_BITS) ==
> > +		    (DEVICE_TYPE_DP_DUAL_MODE &
> > DEVICE_TYPE_DP_DUAL_MODE_BITS))
> > +			return true;
> > +	}
> 
> Some thoughts:
> 
> This is going to be implemented for all VBT versions. Since there's no
> real history about anything before version 155, is this really what we
> want? A huge part of the "we don't trust the VBT" culture we have on
> our team is because of those old versions being completely unreliable.
> If this is the case, we could make this implementation just be a small
> patch in parse_ddi_port(). I'm kinda afraid we may somehow break old
> machines yet again.

I don't think it matters much. ILK being the oldest platform with DP++
capable of >165MHz, and at least my ILK here already has VBT version
163. Also this device type stuff was there before 155 already. And
the is_edp() thing has worked for us mostly fine so if we things the
same way it seems unlikely we get too many problems.

> 
> - Instead of creating these complicated bit masks, why don't we just
> specifically check "if bit 2 and bit 4 are enabled, we're using an
> adaptor"? Much simpler IMHO.

I'm not sure it's a good idea to trust that some crappy BIOS doesn't
just set whatever random bits on a random port. So a more
conservative approach seems like a better idea to me. Also it matches
how we do the is_edp() check.

> 
> - Jani's recent patch suggests you may want to move this function to
> intel_bios.c in order to avoid including intel_vbt_defs.h from
> intel_hdmi.c. Anyway, you'll have to rebase.
> 
> > +	return false;
> > +}
> > +
> >  void
> >  intel_dp_add_properties(struct intel_dp *intel_dp, struct
> > drm_connector *connector)
> >  {
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 3ca29a181e64..c7d1ea4dbe42 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1248,6 +1248,7 @@ int intel_dp_sink_crc(struct intel_dp
> > *intel_dp, u8 *crc);
> >  bool intel_dp_compute_config(struct intel_encoder *encoder,
> >  			     struct intel_crtc_state *pipe_config);
> >  bool intel_dp_is_edp(struct drm_device *dev, enum port port);
> > +bool intel_dp_is_dual_mode(struct drm_i915_private *dev_priv, enum
> > port port);
> >  enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port
> > *intel_dig_port,
> >  				  bool long_hpd);
> >  void intel_edp_backlight_on(struct intel_dp *intel_dp);
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 660a65f48fd8..1476f3afb7e2 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -1390,14 +1390,33 @@ intel_hdmi_unset_edid(struct drm_connector
> > *connector)
> >  }
> >  
> >  static void
> > -intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector)
> > +intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector, bool
> > has_edid)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(connector->dev);
> >  	struct intel_hdmi *hdmi = intel_attached_hdmi(connector);
> > +	enum port port = hdmi_to_dig_port(hdmi)->port;
> >  	struct i2c_adapter *adapter =
> >  		intel_gmbus_get_adapter(dev_priv, hdmi->ddc_bus);
> >  	enum drm_dp_dual_mode_type type =
> > drm_dp_dual_mode_detect(adapter);
> >  
> > +	/*
> > +	 * Type 1 DVI adaptors are not required to implement any
> > +	 * registers, so we can't always detect their presence.
> > +	 * Ideally we should be able to check the state of the
> > +	 * CONFIG1 pin, but no such luck on our hardware.
> > +	 *
> > +	 * The only method left to us is to check the VBT to see
> > +	 * if the port is a dual mode capable DP port. But let's
> > +	 * only do that when we sucesfully read the EDID, to avoid
> > +	 * confusing log messages about DP dual mode adaptors when
> > +	 * there's nothing connected to the port.
> > +	 */
> > +	if (type == DRM_DP_DUAL_MODE_NONE && has_edid &&
> > +	    intel_dp_is_dual_mode(dev_priv, port)) {
> > +		DRM_DEBUG_KMS("Assuming DP dual mode adaptor
> > presence based on VBT\n");
> > +		type = DRM_DP_DUAL_MODE_TYPE1_DVI;
> > +	}
> > +
> >  	if (type == DRM_DP_DUAL_MODE_NONE)
> >  		return;
> >  
> > @@ -1441,7 +1460,7 @@ intel_hdmi_set_edid(struct drm_connector
> > *connector, bool force)
> >  				    intel_gmbus_get_adapter(dev_priv
> > ,
> >  				    intel_hdmi->ddc_bus));
> >  
> > -		intel_hdmi_dp_dual_mode_detect(connector);
> > +		intel_hdmi_dp_dual_mode_detect(connector, edid !=
> > NULL);
> >  
> >  		intel_display_power_put(dev_priv,
> > POWER_DOMAIN_GMBUS);
> >  	}

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

Re: [PATCH 4/4] drm/i915: Determine DP++ type 1 DVI adaptor presence based on VBT

[Zanoni, Paulo R]

Em Sex, 2016-04-01 às 17:45 +0300, Ville Syrjälä escreveu:
> On Thu, Mar 31, 2016 at 10:06:37PM +0000, Zanoni, Paulo R wrote:
> > 
> > Em Ter, 2016-02-23 às 18:46 +0200, ville.syrjala@linux.intel.com
> > escreveu:
> > > 
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > DP dual mode type 1 DVI adaptors aren't required to implement any
> > > registers, so it's a bit hard to detect them. The best way would
> > > be to check the state of the CONFIG1 pin, but we have no way to
> > > do that. So as a last resort, check the VBT to see if the HDMI
> > > port is in fact a dual mode capable DP port.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_bios.h |  3 +++
> > >  drivers/gpu/drm/i915/intel_dp.c   | 28
> > > ++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h  |  1 +
> > >  drivers/gpu/drm/i915/intel_hdmi.c | 23 +++++++++++++++++++++--
> > >  4 files changed, 53 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_bios.h
> > > b/drivers/gpu/drm/i915/intel_bios.h
> > > index 350d4e0f75a4..50d1659efe47 100644
> > > --- a/drivers/gpu/drm/i915/intel_bios.h
> > > +++ b/drivers/gpu/drm/i915/intel_bios.h
> > > @@ -730,6 +730,7 @@ struct bdb_psr {
> > >  #define	 DEVICE_TYPE_INT_TV	0x1009
> > >  #define	 DEVICE_TYPE_HDMI	0x60D2
> > >  #define	 DEVICE_TYPE_DP		0x68C6
> > > +#define	 DEVICE_TYPE_DP_DUAL_MODE	0x60D6
> > >  #define	 DEVICE_TYPE_eDP	0x78C6
> > >  
> > >  #define  DEVICE_TYPE_CLASS_EXTENSION	(1 << 15)
> > > @@ -764,6 +765,8 @@ struct bdb_psr {
> > >  	 DEVICE_TYPE_DISPLAYPORT_OUTPUT | \
> > >  	 DEVICE_TYPE_ANALOG_OUTPUT)
> > >  
> > > +#define DEVICE_TYPE_DP_DUAL_MODE_BITS
> > > ~DEVICE_TYPE_NOT_HDMI_OUTPUT
> > > +
> > >  /* define the DVO port for HDMI output type */
> > >  #define		DVO_B		1
> > >  #define		DVO_C		2
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index cbc06596659a..f3edacf517ac 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -5104,6 +5104,34 @@ bool intel_dp_is_edp(struct drm_device
> > > *dev,
> > > enum port port)
> > >  	return false;
> > >  }
> > >  
> > > +bool intel_dp_is_dual_mode(struct drm_i915_private *dev_priv,
> > > enum
> > > port port)
> > > +{
> > > +	const union child_device_config *p_child;
> > > +	int i;
> > > +	static const short port_mapping[] = {
> > > +		[PORT_B] = DVO_PORT_DPB,
> > > +		[PORT_C] = DVO_PORT_DPC,
> > > +		[PORT_D] = DVO_PORT_DPD,
> > > +		[PORT_E] = DVO_PORT_DPE,
> > > +	};
> > > +
> > > +	if (port == PORT_A || port >= ARRAY_SIZE(port_mapping))
> > > +		return false;
> > > +
> > > +	if (!dev_priv->vbt.child_dev_num)
> > > +		return false;
> > > +
> > > +	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
> > > +		p_child = &dev_priv->vbt.child_dev[i];
> > > +
> > > +		if (p_child->common.dvo_port ==
> > > port_mapping[port]
> > > &&
> > > +		    (p_child->common.device_type &
> > > DEVICE_TYPE_DP_DUAL_MODE_BITS) ==
> > > +		    (DEVICE_TYPE_DP_DUAL_MODE &
> > > DEVICE_TYPE_DP_DUAL_MODE_BITS))
> > > +			return true;
> > > +	}
> > Some thoughts:
> > 
> > This is going to be implemented for all VBT versions. Since there's
> > no
> > real history about anything before version 155, is this really what
> > we
> > want? A huge part of the "we don't trust the VBT" culture we have
> > on
> > our team is because of those old versions being completely
> > unreliable.
> > If this is the case, we could make this implementation just be a
> > small
> > patch in parse_ddi_port(). I'm kinda afraid we may somehow break
> > old
> > machines yet again.
> I don't think it matters much. ILK being the oldest platform with
> DP++
> capable of >165MHz, and at least my ILK here already has VBT version
> 163. Also this device type stuff was there before 155 already. And
> the is_edp() thing has worked for us mostly fine so if we things the
> same way it seems unlikely we get too many problems.

> 
> > 
> > 
> > - Instead of creating these complicated bit masks, why don't we
> > just
> > specifically check "if bit 2 and bit 4 are enabled, we're using an
> > adaptor"? Much simpler IMHO.
> I'm not sure it's a good idea to trust that some crappy BIOS doesn't
> just set whatever random bits on a random port. So a more
> conservative approach seems like a better idea to me. Also it matches
> how we do the is_edp() check.

Funcion intel_ddi_init() already only looks at the specific bits when
deciding whether to initialize DP or HDMI. OTOH that's just for HSW+
which has a somewhat-trustable VBT.

Anyway, I have no further comments for the series. Feel free to send v2
based on the discussion and your conclusions.

> 
> > 
> > 
> > - Jani's recent patch suggests you may want to move this function
> > to
> > intel_bios.c in order to avoid including intel_vbt_defs.h from
> > intel_hdmi.c. Anyway, you'll have to rebase.
> > 
> > > 
> > > +	return false;
> > > +}
> > > +
> > >  void
> > >  intel_dp_add_properties(struct intel_dp *intel_dp, struct
> > > drm_connector *connector)
> > >  {
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 3ca29a181e64..c7d1ea4dbe42 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1248,6 +1248,7 @@ int intel_dp_sink_crc(struct intel_dp
> > > *intel_dp, u8 *crc);
> > >  bool intel_dp_compute_config(struct intel_encoder *encoder,
> > >  			     struct intel_crtc_state
> > > *pipe_config);
> > >  bool intel_dp_is_edp(struct drm_device *dev, enum port port);
> > > +bool intel_dp_is_dual_mode(struct drm_i915_private *dev_priv,
> > > enum
> > > port port);
> > >  enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port
> > > *intel_dig_port,
> > >  				  bool long_hpd);
> > >  void intel_edp_backlight_on(struct intel_dp *intel_dp);
> > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > > b/drivers/gpu/drm/i915/intel_hdmi.c
> > > index 660a65f48fd8..1476f3afb7e2 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > @@ -1390,14 +1390,33 @@ intel_hdmi_unset_edid(struct
> > > drm_connector
> > > *connector)
> > >  }
> > >  
> > >  static void
> > > -intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector)
> > > +intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector,
> > > bool
> > > has_edid)
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(connector-
> > > >dev);
> > >  	struct intel_hdmi *hdmi =
> > > intel_attached_hdmi(connector);
> > > +	enum port port = hdmi_to_dig_port(hdmi)->port;
> > >  	struct i2c_adapter *adapter =
> > >  		intel_gmbus_get_adapter(dev_priv, hdmi-
> > > >ddc_bus);
> > >  	enum drm_dp_dual_mode_type type =
> > > drm_dp_dual_mode_detect(adapter);
> > >  
> > > +	/*
> > > +	 * Type 1 DVI adaptors are not required to implement any
> > > +	 * registers, so we can't always detect their presence.
> > > +	 * Ideally we should be able to check the state of the
> > > +	 * CONFIG1 pin, but no such luck on our hardware.
> > > +	 *
> > > +	 * The only method left to us is to check the VBT to see
> > > +	 * if the port is a dual mode capable DP port. But let's
> > > +	 * only do that when we sucesfully read the EDID, to
> > > avoid
> > > +	 * confusing log messages about DP dual mode adaptors
> > > when
> > > +	 * there's nothing connected to the port.
> > > +	 */
> > > +	if (type == DRM_DP_DUAL_MODE_NONE && has_edid &&
> > > +	    intel_dp_is_dual_mode(dev_priv, port)) {
> > > +		DRM_DEBUG_KMS("Assuming DP dual mode adaptor
> > > presence based on VBT\n");
> > > +		type = DRM_DP_DUAL_MODE_TYPE1_DVI;
> > > +	}
> > > +
> > >  	if (type == DRM_DP_DUAL_MODE_NONE)
> > >  		return;
> > >  
> > > @@ -1441,7 +1460,7 @@ intel_hdmi_set_edid(struct drm_connector
> > > *connector, bool force)
> > >  				    intel_gmbus_get_adapter(dev_
> > > priv
> > > ,
> > >  				    intel_hdmi->ddc_bus));
> > >  
> > > -		intel_hdmi_dp_dual_mode_detect(connector);
> > > +		intel_hdmi_dp_dual_mode_detect(connector, edid
> > > !=
> > > NULL);
> > >  
> > >  		intel_display_power_put(dev_priv,
> > > POWER_DOMAIN_GMBUS);
> > >  	}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for drm: DP++ adaptor support

[Patchwork]

== Series Details ==

Series: drm: DP++ adaptor support
URL   : https://patchwork.freedesktop.org/series/3735/
State : warning

== Summary ==

Series 3735v1 drm: DP++ adaptor support
http://patchwork.freedesktop.org/api/1.0/series/3735/revisions/1/mbox/

Test gem_cs_prefetch:
        Subgroup basic-default:
                incomplete -> PASS       (ilk-hp8440p)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
Test kms_force_connector_basic:
        Subgroup force-edid:
                skip       -> PASS       (snb-x220t)
        Subgroup force-load-detect:
                dmesg-fail -> FAIL       (snb-x220t)
                dmesg-fail -> FAIL       (snb-dellxps)
                dmesg-fail -> FAIL       (hsw-gt2)
                fail       -> DMESG-FAIL (ilk-hp8440p)
        Subgroup prune-stale-modes:
                pass       -> SKIP       (ilk-hp8440p)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                pass       -> DMESG-WARN (ivb-t430s)
Test pm_rpm:
        Subgroup basic-rte:
                pass       -> DMESG-WARN (bsw-nuc-2)

bdw-nuci7        total:165  pass:154  dwarn:0   dfail:0   fail:0   skip:11 
bdw-ultra        total:168  pass:154  dwarn:0   dfail:0   fail:0   skip:14 
bsw-nuc-2        total:168  pass:136  dwarn:1   dfail:0   fail:1   skip:30 
byt-nuc          total:168  pass:142  dwarn:1   dfail:0   fail:0   skip:25 
hsw-gt2          total:168  pass:157  dwarn:0   dfail:0   fail:1   skip:10 
ilk-hp8440p      total:168  pass:116  dwarn:1   dfail:1   fail:0   skip:50 
ivb-t430s        total:168  pass:152  dwarn:1   dfail:0   fail:1   skip:14 
snb-dellxps      total:168  pass:145  dwarn:0   dfail:0   fail:1   skip:22 
snb-x220t        total:168  pass:145  dwarn:0   dfail:0   fail:2   skip:21 

Results at /archive/results/CI_IGT_test/Patchwork_1468/

08fc1b101049694778bff7559e1d05250d2e7072 drm-intel-nightly: 2016y-02m-22d-17h-30m-27s UTC integration manifest
05bb77f0fe7066499d2a5d84f5f9b84972c558fd drm/i915: Determine DP++ type 1 DVI adaptor presence based on VBT
adb14e9638f527e31c900b459cc302ff08de7db9 drm/i915: Enable/disable TMDS output buffers in DP++ adaptor as needed
637d45111be8a054f30f5bbb019cc70c74f7f9c5 drm/i915: Respect DP++ adaptor TMDS clock limit
5682fbc251a05fbad3306834c761cc59607f8c62 drm: Add helper for DP++ adaptors

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