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

[PATCHv4 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.

Changes since v3:

Incorporated Ville's comments. The two main changes (besides some small
readability changes) are:

- use drm_dp_read_desc() in drm_dp_cec_adap_status().
- drop the 'for (attempts = 0...)' loop in drm_dp_cec_irq().

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          | 292 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c       |  18 ++-
 include/drm/drm_dp_helper.h           |  24 +++
 6 files changed, 350 insertions(+), 4 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_dp_cec.c

-- 
2.14.1

[PATCHv4 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 | 292 +++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_dp_helper.h  |  24 ++++
 4 files changed, 327 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..d110cac007de
--- /dev/null
+++ b/drivers/gpu/drm/drm_dp_cec.c
@@ -0,0 +1,292 @@
+/*
+ * 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);
+	struct drm_dp_desc desc;
+	struct drm_dp_dpcd_ident *id = &desc.ident;
+
+	if (drm_dp_read_desc(aux, &desc, true))
+		return;
+	seq_printf(file, "OUI: %02x-%02x-%02x\n",
+		   id->oui[0], id->oui[1], id->oui[2]);
+	seq_printf(file, "ID: %.*s\n",
+		   (int)sizeof(id->device_id), id->device_id);
+	seq_printf(file, "HW Rev: %d.%d\n", id->hw_rev >> 4, id->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",
+		   id->sw_major_rev, id->sw_minor_rev,
+		   id->sw_major_rev, id->sw_minor_rev);
+}
+
+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 bool drm_dp_cec_handle_irq(struct drm_dp_aux *aux)
+{
+	struct cec_adapter *adap = aux->cec_adap;
+	u8 flags;
+
+	if (drm_dp_dpcd_readb(aux, DP_CEC_TUNNELING_IRQ_FLAGS, &flags) < 0)
+		return false;
+
+	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 true;
+}
+
+/**
+ * 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;
+	u8 cec_irq;
+	int ret;
+
+	if (!aux->cec_adap)
+		return false;
+
+	ret = drm_dp_dpcd_readb(aux, DP_DEVICE_SERVICE_IRQ_VECTOR_ESI1,
+				&cec_irq);
+	if (ret < 0 || !(cec_irq & DP_CEC_IRQ))
+		return false;
+
+	handled = drm_dp_cec_handle_irq(aux);
+	drm_dp_dpcd_writeb(aux, DP_DEVICE_SERVICE_IRQ_VECTOR_ESI1, DP_CEC_IRQ);
+	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

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

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

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

[Ville Syrjälä]

On Sat, Sep 16, 2017 at 04:17:24PM +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.
> 

FYI we prefer to put the changelog into the commit message. Makes it
easier to see which version of the patch we're dealing with, and avoids
having to open up some other mail when doing the review.

> 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 | 292 +++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_dp_helper.h  |  24 ++++
>  4 files changed, 327 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..d110cac007de
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_dp_cec.c
> @@ -0,0 +1,292 @@
> +/*
> + * 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);
> +	struct drm_dp_desc desc;
> +	struct drm_dp_dpcd_ident *id = &desc.ident;
> +
> +	if (drm_dp_read_desc(aux, &desc, true))
> +		return;
> +	seq_printf(file, "OUI: %02x-%02x-%02x\n",
> +		   id->oui[0], id->oui[1], id->oui[2]);

We use %*phD elsewhere

> +	seq_printf(file, "ID: %.*s\n",
> +		   (int)sizeof(id->device_id), id->device_id);

%*pE used elsewhere

> +	seq_printf(file, "HW Rev: %d.%d\n", id->hw_rev >> 4, id->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",
> +		   id->sw_major_rev, id->sw_minor_rev,
> +		   id->sw_major_rev, id->sw_minor_rev);
> +}
> +
> +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 bool drm_dp_cec_handle_irq(struct drm_dp_aux *aux)
> +{
> +	struct cec_adapter *adap = aux->cec_adap;
> +	u8 flags;
> +
> +	if (drm_dp_dpcd_readb(aux, DP_CEC_TUNNELING_IRQ_FLAGS, &flags) < 0)
> +		return false;
> +
> +	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 true;
> +}
> +
> +/**
> + * 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;
> +	u8 cec_irq;
> +	int ret;
> +
> +	if (!aux->cec_adap)
> +		return false;
> +
> +	ret = drm_dp_dpcd_readb(aux, DP_DEVICE_SERVICE_IRQ_VECTOR_ESI1,
> +				&cec_irq);
> +	if (ret < 0 || !(cec_irq & DP_CEC_IRQ))
> +		return false;
> +
> +	handled = drm_dp_cec_handle_irq(aux);
> +	drm_dp_dpcd_writeb(aux, DP_DEVICE_SERVICE_IRQ_VECTOR_ESI1, DP_CEC_IRQ);
> +	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);

There's a bit of chicken and egg problem here. We should not register
any user visible interfaces until the full drm device has been set up.
There's a .late_register() hook for connectors for this purpose.

So I guess what we want to do is defer this registration until that
time, unless the connector is already registered. drm_dp_aux_register()
might be the right spot for this call, or maybe we want to put it next
to that call in driver code? Not sure.

Hmm. And I guess that's going to be somewhat racy unless we protect
the direct registration with connector->mutex. I hope that can't
deadlock...

> +	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: [PATCHv4 3/3] drm/i915: add DisplayPort CEC-Tunneling-over-AUX support

[Ville Syrjälä]

On Sat, Sep 16, 2017 at 04:17:26PM +0200, Hans Verkuil wrote:
> 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);

This is cluttering up the code a bit. Maybe do this call somewhere
around the intel_dp_configure_mst() call instead since that seems to be
the place where we start to do changes to externally visible state.

Actually, do we want to register cec adapters for MST devices?

And shouldn't we call this regardless of the connector state so that
the cec adapter gets unregistered when the device is disconnected?

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

-- 
Ville Syrjälä
Intel OTC

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

[Hans Verkuil]

Hi Ville,

On 09/18/2017 03:05 PM, Ville Syrjälä wrote:
> On Sat, Sep 16, 2017 at 04:17:26PM +0200, Hans Verkuil wrote:
>> 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);
> 
> This is cluttering up the code a bit. Maybe do this call somewhere
> around the intel_dp_configure_mst() call instead since that seems to be
> the place where we start to do changes to externally visible state.
> 
> Actually, do we want to register cec adapters for MST devices?
> 
> And shouldn't we call this regardless of the connector state so that
> the cec adapter gets unregistered when the device is disconnected?

This hasn't (AFAIK) anything to do with MST. This is in a branch device (i.e.
a DP to HDMI adapter).

The CEC adapter should ideally be associated with the branch device (since that
is what implements the CEC tunneling): i.e. when you connect the adapter, then
the CEC device is created, when you disconnect the adapter, then the CEC device
should be unregistered. This is not the same as connecting/disconnecting the
HDMI cable to/from the adapter: that just sets or invalidates the CEC physical
address (which is read from the EDID).

However, I have not seen any code that tells me when the adapter is plugged in
or is unplugged. So all I have to go on is when the HDMI cable is connected.

Note that the 'late_register' you mentioned in your 1/3 review isn't called when
connecting the adapter. So that too cannot be used as a trigger to detect if
this protocol is supported.

I know doing this here is not ideal, but I have not found another way and I am
not even certain if it is possible at all, it might be intrinsic to how DP works.
I do not consider myself a DP expert, though.

So this is how it works now, e.g. on my Intel NUC and a USB-C to HDMI adapter:

1) I connect the adapter (no HDMI connected yet): nothing happens.
2) I connect the HDMI cable to the adapter: drm_dp_cec_configure_adapter is called and
   it detects the CEC capability in the DPCD. It now creates the CEC device.
3) I disconnect the HDMI cable: the physical address of the CEC device is invalidated,
   but the CEC device is not removed.
4) I disconnect the adapter: nothing happens.
5) I reconnect the adapter: nothing happens.
6) I reconnect the HDMI cable: drm_dp_cec_configure_adapter is called and it checks the
   DPCD. If the capabilities are unchanged, then it will continue to use the registered
   CEC device. If the capabilities have changed (i.e. the adapter is apparently a
   different one), then the CEC device is unregistered and a new one is created, provided
   that the new adapter supports CEC, of course.

The bottom-line is that I cannot tell the difference between disconnecting the adapter
and disconnecting the HDMI cable to the adapter.

Another consideration is that CEC applications (i.e. HTPCs) do not expect the CEC device
to disappear when you disconnect either the HDMI cable or the adapter. Even though the
CEC device is strictly speaking associated with the adapter, from the point of view of
the user there is no difference between disconnecting an HDMI cable from an adapter, or
disconnecting the adapter itself. So there is a good argument to be made to only unregister
the CEC device when a different (or no) adapter is detected the next time the HPD goes high.

Regards,

	Hans

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

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

[Ville Syrjälä]

On Mon, Sep 18, 2017 at 04:07:41PM +0200, Hans Verkuil wrote:
> Hi Ville,
> 
> On 09/18/2017 03:05 PM, Ville Syrjälä wrote:
> > On Sat, Sep 16, 2017 at 04:17:26PM +0200, Hans Verkuil wrote:
> >> 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);
> > 
> > This is cluttering up the code a bit. Maybe do this call somewhere
> > around the intel_dp_configure_mst() call instead since that seems to be
> > the place where we start to do changes to externally visible state.
> > 
> > Actually, do we want to register cec adapters for MST devices?
> > 
> > And shouldn't we call this regardless of the connector state so that
> > the cec adapter gets unregistered when the device is disconnected?
> 
> This hasn't (AFAIK) anything to do with MST. This is in a branch device (i.e.
> a DP to HDMI adapter).

You are now potentiall registering the CEC adapter to the immediately
upstream MST device (ie. the one that we talk to over the normal AUX stuff),
but kms will consider that paticular connector as disconnected, and
instead only sinks downstream of that device may have connected connectors
associated with them. Presumably the CEC towards that device goes
nowhere, and instead we'd have to talk to the remote branch devices
somewhere downstream.

Thus my question whether we want to potentially register the CEC adapter
to the immediately upstream MST device or not. I would imagine not, and
thus the call should perhaps be moved past the 'is_mst? -> disconnected'
checks.

> 
> The CEC adapter should ideally be associated with the branch device (since that
> is what implements the CEC tunneling): i.e. when you connect the adapter, then
> the CEC device is created, when you disconnect the adapter, then the CEC device
> should be unregistered. This is not the same as connecting/disconnecting the
> HDMI cable to/from the adapter: that just sets or invalidates the CEC physical
> address (which is read from the EDID).
> 
> However, I have not seen any code that tells me when the adapter is plugged in
> or is unplugged. So all I have to go on is when the HDMI cable is connected.
> 
> Note that the 'late_register' you mentioned in your 1/3 review isn't called when
> connecting the adapter. So that too cannot be used as a trigger to detect if
> this protocol is supported.

Like I said, you should do the registration directly if the connector
has already been registered, otherwise defer to .late_register().

> 
> I know doing this here is not ideal, but I have not found another way and I am
> not even certain if it is possible at all, it might be intrinsic to how DP works.
> I do not consider myself a DP expert, though.
> 
> So this is how it works now, e.g. on my Intel NUC and a USB-C to HDMI adapter:
> 
> 1) I connect the adapter (no HDMI connected yet): nothing happens.
> 2) I connect the HDMI cable to the adapter: drm_dp_cec_configure_adapter is called and
>    it detects the CEC capability in the DPCD. It now creates the CEC device.
> 3) I disconnect the HDMI cable: the physical address of the CEC device is invalidated,
>    but the CEC device is not removed.
> 4) I disconnect the adapter: nothing happens.
> 5) I reconnect the adapter: nothing happens.
> 6) I reconnect the HDMI cable: drm_dp_cec_configure_adapter is called and it checks the
>    DPCD. If the capabilities are unchanged, then it will continue to use the registered
>    CEC device. If the capabilities have changed (i.e. the adapter is apparently a
>    different one), then the CEC device is unregistered and a new one is created, provided
>    that the new adapter supports CEC, of course.
> 
> The bottom-line is that I cannot tell the difference between disconnecting the adapter
> and disconnecting the HDMI cable to the adapter.
> 
> Another consideration is that CEC applications (i.e. HTPCs) do not expect the CEC device
> to disappear when you disconnect either the HDMI cable or the adapter. Even though the
> CEC device is strictly speaking associated with the adapter, from the point of view of
> the user there is no difference between disconnecting an HDMI cable from an adapter, or
> disconnecting the adapter itself. So there is a good argument to be made to only unregister
> the CEC device when a different (or no) adapter is detected the next time the HPD goes high.

Should we just register a CEC adapter always then? Seems rather
inconsistent to do it only when a CEC capable device gets plugged in but
then leave it lingering around when the device gets disconnected.

> 
> Regards,
> 
> 	Hans
> 
> > 
> >> +	} 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
> > 

-- 
Ville Syrjälä
Intel OTC

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

[Hans Verkuil]

On 09/18/2017 04:36 PM, Ville Syrjälä wrote:
> On Mon, Sep 18, 2017 at 04:07:41PM +0200, Hans Verkuil wrote:
>> Hi Ville,
>>
>> On 09/18/2017 03:05 PM, Ville Syrjälä wrote:
>>> On Sat, Sep 16, 2017 at 04:17:26PM +0200, Hans Verkuil wrote:
>>>> 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);
>>>
>>> This is cluttering up the code a bit. Maybe do this call somewhere
>>> around the intel_dp_configure_mst() call instead since that seems to be
>>> the place where we start to do changes to externally visible state.
>>>
>>> Actually, do we want to register cec adapters for MST devices?
>>>
>>> And shouldn't we call this regardless of the connector state so that
>>> the cec adapter gets unregistered when the device is disconnected?
>>
>> This hasn't (AFAIK) anything to do with MST. This is in a branch device (i.e.
>> a DP to HDMI adapter).
> 
> You are now potentiall registering the CEC adapter to the immediately
> upstream MST device (ie. the one that we talk to over the normal AUX stuff),
> but kms will consider that paticular connector as disconnected, and
> instead only sinks downstream of that device may have connected connectors
> associated with them. Presumably the CEC towards that device goes
> nowhere, and instead we'd have to talk to the remote branch devices
> somewhere downstream.
> 
> Thus my question whether we want to potentially register the CEC adapter
> to the immediately upstream MST device or not. I would imagine not, and
> thus the call should perhaps be moved past the 'is_mst? -> disconnected'
> checks.

Ah, now I see what you mean. Sorry, I misunderstood you earlier. I can
certainly move it down. But an MST device would never set the CEC capability
in the DPCD, would it? That makes no sense. So it would never register a
CEC device in practice. Although I do need to test what happens when you
first connect a USB-C to HDMI adapter that supports CEC, then disconnect it,
then connect an MST hub. The CEC device should be unregistered in that case,
but I'm not sure if that actually happens. I'll have to test that tomorrow.

> 
>>
>> The CEC adapter should ideally be associated with the branch device (since that
>> is what implements the CEC tunneling): i.e. when you connect the adapter, then
>> the CEC device is created, when you disconnect the adapter, then the CEC device
>> should be unregistered. This is not the same as connecting/disconnecting the
>> HDMI cable to/from the adapter: that just sets or invalidates the CEC physical
>> address (which is read from the EDID).
>>
>> However, I have not seen any code that tells me when the adapter is plugged in
>> or is unplugged. So all I have to go on is when the HDMI cable is connected.
>>
>> Note that the 'late_register' you mentioned in your 1/3 review isn't called when
>> connecting the adapter. So that too cannot be used as a trigger to detect if
>> this protocol is supported.
> 
> Like I said, you should do the registration directly if the connector
> has already been registered, otherwise defer to .late_register().

So intel_dp_long_pulse() can be called even if the connector hasn't been fully
registered yet? Just making sure I really understand this.

> 
>>
>> I know doing this here is not ideal, but I have not found another way and I am
>> not even certain if it is possible at all, it might be intrinsic to how DP works.
>> I do not consider myself a DP expert, though.
>>
>> So this is how it works now, e.g. on my Intel NUC and a USB-C to HDMI adapter:
>>
>> 1) I connect the adapter (no HDMI connected yet): nothing happens.
>> 2) I connect the HDMI cable to the adapter: drm_dp_cec_configure_adapter is called and
>>    it detects the CEC capability in the DPCD. It now creates the CEC device.
>> 3) I disconnect the HDMI cable: the physical address of the CEC device is invalidated,
>>    but the CEC device is not removed.
>> 4) I disconnect the adapter: nothing happens.
>> 5) I reconnect the adapter: nothing happens.
>> 6) I reconnect the HDMI cable: drm_dp_cec_configure_adapter is called and it checks the
>>    DPCD. If the capabilities are unchanged, then it will continue to use the registered
>>    CEC device. If the capabilities have changed (i.e. the adapter is apparently a
>>    different one), then the CEC device is unregistered and a new one is created, provided
>>    that the new adapter supports CEC, of course.
>>
>> The bottom-line is that I cannot tell the difference between disconnecting the adapter
>> and disconnecting the HDMI cable to the adapter.
>>
>> Another consideration is that CEC applications (i.e. HTPCs) do not expect the CEC device
>> to disappear when you disconnect either the HDMI cable or the adapter. Even though the
>> CEC device is strictly speaking associated with the adapter, from the point of view of
>> the user there is no difference between disconnecting an HDMI cable from an adapter, or
>> disconnecting the adapter itself. So there is a good argument to be made to only unregister
>> the CEC device when a different (or no) adapter is detected the next time the HPD goes high.
> 
> Should we just register a CEC adapter always then? Seems rather
> inconsistent to do it only when a CEC capable device gets plugged in but
> then leave it lingering around when the device gets disconnected.

I thought about that, but that would clutter /dev with lots of non-functioning CEC
device nodes. CEC only becomes available if you have an HDMI adapter that actually
supports this. Most do not. And of course if you connect to a display using a DP
cable (i.e. without an HDMI adapter), then it is obviously not supported either.

Another problem is that if you switch between adapters with different CEC capabilities,
then there is no way to signal that to userspace in the CEC API, and I don't think
that would be a good idea at all anyway.

Having experimented with this for some time now I found that this is a good compromise
that fits how this is used in practice.

Regards,

	Hans

> 
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>>> +	} 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
>>>
> 

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

[Ville Syrjälä]

On Mon, Sep 18, 2017 at 05:26:43PM +0200, Hans Verkuil wrote:
> On 09/18/2017 04:36 PM, Ville Syrjälä wrote:
> > On Mon, Sep 18, 2017 at 04:07:41PM +0200, Hans Verkuil wrote:
> >> Hi Ville,
> >>
> >> On 09/18/2017 03:05 PM, Ville Syrjälä wrote:
> >>> On Sat, Sep 16, 2017 at 04:17:26PM +0200, Hans Verkuil wrote:
> >>>> 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);
> >>>
> >>> This is cluttering up the code a bit. Maybe do this call somewhere
> >>> around the intel_dp_configure_mst() call instead since that seems to be
> >>> the place where we start to do changes to externally visible state.
> >>>
> >>> Actually, do we want to register cec adapters for MST devices?
> >>>
> >>> And shouldn't we call this regardless of the connector state so that
> >>> the cec adapter gets unregistered when the device is disconnected?
> >>
> >> This hasn't (AFAIK) anything to do with MST. This is in a branch device (i.e.
> >> a DP to HDMI adapter).
> > 
> > You are now potentiall registering the CEC adapter to the immediately
> > upstream MST device (ie. the one that we talk to over the normal AUX stuff),
> > but kms will consider that paticular connector as disconnected, and
> > instead only sinks downstream of that device may have connected connectors
> > associated with them. Presumably the CEC towards that device goes
> > nowhere, and instead we'd have to talk to the remote branch devices
> > somewhere downstream.
> > 
> > Thus my question whether we want to potentially register the CEC adapter
> > to the immediately upstream MST device or not. I would imagine not, and
> > thus the call should perhaps be moved past the 'is_mst? -> disconnected'
> > checks.
> 
> Ah, now I see what you mean. Sorry, I misunderstood you earlier. I can
> certainly move it down. But an MST device would never set the CEC capability
> in the DPCD, would it?

It might. You don't even have to drive it as an MST device if you don't
want to, or you source device might not even be capable of MST in which
case you have not choice but to drive it in SST mode.

> That makes no sense. So it would never register a
> CEC device in practice. Although I do need to test what happens when you
> first connect a USB-C to HDMI adapter that supports CEC, then disconnect it,
> then connect an MST hub. The CEC device should be unregistered in that case,
> but I'm not sure if that actually happens. I'll have to test that tomorrow.
> 
> > 
> >>
> >> The CEC adapter should ideally be associated with the branch device (since that
> >> is what implements the CEC tunneling): i.e. when you connect the adapter, then
> >> the CEC device is created, when you disconnect the adapter, then the CEC device
> >> should be unregistered. This is not the same as connecting/disconnecting the
> >> HDMI cable to/from the adapter: that just sets or invalidates the CEC physical
> >> address (which is read from the EDID).
> >>
> >> However, I have not seen any code that tells me when the adapter is plugged in
> >> or is unplugged. So all I have to go on is when the HDMI cable is connected.
> >>
> >> Note that the 'late_register' you mentioned in your 1/3 review isn't called when
> >> connecting the adapter. So that too cannot be used as a trigger to detect if
> >> this protocol is supported.
> > 
> > Like I said, you should do the registration directly if the connector
> > has already been registered, otherwise defer to .late_register().
> 
> So intel_dp_long_pulse() can be called even if the connector hasn't been fully
> registered yet? Just making sure I really understand this.

Yeah, we enable hotplug processing slightly before registering
everything.

> 
> > 
> >>
> >> I know doing this here is not ideal, but I have not found another way and I am
> >> not even certain if it is possible at all, it might be intrinsic to how DP works.
> >> I do not consider myself a DP expert, though.
> >>
> >> So this is how it works now, e.g. on my Intel NUC and a USB-C to HDMI adapter:
> >>
> >> 1) I connect the adapter (no HDMI connected yet): nothing happens.
> >> 2) I connect the HDMI cable to the adapter: drm_dp_cec_configure_adapter is called and
> >>    it detects the CEC capability in the DPCD. It now creates the CEC device.
> >> 3) I disconnect the HDMI cable: the physical address of the CEC device is invalidated,
> >>    but the CEC device is not removed.
> >> 4) I disconnect the adapter: nothing happens.
> >> 5) I reconnect the adapter: nothing happens.
> >> 6) I reconnect the HDMI cable: drm_dp_cec_configure_adapter is called and it checks the
> >>    DPCD. If the capabilities are unchanged, then it will continue to use the registered
> >>    CEC device. If the capabilities have changed (i.e. the adapter is apparently a
> >>    different one), then the CEC device is unregistered and a new one is created, provided
> >>    that the new adapter supports CEC, of course.
> >>
> >> The bottom-line is that I cannot tell the difference between disconnecting the adapter
> >> and disconnecting the HDMI cable to the adapter.
> >>
> >> Another consideration is that CEC applications (i.e. HTPCs) do not expect the CEC device
> >> to disappear when you disconnect either the HDMI cable or the adapter. Even though the
> >> CEC device is strictly speaking associated with the adapter, from the point of view of
> >> the user there is no difference between disconnecting an HDMI cable from an adapter, or
> >> disconnecting the adapter itself. So there is a good argument to be made to only unregister
> >> the CEC device when a different (or no) adapter is detected the next time the HPD goes high.
> > 
> > Should we just register a CEC adapter always then? Seems rather
> > inconsistent to do it only when a CEC capable device gets plugged in but
> > then leave it lingering around when the device gets disconnected.
> 
> I thought about that, but that would clutter /dev with lots of non-functioning CEC
> device nodes. CEC only becomes available if you have an HDMI adapter that actually
> supports this. Most do not. And of course if you connect to a display using a DP
> cable (i.e. without an HDMI adapter), then it is obviously not supported either.
> 
> Another problem is that if you switch between adapters with different CEC capabilities,
> then there is no way to signal that to userspace in the CEC API, and I don't think
> that would be a good idea at all anyway.
> 
> Having experimented with this for some time now I found that this is a good compromise
> that fits how this is used in practice.

But what you have means userspace will still have to be prepared for the
adapter to disappear at any point, so I don't really see what we gain by
deferring the unregistration.

-- 
Ville Syrjälä
Intel OTC

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

[Hans Verkuil]

Hi Ville,

On 18/09/17 17:49, Ville Syrjälä wrote:
> On Mon, Sep 18, 2017 at 05:26:43PM +0200, Hans Verkuil wrote:
>> On 09/18/2017 04:36 PM, Ville Syrjälä wrote:
>>> On Mon, Sep 18, 2017 at 04:07:41PM +0200, Hans Verkuil wrote:
>>>> Hi Ville,
>>>>
>>>> On 09/18/2017 03:05 PM, Ville Syrjälä wrote:
>>>>> On Sat, Sep 16, 2017 at 04:17:26PM +0200, Hans Verkuil wrote:
>>>>>> 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);
>>>>>
>>>>> This is cluttering up the code a bit. Maybe do this call somewhere
>>>>> around the intel_dp_configure_mst() call instead since that seems to be
>>>>> the place where we start to do changes to externally visible state.
>>>>>
>>>>> Actually, do we want to register cec adapters for MST devices?
>>>>>
>>>>> And shouldn't we call this regardless of the connector state so that
>>>>> the cec adapter gets unregistered when the device is disconnected?
>>>>
>>>> This hasn't (AFAIK) anything to do with MST. This is in a branch device (i.e.
>>>> a DP to HDMI adapter).
>>>
>>> You are now potentiall registering the CEC adapter to the immediately
>>> upstream MST device (ie. the one that we talk to over the normal AUX stuff),
>>> but kms will consider that paticular connector as disconnected, and
>>> instead only sinks downstream of that device may have connected connectors
>>> associated with them. Presumably the CEC towards that device goes
>>> nowhere, and instead we'd have to talk to the remote branch devices
>>> somewhere downstream.
>>>
>>> Thus my question whether we want to potentially register the CEC adapter
>>> to the immediately upstream MST device or not. I would imagine not, and
>>> thus the call should perhaps be moved past the 'is_mst? -> disconnected'
>>> checks.
>>
>> Ah, now I see what you mean. Sorry, I misunderstood you earlier. I can
>> certainly move it down. But an MST device would never set the CEC capability
>> in the DPCD, would it?
> 
> It might. You don't even have to drive it as an MST device if you don't
> want to, or you source device might not even be capable of MST in which
> case you have not choice but to drive it in SST mode.

I really don't think it can set the CEC capability but I will have to dig
deeper into the spec to be 100% certain of that.

<snip>

>>>> I know doing this here is not ideal, but I have not found another way and I am
>>>> not even certain if it is possible at all, it might be intrinsic to how DP works.
>>>> I do not consider myself a DP expert, though.
>>>>
>>>> So this is how it works now, e.g. on my Intel NUC and a USB-C to HDMI adapter:
>>>>
>>>> 1) I connect the adapter (no HDMI connected yet): nothing happens.
>>>> 2) I connect the HDMI cable to the adapter: drm_dp_cec_configure_adapter is called and
>>>>    it detects the CEC capability in the DPCD. It now creates the CEC device.
>>>> 3) I disconnect the HDMI cable: the physical address of the CEC device is invalidated,
>>>>    but the CEC device is not removed.
>>>> 4) I disconnect the adapter: nothing happens.
>>>> 5) I reconnect the adapter: nothing happens.
>>>> 6) I reconnect the HDMI cable: drm_dp_cec_configure_adapter is called and it checks the
>>>>    DPCD. If the capabilities are unchanged, then it will continue to use the registered
>>>>    CEC device. If the capabilities have changed (i.e. the adapter is apparently a
>>>>    different one), then the CEC device is unregistered and a new one is created, provided
>>>>    that the new adapter supports CEC, of course.
>>>>
>>>> The bottom-line is that I cannot tell the difference between disconnecting the adapter
>>>> and disconnecting the HDMI cable to the adapter.
>>>>
>>>> Another consideration is that CEC applications (i.e. HTPCs) do not expect the CEC device
>>>> to disappear when you disconnect either the HDMI cable or the adapter. Even though the
>>>> CEC device is strictly speaking associated with the adapter, from the point of view of
>>>> the user there is no difference between disconnecting an HDMI cable from an adapter, or
>>>> disconnecting the adapter itself. So there is a good argument to be made to only unregister
>>>> the CEC device when a different (or no) adapter is detected the next time the HPD goes high.
>>>
>>> Should we just register a CEC adapter always then? Seems rather
>>> inconsistent to do it only when a CEC capable device gets plugged in but
>>> then leave it lingering around when the device gets disconnected.
>>
>> I thought about that, but that would clutter /dev with lots of non-functioning CEC
>> device nodes. CEC only becomes available if you have an HDMI adapter that actually
>> supports this. Most do not. And of course if you connect to a display using a DP
>> cable (i.e. without an HDMI adapter), then it is obviously not supported either.
>>
>> Another problem is that if you switch between adapters with different CEC capabilities,
>> then there is no way to signal that to userspace in the CEC API, and I don't think
>> that would be a good idea at all anyway.
>>
>> Having experimented with this for some time now I found that this is a good compromise
>> that fits how this is used in practice.
> 
> But what you have means userspace will still have to be prepared for the
> adapter to disappear at any point, so I don't really see what we gain by
> deferring the unregistration.

There are two situations: one is that the DP-to-HDMI adapter is integrated in
the hardware (such as on an Intel NUC with an HDMI output). In this case keeping the
cec device is exactly what you want.

The second situation is that the adapter is a separate dongle that can be
removed. Now keeping the cec device is almost always what you want, unless
the user is trying different adapters with different capabilities. In that
case CEC devices can indeed be unregistered. But you really have different
hardware in that case.

BTW, I've just tested a USB-C to dual DP MST hub and connecting a DP-to-HDMI
adapter with CEC support to see what happens, and the cec device did not
come up. So I need to do a bit more digging to understand why not. Since I
have a conference this week and am on vacation next week this might take some
time before I get around to it.

Regards,

	Hans