[PATCHv3 0/3] drm/i915: add DisplayPort CEC-Tunneling-over-AUX support

[PATCHv3 0/3] drm/i915: add DisplayPort CEC-Tunneling-over-AUX support

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This patch series adds support for the DisplayPort CEC-Tunneling-over-AUX
feature. This patch series is based on the current pre-4.14-rc1 mainline
which has all the needed cec 4.14 patches merged.

This patch series has been tested with my NUC7i5BNK and a Samsung USB-C to 
HDMI adapter.

Please note this comment at the start of drm_dp_cec.c:

----------------------------------------------------------------------
Unfortunately it turns out that we have a chicken-and-egg situation
here. Quite a few active (mini-)DP-to-HDMI or USB-C-to-HDMI adapters
have a converter chip that supports CEC-Tunneling-over-AUX (usually the
Parade PS176), but they do not wire up the CEC pin, thus making CEC
useless.

Sadly there is no way for this driver to know this. What happens is 
that a /dev/cecX device is created that is isolated and unable to see
any of the other CEC devices. Quite literally the CEC wire is cut
(or in this case, never connected in the first place).

I suspect that the reason so few adapters support this is that this
tunneling protocol was never supported by any OS. So there was no 
easy way of testing it, and no incentive to correctly wire up the
CEC pin.

Hopefully by creating this driver it will be easier for vendors to 
finally fix their adapters and test the CEC functionality.

I keep a list of known working adapters here:

https://hverkuil.home.xs4all.nl/cec-status.txt

Please mail me (hverkuil@xs4all.nl) if you find an adapter that works
and is not yet listed there.
----------------------------------------------------------------------

I really hope that this work will provide an incentive for vendors to
finally connect the CEC pin. It's a shame that there are so few adapters
that work (I found only two USB-C to HDMI adapters that work, and no
(mini-)DP to HDMI adapters at all).

Note that a colleague who actually knows his way around a soldering iron
modified an UpTab DisplayPort-to-HDMI adapter for me, hooking up the CEC
pin. And after that change it worked. I also received confirmation that
this really is a chicken-and-egg situation: it is because there is no CEC
support for this feature in any OS that they do not hook up the CEC pin.

So hopefully if this gets merged there will be an incentive for vendors
to make adapters where this actually works. It is a very nice feature
for HTPC boxes.

I've added Imre and Ville. It would be great if this can go in for 4.15.

Changes since v2:

- Use the new CEC_CAP_DEFAULTS define
- Replace 'if (!IS_ERR_OR_NULL(aux->cec_adap)) {' in drm_dp_cec_configure_adapter()
  by just 'if (aux->cec_adap) {'.

Changes since v1:

- Incorporated Sean's review comments in patch 1/3.

Regards,

        Hans

Hans Verkuil (3):
  drm: add support for DisplayPort CEC-Tunneling-over-AUX
  drm-kms-helpers.rst: document the DP CEC helpers
  drm/i915: add DisplayPort CEC-Tunneling-over-AUX support

 Documentation/gpu/drm-kms-helpers.rst |   9 +
 drivers/gpu/drm/Kconfig               |  10 ++
 drivers/gpu/drm/Makefile              |   1 +
 drivers/gpu/drm/drm_dp_cec.c          | 301 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c       |  18 +-
 include/drm/drm_dp_helper.h           |  24 +++
 6 files changed, 359 insertions(+), 4 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_dp_cec.c

-- 
2.14.1

[PATCHv3 1/3] drm: add support for DisplayPort CEC-Tunneling-over-AUX

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This adds support for the DisplayPort CEC-Tunneling-over-AUX
feature that is part of the DisplayPort 1.3 standard.

Unfortunately, not all DisplayPort/USB-C to HDMI adapters with a
chip that has this capability actually hook up the CEC pin, so
even though a CEC device is created, it may not actually work.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Tested-by: Carlos Santa <carlos.santa@intel.com>
---
 drivers/gpu/drm/Kconfig      |  10 ++
 drivers/gpu/drm/Makefile     |   1 +
 drivers/gpu/drm/drm_dp_cec.c | 301 +++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_dp_helper.h  |  24 ++++
 4 files changed, 336 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_dp_cec.c

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 83cb2a88c204..1f2708df5c4e 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -120,6 +120,16 @@ config DRM_LOAD_EDID_FIRMWARE
 	  default case is N. Details and instructions how to build your own
 	  EDID data are given in Documentation/EDID/HOWTO.txt.
 
+config DRM_DP_CEC
+	bool "Enable DisplayPort CEC-Tunneling-over-AUX HDMI support"
+	select CEC_CORE
+	help
+	  Choose this option if you want to enable HDMI CEC support for
+	  DisplayPort/USB-C to HDMI adapters.
+
+	  Note: not all adapters support this feature, and even for those
+	  that do support this they often do not hook up the CEC pin.
+
 config DRM_TTM
 	tristate
 	depends on DRM && MMU
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 24a066e1841c..c6552c62049e 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -40,6 +40,7 @@ drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
 drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
 drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
+drm_kms_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o
 
 obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
 obj-$(CONFIG_DRM_DEBUG_MM_SELFTEST) += selftests/
diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
new file mode 100644
index 000000000000..b831bb72c932
--- /dev/null
+++ b/drivers/gpu/drm/drm_dp_cec.c
@@ -0,0 +1,301 @@
+/*
+ * DisplayPort CEC-Tunneling-over-AUX support
+ *
+ * Copyright 2017 Cisco Systems, Inc. and/or its affiliates. All rights reserved.
+ *
+ * This program is free software; you may redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * 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 AUTHORS OR COPYRIGHT HOLDERS
+ * 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/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <drm/drm_dp_helper.h>
+#include <media/cec.h>
+
+/*
+ * Unfortunately it turns out that we have a chicken-and-egg situation
+ * here. Quite a few active (mini-)DP-to-HDMI or USB-C-to-HDMI adapters
+ * have a converter chip that supports CEC-Tunneling-over-AUX (usually the
+ * Parade PS176), but they do not wire up the CEC pin, thus making CEC
+ * useless.
+ *
+ * Sadly there is no way for this driver to know this. What happens is
+ * that a /dev/cecX device is created that is isolated and unable to see
+ * any of the other CEC devices. Quite literally the CEC wire is cut
+ * (or in this case, never connected in the first place).
+ *
+ * I suspect that the reason so few adapters support this is that this
+ * tunneling protocol was never supported by any OS. So there was no
+ * easy way of testing it, and no incentive to correctly wire up the
+ * CEC pin.
+ *
+ * Hopefully by creating this driver it will be easier for vendors to
+ * finally fix their adapters and test the CEC functionality.
+ *
+ * I keep a list of known working adapters here:
+ *
+ * https://hverkuil.home.xs4all.nl/cec-status.txt
+ *
+ * Please mail me (hverkuil@xs4all.nl) if you find an adapter that works
+ * and is not yet listed there.
+ */
+
+/**
+ * DOC: dp cec helpers
+ *
+ * These functions take care of supporting the CEC-Tunneling-over-AUX
+ * feature of DisplayPort-to-HDMI adapters.
+ */
+
+static int drm_dp_cec_adap_enable(struct cec_adapter *adap, bool enable)
+{
+	struct drm_dp_aux *aux = cec_get_drvdata(adap);
+	u32 val = enable ? DP_CEC_TUNNELING_ENABLE : 0;
+	ssize_t err = 0;
+
+	err = drm_dp_dpcd_writeb(aux, DP_CEC_TUNNELING_CONTROL, val);
+	return (enable && err < 0) ? err : 0;
+}
+
+static int drm_dp_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
+{
+	struct drm_dp_aux *aux = cec_get_drvdata(adap);
+	/* Bit 15 (logical address 15) should always be set */
+	u16 la_mask = 1 << CEC_LOG_ADDR_BROADCAST;
+	u8 mask[2];
+	ssize_t err;
+
+	if (addr != CEC_LOG_ADDR_INVALID)
+		la_mask |= adap->log_addrs.log_addr_mask | (1 << addr);
+	mask[0] = la_mask & 0xff;
+	mask[1] = la_mask >> 8;
+	err = drm_dp_dpcd_write(aux, DP_CEC_LOGICAL_ADDRESS_MASK, mask, 2);
+	return (addr != CEC_LOG_ADDR_INVALID && err < 0) ? err : 0;
+}
+
+static int drm_dp_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
+				    u32 signal_free_time, struct cec_msg *msg)
+{
+	struct drm_dp_aux *aux = cec_get_drvdata(adap);
+	unsigned int retries = min(5, attempts - 1);
+	ssize_t err;
+
+	err = drm_dp_dpcd_write(aux, DP_CEC_TX_MESSAGE_BUFFER,
+				msg->msg, msg->len);
+	if (err < 0)
+		return err;
+
+	err = drm_dp_dpcd_writeb(aux, DP_CEC_TX_MESSAGE_INFO,
+				 (msg->len - 1) | (retries << 4) |
+				 DP_CEC_TX_MESSAGE_SEND);
+	return err < 0 ? err : 0;
+}
+
+static int drm_dp_cec_adap_monitor_all_enable(struct cec_adapter *adap,
+					      bool enable)
+{
+	struct drm_dp_aux *aux = cec_get_drvdata(adap);
+	ssize_t err;
+	u8 val;
+
+	if (!(adap->capabilities & CEC_CAP_MONITOR_ALL))
+		return 0;
+
+	err = drm_dp_dpcd_readb(aux, DP_CEC_TUNNELING_CONTROL, &val);
+	if (err >= 0) {
+		if (enable)
+			val |= DP_CEC_SNOOPING_ENABLE;
+		else
+			val &= ~DP_CEC_SNOOPING_ENABLE;
+		err = drm_dp_dpcd_writeb(aux, DP_CEC_TUNNELING_CONTROL, val);
+	}
+	return (enable && err < 0) ? err : 0;
+}
+
+static void drm_dp_cec_adap_status(struct cec_adapter *adap,
+				   struct seq_file *file)
+{
+	struct drm_dp_aux *aux = cec_get_drvdata(adap);
+	u8 buf[DP_AUX_MAX_PAYLOAD_BYTES];
+	u8 hw_rev;
+
+	if (drm_dp_dpcd_read(aux, DP_BRANCH_OUI,
+			     buf, sizeof(buf)) != sizeof(buf))
+		return;
+	hw_rev = buf[9];
+	buf[9] = 0;
+	seq_printf(file, "OUI: %02x-%02x-%02x\n",
+		   buf[0], buf[1], buf[2]);
+	seq_printf(file, "ID: %s\n", buf + 3);
+	seq_printf(file, "HW Rev: %d.%d\n", hw_rev >> 4, hw_rev & 0xf);
+	/*
+	 * Show this both in decimal and hex: at least one vendor
+	 * always reports this in hex.
+	 */
+	seq_printf(file, "FW/SW Rev: %d.%d (0x%02x.0x%02x)\n",
+		   buf[10], buf[11], buf[10], buf[11]);
+}
+
+static const struct cec_adap_ops drm_dp_cec_adap_ops = {
+	.adap_enable = drm_dp_cec_adap_enable,
+	.adap_log_addr = drm_dp_cec_adap_log_addr,
+	.adap_transmit = drm_dp_cec_adap_transmit,
+	.adap_monitor_all_enable = drm_dp_cec_adap_monitor_all_enable,
+	.adap_status = drm_dp_cec_adap_status,
+};
+
+static int drm_dp_cec_received(struct drm_dp_aux *aux)
+{
+	struct cec_adapter *adap = aux->cec_adap;
+	struct cec_msg msg;
+	u8 rx_msg_info;
+	ssize_t err;
+
+	err = drm_dp_dpcd_readb(aux, DP_CEC_RX_MESSAGE_INFO, &rx_msg_info);
+	if (err < 0)
+		return err;
+	if (!(rx_msg_info & DP_CEC_RX_MESSAGE_ENDED))
+		return 0;
+	msg.len = (rx_msg_info & DP_CEC_RX_MESSAGE_LEN_MASK) + 1;
+	err = drm_dp_dpcd_read(aux, DP_CEC_RX_MESSAGE_BUFFER, msg.msg, msg.len);
+	if (err < 0)
+		return err;
+	cec_received_msg(adap, &msg);
+	return 0;
+}
+
+static int drm_dp_cec_handle_irq(struct drm_dp_aux *aux)
+{
+	struct cec_adapter *adap = aux->cec_adap;
+	u8 flags;
+	ssize_t err;
+
+	err = drm_dp_dpcd_readb(aux, DP_CEC_TUNNELING_IRQ_FLAGS, &flags);
+	if (err < 0)
+		return err;
+
+	if (flags & DP_CEC_RX_MESSAGE_INFO_VALID)
+		drm_dp_cec_received(aux);
+
+	if (flags & DP_CEC_TX_MESSAGE_SENT)
+		cec_transmit_attempt_done(adap, CEC_TX_STATUS_OK);
+	else if (flags & DP_CEC_TX_LINE_ERROR)
+		cec_transmit_attempt_done(adap, CEC_TX_STATUS_ERROR |
+						CEC_TX_STATUS_MAX_RETRIES);
+	else if (flags &
+		 (DP_CEC_TX_ADDRESS_NACK_ERROR | DP_CEC_TX_DATA_NACK_ERROR))
+		cec_transmit_attempt_done(adap, CEC_TX_STATUS_NACK |
+						CEC_TX_STATUS_MAX_RETRIES);
+	drm_dp_dpcd_writeb(aux, DP_CEC_TUNNELING_IRQ_FLAGS, flags);
+	return 0;
+}
+
+/**
+ * drm_dp_cec_irq() - handle CEC interrupt, if any
+ * @aux: DisplayPort AUX channel
+ *
+ * Should be called when handling an IRQ_HPD request. If CEC-tunneling-over-AUX
+ * is present, then it will check for a CEC_IRQ and handle it accordingly.
+ *
+ * Returns true if an interrupt was handled successfully or false otherwise.
+ */
+bool drm_dp_cec_irq(struct drm_dp_aux *aux)
+{
+	bool handled = false;
+	int attempts;
+
+	if (!aux->cec_adap)
+		return false;
+
+	for (attempts = 0; attempts < 4; attempts++) {
+		u8 cec_irq;
+		int ret;
+
+		ret = drm_dp_dpcd_readb(aux, DP_DEVICE_SERVICE_IRQ_VECTOR_ESI1,
+					&cec_irq);
+		if (ret < 0 || !(cec_irq & DP_CEC_IRQ))
+			break;
+
+		if (!drm_dp_cec_handle_irq(aux))
+			handled = true;
+
+		ret = drm_dp_dpcd_writeb(aux, DP_DEVICE_SERVICE_IRQ_VECTOR_ESI1,
+					 DP_CEC_IRQ);
+		if (ret < 0)
+			break;
+	}
+	return handled;
+}
+EXPORT_SYMBOL(drm_dp_cec_irq);
+
+/**
+ * drm_dp_cec_configure_adapter() - configure the CEC adapter
+ * @aux: DisplayPort AUX channel
+ * @name: name of the CEC device
+ * @parent: parent device
+ *
+ * Checks if this is a DisplayPort-to-HDMI adapter that supports
+ * CEC-tunneling-over-AUX, and if so it creates a CEC device.
+ *
+ * If a CEC device was already created, then check if the capabilities
+ * have changed. If not, then do nothing. Otherwise destroy the old
+ * CEC device and create a new CEC device.
+ *
+ * This can happen when one DP-to-HDMI adapter is disconnected and
+ * replaced by another adapter with different CEC capabilities.
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int drm_dp_cec_configure_adapter(struct drm_dp_aux *aux, const char *name,
+				 struct device *parent)
+{
+	u32 cec_caps = CEC_CAP_DEFAULTS | CEC_CAP_NEEDS_HPD;
+	unsigned int num_las = 1;
+	int err;
+	u8 cap;
+
+	if (drm_dp_dpcd_readb(aux, DP_CEC_TUNNELING_CAPABILITY, &cap) != 1 ||
+	    !(cap & DP_CEC_TUNNELING_CAPABLE)) {
+		cec_unregister_adapter(aux->cec_adap);
+		aux->cec_adap = NULL;
+		return -ENODEV;
+	}
+
+	if (cap & DP_CEC_SNOOPING_CAPABLE)
+		cec_caps |= CEC_CAP_MONITOR_ALL;
+	if (cap & DP_CEC_MULTIPLE_LA_CAPABLE)
+		num_las = CEC_MAX_LOG_ADDRS;
+
+	if (aux->cec_adap) {
+		if (aux->cec_adap->capabilities == cec_caps &&
+		    aux->cec_adap->available_log_addrs == num_las)
+			return 0;
+		cec_unregister_adapter(aux->cec_adap);
+	}
+
+	aux->cec_adap = cec_allocate_adapter(&drm_dp_cec_adap_ops,
+			 aux, name, cec_caps, num_las);
+	if (IS_ERR(aux->cec_adap)) {
+		err = PTR_ERR(aux->cec_adap);
+		aux->cec_adap = NULL;
+		return err;
+	}
+	err = cec_register_adapter(aux->cec_adap, parent);
+	if (err) {
+		cec_delete_adapter(aux->cec_adap);
+		aux->cec_adap = NULL;
+	}
+	return err;
+}
+EXPORT_SYMBOL(drm_dp_cec_configure_adapter);
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index b17476a6909c..0e236dd40b42 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -952,6 +952,8 @@ struct drm_dp_aux_msg {
 	size_t size;
 };
 
+struct cec_adapter;
+
 /**
  * struct drm_dp_aux - DisplayPort AUX channel
  * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
@@ -1010,6 +1012,10 @@ struct drm_dp_aux {
 	 * @i2c_defer_count: Counts I2C DEFERs, used for DP validation.
 	 */
 	unsigned i2c_defer_count;
