2020-09-23 02:58:23

by Sam McNally

Subject: [PATCH v3 1/4] dp/dp_mst: Add support for sink event notify messages

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 <[email protected]>
---

(no changes since v1)

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.681.g6f77f65b4e-goog

2020-09-23 02:59:26

by Sam McNally

Subject: [PATCH v3 3/4] drm_dp_mst_topology: export two functions

From: Hans Verkuil <[email protected]>

These are required for the CEC MST support.

Signed-off-by: Hans Verkuil <[email protected]>
Signed-off-by: Sam McNally <[email protected]>
---

(no changes since v1)

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.681.g6f77f65b4e-goog

2020-09-23 02:59:34

by Sam McNally

Subject: [PATCH v3 2/4] drm_dp_mst_topology: use correct AUX channel

From: Hans Verkuil <[email protected]>

For adapters behind an MST hub use the correct AUX channel.

Signed-off-by: Hans Verkuil <[email protected]>
[[email protected]: rebased, removing redundant changes]
Signed-off-by: Sam McNally <[email protected]>
---

(no changes since v1)

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.681.g6f77f65b4e-goog

2020-09-23 02:59:34

by Sam McNally

Subject: [PATCH v3 4/4] drm_dp_cec: add MST support

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.

Reviewed-by: Hans Verkuil <[email protected]>
Signed-off-by: Sam McNally <[email protected]>
---

Changes in v3:
- Fixed whitespace in drm_dp_cec_mst_irq_work()
- Moved drm_dp_cec_mst_set_edid_work() with the other set_edid functions

Changes in v2:
- Used aux->is_remote instead of aux->cec.is_mst, removing the need for
the previous patch in the series
- Added a defensive check for null edid in the deferred set_edid work,
in case the edid is no longer valid at that point

drivers/gpu/drm/drm_dp_cec.c | 68 +++++++++++++++++++++++++--
drivers/gpu/drm/drm_dp_mst_topology.c | 24 ++++++++++
include/drm/drm_dp_helper.h | 4 ++
3 files changed, 91 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
index 3ab2609f9ec7..1020b2cffdf0 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
@@ -248,6 +249,10 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux)
if (!aux->transfer)
return;

+ if (aux->is_remote) {
+ schedule_work(&aux->cec.mst_irq_work);
+ return;
+ }
mutex_lock(&aux->cec.lock);
if (!aux->cec.adap)
goto unlock;
@@ -276,6 +281,23 @@ 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.
@@ -297,7 +319,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 |
@@ -306,10 +329,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
@@ -320,6 +339,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,8 +395,40 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
unlock:
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->is_remote)
+ schedule_work(&aux->cec.mst_set_edid_work);
+ else
+ drm_dp_cec_handle_set_edid(aux, edid);
+}
EXPORT_SYMBOL(drm_dp_cec_set_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);
+
+ if (edid)
+ drm_dp_cec_handle_set_edid(aux, edid);
+
+ drm_dp_mst_topology_put_port(port);
+}
+
/*
* The EDID disappeared (likely because of the HPD going down).
*/
@@ -393,6 +445,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
@@ -433,6 +487,8 @@ void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
aux->cec.connector = connector;
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);

@@ -442,6 +498,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..221c30133739 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);
+
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 85513eeb2196..d8ee24a6319c 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1496,12 +1496,16 @@ struct drm_connector;
* @adap: the CEC adapter for CEC-Tunneling-over-AUX support.
* @connector: the connector this CEC adapter is associated with
* @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;
struct cec_adapter *adap;
struct drm_connector *connector;
struct delayed_work unregister_work;
+ struct work_struct mst_irq_work;
+ struct work_struct mst_set_edid_work;
};

