Sink event notify messages are used for MST CEC IRQs. Add parsing support for sink event notify messages in preparation for handling MST CEC IRQs.
Signed-off-by: Sam McNally sammc@chromium.org ---
(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;
From: Hans Verkuil hans.verkuil@cisco.com
For adapters behind an MST hub use the correct AUX channel.
Signed-off-by: Hans Verkuil hans.verkuil@cisco.com [sammc@chromium.org: rebased, removing redundant changes] Signed-off-by: Sam McNally sammc@chromium.org ---
(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)
On Wed, 2020-09-23 at 12:13 +1000, Sam McNally wrote:
From: Hans Verkuil hans.verkuil@cisco.com
For adapters behind an MST hub use the correct AUX channel.
Signed-off-by: Hans Verkuil hans.verkuil@cisco.com [sammc@chromium.org: rebased, removing redundant changes] Signed-off-by: Sam McNally sammc@chromium.org
(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 lyude@redhat.com
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)
On Wed, Sep 23, 2020 at 12:13:53PM +1000, Sam McNally wrote:
From: Hans Verkuil hans.verkuil@cisco.com
For adapters behind an MST hub use the correct AUX channel.
Signed-off-by: Hans Verkuil hans.verkuil@cisco.com [sammc@chromium.org: rebased, removing redundant changes] Signed-off-by: Sam McNally sammc@chromium.org
(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 dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
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 hans.verkuil@cisco.com
For adapters behind an MST hub use the correct AUX channel.
Signed-off-by: Hans Verkuil hans.verkuil@cisco.com [sammc@chromium.org: rebased, removing redundant changes] Signed-off-by: Sam McNally sammc@chromium.org
(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 dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, 4 Feb 2021 at 21:19, Hans Verkuil hverkuil@xs4all.nl 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 hans.verkuil@cisco.com
For adapters behind an MST hub use the correct AUX channel.
Signed-off-by: Hans Verkuil hans.verkuil@cisco.com [sammc@chromium.org: rebased, removing redundant changes] Signed-off-by: Sam McNally sammc@chromium.org
(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 dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Feb 05, 2021 at 04:17:51PM +1100, Sam McNally wrote:
On Thu, 4 Feb 2021 at 21:19, Hans Verkuil hverkuil@xs4all.nl 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 hans.verkuil@cisco.com
For adapters behind an MST hub use the correct AUX channel.
Signed-off-by: Hans Verkuil hans.verkuil@cisco.com [sammc@chromium.org: rebased, removing redundant changes] Signed-off-by: Sam McNally sammc@chromium.org
(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 dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
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 hverkuil@xs4all.nl 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 hans.verkuil@cisco.com
For adapters behind an MST hub use the correct AUX channel.
Signed-off-by: Hans Verkuil hans.verkuil@cisco.com [sammc@chromium.org: rebased, removing redundant changes] Signed-off-by: Sam McNally sammc@chromium.org
(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 dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
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 hverkuil@xs4all.nl 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 hans.verkuil@cisco.com
For adapters behind an MST hub use the correct AUX channel.
Signed-off-by: Hans Verkuil hans.verkuil@cisco.com [sammc@chromium.org: rebased, removing redundant changes] Signed-off-by: Sam McNally sammc@chromium.org
(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.
From: Hans Verkuil hans.verkuil@cisco.com
These are required for the CEC MST support.
Signed-off-by: Hans Verkuil hans.verkuil@cisco.com Signed-off-by: Sam McNally sammc@chromium.org ---
(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);
On Wed, 2020-09-23 at 12:13 +1000, Sam McNally wrote:
From: Hans Verkuil hans.verkuil@cisco.com
These are required for the CEC MST support.
Signed-off-by: Hans Verkuil hans.verkuil@cisco.com Signed-off-by: Sam McNally sammc@chromium.org
(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);
On Tue, 2 Feb 2021 at 09:03, Lyude Paul lyude@redhat.com wrote:
On Wed, 2020-09-23 at 12:13 +1000, Sam McNally wrote:
From: Hans Verkuil hans.verkuil@cisco.com
These are required for the CEC MST support.
Signed-off-by: Hans Verkuil hans.verkuil@cisco.com Signed-off-by: Sam McNally sammc@chromium.org
(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!
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 hverkuil-cisco@xs4all.nl Signed-off-by: Sam McNally sammc@chromium.org ---
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; };
/**
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 hverkuil-cisco@xs4all.nl Signed-off-by: Sam McNally sammc@chromium.org
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_mst_topology_put_port(port); return ret;drm_dp_cec_unset_edid(&port->aux);
} @@ -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;
};
/**
On Thu, 4 Feb 2021 at 21:42, Hans Verkuil hverkuil@xs4all.nl 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 hverkuil-cisco@xs4all.nl Signed-off-by: Sam McNally sammc@chromium.org
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;
};
/**
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 sammc@chromium.org
(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:
default: DRM_ERROR("Got unknown request 0x%02x (%s)\n", msg->req_type, drm_dp_mst_req_type_str(msg->req_type));return drm_dp_sideband_parse_sink_event_notify(raw, msg);
@@ -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 &&
DRM_DEBUG_KMS("Received unknown up req type, ignoring: %x\n", up_req->msg.req_type); kfree(up_req);up_req->msg.req_type != DP_SINK_EVENT_NOTIFY) {
@@ -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;
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 sammc@chromium.org
(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:
default: DRM_ERROR("Got unknown request 0x%02x (%s)\n", msg->req_type, drm_dp_mst_req_type_str(msg->req_type));return drm_dp_sideband_parse_sink_event_notify(raw, msg);
@@ -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 &&
DRM_DEBUG_KMS("Received unknown up req type, ignoring: %x\n", up_req->msg.req_type); kfree(up_req);up_req->msg.req_type != DP_SINK_EVENT_NOTIFY) {
@@ -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;
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 sammc@chromium.org
(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;
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 sammc@chromium.org
(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;
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 hverkuil@xs4all.nl 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 sammc@chromium.org
(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;
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 <hverkuil@xs4all.nl mailto:hverkuil@xs4all.nl> 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 <sammc@chromium.org <mailto:sammc@chromium.org>> >> --- >> >> (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; >
dri-devel@lists.freedesktop.org