+	/**
+	 * @cec_adap: the CEC adapter for CEC-Tunneling-over-AUX support.
+	 */
+	struct cec_adapter *cec_adap;
 };
 
 ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
@@ -1132,4 +1138,22 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc, enum drm_dp_quirk quirk)
 	return desc->quirks & BIT(quirk);
 }
 
+#ifdef CONFIG_DRM_DP_CEC
+bool drm_dp_cec_irq(struct drm_dp_aux *aux);
+int drm_dp_cec_configure_adapter(struct drm_dp_aux *aux, const char *name,
+				 struct device *parent);
+#else
+static inline bool drm_dp_cec_irq(struct drm_dp_aux *aux)
+{
+	return false;
+}
+
+static inline int drm_dp_cec_configure_adapter(struct drm_dp_aux *aux,
+					       const char *name,
+					       struct device *parent)
+{
+	return -ENODEV;
+}
+#endif
+
 #endif /* _DRM_DP_HELPER_H_ */
-- 
2.14.1

[PATCHv3 2/3] drm-kms-helpers.rst: document the DP CEC helpers

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Document the Display Port CEC helper functions.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 Documentation/gpu/drm-kms-helpers.rst | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
index 7c5e2549a58a..0d2fa879edd1 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -175,6 +175,15 @@ Display Port Helper Functions Reference
 .. kernel-doc:: drivers/gpu/drm/drm_dp_helper.c
    :export:
 
+Display Port CEC Helper Functions Reference
+===========================================
+
+.. kernel-doc:: drivers/gpu/drm/drm_dp_cec.c
+   :doc: dp cec helpers
+
+.. kernel-doc:: drivers/gpu/drm/drm_dp_cec.c
+   :export:
+
 Display Port Dual Mode Adaptor Helper Functions Reference
 =========================================================
 
-- 
2.14.1

[PATCHv3 3/3] drm/i915: add DisplayPort CEC-Tunneling-over-AUX support

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Implement support for this DisplayPort feature.

The cec device is created whenever it detects an adapter that
has this feature. It is only removed when a new adapter is connected
that does not support this. If a new adapter is connected that has
different properties than the previous one, then the old cec device is
unregistered and a new one is registered to replace the old one.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Tested-by: Carlos Santa <carlos.santa@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 64fa774c855b..fdb853d2c458 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -32,6 +32,7 @@
 #include <linux/notifier.h>
 #include <linux/reboot.h>
 #include <asm/byteorder.h>
+#include <media/cec.h>
 #include <drm/drmP.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc.h>
@@ -1449,6 +1450,7 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
 static void
 intel_dp_aux_fini(struct intel_dp *intel_dp)
 {
+	cec_unregister_adapter(intel_dp->aux.cec_adap);
 	kfree(intel_dp->aux.name);
 }
 
@@ -4587,6 +4589,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
 	intel_connector->detect_edid = edid;
 
 	intel_dp->has_audio = drm_detect_monitor_audio(edid);
+	cec_s_phys_addr_from_edid(intel_dp->aux.cec_adap, edid);
 }
 
 static void
@@ -4596,6 +4599,7 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
 
 	kfree(intel_connector->detect_edid);
 	intel_connector->detect_edid = NULL;
+	cec_phys_addr_invalidate(intel_dp->aux.cec_adap);
 
 	intel_dp->has_audio = false;
 }
@@ -4616,13 +4620,17 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	intel_display_power_get(to_i915(dev), intel_dp->aux_power_domain);
 
 	/* Can't disconnect eDP, but you can close the lid... */
