* [PATCH 1/5] dp/dp_mst: Add support for sink event notify messages
@ 2020-09-01 6:22 Sam McNally
2020-09-01 6:22 ` [PATCH 2/5] drm_dp_mst_topology: use correct AUX channel Sam McNally
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Sam McNally @ 2020-09-01 6:22 UTC (permalink / raw)
To: LKML
Cc: Hans Verkuil, Sam McNally, Daniel Vetter, David Airlie,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, dri-devel
Sink event notify messages are used for MST CEC IRQs. Add parsing
support for sink event notify messages in preparation for handling MST
CEC IRQs.
Signed-off-by: Sam McNally <sammc@chromium.org>
---
drivers/gpu/drm/drm_dp_mst_topology.c | 37 ++++++++++++++++++++++++++-
include/drm/drm_dp_mst_helper.h | 14 ++++++++++
2 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 17dbed0a9800..15b6cc39a754 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1027,6 +1027,30 @@ static bool drm_dp_sideband_parse_resource_status_notify(struct drm_dp_sideband_
return false;
}
+static bool drm_dp_sideband_parse_sink_event_notify(
+ struct drm_dp_sideband_msg_rx *raw,
+ struct drm_dp_sideband_msg_req_body *msg)
+{
+ int idx = 1;
+
+ msg->u.sink_event.port_number = (raw->msg[idx] & 0xf0) >> 4;
+ idx++;
+ if (idx > raw->curlen)
+ goto fail_len;
+
+ memcpy(msg->u.sink_event.guid, &raw->msg[idx], 16);
+ idx += 16;
+ if (idx > raw->curlen)
+ goto fail_len;
+
+ msg->u.sink_event.event_id = (raw->msg[idx] << 8) | (raw->msg[idx + 1]);
+ idx++;
+ return true;
+fail_len:
+ DRM_DEBUG_KMS("sink event notify parse length fail %d %d\n", idx, raw->curlen);
+ return false;
+}
+
static bool drm_dp_sideband_parse_req(struct drm_dp_sideband_msg_rx *raw,
struct drm_dp_sideband_msg_req_body *msg)
{
@@ -1038,6 +1062,8 @@ static bool drm_dp_sideband_parse_req(struct drm_dp_sideband_msg_rx *raw,
return drm_dp_sideband_parse_connection_status_notify(raw, msg);
case DP_RESOURCE_STATUS_NOTIFY:
return drm_dp_sideband_parse_resource_status_notify(raw, msg);
+ case DP_SINK_EVENT_NOTIFY:
+ return drm_dp_sideband_parse_sink_event_notify(raw, msg);
default:
DRM_ERROR("Got unknown request 0x%02x (%s)\n", msg->req_type,
drm_dp_mst_req_type_str(msg->req_type));
@@ -3875,6 +3901,8 @@ drm_dp_mst_process_up_req(struct drm_dp_mst_topology_mgr *mgr,
guid = msg->u.conn_stat.guid;
else if (msg->req_type == DP_RESOURCE_STATUS_NOTIFY)
guid = msg->u.resource_stat.guid;
+ else if (msg->req_type == DP_SINK_EVENT_NOTIFY)
+ guid = msg->u.sink_event.guid;
if (guid)
mstb = drm_dp_get_mst_branch_device_by_guid(mgr, guid);
@@ -3948,7 +3976,8 @@ static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr)
drm_dp_sideband_parse_req(&mgr->up_req_recv, &up_req->msg);
if (up_req->msg.req_type != DP_CONNECTION_STATUS_NOTIFY &&
- up_req->msg.req_type != DP_RESOURCE_STATUS_NOTIFY) {
+ up_req->msg.req_type != DP_RESOURCE_STATUS_NOTIFY &&
+ up_req->msg.req_type != DP_SINK_EVENT_NOTIFY) {
DRM_DEBUG_KMS("Received unknown up req type, ignoring: %x\n",
up_req->msg.req_type);
kfree(up_req);
@@ -3976,6 +4005,12 @@ static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr)
DRM_DEBUG_KMS("Got RSN: pn: %d avail_pbn %d\n",
res_stat->port_number,
res_stat->available_pbn);
+ } else if (up_req->msg.req_type == DP_SINK_EVENT_NOTIFY) {
+ const struct drm_dp_sink_event_notify *sink_event =
+ &up_req->msg.u.sink_event;
+
+ DRM_DEBUG_KMS("Got SEN: pn: %d event_id %d\n",
+ sink_event->port_number, sink_event->event_id);
}
up_req->hdr = mgr->up_req_recv.initial_hdr;
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 6ae5860d8644..c7c79e0ced18 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -402,6 +402,19 @@ struct drm_dp_resource_status_notify {
u16 available_pbn;
};
+#define DP_SINK_EVENT_PANEL_REPLAY_ACTIVE_FRAME_CRC_ERROR BIT(0)
+#define DP_SINK_EVENT_PANEL_REPLAY_RFB_STORAGE_ERROR BIT(1)
+#define DP_SINK_EVENT_DSC_RC_BUFFER_UNDER_RUN BIT(2)
+#define DP_SINK_EVENT_DSC_RC_BUFFER_OVERFLOW BIT(3)
+#define DP_SINK_EVENT_DSC_CHUNK_LENGTH_ERROR BIT(4)
+#define DP_SINK_EVENT_CEC_IRQ_EVENT BIT(5)
+
+struct drm_dp_sink_event_notify {
+ u8 port_number;
+ u8 guid[16];
+ u16 event_id;
+};
+
struct drm_dp_query_payload_ack_reply {
u8 port_number;
u16 allocated_pbn;
@@ -413,6 +426,7 @@ struct drm_dp_sideband_msg_req_body {
struct drm_dp_connection_status_notify conn_stat;
struct drm_dp_port_number_req port_num;
struct drm_dp_resource_status_notify resource_stat;
+ struct drm_dp_sink_event_notify sink_event;
struct drm_dp_query_payload query_payload;
struct drm_dp_allocate_payload allocate_payload;
--
2.28.0.402.g5ffc5be6b7-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/5] drm_dp_mst_topology: use correct AUX channel
2020-09-01 6:22 [PATCH 1/5] dp/dp_mst: Add support for sink event notify messages Sam McNally
@ 2020-09-01 6:22 ` Sam McNally
2020-09-01 6:22 ` [PATCH 3/5] drm_dp_mst_topology: export two functions Sam McNally
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: Sam McNally @ 2020-09-01 6:22 UTC (permalink / raw)
To: LKML
Cc: Hans Verkuil, Sam McNally, Daniel Vetter, David Airlie,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, dri-devel
From: Hans Verkuil <hans.verkuil@cisco.com>
For adapters behind an MST hub use the correct AUX channel.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
[sammc@chromium.org: rebased, removing redundant changes]
Signed-off-by: Sam McNally <sammc@chromium.org>
---
drivers/gpu/drm/drm_dp_mst_topology.c | 36 +++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 15b6cc39a754..0d753201adbd 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2255,6 +2255,9 @@ drm_dp_mst_topology_unlink_port(struct drm_dp_mst_topology_mgr *mgr,
drm_dp_mst_topology_put_port(port);
}
+static ssize_t
+drm_dp_mst_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg);
+
static struct drm_dp_mst_port *
drm_dp_mst_add_port(struct drm_device *dev,
struct drm_dp_mst_topology_mgr *mgr,
@@ -2271,9 +2274,13 @@ drm_dp_mst_add_port(struct drm_device *dev,
port->port_num = port_number;
port->mgr = mgr;
port->aux.name = "DPMST";
+ mutex_init(&port->aux.hw_mutex);
+ mutex_init(&port->aux.cec.lock);
port->aux.dev = dev->dev;
port->aux.is_remote = true;
+ port->aux.transfer = drm_dp_mst_aux_transfer;
+
/* initialize the MST downstream port's AUX crc work queue */
drm_dp_remote_aux_init(&port->aux);
@@ -3503,6 +3510,35 @@ static int drm_dp_send_up_ack_reply(struct drm_dp_mst_topology_mgr *mgr,
return 0;
}
+static ssize_t
+drm_dp_mst_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
+{
+ struct drm_dp_mst_port *port =
+ container_of(aux, struct drm_dp_mst_port, aux);
+ int ret;
+
+ switch (msg->request & ~DP_AUX_I2C_MOT) {
+ case DP_AUX_NATIVE_WRITE:
+ case DP_AUX_I2C_WRITE:
+ case DP_AUX_I2C_WRITE_STATUS_UPDATE:
+ ret = drm_dp_send_dpcd_write(port->mgr, port, msg->address,
+ msg->size, msg->buffer);
+ break;
+
+ case DP_AUX_NATIVE_READ:
+ case DP_AUX_I2C_READ:
+ ret = drm_dp_send_dpcd_read(port->mgr, port, msg->address,
+ msg->size, msg->buffer);
+ break;
+
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ return ret;
+}
+
static int drm_dp_get_vc_payload_bw(u8 dp_link_bw, u8 dp_link_count)
{
if (dp_link_bw == 0 || dp_link_count == 0)
--
2.28.0.402.g5ffc5be6b7-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/5] drm_dp_mst_topology: export two functions
2020-09-01 6:22 [PATCH 1/5] dp/dp_mst: Add support for sink event notify messages Sam McNally
2020-09-01 6:22 ` [PATCH 2/5] drm_dp_mst_topology: use correct AUX channel Sam McNally
@ 2020-09-01 6:22 ` Sam McNally
2020-09-01 6:22 ` [PATCH 4/5] drm_dp_cec: add plumbing in preparation for MST support Sam McNally
2020-09-01 6:22 ` [PATCH 5/5] drm_dp_cec: add the implementation of " Sam McNally
3 siblings, 0 replies; 12+ messages in thread
From: Sam McNally @ 2020-09-01 6:22 UTC (permalink / raw)
To: LKML
Cc: Hans Verkuil, Sam McNally, Daniel Vetter, David Airlie,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, dri-devel
From: Hans Verkuil <hans.verkuil@cisco.com>
These are required for the CEC MST support.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Sam McNally <sammc@chromium.org>
---
drivers/gpu/drm/drm_dp_mst_topology.c | 6 ++----
include/drm/drm_dp_mst_helper.h | 4 ++++
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 0d753201adbd..c783a2a1c114 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -62,8 +62,6 @@ struct drm_dp_pending_up_req {
static bool dump_dp_payload_table(struct drm_dp_mst_topology_mgr *mgr,
char *buf);
-static void drm_dp_mst_topology_put_port(struct drm_dp_mst_port *port);
-
static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr,
int id,
struct drm_dp_payload *payload);
@@ -1864,7 +1862,7 @@ static void drm_dp_mst_topology_get_port(struct drm_dp_mst_port *port)
* drm_dp_mst_topology_try_get_port()
* drm_dp_mst_topology_get_port()
*/
-static void drm_dp_mst_topology_put_port(struct drm_dp_mst_port *port)
+void drm_dp_mst_topology_put_port(struct drm_dp_mst_port *port)
{
topology_ref_history_lock(port->mgr);
@@ -1935,7 +1933,7 @@ drm_dp_mst_topology_get_port_validated_locked(struct drm_dp_mst_branch *mstb,
return NULL;
}
-static struct drm_dp_mst_port *
+struct drm_dp_mst_port *
drm_dp_mst_topology_get_port_validated(struct drm_dp_mst_topology_mgr *mgr,
struct drm_dp_mst_port *port)
{
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index c7c79e0ced18..d036222e0d64 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -754,6 +754,10 @@ drm_dp_mst_detect_port(struct drm_connector *connector,
struct drm_dp_mst_topology_mgr *mgr,
struct drm_dp_mst_port *port);
+struct drm_dp_mst_port *drm_dp_mst_topology_get_port_validated
+(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);
+void drm_dp_mst_topology_put_port(struct drm_dp_mst_port *port);
+
struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);
--
2.28.0.402.g5ffc5be6b7-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 4/5] drm_dp_cec: add plumbing in preparation for MST support
2020-09-01 6:22 [PATCH 1/5] dp/dp_mst: Add support for sink event notify messages Sam McNally
2020-09-01 6:22 ` [PATCH 2/5] drm_dp_mst_topology: use correct AUX channel Sam McNally
2020-09-01 6:22 ` [PATCH 3/5] drm_dp_mst_topology: export two functions Sam McNally
@ 2020-09-01 6:22 ` Sam McNally
2020-09-01 18:12 ` Lyude Paul
2020-09-08 8:41 ` Hans Verkuil
2020-09-01 6:22 ` [PATCH 5/5] drm_dp_cec: add the implementation of " Sam McNally
3 siblings, 2 replies; 12+ messages in thread
From: Sam McNally @ 2020-09-01 6:22 UTC (permalink / raw)
To: LKML
Cc: Hans Verkuil, Sam McNally, Alex Deucher, Ben Skeggs,
Christian König, Daniel Vetter, David Airlie, David Francis,
Gwan-gyeong Mun, Harry Wentland, Imre Deak, Jani Nikula,
Joonas Lahtinen, José Roberto de Souza, Leo Li,
Lucas De Marchi, Lyude Paul, Maarten Lankhorst, Manasi Navare,
Matt Roper, Maxime Ripard, Mikita Lipski, Pankaj Bharadiya,
Rodrigo Siqueira, Rodrigo Vivi, Thomas Zimmermann,
Ville Syrjälä,
Wambui Karuga, amd-gfx, dri-devel, intel-gfx, nouveau
From: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
[sammc@chromium.org:
- rebased
- removed polling-related changes
- moved the calls to drm_dp_cec_(un)set_edid() into the next patch
]
Signed-off-by: Sam McNally <sammc@chromium.org>
---
.../display/amdgpu_dm/amdgpu_dm_mst_types.c | 2 +-
drivers/gpu/drm/drm_dp_cec.c | 22 ++++++++++---------
drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_connector.c | 2 +-
include/drm/drm_dp_helper.h | 6 +++--
5 files changed, 19 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 461fa4da0a34..6e7075893ec9 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -419,7 +419,7 @@ void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm,
drm_dp_aux_init(&aconnector->dm_dp_aux.aux);
drm_dp_cec_register_connector(&aconnector->dm_dp_aux.aux,
- &aconnector->base);
+ &aconnector->base, false);
if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
return;
diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
index 3ab2609f9ec7..04ab7b88055c 100644
--- a/drivers/gpu/drm/drm_dp_cec.c
+++ b/drivers/gpu/drm/drm_dp_cec.c
@@ -14,6 +14,7 @@
#include <drm/drm_connector.h>
#include <drm/drm_device.h>
#include <drm/drm_dp_helper.h>
+#include <drm/drm_dp_mst_helper.h>
/*
* Unfortunately it turns out that we have a chicken-and-egg situation
@@ -338,8 +339,6 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
if (aux->cec.adap) {
if (aux->cec.adap->capabilities == cec_caps &&
aux->cec.adap->available_log_addrs == num_las) {
- /* Unchanged, so just set the phys addr */
- cec_s_phys_addr_from_edid(aux->cec.adap, edid);
goto unlock;
}
/*
@@ -364,15 +363,16 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
if (cec_register_adapter(aux->cec.adap, connector->dev->dev)) {
cec_delete_adapter(aux->cec.adap);
aux->cec.adap = NULL;
- } else {
- /*
- * Update the phys addr for the new CEC adapter. When called
- * from drm_dp_cec_register_connector() edid == NULL, so in
- * that case the phys addr is just invalidated.
- */
- cec_s_phys_addr_from_edid(aux->cec.adap, edid);
}
unlock:
+ /*
+ * Update the phys addr for the new CEC adapter. When called
+ * from drm_dp_cec_register_connector() edid == NULL, so in
+ * that case the phys addr is just invalidated.
+ */
+ if (aux->cec.adap && edid) {
+ cec_s_phys_addr_from_edid(aux->cec.adap, edid);
+ }
mutex_unlock(&aux->cec.lock);
}
EXPORT_SYMBOL(drm_dp_cec_set_edid);
@@ -418,6 +418,7 @@ EXPORT_SYMBOL(drm_dp_cec_unset_edid);
* drm_dp_cec_register_connector() - register a new connector
* @aux: DisplayPort AUX channel
* @connector: drm connector
+ * @is_mst: set to true if this is an MST branch
*
* A new connector was registered with associated CEC adapter name and
* CEC adapter parent device. After registering the name and parent
@@ -425,12 +426,13 @@ EXPORT_SYMBOL(drm_dp_cec_unset_edid);
* CEC and to register a CEC adapter if that is the case.
*/
void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
- struct drm_connector *connector)
+ struct drm_connector *connector, bool is_mst)
{
WARN_ON(aux->cec.adap);
if (WARN_ON(!aux->transfer))
return;
aux->cec.connector = connector;
+ aux->cec.is_mst = is_mst;
INIT_DELAYED_WORK(&aux->cec.unregister_work,
drm_dp_cec_unregister_work);
}
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 82b9de274f65..744cb55572f9 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -6261,7 +6261,7 @@ intel_dp_connector_register(struct drm_connector *connector)
intel_dp->aux.dev = connector->kdev;
ret = drm_dp_aux_register(&intel_dp->aux);
if (!ret)
- drm_dp_cec_register_connector(&intel_dp->aux, connector);
+ drm_dp_cec_register_connector(&intel_dp->aux, connector, false);
return ret;
}
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 49dd0cbc332f..671a70e95cd1 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -1414,7 +1414,7 @@ nouveau_connector_create(struct drm_device *dev,
switch (type) {
case DRM_MODE_CONNECTOR_DisplayPort:
case DRM_MODE_CONNECTOR_eDP:
- drm_dp_cec_register_connector(&nv_connector->aux, connector);
+ drm_dp_cec_register_connector(&nv_connector->aux, connector, false);
break;
}
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 85513eeb2196..12bca1b9512b 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1495,12 +1495,14 @@ struct drm_connector;
* @lock: mutex protecting this struct
* @adap: the CEC adapter for CEC-Tunneling-over-AUX support.
* @connector: the connector this CEC adapter is associated with
+ * @is_mst: this is an MST branch
* @unregister_work: unregister the CEC adapter
*/
struct drm_dp_aux_cec {
struct mutex lock;
struct cec_adapter *adap;
struct drm_connector *connector;
+ bool is_mst;
struct delayed_work unregister_work;
};
@@ -1746,7 +1748,7 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc, u32 edid_quirks,
#ifdef CONFIG_DRM_DP_CEC
void drm_dp_cec_irq(struct drm_dp_aux *aux);
void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
- struct drm_connector *connector);
+ struct drm_connector *connector, bool is_mst);
void drm_dp_cec_unregister_connector(struct drm_dp_aux *aux);
void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid);
void drm_dp_cec_unset_edid(struct drm_dp_aux *aux);
@@ -1757,7 +1759,7 @@ static inline void drm_dp_cec_irq(struct drm_dp_aux *aux)
static inline void
drm_dp_cec_register_connector(struct drm_dp_aux *aux,
- struct drm_connector *connector)
+ struct drm_connector *connector, bool is_mst)
{
}
--
2.28.0.402.g5ffc5be6b7-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 5/5] drm_dp_cec: add the implementation of MST support
2020-09-01 6:22 [PATCH 1/5] dp/dp_mst: Add support for sink event notify messages Sam McNally
` (2 preceding siblings ...)
2020-09-01 6:22 ` [PATCH 4/5] drm_dp_cec: add plumbing in preparation for MST support Sam McNally
@ 2020-09-01 6:22 ` Sam McNally
2020-09-08 8:08 ` Hans Verkuil
2020-09-08 9:24 ` Hans Verkuil
3 siblings, 2 replies; 12+ messages in thread
From: Sam McNally @ 2020-09-01 6:22 UTC (permalink / raw)
To: LKML
Cc: Hans Verkuil, Sam McNally, Daniel Vetter, David Airlie,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, dri-devel
With DP v2.0 errata E5, CEC tunneling can be supported through an MST
topology.
There are some minor differences for CEC tunneling through an MST
topology compared to CEC tunneling to an SST port:
- CEC IRQs are delivered via a sink event notify message
- CEC-related DPCD registers are accessed via remote DPCD reads and
writes.
This results in the MST implementation diverging from the existing SST
implementation:
- sink event notify messages with CEC_IRQ ID set indicate CEC IRQ rather
than ESI1
- setting edid and handling CEC IRQs, which can be triggered from
contexts where locks held preclude HPD handling, are deferred to avoid
remote DPCD access which would block until HPD handling is performed
or a timeout
Register and unregister for all MST connectors, ensuring their
drm_dp_aux_cec struct won't be accessed uninitialized.
Signed-off-by: Sam McNally <sammc@chromium.org>
---
drivers/gpu/drm/drm_dp_cec.c | 67 +++++++++++++++++++++++++--
drivers/gpu/drm/drm_dp_mst_topology.c | 24 ++++++++++
include/drm/drm_dp_helper.h | 4 ++
3 files changed, 90 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
index 04ab7b88055c..9f6aeaa27f00 100644
--- a/drivers/gpu/drm/drm_dp_cec.c
+++ b/drivers/gpu/drm/drm_dp_cec.c
@@ -249,6 +249,10 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux)
if (!aux->transfer)
return;
+ if (aux->cec.is_mst) {
+ schedule_work(&aux->cec.mst_irq_work);
+ return;
+ }
mutex_lock(&aux->cec.lock);
if (!aux->cec.adap)
goto unlock;
@@ -277,6 +281,24 @@ static bool drm_dp_cec_cap(struct drm_dp_aux *aux, u8 *cec_cap)
return true;
}
+static void drm_dp_cec_mst_irq_work(struct work_struct *work)
+{
+ struct drm_dp_aux *aux = container_of(work, struct drm_dp_aux,
+ cec.mst_irq_work);
+ struct drm_dp_mst_port *port =
+ container_of(aux, struct drm_dp_mst_port, aux);
+
+ port = drm_dp_mst_topology_get_port_validated(port->mgr, port);
+ if (!port)
+ return;
+ mutex_lock(&aux->cec.lock);
+ if (aux->cec.adap)
+ drm_dp_cec_handle_irq(aux);
+
+ mutex_unlock(&aux->cec.lock);
+ drm_dp_mst_topology_put_port(port);
+}
+
/*
* Called if the HPD was low for more than drm_dp_cec_unregister_delay
* seconds. This unregisters the CEC adapter.
@@ -298,7 +320,8 @@ static void drm_dp_cec_unregister_work(struct work_struct *work)
* were unchanged and just update the CEC physical address. Otherwise
* unregister the old CEC adapter and create a new one.
*/
-void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
+static void drm_dp_cec_handle_set_edid(struct drm_dp_aux *aux,
+ const struct edid *edid)
{
struct drm_connector *connector = aux->cec.connector;
u32 cec_caps = CEC_CAP_DEFAULTS | CEC_CAP_NEEDS_HPD |
@@ -307,10 +330,6 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
unsigned int num_las = 1;
u8 cap;
- /* No transfer function was set, so not a DP connector */
- if (!aux->transfer)
- return;
-
#ifndef CONFIG_MEDIA_CEC_RC
/*
* CEC_CAP_RC is part of CEC_CAP_DEFAULTS, but it is stripped by
@@ -321,6 +340,7 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
*/
cec_caps &= ~CEC_CAP_RC;
#endif
+ cancel_work_sync(&aux->cec.mst_irq_work);
cancel_delayed_work_sync(&aux->cec.unregister_work);
mutex_lock(&aux->cec.lock);
@@ -375,6 +395,18 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
}
mutex_unlock(&aux->cec.lock);
}
+
+void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
+{
+ /* No transfer function was set, so not a DP connector */
+ if (!aux->transfer)
+ return;
+
+ if (aux->cec.is_mst)
+ schedule_work(&aux->cec.mst_set_edid_work);
+ else
+ drm_dp_cec_handle_set_edid(aux, edid);
+}
EXPORT_SYMBOL(drm_dp_cec_set_edid);
/*
@@ -393,6 +425,8 @@ void drm_dp_cec_unset_edid(struct drm_dp_aux *aux)
goto unlock;
cec_phys_addr_invalidate(aux->cec.adap);
+ cancel_work_sync(&aux->cec.mst_irq_work);
+
/*
* We're done if we want to keep the CEC device
* (drm_dp_cec_unregister_delay is >= NEVER_UNREG_DELAY) or if the
@@ -414,6 +448,25 @@ void drm_dp_cec_unset_edid(struct drm_dp_aux *aux)
}
EXPORT_SYMBOL(drm_dp_cec_unset_edid);
+static void drm_dp_cec_mst_set_edid_work(struct work_struct *work)
+{
+ struct drm_dp_aux *aux =
+ container_of(work, struct drm_dp_aux, cec.mst_set_edid_work);
+ struct drm_dp_mst_port *port =
+ container_of(aux, struct drm_dp_mst_port, aux);
+ struct edid *edid = NULL;
+
+ port = drm_dp_mst_topology_get_port_validated(port->mgr, port);
+ if (!port)
+ return;
+
+ edid = drm_get_edid(port->connector, &port->aux.ddc);
+
+ drm_dp_cec_handle_set_edid(aux, edid);
+
+ drm_dp_mst_topology_put_port(port);
+}
+
/**
* drm_dp_cec_register_connector() - register a new connector
* @aux: DisplayPort AUX channel
@@ -435,6 +488,8 @@ void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
aux->cec.is_mst = is_mst;
INIT_DELAYED_WORK(&aux->cec.unregister_work,
drm_dp_cec_unregister_work);
+ INIT_WORK(&aux->cec.mst_irq_work, drm_dp_cec_mst_irq_work);
+ INIT_WORK(&aux->cec.mst_set_edid_work, drm_dp_cec_mst_set_edid_work);
}
EXPORT_SYMBOL(drm_dp_cec_register_connector);
@@ -444,6 +499,8 @@ EXPORT_SYMBOL(drm_dp_cec_register_connector);
*/
void drm_dp_cec_unregister_connector(struct drm_dp_aux *aux)
{
+ cancel_work_sync(&aux->cec.mst_irq_work);
+ cancel_work_sync(&aux->cec.mst_set_edid_work);
if (!aux->cec.adap)
return;
cancel_delayed_work_sync(&aux->cec.unregister_work);
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index c783a2a1c114..fd9430d88fd6 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2183,6 +2183,8 @@ static void build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
int drm_dp_mst_connector_late_register(struct drm_connector *connector,
struct drm_dp_mst_port *port)
{
+ drm_dp_cec_register_connector(&port->aux, connector, true);
+
DRM_DEBUG_KMS("registering %s remote bus for %s\n",
port->aux.name, connector->kdev->kobj.name);
@@ -2206,6 +2208,8 @@ void drm_dp_mst_connector_early_unregister(struct drm_connector *connector,
DRM_DEBUG_KMS("unregistering %s remote bus for %s\n",
port->aux.name, connector->kdev->kobj.name);
drm_dp_aux_unregister_devnode(&port->aux);
+
+ drm_dp_cec_unregister_connector(&port->aux);
}
EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister);
@@ -2515,6 +2519,21 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
queue_work(system_long_wq, &mstb->mgr->work);
}
+static void
+drm_dp_mst_handle_sink_event(struct drm_dp_mst_branch *mstb,
+ struct drm_dp_sink_event_notify *sink_event)
+{
+ struct drm_dp_mst_port *port;
+
+ if (sink_event->event_id & DP_SINK_EVENT_CEC_IRQ_EVENT) {
+ port = drm_dp_get_port(mstb, sink_event->port_number);
+ if (port) {
+ drm_dp_cec_irq(&port->aux);
+ drm_dp_mst_topology_put_port(port);
+ }
+ }
+}
+
static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct drm_dp_mst_topology_mgr *mgr,
u8 lct, u8 *rad)
{
@@ -3954,6 +3973,8 @@ drm_dp_mst_process_up_req(struct drm_dp_mst_topology_mgr *mgr,
if (msg->req_type == DP_CONNECTION_STATUS_NOTIFY) {
drm_dp_mst_handle_conn_stat(mstb, &msg->u.conn_stat);
hotplug = true;
+ } else if (msg->req_type == DP_SINK_EVENT_NOTIFY) {
+ drm_dp_mst_handle_sink_event(mstb, &msg->u.sink_event);
}
drm_dp_mst_topology_put_mstb(mstb);
@@ -4147,6 +4168,8 @@ drm_dp_mst_detect_port(struct drm_connector *connector,
break;
}
out:
+ if (ret != connector_status_connected)
+ drm_dp_cec_unset_edid(&port->aux);
drm_dp_mst_topology_put_port(port);
return ret;
}
@@ -4177,6 +4200,7 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_
edid = drm_get_edid(connector, &port->aux.ddc);
}
port->has_audio = drm_detect_monitor_audio(edid);
+ drm_dp_cec_set_edid(&port->aux, edid);
drm_dp_mst_topology_put_port(port);
return edid;
}
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 12bca1b9512b..e973eba06875 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1497,6 +1497,8 @@ struct drm_connector;
* @connector: the connector this CEC adapter is associated with
* @is_mst: this is an MST branch
* @unregister_work: unregister the CEC adapter
+ * @mst_irq_work: IRQ for CEC events on an MST branch
+ * @mst_set_edid_work: set the EDID for an MST branch
*/
struct drm_dp_aux_cec {
struct mutex lock;
@@ -1504,6 +1506,8 @@ struct drm_dp_aux_cec {
struct drm_connector *connector;
bool is_mst;
struct delayed_work unregister_work;
+ struct work_struct mst_irq_work;
+ struct work_struct mst_set_edid_work;
};
/**
--
2.28.0.402.g5ffc5be6b7-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 4/5] drm_dp_cec: add plumbing in preparation for MST support
2020-09-01 6:22 ` [PATCH 4/5] drm_dp_cec: add plumbing in preparation for MST support Sam McNally
@ 2020-09-01 18:12 ` Lyude Paul
2020-09-09 6:59 ` Sam McNally
2020-09-08 8:41 ` Hans Verkuil
1 sibling, 1 reply; 12+ messages in thread
From: Lyude Paul @ 2020-09-01 18:12 UTC (permalink / raw)
To: Sam McNally, LKML
Cc: Hans Verkuil, Alex Deucher, Ben Skeggs, Christian König,
Daniel Vetter, David Airlie, David Francis, Gwan-gyeong Mun,
Harry Wentland, Imre Deak, Jani Nikula, Joonas Lahtinen,
José Roberto de Souza, Leo Li, Lucas De Marchi,
Maarten Lankhorst, Manasi Navare, Matt Roper, Maxime Ripard,
Mikita Lipski, Pankaj Bharadiya, Rodrigo Siqueira, Rodrigo Vivi,
Thomas Zimmermann, Ville Syrjälä,
Wambui Karuga, amd-gfx, dri-devel, intel-gfx, nouveau
Super minor nitpicks:
On Tue, 2020-09-01 at 16:22 +1000, Sam McNally wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> [sammc@chromium.org:
> - rebased
> - removed polling-related changes
> - moved the calls to drm_dp_cec_(un)set_edid() into the next patch
> ]
> Signed-off-by: Sam McNally <sammc@chromium.org>
> ---
>
> .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 2 +-
> drivers/gpu/drm/drm_dp_cec.c | 22 ++++++++++---------
> drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
> drivers/gpu/drm/nouveau/nouveau_connector.c | 2 +-
> include/drm/drm_dp_helper.h | 6 +++--
> 5 files changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 461fa4da0a34..6e7075893ec9 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -419,7 +419,7 @@ void amdgpu_dm_initialize_dp_connector(struct
> amdgpu_display_manager *dm,
>
> drm_dp_aux_init(&aconnector->dm_dp_aux.aux);
> drm_dp_cec_register_connector(&aconnector->dm_dp_aux.aux,
> - &aconnector->base);
> + &aconnector->base, false);
>
> if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
> return;
> diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> index 3ab2609f9ec7..04ab7b88055c 100644
> --- a/drivers/gpu/drm/drm_dp_cec.c
> +++ b/drivers/gpu/drm/drm_dp_cec.c
> @@ -14,6 +14,7 @@
> #include <drm/drm_connector.h>
> #include <drm/drm_device.h>
> #include <drm/drm_dp_helper.h>
> +#include <drm/drm_dp_mst_helper.h>
>
> /*
> * Unfortunately it turns out that we have a chicken-and-egg situation
> @@ -338,8 +339,6 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
> struct edid *edid)
> if (aux->cec.adap) {
> if (aux->cec.adap->capabilities == cec_caps &&
> aux->cec.adap->available_log_addrs == num_las) {
> - /* Unchanged, so just set the phys addr */
> - cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> goto unlock;
> }
May as well drop the braces here
> /*
> @@ -364,15 +363,16 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
> struct edid *edid)
> if (cec_register_adapter(aux->cec.adap, connector->dev->dev)) {
> cec_delete_adapter(aux->cec.adap);
> aux->cec.adap = NULL;
> - } else {
> - /*
> - * Update the phys addr for the new CEC adapter. When called
> - * from drm_dp_cec_register_connector() edid == NULL, so in
> - * that case the phys addr is just invalidated.
> - */
> - cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> }
> unlock:
> + /*
> + * Update the phys addr for the new CEC adapter. When called
> + * from drm_dp_cec_register_connector() edid == NULL, so in
> + * that case the phys addr is just invalidated.
> + */
> + if (aux->cec.adap && edid) {
> + cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> + }
And here
> mutex_unlock(&aux->cec.lock);
> }
> EXPORT_SYMBOL(drm_dp_cec_set_edid);
> @@ -418,6 +418,7 @@ EXPORT_SYMBOL(drm_dp_cec_unset_edid);
> * drm_dp_cec_register_connector() - register a new connector
> * @aux: DisplayPort AUX channel
> * @connector: drm connector
> + * @is_mst: set to true if this is an MST branch
> *
> * A new connector was registered with associated CEC adapter name and
> * CEC adapter parent device. After registering the name and parent
> @@ -425,12 +426,13 @@ EXPORT_SYMBOL(drm_dp_cec_unset_edid);
> * CEC and to register a CEC adapter if that is the case.
> */
> void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> - struct drm_connector *connector)
> + struct drm_connector *connector, bool is_mst)
> {
> WARN_ON(aux->cec.adap);
> if (WARN_ON(!aux->transfer))
> return;
> aux->cec.connector = connector;
> + aux->cec.is_mst = is_mst;
Also JFYI, you can also check aux->is_remote, but maybe you've got another
reason for copying this here
Either way:
Reviewed-by: Lyude Paul <lyude@redhat.com>
...Also, maybe this is just a coincidence - but do I know your name from
somewhere? Perhaps an IRC community from long ago?
> INIT_DELAYED_WORK(&aux->cec.unregister_work,
> drm_dp_cec_unregister_work);
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 82b9de274f65..744cb55572f9 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -6261,7 +6261,7 @@ intel_dp_connector_register(struct drm_connector
> *connector)
> intel_dp->aux.dev = connector->kdev;
> ret = drm_dp_aux_register(&intel_dp->aux);
> if (!ret)
> - drm_dp_cec_register_connector(&intel_dp->aux, connector);
> + drm_dp_cec_register_connector(&intel_dp->aux, connector, false);
> return ret;
> }
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
> b/drivers/gpu/drm/nouveau/nouveau_connector.c
> index 49dd0cbc332f..671a70e95cd1 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> @@ -1414,7 +1414,7 @@ nouveau_connector_create(struct drm_device *dev,
> switch (type) {
> case DRM_MODE_CONNECTOR_DisplayPort:
> case DRM_MODE_CONNECTOR_eDP:
> - drm_dp_cec_register_connector(&nv_connector->aux, connector);
> + drm_dp_cec_register_connector(&nv_connector->aux, connector,
> false);
> break;
> }
>
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 85513eeb2196..12bca1b9512b 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1495,12 +1495,14 @@ struct drm_connector;
> * @lock: mutex protecting this struct
> * @adap: the CEC adapter for CEC-Tunneling-over-AUX support.
> * @connector: the connector this CEC adapter is associated with
> + * @is_mst: this is an MST branch
> * @unregister_work: unregister the CEC adapter
> */
> struct drm_dp_aux_cec {
> struct mutex lock;
> struct cec_adapter *adap;
> struct drm_connector *connector;
> + bool is_mst;
> struct delayed_work unregister_work;
> };
>
> @@ -1746,7 +1748,7 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc, u32
> edid_quirks,
> #ifdef CONFIG_DRM_DP_CEC
> void drm_dp_cec_irq(struct drm_dp_aux *aux);
> void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> - struct drm_connector *connector);
> + struct drm_connector *connector, bool
> is_mst);
> void drm_dp_cec_unregister_connector(struct drm_dp_aux *aux);
> void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid);
> void drm_dp_cec_unset_edid(struct drm_dp_aux *aux);
> @@ -1757,7 +1759,7 @@ static inline void drm_dp_cec_irq(struct drm_dp_aux
> *aux)
>
> static inline void
> drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> - struct drm_connector *connector)
> + struct drm_connector *connector, bool is_mst)
> {
> }
>
--
Sincerely,
Lyude Paul (she/her)
Software Engineer at Red Hat
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 5/5] drm_dp_cec: add the implementation of MST support
2020-09-01 6:22 ` [PATCH 5/5] drm_dp_cec: add the implementation of " Sam McNally
@ 2020-09-08 8:08 ` Hans Verkuil
2020-09-09 7:09 ` Sam McNally
2020-09-08 9:24 ` Hans Verkuil
1 sibling, 1 reply; 12+ messages in thread
From: Hans Verkuil @ 2020-09-08 8:08 UTC (permalink / raw)
To: Sam McNally, LKML
Cc: Thomas Zimmermann, David Airlie, Hans Verkuil, dri-devel
Hi Sam,
On 01/09/2020 08:22, Sam McNally wrote:
> With DP v2.0 errata E5, CEC tunneling can be supported through an MST
> topology.
Oh wow, this is finally supported in the spec. Very nice to see this.
Also very nice to see that my old experimental patches attempting to
support CEC on MST didn't go to waste!
Do you know of any MST HW that supports this? I'd love to experiment
with this.
Regards,
Hans
>
> There are some minor differences for CEC tunneling through an MST
> topology compared to CEC tunneling to an SST port:
> - CEC IRQs are delivered via a sink event notify message
> - CEC-related DPCD registers are accessed via remote DPCD reads and
> writes.
>
> This results in the MST implementation diverging from the existing SST
> implementation:
> - sink event notify messages with CEC_IRQ ID set indicate CEC IRQ rather
> than ESI1
> - setting edid and handling CEC IRQs, which can be triggered from
> contexts where locks held preclude HPD handling, are deferred to avoid
> remote DPCD access which would block until HPD handling is performed
> or a timeout
>
> Register and unregister for all MST connectors, ensuring their
> drm_dp_aux_cec struct won't be accessed uninitialized.
>
> Signed-off-by: Sam McNally <sammc@chromium.org>
> ---
>
> drivers/gpu/drm/drm_dp_cec.c | 67 +++++++++++++++++++++++++--
> drivers/gpu/drm/drm_dp_mst_topology.c | 24 ++++++++++
> include/drm/drm_dp_helper.h | 4 ++
> 3 files changed, 90 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> index 04ab7b88055c..9f6aeaa27f00 100644
> --- a/drivers/gpu/drm/drm_dp_cec.c
> +++ b/drivers/gpu/drm/drm_dp_cec.c
> @@ -249,6 +249,10 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux)
> if (!aux->transfer)
> return;
>
> + if (aux->cec.is_mst) {
> + schedule_work(&aux->cec.mst_irq_work);
> + return;
> + }
> mutex_lock(&aux->cec.lock);
> if (!aux->cec.adap)
> goto unlock;
> @@ -277,6 +281,24 @@ static bool drm_dp_cec_cap(struct drm_dp_aux *aux, u8 *cec_cap)
> return true;
> }
>
> +static void drm_dp_cec_mst_irq_work(struct work_struct *work)
> +{
> + struct drm_dp_aux *aux = container_of(work, struct drm_dp_aux,
> + cec.mst_irq_work);
> + struct drm_dp_mst_port *port =
> + container_of(aux, struct drm_dp_mst_port, aux);
> +
> + port = drm_dp_mst_topology_get_port_validated(port->mgr, port);
> + if (!port)
> + return;
> + mutex_lock(&aux->cec.lock);
> + if (aux->cec.adap)
> + drm_dp_cec_handle_irq(aux);
> +
> + mutex_unlock(&aux->cec.lock);
> + drm_dp_mst_topology_put_port(port);
> +}
> +
> /*
> * Called if the HPD was low for more than drm_dp_cec_unregister_delay
> * seconds. This unregisters the CEC adapter.
> @@ -298,7 +320,8 @@ static void drm_dp_cec_unregister_work(struct work_struct *work)
> * were unchanged and just update the CEC physical address. Otherwise
> * unregister the old CEC adapter and create a new one.
> */
> -void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> +static void drm_dp_cec_handle_set_edid(struct drm_dp_aux *aux,
> + const struct edid *edid)
> {
> struct drm_connector *connector = aux->cec.connector;
> u32 cec_caps = CEC_CAP_DEFAULTS | CEC_CAP_NEEDS_HPD |
> @@ -307,10 +330,6 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> unsigned int num_las = 1;
> u8 cap;
>
> - /* No transfer function was set, so not a DP connector */
> - if (!aux->transfer)
> - return;
> -
> #ifndef CONFIG_MEDIA_CEC_RC
> /*
> * CEC_CAP_RC is part of CEC_CAP_DEFAULTS, but it is stripped by
> @@ -321,6 +340,7 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> */
> cec_caps &= ~CEC_CAP_RC;
> #endif
> + cancel_work_sync(&aux->cec.mst_irq_work);
> cancel_delayed_work_sync(&aux->cec.unregister_work);
>
> mutex_lock(&aux->cec.lock);
> @@ -375,6 +395,18 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> }
> mutex_unlock(&aux->cec.lock);
> }
> +
> +void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> +{
> + /* No transfer function was set, so not a DP connector */
> + if (!aux->transfer)
> + return;
> +
> + if (aux->cec.is_mst)
> + schedule_work(&aux->cec.mst_set_edid_work);
> + else
> + drm_dp_cec_handle_set_edid(aux, edid);
> +}
> EXPORT_SYMBOL(drm_dp_cec_set_edid);
>
> /*
> @@ -393,6 +425,8 @@ void drm_dp_cec_unset_edid(struct drm_dp_aux *aux)
> goto unlock;
>
> cec_phys_addr_invalidate(aux->cec.adap);
> + cancel_work_sync(&aux->cec.mst_irq_work);
> +
> /*
> * We're done if we want to keep the CEC device
> * (drm_dp_cec_unregister_delay is >= NEVER_UNREG_DELAY) or if the
> @@ -414,6 +448,25 @@ void drm_dp_cec_unset_edid(struct drm_dp_aux *aux)
> }
> EXPORT_SYMBOL(drm_dp_cec_unset_edid);
>
> +static void drm_dp_cec_mst_set_edid_work(struct work_struct *work)
> +{
> + struct drm_dp_aux *aux =
> + container_of(work, struct drm_dp_aux, cec.mst_set_edid_work);
> + struct drm_dp_mst_port *port =
> + container_of(aux, struct drm_dp_mst_port, aux);
> + struct edid *edid = NULL;
> +
> + port = drm_dp_mst_topology_get_port_validated(port->mgr, port);
> + if (!port)
> + return;
> +
> + edid = drm_get_edid(port->connector, &port->aux.ddc);
> +
> + drm_dp_cec_handle_set_edid(aux, edid);
> +
> + drm_dp_mst_topology_put_port(port);
> +}
> +
> /**
> * drm_dp_cec_register_connector() - register a new connector
> * @aux: DisplayPort AUX channel
> @@ -435,6 +488,8 @@ void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> aux->cec.is_mst = is_mst;
> INIT_DELAYED_WORK(&aux->cec.unregister_work,
> drm_dp_cec_unregister_work);
> + INIT_WORK(&aux->cec.mst_irq_work, drm_dp_cec_mst_irq_work);
> + INIT_WORK(&aux->cec.mst_set_edid_work, drm_dp_cec_mst_set_edid_work);
> }
> EXPORT_SYMBOL(drm_dp_cec_register_connector);
>
> @@ -444,6 +499,8 @@ EXPORT_SYMBOL(drm_dp_cec_register_connector);
> */
> void drm_dp_cec_unregister_connector(struct drm_dp_aux *aux)
> {
> + cancel_work_sync(&aux->cec.mst_irq_work);
> + cancel_work_sync(&aux->cec.mst_set_edid_work);
> if (!aux->cec.adap)
> return;
> cancel_delayed_work_sync(&aux->cec.unregister_work);
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index c783a2a1c114..fd9430d88fd6 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2183,6 +2183,8 @@ static void build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
> int drm_dp_mst_connector_late_register(struct drm_connector *connector,
> struct drm_dp_mst_port *port)
> {
> + drm_dp_cec_register_connector(&port->aux, connector, true);
> +
> DRM_DEBUG_KMS("registering %s remote bus for %s\n",
> port->aux.name, connector->kdev->kobj.name);
>
> @@ -2206,6 +2208,8 @@ void drm_dp_mst_connector_early_unregister(struct drm_connector *connector,
> DRM_DEBUG_KMS("unregistering %s remote bus for %s\n",
> port->aux.name, connector->kdev->kobj.name);
> drm_dp_aux_unregister_devnode(&port->aux);
> +
> + drm_dp_cec_unregister_connector(&port->aux);
> }
> EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister);
>
> @@ -2515,6 +2519,21 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> queue_work(system_long_wq, &mstb->mgr->work);
> }
>
> +static void
> +drm_dp_mst_handle_sink_event(struct drm_dp_mst_branch *mstb,
> + struct drm_dp_sink_event_notify *sink_event)
> +{
> + struct drm_dp_mst_port *port;
> +
> + if (sink_event->event_id & DP_SINK_EVENT_CEC_IRQ_EVENT) {
> + port = drm_dp_get_port(mstb, sink_event->port_number);
> + if (port) {
> + drm_dp_cec_irq(&port->aux);
> + drm_dp_mst_topology_put_port(port);
> + }
> + }
> +}
> +
> static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct drm_dp_mst_topology_mgr *mgr,
> u8 lct, u8 *rad)
> {
> @@ -3954,6 +3973,8 @@ drm_dp_mst_process_up_req(struct drm_dp_mst_topology_mgr *mgr,
> if (msg->req_type == DP_CONNECTION_STATUS_NOTIFY) {
> drm_dp_mst_handle_conn_stat(mstb, &msg->u.conn_stat);
> hotplug = true;
> + } else if (msg->req_type == DP_SINK_EVENT_NOTIFY) {
> + drm_dp_mst_handle_sink_event(mstb, &msg->u.sink_event);
> }
>
> drm_dp_mst_topology_put_mstb(mstb);
> @@ -4147,6 +4168,8 @@ drm_dp_mst_detect_port(struct drm_connector *connector,
> break;
> }
> out:
> + if (ret != connector_status_connected)
> + drm_dp_cec_unset_edid(&port->aux);
> drm_dp_mst_topology_put_port(port);
> return ret;
> }
> @@ -4177,6 +4200,7 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_
> edid = drm_get_edid(connector, &port->aux.ddc);
> }
> port->has_audio = drm_detect_monitor_audio(edid);
> + drm_dp_cec_set_edid(&port->aux, edid);
> drm_dp_mst_topology_put_port(port);
> return edid;
> }
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 12bca1b9512b..e973eba06875 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1497,6 +1497,8 @@ struct drm_connector;
> * @connector: the connector this CEC adapter is associated with
> * @is_mst: this is an MST branch
> * @unregister_work: unregister the CEC adapter
> + * @mst_irq_work: IRQ for CEC events on an MST branch
> + * @mst_set_edid_work: set the EDID for an MST branch
> */
> struct drm_dp_aux_cec {
> struct mutex lock;
> @@ -1504,6 +1506,8 @@ struct drm_dp_aux_cec {
> struct drm_connector *connector;
> bool is_mst;
> struct delayed_work unregister_work;
> + struct work_struct mst_irq_work;
> + struct work_struct mst_set_edid_work;
> };
>
> /**
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 4/5] drm_dp_cec: add plumbing in preparation for MST support
2020-09-01 6:22 ` [PATCH 4/5] drm_dp_cec: add plumbing in preparation for MST support Sam McNally
2020-09-01 18:12 ` Lyude Paul
@ 2020-09-08 8:41 ` Hans Verkuil
2020-09-09 6:59 ` Sam McNally
1 sibling, 1 reply; 12+ messages in thread
From: Hans Verkuil @ 2020-09-08 8:41 UTC (permalink / raw)
To: Sam McNally, LKML
Cc: David Airlie, nouveau, José Roberto de Souza, David Francis,
Rodrigo Siqueira, amd-gfx, Ben Skeggs, Wambui Karuga,
Thomas Zimmermann, Leo Li, Lucas De Marchi, intel-gfx,
Gwan-gyeong Mun, Rodrigo Vivi, Mikita Lipski, Pankaj Bharadiya,
dri-devel, Manasi Navare, Alex Deucher, Christian König
On 01/09/2020 08:22, Sam McNally wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> [sammc@chromium.org:
> - rebased
> - removed polling-related changes
> - moved the calls to drm_dp_cec_(un)set_edid() into the next patch
> ]
> Signed-off-by: Sam McNally <sammc@chromium.org>
> ---
>
> .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 2 +-
> drivers/gpu/drm/drm_dp_cec.c | 22 ++++++++++---------
> drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
> drivers/gpu/drm/nouveau/nouveau_connector.c | 2 +-
> include/drm/drm_dp_helper.h | 6 +++--
> 5 files changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 461fa4da0a34..6e7075893ec9 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -419,7 +419,7 @@ void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm,
>
> drm_dp_aux_init(&aconnector->dm_dp_aux.aux);
> drm_dp_cec_register_connector(&aconnector->dm_dp_aux.aux,
> - &aconnector->base);
> + &aconnector->base, false);
>
> if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
> return;
> diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> index 3ab2609f9ec7..04ab7b88055c 100644
> --- a/drivers/gpu/drm/drm_dp_cec.c
> +++ b/drivers/gpu/drm/drm_dp_cec.c
> @@ -14,6 +14,7 @@
> #include <drm/drm_connector.h>
> #include <drm/drm_device.h>
> #include <drm/drm_dp_helper.h>
> +#include <drm/drm_dp_mst_helper.h>
>
> /*
> * Unfortunately it turns out that we have a chicken-and-egg situation
> @@ -338,8 +339,6 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> if (aux->cec.adap) {
> if (aux->cec.adap->capabilities == cec_caps &&
> aux->cec.adap->available_log_addrs == num_las) {
> - /* Unchanged, so just set the phys addr */
> - cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> goto unlock;
> }
> /*
> @@ -364,15 +363,16 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> if (cec_register_adapter(aux->cec.adap, connector->dev->dev)) {
> cec_delete_adapter(aux->cec.adap);
> aux->cec.adap = NULL;
> - } else {
> - /*
> - * Update the phys addr for the new CEC adapter. When called
> - * from drm_dp_cec_register_connector() edid == NULL, so in
> - * that case the phys addr is just invalidated.
> - */
> - cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> }
> unlock:
> + /*
> + * Update the phys addr for the new CEC adapter. When called
> + * from drm_dp_cec_register_connector() edid == NULL, so in
> + * that case the phys addr is just invalidated.
> + */
The comment is no longer in sync with the code: if EDID == NULL, then
nothing is done due to the edid check in the 'if' below.
> + if (aux->cec.adap && edid) {
I think this should just be: if (aux->cec.adap)
Also, the {} aren't necessary here.
> + cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> + }
> mutex_unlock(&aux->cec.lock);
> }
> EXPORT_SYMBOL(drm_dp_cec_set_edid);
Frankly, the changes to this function should be dropped completely, from
what I can see they are not necessary. It was done in my original patch
because of the way I handled mst, but you did it differently (and I think
better), so these changes are no longer needed.
I know I am actually commenting on my old patch, but that patch was from a
work-in-progress git branch and was never meant as a 'proper' patch.
However, what complicates matters is that after digging a bit more I discovered
that commit 732300154980 ("drm: Do not call drm_dp_cec_set_edid() while registering
DP connectors") changed drm_dp_cec_register_connector() so that it no longer
calls drm_dp_cec_set_edid(), but the comments there and in this function were
not updated. It would be nice if you can add a patch fixing these outdated
comments.
Regardless of that change in commit 732300154980, the edid pointer can still be
NULL and the existing behavior should be kept (i.e. create a CEC device, but with
an invalid physical address since there is no EDID for some reason).
Regards,
Hans
> @@ -418,6 +418,7 @@ EXPORT_SYMBOL(drm_dp_cec_unset_edid);
> * drm_dp_cec_register_connector() - register a new connector
> * @aux: DisplayPort AUX channel
> * @connector: drm connector
> + * @is_mst: set to true if this is an MST branch
> *
> * A new connector was registered with associated CEC adapter name and
> * CEC adapter parent device. After registering the name and parent
> @@ -425,12 +426,13 @@ EXPORT_SYMBOL(drm_dp_cec_unset_edid);
> * CEC and to register a CEC adapter if that is the case.
> */
> void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> - struct drm_connector *connector)
> + struct drm_connector *connector, bool is_mst)
> {
> WARN_ON(aux->cec.adap);
> if (WARN_ON(!aux->transfer))
> return;
> aux->cec.connector = connector;
> + aux->cec.is_mst = is_mst;
> INIT_DELAYED_WORK(&aux->cec.unregister_work,
> drm_dp_cec_unregister_work);
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 82b9de274f65..744cb55572f9 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -6261,7 +6261,7 @@ intel_dp_connector_register(struct drm_connector *connector)
> intel_dp->aux.dev = connector->kdev;
> ret = drm_dp_aux_register(&intel_dp->aux);
> if (!ret)
> - drm_dp_cec_register_connector(&intel_dp->aux, connector);
> + drm_dp_cec_register_connector(&intel_dp->aux, connector, false);
> return ret;
> }
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
> index 49dd0cbc332f..671a70e95cd1 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> @@ -1414,7 +1414,7 @@ nouveau_connector_create(struct drm_device *dev,
> switch (type) {
> case DRM_MODE_CONNECTOR_DisplayPort:
> case DRM_MODE_CONNECTOR_eDP:
> - drm_dp_cec_register_connector(&nv_connector->aux, connector);
> + drm_dp_cec_register_connector(&nv_connector->aux, connector, false);
> break;
> }
>
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 85513eeb2196..12bca1b9512b 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1495,12 +1495,14 @@ struct drm_connector;
> * @lock: mutex protecting this struct
> * @adap: the CEC adapter for CEC-Tunneling-over-AUX support.
> * @connector: the connector this CEC adapter is associated with
> + * @is_mst: this is an MST branch
> * @unregister_work: unregister the CEC adapter
> */
> struct drm_dp_aux_cec {
> struct mutex lock;
> struct cec_adapter *adap;
> struct drm_connector *connector;
> + bool is_mst;
> struct delayed_work unregister_work;
> };
>
> @@ -1746,7 +1748,7 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc, u32 edid_quirks,
> #ifdef CONFIG_DRM_DP_CEC
> void drm_dp_cec_irq(struct drm_dp_aux *aux);
> void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> - struct drm_connector *connector);
> + struct drm_connector *connector, bool is_mst);
> void drm_dp_cec_unregister_connector(struct drm_dp_aux *aux);
> void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid);
> void drm_dp_cec_unset_edid(struct drm_dp_aux *aux);
> @@ -1757,7 +1759,7 @@ static inline void drm_dp_cec_irq(struct drm_dp_aux *aux)
>
> static inline void
> drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> - struct drm_connector *connector)
> + struct drm_connector *connector, bool is_mst)
> {
> }
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 5/5] drm_dp_cec: add the implementation of MST support
2020-09-01 6:22 ` [PATCH 5/5] drm_dp_cec: add the implementation of " Sam McNally
2020-09-08 8:08 ` Hans Verkuil
@ 2020-09-08 9:24 ` Hans Verkuil
1 sibling, 0 replies; 12+ messages in thread
From: Hans Verkuil @ 2020-09-08 9:24 UTC (permalink / raw)
To: Sam McNally, LKML
Cc: Thomas Zimmermann, David Airlie, Hans Verkuil, dri-devel
On 01/09/2020 08:22, Sam McNally wrote:
> With DP v2.0 errata E5, CEC tunneling can be supported through an MST
> topology.
>
> There are some minor differences for CEC tunneling through an MST
> topology compared to CEC tunneling to an SST port:
> - CEC IRQs are delivered via a sink event notify message
> - CEC-related DPCD registers are accessed via remote DPCD reads and
> writes.
>
> This results in the MST implementation diverging from the existing SST
> implementation:
> - sink event notify messages with CEC_IRQ ID set indicate CEC IRQ rather
> than ESI1
> - setting edid and handling CEC IRQs, which can be triggered from
> contexts where locks held preclude HPD handling, are deferred to avoid
> remote DPCD access which would block until HPD handling is performed
> or a timeout
>
> Register and unregister for all MST connectors, ensuring their
> drm_dp_aux_cec struct won't be accessed uninitialized.
>
> Signed-off-by: Sam McNally <sammc@chromium.org>
> ---
>
> drivers/gpu/drm/drm_dp_cec.c | 67 +++++++++++++++++++++++++--
> drivers/gpu/drm/drm_dp_mst_topology.c | 24 ++++++++++
> include/drm/drm_dp_helper.h | 4 ++
> 3 files changed, 90 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> index 04ab7b88055c..9f6aeaa27f00 100644
> --- a/drivers/gpu/drm/drm_dp_cec.c
> +++ b/drivers/gpu/drm/drm_dp_cec.c
> @@ -249,6 +249,10 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux)
> if (!aux->transfer)
> return;
>
> + if (aux->cec.is_mst) {
> + schedule_work(&aux->cec.mst_irq_work);
> + return;
> + }
> mutex_lock(&aux->cec.lock);
> if (!aux->cec.adap)
> goto unlock;
> @@ -277,6 +281,24 @@ static bool drm_dp_cec_cap(struct drm_dp_aux *aux, u8 *cec_cap)
> return true;
> }
>
> +static void drm_dp_cec_mst_irq_work(struct work_struct *work)
> +{
> + struct drm_dp_aux *aux = container_of(work, struct drm_dp_aux,
> + cec.mst_irq_work);
> + struct drm_dp_mst_port *port =
> + container_of(aux, struct drm_dp_mst_port, aux);
> +
> + port = drm_dp_mst_topology_get_port_validated(port->mgr, port);
> + if (!port)
> + return;
> + mutex_lock(&aux->cec.lock);
> + if (aux->cec.adap)
> + drm_dp_cec_handle_irq(aux);
> +
Nitpick: this empty line feels unbalanced. It makes more sense to add
an empty line just before the mutex_lock() and remove this one.
> + mutex_unlock(&aux->cec.lock);
> + drm_dp_mst_topology_put_port(port);
> +}
> +
> /*
> * Called if the HPD was low for more than drm_dp_cec_unregister_delay
> * seconds. This unregisters the CEC adapter.
> @@ -298,7 +320,8 @@ static void drm_dp_cec_unregister_work(struct work_struct *work)
> * were unchanged and just update the CEC physical address. Otherwise
> * unregister the old CEC adapter and create a new one.
> */
> -void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> +static void drm_dp_cec_handle_set_edid(struct drm_dp_aux *aux,
> + const struct edid *edid)
> {
> struct drm_connector *connector = aux->cec.connector;
> u32 cec_caps = CEC_CAP_DEFAULTS | CEC_CAP_NEEDS_HPD |
> @@ -307,10 +330,6 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> unsigned int num_las = 1;
> u8 cap;
>
> - /* No transfer function was set, so not a DP connector */
> - if (!aux->transfer)
> - return;
> -
> #ifndef CONFIG_MEDIA_CEC_RC
> /*
> * CEC_CAP_RC is part of CEC_CAP_DEFAULTS, but it is stripped by
> @@ -321,6 +340,7 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> */
> cec_caps &= ~CEC_CAP_RC;
> #endif
> + cancel_work_sync(&aux->cec.mst_irq_work);
> cancel_delayed_work_sync(&aux->cec.unregister_work);
>
> mutex_lock(&aux->cec.lock);
> @@ -375,6 +395,18 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> }
> mutex_unlock(&aux->cec.lock);
> }
> +
> +void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> +{
> + /* No transfer function was set, so not a DP connector */
> + if (!aux->transfer)
> + return;
> +
> + if (aux->cec.is_mst)
> + schedule_work(&aux->cec.mst_set_edid_work);
> + else
> + drm_dp_cec_handle_set_edid(aux, edid);
> +}
> EXPORT_SYMBOL(drm_dp_cec_set_edid);
>
> /*
> @@ -393,6 +425,8 @@ void drm_dp_cec_unset_edid(struct drm_dp_aux *aux)
> goto unlock;
>
> cec_phys_addr_invalidate(aux->cec.adap);
> + cancel_work_sync(&aux->cec.mst_irq_work);
I'd swap these two lines. It's a bit more robust.
Shouldn't this also cancel cec.mst_set_edid_work?
> +
> /*
> * We're done if we want to keep the CEC device
> * (drm_dp_cec_unregister_delay is >= NEVER_UNREG_DELAY) or if the
> @@ -414,6 +448,25 @@ void drm_dp_cec_unset_edid(struct drm_dp_aux *aux)
> }
> EXPORT_SYMBOL(drm_dp_cec_unset_edid);
>
> +static void drm_dp_cec_mst_set_edid_work(struct work_struct *work)
It's a bit weird to have this function appear after the drm_dp_cec_unset_edid()
function. Wouldn't it be better to move it to just before drm_dp_cec_set_edid()?
That way all the 'set_edid()' functions are grouped together.
Regards,
Hans
> +{
> + struct drm_dp_aux *aux =
> + container_of(work, struct drm_dp_aux, cec.mst_set_edid_work);
> + struct drm_dp_mst_port *port =
> + container_of(aux, struct drm_dp_mst_port, aux);
> + struct edid *edid = NULL;
> +
> + port = drm_dp_mst_topology_get_port_validated(port->mgr, port);
> + if (!port)
> + return;
> +
> + edid = drm_get_edid(port->connector, &port->aux.ddc);
> +
> + drm_dp_cec_handle_set_edid(aux, edid);
> +
> + drm_dp_mst_topology_put_port(port);
> +}
> +
> /**
> * drm_dp_cec_register_connector() - register a new connector
> * @aux: DisplayPort AUX channel
> @@ -435,6 +488,8 @@ void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> aux->cec.is_mst = is_mst;
> INIT_DELAYED_WORK(&aux->cec.unregister_work,
> drm_dp_cec_unregister_work);
> + INIT_WORK(&aux->cec.mst_irq_work, drm_dp_cec_mst_irq_work);
> + INIT_WORK(&aux->cec.mst_set_edid_work, drm_dp_cec_mst_set_edid_work);
> }
> EXPORT_SYMBOL(drm_dp_cec_register_connector);
>
> @@ -444,6 +499,8 @@ EXPORT_SYMBOL(drm_dp_cec_register_connector);
> */
> void drm_dp_cec_unregister_connector(struct drm_dp_aux *aux)
> {
> + cancel_work_sync(&aux->cec.mst_irq_work);
> + cancel_work_sync(&aux->cec.mst_set_edid_work);
> if (!aux->cec.adap)
> return;
> cancel_delayed_work_sync(&aux->cec.unregister_work);
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index c783a2a1c114..fd9430d88fd6 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2183,6 +2183,8 @@ static void build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
> int drm_dp_mst_connector_late_register(struct drm_connector *connector,
> struct drm_dp_mst_port *port)
> {
> + drm_dp_cec_register_connector(&port->aux, connector, true);
> +
> DRM_DEBUG_KMS("registering %s remote bus for %s\n",
> port->aux.name, connector->kdev->kobj.name);
>
> @@ -2206,6 +2208,8 @@ void drm_dp_mst_connector_early_unregister(struct drm_connector *connector,
> DRM_DEBUG_KMS("unregistering %s remote bus for %s\n",
> port->aux.name, connector->kdev->kobj.name);
> drm_dp_aux_unregister_devnode(&port->aux);
> +
> + drm_dp_cec_unregister_connector(&port->aux);
> }
> EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister);
>
> @@ -2515,6 +2519,21 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> queue_work(system_long_wq, &mstb->mgr->work);
> }
>
> +static void
> +drm_dp_mst_handle_sink_event(struct drm_dp_mst_branch *mstb,
> + struct drm_dp_sink_event_notify *sink_event)
> +{
> + struct drm_dp_mst_port *port;
> +
> + if (sink_event->event_id & DP_SINK_EVENT_CEC_IRQ_EVENT) {
> + port = drm_dp_get_port(mstb, sink_event->port_number);
> + if (port) {
> + drm_dp_cec_irq(&port->aux);
> + drm_dp_mst_topology_put_port(port);
> + }
> + }
> +}
> +
> static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct drm_dp_mst_topology_mgr *mgr,
> u8 lct, u8 *rad)
> {
> @@ -3954,6 +3973,8 @@ drm_dp_mst_process_up_req(struct drm_dp_mst_topology_mgr *mgr,
> if (msg->req_type == DP_CONNECTION_STATUS_NOTIFY) {
> drm_dp_mst_handle_conn_stat(mstb, &msg->u.conn_stat);
> hotplug = true;
> + } else if (msg->req_type == DP_SINK_EVENT_NOTIFY) {
> + drm_dp_mst_handle_sink_event(mstb, &msg->u.sink_event);
> }
>
> drm_dp_mst_topology_put_mstb(mstb);
> @@ -4147,6 +4168,8 @@ drm_dp_mst_detect_port(struct drm_connector *connector,
> break;
> }
> out:
> + if (ret != connector_status_connected)
> + drm_dp_cec_unset_edid(&port->aux);
> drm_dp_mst_topology_put_port(port);
> return ret;
> }
> @@ -4177,6 +4200,7 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_
> edid = drm_get_edid(connector, &port->aux.ddc);
> }
> port->has_audio = drm_detect_monitor_audio(edid);
> + drm_dp_cec_set_edid(&port->aux, edid);
> drm_dp_mst_topology_put_port(port);
> return edid;
> }
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 12bca1b9512b..e973eba06875 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1497,6 +1497,8 @@ struct drm_connector;
> * @connector: the connector this CEC adapter is associated with
> * @is_mst: this is an MST branch
> * @unregister_work: unregister the CEC adapter
> + * @mst_irq_work: IRQ for CEC events on an MST branch
> + * @mst_set_edid_work: set the EDID for an MST branch
> */
> struct drm_dp_aux_cec {
> struct mutex lock;
> @@ -1504,6 +1506,8 @@ struct drm_dp_aux_cec {
> struct drm_connector *connector;
> bool is_mst;
> struct delayed_work unregister_work;
> + struct work_struct mst_irq_work;
> + struct work_struct mst_set_edid_work;
> };
>
> /**
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 4/5] drm_dp_cec: add plumbing in preparation for MST support
2020-09-01 18:12 ` Lyude Paul
@ 2020-09-09 6:59 ` Sam McNally
0 siblings, 0 replies; 12+ messages in thread
From: Sam McNally @ 2020-09-09 6:59 UTC (permalink / raw)
To: lyude
Cc: LKML, Hans Verkuil, Alex Deucher, Ben Skeggs,
Christian König, Daniel Vetter, David Airlie, David Francis,
Gwan-gyeong Mun, Harry Wentland, Imre Deak, Jani Nikula,
Joonas Lahtinen, José Roberto de Souza, Leo Li,
Lucas De Marchi, Maarten Lankhorst, Manasi Navare, Matt Roper,
Maxime Ripard, Mikita Lipski, Pankaj Bharadiya, Rodrigo Siqueira,
Rodrigo Vivi, Thomas Zimmermann, Ville Syrjälä,
Wambui Karuga, amd-gfx, dri-devel, intel-gfx, nouveau
On Wed, 2 Sep 2020 at 04:12, Lyude Paul <lyude@redhat.com> wrote:
>
> Super minor nitpicks:
>
> On Tue, 2020-09-01 at 16:22 +1000, Sam McNally wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> >
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > [sammc@chromium.org:
> > - rebased
> > - removed polling-related changes
> > - moved the calls to drm_dp_cec_(un)set_edid() into the next patch
> > ]
> > Signed-off-by: Sam McNally <sammc@chromium.org>
> > ---
> >
> > .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 2 +-
> > drivers/gpu/drm/drm_dp_cec.c | 22 ++++++++++---------
> > drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
> > drivers/gpu/drm/nouveau/nouveau_connector.c | 2 +-
> > include/drm/drm_dp_helper.h | 6 +++--
> > 5 files changed, 19 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > index 461fa4da0a34..6e7075893ec9 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > @@ -419,7 +419,7 @@ void amdgpu_dm_initialize_dp_connector(struct
> > amdgpu_display_manager *dm,
> >
> > drm_dp_aux_init(&aconnector->dm_dp_aux.aux);
> > drm_dp_cec_register_connector(&aconnector->dm_dp_aux.aux,
> > - &aconnector->base);
> > + &aconnector->base, false);
> >
> > if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
> > return;
> > diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> > index 3ab2609f9ec7..04ab7b88055c 100644
> > --- a/drivers/gpu/drm/drm_dp_cec.c
> > +++ b/drivers/gpu/drm/drm_dp_cec.c
> > @@ -14,6 +14,7 @@
> > #include <drm/drm_connector.h>
> > #include <drm/drm_device.h>
> > #include <drm/drm_dp_helper.h>
> > +#include <drm/drm_dp_mst_helper.h>
> >
> > /*
> > * Unfortunately it turns out that we have a chicken-and-egg situation
> > @@ -338,8 +339,6 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
> > struct edid *edid)
> > if (aux->cec.adap) {
> > if (aux->cec.adap->capabilities == cec_caps &&
> > aux->cec.adap->available_log_addrs == num_las) {
> > - /* Unchanged, so just set the phys addr */
> > - cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> > goto unlock;
> > }
>
> May as well drop the braces here
>
> > /*
> > @@ -364,15 +363,16 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
> > struct edid *edid)
> > if (cec_register_adapter(aux->cec.adap, connector->dev->dev)) {
> > cec_delete_adapter(aux->cec.adap);
> > aux->cec.adap = NULL;
> > - } else {
> > - /*
> > - * Update the phys addr for the new CEC adapter. When called
> > - * from drm_dp_cec_register_connector() edid == NULL, so in
> > - * that case the phys addr is just invalidated.
> > - */
> > - cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> > }
> > unlock:
> > + /*
> > + * Update the phys addr for the new CEC adapter. When called
> > + * from drm_dp_cec_register_connector() edid == NULL, so in
> > + * that case the phys addr is just invalidated.
> > + */
> > + if (aux->cec.adap && edid) {
> > + cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> > + }
>
> And here
>
> > mutex_unlock(&aux->cec.lock);
> > }
> > EXPORT_SYMBOL(drm_dp_cec_set_edid);
> > @@ -418,6 +418,7 @@ EXPORT_SYMBOL(drm_dp_cec_unset_edid);
> > * drm_dp_cec_register_connector() - register a new connector
> > * @aux: DisplayPort AUX channel
> > * @connector: drm connector
> > + * @is_mst: set to true if this is an MST branch
> > *
> > * A new connector was registered with associated CEC adapter name and
> > * CEC adapter parent device. After registering the name and parent
> > @@ -425,12 +426,13 @@ EXPORT_SYMBOL(drm_dp_cec_unset_edid);
> > * CEC and to register a CEC adapter if that is the case.
> > */
> > void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> > - struct drm_connector *connector)
> > + struct drm_connector *connector, bool is_mst)
> > {
> > WARN_ON(aux->cec.adap);
> > if (WARN_ON(!aux->transfer))
> > return;
> > aux->cec.connector = connector;
> > + aux->cec.is_mst = is_mst;
>
> Also JFYI, you can also check aux->is_remote, but maybe you've got another
> reason for copying this here
>
I think this was just an artefact of this patch originally being
written before aux->is_remote was added. Switching to it mostly
removes the need for this patch, and leaving drm_dp_cec_set_edid()
unchanged, as Hans suggests, removes the rest.
> Either way:
>
> Reviewed-by: Lyude Paul <lyude@redhat.com>
>
> ...Also, maybe this is just a coincidence - but do I know your name from
> somewhere? Perhaps an IRC community from long ago?
>
Not that I can think of; it's probably just a coincidence.
> > INIT_DELAYED_WORK(&aux->cec.unregister_work,
> > drm_dp_cec_unregister_work);
> > }
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 82b9de274f65..744cb55572f9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -6261,7 +6261,7 @@ intel_dp_connector_register(struct drm_connector
> > *connector)
> > intel_dp->aux.dev = connector->kdev;
> > ret = drm_dp_aux_register(&intel_dp->aux);
> > if (!ret)
> > - drm_dp_cec_register_connector(&intel_dp->aux, connector);
> > + drm_dp_cec_register_connector(&intel_dp->aux, connector, false);
> > return ret;
> > }
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
> > b/drivers/gpu/drm/nouveau/nouveau_connector.c
> > index 49dd0cbc332f..671a70e95cd1 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> > @@ -1414,7 +1414,7 @@ nouveau_connector_create(struct drm_device *dev,
> > switch (type) {
> > case DRM_MODE_CONNECTOR_DisplayPort:
> > case DRM_MODE_CONNECTOR_eDP:
> > - drm_dp_cec_register_connector(&nv_connector->aux, connector);
> > + drm_dp_cec_register_connector(&nv_connector->aux, connector,
> > false);
> > break;
> > }
> >
> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > index 85513eeb2196..12bca1b9512b 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -1495,12 +1495,14 @@ struct drm_connector;
> > * @lock: mutex protecting this struct
> > * @adap: the CEC adapter for CEC-Tunneling-over-AUX support.
> > * @connector: the connector this CEC adapter is associated with
> > + * @is_mst: this is an MST branch
> > * @unregister_work: unregister the CEC adapter
> > */
> > struct drm_dp_aux_cec {
> > struct mutex lock;
> > struct cec_adapter *adap;
> > struct drm_connector *connector;
> > + bool is_mst;
> > struct delayed_work unregister_work;
> > };
> >
> > @@ -1746,7 +1748,7 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc, u32
> > edid_quirks,
> > #ifdef CONFIG_DRM_DP_CEC
> > void drm_dp_cec_irq(struct drm_dp_aux *aux);
> > void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> > - struct drm_connector *connector);
> > + struct drm_connector *connector, bool
> > is_mst);
> > void drm_dp_cec_unregister_connector(struct drm_dp_aux *aux);
> > void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid);
> > void drm_dp_cec_unset_edid(struct drm_dp_aux *aux);
> > @@ -1757,7 +1759,7 @@ static inline void drm_dp_cec_irq(struct drm_dp_aux
> > *aux)
> >
> > static inline void
> > drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> > - struct drm_connector *connector)
> > + struct drm_connector *connector, bool is_mst)
> > {
> > }
> >
> --
> Sincerely,
> Lyude Paul (she/her)
> Software Engineer at Red Hat
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 4/5] drm_dp_cec: add plumbing in preparation for MST support
2020-09-08 8:41 ` Hans Verkuil
@ 2020-09-09 6:59 ` Sam McNally
0 siblings, 0 replies; 12+ messages in thread
From: Sam McNally @ 2020-09-09 6:59 UTC (permalink / raw)
To: Hans Verkuil
Cc: LKML, David Airlie, nouveau, José Roberto de Souza,
David Francis, Rodrigo Siqueira, amd-gfx, Ben Skeggs,
Wambui Karuga, Thomas Zimmermann, Leo Li, Lucas De Marchi,
intel-gfx, Gwan-gyeong Mun, Rodrigo Vivi, Mikita Lipski,
Pankaj Bharadiya, dri-devel, Manasi Navare, Alex Deucher,
Christian König
On Tue, 8 Sep 2020 at 18:41, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 01/09/2020 08:22, Sam McNally wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> >
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > [sammc@chromium.org:
> > - rebased
> > - removed polling-related changes
> > - moved the calls to drm_dp_cec_(un)set_edid() into the next patch
> > ]
> > Signed-off-by: Sam McNally <sammc@chromium.org>
> > ---
> >
> > .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 2 +-
> > drivers/gpu/drm/drm_dp_cec.c | 22 ++++++++++---------
> > drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
> > drivers/gpu/drm/nouveau/nouveau_connector.c | 2 +-
> > include/drm/drm_dp_helper.h | 6 +++--
> > 5 files changed, 19 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > index 461fa4da0a34..6e7075893ec9 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > @@ -419,7 +419,7 @@ void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm,
> >
> > drm_dp_aux_init(&aconnector->dm_dp_aux.aux);
> > drm_dp_cec_register_connector(&aconnector->dm_dp_aux.aux,
> > - &aconnector->base);
> > + &aconnector->base, false);
> >
> > if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
> > return;
> > diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> > index 3ab2609f9ec7..04ab7b88055c 100644
> > --- a/drivers/gpu/drm/drm_dp_cec.c
> > +++ b/drivers/gpu/drm/drm_dp_cec.c
> > @@ -14,6 +14,7 @@
> > #include <drm/drm_connector.h>
> > #include <drm/drm_device.h>
> > #include <drm/drm_dp_helper.h>
> > +#include <drm/drm_dp_mst_helper.h>
> >
> > /*
> > * Unfortunately it turns out that we have a chicken-and-egg situation
> > @@ -338,8 +339,6 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> > if (aux->cec.adap) {
> > if (aux->cec.adap->capabilities == cec_caps &&
> > aux->cec.adap->available_log_addrs == num_las) {
> > - /* Unchanged, so just set the phys addr */
> > - cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> > goto unlock;
> > }
> > /*
> > @@ -364,15 +363,16 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> > if (cec_register_adapter(aux->cec.adap, connector->dev->dev)) {
> > cec_delete_adapter(aux->cec.adap);
> > aux->cec.adap = NULL;
> > - } else {
> > - /*
> > - * Update the phys addr for the new CEC adapter. When called
> > - * from drm_dp_cec_register_connector() edid == NULL, so in
> > - * that case the phys addr is just invalidated.
> > - */
> > - cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> > }
> > unlock:
> > + /*
> > + * Update the phys addr for the new CEC adapter. When called
> > + * from drm_dp_cec_register_connector() edid == NULL, so in
> > + * that case the phys addr is just invalidated.
> > + */
>
> The comment is no longer in sync with the code: if EDID == NULL, then
> nothing is done due to the edid check in the 'if' below.
>
> > + if (aux->cec.adap && edid) {
>
> I think this should just be: if (aux->cec.adap)
>
> Also, the {} aren't necessary here.
>
> > + cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> > + }
> > mutex_unlock(&aux->cec.lock);
> > }
> > EXPORT_SYMBOL(drm_dp_cec_set_edid);
>
> Frankly, the changes to this function should be dropped completely, from
> what I can see they are not necessary. It was done in my original patch
> because of the way I handled mst, but you did it differently (and I think
> better), so these changes are no longer needed.
>
> I know I am actually commenting on my old patch, but that patch was from a
> work-in-progress git branch and was never meant as a 'proper' patch.
>
> However, what complicates matters is that after digging a bit more I discovered
> that commit 732300154980 ("drm: Do not call drm_dp_cec_set_edid() while registering
> DP connectors") changed drm_dp_cec_register_connector() so that it no longer
> calls drm_dp_cec_set_edid(), but the comments there and in this function were
> not updated. It would be nice if you can add a patch fixing these outdated
> comments.
>
> Regardless of that change in commit 732300154980, the edid pointer can still be
> NULL and the existing behavior should be kept (i.e. create a CEC device, but with
> an invalid physical address since there is no EDID for some reason).
>
> Regards,
>
> Hans
>
Thanks. Leaving drm_dp_cec_set_edid() unchanged combined with Lyude's
suggestion to use aux->is_remote removes the need for this patch
entirely.
> > @@ -418,6 +418,7 @@ EXPORT_SYMBOL(drm_dp_cec_unset_edid);
> > * drm_dp_cec_register_connector() - register a new connector
> > * @aux: DisplayPort AUX channel
> > * @connector: drm connector
> > + * @is_mst: set to true if this is an MST branch
> > *
> > * A new connector was registered with associated CEC adapter name and
> > * CEC adapter parent device. After registering the name and parent
> > @@ -425,12 +426,13 @@ EXPORT_SYMBOL(drm_dp_cec_unset_edid);
> > * CEC and to register a CEC adapter if that is the case.
> > */
> > void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> > - struct drm_connector *connector)
> > + struct drm_connector *connector, bool is_mst)
> > {
> > WARN_ON(aux->cec.adap);
> > if (WARN_ON(!aux->transfer))
> > return;
> > aux->cec.connector = connector;
> > + aux->cec.is_mst = is_mst;
> > INIT_DELAYED_WORK(&aux->cec.unregister_work,
> > drm_dp_cec_unregister_work);
> > }
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 82b9de274f65..744cb55572f9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -6261,7 +6261,7 @@ intel_dp_connector_register(struct drm_connector *connector)
> > intel_dp->aux.dev = connector->kdev;
> > ret = drm_dp_aux_register(&intel_dp->aux);
> > if (!ret)
> > - drm_dp_cec_register_connector(&intel_dp->aux, connector);
> > + drm_dp_cec_register_connector(&intel_dp->aux, connector, false);
> > return ret;
> > }
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
> > index 49dd0cbc332f..671a70e95cd1 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> > @@ -1414,7 +1414,7 @@ nouveau_connector_create(struct drm_device *dev,
> > switch (type) {
> > case DRM_MODE_CONNECTOR_DisplayPort:
> > case DRM_MODE_CONNECTOR_eDP:
> > - drm_dp_cec_register_connector(&nv_connector->aux, connector);
> > + drm_dp_cec_register_connector(&nv_connector->aux, connector, false);
> > break;
> > }
> >
> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > index 85513eeb2196..12bca1b9512b 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -1495,12 +1495,14 @@ struct drm_connector;
> > * @lock: mutex protecting this struct
> > * @adap: the CEC adapter for CEC-Tunneling-over-AUX support.
> > * @connector: the connector this CEC adapter is associated with
> > + * @is_mst: this is an MST branch
> > * @unregister_work: unregister the CEC adapter
> > */
> > struct drm_dp_aux_cec {
> > struct mutex lock;
> > struct cec_adapter *adap;
> > struct drm_connector *connector;
> > + bool is_mst;
> > struct delayed_work unregister_work;
> > };
> >
> > @@ -1746,7 +1748,7 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc, u32 edid_quirks,
> > #ifdef CONFIG_DRM_DP_CEC
> > void drm_dp_cec_irq(struct drm_dp_aux *aux);
> > void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> > - struct drm_connector *connector);
> > + struct drm_connector *connector, bool is_mst);
> > void drm_dp_cec_unregister_connector(struct drm_dp_aux *aux);
> > void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid);
> > void drm_dp_cec_unset_edid(struct drm_dp_aux *aux);
> > @@ -1757,7 +1759,7 @@ static inline void drm_dp_cec_irq(struct drm_dp_aux *aux)
> >
> > static inline void
> > drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> > - struct drm_connector *connector)
> > + struct drm_connector *connector, bool is_mst)
> > {
> > }
> >
> >
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 5/5] drm_dp_cec: add the implementation of MST support
2020-09-08 8:08 ` Hans Verkuil
@ 2020-09-09 7:09 ` Sam McNally
0 siblings, 0 replies; 12+ messages in thread
From: Sam McNally @ 2020-09-09 7:09 UTC (permalink / raw)
To: Hans Verkuil
Cc: LKML, Thomas Zimmermann, David Airlie, Hans Verkuil, dri-devel
On Tue, 8 Sep 2020 at 18:08, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Hi Sam,
>
> On 01/09/2020 08:22, Sam McNally wrote:
> > With DP v2.0 errata E5, CEC tunneling can be supported through an MST
> > topology.
>
> Oh wow, this is finally supported in the spec. Very nice to see this.
> Also very nice to see that my old experimental patches attempting to
> support CEC on MST didn't go to waste!
>
> Do you know of any MST HW that supports this? I'd love to experiment
> with this.
A Realtek RTD2142 with the right firmware can support CEC over MST.
There's nothing released with it that I know of though.
>
> Regards,
>
> Hans
>
> >
> > There are some minor differences for CEC tunneling through an MST
> > topology compared to CEC tunneling to an SST port:
> > - CEC IRQs are delivered via a sink event notify message
> > - CEC-related DPCD registers are accessed via remote DPCD reads and
> > writes.
> >
> > This results in the MST implementation diverging from the existing SST
> > implementation:
> > - sink event notify messages with CEC_IRQ ID set indicate CEC IRQ rather
> > than ESI1
> > - setting edid and handling CEC IRQs, which can be triggered from
> > contexts where locks held preclude HPD handling, are deferred to avoid
> > remote DPCD access which would block until HPD handling is performed
> > or a timeout
> >
> > Register and unregister for all MST connectors, ensuring their
> > drm_dp_aux_cec struct won't be accessed uninitialized.
> >
> > Signed-off-by: Sam McNally <sammc@chromium.org>
> > ---
> >
> > drivers/gpu/drm/drm_dp_cec.c | 67 +++++++++++++++++++++++++--
> > drivers/gpu/drm/drm_dp_mst_topology.c | 24 ++++++++++
> > include/drm/drm_dp_helper.h | 4 ++
> > 3 files changed, 90 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> > index 04ab7b88055c..9f6aeaa27f00 100644
> > --- a/drivers/gpu/drm/drm_dp_cec.c
> > +++ b/drivers/gpu/drm/drm_dp_cec.c
> > @@ -249,6 +249,10 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux)
> > if (!aux->transfer)
> > return;
> >
> > + if (aux->cec.is_mst) {
> > + schedule_work(&aux->cec.mst_irq_work);
> > + return;
> > + }
> > mutex_lock(&aux->cec.lock);
> > if (!aux->cec.adap)
> > goto unlock;
> > @@ -277,6 +281,24 @@ static bool drm_dp_cec_cap(struct drm_dp_aux *aux, u8 *cec_cap)
> > return true;
> > }
> >
> > +static void drm_dp_cec_mst_irq_work(struct work_struct *work)
> > +{
> > + struct drm_dp_aux *aux = container_of(work, struct drm_dp_aux,
> > + cec.mst_irq_work);
> > + struct drm_dp_mst_port *port =
> > + container_of(aux, struct drm_dp_mst_port, aux);
> > +
> > + port = drm_dp_mst_topology_get_port_validated(port->mgr, port);
> > + if (!port)
> > + return;
> > + mutex_lock(&aux->cec.lock);
> > + if (aux->cec.adap)
> > + drm_dp_cec_handle_irq(aux);
> > +
> > + mutex_unlock(&aux->cec.lock);
> > + drm_dp_mst_topology_put_port(port);
> > +}
> > +
> > /*
> > * Called if the HPD was low for more than drm_dp_cec_unregister_delay
> > * seconds. This unregisters the CEC adapter.
> > @@ -298,7 +320,8 @@ static void drm_dp_cec_unregister_work(struct work_struct *work)
> > * were unchanged and just update the CEC physical address. Otherwise
> > * unregister the old CEC adapter and create a new one.
> > */
> > -void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> > +static void drm_dp_cec_handle_set_edid(struct drm_dp_aux *aux,
> > + const struct edid *edid)
> > {
> > struct drm_connector *connector = aux->cec.connector;
> > u32 cec_caps = CEC_CAP_DEFAULTS | CEC_CAP_NEEDS_HPD |
> > @@ -307,10 +330,6 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> > unsigned int num_las = 1;
> > u8 cap;
> >
> > - /* No transfer function was set, so not a DP connector */
> > - if (!aux->transfer)
> > - return;
> > -
> > #ifndef CONFIG_MEDIA_CEC_RC
> > /*
> > * CEC_CAP_RC is part of CEC_CAP_DEFAULTS, but it is stripped by
> > @@ -321,6 +340,7 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> > */
> > cec_caps &= ~CEC_CAP_RC;
> > #endif
> > + cancel_work_sync(&aux->cec.mst_irq_work);
> > cancel_delayed_work_sync(&aux->cec.unregister_work);
> >
> > mutex_lock(&aux->cec.lock);
> > @@ -375,6 +395,18 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> > }
> > mutex_unlock(&aux->cec.lock);
> > }
> > +
> > +void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> > +{
> > + /* No transfer function was set, so not a DP connector */
> > + if (!aux->transfer)
> > + return;
> > +
> > + if (aux->cec.is_mst)
> > + schedule_work(&aux->cec.mst_set_edid_work);
> > + else
> > + drm_dp_cec_handle_set_edid(aux, edid);
> > +}
> > EXPORT_SYMBOL(drm_dp_cec_set_edid);
> >
> > /*
> > @@ -393,6 +425,8 @@ void drm_dp_cec_unset_edid(struct drm_dp_aux *aux)
> > goto unlock;
> >
> > cec_phys_addr_invalidate(aux->cec.adap);
> > + cancel_work_sync(&aux->cec.mst_irq_work);
> > +
> > /*
> > * We're done if we want to keep the CEC device
> > * (drm_dp_cec_unregister_delay is >= NEVER_UNREG_DELAY) or if the
> > @@ -414,6 +448,25 @@ void drm_dp_cec_unset_edid(struct drm_dp_aux *aux)
> > }
> > EXPORT_SYMBOL(drm_dp_cec_unset_edid);
> >
> > +static void drm_dp_cec_mst_set_edid_work(struct work_struct *work)
> > +{
> > + struct drm_dp_aux *aux =
> > + container_of(work, struct drm_dp_aux, cec.mst_set_edid_work);
> > + struct drm_dp_mst_port *port =
> > + container_of(aux, struct drm_dp_mst_port, aux);
> > + struct edid *edid = NULL;
> > +
> > + port = drm_dp_mst_topology_get_port_validated(port->mgr, port);
> > + if (!port)
> > + return;
> > +
> > + edid = drm_get_edid(port->connector, &port->aux.ddc);
> > +
> > + drm_dp_cec_handle_set_edid(aux, edid);
> > +
> > + drm_dp_mst_topology_put_port(port);
> > +}
> > +
> > /**
> > * drm_dp_cec_register_connector() - register a new connector
> > * @aux: DisplayPort AUX channel
> > @@ -435,6 +488,8 @@ void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> > aux->cec.is_mst = is_mst;
> > INIT_DELAYED_WORK(&aux->cec.unregister_work,
> > drm_dp_cec_unregister_work);
> > + INIT_WORK(&aux->cec.mst_irq_work, drm_dp_cec_mst_irq_work);
> > + INIT_WORK(&aux->cec.mst_set_edid_work, drm_dp_cec_mst_set_edid_work);
> > }
> > EXPORT_SYMBOL(drm_dp_cec_register_connector);
> >
> > @@ -444,6 +499,8 @@ EXPORT_SYMBOL(drm_dp_cec_register_connector);
> > */
> > void drm_dp_cec_unregister_connector(struct drm_dp_aux *aux)
> > {
> > + cancel_work_sync(&aux->cec.mst_irq_work);
> > + cancel_work_sync(&aux->cec.mst_set_edid_work);
> > if (!aux->cec.adap)
> > return;
> > cancel_delayed_work_sync(&aux->cec.unregister_work);
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index c783a2a1c114..fd9430d88fd6 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -2183,6 +2183,8 @@ static void build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
> > int drm_dp_mst_connector_late_register(struct drm_connector *connector,
> > struct drm_dp_mst_port *port)
> > {
> > + drm_dp_cec_register_connector(&port->aux, connector, true);
> > +
> > DRM_DEBUG_KMS("registering %s remote bus for %s\n",
> > port->aux.name, connector->kdev->kobj.name);
> >
> > @@ -2206,6 +2208,8 @@ void drm_dp_mst_connector_early_unregister(struct drm_connector *connector,
> > DRM_DEBUG_KMS("unregistering %s remote bus for %s\n",
> > port->aux.name, connector->kdev->kobj.name);
> > drm_dp_aux_unregister_devnode(&port->aux);
> > +
> > + drm_dp_cec_unregister_connector(&port->aux);
> > }
> > EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister);
> >
> > @@ -2515,6 +2519,21 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> > queue_work(system_long_wq, &mstb->mgr->work);
> > }
> >
> > +static void
> > +drm_dp_mst_handle_sink_event(struct drm_dp_mst_branch *mstb,
> > + struct drm_dp_sink_event_notify *sink_event)
> > +{
> > + struct drm_dp_mst_port *port;
> > +
> > + if (sink_event->event_id & DP_SINK_EVENT_CEC_IRQ_EVENT) {
> > + port = drm_dp_get_port(mstb, sink_event->port_number);
> > + if (port) {
> > + drm_dp_cec_irq(&port->aux);
> > + drm_dp_mst_topology_put_port(port);
> > + }
> > + }
> > +}
> > +
> > static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct drm_dp_mst_topology_mgr *mgr,
> > u8 lct, u8 *rad)
> > {
> > @@ -3954,6 +3973,8 @@ drm_dp_mst_process_up_req(struct drm_dp_mst_topology_mgr *mgr,
> > if (msg->req_type == DP_CONNECTION_STATUS_NOTIFY) {
> > drm_dp_mst_handle_conn_stat(mstb, &msg->u.conn_stat);
> > hotplug = true;
> > + } else if (msg->req_type == DP_SINK_EVENT_NOTIFY) {
> > + drm_dp_mst_handle_sink_event(mstb, &msg->u.sink_event);
> > }
> >
> > drm_dp_mst_topology_put_mstb(mstb);
> > @@ -4147,6 +4168,8 @@ drm_dp_mst_detect_port(struct drm_connector *connector,
> > break;
> > }
> > out:
> > + if (ret != connector_status_connected)
> > + drm_dp_cec_unset_edid(&port->aux);
> > drm_dp_mst_topology_put_port(port);
> > return ret;
> > }
> > @@ -4177,6 +4200,7 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_
> > edid = drm_get_edid(connector, &port->aux.ddc);
> > }
> > port->has_audio = drm_detect_monitor_audio(edid);
> > + drm_dp_cec_set_edid(&port->aux, edid);
> > drm_dp_mst_topology_put_port(port);
> > return edid;
> > }
> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > index 12bca1b9512b..e973eba06875 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -1497,6 +1497,8 @@ struct drm_connector;
> > * @connector: the connector this CEC adapter is associated with
> > * @is_mst: this is an MST branch
> > * @unregister_work: unregister the CEC adapter
> > + * @mst_irq_work: IRQ for CEC events on an MST branch
> > + * @mst_set_edid_work: set the EDID for an MST branch
> > */
> > struct drm_dp_aux_cec {
> > struct mutex lock;
> > @@ -1504,6 +1506,8 @@ struct drm_dp_aux_cec {
> > struct drm_connector *connector;
> > bool is_mst;
> > struct delayed_work unregister_work;
> > + struct work_struct mst_irq_work;
> > + struct work_struct mst_set_edid_work;
> > };
> >
> > /**
> >
>
On Tue, 8 Sep 2020 at 18:08, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Hi Sam,
>
> On 01/09/2020 08:22, Sam McNally wrote:
> > With DP v2.0 errata E5, CEC tunneling can be supported through an MST
> > topology.
>
> Oh wow, this is finally supported in the spec. Very nice to see this.
> Also very nice to see that my old experimental patches attempting to
> support CEC on MST didn't go to waste!
>
> Do you know of any MST HW that supports this? I'd love to experiment
> with this.
>
> Regards,
>
> Hans
>
> >
> > There are some minor differences for CEC tunneling through an MST
> > topology compared to CEC tunneling to an SST port:
> > - CEC IRQs are delivered via a sink event notify message
> > - CEC-related DPCD registers are accessed via remote DPCD reads and
> > writes.
> >
> > This results in the MST implementation diverging from the existing SST
> > implementation:
> > - sink event notify messages with CEC_IRQ ID set indicate CEC IRQ rather
> > than ESI1
> > - setting edid and handling CEC IRQs, which can be triggered from
> > contexts where locks held preclude HPD handling, are deferred to avoid
> > remote DPCD access which would block until HPD handling is performed
> > or a timeout
> >
> > Register and unregister for all MST connectors, ensuring their
> > drm_dp_aux_cec struct won't be accessed uninitialized.
> >
> > Signed-off-by: Sam McNally <sammc@chromium.org>
> > ---
> >
> > drivers/gpu/drm/drm_dp_cec.c | 67 +++++++++++++++++++++++++--
> > drivers/gpu/drm/drm_dp_mst_topology.c | 24 ++++++++++
> > include/drm/drm_dp_helper.h | 4 ++
> > 3 files changed, 90 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> > index 04ab7b88055c..9f6aeaa27f00 100644
> > --- a/drivers/gpu/drm/drm_dp_cec.c
> > +++ b/drivers/gpu/drm/drm_dp_cec.c
> > @@ -249,6 +249,10 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux)
> > if (!aux->transfer)
> > return;
> >
> > + if (aux->cec.is_mst) {
> > + schedule_work(&aux->cec.mst_irq_work);
> > + return;
> > + }
> > mutex_lock(&aux->cec.lock);
> > if (!aux->cec.adap)
> > goto unlock;
> > @@ -277,6 +281,24 @@ static bool drm_dp_cec_cap(struct drm_dp_aux *aux, u8 *cec_cap)
> > return true;
> > }
> >
> > +static void drm_dp_cec_mst_irq_work(struct work_struct *work)
> > +{
> > + struct drm_dp_aux *aux = container_of(work, struct drm_dp_aux,
> > + cec.mst_irq_work);
> > + struct drm_dp_mst_port *port =
> > + container_of(aux, struct drm_dp_mst_port, aux);
> > +
> > + port = drm_dp_mst_topology_get_port_validated(port->mgr, port);
> > + if (!port)
> > + return;
> > + mutex_lock(&aux->cec.lock);
> > + if (aux->cec.adap)
> > + drm_dp_cec_handle_irq(aux);
> > +
> > + mutex_unlock(&aux->cec.lock);
> > + drm_dp_mst_topology_put_port(port);
> > +}
> > +
> > /*
> > * Called if the HPD was low for more than drm_dp_cec_unregister_delay
> > * seconds. This unregisters the CEC adapter.
> > @@ -298,7 +320,8 @@ static void drm_dp_cec_unregister_work(struct work_struct *work)
> > * were unchanged and just update the CEC physical address. Otherwise
> > * unregister the old CEC adapter and create a new one.
> > */
> > -void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> > +static void drm_dp_cec_handle_set_edid(struct drm_dp_aux *aux,
> > + const struct edid *edid)
> > {
> > struct drm_connector *connector = aux->cec.connector;
> > u32 cec_caps = CEC_CAP_DEFAULTS | CEC_CAP_NEEDS_HPD |
> > @@ -307,10 +330,6 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> > unsigned int num_las = 1;
> > u8 cap;
> >
> > - /* No transfer function was set, so not a DP connector */
> > - if (!aux->transfer)
> > - return;
> > -
> > #ifndef CONFIG_MEDIA_CEC_RC
> > /*
> > * CEC_CAP_RC is part of CEC_CAP_DEFAULTS, but it is stripped by
> > @@ -321,6 +340,7 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> > */
> > cec_caps &= ~CEC_CAP_RC;
> > #endif
> > + cancel_work_sync(&aux->cec.mst_irq_work);
> > cancel_delayed_work_sync(&aux->cec.unregister_work);
> >
> > mutex_lock(&aux->cec.lock);
> > @@ -375,6 +395,18 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> > }
> > mutex_unlock(&aux->cec.lock);
> > }
> > +
> > +void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> > +{
> > + /* No transfer function was set, so not a DP connector */
> > + if (!aux->transfer)
> > + return;
> > +
> > + if (aux->cec.is_mst)
> > + schedule_work(&aux->cec.mst_set_edid_work);
> > + else
> > + drm_dp_cec_handle_set_edid(aux, edid);
> > +}
> > EXPORT_SYMBOL(drm_dp_cec_set_edid);
> >
> > /*
> > @@ -393,6 +425,8 @@ void drm_dp_cec_unset_edid(struct drm_dp_aux *aux)
> > goto unlock;
> >
> > cec_phys_addr_invalidate(aux->cec.adap);
> > + cancel_work_sync(&aux->cec.mst_irq_work);
> > +
> > /*
> > * We're done if we want to keep the CEC device
> > * (drm_dp_cec_unregister_delay is >= NEVER_UNREG_DELAY) or if the
> > @@ -414,6 +448,25 @@ void drm_dp_cec_unset_edid(struct drm_dp_aux *aux)
> > }
> > EXPORT_SYMBOL(drm_dp_cec_unset_edid);
> >
> > +static void drm_dp_cec_mst_set_edid_work(struct work_struct *work)
> > +{
> > + struct drm_dp_aux *aux =
> > + container_of(work, struct drm_dp_aux, cec.mst_set_edid_work);
> > + struct drm_dp_mst_port *port =
> > + container_of(aux, struct drm_dp_mst_port, aux);
> > + struct edid *edid = NULL;
> > +
> > + port = drm_dp_mst_topology_get_port_validated(port->mgr, port);
> > + if (!port)
> > + return;
> > +
> > + edid = drm_get_edid(port->connector, &port->aux.ddc);
> > +
> > + drm_dp_cec_handle_set_edid(aux, edid);
> > +
> > + drm_dp_mst_topology_put_port(port);
> > +}
> > +
> > /**
> > * drm_dp_cec_register_connector() - register a new connector
> > * @aux: DisplayPort AUX channel
> > @@ -435,6 +488,8 @@ void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> > aux->cec.is_mst = is_mst;
> > INIT_DELAYED_WORK(&aux->cec.unregister_work,
> > drm_dp_cec_unregister_work);
> > + INIT_WORK(&aux->cec.mst_irq_work, drm_dp_cec_mst_irq_work);
> > + INIT_WORK(&aux->cec.mst_set_edid_work, drm_dp_cec_mst_set_edid_work);
> > }
> > EXPORT_SYMBOL(drm_dp_cec_register_connector);
> >
> > @@ -444,6 +499,8 @@ EXPORT_SYMBOL(drm_dp_cec_register_connector);
> > */
> > void drm_dp_cec_unregister_connector(struct drm_dp_aux *aux)
> > {
> > + cancel_work_sync(&aux->cec.mst_irq_work);
> > + cancel_work_sync(&aux->cec.mst_set_edid_work);
> > if (!aux->cec.adap)
> > return;
> > cancel_delayed_work_sync(&aux->cec.unregister_work);
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index c783a2a1c114..fd9430d88fd6 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -2183,6 +2183,8 @@ static void build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
> > int drm_dp_mst_connector_late_register(struct drm_connector *connector,
> > struct drm_dp_mst_port *port)
> > {
> > + drm_dp_cec_register_connector(&port->aux, connector, true);
> > +
> > DRM_DEBUG_KMS("registering %s remote bus for %s\n",
> > port->aux.name, connector->kdev->kobj.name);
> >
> > @@ -2206,6 +2208,8 @@ void drm_dp_mst_connector_early_unregister(struct drm_connector *connector,
> > DRM_DEBUG_KMS("unregistering %s remote bus for %s\n",
> > port->aux.name, connector->kdev->kobj.name);
> > drm_dp_aux_unregister_devnode(&port->aux);
> > +
> > + drm_dp_cec_unregister_connector(&port->aux);
> > }
> > EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister);
> >
> > @@ -2515,6 +2519,21 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> > queue_work(system_long_wq, &mstb->mgr->work);
> > }
> >
> > +static void
> > +drm_dp_mst_handle_sink_event(struct drm_dp_mst_branch *mstb,
> > + struct drm_dp_sink_event_notify *sink_event)
> > +{
> > + struct drm_dp_mst_port *port;
> > +
> > + if (sink_event->event_id & DP_SINK_EVENT_CEC_IRQ_EVENT) {
> > + port = drm_dp_get_port(mstb, sink_event->port_number);
> > + if (port) {
> > + drm_dp_cec_irq(&port->aux);
> > + drm_dp_mst_topology_put_port(port);
> > + }
> > + }
> > +}
> > +
> > static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct drm_dp_mst_topology_mgr *mgr,
> > u8 lct, u8 *rad)
> > {
> > @@ -3954,6 +3973,8 @@ drm_dp_mst_process_up_req(struct drm_dp_mst_topology_mgr *mgr,
> > if (msg->req_type == DP_CONNECTION_STATUS_NOTIFY) {
> > drm_dp_mst_handle_conn_stat(mstb, &msg->u.conn_stat);
> > hotplug = true;
> > + } else if (msg->req_type == DP_SINK_EVENT_NOTIFY) {
> > + drm_dp_mst_handle_sink_event(mstb, &msg->u.sink_event);
> > }
> >
> > drm_dp_mst_topology_put_mstb(mstb);
> > @@ -4147,6 +4168,8 @@ drm_dp_mst_detect_port(struct drm_connector *connector,
> > break;
> > }
> > out:
> > + if (ret != connector_status_connected)
> > + drm_dp_cec_unset_edid(&port->aux);
> > drm_dp_mst_topology_put_port(port);
> > return ret;
> > }
> > @@ -4177,6 +4200,7 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_
> > edid = drm_get_edid(connector, &port->aux.ddc);
> > }
> > port->has_audio = drm_detect_monitor_audio(edid);
> > + drm_dp_cec_set_edid(&port->aux, edid);
> > drm_dp_mst_topology_put_port(port);
> > return edid;
> > }
> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > index 12bca1b9512b..e973eba06875 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -1497,6 +1497,8 @@ struct drm_connector;
> > * @connector: the connector this CEC adapter is associated with
> > * @is_mst: this is an MST branch
> > * @unregister_work: unregister the CEC adapter
> > + * @mst_irq_work: IRQ for CEC events on an MST branch
> > + * @mst_set_edid_work: set the EDID for an MST branch
> > */
> > struct drm_dp_aux_cec {
> > struct mutex lock;
> > @@ -1504,6 +1506,8 @@ struct drm_dp_aux_cec {
> > struct drm_connector *connector;
> > bool is_mst;
> > struct delayed_work unregister_work;
> > + struct work_struct mst_irq_work;
> > + struct work_struct mst_set_edid_work;
> > };
> >
> > /**
> >
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-09-09 7:10 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-01 6:22 [PATCH 1/5] dp/dp_mst: Add support for sink event notify messages Sam McNally
2020-09-01 6:22 ` [PATCH 2/5] drm_dp_mst_topology: use correct AUX channel Sam McNally
2020-09-01 6:22 ` [PATCH 3/5] drm_dp_mst_topology: export two functions Sam McNally
2020-09-01 6:22 ` [PATCH 4/5] drm_dp_cec: add plumbing in preparation for MST support Sam McNally
2020-09-01 18:12 ` Lyude Paul
2020-09-09 6:59 ` Sam McNally
2020-09-08 8:41 ` Hans Verkuil
2020-09-09 6:59 ` Sam McNally
2020-09-01 6:22 ` [PATCH 5/5] drm_dp_cec: add the implementation of " Sam McNally
2020-09-08 8:08 ` Hans Verkuil
2020-09-09 7:09 ` Sam McNally
2020-09-08 9:24 ` Hans Verkuil
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).