/**
--
2.28.0.681.g6f77f65b4e-goog

2021-01-13 01:26:14

by Hans Verkuil

Subject: Re: [PATCH v3 1/4] dp/dp_mst: Add support for sink event notify messages

Hi Sam,

This series still hasn't been merged. It still applies cleanly to v5.11-rc1.

Daniel, can you merge this series for 5.12? Or Ack this series so I can merge it?

The first three patches deal with DP MST support, and this needs review from
you or David.

Regards,

Hans

On 23/09/2020 04:13, Sam McNally wrote:
> 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 <[email protected]>
> ---
>
> (no changes since v1)
>
> 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;
>

2021-02-01 09:59:35

by Hans Verkuil

Subject: Re: [PATCH v3 1/4] dp/dp_mst: Add support for sink event notify messages

Hi Lyude,

Daniel referred me to you as the best person to review the MST parts of this
series.

I can commit this, but then I prefer to have a Reviewed-by or Acked-by from
someone for the first 3 DP MST patches. Alternatively, you can take the whole
series (I've reviewed the 4th CEC patch).

Regards,

Hans

On 12/01/2021 10:24, Hans Verkuil wrote:
> Hi Sam,
>
> This series still hasn't been merged. It still applies cleanly to v5.11-rc1.
>
> Daniel, can you merge this series for 5.12? Or Ack this series so I can merge it?
>
> The first three patches deal with DP MST support, and this needs review from
> you or David.
>
> Regards,
>
> Hans
>
> On 23/09/2020 04:13, Sam McNally wrote:
>> 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 <[email protected]>
>> ---
>>
>> (no changes since v1)
>>
>> 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;
>>
>

2021-02-01 22:00:13

by Lyude Paul

Subject: Re: [PATCH v3 1/4] dp/dp_mst: Add support for sink event notify messages

On Wed, 2020-09-23 at 12:13 +1000, Sam McNally wrote:
> 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 <[email protected]>
> ---
>
> (no changes since v1)
>
>  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);

Is it possible for us to use drm_dbg_kms() here?

Also-there is an MST selftest you should update for this

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

--
Sincerely,
Lyude Paul (she/her)
Software Engineer at Red Hat

Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to check
on my status. I don't bite!

2021-02-01 22:01:54

by Lyude Paul

Subject: Re: [PATCH v3 2/4] drm_dp_mst_topology: use correct AUX channel

On Wed, 2020-09-23 at 12:13 +1000, Sam McNally wrote:
> From: Hans Verkuil <[email protected]>
>
> For adapters behind an MST hub use the correct AUX channel.
>
> Signed-off-by: Hans Verkuil <[email protected]>
> [[email protected]: rebased, removing redundant changes]
> Signed-off-by: Sam McNally <[email protected]>
> ---
>
> (no changes since v1)
>
>  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);

You're missing a matching mutex_destroy() for both of these

With that fixed:

Reviewed-by: Lyude Paul <[email protected]>

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

--
Sincerely,
Lyude Paul (she/her)
Software Engineer at Red Hat

Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to check
on my status. I don't bite!

2021-02-01 22:07:55

by Lyude Paul

Subject: Re: [PATCH v3 3/4] drm_dp_mst_topology: export two functions

On Wed, 2020-09-23 at 12:13 +1000, Sam McNally wrote:
> From: Hans Verkuil <[email protected]>
>
> These are required for the CEC MST support.
>
> Signed-off-by: Hans Verkuil <[email protected]>
> Signed-off-by: Sam McNally <[email protected]>
> ---
>
> (no changes since v1)
>
>  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)

Mhhhhhh-can you think of some way around this? I really don't think it's a good
idea for us to be exposing topology references to things as-is, the thing is
they're really meant to be used for critical sections in code where it'd become
very painful to deal with an mst port disappearing from under us. Outside of MST
helpers, everyone else should be dealing with the expectation that these things
can disappear as a result of hotplugs at any moment.

Note that we do expose malloc refs, but that's intentional as holding a malloc
ref to something doesn't cause it to stay around even when it's unplugged - it
just stops it from being unallocated.


>  {
>         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);
>  
>  

--
Sincerely,
Lyude Paul (she/her)
Software Engineer at Red Hat

Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to check
on my status. I don't bite!

2021-02-01 22:17:14

by Ville Syrjälä

Subject: Re: [PATCH v3 2/4] drm_dp_mst_topology: use correct AUX channel

On Wed, Sep 23, 2020 at 12:13:53PM +1000, Sam McNally wrote:
> From: Hans Verkuil <[email protected]>
>
> For adapters behind an MST hub use the correct AUX channel.
>
> Signed-off-by: Hans Verkuil <[email protected]>
> [[email protected]: rebased, removing redundant changes]
> Signed-off-by: Sam McNally <[email protected]>
> ---
>
> (no changes since v1)
>
> 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;
> +

This was supposed to be handled via higher levels checking for
is_remote==true.

> /* 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);

That doesn't make sense to me. I2c writes and DPCD writes
are definitely not the same thing.

aux->transfer is a very low level thing. I don't think it's the
correct level of abstraction for sideband.

> + 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.681.g6f77f65b4e-goog
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

--
Ville Syrj?l?
Intel

2021-02-03 10:00:20

by Hans Verkuil

Subject: Re: [PATCH v3 1/4] dp/dp_mst: Add support for sink event notify messages

Hi Sam,

Are you able to work on a v4?

I haven't heard from you for some time now. I would be willing to take over
this series if it wasn't for the fact that I do not have any hardware to test
this with.

Regards,

Hans

On 01/02/2021 22:56, Lyude Paul wrote:
> On Wed, 2020-09-23 at 12:13 +1000, Sam McNally wrote:
>> 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 <[email protected]>
>> ---
>>
>> (no changes since v1)
>>
>>  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);
>
> Is it possible for us to use drm_dbg_kms() here?
>
> Also-there is an MST selftest you should update for this
>
>> +       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;
>

2021-02-04 10:22:23

by Hans Verkuil

Subject: Re: [PATCH v3 2/4] drm_dp_mst_topology: use correct AUX channel

On 01/02/2021 23:13, Ville Syrjälä wrote:
> On Wed, Sep 23, 2020 at 12:13:53PM +1000, Sam McNally wrote:
>> From: Hans Verkuil <[email protected]>
>>
>> For adapters behind an MST hub use the correct AUX channel.
>>
>> Signed-off-by: Hans Verkuil <[email protected]>
>> [[email protected]: rebased, removing redundant changes]
>> Signed-off-by: Sam McNally <[email protected]>
>> ---
>>
>> (no changes since v1)
>>
>> 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;
>> +
>
> This was supposed to be handled via higher levels checking for
> is_remote==true.

Ah, I suspect this patch can be dropped entirely: it predates commit 2f221a5efed4
("drm/dp_mst: Add MST support to DP DPCD R/W functions").

It looks like that commit basically solved what this older patch attempts to do
as well.

Sam, can you test if it works without this patch?

Regards,

Hans

>
>> /* 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);
>
> That doesn't make sense to me. I2c writes and DPCD writes
> are definitely not the same thing.
>
> aux->transfer is a very low level thing. I don't think it's the
> correct level of abstraction for sideband.
>
>> + 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.681.g6f77f65b4e-goog
>>
>> _______________________________________________
>> dri-devel mailing list
>> [email protected]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

2021-02-04 10:47:27

by Hans Verkuil

Subject: Re: [PATCH v3 1/4] dp/dp_mst: Add support for sink event notify messages

Hi Sam,

I replied to several of the patches: it looks like the drm code has changed since
some of this patches were written, and I think it can be simplified quite a bit.

Regards,

Hans

On 04/02/2021 10:54, Sam McNally wrote:
> I can for this patch; I'm not really sure of the right approach for the other two though.
>
> On Wed, 3 Feb 2021 at 20:57, Hans Verkuil <[email protected] <mailto:[email protected]>> wrote:
>
> Hi Sam,
>
> Are you able to work on a v4?
>
> I haven't heard from you for some time now. I would be willing to take over
> this series if it wasn't for the fact that I do not have any hardware to test
> this with.
>
> Regards,
>
>         Hans
>
> On 01/02/2021 22:56, Lyude Paul wrote:
> > On Wed, 2020-09-23 at 12:13 +1000, Sam McNally wrote:
> >> 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 <[email protected] <mailto:[email protected]>>
> >> ---
> >>
> >> (no changes since v1)
> >>
> >>  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);
> >
> > Is it possible for us to use drm_dbg_kms() here?
> >
> > Also-there is an MST selftest you should update for this
> >
> >> +       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;
> >
>

2021-02-04 23:46:13

by Hans Verkuil

Subject: Re: [PATCH v3 4/4] drm_dp_cec: add MST support

On 23/09/2020 04:13, 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.
>
> Reviewed-by: Hans Verkuil <[email protected]>
> Signed-off-by: Sam McNally <[email protected]>
> ---
>
> Changes in v3:
> - Fixed whitespace in drm_dp_cec_mst_irq_work()
> - Moved drm_dp_cec_mst_set_edid_work() with the other set_edid functions
>
> Changes in v2:
> - Used aux->is_remote instead of aux->cec.is_mst, removing the need for
> the previous patch in the series
> - Added a defensive check for null edid in the deferred set_edid work,
> in case the edid is no longer valid at that point
>
> drivers/gpu/drm/drm_dp_cec.c | 68 +++++++++++++++++++++++++--
> drivers/gpu/drm/drm_dp_mst_topology.c | 24 ++++++++++
> include/drm/drm_dp_helper.h | 4 ++
> 3 files changed, 91 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> index 3ab2609f9ec7..1020b2cffdf0 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
> @@ -248,6 +249,10 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux)
> if (!aux->transfer)
> return;
>
> + if (aux->is_remote) {
> + schedule_work(&aux->cec.mst_irq_work);
> + return;
> + }

Are these workqueues for cec_irq and cec_set_edid actually needed? They are called
directly from drm_dp_mst_topology.c, and I think they can be handled identically to
a normal, non-remote, aux device.

Avoiding using a workqueue probably also means that there is no need for exporting
drm_dp_mst_topology_get_port_validated and drm_dp_mst_topology_put_port.

Provided that I am not missing something, this should simplify the code quite a bit.

Regards,

Hans

> mutex_lock(&aux->cec.lock);
> if (!aux->cec.adap)
> goto unlock;
> @@ -276,6 +281,23 @@ 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.
> @@ -297,7 +319,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 |
> @@ -306,10 +329,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
> @@ -320,6 +339,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,8 +395,40 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> unlock:
> 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->is_remote)
> + schedule_work(&aux->cec.mst_set_edid_work);
> + else
> + drm_dp_cec_handle_set_edid(aux, edid);
> +}
> EXPORT_SYMBOL(drm_dp_cec_set_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);
> +
> + if (edid)
> + drm_dp_cec_handle_set_edid(aux, edid);
> +
> + drm_dp_mst_topology_put_port(port);
> +}
> +
> /*
> * The EDID disappeared (likely because of the HPD going down).
> */
> @@ -393,6 +445,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
> @@ -433,6 +487,8 @@ void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> aux->cec.connector = connector;
> 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);
>
> @@ -442,6 +498,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..221c30133739 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);
> +
> 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 85513eeb2196..d8ee24a6319c 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1496,12 +1496,16 @@ struct drm_connector;
> * @adap: the CEC adapter for CEC-Tunneling-over-AUX support.
> * @connector: the connector this CEC adapter is associated with
> * @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;
> struct cec_adapter *adap;
> struct drm_connector *connector;
> struct delayed_work unregister_work;
> + struct work_struct mst_irq_work;
> + struct work_struct mst_set_edid_work;
> };
>
> /**
>

2021-02-05 05:22:29

by Sam McNally

Subject: Re: [PATCH v3 2/4] drm_dp_mst_topology: use correct AUX channel

On Thu, 4 Feb 2021 at 21:19, Hans Verkuil <[email protected]> wrote:
>
> On 01/02/2021 23:13, Ville Syrjälä wrote:
> > On Wed, Sep 23, 2020 at 12:13:53PM +1000, Sam McNally wrote:
> >> From: Hans Verkuil <[email protected]>
> >>
> >> For adapters behind an MST hub use the correct AUX channel.
> >>
> >> Signed-off-by: Hans Verkuil <[email protected]>
> >> [[email protected]: rebased, removing redundant changes]
> >> Signed-off-by: Sam McNally <[email protected]>
> >> ---
> >>
> >> (no changes since v1)
> >>
> >> 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;
> >> +
> >
> > This was supposed to be handled via higher levels checking for
> > is_remote==true.
>
> Ah, I suspect this patch can be dropped entirely: it predates commit 2f221a5efed4
> ("drm/dp_mst: Add MST support to DP DPCD R/W functions").
>
> It looks like that commit basically solved what this older patch attempts to do
> as well.
>
> Sam, can you test if it works without this patch?

It almost just works; drm_dp_cec uses whether aux.transfer is non-null
to filter out non-DP connectors. Using aux.is_remote as another signal
indicating a DP connector seems plausible. We can drop this patch.
Thanks all!
>
> Regards,
>
> Hans
>
> >
> >> /* 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);
> >
> > That doesn't make sense to me. I2c writes and DPCD writes
> > are definitely not the same thing.
> >
> > aux->transfer is a very low level thing. I don't think it's the
> > correct level of abstraction for sideband.
> >
> >> + 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.681.g6f77f65b4e-goog
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> [email protected]
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
>

2021-02-05 07:41:31

by Sam McNally

Subject: Re: [PATCH v3 4/4] drm_dp_cec: add MST support

On Thu, 4 Feb 2021 at 21:42, Hans Verkuil <[email protected]> wrote:
>
> On 23/09/2020 04:13, 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.
> >
> > Reviewed-by: Hans Verkuil <[email protected]>
> > Signed-off-by: Sam McNally <[email protected]>
> > ---
> >
> > Changes in v3:
> > - Fixed whitespace in drm_dp_cec_mst_irq_work()
> > - Moved drm_dp_cec_mst_set_edid_work() with the other set_edid functions
> >
> > Changes in v2:
> > - Used aux->is_remote instead of aux->cec.is_mst, removing the need for
> > the previous patch in the series
> > - Added a defensive check for null edid in the deferred set_edid work,
> > in case the edid is no longer valid at that point
> >
> > drivers/gpu/drm/drm_dp_cec.c | 68 +++++++++++++++++++++++++--
> > drivers/gpu/drm/drm_dp_mst_topology.c | 24 ++++++++++
> > include/drm/drm_dp_helper.h | 4 ++
> > 3 files changed, 91 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> > index 3ab2609f9ec7..1020b2cffdf0 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
> > @@ -248,6 +249,10 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux)
> > if (!aux->transfer)
> > return;
> >
> > + if (aux->is_remote) {
> > + schedule_work(&aux->cec.mst_irq_work);
> > + return;
> > + }
>
> Are these workqueues for cec_irq and cec_set_edid actually needed? They are called
> directly from drm_dp_mst_topology.c, and I think they can be handled identically to
> a normal, non-remote, aux device.
>
> Avoiding using a workqueue probably also means that there is no need for exporting
> drm_dp_mst_topology_get_port_validated and drm_dp_mst_topology_put_port.
>
> Provided that I am not missing something, this should simplify the code quite a bit.

Indeed; with commit 9408cc94eb04 ("drm/dp_mst: Handle UP requests
asynchronously") they're unnecessary. This all simplifies quite
nicely.
>
> Regards,
>
> Hans
>
> > mutex_lock(&aux->cec.lock);
> > if (!aux->cec.adap)
> > goto unlock;
> > @@ -276,6 +281,23 @@ 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.
> > @@ -297,7 +319,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 |
> > @@ -306,10 +329,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
> > @@ -320,6 +339,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,8 +395,40 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> > unlock:
> > 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->is_remote)
> > + schedule_work(&aux->cec.mst_set_edid_work);
> > + else
> > + drm_dp_cec_handle_set_edid(aux, edid);
> > +}
> > EXPORT_SYMBOL(drm_dp_cec_set_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);
> > +
> > + if (edid)
> > + drm_dp_cec_handle_set_edid(aux, edid);
> > +
> > + drm_dp_mst_topology_put_port(port);
> > +}
> > +
> > /*
> > * The EDID disappeared (likely because of the HPD going down).
> > */
> > @@ -393,6 +445,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
> > @@ -433,6 +487,8 @@ void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> > aux->cec.connector = connector;
> > 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);
> >
> > @@ -442,6 +498,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..221c30133739 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);
> > +
> > 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 85513eeb2196..d8ee24a6319c 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -1496,12 +1496,16 @@ struct drm_connector;
> > * @adap: the CEC adapter for CEC-Tunneling-over-AUX support.
> > * @connector: the connector this CEC adapter is associated with
> > * @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;
> > struct cec_adapter *adap;
> > struct drm_connector *connector;
> > struct delayed_work unregister_work;
> > + struct work_struct mst_irq_work;
> > + struct work_struct mst_set_edid_work;
> > };
> >
> > /**
> >
>

2021-02-05 07:47:34

by Sam McNally

Subject: Re: [PATCH v3 3/4] drm_dp_mst_topology: export two functions

On Tue, 2 Feb 2021 at 09:03, Lyude Paul <[email protected]> wrote:
>
> On Wed, 2020-09-23 at 12:13 +1000, Sam McNally wrote:
> > From: Hans Verkuil <[email protected]>
> >
> > These are required for the CEC MST support.
> >
> > Signed-off-by: Hans Verkuil <[email protected]>
> > Signed-off-by: Sam McNally <[email protected]>
> > ---
> >
> > (no changes since v1)
> >
> > 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)
>
> Mhhhhhh-can you think of some way around this? I really don't think it's a good
> idea for us to be exposing topology references to things as-is, the thing is
> they're really meant to be used for critical sections in code where it'd become
> very painful to deal with an mst port disappearing from under us. Outside of MST
> helpers, everyone else should be dealing with the expectation that these things
> can disappear as a result of hotplugs at any moment.
>
> Note that we do expose malloc refs, but that's intentional as holding a malloc
> ref to something doesn't cause it to stay around even when it's unplugged - it
> just stops it from being unallocated.
>
>

Yes, it turns out we won't need this after all.

> > {
> > 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);
> >
> >
>
> --
> Sincerely,
> Lyude Paul (she/her)
> Software Engineer at Red Hat
>
> Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
> asked me a question, are waiting for a review/merge on a patch, etc. and I
> haven't responded in a while, please feel free to send me another email to check
> on my status. I don't bite!
>

2021-02-05 23:58:09

by Ville Syrjälä

Subject: Re: [PATCH v3 2/4] drm_dp_mst_topology: use correct AUX channel

On Fri, Feb 05, 2021 at 02:46:44PM +0100, Hans Verkuil wrote:
> On 05/02/2021 14:24, Ville Syrj?l? wrote:
> > On Fri, Feb 05, 2021 at 04:17:51PM +1100, Sam McNally wrote:
> >> On Thu, 4 Feb 2021 at 21:19, Hans Verkuil <[email protected]> wrote:
> >>>
> >>> On 01/02/2021 23:13, Ville Syrj?l? wrote:
> >>>> On Wed, Sep 23, 2020 at 12:13:53PM +1000, Sam McNally wrote:
> >>>>> From: Hans Verkuil <[email protected]>
> >>>>>
> >>>>> For adapters behind an MST hub use the correct AUX channel.
> >>>>>
> >>>>> Signed-off-by: Hans Verkuil <[email protected]>
> >>>>> [[email protected]: rebased, removing redundant changes]
> >>>>> Signed-off-by: Sam McNally <[email protected]>
> >>>>> ---
> >>>>>
> >>>>> (no changes since v1)
> >>>>>
> >>>>> 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;
> >>>>> +
> >>>>
> >>>> This was supposed to be handled via higher levels checking for
> >>>> is_remote==true.
> >>>
> >>> Ah, I suspect this patch can be dropped entirely: it predates commit 2f221a5efed4
> >>> ("drm/dp_mst: Add MST support to DP DPCD R/W functions").
> >>>
> >>> It looks like that commit basically solved what this older patch attempts to do
> >>> as well.
> >>>
> >>> Sam, can you test if it works without this patch?
> >>
> >> It almost just works; drm_dp_cec uses whether aux.transfer is non-null
> >> to filter out non-DP connectors. Using aux.is_remote as another signal
> >> indicating a DP connector seems plausible. We can drop this patch.
> >
> > Why would anyone even call this stuff on a non-DP connector?
> > And where did they even get the struct drm_dp_aux to do so?
>
> This check came in with commit 5ce70c799ac2 ("drm_dp_cec: check that aux
> has a transfer function"). It seems nouveau and amdgpu specific.

I see.

>
> A better approach would be to fix those drivers to only call these cec
> functions for DP outputs. I think I moved the test to drm_dp_cec.c primarily
> for robustness (i.e. do nothing if called for a non-DP output). But that
> might not be the right approach after all.

Shrug. I guess just extending to check is_remote (or maybe there is
some other member that's always set?) is a good enough short term
solution. Someone may want to have a look at adjusting
amdgpu/nouveau to not need it, but who knows how much work that is.

--
Ville Syrj?l?
Intel

2021-02-06 00:11:31

by Hans Verkuil

Subject: Re: [PATCH v3 2/4] drm_dp_mst_topology: use correct AUX channel

On 05/02/2021 14:24, Ville Syrjälä wrote:
> On Fri, Feb 05, 2021 at 04:17:51PM +1100, Sam McNally wrote:
>> On Thu, 4 Feb 2021 at 21:19, Hans Verkuil <[email protected]> wrote:
>>>
>>> On 01/02/2021 23:13, Ville Syrjälä wrote:
>>>> On Wed, Sep 23, 2020 at 12:13:53PM +1000, Sam McNally wrote:
>>>>> From: Hans Verkuil <[email protected]>
>>>>>
>>>>> For adapters behind an MST hub use the correct AUX channel.
>>>>>
>>>>> Signed-off-by: Hans Verkuil <[email protected]>
>>>>> [[email protected]: rebased, removing redundant changes]
>>>>> Signed-off-by: Sam McNally <[email protected]>
>>>>> ---
>>>>>
>>>>> (no changes since v1)
>>>>>
>>>>> 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;
>>>>> +
>>>>
>>>> This was supposed to be handled via higher levels checking for
>>>> is_remote==true.
>>>
>>> Ah, I suspect this patch can be dropped entirely: it predates commit 2f221a5efed4
>>> ("drm/dp_mst: Add MST support to DP DPCD R/W functions").
>>>
>>> It looks like that commit basically solved what this older patch attempts to do
>>> as well.
>>>
>>> Sam, can you test if it works without this patch?
>>
>> It almost just works; drm_dp_cec uses whether aux.transfer is non-null
>> to filter out non-DP connectors. Using aux.is_remote as another signal
>> indicating a DP connector seems plausible. We can drop this patch.
>
> Why would anyone even call this stuff on a non-DP connector?
> And where did they even get the struct drm_dp_aux to do so?

This check came in with commit 5ce70c799ac2 ("drm_dp_cec: check that aux
has a transfer function"). It seems nouveau and amdgpu specific.

A better approach would be to fix those drivers to only call these cec
functions for DP outputs. I think I moved the test to drm_dp_cec.c primarily
for robustness (i.e. do nothing if called for a non-DP output). But that
might not be the right approach after all.

Regards,

Hans

>
>> Thanks all!
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>>
>>>>> /* 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);
>>>>
>>>> That doesn't make sense to me. I2c writes and DPCD writes
>>>> are definitely not the same thing.
>>>>
>>>> aux->transfer is a very low level thing. I don't think it's the
>>>> correct level of abstraction for sideband.
>>>>
>>>>> + 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.681.g6f77f65b4e-goog
>>>>>
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> [email protected]
>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>
>>>
>

2021-02-06 00:12:24

by Ville Syrjälä

Subject: Re: [PATCH v3 2/4] drm_dp_mst_topology: use correct AUX channel

On Fri, Feb 05, 2021 at 04:17:51PM +1100, Sam McNally wrote:
> On Thu, 4 Feb 2021 at 21:19, Hans Verkuil <[email protected]> wrote:
> >
> > On 01/02/2021 23:13, Ville Syrj?l? wrote:
> > > On Wed, Sep 23, 2020 at 12:13:53PM +1000, Sam McNally wrote:
> > >> From: Hans Verkuil <[email protected]>
> > >>
> > >> For adapters behind an MST hub use the correct AUX channel.
> > >>
> > >> Signed-off-by: Hans Verkuil <[email protected]>
> > >> [[email protected]: rebased, removing redundant changes]
> > >> Signed-off-by: Sam McNally <[email protected]>
> > >> ---
> > >>
> > >> (no changes since v1)
> > >>
> > >> 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;
> > >> +
> > >
> > > This was supposed to be handled via higher levels checking for
> > > is_remote==true.
> >
> > Ah, I suspect this patch can be dropped entirely: it predates commit 2f221a5efed4
> > ("drm/dp_mst: Add MST support to DP DPCD R/W functions").
> >
> > It looks like that commit basically solved what this older patch attempts to do
> > as well.
> >
> > Sam, can you test if it works without this patch?
>
> It almost just works; drm_dp_cec uses whether aux.transfer is non-null
> to filter out non-DP connectors. Using aux.is_remote as another signal
> indicating a DP connector seems plausible. We can drop this patch.

Why would anyone even call this stuff on a non-DP connector?
And where did they even get the struct drm_dp_aux to do so?

> Thanks all!
> >
> > Regards,
> >
> > Hans
> >
> > >
> > >> /* 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);
> > >
> > > That doesn't make sense to me. I2c writes and DPCD writes
> > > are definitely not the same thing.
> > >
> > > aux->transfer is a very low level thing. I don't think it's the
> > > correct level of abstraction for sideband.
> > >
> > >> + 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.681.g6f77f65b4e-goog
> > >>
> > >> _______________________________________________
> > >> dri-devel mailing list
> > >> [email protected]
> > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> >

--
Ville Syrj?l?
Intel