-	if (is_edp(intel_dp))
+	if (is_edp(intel_dp)) {
 		status = edp_detect(intel_dp);
-	else if (intel_digital_port_connected(to_i915(dev),
-					      dp_to_dig_port(intel_dp)))
+	} else if (intel_digital_port_connected(to_i915(dev),
+						dp_to_dig_port(intel_dp))) {
 		status = intel_dp_detect_dpcd(intel_dp);
-	else
+		if (status == connector_status_connected)
+			drm_dp_cec_configure_adapter(&intel_dp->aux,
+				     intel_dp->aux.name, dev->dev);
+	} else {
 		status = connector_status_disconnected;
+	}
 
 	if (status == connector_status_disconnected) {
 		memset(&intel_dp->compliance, 0, sizeof(intel_dp->compliance));
@@ -5011,6 +5019,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 
 	intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
 
+	drm_dp_cec_irq(&intel_dp->aux);
+
 	if (intel_dp->is_mst) {
 		if (intel_dp_check_mst_status(intel_dp) == -EINVAL) {
 			/*
-- 
2.14.1

[PATCHv3 3/3] drm/i915: add DisplayPort CEC-Tunneling-over-AUX support

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Implement support for this DisplayPort feature.

The cec device is created whenever it detects an adapter that
has this feature. It is only removed when a new adapter is connected
that does not support this. If a new adapter is connected that has
different properties than the previous one, then the old cec device is
unregistered and a new one is registered to replace the old one.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Tested-by: Carlos Santa <carlos.santa@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 64fa774c855b..fdb853d2c458 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -32,6 +32,7 @@
 #include <linux/notifier.h>
 #include <linux/reboot.h>
 #include <asm/byteorder.h>
+#include <media/cec.h>
 #include <drm/drmP.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc.h>
@@ -1449,6 +1450,7 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
 static void
 intel_dp_aux_fini(struct intel_dp *intel_dp)
 {
+	cec_unregister_adapter(intel_dp->aux.cec_adap);
 	kfree(intel_dp->aux.name);
 }
 
@@ -4587,6 +4589,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
 	intel_connector->detect_edid = edid;
 
 	intel_dp->has_audio = drm_detect_monitor_audio(edid);
+	cec_s_phys_addr_from_edid(intel_dp->aux.cec_adap, edid);
 }
 
 static void
@@ -4596,6 +4599,7 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
 
 	kfree(intel_connector->detect_edid);
 	intel_connector->detect_edid = NULL;
+	cec_phys_addr_invalidate(intel_dp->aux.cec_adap);
 
 	intel_dp->has_audio = false;
 }
@@ -4616,13 +4620,17 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	intel_display_power_get(to_i915(dev), intel_dp->aux_power_domain);
 
 	/* Can't disconnect eDP, but you can close the lid... */
-	if (is_edp(intel_dp))
+	if (is_edp(intel_dp)) {
 		status = edp_detect(intel_dp);
-	else if (intel_digital_port_connected(to_i915(dev),
-					      dp_to_dig_port(intel_dp)))
+	} else if (intel_digital_port_connected(to_i915(dev),
+						dp_to_dig_port(intel_dp))) {
 		status = intel_dp_detect_dpcd(intel_dp);
-	else
+		if (status == connector_status_connected)
+			drm_dp_cec_configure_adapter(&intel_dp->aux,
+				     intel_dp->aux.name, dev->dev);
+	} else {
 		status = connector_status_disconnected;
+	}
 
 	if (status == connector_status_disconnected) {
 		memset(&intel_dp->compliance, 0, sizeof(intel_dp->compliance));
@@ -5011,6 +5019,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 
 	intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
 
+	drm_dp_cec_irq(&intel_dp->aux);
+
 	if (intel_dp->is_mst) {
 		if (intel_dp_check_mst_status(intel_dp) == -EINVAL) {
 			/*
-- 
2.14.1

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

Re: [PATCHv3 1/3] drm: add support for DisplayPort CEC-Tunneling-over-AUX

[Ville Syrjälä]

On Mon, Sep 11, 2017 at 01:25:45PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> This adds support for the DisplayPort CEC-Tunneling-over-AUX
> feature that is part of the DisplayPort 1.3 standard.
> 
> Unfortunately, not all DisplayPort/USB-C to HDMI adapters with a
> chip that has this capability actually hook up the CEC pin, so
> even though a CEC device is created, it may not actually work.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Tested-by: Carlos Santa <carlos.santa@intel.com>
> ---
>  drivers/gpu/drm/Kconfig      |  10 ++
>  drivers/gpu/drm/Makefile     |   1 +
>  drivers/gpu/drm/drm_dp_cec.c | 301 +++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_dp_helper.h  |  24 ++++
>  4 files changed, 336 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_dp_cec.c
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 83cb2a88c204..1f2708df5c4e 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -120,6 +120,16 @@ config DRM_LOAD_EDID_FIRMWARE
>  	  default case is N. Details and instructions how to build your own
>  	  EDID data are given in Documentation/EDID/HOWTO.txt.
>  
> +config DRM_DP_CEC
> +	bool "Enable DisplayPort CEC-Tunneling-over-AUX HDMI support"
> +	select CEC_CORE
> +	help
> +	  Choose this option if you want to enable HDMI CEC support for
> +	  DisplayPort/USB-C to HDMI adapters.
> +
> +	  Note: not all adapters support this feature, and even for those
> +	  that do support this they often do not hook up the CEC pin.
> +
>  config DRM_TTM
>  	tristate
>  	depends on DRM && MMU
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 24a066e1841c..c6552c62049e 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -40,6 +40,7 @@ drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
>  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
>  drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
> +drm_kms_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o
>  
>  obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
>  obj-$(CONFIG_DRM_DEBUG_MM_SELFTEST) += selftests/
> diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> new file mode 100644
> index 000000000000..b831bb72c932
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_dp_cec.c
> @@ -0,0 +1,301 @@
> +/*
> + * DisplayPort CEC-Tunneling-over-AUX support
> + *
> + * Copyright 2017 Cisco Systems, Inc. and/or its affiliates. All rights reserved.
> + *
> + * This program is free software; you may redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * 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 AUTHORS OR COPYRIGHT HOLDERS
> + * 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/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <drm/drm_dp_helper.h>
> +#include <media/cec.h>
> +
> +/*
> + * Unfortunately it turns out that we have a chicken-and-egg situation
> + * here. Quite a few active (mini-)DP-to-HDMI or USB-C-to-HDMI adapters
> + * have a converter chip that supports CEC-Tunneling-over-AUX (usually the
> + * Parade PS176), but they do not wire up the CEC pin, thus making CEC
> + * useless.
> + *
> + * Sadly there is no way for this driver to know this. What happens is
> + * that a /dev/cecX device is created that is isolated and unable to see
> + * any of the other CEC devices. Quite literally the CEC wire is cut
> + * (or in this case, never connected in the first place).
> + *
> + * I suspect that the reason so few adapters support this is that this
> + * tunneling protocol was never supported by any OS. So there was no
> + * easy way of testing it, and no incentive to correctly wire up the
> + * CEC pin.
> + *
> + * Hopefully by creating this driver it will be easier for vendors to
> + * finally fix their adapters and test the CEC functionality.
> + *
> + * I keep a list of known working adapters here:
> + *
> + * https://hverkuil.home.xs4all.nl/cec-status.txt
> + *
> + * Please mail me (hverkuil@xs4all.nl) if you find an adapter that works
> + * and is not yet listed there.
> + */
> +
> +/**
> + * DOC: dp cec helpers
> + *
> + * These functions take care of supporting the CEC-Tunneling-over-AUX
> + * feature of DisplayPort-to-HDMI adapters.
> + */
> +
> +static int drm_dp_cec_adap_enable(struct cec_adapter *adap, bool enable)
> +{
> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
> +	u32 val = enable ? DP_CEC_TUNNELING_ENABLE : 0;
> +	ssize_t err = 0;
> +
> +	err = drm_dp_dpcd_writeb(aux, DP_CEC_TUNNELING_CONTROL, val);
> +	return (enable && err < 0) ? err : 0;
> +}
> +
> +static int drm_dp_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
> +{
> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
> +	/* Bit 15 (logical address 15) should always be set */
> +	u16 la_mask = 1 << CEC_LOG_ADDR_BROADCAST;
> +	u8 mask[2];
> +	ssize_t err;
> +
> +	if (addr != CEC_LOG_ADDR_INVALID)
> +		la_mask |= adap->log_addrs.log_addr_mask | (1 << addr);
> +	mask[0] = la_mask & 0xff;
> +	mask[1] = la_mask >> 8;
> +	err = drm_dp_dpcd_write(aux, DP_CEC_LOGICAL_ADDRESS_MASK, mask, 2);
> +	return (addr != CEC_LOG_ADDR_INVALID && err < 0) ? err : 0;
> +}
> +
> +static int drm_dp_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> +				    u32 signal_free_time, struct cec_msg *msg)
> +{
> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
> +	unsigned int retries = min(5, attempts - 1);
> +	ssize_t err;
> +
> +	err = drm_dp_dpcd_write(aux, DP_CEC_TX_MESSAGE_BUFFER,
> +				msg->msg, msg->len);
> +	if (err < 0)
> +		return err;

What happens if we managed to write the data only partially?

Looking at our code, I *think* we can't actally return 0 ever, so the 
_writeb() variant seem safe with the <0 checks. But the full _write()
could certainly return something between 1 and the total size.

> +
> +	err = drm_dp_dpcd_writeb(aux, DP_CEC_TX_MESSAGE_INFO,
> +				 (msg->len - 1) | (retries << 4) |
> +				 DP_CEC_TX_MESSAGE_SEND);
> +	return err < 0 ? err : 0;
> +}
> +
> +static int drm_dp_cec_adap_monitor_all_enable(struct cec_adapter *adap,
> +					      bool enable)
> +{
> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
> +	ssize_t err;
> +	u8 val;
> +
> +	if (!(adap->capabilities & CEC_CAP_MONITOR_ALL))
> +		return 0;
> +
> +	err = drm_dp_dpcd_readb(aux, DP_CEC_TUNNELING_CONTROL, &val);
> +	if (err >= 0) {
> +		if (enable)
> +			val |= DP_CEC_SNOOPING_ENABLE;
> +		else
> +			val &= ~DP_CEC_SNOOPING_ENABLE;
> +		err = drm_dp_dpcd_writeb(aux, DP_CEC_TUNNELING_CONTROL, val);
> +	}
> +	return (enable && err < 0) ? err : 0;
> +}
> +
> +static void drm_dp_cec_adap_status(struct cec_adapter *adap,
> +				   struct seq_file *file)
> +{
> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
> +	u8 buf[DP_AUX_MAX_PAYLOAD_BYTES];
> +	u8 hw_rev;
> +
> +	if (drm_dp_dpcd_read(aux, DP_BRANCH_OUI,
> +			     buf, sizeof(buf)) != sizeof(buf))
> +		return;
> +	hw_rev = buf[9];
> +	buf[9] = 0;
> +	seq_printf(file, "OUI: %02x-%02x-%02x\n",
> +		   buf[0], buf[1], buf[2]);
> +	seq_printf(file, "ID: %s\n", buf + 3);
> +	seq_printf(file, "HW Rev: %d.%d\n", hw_rev >> 4, hw_rev & 0xf);
> +	/*
> +	 * Show this both in decimal and hex: at least one vendor
> +	 * always reports this in hex.
> +	 */
> +	seq_printf(file, "FW/SW Rev: %d.%d (0x%02x.0x%02x)\n",
> +		   buf[10], buf[11], buf[10], buf[11]);

I believe struct drm_dp_dpcd_ident provides a slightly easier way to
parse these registers.

> +}
> +
> +static const struct cec_adap_ops drm_dp_cec_adap_ops = {
> +	.adap_enable = drm_dp_cec_adap_enable,
> +	.adap_log_addr = drm_dp_cec_adap_log_addr,
> +	.adap_transmit = drm_dp_cec_adap_transmit,
> +	.adap_monitor_all_enable = drm_dp_cec_adap_monitor_all_enable,
> +	.adap_status = drm_dp_cec_adap_status,
> +};
> +
> +static int drm_dp_cec_received(struct drm_dp_aux *aux)
> +{
> +	struct cec_adapter *adap = aux->cec_adap;
> +	struct cec_msg msg;
> +	u8 rx_msg_info;
> +	ssize_t err;
> +
> +	err = drm_dp_dpcd_readb(aux, DP_CEC_RX_MESSAGE_INFO, &rx_msg_info);
> +	if (err < 0)
> +		return err;
> +	if (!(rx_msg_info & DP_CEC_RX_MESSAGE_ENDED))
> +		return 0;
> +	msg.len = (rx_msg_info & DP_CEC_RX_MESSAGE_LEN_MASK) + 1;
> +	err = drm_dp_dpcd_read(aux, DP_CEC_RX_MESSAGE_BUFFER, msg.msg, msg.len);
> +	if (err < 0)
> +		return err;
> +	cec_received_msg(adap, &msg);
> +	return 0;

nit: The code in general feels a bit crowded. Could use a few more
empty lines IMO.

> +}
> +
> +static int drm_dp_cec_handle_irq(struct drm_dp_aux *aux)
> +{
> +	struct cec_adapter *adap = aux->cec_adap;
> +	u8 flags;
> +	ssize_t err;
> +
> +	err = drm_dp_dpcd_readb(aux, DP_CEC_TUNNELING_IRQ_FLAGS, &flags);
> +	if (err < 0)
> +		return err;
> +
> +	if (flags & DP_CEC_RX_MESSAGE_INFO_VALID)
> +		drm_dp_cec_received(aux);
> +
> +	if (flags & DP_CEC_TX_MESSAGE_SENT)
> +		cec_transmit_attempt_done(adap, CEC_TX_STATUS_OK);
> +	else if (flags & DP_CEC_TX_LINE_ERROR)
> +		cec_transmit_attempt_done(adap, CEC_TX_STATUS_ERROR |
> +						CEC_TX_STATUS_MAX_RETRIES);
> +	else if (flags &
> +		 (DP_CEC_TX_ADDRESS_NACK_ERROR | DP_CEC_TX_DATA_NACK_ERROR))
> +		cec_transmit_attempt_done(adap, CEC_TX_STATUS_NACK |
> +						CEC_TX_STATUS_MAX_RETRIES);
> +	drm_dp_dpcd_writeb(aux, DP_CEC_TUNNELING_IRQ_FLAGS, flags);
> +	return 0;
> +}
> +
> +/**
> + * drm_dp_cec_irq() - handle CEC interrupt, if any
> + * @aux: DisplayPort AUX channel
> + *
> + * Should be called when handling an IRQ_HPD request. If CEC-tunneling-over-AUX
> + * is present, then it will check for a CEC_IRQ and handle it accordingly.
> + *
> + * Returns true if an interrupt was handled successfully or false otherwise.
> + */
> +bool drm_dp_cec_irq(struct drm_dp_aux *aux)
> +{
> +	bool handled = false;
> +	int attempts;
> +
> +	if (!aux->cec_adap)
> +		return false;
> +
> +	for (attempts = 0; attempts < 4; attempts++) {

What's the deal with this loop?

> +		u8 cec_irq;
> +		int ret;
> +
> +		ret = drm_dp_dpcd_readb(aux, DP_DEVICE_SERVICE_IRQ_VECTOR_ESI1,
> +					&cec_irq);

I believe ESI only exists for DPCD 1.2+. Are we protected from reaching
this on earlier devices? Hmm. I guess the cec_adap check should protect us.

Supposedly DPCD should just return zeroes for nonexisting registers, but
I've seen at least one device that failed on that front. In that
particular case there were just a handful of bytes in the entire DPCD
address space that couldn't be read succesfully.

I don't entirely like this function doing irq read/ack part. There
really should be some kind of central sink irq dispatch thingy, but
since we don't have that currently I think this is an OK approach.
At some point I did start on trying to clean up the sink irq handling
mess, but I didn't get very far with it before I had to move on to
something else.

> +		if (ret < 0 || !(cec_irq & DP_CEC_IRQ))
> +			break;
> +
> +		if (!drm_dp_cec_handle_irq(aux))
> +			handled = true;
> +
> +		ret = drm_dp_dpcd_writeb(aux, DP_DEVICE_SERVICE_IRQ_VECTOR_ESI1,
> +					 DP_CEC_IRQ);
> +		if (ret < 0)
> +			break;
> +	}
> +	return handled;
> +}
> +EXPORT_SYMBOL(drm_dp_cec_irq);
> +
> +/**
> + * drm_dp_cec_configure_adapter() - configure the CEC adapter
> + * @aux: DisplayPort AUX channel
> + * @name: name of the CEC device
> + * @parent: parent device
> + *
> + * Checks if this is a DisplayPort-to-HDMI adapter that supports
> + * CEC-tunneling-over-AUX, and if so it creates a CEC device.
> + *
> + * If a CEC device was already created, then check if the capabilities
> + * have changed. If not, then do nothing. Otherwise destroy the old
> + * CEC device and create a new CEC device.
> + *
> + * This can happen when one DP-to-HDMI adapter is disconnected and
> + * replaced by another adapter with different CEC capabilities.
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int drm_dp_cec_configure_adapter(struct drm_dp_aux *aux, const char *name,
> +				 struct device *parent)
> +{
> +	u32 cec_caps = CEC_CAP_DEFAULTS | CEC_CAP_NEEDS_HPD;
> +	unsigned int num_las = 1;
> +	int err;
> +	u8 cap;
> +
> +	if (drm_dp_dpcd_readb(aux, DP_CEC_TUNNELING_CAPABILITY, &cap) != 1 ||
> +	    !(cap & DP_CEC_TUNNELING_CAPABLE)) {
> +		cec_unregister_adapter(aux->cec_adap);
> +		aux->cec_adap = NULL;
> +		return -ENODEV;
> +	}
> +
> +	if (cap & DP_CEC_SNOOPING_CAPABLE)
> +		cec_caps |= CEC_CAP_MONITOR_ALL;
> +	if (cap & DP_CEC_MULTIPLE_LA_CAPABLE)
> +		num_las = CEC_MAX_LOG_ADDRS;
> +
> +	if (aux->cec_adap) {
> +		if (aux->cec_adap->capabilities == cec_caps &&
> +		    aux->cec_adap->available_log_addrs == num_las)
> +			return 0;
> +		cec_unregister_adapter(aux->cec_adap);
> +	}
> +
> +	aux->cec_adap = cec_allocate_adapter(&drm_dp_cec_adap_ops,
> +			 aux, name, cec_caps, num_las);
> +	if (IS_ERR(aux->cec_adap)) {
> +		err = PTR_ERR(aux->cec_adap);
> +		aux->cec_adap = NULL;
> +		return err;
> +	}
> +	err = cec_register_adapter(aux->cec_adap, parent);
> +	if (err) {
> +		cec_delete_adapter(aux->cec_adap);
> +		aux->cec_adap = NULL;
> +	}
> +	return err;
> +}
> +EXPORT_SYMBOL(drm_dp_cec_configure_adapter);
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index b17476a6909c..0e236dd40b42 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -952,6 +952,8 @@ struct drm_dp_aux_msg {
>  	size_t size;
>  };
>  
> +struct cec_adapter;
> +
>  /**
>   * struct drm_dp_aux - DisplayPort AUX channel
>   * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
> @@ -1010,6 +1012,10 @@ struct drm_dp_aux {
>  	 * @i2c_defer_count: Counts I2C DEFERs, used for DP validation.
>  	 */
>  	unsigned i2c_defer_count;
> +	/**
> +	 * @cec_adap: the CEC adapter for CEC-Tunneling-over-AUX support.
> +	 */
> +	struct cec_adapter *cec_adap;
>  };
>  
>  ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
> @@ -1132,4 +1138,22 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc, enum drm_dp_quirk quirk)
>  	return desc->quirks & BIT(quirk);
>  }
>  
> +#ifdef CONFIG_DRM_DP_CEC
> +bool drm_dp_cec_irq(struct drm_dp_aux *aux);
> +int drm_dp_cec_configure_adapter(struct drm_dp_aux *aux, const char *name,
> +				 struct device *parent);
> +#else
> +static inline bool drm_dp_cec_irq(struct drm_dp_aux *aux)
> +{
> +	return false;
> +}
> +
> +static inline int drm_dp_cec_configure_adapter(struct drm_dp_aux *aux,
> +					       const char *name,
> +					       struct device *parent)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +
>  #endif /* _DRM_DP_HELPER_H_ */
> -- 
> 2.14.1

-- 
Ville Syrjälä
Intel OTC

Re: [PATCHv3 1/3] drm: add support for DisplayPort CEC-Tunneling-over-AUX

[Hans Verkuil]

On 09/12/2017 07:39 PM, Ville Syrjälä wrote:
> On Mon, Sep 11, 2017 at 01:25:45PM +0200, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> This adds support for the DisplayPort CEC-Tunneling-over-AUX
>> feature that is part of the DisplayPort 1.3 standard.
>>
>> Unfortunately, not all DisplayPort/USB-C to HDMI adapters with a
>> chip that has this capability actually hook up the CEC pin, so
>> even though a CEC device is created, it may not actually work.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> Tested-by: Carlos Santa <carlos.santa@intel.com>
>> ---
>>  drivers/gpu/drm/Kconfig      |  10 ++
>>  drivers/gpu/drm/Makefile     |   1 +
>>  drivers/gpu/drm/drm_dp_cec.c | 301 +++++++++++++++++++++++++++++++++++++++++++
>>  include/drm/drm_dp_helper.h  |  24 ++++
>>  4 files changed, 336 insertions(+)
>>  create mode 100644 drivers/gpu/drm/drm_dp_cec.c
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index 83cb2a88c204..1f2708df5c4e 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -120,6 +120,16 @@ config DRM_LOAD_EDID_FIRMWARE
>>  	  default case is N. Details and instructions how to build your own
>>  	  EDID data are given in Documentation/EDID/HOWTO.txt.
>>  
>> +config DRM_DP_CEC
>> +	bool "Enable DisplayPort CEC-Tunneling-over-AUX HDMI support"
>> +	select CEC_CORE
>> +	help
>> +	  Choose this option if you want to enable HDMI CEC support for
>> +	  DisplayPort/USB-C to HDMI adapters.
>> +
>> +	  Note: not all adapters support this feature, and even for those
>> +	  that do support this they often do not hook up the CEC pin.
>> +
>>  config DRM_TTM
>>  	tristate
>>  	depends on DRM && MMU
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index 24a066e1841c..c6552c62049e 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -40,6 +40,7 @@ drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>>  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
>>  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
>>  drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
>> +drm_kms_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o
>>  
>>  obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
>>  obj-$(CONFIG_DRM_DEBUG_MM_SELFTEST) += selftests/
>> diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
>> new file mode 100644
>> index 000000000000..b831bb72c932
>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_dp_cec.c
>> @@ -0,0 +1,301 @@
>> +/*
>> + * DisplayPort CEC-Tunneling-over-AUX support
>> + *
>> + * Copyright 2017 Cisco Systems, Inc. and/or its affiliates. All rights reserved.
>> + *
>> + * This program is free software; you may redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; version 2 of the License.
>> + *
>> + * 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 AUTHORS OR COPYRIGHT HOLDERS
>> + * 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/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <drm/drm_dp_helper.h>
>> +#include <media/cec.h>
>> +
>> +/*
>> + * Unfortunately it turns out that we have a chicken-and-egg situation
>> + * here. Quite a few active (mini-)DP-to-HDMI or USB-C-to-HDMI adapters
>> + * have a converter chip that supports CEC-Tunneling-over-AUX (usually the
>> + * Parade PS176), but they do not wire up the CEC pin, thus making CEC
>> + * useless.
>> + *
>> + * Sadly there is no way for this driver to know this. What happens is
>> + * that a /dev/cecX device is created that is isolated and unable to see
>> + * any of the other CEC devices. Quite literally the CEC wire is cut
>> + * (or in this case, never connected in the first place).
>> + *
>> + * I suspect that the reason so few adapters support this is that this
>> + * tunneling protocol was never supported by any OS. So there was no
>> + * easy way of testing it, and no incentive to correctly wire up the
>> + * CEC pin.
>> + *
>> + * Hopefully by creating this driver it will be easier for vendors to
>> + * finally fix their adapters and test the CEC functionality.
>> + *
>> + * I keep a list of known working adapters here:
>> + *
>> + * https://hverkuil.home.xs4all.nl/cec-status.txt
>> + *
>> + * Please mail me (hverkuil@xs4all.nl) if you find an adapter that works
>> + * and is not yet listed there.
>> + */
>> +
>> +/**
>> + * DOC: dp cec helpers
>> + *
>> + * These functions take care of supporting the CEC-Tunneling-over-AUX
>> + * feature of DisplayPort-to-HDMI adapters.
>> + */
>> +
>> +static int drm_dp_cec_adap_enable(struct cec_adapter *adap, bool enable)
>> +{
>> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
>> +	u32 val = enable ? DP_CEC_TUNNELING_ENABLE : 0;
>> +	ssize_t err = 0;
>> +
>> +	err = drm_dp_dpcd_writeb(aux, DP_CEC_TUNNELING_CONTROL, val);
>> +	return (enable && err < 0) ? err : 0;
>> +}
>> +
>> +static int drm_dp_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
>> +{
>> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
>> +	/* Bit 15 (logical address 15) should always be set */
>> +	u16 la_mask = 1 << CEC_LOG_ADDR_BROADCAST;
>> +	u8 mask[2];
>> +	ssize_t err;
>> +
>> +	if (addr != CEC_LOG_ADDR_INVALID)
>> +		la_mask |= adap->log_addrs.log_addr_mask | (1 << addr);
>> +	mask[0] = la_mask & 0xff;
>> +	mask[1] = la_mask >> 8;
>> +	err = drm_dp_dpcd_write(aux, DP_CEC_LOGICAL_ADDRESS_MASK, mask, 2);
>> +	return (addr != CEC_LOG_ADDR_INVALID && err < 0) ? err : 0;
>> +}
>> +
>> +static int drm_dp_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
>> +				    u32 signal_free_time, struct cec_msg *msg)
>> +{
>> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
>> +	unsigned int retries = min(5, attempts - 1);
>> +	ssize_t err;
>> +
>> +	err = drm_dp_dpcd_write(aux, DP_CEC_TX_MESSAGE_BUFFER,
>> +				msg->msg, msg->len);
>> +	if (err < 0)
>> +		return err;
> 
> What happens if we managed to write the data only partially?
> 
> Looking at our code, I *think* we can't actally return 0 ever, so the 
> _writeb() variant seem safe with the <0 checks. But the full _write()
> could certainly return something between 1 and the total size.

Not according to the drm_dp_dpcd_write() documentation:

"If not all bytes were transferred, this function returns -EPROTO."

Looking at the implementation, that is exactly what happens.

> 
>> +
>> +	err = drm_dp_dpcd_writeb(aux, DP_CEC_TX_MESSAGE_INFO,
>> +				 (msg->len - 1) | (retries << 4) |
>> +				 DP_CEC_TX_MESSAGE_SEND);
>> +	return err < 0 ? err : 0;
>> +}
>> +
>> +static int drm_dp_cec_adap_monitor_all_enable(struct cec_adapter *adap,
>> +					      bool enable)
>> +{
>> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
>> +	ssize_t err;
>> +	u8 val;
>> +
>> +	if (!(adap->capabilities & CEC_CAP_MONITOR_ALL))
>> +		return 0;
>> +
>> +	err = drm_dp_dpcd_readb(aux, DP_CEC_TUNNELING_CONTROL, &val);
>> +	if (err >= 0) {
>> +		if (enable)
>> +			val |= DP_CEC_SNOOPING_ENABLE;
>> +		else
>> +			val &= ~DP_CEC_SNOOPING_ENABLE;
>> +		err = drm_dp_dpcd_writeb(aux, DP_CEC_TUNNELING_CONTROL, val);
>> +	}
>> +	return (enable && err < 0) ? err : 0;
>> +}
>> +
>> +static void drm_dp_cec_adap_status(struct cec_adapter *adap,
>> +				   struct seq_file *file)
>> +{
>> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
>> +	u8 buf[DP_AUX_MAX_PAYLOAD_BYTES];
>> +	u8 hw_rev;
>> +
>> +	if (drm_dp_dpcd_read(aux, DP_BRANCH_OUI,
>> +			     buf, sizeof(buf)) != sizeof(buf))
>> +		return;
>> +	hw_rev = buf[9];
>> +	buf[9] = 0;
>> +	seq_printf(file, "OUI: %02x-%02x-%02x\n",
>> +		   buf[0], buf[1], buf[2]);
>> +	seq_printf(file, "ID: %s\n", buf + 3);
>> +	seq_printf(file, "HW Rev: %d.%d\n", hw_rev >> 4, hw_rev & 0xf);
>> +	/*
>> +	 * Show this both in decimal and hex: at least one vendor
>> +	 * always reports this in hex.
>> +	 */
>> +	seq_printf(file, "FW/SW Rev: %d.%d (0x%02x.0x%02x)\n",
>> +		   buf[10], buf[11], buf[10], buf[11]);
> 
> I believe struct drm_dp_dpcd_ident provides a slightly easier way to
> parse these registers.

I'll take a look at that.

> 
>> +}
>> +
>> +static const struct cec_adap_ops drm_dp_cec_adap_ops = {
>> +	.adap_enable = drm_dp_cec_adap_enable,
>> +	.adap_log_addr = drm_dp_cec_adap_log_addr,
>> +	.adap_transmit = drm_dp_cec_adap_transmit,
>> +	.adap_monitor_all_enable = drm_dp_cec_adap_monitor_all_enable,
>> +	.adap_status = drm_dp_cec_adap_status,
>> +};
>> +
>> +static int drm_dp_cec_received(struct drm_dp_aux *aux)
>> +{
>> +	struct cec_adapter *adap = aux->cec_adap;
>> +	struct cec_msg msg;
>> +	u8 rx_msg_info;
>> +	ssize_t err;
>> +
>> +	err = drm_dp_dpcd_readb(aux, DP_CEC_RX_MESSAGE_INFO, &rx_msg_info);
>> +	if (err < 0)
>> +		return err;
>> +	if (!(rx_msg_info & DP_CEC_RX_MESSAGE_ENDED))
>> +		return 0;
>> +	msg.len = (rx_msg_info & DP_CEC_RX_MESSAGE_LEN_MASK) + 1;
>> +	err = drm_dp_dpcd_read(aux, DP_CEC_RX_MESSAGE_BUFFER, msg.msg, msg.len);
>> +	if (err < 0)
>> +		return err;
>> +	cec_received_msg(adap, &msg);
>> +	return 0;
> 
> nit: The code in general feels a bit crowded. Could use a few more
> empty lines IMO.

Will do.

> 
>> +}
>> +
>> +static int drm_dp_cec_handle_irq(struct drm_dp_aux *aux)
>> +{
>> +	struct cec_adapter *adap = aux->cec_adap;
>> +	u8 flags;
>> +	ssize_t err;
>> +
>> +	err = drm_dp_dpcd_readb(aux, DP_CEC_TUNNELING_IRQ_FLAGS, &flags);
>> +	if (err < 0)
>> +		return err;
>> +
>> +	if (flags & DP_CEC_RX_MESSAGE_INFO_VALID)
>> +		drm_dp_cec_received(aux);
>> +
>> +	if (flags & DP_CEC_TX_MESSAGE_SENT)
>> +		cec_transmit_attempt_done(adap, CEC_TX_STATUS_OK);
>> +	else if (flags & DP_CEC_TX_LINE_ERROR)
>> +		cec_transmit_attempt_done(adap, CEC_TX_STATUS_ERROR |
>> +						CEC_TX_STATUS_MAX_RETRIES);
>> +	else if (flags &
>> +		 (DP_CEC_TX_ADDRESS_NACK_ERROR | DP_CEC_TX_DATA_NACK_ERROR))
>> +		cec_transmit_attempt_done(adap, CEC_TX_STATUS_NACK |
>> +						CEC_TX_STATUS_MAX_RETRIES);
>> +	drm_dp_dpcd_writeb(aux, DP_CEC_TUNNELING_IRQ_FLAGS, flags);
>> +	return 0;
>> +}
>> +
>> +/**
>> + * drm_dp_cec_irq() - handle CEC interrupt, if any
>> + * @aux: DisplayPort AUX channel
>> + *
>> + * Should be called when handling an IRQ_HPD request. If CEC-tunneling-over-AUX
>> + * is present, then it will check for a CEC_IRQ and handle it accordingly.
>> + *
>> + * Returns true if an interrupt was handled successfully or false otherwise.
>> + */
>> +bool drm_dp_cec_irq(struct drm_dp_aux *aux)
>> +{
>> +	bool handled = false;
>> +	int attempts;
>> +
>> +	if (!aux->cec_adap)
>> +		return false;
>> +
>> +	for (attempts = 0; attempts < 4; attempts++) {
> 
> What's the deal with this loop?

It was copied from what intel_dp_check_mst_status() does.

However, I think it can be removed since drm_dp_dpcd_access already retries
for up to 32 times.

Does that make sense?

> 
>> +		u8 cec_irq;
>> +		int ret;
>> +
>> +		ret = drm_dp_dpcd_readb(aux, DP_DEVICE_SERVICE_IRQ_VECTOR_ESI1,
>> +					&cec_irq);
> 
> I believe ESI only exists for DPCD 1.2+. Are we protected from reaching
> this on earlier devices? Hmm. I guess the cec_adap check should protect us.
> 
> Supposedly DPCD should just return zeroes for nonexisting registers, but
> I've seen at least one device that failed on that front. In that
> particular case there were just a handful of bytes in the entire DPCD
> address space that couldn't be read succesfully.

I'm not sure if I can do anything to make this more robust. If you have
any suggestions, then I'm happy to try and implement those.

> 
> I don't entirely like this function doing irq read/ack part. There
> really should be some kind of central sink irq dispatch thingy, but
> since we don't have that currently I think this is an OK approach.
> At some point I did start on trying to clean up the sink irq handling
> mess, but I didn't get very far with it before I had to move on to
> something else.
> 
>> +		if (ret < 0 || !(cec_irq & DP_CEC_IRQ))
>> +			break;
>> +
>> +		if (!drm_dp_cec_handle_irq(aux))
>> +			handled = true;
>> +
>> +		ret = drm_dp_dpcd_writeb(aux, DP_DEVICE_SERVICE_IRQ_VECTOR_ESI1,
>> +					 DP_CEC_IRQ);
>> +		if (ret < 0)
>> +			break;
>> +	}
>> +	return handled;
>> +}
>> +EXPORT_SYMBOL(drm_dp_cec_irq);
>> +
>> +/**
>> + * drm_dp_cec_configure_adapter() - configure the CEC adapter
>> + * @aux: DisplayPort AUX channel
>> + * @name: name of the CEC device
>> + * @parent: parent device
>> + *
>> + * Checks if this is a DisplayPort-to-HDMI adapter that supports
>> + * CEC-tunneling-over-AUX, and if so it creates a CEC device.
>> + *
>> + * If a CEC device was already created, then check if the capabilities
>> + * have changed. If not, then do nothing. Otherwise destroy the old
>> + * CEC device and create a new CEC device.
>> + *
>> + * This can happen when one DP-to-HDMI adapter is disconnected and
>> + * replaced by another adapter with different CEC capabilities.
>> + *
>> + * Returns 0 on success or a negative error code on failure.
>> + */
>> +int drm_dp_cec_configure_adapter(struct drm_dp_aux *aux, const char *name,
>> +				 struct device *parent)
>> +{
>> +	u32 cec_caps = CEC_CAP_DEFAULTS | CEC_CAP_NEEDS_HPD;
>> +	unsigned int num_las = 1;
>> +	int err;
>> +	u8 cap;
>> +
>> +	if (drm_dp_dpcd_readb(aux, DP_CEC_TUNNELING_CAPABILITY, &cap) != 1 ||
>> +	    !(cap & DP_CEC_TUNNELING_CAPABLE)) {
>> +		cec_unregister_adapter(aux->cec_adap);
>> +		aux->cec_adap = NULL;
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (cap & DP_CEC_SNOOPING_CAPABLE)
>> +		cec_caps |= CEC_CAP_MONITOR_ALL;
>> +	if (cap & DP_CEC_MULTIPLE_LA_CAPABLE)
>> +		num_las = CEC_MAX_LOG_ADDRS;
>> +
>> +	if (aux->cec_adap) {
>> +		if (aux->cec_adap->capabilities == cec_caps &&
>> +		    aux->cec_adap->available_log_addrs == num_las)
>> +			return 0;
>> +		cec_unregister_adapter(aux->cec_adap);
>> +	}
>> +
>> +	aux->cec_adap = cec_allocate_adapter(&drm_dp_cec_adap_ops,
>> +			 aux, name, cec_caps, num_las);
>> +	if (IS_ERR(aux->cec_adap)) {
>> +		err = PTR_ERR(aux->cec_adap);
>> +		aux->cec_adap = NULL;
>> +		return err;
>> +	}
>> +	err = cec_register_adapter(aux->cec_adap, parent);
>> +	if (err) {
>> +		cec_delete_adapter(aux->cec_adap);
>> +		aux->cec_adap = NULL;
>> +	}
>> +	return err;
>> +}
>> +EXPORT_SYMBOL(drm_dp_cec_configure_adapter);
>> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>> index b17476a6909c..0e236dd40b42 100644
>> --- a/include/drm/drm_dp_helper.h
>> +++ b/include/drm/drm_dp_helper.h
>> @@ -952,6 +952,8 @@ struct drm_dp_aux_msg {
>>  	size_t size;
>>  };
>>  
>> +struct cec_adapter;
>> +
>>  /**
>>   * struct drm_dp_aux - DisplayPort AUX channel
>>   * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
>> @@ -1010,6 +1012,10 @@ struct drm_dp_aux {
>>  	 * @i2c_defer_count: Counts I2C DEFERs, used for DP validation.
>>  	 */
>>  	unsigned i2c_defer_count;
>> +	/**
>> +	 * @cec_adap: the CEC adapter for CEC-Tunneling-over-AUX support.
>> +	 */
>> +	struct cec_adapter *cec_adap;
>>  };
>>  
>>  ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
>> @@ -1132,4 +1138,22 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc, enum drm_dp_quirk quirk)
>>  	return desc->quirks & BIT(quirk);
>>  }
>>  
>> +#ifdef CONFIG_DRM_DP_CEC
>> +bool drm_dp_cec_irq(struct drm_dp_aux *aux);
>> +int drm_dp_cec_configure_adapter(struct drm_dp_aux *aux, const char *name,
>> +				 struct device *parent);
>> +#else
>> +static inline bool drm_dp_cec_irq(struct drm_dp_aux *aux)
>> +{
>> +	return false;
>> +}
>> +
>> +static inline int drm_dp_cec_configure_adapter(struct drm_dp_aux *aux,
>> +					       const char *name,
>> +					       struct device *parent)
>> +{
>> +	return -ENODEV;
>> +}
>> +#endif
>> +
>>  #endif /* _DRM_DP_HELPER_H_ */
>> -- 
>> 2.14.1
> 

Regards,

	Hans

Re: [PATCHv3 1/3] drm: add support for DisplayPort CEC-Tunneling-over-AUX

[Hans Verkuil]

On 09/12/2017 07:39 PM, Ville Syrjälä wrote:
> On Mon, Sep 11, 2017 at 01:25:45PM +0200, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> This adds support for the DisplayPort CEC-Tunneling-over-AUX
>> feature that is part of the DisplayPort 1.3 standard.
>>
>> Unfortunately, not all DisplayPort/USB-C to HDMI adapters with a
>> chip that has this capability actually hook up the CEC pin, so
>> even though a CEC device is created, it may not actually work.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> Tested-by: Carlos Santa <carlos.santa@intel.com>
>> ---
>>  drivers/gpu/drm/Kconfig      |  10 ++
>>  drivers/gpu/drm/Makefile     |   1 +
>>  drivers/gpu/drm/drm_dp_cec.c | 301 +++++++++++++++++++++++++++++++++++++++++++
>>  include/drm/drm_dp_helper.h  |  24 ++++
>>  4 files changed, 336 insertions(+)
>>  create mode 100644 drivers/gpu/drm/drm_dp_cec.c
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index 83cb2a88c204..1f2708df5c4e 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -120,6 +120,16 @@ config DRM_LOAD_EDID_FIRMWARE
>>  	  default case is N. Details and instructions how to build your own
>>  	  EDID data are given in Documentation/EDID/HOWTO.txt.
>>  
>> +config DRM_DP_CEC
>> +	bool "Enable DisplayPort CEC-Tunneling-over-AUX HDMI support"
>> +	select CEC_CORE
>> +	help
>> +	  Choose this option if you want to enable HDMI CEC support for
>> +	  DisplayPort/USB-C to HDMI adapters.
>> +
>> +	  Note: not all adapters support this feature, and even for those
>> +	  that do support this they often do not hook up the CEC pin.
>> +
>>  config DRM_TTM
>>  	tristate
>>  	depends on DRM && MMU
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index 24a066e1841c..c6552c62049e 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -40,6 +40,7 @@ drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>>  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
>>  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
>>  drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
>> +drm_kms_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o
>>  
>>  obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
>>  obj-$(CONFIG_DRM_DEBUG_MM_SELFTEST) += selftests/
>> diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
>> new file mode 100644
>> index 000000000000..b831bb72c932
>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_dp_cec.c
>> @@ -0,0 +1,301 @@
>> +/*
>> + * DisplayPort CEC-Tunneling-over-AUX support
>> + *
>> + * Copyright 2017 Cisco Systems, Inc. and/or its affiliates. All rights reserved.
>> + *
>> + * This program is free software; you may redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; version 2 of the License.
>> + *
>> + * 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 AUTHORS OR COPYRIGHT HOLDERS
>> + * 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/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <drm/drm_dp_helper.h>
>> +#include <media/cec.h>
>> +
>> +/*
>> + * Unfortunately it turns out that we have a chicken-and-egg situation
>> + * here. Quite a few active (mini-)DP-to-HDMI or USB-C-to-HDMI adapters
>> + * have a converter chip that supports CEC-Tunneling-over-AUX (usually the
>> + * Parade PS176), but they do not wire up the CEC pin, thus making CEC
>> + * useless.
>> + *
>> + * Sadly there is no way for this driver to know this. What happens is
>> + * that a /dev/cecX device is created that is isolated and unable to see
>> + * any of the other CEC devices. Quite literally the CEC wire is cut
>> + * (or in this case, never connected in the first place).
>> + *
>> + * I suspect that the reason so few adapters support this is that this
>> + * tunneling protocol was never supported by any OS. So there was no
>> + * easy way of testing it, and no incentive to correctly wire up the
>> + * CEC pin.
>> + *
>> + * Hopefully by creating this driver it will be easier for vendors to
>> + * finally fix their adapters and test the CEC functionality.
>> + *
>> + * I keep a list of known working adapters here:
>> + *
>> + * https://hverkuil.home.xs4all.nl/cec-status.txt
>> + *
>> + * Please mail me (hverkuil@xs4all.nl) if you find an adapter that works
>> + * and is not yet listed there.
>> + */
>> +
>> +/**
>> + * DOC: dp cec helpers
>> + *
>> + * These functions take care of supporting the CEC-Tunneling-over-AUX
>> + * feature of DisplayPort-to-HDMI adapters.
>> + */
>> +
>> +static int drm_dp_cec_adap_enable(struct cec_adapter *adap, bool enable)
>> +{
>> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
>> +	u32 val = enable ? DP_CEC_TUNNELING_ENABLE : 0;
>> +	ssize_t err = 0;
>> +
>> +	err = drm_dp_dpcd_writeb(aux, DP_CEC_TUNNELING_CONTROL, val);
>> +	return (enable && err < 0) ? err : 0;
>> +}
>> +
>> +static int drm_dp_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
>> +{
>> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
>> +	/* Bit 15 (logical address 15) should always be set */
>> +	u16 la_mask = 1 << CEC_LOG_ADDR_BROADCAST;
>> +	u8 mask[2];
>> +	ssize_t err;
>> +
>> +	if (addr != CEC_LOG_ADDR_INVALID)
>> +		la_mask |= adap->log_addrs.log_addr_mask | (1 << addr);
>> +	mask[0] = la_mask & 0xff;
>> +	mask[1] = la_mask >> 8;
>> +	err = drm_dp_dpcd_write(aux, DP_CEC_LOGICAL_ADDRESS_MASK, mask, 2);
>> +	return (addr != CEC_LOG_ADDR_INVALID && err < 0) ? err : 0;
>> +}
>> +
>> +static int drm_dp_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
>> +				    u32 signal_free_time, struct cec_msg *msg)
>> +{
>> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
>> +	unsigned int retries = min(5, attempts - 1);
>> +	ssize_t err;
>> +
>> +	err = drm_dp_dpcd_write(aux, DP_CEC_TX_MESSAGE_BUFFER,
>> +				msg->msg, msg->len);
>> +	if (err < 0)
>> +		return err;
> 
> What happens if we managed to write the data only partially?
> 
> Looking at our code, I *think* we can't actally return 0 ever, so the 
> _writeb() variant seem safe with the <0 checks. But the full _write()
> could certainly return something between 1 and the total size.

Not according to the drm_dp_dpcd_write() documentation:

"If not all bytes were transferred, this function returns -EPROTO."

Looking at the implementation, that is exactly what happens.

> 
>> +
>> +	err = drm_dp_dpcd_writeb(aux, DP_CEC_TX_MESSAGE_INFO,
>> +				 (msg->len - 1) | (retries << 4) |
>> +				 DP_CEC_TX_MESSAGE_SEND);
>> +	return err < 0 ? err : 0;
>> +}
>> +
>> +static int drm_dp_cec_adap_monitor_all_enable(struct cec_adapter *adap,
>> +					      bool enable)
>> +{
>> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
>> +	ssize_t err;
>> +	u8 val;
>> +
>> +	if (!(adap->capabilities & CEC_CAP_MONITOR_ALL))
>> +		return 0;
>> +
>> +	err = drm_dp_dpcd_readb(aux, DP_CEC_TUNNELING_CONTROL, &val);
>> +	if (err >= 0) {
>> +		if (enable)
>> +			val |= DP_CEC_SNOOPING_ENABLE;
>> +		else
>> +			val &= ~DP_CEC_SNOOPING_ENABLE;
>> +		err = drm_dp_dpcd_writeb(aux, DP_CEC_TUNNELING_CONTROL, val);
>> +	}
>> +	return (enable && err < 0) ? err : 0;
>> +}
>> +
>> +static void drm_dp_cec_adap_status(struct cec_adapter *adap,
>> +				   struct seq_file *file)
>> +{
>> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
>> +	u8 buf[DP_AUX_MAX_PAYLOAD_BYTES];
>> +	u8 hw_rev;
>> +
>> +	if (drm_dp_dpcd_read(aux, DP_BRANCH_OUI,
>> +			     buf, sizeof(buf)) != sizeof(buf))
>> +		return;
>> +	hw_rev = buf[9];
>> +	buf[9] = 0;
>> +	seq_printf(file, "OUI: %02x-%02x-%02x\n",
>> +		   buf[0], buf[1], buf[2]);
>> +	seq_printf(file, "ID: %s\n", buf + 3);
>> +	seq_printf(file, "HW Rev: %d.%d\n", hw_rev >> 4, hw_rev & 0xf);
>> +	/*
>> +	 * Show this both in decimal and hex: at least one vendor
>> +	 * always reports this in hex.
>> +	 */
>> +	seq_printf(file, "FW/SW Rev: %d.%d (0x%02x.0x%02x)\n",
>> +		   buf[10], buf[11], buf[10], buf[11]);
> 
> I believe struct drm_dp_dpcd_ident provides a slightly easier way to
> parse these registers.

I'll take a look at that.

> 
>> +}
>> +
>> +static const struct cec_adap_ops drm_dp_cec_adap_ops = {
>> +	.adap_enable = drm_dp_cec_adap_enable,
>> +	.adap_log_addr = drm_dp_cec_adap_log_addr,
>> +	.adap_transmit = drm_dp_cec_adap_transmit,
>> +	.adap_monitor_all_enable = drm_dp_cec_adap_monitor_all_enable,
>> +	.adap_status = drm_dp_cec_adap_status,
>> +};
>> +
>> +static int drm_dp_cec_received(struct drm_dp_aux *aux)
>> +{
>> +	struct cec_adapter *adap = aux->cec_adap;
>> +	struct cec_msg msg;
>> +	u8 rx_msg_info;
>> +	ssize_t err;
>> +
>> +	err = drm_dp_dpcd_readb(aux, DP_CEC_RX_MESSAGE_INFO, &rx_msg_info);
>> +	if (err < 0)
>> +		return err;
>> +	if (!(rx_msg_info & DP_CEC_RX_MESSAGE_ENDED))
>> +		return 0;
>> +	msg.len = (rx_msg_info & DP_CEC_RX_MESSAGE_LEN_MASK) + 1;
>> +	err = drm_dp_dpcd_read(aux, DP_CEC_RX_MESSAGE_BUFFER, msg.msg, msg.len);
>> +	if (err < 0)
>> +		return err;
>> +	cec_received_msg(adap, &msg);
>> +	return 0;
> 
> nit: The code in general feels a bit crowded. Could use a few more
> empty lines IMO.

Will do.

> 
>> +}
>> +
>> +static int drm_dp_cec_handle_irq(struct drm_dp_aux *aux)
>> +{
>> +	struct cec_adapter *adap = aux->cec_adap;
>> +	u8 flags;
>> +	ssize_t err;
>> +
>> +	err = drm_dp_dpcd_readb(aux, DP_CEC_TUNNELING_IRQ_FLAGS, &flags);
>> +	if (err < 0)
>> +		return err;
>> +
>> +	if (flags & DP_CEC_RX_MESSAGE_INFO_VALID)
>> +		drm_dp_cec_received(aux);
>> +
>> +	if (flags & DP_CEC_TX_MESSAGE_SENT)
>> +		cec_transmit_attempt_done(adap, CEC_TX_STATUS_OK);
>> +	else if (flags & DP_CEC_TX_LINE_ERROR)
>> +		cec_transmit_attempt_done(adap, CEC_TX_STATUS_ERROR |
>> +						CEC_TX_STATUS_MAX_RETRIES);
>> +	else if (flags &
>> +		 (DP_CEC_TX_ADDRESS_NACK_ERROR | DP_CEC_TX_DATA_NACK_ERROR))
>> +		cec_transmit_attempt_done(adap, CEC_TX_STATUS_NACK |
>> +						CEC_TX_STATUS_MAX_RETRIES);
>> +	drm_dp_dpcd_writeb(aux, DP_CEC_TUNNELING_IRQ_FLAGS, flags);
>> +	return 0;
>> +}
>> +
>> +/**
>> + * drm_dp_cec_irq() - handle CEC interrupt, if any
>> + * @aux: DisplayPort AUX channel
>> + *
>> + * Should be called when handling an IRQ_HPD request. If CEC-tunneling-over-AUX
>> + * is present, then it will check for a CEC_IRQ and handle it accordingly.
>> + *
>> + * Returns true if an interrupt was handled successfully or false otherwise.
>> + */
>> +bool drm_dp_cec_irq(struct drm_dp_aux *aux)
>> +{
>> +	bool handled = false;
>> +	int attempts;
>> +
>> +	if (!aux->cec_adap)
>> +		return false;
>> +
>> +	for (attempts = 0; attempts < 4; attempts++) {
> 
> What's the deal with this loop?

It was copied from what intel_dp_check_mst_status() does.

However, I think it can be removed since drm_dp_dpcd_access already retries
for up to 32 times.

Does that make sense?

> 
>> +		u8 cec_irq;
>> +		int ret;
>> +
>> +		ret = drm_dp_dpcd_readb(aux, DP_DEVICE_SERVICE_IRQ_VECTOR_ESI1,
>> +					&cec_irq);
> 
> I believe ESI only exists for DPCD 1.2+. Are we protected from reaching
> this on earlier devices? Hmm. I guess the cec_adap check should protect us.
> 
> Supposedly DPCD should just return zeroes for nonexisting registers, but
> I've seen at least one device that failed on that front. In that
> particular case there were just a handful of bytes in the entire DPCD
> address space that couldn't be read succesfully.

I'm not sure if I can do anything to make this more robust. If you have
any suggestions, then I'm happy to try and implement those.

> 
> I don't entirely like this function doing irq read/ack part. There
> really should be some kind of central sink irq dispatch thingy, but
> since we don't have that currently I think this is an OK approach.
> At some point I did start on trying to clean up the sink irq handling
> mess, but I didn't get very far with it before I had to move on to
> something else.
> 
>> +		if (ret < 0 || !(cec_irq & DP_CEC_IRQ))
>> +			break;
>> +
>> +		if (!drm_dp_cec_handle_irq(aux))
>> +			handled = true;
>> +
>> +		ret = drm_dp_dpcd_writeb(aux, DP_DEVICE_SERVICE_IRQ_VECTOR_ESI1,
>> +					 DP_CEC_IRQ);
>> +		if (ret < 0)
>> +			break;
>> +	}
>> +	return handled;
>> +}
>> +EXPORT_SYMBOL(drm_dp_cec_irq);
>> +
>> +/**
>> + * drm_dp_cec_configure_adapter() - configure the CEC adapter
>> + * @aux: DisplayPort AUX channel
>> + * @name: name of the CEC device
>> + * @parent: parent device
>> + *
>> + * Checks if this is a DisplayPort-to-HDMI adapter that supports
>> + * CEC-tunneling-over-AUX, and if so it creates a CEC device.
>> + *
>> + * If a CEC device was already created, then check if the capabilities
>> + * have changed. If not, then do nothing. Otherwise destroy the old
>> + * CEC device and create a new CEC device.
>> + *
>> + * This can happen when one DP-to-HDMI adapter is disconnected and
>> + * replaced by another adapter with different CEC capabilities.
>> + *
>> + * Returns 0 on success or a negative error code on failure.
>> + */
>> +int drm_dp_cec_configure_adapter(struct drm_dp_aux *aux, const char *name,
>> +				 struct device *parent)
>> +{
>> +	u32 cec_caps = CEC_CAP_DEFAULTS | CEC_CAP_NEEDS_HPD;
>> +	unsigned int num_las = 1;
>> +	int err;
>> +	u8 cap;
>> +
>> +	if (drm_dp_dpcd_readb(aux, DP_CEC_TUNNELING_CAPABILITY, &cap) != 1 ||
>> +	    !(cap & DP_CEC_TUNNELING_CAPABLE)) {
>> +		cec_unregister_adapter(aux->cec_adap);
>> +		aux->cec_adap = NULL;
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (cap & DP_CEC_SNOOPING_CAPABLE)
>> +		cec_caps |= CEC_CAP_MONITOR_ALL;
>> +	if (cap & DP_CEC_MULTIPLE_LA_CAPABLE)
>> +		num_las = CEC_MAX_LOG_ADDRS;
>> +
>> +	if (aux->cec_adap) {
>> +		if (aux->cec_adap->capabilities == cec_caps &&
>> +		    aux->cec_adap->available_log_addrs == num_las)
>> +			return 0;
>> +		cec_unregister_adapter(aux->cec_adap);
>> +	}
>> +
>> +	aux->cec_adap = cec_allocate_adapter(&drm_dp_cec_adap_ops,
>> +			 aux, name, cec_caps, num_las);
>> +	if (IS_ERR(aux->cec_adap)) {
>> +		err = PTR_ERR(aux->cec_adap);
>> +		aux->cec_adap = NULL;
>> +		return err;
>> +	}
>> +	err = cec_register_adapter(aux->cec_adap, parent);
>> +	if (err) {
>> +		cec_delete_adapter(aux->cec_adap);
>> +		aux->cec_adap = NULL;
>> +	}
>> +	return err;
>> +}
>> +EXPORT_SYMBOL(drm_dp_cec_configure_adapter);
>> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>> index b17476a6909c..0e236dd40b42 100644
>> --- a/include/drm/drm_dp_helper.h
>> +++ b/include/drm/drm_dp_helper.h
>> @@ -952,6 +952,8 @@ struct drm_dp_aux_msg {
>>  	size_t size;
>>  };
>>  
>> +struct cec_adapter;
>> +
>>  /**
>>   * struct drm_dp_aux - DisplayPort AUX channel
>>   * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
>> @@ -1010,6 +1012,10 @@ struct drm_dp_aux {
>>  	 * @i2c_defer_count: Counts I2C DEFERs, used for DP validation.
>>  	 */
>>  	unsigned i2c_defer_count;
>> +	/**
>> +	 * @cec_adap: the CEC adapter for CEC-Tunneling-over-AUX support.
>> +	 */
>> +	struct cec_adapter *cec_adap;
>>  };
>>  
>>  ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
>> @@ -1132,4 +1138,22 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc, enum drm_dp_quirk quirk)
>>  	return desc->quirks & BIT(quirk);
>>  }
>>  
>> +#ifdef CONFIG_DRM_DP_CEC
>> +bool drm_dp_cec_irq(struct drm_dp_aux *aux);
>> +int drm_dp_cec_configure_adapter(struct drm_dp_aux *aux, const char *name,
>> +				 struct device *parent);
>> +#else
>> +static inline bool drm_dp_cec_irq(struct drm_dp_aux *aux)
>> +{
>> +	return false;
>> +}
>> +
>> +static inline int drm_dp_cec_configure_adapter(struct drm_dp_aux *aux,
>> +					       const char *name,
>> +					       struct device *parent)
>> +{
>> +	return -ENODEV;
>> +}
>> +#endif
>> +
>>  #endif /* _DRM_DP_HELPER_H_ */
>> -- 
>> 2.14.1
> 

Regards,

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

Re: [PATCHv3 1/3] drm: add support for DisplayPort CEC-Tunneling-over-AUX

[Ville Syrjälä]

On Wed, Sep 13, 2017 at 10:51:02AM +0200, Hans Verkuil wrote:
> On 09/12/2017 07:39 PM, Ville Syrjälä wrote:
> > On Mon, Sep 11, 2017 at 01:25:45PM +0200, Hans Verkuil wrote:
> >> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>
> >> This adds support for the DisplayPort CEC-Tunneling-over-AUX
> >> feature that is part of the DisplayPort 1.3 standard.
> >>
> >> Unfortunately, not all DisplayPort/USB-C to HDMI adapters with a
> >> chip that has this capability actually hook up the CEC pin, so
> >> even though a CEC device is created, it may not actually work.
> >>
> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> Tested-by: Carlos Santa <carlos.santa@intel.com>
> >> ---
> >>  drivers/gpu/drm/Kconfig      |  10 ++
> >>  drivers/gpu/drm/Makefile     |   1 +
> >>  drivers/gpu/drm/drm_dp_cec.c | 301 +++++++++++++++++++++++++++++++++++++++++++
> >>  include/drm/drm_dp_helper.h  |  24 ++++
> >>  4 files changed, 336 insertions(+)
> >>  create mode 100644 drivers/gpu/drm/drm_dp_cec.c
> >>
> >> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> >> index 83cb2a88c204..1f2708df5c4e 100644
> >> --- a/drivers/gpu/drm/Kconfig
> >> +++ b/drivers/gpu/drm/Kconfig
> >> @@ -120,6 +120,16 @@ config DRM_LOAD_EDID_FIRMWARE
> >>  	  default case is N. Details and instructions how to build your own
> >>  	  EDID data are given in Documentation/EDID/HOWTO.txt.
> >>  
> >> +config DRM_DP_CEC
> >> +	bool "Enable DisplayPort CEC-Tunneling-over-AUX HDMI support"
> >> +	select CEC_CORE
> >> +	help
> >> +	  Choose this option if you want to enable HDMI CEC support for
> >> +	  DisplayPort/USB-C to HDMI adapters.
> >> +
> >> +	  Note: not all adapters support this feature, and even for those
> >> +	  that do support this they often do not hook up the CEC pin.
> >> +
> >>  config DRM_TTM
> >>  	tristate
> >>  	depends on DRM && MMU
> >> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> >> index 24a066e1841c..c6552c62049e 100644
> >> --- a/drivers/gpu/drm/Makefile
> >> +++ b/drivers/gpu/drm/Makefile
> >> @@ -40,6 +40,7 @@ drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> >>  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> >>  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
> >>  drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
> >> +drm_kms_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o
> >>  
> >>  obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
> >>  obj-$(CONFIG_DRM_DEBUG_MM_SELFTEST) += selftests/
> >> diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> >> new file mode 100644
> >> index 000000000000..b831bb72c932
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/drm_dp_cec.c
> >> @@ -0,0 +1,301 @@
> >> +/*
> >> + * DisplayPort CEC-Tunneling-over-AUX support
> >> + *
> >> + * Copyright 2017 Cisco Systems, Inc. and/or its affiliates. All rights reserved.
> >> + *
> >> + * This program is free software; you may redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License as published by
> >> + * the Free Software Foundation; version 2 of the License.
> >> + *
> >> + * 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 AUTHORS OR COPYRIGHT HOLDERS
> >> + * 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/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/slab.h>
> >> +#include <drm/drm_dp_helper.h>
> >> +#include <media/cec.h>
> >> +
> >> +/*
> >> + * Unfortunately it turns out that we have a chicken-and-egg situation
> >> + * here. Quite a few active (mini-)DP-to-HDMI or USB-C-to-HDMI adapters
> >> + * have a converter chip that supports CEC-Tunneling-over-AUX (usually the
> >> + * Parade PS176), but they do not wire up the CEC pin, thus making CEC
> >> + * useless.
> >> + *
> >> + * Sadly there is no way for this driver to know this. What happens is
> >> + * that a /dev/cecX device is created that is isolated and unable to see
> >> + * any of the other CEC devices. Quite literally the CEC wire is cut
> >> + * (or in this case, never connected in the first place).
> >> + *
> >> + * I suspect that the reason so few adapters support this is that this
> >> + * tunneling protocol was never supported by any OS. So there was no
> >> + * easy way of testing it, and no incentive to correctly wire up the
> >> + * CEC pin.
> >> + *
> >> + * Hopefully by creating this driver it will be easier for vendors to
> >> + * finally fix their adapters and test the CEC functionality.
> >> + *
> >> + * I keep a list of known working adapters here:
> >> + *
> >> + * https://hverkuil.home.xs4all.nl/cec-status.txt
> >> + *
> >> + * Please mail me (hverkuil@xs4all.nl) if you find an adapter that works
> >> + * and is not yet listed there.
> >> + */
> >> +
> >> +/**
> >> + * DOC: dp cec helpers
> >> + *
> >> + * These functions take care of supporting the CEC-Tunneling-over-AUX
> >> + * feature of DisplayPort-to-HDMI adapters.
> >> + */
> >> +
> >> +static int drm_dp_cec_adap_enable(struct cec_adapter *adap, bool enable)
> >> +{
> >> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
> >> +	u32 val = enable ? DP_CEC_TUNNELING_ENABLE : 0;
> >> +	ssize_t err = 0;
> >> +
> >> +	err = drm_dp_dpcd_writeb(aux, DP_CEC_TUNNELING_CONTROL, val);
> >> +	return (enable && err < 0) ? err : 0;
> >> +}
> >> +
> >> +static int drm_dp_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
> >> +{
> >> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
> >> +	/* Bit 15 (logical address 15) should always be set */
> >> +	u16 la_mask = 1 << CEC_LOG_ADDR_BROADCAST;
> >> +	u8 mask[2];
> >> +	ssize_t err;
> >> +
> >> +	if (addr != CEC_LOG_ADDR_INVALID)
> >> +		la_mask |= adap->log_addrs.log_addr_mask | (1 << addr);
> >> +	mask[0] = la_mask & 0xff;
> >> +	mask[1] = la_mask >> 8;
> >> +	err = drm_dp_dpcd_write(aux, DP_CEC_LOGICAL_ADDRESS_MASK, mask, 2);
> >> +	return (addr != CEC_LOG_ADDR_INVALID && err < 0) ? err : 0;
> >> +}
> >> +
> >> +static int drm_dp_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> >> +				    u32 signal_free_time, struct cec_msg *msg)
> >> +{
> >> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
> >> +	unsigned int retries = min(5, attempts - 1);
> >> +	ssize_t err;
> >> +
> >> +	err = drm_dp_dpcd_write(aux, DP_CEC_TX_MESSAGE_BUFFER,
> >> +				msg->msg, msg->len);
> >> +	if (err < 0)
> >> +		return err;
> > 
> > What happens if we managed to write the data only partially?
> > 
> > Looking at our code, I *think* we can't actally return 0 ever, so the 
> > _writeb() variant seem safe with the <0 checks. But the full _write()
> > could certainly return something between 1 and the total size.
> 
> Not according to the drm_dp_dpcd_write() documentation:
> 
> "If not all bytes were transferred, this function returns -EPROTO."
> 
> Looking at the implementation, that is exactly what happens.

Hmm. Indeed. The documentation sure is confusing because it starts by
saying "Returns the number of bytes transferred on success, or a
negative error code on failure.", and only later it seems to clarify
that a not being able to transfer all the bytes is considered an error.
We might want to reword that somehow because I'm surely not the only
one that's confused by this.

Also making something look like read(2)/write(2) and then not following
the same conventions is rather bad IMO. We should probably just change
these to return 0/-errno, or we should really start following the
common convention for read/write. Not quite sure which is better really.
The 0/-errno thing is easier to use certainly, but for just dumping out
the entire DPCD short reads might be nice for the cases where some
random pieces of DPCD can't be succesfully read. But I guess the
0/-errno would be the safer option since it wouldn't actually change
the implementation.

> 
> > 
> >> +
> >> +	err = drm_dp_dpcd_writeb(aux, DP_CEC_TX_MESSAGE_INFO,
> >> +				 (msg->len - 1) | (retries << 4) |
> >> +				 DP_CEC_TX_MESSAGE_SEND);
> >> +	return err < 0 ? err : 0;
> >> +}
> >> +
> >> +static int drm_dp_cec_adap_monitor_all_enable(struct cec_adapter *adap,
> >> +					      bool enable)
> >> +{
> >> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
> >> +	ssize_t err;
> >> +	u8 val;
> >> +
> >> +	if (!(adap->capabilities & CEC_CAP_MONITOR_ALL))
> >> +		return 0;
> >> +
> >> +	err = drm_dp_dpcd_readb(aux, DP_CEC_TUNNELING_CONTROL, &val);
> >> +	if (err >= 0) {
> >> +		if (enable)
> >> +			val |= DP_CEC_SNOOPING_ENABLE;
> >> +		else
> >> +			val &= ~DP_CEC_SNOOPING_ENABLE;
> >> +		err = drm_dp_dpcd_writeb(aux, DP_CEC_TUNNELING_CONTROL, val);
> >> +	}
> >> +	return (enable && err < 0) ? err : 0;
> >> +}
> >> +
> >> +static void drm_dp_cec_adap_status(struct cec_adapter *adap,
> >> +				   struct seq_file *file)
> >> +{
> >> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
> >> +	u8 buf[DP_AUX_MAX_PAYLOAD_BYTES];
> >> +	u8 hw_rev;
> >> +
> >> +	if (drm_dp_dpcd_read(aux, DP_BRANCH_OUI,
> >> +			     buf, sizeof(buf)) != sizeof(buf))
> >> +		return;
> >> +	hw_rev = buf[9];
> >> +	buf[9] = 0;
> >> +	seq_printf(file, "OUI: %02x-%02x-%02x\n",
> >> +		   buf[0], buf[1], buf[2]);
> >> +	seq_printf(file, "ID: %s\n", buf + 3);
> >> +	seq_printf(file, "HW Rev: %d.%d\n", hw_rev >> 4, hw_rev & 0xf);
> >> +	/*
> >> +	 * Show this both in decimal and hex: at least one vendor
> >> +	 * always reports this in hex.
> >> +	 */
> >> +	seq_printf(file, "FW/SW Rev: %d.%d (0x%02x.0x%02x)\n",
> >> +		   buf[10], buf[11], buf[10], buf[11]);
> > 
> > I believe struct drm_dp_dpcd_ident provides a slightly easier way to
> > parse these registers.
> 
> I'll take a look at that.
> 
> > 
> >> +}
> >> +
> >> +static const struct cec_adap_ops drm_dp_cec_adap_ops = {
> >> +	.adap_enable = drm_dp_cec_adap_enable,
> >> +	.adap_log_addr = drm_dp_cec_adap_log_addr,
> >> +	.adap_transmit = drm_dp_cec_adap_transmit,
> >> +	.adap_monitor_all_enable = drm_dp_cec_adap_monitor_all_enable,
> >> +	.adap_status = drm_dp_cec_adap_status,
> >> +};
> >> +
> >> +static int drm_dp_cec_received(struct drm_dp_aux *aux)
> >> +{
> >> +	struct cec_adapter *adap = aux->cec_adap;
> >> +	struct cec_msg msg;
> >> +	u8 rx_msg_info;
> >> +	ssize_t err;
> >> +
> >> +	err = drm_dp_dpcd_readb(aux, DP_CEC_RX_MESSAGE_INFO, &rx_msg_info);
> >> +	if (err < 0)
> >> +		return err;
> >> +	if (!(rx_msg_info & DP_CEC_RX_MESSAGE_ENDED))
> >> +		return 0;
> >> +	msg.len = (rx_msg_info & DP_CEC_RX_MESSAGE_LEN_MASK) + 1;
> >> +	err = drm_dp_dpcd_read(aux, DP_CEC_RX_MESSAGE_BUFFER, msg.msg, msg.len);
> >> +	if (err < 0)
> >> +		return err;
> >> +	cec_received_msg(adap, &msg);
> >> +	return 0;
> > 
> > nit: The code in general feels a bit crowded. Could use a few more
> > empty lines IMO.
> 
> Will do.
> 
> > 
> >> +}
> >> +
> >> +static int drm_dp_cec_handle_irq(struct drm_dp_aux *aux)
> >> +{
> >> +	struct cec_adapter *adap = aux->cec_adap;
> >> +	u8 flags;
> >> +	ssize_t err;
> >> +
> >> +	err = drm_dp_dpcd_readb(aux, DP_CEC_TUNNELING_IRQ_FLAGS, &flags);
> >> +	if (err < 0)
> >> +		return err;
> >> +
> >> +	if (flags & DP_CEC_RX_MESSAGE_INFO_VALID)
> >> +		drm_dp_cec_received(aux);
> >> +
> >> +	if (flags & DP_CEC_TX_MESSAGE_SENT)
> >> +		cec_transmit_attempt_done(adap, CEC_TX_STATUS_OK);
> >> +	else if (flags & DP_CEC_TX_LINE_ERROR)
> >> +		cec_transmit_attempt_done(adap, CEC_TX_STATUS_ERROR |
> >> +						CEC_TX_STATUS_MAX_RETRIES);
> >> +	else if (flags &
> >> +		 (DP_CEC_TX_ADDRESS_NACK_ERROR | DP_CEC_TX_DATA_NACK_ERROR))
> >> +		cec_transmit_attempt_done(adap, CEC_TX_STATUS_NACK |
> >> +						CEC_TX_STATUS_MAX_RETRIES);
> >> +	drm_dp_dpcd_writeb(aux, DP_CEC_TUNNELING_IRQ_FLAGS, flags);
> >> +	return 0;
> >> +}
> >> +
> >> +/**
> >> + * drm_dp_cec_irq() - handle CEC interrupt, if any
> >> + * @aux: DisplayPort AUX channel
> >> + *
> >> + * Should be called when handling an IRQ_HPD request. If CEC-tunneling-over-AUX
> >> + * is present, then it will check for a CEC_IRQ and handle it accordingly.
> >> + *
> >> + * Returns true if an interrupt was handled successfully or false otherwise.
> >> + */
> >> +bool drm_dp_cec_irq(struct drm_dp_aux *aux)
> >> +{
> >> +	bool handled = false;
> >> +	int attempts;
> >> +
> >> +	if (!aux->cec_adap)
> >> +		return false;
> >> +
> >> +	for (attempts = 0; attempts < 4; attempts++) {
> > 
> > What's the deal with this loop?
> 
> It was copied from what intel_dp_check_mst_status() does.
> 
> However, I think it can be removed since drm_dp_dpcd_access already retries
> for up to 32 times.
> 
> Does that make sense?

Yeah, better to not add something if we don't have a good reason for
it. It's just going make life hard when/if when/if we want to refactor
the sink irq handling.

> 
> > 
> >> +		u8 cec_irq;
> >> +		int ret;
> >> +
> >> +		ret = drm_dp_dpcd_readb(aux, DP_DEVICE_SERVICE_IRQ_VECTOR_ESI1,
> >> +					&cec_irq);
> > 
> > I believe ESI only exists for DPCD 1.2+. Are we protected from reaching
> > this on earlier devices? Hmm. I guess the cec_adap check should protect us.
> > 
> > Supposedly DPCD should just return zeroes for nonexisting registers, but
> > I've seen at least one device that failed on that front. In that
> > particular case there were just a handful of bytes in the entire DPCD
> > address space that couldn't be read succesfully.
> 
> I'm not sure if I can do anything to make this more robust. If you have
> any suggestions, then I'm happy to try and implement those.

Nah. I think looks fine as is.

> 
> > 
> > I don't entirely like this function doing irq read/ack part. There
> > really should be some kind of central sink irq dispatch thingy, but
> > since we don't have that currently I think this is an OK approach.
> > At some point I did start on trying to clean up the sink irq handling
> > mess, but I didn't get very far with it before I had to move on to
> > something else.
> > 
> >> +		if (ret < 0 || !(cec_irq & DP_CEC_IRQ))
> >> +			break;
> >> +
> >> +		if (!drm_dp_cec_handle_irq(aux))
> >> +			handled = true;
> >> +
> >> +		ret = drm_dp_dpcd_writeb(aux, DP_DEVICE_SERVICE_IRQ_VECTOR_ESI1,
> >> +					 DP_CEC_IRQ);
> >> +		if (ret < 0)
> >> +			break;
> >> +	}
> >> +	return handled;
> >> +}
> >> +EXPORT_SYMBOL(drm_dp_cec_irq);
> >> +
> >> +/**
> >> + * drm_dp_cec_configure_adapter() - configure the CEC adapter
> >> + * @aux: DisplayPort AUX channel
> >> + * @name: name of the CEC device
> >> + * @parent: parent device
> >> + *
> >> + * Checks if this is a DisplayPort-to-HDMI adapter that supports
> >> + * CEC-tunneling-over-AUX, and if so it creates a CEC device.
> >> + *
> >> + * If a CEC device was already created, then check if the capabilities
> >> + * have changed. If not, then do nothing. Otherwise destroy the old
> >> + * CEC device and create a new CEC device.
> >> + *
> >> + * This can happen when one DP-to-HDMI adapter is disconnected and
> >> + * replaced by another adapter with different CEC capabilities.
> >> + *
> >> + * Returns 0 on success or a negative error code on failure.
> >> + */
> >> +int drm_dp_cec_configure_adapter(struct drm_dp_aux *aux, const char *name,
> >> +				 struct device *parent)
> >> +{
> >> +	u32 cec_caps = CEC_CAP_DEFAULTS | CEC_CAP_NEEDS_HPD;
> >> +	unsigned int num_las = 1;
> >> +	int err;
> >> +	u8 cap;
> >> +
> >> +	if (drm_dp_dpcd_readb(aux, DP_CEC_TUNNELING_CAPABILITY, &cap) != 1 ||
> >> +	    !(cap & DP_CEC_TUNNELING_CAPABLE)) {
> >> +		cec_unregister_adapter(aux->cec_adap);
> >> +		aux->cec_adap = NULL;
> >> +		return -ENODEV;
> >> +	}
> >> +
> >> +	if (cap & DP_CEC_SNOOPING_CAPABLE)
> >> +		cec_caps |= CEC_CAP_MONITOR_ALL;
> >> +	if (cap & DP_CEC_MULTIPLE_LA_CAPABLE)
> >> +		num_las = CEC_MAX_LOG_ADDRS;
> >> +
> >> +	if (aux->cec_adap) {
> >> +		if (aux->cec_adap->capabilities == cec_caps &&
> >> +		    aux->cec_adap->available_log_addrs == num_las)
> >> +			return 0;
> >> +		cec_unregister_adapter(aux->cec_adap);
> >> +	}
> >> +
> >> +	aux->cec_adap = cec_allocate_adapter(&drm_dp_cec_adap_ops,
> >> +			 aux, name, cec_caps, num_las);
> >> +	if (IS_ERR(aux->cec_adap)) {
> >> +		err = PTR_ERR(aux->cec_adap);
> >> +		aux->cec_adap = NULL;
> >> +		return err;
> >> +	}
> >> +	err = cec_register_adapter(aux->cec_adap, parent);
> >> +	if (err) {
> >> +		cec_delete_adapter(aux->cec_adap);
> >> +		aux->cec_adap = NULL;
> >> +	}
> >> +	return err;
> >> +}
> >> +EXPORT_SYMBOL(drm_dp_cec_configure_adapter);
> >> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> >> index b17476a6909c..0e236dd40b42 100644
> >> --- a/include/drm/drm_dp_helper.h
> >> +++ b/include/drm/drm_dp_helper.h
> >> @@ -952,6 +952,8 @@ struct drm_dp_aux_msg {
> >>  	size_t size;
> >>  };
> >>  
> >> +struct cec_adapter;
> >> +
> >>  /**
> >>   * struct drm_dp_aux - DisplayPort AUX channel
> >>   * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
> >> @@ -1010,6 +1012,10 @@ struct drm_dp_aux {
> >>  	 * @i2c_defer_count: Counts I2C DEFERs, used for DP validation.
> >>  	 */
> >>  	unsigned i2c_defer_count;
> >> +	/**
> >> +	 * @cec_adap: the CEC adapter for CEC-Tunneling-over-AUX support.
> >> +	 */
> >> +	struct cec_adapter *cec_adap;
> >>  };
> >>  
> >>  ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
> >> @@ -1132,4 +1138,22 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc, enum drm_dp_quirk quirk)
> >>  	return desc->quirks & BIT(quirk);
> >>  }
> >>  
> >> +#ifdef CONFIG_DRM_DP_CEC
> >> +bool drm_dp_cec_irq(struct drm_dp_aux *aux);
> >> +int drm_dp_cec_configure_adapter(struct drm_dp_aux *aux, const char *name,
> >> +				 struct device *parent);
> >> +#else
> >> +static inline bool drm_dp_cec_irq(struct drm_dp_aux *aux)
> >> +{
> >> +	return false;
> >> +}
> >> +
> >> +static inline int drm_dp_cec_configure_adapter(struct drm_dp_aux *aux,
> >> +					       const char *name,
> >> +					       struct device *parent)
> >> +{
> >> +	return -ENODEV;
> >> +}
> >> +#endif
> >> +
> >>  #endif /* _DRM_DP_HELPER_H_ */
> >> -- 
> >> 2.14.1
> > 
> 
> Regards,
> 
> 	Hans

-- 
Ville Syrjälä
Intel OTC

Re: [PATCHv3 1/3] drm: add support for DisplayPort CEC-Tunneling-over-AUX

[Ville Syrjälä]

On Wed, Sep 13, 2017 at 10:51:02AM +0200, Hans Verkuil wrote:
> On 09/12/2017 07:39 PM, Ville Syrjälä wrote:
> > On Mon, Sep 11, 2017 at 01:25:45PM +0200, Hans Verkuil wrote:
> >> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>
> >> This adds support for the DisplayPort CEC-Tunneling-over-AUX
> >> feature that is part of the DisplayPort 1.3 standard.
> >>
> >> Unfortunately, not all DisplayPort/USB-C to HDMI adapters with a
> >> chip that has this capability actually hook up the CEC pin, so
> >> even though a CEC device is created, it may not actually work.
> >>
> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> Tested-by: Carlos Santa <carlos.santa@intel.com>
> >> ---
> >>  drivers/gpu/drm/Kconfig      |  10 ++
> >>  drivers/gpu/drm/Makefile     |   1 +
> >>  drivers/gpu/drm/drm_dp_cec.c | 301 +++++++++++++++++++++++++++++++++++++++++++
> >>  include/drm/drm_dp_helper.h  |  24 ++++
> >>  4 files changed, 336 insertions(+)
> >>  create mode 100644 drivers/gpu/drm/drm_dp_cec.c
> >>
> >> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> >> index 83cb2a88c204..1f2708df5c4e 100644
> >> --- a/drivers/gpu/drm/Kconfig
> >> +++ b/drivers/gpu/drm/Kconfig
> >> @@ -120,6 +120,16 @@ config DRM_LOAD_EDID_FIRMWARE
> >>  	  default case is N. Details and instructions how to build your own
> >>  	  EDID data are given in Documentation/EDID/HOWTO.txt.
> >>  
> >> +config DRM_DP_CEC
> >> +	bool "Enable DisplayPort CEC-Tunneling-over-AUX HDMI support"
> >> +	select CEC_CORE
> >> +	help
> >> +	  Choose this option if you want to enable HDMI CEC support for
> >> +	  DisplayPort/USB-C to HDMI adapters.
> >> +
> >> +	  Note: not all adapters support this feature, and even for those
> >> +	  that do support this they often do not hook up the CEC pin.
> >> +
> >>  config DRM_TTM
> >>  	tristate
> >>  	depends on DRM && MMU
> >> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> >> index 24a066e1841c..c6552c62049e 100644
> >> --- a/drivers/gpu/drm/Makefile
> >> +++ b/drivers/gpu/drm/Makefile
> >> @@ -40,6 +40,7 @@ drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> >>  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> >>  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
> >>  drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
> >> +drm_kms_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o
> >>  
> >>  obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
> >>  obj-$(CONFIG_DRM_DEBUG_MM_SELFTEST) += selftests/
> >> diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> >> new file mode 100644
> >> index 000000000000..b831bb72c932
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/drm_dp_cec.c
> >> @@ -0,0 +1,301 @@
> >> +/*
> >> + * DisplayPort CEC-Tunneling-over-AUX support
> >> + *
> >> + * Copyright 2017 Cisco Systems, Inc. and/or its affiliates. All rights reserved.
> >> + *
> >> + * This program is free software; you may redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License as published by
> >> + * the Free Software Foundation; version 2 of the License.
> >> + *
> >> + * 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 AUTHORS OR COPYRIGHT HOLDERS
> >> + * 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/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/slab.h>
> >> +#include <drm/drm_dp_helper.h>
> >> +#include <media/cec.h>
> >> +
> >> +/*
> >> + * Unfortunately it turns out that we have a chicken-and-egg situation
> >> + * here. Quite a few active (mini-)DP-to-HDMI or USB-C-to-HDMI adapters
> >> + * have a converter chip that supports CEC-Tunneling-over-AUX (usually the
> >> + * Parade PS176), but they do not wire up the CEC pin, thus making CEC
> >> + * useless.
> >> + *
> >> + * Sadly there is no way for this driver to know this. What happens is
> >> + * that a /dev/cecX device is created that is isolated and unable to see
> >> + * any of the other CEC devices. Quite literally the CEC wire is cut
> >> + * (or in this case, never connected in the first place).
> >> + *
> >> + * I suspect that the reason so few adapters support this is that this
> >> + * tunneling protocol was never supported by any OS. So there was no
> >> + * easy way of testing it, and no incentive to correctly wire up the
> >> + * CEC pin.
> >> + *
> >> + * Hopefully by creating this driver it will be easier for vendors to
> >> + * finally fix their adapters and test the CEC functionality.
> >> + *
> >> + * I keep a list of known working adapters here:
> >> + *
> >> + * https://hverkuil.home.xs4all.nl/cec-status.txt
> >> + *
> >> + * Please mail me (hverkuil@xs4all.nl) if you find an adapter that works
> >> + * and is not yet listed there.
> >> + */
> >> +
> >> +/**
> >> + * DOC: dp cec helpers
> >> + *
> >> + * These functions take care of supporting the CEC-Tunneling-over-AUX
> >> + * feature of DisplayPort-to-HDMI adapters.
> >> + */
> >> +
> >> +static int drm_dp_cec_adap_enable(struct cec_adapter *adap, bool enable)
> >> +{
> >> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
> >> +	u32 val = enable ? DP_CEC_TUNNELING_ENABLE : 0;
> >> +	ssize_t err = 0;
> >> +
> >> +	err = drm_dp_dpcd_writeb(aux, DP_CEC_TUNNELING_CONTROL, val);
> >> +	return (enable && err < 0) ? err : 0;
> >> +}
> >> +
> >> +static int drm_dp_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
> >> +{
> >> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
> >> +	/* Bit 15 (logical address 15) should always be set */
> >> +	u16 la_mask = 1 << CEC_LOG_ADDR_BROADCAST;
> >> +	u8 mask[2];
> >> +	ssize_t err;
> >> +
> >> +	if (addr != CEC_LOG_ADDR_INVALID)
> >> +		la_mask |= adap->log_addrs.log_addr_mask | (1 << addr);
> >> +	mask[0] = la_mask & 0xff;
> >> +	mask[1] = la_mask >> 8;
> >> +	err = drm_dp_dpcd_write(aux, DP_CEC_LOGICAL_ADDRESS_MASK, mask, 2);
> >> +	return (addr != CEC_LOG_ADDR_INVALID && err < 0) ? err : 0;
> >> +}
> >> +
> >> +static int drm_dp_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> >> +				    u32 signal_free_time, struct cec_msg *msg)
> >> +{
> >> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
> >> +	unsigned int retries = min(5, attempts - 1);
> >> +	ssize_t err;
> >> +
> >> +	err = drm_dp_dpcd_write(aux, DP_CEC_TX_MESSAGE_BUFFER,
> >> +				msg->msg, msg->len);
> >> +	if (err < 0)
> >> +		return err;
> > 
> > What happens if we managed to write the data only partially?
> > 
> > Looking at our code, I *think* we can't actally return 0 ever, so the 
> > _writeb() variant seem safe with the <0 checks. But the full _write()
> > could certainly return something between 1 and the total size.
> 
> Not according to the drm_dp_dpcd_write() documentation:
> 
> "If not all bytes were transferred, this function returns -EPROTO."
> 
> Looking at the implementation, that is exactly what happens.

Hmm. Indeed. The documentation sure is confusing because it starts by
saying "Returns the number of bytes transferred on success, or a
negative error code on failure.", and only later it seems to clarify
that a not being able to transfer all the bytes is considered an error.
We might want to reword that somehow because I'm surely not the only
one that's confused by this.

Also making something look like read(2)/write(2) and then not following
the same conventions is rather bad IMO. We should probably just change
these to return 0/-errno, or we should really start following the
common convention for read/write. Not quite sure which is better really.
The 0/-errno thing is easier to use certainly, but for just dumping out
the entire DPCD short reads might be nice for the cases where some
random pieces of DPCD can't be succesfully read. But I guess the
0/-errno would be the safer option since it wouldn't actually change
the implementation.

> 
> > 
> >> +
> >> +	err = drm_dp_dpcd_writeb(aux, DP_CEC_TX_MESSAGE_INFO,
> >> +				 (msg->len - 1) | (retries << 4) |
> >> +				 DP_CEC_TX_MESSAGE_SEND);
> >> +	return err < 0 ? err : 0;
> >> +}
> >> +
> >> +static int drm_dp_cec_adap_monitor_all_enable(struct cec_adapter *adap,
> >> +					      bool enable)
> >> +{
> >> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
> >> +	ssize_t err;
> >> +	u8 val;
> >> +
> >> +	if (!(adap->capabilities & CEC_CAP_MONITOR_ALL))
> >> +		return 0;
> >> +
> >> +	err = drm_dp_dpcd_readb(aux, DP_CEC_TUNNELING_CONTROL, &val);
> >> +	if (err >= 0) {
> >> +		if (enable)
> >> +			val |= DP_CEC_SNOOPING_ENABLE;
> >> +		else
> >> +			val &= ~DP_CEC_SNOOPING_ENABLE;
> >> +		err = drm_dp_dpcd_writeb(aux, DP_CEC_TUNNELING_CONTROL, val);
> >> +	}
> >> +	return (enable && err < 0) ? err : 0;
> >> +}
> >> +
> >> +static void drm_dp_cec_adap_status(struct cec_adapter *adap,
> >> +				   struct seq_file *file)
> >> +{
> >> +	struct drm_dp_aux *aux = cec_get_drvdata(adap);
> >> +	u8 buf[DP_AUX_MAX_PAYLOAD_BYTES];
> >> +	u8 hw_rev;
> >> +
> >> +	if (drm_dp_dpcd_read(aux, DP_BRANCH_OUI,
> >> +			     buf, sizeof(buf)) != sizeof(buf))
> >> +		return;
> >> +	hw_rev = buf[9];
> >> +	buf[9] = 0;
> >> +	seq_printf(file, "OUI: %02x-%02x-%02x\n",
> >> +		   buf[0], buf[1], buf[2]);
> >> +	seq_printf(file, "ID: %s\n", buf + 3);
> >> +	seq_printf(file, "HW Rev: %d.%d\n", hw_rev >> 4, hw_rev & 0xf);
> >> +	/*
> >> +	 * Show this both in decimal and hex: at least one vendor
> >> +	 * always reports this in hex.
> >> +	 */
> >> +	seq_printf(file, "FW/SW Rev: %d.%d (0x%02x.0x%02x)\n",
> >> +		   buf[10], buf[11], buf[10], buf[11]);
> > 
> > I believe struct drm_dp_dpcd_ident provides a slightly easier way to
> > parse these registers.
> 
> I'll take a look at that.
> 
> > 
> >> +}
> >> +
> >> +static const struct cec_adap_ops drm_dp_cec_adap_ops = {
> >> +	.adap_enable = drm_dp_cec_adap_enable,
> >> +	.adap_log_addr = drm_dp_cec_adap_log_addr,
> >> +	.adap_transmit = drm_dp_cec_adap_transmit,
> >> +	.adap_monitor_all_enable = drm_dp_cec_adap_monitor_all_enable,
> >> +	.adap_status = drm_dp_cec_adap_status,
> >> +};
> >> +
> >> +static int drm_dp_cec_received(struct drm_dp_aux *aux)
> >> +{
> >> +	struct cec_adapter *adap = aux->cec_adap;
> >> +	struct cec_msg msg;
> >> +	u8 rx_msg_info;
> >> +	ssize_t err;
> >> +
> >> +	err = drm_dp_dpcd_readb(aux, DP_CEC_RX_MESSAGE_INFO, &rx_msg_info);
> >> +	if (err < 0)
> >> +		return err;
> >> +	if (!(rx_msg_info & DP_CEC_RX_MESSAGE_ENDED))
> >> +		return 0;
> >> +	msg.len = (rx_msg_info & DP_CEC_RX_MESSAGE_LEN_MASK) + 1;
> >> +	err = drm_dp_dpcd_read(aux, DP_CEC_RX_MESSAGE_BUFFER, msg.msg, msg.len);
> >> +	if (err < 0)
> >> +		return err;
> >> +	cec_received_msg(adap, &msg);
> >> +	return 0;
> > 
> > nit: The code in general feels a bit crowded. Could use a few more
> > empty lines IMO.
> 
> Will do.
> 
> > 
> >> +}
> >> +
> >> +static int drm_dp_cec_handle_irq(struct drm_dp_aux *aux)
> >> +{
> >> +	struct cec_adapter *adap = aux->cec_adap;
> >> +	u8 flags;
> >> +	ssize_t err;
> >> +
> >> +	err = drm_dp_dpcd_readb(aux, DP_CEC_TUNNELING_IRQ_FLAGS, &flags);
> >> +	if (err < 0)
> >> +		return err;
> >> +
> >> +	if (flags & DP_CEC_RX_MESSAGE_INFO_VALID)
> >> +		drm_dp_cec_received(aux);
> >> +
> >> +	if (flags & DP_CEC_TX_MESSAGE_SENT)
> >> +		cec_transmit_attempt_done(adap, CEC_TX_STATUS_OK);
> >> +	else if (flags & DP_CEC_TX_LINE_ERROR)
> >> +		cec_transmit_attempt_done(adap, CEC_TX_STATUS_ERROR |
> >> +						CEC_TX_STATUS_MAX_RETRIES);
> >> +	else if (flags &
> >> +		 (DP_CEC_TX_ADDRESS_NACK_ERROR | DP_CEC_TX_DATA_NACK_ERROR))
> >> +		cec_transmit_attempt_done(adap, CEC_TX_STATUS_NACK |
> >> +						CEC_TX_STATUS_MAX_RETRIES);
> >> +	drm_dp_dpcd_writeb(aux, DP_CEC_TUNNELING_IRQ_FLAGS, flags);
> >> +	return 0;
> >> +}
> >> +
> >> +/**
> >> + * drm_dp_cec_irq() - handle CEC interrupt, if any
> >> + * @aux: DisplayPort AUX channel
> >> + *
> >> + * Should be called when handling an IRQ_HPD request. If CEC-tunneling-over-AUX
> >> + * is present, then it will check for a CEC_IRQ and handle it accordingly.
> >> + *
> >> + * Returns true if an interrupt was handled successfully or false otherwise.
> >> + */
> >> +bool drm_dp_cec_irq(struct drm_dp_aux *aux)
> >> +{
> >> +	bool handled = false;
> >> +	int attempts;
> >> +
> >> +	if (!aux->cec_adap)
> >> +		return false;
> >> +
> >> +	for (attempts = 0; attempts < 4; attempts++) {
> > 
> > What's the deal with this loop?
> 
> It was copied from what intel_dp_check_mst_status() does.
> 
> However, I think it can be removed since drm_dp_dpcd_access already retries
> for up to 32 times.
> 
> Does that make sense?

Yeah, better to not add something if we don't have a good reason for
it. It's just going make life hard when/if when/if we want to refactor
the sink irq handling.

> 
> > 
> >> +		u8 cec_irq;
> >> +		int ret;
> >> +
> >> +		ret = drm_dp_dpcd_readb(aux, DP_DEVICE_SERVICE_IRQ_VECTOR_ESI1,
> >> +					&cec_irq);
> > 
> > I believe ESI only exists for DPCD 1.2+. Are we protected from reaching
> > this on earlier devices? Hmm. I guess the cec_adap check should protect us.
> > 
> > Supposedly DPCD should just return zeroes for nonexisting registers, but
> > I've seen at least one device that failed on that front. In that
> > particular case there were just a handful of bytes in the entire DPCD
> > address space that couldn't be read succesfully.
> 
> I'm not sure if I can do anything to make this more robust. If you have
> any suggestions, then I'm happy to try and implement those.

Nah. I think looks fine as is.

> 
> > 
> > I don't entirely like this function doing irq read/ack part. There
> > really should be some kind of central sink irq dispatch thingy, but
> > since we don't have that currently I think this is an OK approach.
> > At some point I did start on trying to clean up the sink irq handling
> > mess, but I didn't get very far with it before I had to move on to
> > something else.
> > 
> >> +		if (ret < 0 || !(cec_irq & DP_CEC_IRQ))
> >> +			break;
> >> +
> >> +		if (!drm_dp_cec_handle_irq(aux))
> >> +			handled = true;
> >> +
> >> +		ret = drm_dp_dpcd_writeb(aux, DP_DEVICE_SERVICE_IRQ_VECTOR_ESI1,
> >> +					 DP_CEC_IRQ);
> >> +		if (ret < 0)
> >> +			break;
> >> +	}
> >> +	return handled;
> >> +}
> >> +EXPORT_SYMBOL(drm_dp_cec_irq);
> >> +
> >> +/**
> >> + * drm_dp_cec_configure_adapter() - configure the CEC adapter
> >> + * @aux: DisplayPort AUX channel
> >> + * @name: name of the CEC device
> >> + * @parent: parent device
> >> + *
> >> + * Checks if this is a DisplayPort-to-HDMI adapter that supports
> >> + * CEC-tunneling-over-AUX, and if so it creates a CEC device.
> >> + *
> >> + * If a CEC device was already created, then check if the capabilities
> >> + * have changed. If not, then do nothing. Otherwise destroy the old
> >> + * CEC device and create a new CEC device.
> >> + *
> >> + * This can happen when one DP-to-HDMI adapter is disconnected and
> >> + * replaced by another adapter with different CEC capabilities.
> >> + *
> >> + * Returns 0 on success or a negative error code on failure.
> >> + */
> >> +int drm_dp_cec_configure_adapter(struct drm_dp_aux *aux, const char *name,
> >> +				 struct device *parent)
> >> +{
> >> +	u32 cec_caps = CEC_CAP_DEFAULTS | CEC_CAP_NEEDS_HPD;
> >> +	unsigned int num_las = 1;
> >> +	int err;
> >> +	u8 cap;
> >> +
> >> +	if (drm_dp_dpcd_readb(aux, DP_CEC_TUNNELING_CAPABILITY, &cap) != 1 ||
> >> +	    !(cap & DP_CEC_TUNNELING_CAPABLE)) {
> >> +		cec_unregister_adapter(aux->cec_adap);
> >> +		aux->cec_adap = NULL;
> >> +		return -ENODEV;
> >> +	}
> >> +
> >> +	if (cap & DP_CEC_SNOOPING_CAPABLE)
> >> +		cec_caps |= CEC_CAP_MONITOR_ALL;
> >> +	if (cap & DP_CEC_MULTIPLE_LA_CAPABLE)
> >> +		num_las = CEC_MAX_LOG_ADDRS;
> >> +
> >> +	if (aux->cec_adap) {
> >> +		if (aux->cec_adap->capabilities == cec_caps &&
> >> +		    aux->cec_adap->available_log_addrs == num_las)
> >> +			return 0;
> >> +		cec_unregister_adapter(aux->cec_adap);
> >> +	}
> >> +
> >> +	aux->cec_adap = cec_allocate_adapter(&drm_dp_cec_adap_ops,
> >> +			 aux, name, cec_caps, num_las);
> >> +	if (IS_ERR(aux->cec_adap)) {
> >> +		err = PTR_ERR(aux->cec_adap);
> >> +		aux->cec_adap = NULL;
> >> +		return err;
> >> +	}
> >> +	err = cec_register_adapter(aux->cec_adap, parent);
> >> +	if (err) {
> >> +		cec_delete_adapter(aux->cec_adap);
> >> +		aux->cec_adap = NULL;
> >> +	}
> >> +	return err;
> >> +}
> >> +EXPORT_SYMBOL(drm_dp_cec_configure_adapter);
> >> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> >> index b17476a6909c..0e236dd40b42 100644
> >> --- a/include/drm/drm_dp_helper.h
> >> +++ b/include/drm/drm_dp_helper.h
> >> @@ -952,6 +952,8 @@ struct drm_dp_aux_msg {
> >>  	size_t size;
> >>  };
> >>  
> >> +struct cec_adapter;
> >> +
> >>  /**
> >>   * struct drm_dp_aux - DisplayPort AUX channel
> >>   * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
> >> @@ -1010,6 +1012,10 @@ struct drm_dp_aux {
> >>  	 * @i2c_defer_count: Counts I2C DEFERs, used for DP validation.
> >>  	 */
> >>  	unsigned i2c_defer_count;
> >> +	/**
> >> +	 * @cec_adap: the CEC adapter for CEC-Tunneling-over-AUX support.
> >> +	 */
> >> +	struct cec_adapter *cec_adap;
> >>  };
> >>  
> >>  ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
> >> @@ -1132,4 +1138,22 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc, enum drm_dp_quirk quirk)
> >>  	return desc->quirks & BIT(quirk);
> >>  }
> >>  
> >> +#ifdef CONFIG_DRM_DP_CEC
> >> +bool drm_dp_cec_irq(struct drm_dp_aux *aux);
> >> +int drm_dp_cec_configure_adapter(struct drm_dp_aux *aux, const char *name,
> >> +				 struct device *parent);
> >> +#else
> >> +static inline bool drm_dp_cec_irq(struct drm_dp_aux *aux)
> >> +{
> >> +	return false;
> >> +}
> >> +
> >> +static inline int drm_dp_cec_configure_adapter(struct drm_dp_aux *aux,
> >> +					       const char *name,
> >> +					       struct device *parent)
> >> +{
> >> +	return -ENODEV;
> >> +}
> >> +#endif
> >> +
> >>  #endif /* _DRM_DP_HELPER_H_ */
> >> -- 
> >> 2.14.1
> > 
> 
> Regards,
> 
> 	Hans

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