From 5950f0b797fc77705d46dfa5b7d0db325c8f3c44 Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Tue, 3 Sep 2019 16:45:39 -0400 Subject: drm/dp_mst: Move link address dumping into a function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Makes things easier to read. Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Reviewed-by: Daniel Vetter Signed-off-by: Lyude Paul Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-2-lyude@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 82add736e17d..36db66a0ddb1 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -2103,6 +2103,28 @@ static void drm_dp_queue_down_tx(struct drm_dp_mst_topology_mgr *mgr, mutex_unlock(&mgr->qlock); } +static void +drm_dp_dump_link_address(struct drm_dp_link_address_ack_reply *reply) +{ + struct drm_dp_link_addr_reply_port *port_reply; + int i; + + for (i = 0; i < reply->nports; i++) { + port_reply = &reply->ports[i]; + DRM_DEBUG_KMS("port %d: input %d, pdt: %d, pn: %d, dpcd_rev: %02x, mcs: %d, ddps: %d, ldps %d, sdp %d/%d\n", + i, + port_reply->input_port, + port_reply->peer_device_type, + port_reply->port_number, + port_reply->dpcd_revision, + port_reply->mcs, + port_reply->ddps, + port_reply->legacy_device_plug_status, + port_reply->num_sdp_streams, + port_reply->num_sdp_stream_sinks); + } +} + static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_branch *mstb) { @@ -2128,18 +2150,7 @@ static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, DRM_DEBUG_KMS("link address nak received\n"); } else { DRM_DEBUG_KMS("link address reply: %d\n", txmsg->reply.u.link_addr.nports); - for (i = 0; i < txmsg->reply.u.link_addr.nports; i++) { - DRM_DEBUG_KMS("port %d: input %d, pdt: %d, pn: %d, dpcd_rev: %02x, mcs: %d, ddps: %d, ldps %d, sdp %d/%d\n", i, - txmsg->reply.u.link_addr.ports[i].input_port, - txmsg->reply.u.link_addr.ports[i].peer_device_type, - txmsg->reply.u.link_addr.ports[i].port_number, - txmsg->reply.u.link_addr.ports[i].dpcd_revision, - txmsg->reply.u.link_addr.ports[i].mcs, - txmsg->reply.u.link_addr.ports[i].ddps, - txmsg->reply.u.link_addr.ports[i].legacy_device_plug_status, - txmsg->reply.u.link_addr.ports[i].num_sdp_streams, - txmsg->reply.u.link_addr.ports[i].num_sdp_stream_sinks); - } + drm_dp_dump_link_address(&txmsg->reply.u.link_addr); drm_dp_check_mstb_guid(mstb, txmsg->reply.u.link_addr.guid); -- cgit v1.2.3 From 3ba64aa36c8c6516ea0c01e3acd13c5fcb5a482c Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Tue, 3 Sep 2019 16:45:40 -0400 Subject: drm/dp_mst: Get rid of list clear in destroy_connector_work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This seems to be some leftover detritus from before the port/mstb kref cleanup and doesn't do anything anymore, so get rid of it. Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Reviewed-by: Daniel Vetter Signed-off-by: Lyude Paul Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-3-lyude@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 2 -- 1 file changed, 2 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 36db66a0ddb1..3054ec622506 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3760,8 +3760,6 @@ static void drm_dp_destroy_connector_work(struct work_struct *work) list_del(&port->next); mutex_unlock(&mgr->destroy_connector_lock); - INIT_LIST_HEAD(&port->next); - mgr->cbs->destroy_connector(mgr, port->connector); drm_dp_port_teardown_pdt(port, port->pdt); -- cgit v1.2.3 From 7cbce45d6243225914b5c967b4ee927a2327842a Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Tue, 3 Sep 2019 16:45:42 -0400 Subject: drm/dp_mst: Move test_calc_pbn_mode() into an actual selftest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Yes, apparently we've been testing this for every single driver load for quite a long time now. At least that means our PBN calculation is solid! Anyway, introduce self tests for MST and move this into there. Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Reviewed-by: Daniel Vetter Signed-off-by: Lyude Paul Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-5-lyude@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 27 ----------------- drivers/gpu/drm/selftests/Makefile | 2 +- drivers/gpu/drm/selftests/drm_modeset_selftests.h | 1 + drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c | 34 ++++++++++++++++++++++ .../gpu/drm/selftests/test-drm_modeset_common.h | 1 + 5 files changed, 37 insertions(+), 28 deletions(-) create mode 100644 drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 3054ec622506..625743c76ddf 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -47,7 +47,6 @@ */ static bool dump_dp_payload_table(struct drm_dp_mst_topology_mgr *mgr, char *buf); -static int test_calc_pbn_mode(void); static void drm_dp_mst_topology_put_port(struct drm_dp_mst_port *port); @@ -3578,30 +3577,6 @@ int drm_dp_calc_pbn_mode(int clock, int bpp) } EXPORT_SYMBOL(drm_dp_calc_pbn_mode); -static int test_calc_pbn_mode(void) -{ - int ret; - ret = drm_dp_calc_pbn_mode(154000, 30); - if (ret != 689) { - DRM_ERROR("PBN calculation test failed - clock %d, bpp %d, expected PBN %d, actual PBN %d.\n", - 154000, 30, 689, ret); - return -EINVAL; - } - ret = drm_dp_calc_pbn_mode(234000, 30); - if (ret != 1047) { - DRM_ERROR("PBN calculation test failed - clock %d, bpp %d, expected PBN %d, actual PBN %d.\n", - 234000, 30, 1047, ret); - return -EINVAL; - } - ret = drm_dp_calc_pbn_mode(297000, 24); - if (ret != 1063) { - DRM_ERROR("PBN calculation test failed - clock %d, bpp %d, expected PBN %d, actual PBN %d.\n", - 297000, 24, 1063, ret); - return -EINVAL; - } - return 0; -} - /* we want to kick the TX after we've ack the up/down IRQs. */ static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr) { @@ -3979,8 +3954,6 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr, if (!mgr->proposed_vcpis) return -ENOMEM; set_bit(0, &mgr->payload_mask); - if (test_calc_pbn_mode() < 0) - DRM_ERROR("MST PBN self-test failed\n"); mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL); if (mst_state == NULL) diff --git a/drivers/gpu/drm/selftests/Makefile b/drivers/gpu/drm/selftests/Makefile index aae88f8a016c..d2137342b371 100644 --- a/drivers/gpu/drm/selftests/Makefile +++ b/drivers/gpu/drm/selftests/Makefile @@ -1,6 +1,6 @@ # SPDX-License-Identifier: GPL-2.0-only test-drm_modeset-y := test-drm_modeset_common.o test-drm_plane_helper.o \ test-drm_format.o test-drm_framebuffer.o \ - test-drm_damage_helper.o + test-drm_damage_helper.o test-drm_dp_mst_helper.o obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_modeset.o test-drm_cmdline_parser.o diff --git a/drivers/gpu/drm/selftests/drm_modeset_selftests.h b/drivers/gpu/drm/selftests/drm_modeset_selftests.h index 464753746013..dec3ee3ec96f 100644 --- a/drivers/gpu/drm/selftests/drm_modeset_selftests.h +++ b/drivers/gpu/drm/selftests/drm_modeset_selftests.h @@ -32,3 +32,4 @@ selftest(damage_iter_damage_one_intersect, igt_damage_iter_damage_one_intersect) selftest(damage_iter_damage_one_outside, igt_damage_iter_damage_one_outside) selftest(damage_iter_damage_src_moved, igt_damage_iter_damage_src_moved) selftest(damage_iter_damage_not_visible, igt_damage_iter_damage_not_visible) +selftest(dp_mst_calc_pbn_mode, igt_dp_mst_calc_pbn_mode) diff --git a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c new file mode 100644 index 000000000000..9baa5171988d --- /dev/null +++ b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c @@ -0,0 +1,34 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Test cases for for the DRM DP MST helpers + */ + +#include +#include + +#include "test-drm_modeset_common.h" + +int igt_dp_mst_calc_pbn_mode(void *ignored) +{ + int pbn, i; + const struct { + int rate; + int bpp; + int expected; + } test_params[] = { + { 154000, 30, 689 }, + { 234000, 30, 1047 }, + { 297000, 24, 1063 }, + }; + + for (i = 0; i < ARRAY_SIZE(test_params); i++) { + pbn = drm_dp_calc_pbn_mode(test_params[i].rate, + test_params[i].bpp); + FAIL(pbn != test_params[i].expected, + "Expected PBN %d for clock %d bpp %d, got %d\n", + test_params[i].expected, test_params[i].rate, + test_params[i].bpp, pbn); + } + + return 0; +} diff --git a/drivers/gpu/drm/selftests/test-drm_modeset_common.h b/drivers/gpu/drm/selftests/test-drm_modeset_common.h index 8c76f09c12d1..590bda35a683 100644 --- a/drivers/gpu/drm/selftests/test-drm_modeset_common.h +++ b/drivers/gpu/drm/selftests/test-drm_modeset_common.h @@ -39,5 +39,6 @@ int igt_damage_iter_damage_one_intersect(void *ignored); int igt_damage_iter_damage_one_outside(void *ignored); int igt_damage_iter_damage_src_moved(void *ignored); int igt_damage_iter_damage_not_visible(void *ignored); +int igt_dp_mst_calc_pbn_mode(void *ignored); #endif -- cgit v1.2.3 From aa2a2fe1383fff13d2822ff9fce4013eacf2a0cb Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Tue, 3 Sep 2019 17:57:02 -0400 Subject: drm/dp_mst: Combine redundant cases in drm_dp_encode_sideband_req() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Noticed this while working on adding a drm_dp_decode_sideband_req(). DP_POWER_DOWN_PHY/DP_POWER_UP_PHY both use the same struct as DP_ENUM_PATH_RESOURCES, so we can just combine their cases. Changes since v2: * Fix commit message Signed-off-by: Lyude Paul Reviewed-by: Dave Airlie Cc: Daniel Vetter Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Link: https://patchwork.freedesktop.org/patch/msgid/20190903215702.16984-1-lyude@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 625743c76ddf..bdcdd37c4494 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -271,6 +271,8 @@ static void drm_dp_encode_sideband_req(struct drm_dp_sideband_msg_req_body *req, switch (req->req_type) { case DP_ENUM_PATH_RESOURCES: + case DP_POWER_DOWN_PHY: + case DP_POWER_UP_PHY: buf[idx] = (req->u.port_num.port_number & 0xf) << 4; idx++; break; @@ -358,12 +360,6 @@ static void drm_dp_encode_sideband_req(struct drm_dp_sideband_msg_req_body *req, memcpy(&buf[idx], req->u.i2c_write.bytes, req->u.i2c_write.num_bytes); idx += req->u.i2c_write.num_bytes; break; - - case DP_POWER_DOWN_PHY: - case DP_POWER_UP_PHY: - buf[idx] = (req->u.port_num.port_number & 0xf) << 4; - idx++; - break; } raw->cur_len = idx; } -- cgit v1.2.3 From 2f015ec6eab69301fdcf54d397810d72362d7223 Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Tue, 3 Sep 2019 16:45:45 -0400 Subject: drm/dp_mst: Add sideband down request tracing + selftests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unfortunately the DP MST helpers do not have much in the way of debugging utilities. So, let's add some! This adds basic debugging output for down sideband requests that we send from the driver, so that we can actually discern what's happening when sideband requests timeout. Since there wasn't really a good way of testing that any of this worked, I ended up writing simple selftests that lightly test sideband message encoding and decoding as well. Enjoy! Changes since v1: * Clean up DO_TEST() and sideband_msg_req_encode_decode() - danvet * Get rid of pr_fmt(), just define a prefix string instead and use drm_printf() * Check highest bit of VCPI in drm_dp_decode_sideband_req() - danvet * Make the switch case order between drm_dp_decode_sideband_req() and drm_dp_encode_sideband_req() the same - danvet * Only check DRM_UT_DP - danvet * Clean up sideband_msg_req_equal() from selftests a bit, and add comments explaining why we can't just use memcmp - danvet Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Reviewed-by: Daniel Vetter Signed-off-by: Lyude Paul Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-8-lyude@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 309 ++++++++++++++++++++- drivers/gpu/drm/drm_dp_mst_topology_internal.h | 24 ++ drivers/gpu/drm/selftests/drm_modeset_selftests.h | 1 + drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c | 204 ++++++++++++++ .../gpu/drm/selftests/test-drm_modeset_common.h | 1 + include/drm/drm_dp_mst_helper.h | 2 +- 6 files changed, 536 insertions(+), 5 deletions(-) create mode 100644 drivers/gpu/drm/drm_dp_mst_topology_internal.h (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index bdcdd37c4494..ae1a3c4b6b17 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -37,6 +37,7 @@ #include #include "drm_crtc_helper_internal.h" +#include "drm_dp_mst_topology_internal.h" /** * DOC: dp mst helper @@ -73,6 +74,8 @@ static int drm_dp_mst_register_i2c_bus(struct drm_dp_aux *aux); static void drm_dp_mst_unregister_i2c_bus(struct drm_dp_aux *aux); static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr); +#define DBG_PREFIX "[dp_mst]" + #define DP_STR(x) [DP_ ## x] = #x static const char *drm_dp_mst_req_type_str(u8 req_type) @@ -129,6 +132,43 @@ static const char *drm_dp_mst_nak_reason_str(u8 nak_reason) } #undef DP_STR +#define DP_STR(x) [DRM_DP_SIDEBAND_TX_ ## x] = #x + +static const char *drm_dp_mst_sideband_tx_state_str(int state) +{ + static const char * const sideband_reason_str[] = { + DP_STR(QUEUED), + DP_STR(START_SEND), + DP_STR(SENT), + DP_STR(RX), + DP_STR(TIMEOUT), + }; + + if (state >= ARRAY_SIZE(sideband_reason_str) || + !sideband_reason_str[state]) + return "unknown"; + + return sideband_reason_str[state]; +} + +static int +drm_dp_mst_rad_to_str(const u8 rad[8], u8 lct, char *out, size_t len) +{ + int i; + u8 unpacked_rad[16]; + + for (i = 0; i < lct; i++) { + if (i % 2) + unpacked_rad[i] = rad[i / 2] >> 4; + else + unpacked_rad[i] = rad[i / 2] & BIT_MASK(4); + } + + /* TODO: Eventually add something to printk so we can format the rad + * like this: 1.2.3 + */ + return snprintf(out, len, "%*phC", lct, unpacked_rad); +} /* sideband msg handling */ static u8 drm_dp_msg_header_crc4(const uint8_t *data, size_t num_nibbles) @@ -261,8 +301,9 @@ static bool drm_dp_decode_sideband_msg_hdr(struct drm_dp_sideband_msg_hdr *hdr, return true; } -static void drm_dp_encode_sideband_req(struct drm_dp_sideband_msg_req_body *req, - struct drm_dp_sideband_msg_tx *raw) +void +drm_dp_encode_sideband_req(const struct drm_dp_sideband_msg_req_body *req, + struct drm_dp_sideband_msg_tx *raw) { int idx = 0; int i; @@ -363,6 +404,251 @@ static void drm_dp_encode_sideband_req(struct drm_dp_sideband_msg_req_body *req, } raw->cur_len = idx; } +EXPORT_SYMBOL_FOR_TESTS_ONLY(drm_dp_encode_sideband_req); + +/* Decode a sideband request we've encoded, mainly used for debugging */ +int +drm_dp_decode_sideband_req(const struct drm_dp_sideband_msg_tx *raw, + struct drm_dp_sideband_msg_req_body *req) +{ + const u8 *buf = raw->msg; + int i, idx = 0; + + req->req_type = buf[idx++] & 0x7f; + switch (req->req_type) { + case DP_ENUM_PATH_RESOURCES: + case DP_POWER_DOWN_PHY: + case DP_POWER_UP_PHY: + req->u.port_num.port_number = (buf[idx] >> 4) & 0xf; + break; + case DP_ALLOCATE_PAYLOAD: + { + struct drm_dp_allocate_payload *a = + &req->u.allocate_payload; + + a->number_sdp_streams = buf[idx] & 0xf; + a->port_number = (buf[idx] >> 4) & 0xf; + + WARN_ON(buf[++idx] & 0x80); + a->vcpi = buf[idx] & 0x7f; + + a->pbn = buf[++idx] << 8; + a->pbn |= buf[++idx]; + + idx++; + for (i = 0; i < a->number_sdp_streams; i++) { + a->sdp_stream_sink[i] = + (buf[idx + (i / 2)] >> ((i % 2) ? 0 : 4)) & 0xf; + } + } + break; + case DP_QUERY_PAYLOAD: + req->u.query_payload.port_number = (buf[idx] >> 4) & 0xf; + WARN_ON(buf[++idx] & 0x80); + req->u.query_payload.vcpi = buf[idx] & 0x7f; + break; + case DP_REMOTE_DPCD_READ: + { + struct drm_dp_remote_dpcd_read *r = &req->u.dpcd_read; + + r->port_number = (buf[idx] >> 4) & 0xf; + + r->dpcd_address = (buf[idx] << 16) & 0xf0000; + r->dpcd_address |= (buf[++idx] << 8) & 0xff00; + r->dpcd_address |= buf[++idx] & 0xff; + + r->num_bytes = buf[++idx]; + } + break; + case DP_REMOTE_DPCD_WRITE: + { + struct drm_dp_remote_dpcd_write *w = + &req->u.dpcd_write; + + w->port_number = (buf[idx] >> 4) & 0xf; + + w->dpcd_address = (buf[idx] << 16) & 0xf0000; + w->dpcd_address |= (buf[++idx] << 8) & 0xff00; + w->dpcd_address |= buf[++idx] & 0xff; + + w->num_bytes = buf[++idx]; + + w->bytes = kmemdup(&buf[++idx], w->num_bytes, + GFP_KERNEL); + if (!w->bytes) + return -ENOMEM; + } + break; + case DP_REMOTE_I2C_READ: + { + struct drm_dp_remote_i2c_read *r = &req->u.i2c_read; + struct drm_dp_remote_i2c_read_tx *tx; + bool failed = false; + + r->num_transactions = buf[idx] & 0x3; + r->port_number = (buf[idx] >> 4) & 0xf; + for (i = 0; i < r->num_transactions; i++) { + tx = &r->transactions[i]; + + tx->i2c_dev_id = buf[++idx] & 0x7f; + tx->num_bytes = buf[++idx]; + tx->bytes = kmemdup(&buf[++idx], + tx->num_bytes, + GFP_KERNEL); + if (!tx->bytes) { + failed = true; + break; + } + idx += tx->num_bytes; + tx->no_stop_bit = (buf[idx] >> 5) & 0x1; + tx->i2c_transaction_delay = buf[idx] & 0xf; + } + + if (failed) { + for (i = 0; i < r->num_transactions; i++) + kfree(tx->bytes); + return -ENOMEM; + } + + r->read_i2c_device_id = buf[++idx] & 0x7f; + r->num_bytes_read = buf[++idx]; + } + break; + case DP_REMOTE_I2C_WRITE: + { + struct drm_dp_remote_i2c_write *w = &req->u.i2c_write; + + w->port_number = (buf[idx] >> 4) & 0xf; + w->write_i2c_device_id = buf[++idx] & 0x7f; + w->num_bytes = buf[++idx]; + w->bytes = kmemdup(&buf[++idx], w->num_bytes, + GFP_KERNEL); + if (!w->bytes) + return -ENOMEM; + } + break; + } + + return 0; +} +EXPORT_SYMBOL_FOR_TESTS_ONLY(drm_dp_decode_sideband_req); + +void +drm_dp_dump_sideband_msg_req_body(const struct drm_dp_sideband_msg_req_body *req, + int indent, struct drm_printer *printer) +{ + int i; + +#define P(f, ...) drm_printf_indent(printer, indent, f, ##__VA_ARGS__) + if (req->req_type == DP_LINK_ADDRESS) { + /* No contents to print */ + P("type=%s\n", drm_dp_mst_req_type_str(req->req_type)); + return; + } + + P("type=%s contents:\n", drm_dp_mst_req_type_str(req->req_type)); + indent++; + + switch (req->req_type) { + case DP_ENUM_PATH_RESOURCES: + case DP_POWER_DOWN_PHY: + case DP_POWER_UP_PHY: + P("port=%d\n", req->u.port_num.port_number); + break; + case DP_ALLOCATE_PAYLOAD: + P("port=%d vcpi=%d pbn=%d sdp_streams=%d %*ph\n", + req->u.allocate_payload.port_number, + req->u.allocate_payload.vcpi, req->u.allocate_payload.pbn, + req->u.allocate_payload.number_sdp_streams, + req->u.allocate_payload.number_sdp_streams, + req->u.allocate_payload.sdp_stream_sink); + break; + case DP_QUERY_PAYLOAD: + P("port=%d vcpi=%d\n", + req->u.query_payload.port_number, + req->u.query_payload.vcpi); + break; + case DP_REMOTE_DPCD_READ: + P("port=%d dpcd_addr=%05x len=%d\n", + req->u.dpcd_read.port_number, req->u.dpcd_read.dpcd_address, + req->u.dpcd_read.num_bytes); + break; + case DP_REMOTE_DPCD_WRITE: + P("port=%d addr=%05x len=%d: %*ph\n", + req->u.dpcd_write.port_number, + req->u.dpcd_write.dpcd_address, + req->u.dpcd_write.num_bytes, req->u.dpcd_write.num_bytes, + req->u.dpcd_write.bytes); + break; + case DP_REMOTE_I2C_READ: + P("port=%d num_tx=%d id=%d size=%d:\n", + req->u.i2c_read.port_number, + req->u.i2c_read.num_transactions, + req->u.i2c_read.read_i2c_device_id, + req->u.i2c_read.num_bytes_read); + + indent++; + for (i = 0; i < req->u.i2c_read.num_transactions; i++) { + const struct drm_dp_remote_i2c_read_tx *rtx = + &req->u.i2c_read.transactions[i]; + + P("%d: id=%03d size=%03d no_stop_bit=%d tx_delay=%03d: %*ph\n", + i, rtx->i2c_dev_id, rtx->num_bytes, + rtx->no_stop_bit, rtx->i2c_transaction_delay, + rtx->num_bytes, rtx->bytes); + } + break; + case DP_REMOTE_I2C_WRITE: + P("port=%d id=%d size=%d: %*ph\n", + req->u.i2c_write.port_number, + req->u.i2c_write.write_i2c_device_id, + req->u.i2c_write.num_bytes, req->u.i2c_write.num_bytes, + req->u.i2c_write.bytes); + break; + default: + P("???\n"); + break; + } +#undef P +} +EXPORT_SYMBOL_FOR_TESTS_ONLY(drm_dp_dump_sideband_msg_req_body); + +static inline void +drm_dp_mst_dump_sideband_msg_tx(struct drm_printer *p, + const struct drm_dp_sideband_msg_tx *txmsg) +{ + struct drm_dp_sideband_msg_req_body req; + char buf[64]; + int ret; + int i; + + drm_dp_mst_rad_to_str(txmsg->dst->rad, txmsg->dst->lct, buf, + sizeof(buf)); + drm_printf(p, "txmsg cur_offset=%x cur_len=%x seqno=%x state=%s path_msg=%d dst=%s\n", + txmsg->cur_offset, txmsg->cur_len, txmsg->seqno, + drm_dp_mst_sideband_tx_state_str(txmsg->state), + txmsg->path_msg, buf); + + ret = drm_dp_decode_sideband_req(txmsg, &req); + if (ret) { + drm_printf(p, "\n", ret); + return; + } + drm_dp_dump_sideband_msg_req_body(&req, 1, p); + + switch (req.req_type) { + case DP_REMOTE_DPCD_WRITE: + kfree(req.u.dpcd_write.bytes); + break; + case DP_REMOTE_I2C_READ: + for (i = 0; i < req.u.i2c_read.num_transactions; i++) + kfree(req.u.i2c_read.transactions[i].bytes); + break; + case DP_REMOTE_I2C_WRITE: + kfree(req.u.i2c_write.bytes); + break; + } +} static void drm_dp_crc_sideband_chunk_req(u8 *msg, u8 len) { @@ -894,6 +1180,11 @@ static int drm_dp_mst_wait_tx_reply(struct drm_dp_mst_branch *mstb, } } out: + if (unlikely(ret == -EIO && drm_debug & DRM_UT_DP)) { + struct drm_printer p = drm_debug_printer(DBG_PREFIX); + + drm_dp_mst_dump_sideband_msg_tx(&p, txmsg); + } mutex_unlock(&mgr->qlock); return ret; @@ -2030,8 +2321,11 @@ static int process_single_tx_qlock(struct drm_dp_mst_topology_mgr *mgr, idx += tosend + 1; ret = drm_dp_send_sideband_msg(mgr, up, chunk, idx); - if (ret) { - DRM_DEBUG_KMS("sideband msg failed to send\n"); + if (unlikely(ret && drm_debug & DRM_UT_DP)) { + struct drm_printer p = drm_debug_printer(DBG_PREFIX); + + drm_printf(&p, "sideband msg failed to send\n"); + drm_dp_mst_dump_sideband_msg_tx(&p, txmsg); return ret; } @@ -2093,6 +2387,13 @@ static void drm_dp_queue_down_tx(struct drm_dp_mst_topology_mgr *mgr, { mutex_lock(&mgr->qlock); list_add_tail(&txmsg->next, &mgr->tx_msg_downq); + + if (unlikely(drm_debug & DRM_UT_DP)) { + struct drm_printer p = drm_debug_printer(DBG_PREFIX); + + drm_dp_mst_dump_sideband_msg_tx(&p, txmsg); + } + if (list_is_singular(&mgr->tx_msg_downq)) process_single_down_tx_qlock(mgr); mutex_unlock(&mgr->qlock); diff --git a/drivers/gpu/drm/drm_dp_mst_topology_internal.h b/drivers/gpu/drm/drm_dp_mst_topology_internal.h new file mode 100644 index 000000000000..eeda9a61c657 --- /dev/null +++ b/drivers/gpu/drm/drm_dp_mst_topology_internal.h @@ -0,0 +1,24 @@ +/* SPDX-License-Identifier: GPL-2.0-only + * + * Declarations for DP MST related functions which are only used in selftests + * + * Copyright © 2018 Red Hat + * Authors: + * Lyude Paul + */ + +#ifndef _DRM_DP_MST_HELPER_INTERNAL_H_ +#define _DRM_DP_MST_HELPER_INTERNAL_H_ + +#include + +void +drm_dp_encode_sideband_req(const struct drm_dp_sideband_msg_req_body *req, + struct drm_dp_sideband_msg_tx *raw); +int drm_dp_decode_sideband_req(const struct drm_dp_sideband_msg_tx *raw, + struct drm_dp_sideband_msg_req_body *req); +void +drm_dp_dump_sideband_msg_req_body(const struct drm_dp_sideband_msg_req_body *req, + int indent, struct drm_printer *printer); + +#endif /* !_DRM_DP_MST_HELPER_INTERNAL_H_ */ diff --git a/drivers/gpu/drm/selftests/drm_modeset_selftests.h b/drivers/gpu/drm/selftests/drm_modeset_selftests.h index dec3ee3ec96f..1898de0b4a4d 100644 --- a/drivers/gpu/drm/selftests/drm_modeset_selftests.h +++ b/drivers/gpu/drm/selftests/drm_modeset_selftests.h @@ -33,3 +33,4 @@ selftest(damage_iter_damage_one_outside, igt_damage_iter_damage_one_outside) selftest(damage_iter_damage_src_moved, igt_damage_iter_damage_src_moved) selftest(damage_iter_damage_not_visible, igt_damage_iter_damage_not_visible) selftest(dp_mst_calc_pbn_mode, igt_dp_mst_calc_pbn_mode) +selftest(dp_mst_sideband_msg_req_decode, igt_dp_mst_sideband_msg_req_decode) diff --git a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c index 9baa5171988d..af2b2de65316 100644 --- a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c +++ b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c @@ -3,9 +3,12 @@ * Test cases for for the DRM DP MST helpers */ +#define PREFIX_STR "[drm_dp_mst_helper]" + #include #include +#include "../drm_dp_mst_topology_internal.h" #include "test-drm_modeset_common.h" int igt_dp_mst_calc_pbn_mode(void *ignored) @@ -32,3 +35,204 @@ int igt_dp_mst_calc_pbn_mode(void *ignored) return 0; } + +static bool +sideband_msg_req_equal(const struct drm_dp_sideband_msg_req_body *in, + const struct drm_dp_sideband_msg_req_body *out) +{ + const struct drm_dp_remote_i2c_read_tx *txin, *txout; + int i; + + if (in->req_type != out->req_type) + return false; + + switch (in->req_type) { + /* + * Compare struct members manually for request types which can't be + * compared simply using memcmp(). This is because said request types + * contain pointers to other allocated structs + */ + case DP_REMOTE_I2C_READ: +#define IN in->u.i2c_read +#define OUT out->u.i2c_read + if (IN.num_bytes_read != OUT.num_bytes_read || + IN.num_transactions != OUT.num_transactions || + IN.port_number != OUT.port_number || + IN.read_i2c_device_id != OUT.read_i2c_device_id) + return false; + + for (i = 0; i < IN.num_transactions; i++) { + txin = &IN.transactions[i]; + txout = &OUT.transactions[i]; + + if (txin->i2c_dev_id != txout->i2c_dev_id || + txin->no_stop_bit != txout->no_stop_bit || + txin->num_bytes != txout->num_bytes || + txin->i2c_transaction_delay != + txout->i2c_transaction_delay) + return false; + + if (memcmp(txin->bytes, txout->bytes, + txin->num_bytes) != 0) + return false; + } + break; +#undef IN +#undef OUT + + case DP_REMOTE_DPCD_WRITE: +#define IN in->u.dpcd_write +#define OUT out->u.dpcd_write + if (IN.dpcd_address != OUT.dpcd_address || + IN.num_bytes != OUT.num_bytes || + IN.port_number != OUT.port_number) + return false; + + return memcmp(IN.bytes, OUT.bytes, IN.num_bytes) == 0; +#undef IN +#undef OUT + + case DP_REMOTE_I2C_WRITE: +#define IN in->u.i2c_write +#define OUT out->u.i2c_write + if (IN.port_number != OUT.port_number || + IN.write_i2c_device_id != OUT.write_i2c_device_id || + IN.num_bytes != OUT.num_bytes) + return false; + + return memcmp(IN.bytes, OUT.bytes, IN.num_bytes) == 0; +#undef IN +#undef OUT + + default: + return memcmp(in, out, sizeof(*in)) == 0; + } + + return true; +} + +static bool +sideband_msg_req_encode_decode(struct drm_dp_sideband_msg_req_body *in) +{ + struct drm_dp_sideband_msg_req_body out = {0}; + struct drm_printer p = drm_err_printer(PREFIX_STR); + struct drm_dp_sideband_msg_tx txmsg; + int i, ret; + + drm_dp_encode_sideband_req(in, &txmsg); + ret = drm_dp_decode_sideband_req(&txmsg, &out); + if (ret < 0) { + drm_printf(&p, "Failed to decode sideband request: %d\n", + ret); + return false; + } + + if (!sideband_msg_req_equal(in, &out)) { + drm_printf(&p, "Encode/decode failed, expected:\n"); + drm_dp_dump_sideband_msg_req_body(in, 1, &p); + drm_printf(&p, "Got:\n"); + drm_dp_dump_sideband_msg_req_body(&out, 1, &p); + return false; + } + + switch (in->req_type) { + case DP_REMOTE_DPCD_WRITE: + kfree(out.u.dpcd_write.bytes); + break; + case DP_REMOTE_I2C_READ: + for (i = 0; i < out.u.i2c_read.num_transactions; i++) + kfree(out.u.i2c_read.transactions[i].bytes); + break; + case DP_REMOTE_I2C_WRITE: + kfree(out.u.i2c_write.bytes); + break; + } + + /* Clear everything but the req_type for the input */ + memset(&in->u, 0, sizeof(in->u)); + + return true; +} + +int igt_dp_mst_sideband_msg_req_decode(void *unused) +{ + struct drm_dp_sideband_msg_req_body in = { 0 }; + u8 data[] = { 0xff, 0x0, 0xdd }; + int i; + +#define DO_TEST() FAIL_ON(!sideband_msg_req_encode_decode(&in)) + + in.req_type = DP_ENUM_PATH_RESOURCES; + in.u.port_num.port_number = 5; + DO_TEST(); + + in.req_type = DP_POWER_UP_PHY; + in.u.port_num.port_number = 5; + DO_TEST(); + + in.req_type = DP_POWER_DOWN_PHY; + in.u.port_num.port_number = 5; + DO_TEST(); + + in.req_type = DP_ALLOCATE_PAYLOAD; + in.u.allocate_payload.number_sdp_streams = 3; + for (i = 0; i < in.u.allocate_payload.number_sdp_streams; i++) + in.u.allocate_payload.sdp_stream_sink[i] = i + 1; + DO_TEST(); + in.u.allocate_payload.port_number = 0xf; + DO_TEST(); + in.u.allocate_payload.vcpi = 0x7f; + DO_TEST(); + in.u.allocate_payload.pbn = U16_MAX; + DO_TEST(); + + in.req_type = DP_QUERY_PAYLOAD; + in.u.query_payload.port_number = 0xf; + DO_TEST(); + in.u.query_payload.vcpi = 0x7f; + DO_TEST(); + + in.req_type = DP_REMOTE_DPCD_READ; + in.u.dpcd_read.port_number = 0xf; + DO_TEST(); + in.u.dpcd_read.dpcd_address = 0xfedcb; + DO_TEST(); + in.u.dpcd_read.num_bytes = U8_MAX; + DO_TEST(); + + in.req_type = DP_REMOTE_DPCD_WRITE; + in.u.dpcd_write.port_number = 0xf; + DO_TEST(); + in.u.dpcd_write.dpcd_address = 0xfedcb; + DO_TEST(); + in.u.dpcd_write.num_bytes = ARRAY_SIZE(data); + in.u.dpcd_write.bytes = data; + DO_TEST(); + + in.req_type = DP_REMOTE_I2C_READ; + in.u.i2c_read.port_number = 0xf; + DO_TEST(); + in.u.i2c_read.read_i2c_device_id = 0x7f; + DO_TEST(); + in.u.i2c_read.num_transactions = 3; + in.u.i2c_read.num_bytes_read = ARRAY_SIZE(data) * 3; + for (i = 0; i < in.u.i2c_read.num_transactions; i++) { + in.u.i2c_read.transactions[i].bytes = data; + in.u.i2c_read.transactions[i].num_bytes = ARRAY_SIZE(data); + in.u.i2c_read.transactions[i].i2c_dev_id = 0x7f & ~i; + in.u.i2c_read.transactions[i].i2c_transaction_delay = 0xf & ~i; + } + DO_TEST(); + + in.req_type = DP_REMOTE_I2C_WRITE; + in.u.i2c_write.port_number = 0xf; + DO_TEST(); + in.u.i2c_write.write_i2c_device_id = 0x7f; + DO_TEST(); + in.u.i2c_write.num_bytes = ARRAY_SIZE(data); + in.u.i2c_write.bytes = data; + DO_TEST(); + +#undef DO_TEST + return 0; +} diff --git a/drivers/gpu/drm/selftests/test-drm_modeset_common.h b/drivers/gpu/drm/selftests/test-drm_modeset_common.h index 590bda35a683..0fcb8bbc6a1b 100644 --- a/drivers/gpu/drm/selftests/test-drm_modeset_common.h +++ b/drivers/gpu/drm/selftests/test-drm_modeset_common.h @@ -40,5 +40,6 @@ int igt_damage_iter_damage_one_outside(void *ignored); int igt_damage_iter_damage_src_moved(void *ignored); int igt_damage_iter_damage_not_visible(void *ignored); int igt_dp_mst_calc_pbn_mode(void *ignored); +int igt_dp_mst_sideband_msg_req_decode(void *ignored); #endif diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index fc349204a71b..f09dabd595a5 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -287,7 +287,7 @@ struct drm_dp_remote_dpcd_write { struct drm_dp_remote_i2c_read { u8 num_transactions; u8 port_number; - struct { + struct drm_dp_remote_i2c_read_tx { u8 i2c_dev_id; u8 num_bytes; u8 *bytes; -- cgit v1.2.3 From 95b0013d209dc2fb2a4c8a6025b79e4bb6fabec8 Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Tue, 3 Sep 2019 16:45:47 -0400 Subject: drm/dp_mst: Refactor drm_dp_send_enum_path_resources MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use more pointers so we don't have to write out txmsg->reply.u.path_resources each time. Also, fix line wrapping + rearrange local variables. Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Reviewed-by: Daniel Vetter Signed-off-by: Lyude Paul Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-10-lyude@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index ae1a3c4b6b17..c8502f4a1cde 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -2463,12 +2463,14 @@ static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, kfree(txmsg); } -static int drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr, - struct drm_dp_mst_branch *mstb, - struct drm_dp_mst_port *port) +static int +drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr, + struct drm_dp_mst_branch *mstb, + struct drm_dp_mst_port *port) { - int len; + struct drm_dp_enum_path_resources_ack_reply *path_res; struct drm_dp_sideband_msg_tx *txmsg; + int len; int ret; txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL); @@ -2482,14 +2484,20 @@ static int drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr, ret = drm_dp_mst_wait_tx_reply(mstb, txmsg); if (ret > 0) { + path_res = &txmsg->reply.u.path_resources; + if (txmsg->reply.reply_type == DP_SIDEBAND_REPLY_NAK) { DRM_DEBUG_KMS("enum path resources nak received\n"); } else { - if (port->port_num != txmsg->reply.u.path_resources.port_number) + if (port->port_num != path_res->port_number) DRM_ERROR("got incorrect port in response\n"); - DRM_DEBUG_KMS("enum path resources %d: %d %d\n", txmsg->reply.u.path_resources.port_number, txmsg->reply.u.path_resources.full_payload_bw_number, - txmsg->reply.u.path_resources.avail_payload_bw_number); - port->available_pbn = txmsg->reply.u.path_resources.avail_payload_bw_number; + + DRM_DEBUG_KMS("enum path resources %d: %d %d\n", + path_res->port_number, + path_res->full_payload_bw_number, + path_res->avail_payload_bw_number); + port->available_pbn = + path_res->avail_payload_bw_number; } } -- cgit v1.2.3 From 60f9ae9d0d3dfb4c1ee8af476e04aaa11d5fd34d Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Tue, 3 Sep 2019 16:45:48 -0400 Subject: drm/dp_mst: Remove huge conditional in drm_dp_mst_handle_up_req() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Which reduces indentation and makes this function more legible. Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Reviewed-by: Daniel Vetter Signed-off-by: Lyude Paul Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-11-lyude@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 90 +++++++++++++++++------------------ 1 file changed, 45 insertions(+), 45 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index c8502f4a1cde..22b4dc8e7979 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3271,7 +3271,9 @@ static int drm_dp_mst_handle_down_rep(struct drm_dp_mst_topology_mgr *mgr) static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr) { - int ret = 0; + struct drm_dp_sideband_msg_req_body msg; + struct drm_dp_mst_branch *mstb = NULL; + bool seqno; if (!drm_dp_get_one_sb_msg(mgr, true)) { memset(&mgr->up_req_recv, 0, @@ -3279,62 +3281,60 @@ static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr) return 0; } - if (mgr->up_req_recv.have_eomt) { - struct drm_dp_sideband_msg_req_body msg; - struct drm_dp_mst_branch *mstb = NULL; - bool seqno; - - if (!mgr->up_req_recv.initial_hdr.broadcast) { - mstb = drm_dp_get_mst_branch_device(mgr, - mgr->up_req_recv.initial_hdr.lct, - mgr->up_req_recv.initial_hdr.rad); - if (!mstb) { - DRM_DEBUG_KMS("Got MST reply from unknown device %d\n", mgr->up_req_recv.initial_hdr.lct); - memset(&mgr->up_req_recv, 0, sizeof(struct drm_dp_sideband_msg_rx)); - return 0; - } + if (!mgr->up_req_recv.have_eomt) + return 0; + + if (!mgr->up_req_recv.initial_hdr.broadcast) { + mstb = drm_dp_get_mst_branch_device(mgr, + mgr->up_req_recv.initial_hdr.lct, + mgr->up_req_recv.initial_hdr.rad); + if (!mstb) { + DRM_DEBUG_KMS("Got MST reply from unknown device %d\n", mgr->up_req_recv.initial_hdr.lct); + memset(&mgr->up_req_recv, 0, sizeof(struct drm_dp_sideband_msg_rx)); + return 0; } + } - seqno = mgr->up_req_recv.initial_hdr.seqno; - drm_dp_sideband_parse_req(&mgr->up_req_recv, &msg); + seqno = mgr->up_req_recv.initial_hdr.seqno; + drm_dp_sideband_parse_req(&mgr->up_req_recv, &msg); - if (msg.req_type == DP_CONNECTION_STATUS_NOTIFY) { - drm_dp_send_up_ack_reply(mgr, mgr->mst_primary, msg.req_type, seqno, false); + if (msg.req_type == DP_CONNECTION_STATUS_NOTIFY) { + drm_dp_send_up_ack_reply(mgr, mgr->mst_primary, msg.req_type, seqno, false); - if (!mstb) - mstb = drm_dp_get_mst_branch_device_by_guid(mgr, msg.u.conn_stat.guid); + if (!mstb) + mstb = drm_dp_get_mst_branch_device_by_guid(mgr, msg.u.conn_stat.guid); - if (!mstb) { - DRM_DEBUG_KMS("Got MST reply from unknown device %d\n", mgr->up_req_recv.initial_hdr.lct); - memset(&mgr->up_req_recv, 0, sizeof(struct drm_dp_sideband_msg_rx)); - return 0; - } + if (!mstb) { + DRM_DEBUG_KMS("Got MST reply from unknown device %d\n", mgr->up_req_recv.initial_hdr.lct); + memset(&mgr->up_req_recv, 0, sizeof(struct drm_dp_sideband_msg_rx)); + return 0; + } - drm_dp_update_port(mstb, &msg.u.conn_stat); + drm_dp_update_port(mstb, &msg.u.conn_stat); - DRM_DEBUG_KMS("Got CSN: pn: %d ldps:%d ddps: %d mcs: %d ip: %d pdt: %d\n", msg.u.conn_stat.port_number, msg.u.conn_stat.legacy_device_plug_status, msg.u.conn_stat.displayport_device_plug_status, msg.u.conn_stat.message_capability_status, msg.u.conn_stat.input_port, msg.u.conn_stat.peer_device_type); - drm_kms_helper_hotplug_event(mgr->dev); - - } else if (msg.req_type == DP_RESOURCE_STATUS_NOTIFY) { - drm_dp_send_up_ack_reply(mgr, mgr->mst_primary, msg.req_type, seqno, false); - if (!mstb) - mstb = drm_dp_get_mst_branch_device_by_guid(mgr, msg.u.resource_stat.guid); + DRM_DEBUG_KMS("Got CSN: pn: %d ldps:%d ddps: %d mcs: %d ip: %d pdt: %d\n", msg.u.conn_stat.port_number, msg.u.conn_stat.legacy_device_plug_status, msg.u.conn_stat.displayport_device_plug_status, msg.u.conn_stat.message_capability_status, msg.u.conn_stat.input_port, msg.u.conn_stat.peer_device_type); + drm_kms_helper_hotplug_event(mgr->dev); - if (!mstb) { - DRM_DEBUG_KMS("Got MST reply from unknown device %d\n", mgr->up_req_recv.initial_hdr.lct); - memset(&mgr->up_req_recv, 0, sizeof(struct drm_dp_sideband_msg_rx)); - return 0; - } + } else if (msg.req_type == DP_RESOURCE_STATUS_NOTIFY) { + drm_dp_send_up_ack_reply(mgr, mgr->mst_primary, msg.req_type, seqno, false); + if (!mstb) + mstb = drm_dp_get_mst_branch_device_by_guid(mgr, msg.u.resource_stat.guid); - DRM_DEBUG_KMS("Got RSN: pn: %d avail_pbn %d\n", msg.u.resource_stat.port_number, msg.u.resource_stat.available_pbn); + if (!mstb) { + DRM_DEBUG_KMS("Got MST reply from unknown device %d\n", mgr->up_req_recv.initial_hdr.lct); + memset(&mgr->up_req_recv, 0, sizeof(struct drm_dp_sideband_msg_rx)); + return 0; } - if (mstb) - drm_dp_mst_topology_put_mstb(mstb); - - memset(&mgr->up_req_recv, 0, sizeof(struct drm_dp_sideband_msg_rx)); + DRM_DEBUG_KMS("Got RSN: pn: %d avail_pbn %d\n", msg.u.resource_stat.port_number, msg.u.resource_stat.available_pbn); } - return ret; + + if (mstb) + drm_dp_mst_topology_put_mstb(mstb); + + memset(&mgr->up_req_recv, 0, sizeof(struct drm_dp_sideband_msg_rx)); + + return 0; } /** -- cgit v1.2.3 From fde61a7a7df46ce8206c8a7b3aa181c330113184 Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Tue, 3 Sep 2019 16:45:49 -0400 Subject: drm/dp_mst: Constify guid in drm_dp_get_mst_branch_by_guid() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit And it's helper, we'll be using this in just a moment. Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Cc: Daniel Vetter Signed-off-by: Lyude Paul Reviewed-by: Dave Airlie Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-12-lyude@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 22b4dc8e7979..33098084ad70 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -2086,7 +2086,7 @@ out: static struct drm_dp_mst_branch *get_mst_branch_device_by_guid_helper( struct drm_dp_mst_branch *mstb, - uint8_t *guid) + const uint8_t *guid) { struct drm_dp_mst_branch *found_mstb; struct drm_dp_mst_port *port; @@ -2110,7 +2110,7 @@ static struct drm_dp_mst_branch *get_mst_branch_device_by_guid_helper( static struct drm_dp_mst_branch * drm_dp_get_mst_branch_device_by_guid(struct drm_dp_mst_topology_mgr *mgr, - uint8_t *guid) + const uint8_t *guid) { struct drm_dp_mst_branch *mstb; int ret; -- cgit v1.2.3 From a29d881875fc27c3ffe56af242adf38448b3d857 Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Tue, 3 Sep 2019 16:45:50 -0400 Subject: drm/dp_mst: Refactor drm_dp_mst_handle_up_req() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There's a couple of changes here, so to summarize: * Remove the big ugly mgr->up_req_recv.have_eomt conditional to save on indenting * Store &mgr->up_req_recv.initial_hdr in a variable so we don't keep going over 80 character long lines * De-duplicate code for calling drm_dp_send_up_ack_reply() and getting the MSTB via it's GUID * Remove all of the duplicate calls to memset() and just use a goto instead * Actually do line wrapping * Remove the unnecessary if (mstb) check before calling drm_dp_mst_topology_put_mstb() - we are guaranteed to always have mstb != NULL at that point in the function Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Reviewed-by: Daniel Vetter Signed-off-by: Lyude Paul Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-13-lyude@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 75 ++++++++++++++++++----------------- 1 file changed, 38 insertions(+), 37 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 33098084ad70..b09f253fadec 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3272,68 +3272,69 @@ static int drm_dp_mst_handle_down_rep(struct drm_dp_mst_topology_mgr *mgr) static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr) { struct drm_dp_sideband_msg_req_body msg; + struct drm_dp_sideband_msg_hdr *hdr = &mgr->up_req_recv.initial_hdr; struct drm_dp_mst_branch *mstb = NULL; + const u8 *guid; bool seqno; - if (!drm_dp_get_one_sb_msg(mgr, true)) { - memset(&mgr->up_req_recv, 0, - sizeof(struct drm_dp_sideband_msg_rx)); - return 0; - } + if (!drm_dp_get_one_sb_msg(mgr, true)) + goto out; if (!mgr->up_req_recv.have_eomt) return 0; - if (!mgr->up_req_recv.initial_hdr.broadcast) { - mstb = drm_dp_get_mst_branch_device(mgr, - mgr->up_req_recv.initial_hdr.lct, - mgr->up_req_recv.initial_hdr.rad); + if (!hdr->broadcast) { + mstb = drm_dp_get_mst_branch_device(mgr, hdr->lct, hdr->rad); if (!mstb) { - DRM_DEBUG_KMS("Got MST reply from unknown device %d\n", mgr->up_req_recv.initial_hdr.lct); - memset(&mgr->up_req_recv, 0, sizeof(struct drm_dp_sideband_msg_rx)); - return 0; + DRM_DEBUG_KMS("Got MST reply from unknown device %d\n", + hdr->lct); + goto out; } } - seqno = mgr->up_req_recv.initial_hdr.seqno; + seqno = hdr->seqno; drm_dp_sideband_parse_req(&mgr->up_req_recv, &msg); - if (msg.req_type == DP_CONNECTION_STATUS_NOTIFY) { - drm_dp_send_up_ack_reply(mgr, mgr->mst_primary, msg.req_type, seqno, false); + if (msg.req_type == DP_CONNECTION_STATUS_NOTIFY) + guid = msg.u.conn_stat.guid; + else if (msg.req_type == DP_RESOURCE_STATUS_NOTIFY) + guid = msg.u.resource_stat.guid; + else + goto out; - if (!mstb) - mstb = drm_dp_get_mst_branch_device_by_guid(mgr, msg.u.conn_stat.guid); + drm_dp_send_up_ack_reply(mgr, mgr->mst_primary, msg.req_type, seqno, + false); + if (!mstb) { + mstb = drm_dp_get_mst_branch_device_by_guid(mgr, guid); if (!mstb) { - DRM_DEBUG_KMS("Got MST reply from unknown device %d\n", mgr->up_req_recv.initial_hdr.lct); - memset(&mgr->up_req_recv, 0, sizeof(struct drm_dp_sideband_msg_rx)); - return 0; + DRM_DEBUG_KMS("Got MST reply from unknown device %d\n", + hdr->lct); + goto out; } + } + if (msg.req_type == DP_CONNECTION_STATUS_NOTIFY) { drm_dp_update_port(mstb, &msg.u.conn_stat); - DRM_DEBUG_KMS("Got CSN: pn: %d ldps:%d ddps: %d mcs: %d ip: %d pdt: %d\n", msg.u.conn_stat.port_number, msg.u.conn_stat.legacy_device_plug_status, msg.u.conn_stat.displayport_device_plug_status, msg.u.conn_stat.message_capability_status, msg.u.conn_stat.input_port, msg.u.conn_stat.peer_device_type); - drm_kms_helper_hotplug_event(mgr->dev); + DRM_DEBUG_KMS("Got CSN: pn: %d ldps:%d ddps: %d mcs: %d ip: %d pdt: %d\n", + msg.u.conn_stat.port_number, + msg.u.conn_stat.legacy_device_plug_status, + msg.u.conn_stat.displayport_device_plug_status, + msg.u.conn_stat.message_capability_status, + msg.u.conn_stat.input_port, + msg.u.conn_stat.peer_device_type); + drm_kms_helper_hotplug_event(mgr->dev); } else if (msg.req_type == DP_RESOURCE_STATUS_NOTIFY) { - drm_dp_send_up_ack_reply(mgr, mgr->mst_primary, msg.req_type, seqno, false); - if (!mstb) - mstb = drm_dp_get_mst_branch_device_by_guid(mgr, msg.u.resource_stat.guid); - - if (!mstb) { - DRM_DEBUG_KMS("Got MST reply from unknown device %d\n", mgr->up_req_recv.initial_hdr.lct); - memset(&mgr->up_req_recv, 0, sizeof(struct drm_dp_sideband_msg_rx)); - return 0; - } - - DRM_DEBUG_KMS("Got RSN: pn: %d avail_pbn %d\n", msg.u.resource_stat.port_number, msg.u.resource_stat.available_pbn); + DRM_DEBUG_KMS("Got RSN: pn: %d avail_pbn %d\n", + msg.u.resource_stat.port_number, + msg.u.resource_stat.available_pbn); } - if (mstb) - drm_dp_mst_topology_put_mstb(mstb); - + drm_dp_mst_topology_put_mstb(mstb); +out: memset(&mgr->up_req_recv, 0, sizeof(struct drm_dp_sideband_msg_rx)); - return 0; } -- cgit v1.2.3 From 8b1e589d138c8ad82a3424a9f3df8a74a4a2837a Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Tue, 3 Sep 2019 16:45:51 -0400 Subject: drm/dp_mst: Refactor drm_dp_mst_handle_down_rep() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Remove the big ugly have_eomt conditional * Store &mgr->down_rep_recv.initial_hdr in a var to make line wrapping easier * Remove duplicate memset() calls * Actually wrap lines Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Reviewed-by: Daniel Vetter Signed-off-by: Lyude Paul Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-14-lyude@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 102 +++++++++++++++++----------------- 1 file changed, 50 insertions(+), 52 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index b09f253fadec..c38fd759ea05 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3205,68 +3205,66 @@ static bool drm_dp_get_one_sb_msg(struct drm_dp_mst_topology_mgr *mgr, bool up) static int drm_dp_mst_handle_down_rep(struct drm_dp_mst_topology_mgr *mgr) { - int ret = 0; + struct drm_dp_sideband_msg_tx *txmsg; + struct drm_dp_mst_branch *mstb; + struct drm_dp_sideband_msg_hdr *hdr = &mgr->down_rep_recv.initial_hdr; + int slot = -1; + + if (!drm_dp_get_one_sb_msg(mgr, false)) + goto clear_down_rep_recv; - if (!drm_dp_get_one_sb_msg(mgr, false)) { - memset(&mgr->down_rep_recv, 0, - sizeof(struct drm_dp_sideband_msg_rx)); + if (!mgr->down_rep_recv.have_eomt) return 0; + + mstb = drm_dp_get_mst_branch_device(mgr, hdr->lct, hdr->rad); + if (!mstb) { + DRM_DEBUG_KMS("Got MST reply from unknown device %d\n", + hdr->lct); + goto clear_down_rep_recv; } - if (mgr->down_rep_recv.have_eomt) { - struct drm_dp_sideband_msg_tx *txmsg; - struct drm_dp_mst_branch *mstb; - int slot = -1; - mstb = drm_dp_get_mst_branch_device(mgr, - mgr->down_rep_recv.initial_hdr.lct, - mgr->down_rep_recv.initial_hdr.rad); + /* find the message */ + slot = hdr->seqno; + mutex_lock(&mgr->qlock); + txmsg = mstb->tx_slots[slot]; + /* remove from slots */ + mutex_unlock(&mgr->qlock); - if (!mstb) { - DRM_DEBUG_KMS("Got MST reply from unknown device %d\n", mgr->down_rep_recv.initial_hdr.lct); - memset(&mgr->down_rep_recv, 0, sizeof(struct drm_dp_sideband_msg_rx)); - return 0; - } + if (!txmsg) { + DRM_DEBUG_KMS("Got MST reply with no msg %p %d %d %02x %02x\n", + mstb, hdr->seqno, hdr->lct, hdr->rad[0], + mgr->down_rep_recv.msg[0]); + goto no_msg; + } - /* find the message */ - slot = mgr->down_rep_recv.initial_hdr.seqno; - mutex_lock(&mgr->qlock); - txmsg = mstb->tx_slots[slot]; - /* remove from slots */ - mutex_unlock(&mgr->qlock); - - if (!txmsg) { - DRM_DEBUG_KMS("Got MST reply with no msg %p %d %d %02x %02x\n", - mstb, - mgr->down_rep_recv.initial_hdr.seqno, - mgr->down_rep_recv.initial_hdr.lct, - mgr->down_rep_recv.initial_hdr.rad[0], - mgr->down_rep_recv.msg[0]); - drm_dp_mst_topology_put_mstb(mstb); - memset(&mgr->down_rep_recv, 0, sizeof(struct drm_dp_sideband_msg_rx)); - return 0; - } + drm_dp_sideband_parse_reply(&mgr->down_rep_recv, &txmsg->reply); - drm_dp_sideband_parse_reply(&mgr->down_rep_recv, &txmsg->reply); + if (txmsg->reply.reply_type == DP_SIDEBAND_REPLY_NAK) + DRM_DEBUG_KMS("Got NAK reply: req 0x%02x (%s), reason 0x%02x (%s), nak data 0x%02x\n", + txmsg->reply.req_type, + drm_dp_mst_req_type_str(txmsg->reply.req_type), + txmsg->reply.u.nak.reason, + drm_dp_mst_nak_reason_str(txmsg->reply.u.nak.reason), + txmsg->reply.u.nak.nak_data); - if (txmsg->reply.reply_type == DP_SIDEBAND_REPLY_NAK) - DRM_DEBUG_KMS("Got NAK reply: req 0x%02x (%s), reason 0x%02x (%s), nak data 0x%02x\n", - txmsg->reply.req_type, - drm_dp_mst_req_type_str(txmsg->reply.req_type), - txmsg->reply.u.nak.reason, - drm_dp_mst_nak_reason_str(txmsg->reply.u.nak.reason), - txmsg->reply.u.nak.nak_data); - - memset(&mgr->down_rep_recv, 0, sizeof(struct drm_dp_sideband_msg_rx)); - drm_dp_mst_topology_put_mstb(mstb); + memset(&mgr->down_rep_recv, 0, sizeof(struct drm_dp_sideband_msg_rx)); + drm_dp_mst_topology_put_mstb(mstb); - mutex_lock(&mgr->qlock); - txmsg->state = DRM_DP_SIDEBAND_TX_RX; - mstb->tx_slots[slot] = NULL; - mutex_unlock(&mgr->qlock); + mutex_lock(&mgr->qlock); + txmsg->state = DRM_DP_SIDEBAND_TX_RX; + mstb->tx_slots[slot] = NULL; + mutex_unlock(&mgr->qlock); - wake_up_all(&mgr->tx_waitq); - } - return ret; + wake_up_all(&mgr->tx_waitq); + + return 0; + +no_msg: + drm_dp_mst_topology_put_mstb(mstb); +clear_down_rep_recv: + memset(&mgr->down_rep_recv, 0, sizeof(struct drm_dp_sideband_msg_rx)); + + return 0; } static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr) -- cgit v1.2.3 From 37dfdc55ffeb08b20b623eba04a3e3eb3a261962 Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Tue, 3 Sep 2019 16:45:53 -0400 Subject: drm/dp_mst: Cleanup drm_dp_send_link_address() a bit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Declare local pointer to the drm_dp_link_address_ack_reply struct instead of constantly dereferencing it through the union in txmsg->reply. Then, invert the order of conditionals so we don't have to do the bulk of the work inside them, and can wrap lines even less. Then finally, rearrange variable declarations a bit. Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Cc: Daniel Vetter Signed-off-by: Lyude Paul Reviewed-by: Dave Airlie Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-16-lyude@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 42 +++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 19 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index c38fd759ea05..97216099a718 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -2424,9 +2424,9 @@ drm_dp_dump_link_address(struct drm_dp_link_address_ack_reply *reply) static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_branch *mstb) { - int len; struct drm_dp_sideband_msg_tx *txmsg; - int ret; + struct drm_dp_link_address_ack_reply *reply; + int i, len, ret; txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL); if (!txmsg) @@ -2438,28 +2438,32 @@ static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, mstb->link_address_sent = true; drm_dp_queue_down_tx(mgr, txmsg); + /* FIXME: Actually do some real error handling here */ ret = drm_dp_mst_wait_tx_reply(mstb, txmsg); - if (ret > 0) { - int i; + if (ret <= 0) { + DRM_ERROR("Sending link address failed with %d\n", ret); + goto out; + } + if (txmsg->reply.reply_type == DP_SIDEBAND_REPLY_NAK) { + DRM_ERROR("link address NAK received\n"); + ret = -EIO; + goto out; + } - if (txmsg->reply.reply_type == DP_SIDEBAND_REPLY_NAK) { - DRM_DEBUG_KMS("link address nak received\n"); - } else { - DRM_DEBUG_KMS("link address reply: %d\n", txmsg->reply.u.link_addr.nports); - drm_dp_dump_link_address(&txmsg->reply.u.link_addr); + reply = &txmsg->reply.u.link_addr; + DRM_DEBUG_KMS("link address reply: %d\n", reply->nports); + drm_dp_dump_link_address(reply); - drm_dp_check_mstb_guid(mstb, txmsg->reply.u.link_addr.guid); + drm_dp_check_mstb_guid(mstb, reply->guid); - for (i = 0; i < txmsg->reply.u.link_addr.nports; i++) { - drm_dp_add_port(mstb, mgr->dev, &txmsg->reply.u.link_addr.ports[i]); - } - drm_kms_helper_hotplug_event(mgr->dev); - } - } else { - mstb->link_address_sent = false; - DRM_DEBUG_KMS("link address failed %d\n", ret); - } + for (i = 0; i < reply->nports; i++) + drm_dp_add_port(mstb, mgr->dev, &reply->ports[i]); + + drm_kms_helper_hotplug_event(mgr->dev); +out: + if (ret <= 0) + mstb->link_address_sent = false; kfree(txmsg); } -- cgit v1.2.3 From 50094b5dcd3238c0cf91a5768d0316adaeb32e45 Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Tue, 3 Sep 2019 16:45:52 -0400 Subject: drm/dp_mst: Destroy topology_mgr mutexes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turns out we've been forgetting for a while now to actually destroy any of the mutexes that we create in drm_dp_mst_topology_mgr. So, let's do that. Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Cc: Daniel Vetter Signed-off-by: Lyude Paul Reviewed-by: Sean Paul Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-15-lyude@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 97216099a718..624adb0fc4b2 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -4297,6 +4297,11 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr) mgr->aux = NULL; drm_atomic_private_obj_fini(&mgr->base); mgr->funcs = NULL; + + mutex_destroy(&mgr->delayed_destroy_lock); + mutex_destroy(&mgr->payload_lock); + mutex_destroy(&mgr->qlock); + mutex_destroy(&mgr->lock); } EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy); -- cgit v1.2.3 From e2839ff692c628a5a1caa6721e45ab5d212f9aca Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Tue, 3 Sep 2019 16:45:55 -0400 Subject: drm/dp_mst: Rename drm_dp_add_port and drm_dp_update_port MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The names for these functions are rather confusing. drm_dp_add_port() sounds like a function that would simply create a port and add it to a topology, and do nothing more. Similarly, drm_dp_update_port() would be assumed to be the function that should be used to update port information after initial creation. While those assumptions are currently correct in how these functions are used, a quick glance at drm_dp_add_port() reveals that drm_dp_add_port() can also update the information on a port, and seems explicitly designed to do so. This can be explained pretty simply by the fact that there's more situations that would involve updating the port information based on a link address response as opposed to a connection status notification than the driver's initial topology probe. Case in point: reprobing link addresses after suspend/resume. Since we're about to start using drm_dp_add_port() differently for suspend/resume reprobing, let's rename both functions to clarify what they actually do. Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Cc: Daniel Vetter Signed-off-by: Lyude Paul Reviewed-by: Sean Paul Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-18-lyude@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 624adb0fc4b2..01d97c6cbbb2 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1903,9 +1903,10 @@ void drm_dp_mst_connector_early_unregister(struct drm_connector *connector, } EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister); -static void drm_dp_add_port(struct drm_dp_mst_branch *mstb, - struct drm_device *dev, - struct drm_dp_link_addr_reply_port *port_msg) +static void +drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, + struct drm_device *dev, + struct drm_dp_link_addr_reply_port *port_msg) { struct drm_dp_mst_port *port; bool ret; @@ -2008,8 +2009,9 @@ out: drm_dp_mst_topology_put_port(port); } -static void drm_dp_update_port(struct drm_dp_mst_branch *mstb, - struct drm_dp_connection_status_notify *conn_stat) +static void +drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb, + struct drm_dp_connection_status_notify *conn_stat) { struct drm_dp_mst_port *port; int old_pdt; @@ -2457,7 +2459,8 @@ static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, drm_dp_check_mstb_guid(mstb, reply->guid); for (i = 0; i < reply->nports; i++) - drm_dp_add_port(mstb, mgr->dev, &reply->ports[i]); + drm_dp_mst_handle_link_address_port(mstb, mgr->dev, + &reply->ports[i]); drm_kms_helper_hotplug_event(mgr->dev); @@ -3317,7 +3320,7 @@ static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr) } if (msg.req_type == DP_CONNECTION_STATUS_NOTIFY) { - drm_dp_update_port(mstb, &msg.u.conn_stat); + drm_dp_mst_handle_conn_stat(mstb, &msg.u.conn_stat); DRM_DEBUG_KMS("Got CSN: pn: %d ldps:%d ddps: %d mcs: %d ip: %d pdt: %d\n", msg.u.conn_stat.port_number, -- cgit v1.2.3 From caf81ec6cd72f7efd47eb8793e1e166678e35394 Mon Sep 17 00:00:00 2001 From: Matt Roper Date: Wed, 25 Sep 2019 15:46:17 -0700 Subject: drm: Destroy the correct mutex name in drm_dp_mst_topology_mgr_destroy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It looks like one of the topology manager mutexes may have been renamed during a rebase, but the destruction function wasn't updated with the new name: error: ‘struct drm_dp_mst_topology_mgr’ has no member named ‘delayed_destroy_lock’ Fixes: 50094b5dcd32 ("drm/dp_mst: Destroy topology_mgr mutexes") Cc: Lyude Paul Cc: Sean Paul Signed-off-by: Matt Roper Reviewed-by: Lyude Paul Signed-off-by: Lyude Paul Link: https://patchwork.freedesktop.org/patch/msgid/20190925224617.24027-1-matthew.d.roper@intel.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 01d97c6cbbb2..c8804ca6264c 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -4301,7 +4301,7 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr) drm_atomic_private_obj_fini(&mgr->base); mgr->funcs = NULL; - mutex_destroy(&mgr->delayed_destroy_lock); + mutex_destroy(&mgr->destroy_connector_lock); mutex_destroy(&mgr->payload_lock); mutex_destroy(&mgr->qlock); mutex_destroy(&mgr->lock); -- cgit v1.2.3 From db0cc143b69e48e051610ae716d305270ea41ff6 Mon Sep 17 00:00:00 2001 From: Ville Syrjälä Date: Wed, 25 Sep 2019 17:14:40 +0300 Subject: drm/dp/mst: Reduce nested ifs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the nested ifs with a single if and a logical AND. Cc: Lyude Paul Signed-off-by: Ville Syrjälä Link: https://patchwork.freedesktop.org/patch/msgid/20190925141442.23236-1-ville.syrjala@linux.intel.com Reviewed-by: Lucas De Marchi Reviewed-by: Lyude Paul --- drivers/gpu/drm/drm_dp_mst_topology.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index c8804ca6264c..dd06a50d4938 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1123,11 +1123,11 @@ static void drm_dp_mst_put_payload_id(struct drm_dp_mst_topology_mgr *mgr, clear_bit(vcpi - 1, &mgr->vcpi_mask); for (i = 0; i < mgr->max_payloads; i++) { - if (mgr->proposed_vcpis[i]) - if (mgr->proposed_vcpis[i]->vcpi == vcpi) { - mgr->proposed_vcpis[i] = NULL; - clear_bit(i + 1, &mgr->payload_mask); - } + if (mgr->proposed_vcpis[i] && + mgr->proposed_vcpis[i]->vcpi == vcpi) { + mgr->proposed_vcpis[i] = NULL; + clear_bit(i + 1, &mgr->payload_mask); + } } mutex_unlock(&mgr->payload_lock); } -- cgit v1.2.3 From 0c3bb15cfc0eb1b509de1b7dd9a0e8c18c0ac5e5 Mon Sep 17 00:00:00 2001 From: Ville Syrjälä Date: Wed, 25 Sep 2019 17:14:41 +0300 Subject: drm/dp/mst: Handle arbitrary DP_LINK_BW values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make drm_dp_get_vc_payload() tolerate arbitrary DP_LINK_BW_* values, just like drm_dp_bw_code_to_link_rate() does since commit 57a1b0893782 ("drm: Make the bw/link rate calculations more forgiving"). Cc: Lyude Paul Cc: Sean Paul Signed-off-by: Ville Syrjälä Link: https://patchwork.freedesktop.org/patch/msgid/20190925141442.23236-2-ville.syrjala@linux.intel.com Reviewed-by: Lyude Paul --- drivers/gpu/drm/drm_dp_mst_topology.c | 29 ++++++----------------------- 1 file changed, 6 insertions(+), 23 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index dd06a50d4938..f90f18532367 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -2977,30 +2977,13 @@ static int drm_dp_send_up_ack_reply(struct drm_dp_mst_topology_mgr *mgr, return 0; } -static bool drm_dp_get_vc_payload_bw(int dp_link_bw, - int dp_link_count, - int *out) +static int drm_dp_get_vc_payload_bw(u8 dp_link_bw, u8 dp_link_count) { - switch (dp_link_bw) { - default: + if (dp_link_bw == 0 || dp_link_count == 0) DRM_DEBUG_KMS("invalid link bandwidth in DPCD: %x (link count: %d)\n", dp_link_bw, dp_link_count); - return false; - case DP_LINK_BW_1_62: - *out = 3 * dp_link_count; - break; - case DP_LINK_BW_2_7: - *out = 5 * dp_link_count; - break; - case DP_LINK_BW_5_4: - *out = 10 * dp_link_count; - break; - case DP_LINK_BW_8_1: - *out = 15 * dp_link_count; - break; - } - return true; + return dp_link_bw * dp_link_count / 2; } /** @@ -3032,9 +3015,9 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms goto out_unlock; } - if (!drm_dp_get_vc_payload_bw(mgr->dpcd[1], - mgr->dpcd[2] & DP_MAX_LANE_COUNT_MASK, - &mgr->pbn_div)) { + mgr->pbn_div = drm_dp_get_vc_payload_bw(mgr->dpcd[1], + mgr->dpcd[2] & DP_MAX_LANE_COUNT_MASK); + if (mgr->pbn_div == 0) { ret = -EINVAL; goto out_unlock; } -- cgit v1.2.3 From ed20b7d5c65785909816a462aa7d3174064a58a3 Mon Sep 17 00:00:00 2001 From: Ville Syrjälä Date: Wed, 25 Sep 2019 17:14:42 +0300 Subject: drm/dp/mst: Replace the fixed point thing with straight calculation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Get rid of the drm_fixp_from_fraction() usage and just do the straightforward calculation directly. Cc: Lyude Paul Cc: Harry Wentland Cc: Alex Deucher Signed-off-by: Ville Syrjälä Link: https://patchwork.freedesktop.org/patch/msgid/20190925141442.23236-3-ville.syrjala@linux.intel.com Reviewed-by: Lyude Paul --- drivers/gpu/drm/drm_dp_mst_topology.c | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index f90f18532367..e6801db54d0f 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -32,7 +32,6 @@ #include #include #include -#include #include #include @@ -3843,13 +3842,6 @@ EXPORT_SYMBOL(drm_dp_check_act_status); */ int drm_dp_calc_pbn_mode(int clock, int bpp) { - u64 kbps; - s64 peak_kbps; - u32 numerator; - u32 denominator; - - kbps = clock * bpp; - /* * margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006 * The unit of 54/64Mbytes/sec is an arbitrary unit chosen based on @@ -3860,14 +3852,8 @@ int drm_dp_calc_pbn_mode(int clock, int bpp) * peak_kbps *= (64/54) * peak_kbps *= 8 convert to bytes */ - - numerator = 64 * 1006; - denominator = 54 * 8 * 1000 * 1000; - - kbps *= numerator; - peak_kbps = drm_fixp_from_fraction(kbps, denominator); - - return drm_fixp2int_ceil(peak_kbps); + return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006), + 8 * 54 * 1000 * 1000); } EXPORT_SYMBOL(drm_dp_calc_pbn_mode); -- cgit v1.2.3 From f0a8f533adc202f7dc83961c85187794eaed9cec Mon Sep 17 00:00:00 2001 From: Jani Nikula Date: Tue, 1 Oct 2019 17:06:14 +0300 Subject: drm/print: add drm_debug_enabled() Add helper to check if a drm debug category is enabled. Convert drm core to use it. No functional changes. v2: Move unlikely() to drm_debug_enabled() (Eric) v3: Keep unlikely() when combined with other conditions (Eric) Cc: Eric Engestrom Acked-by: Alex Deucher Reviewed-by: Eric Engestrom Signed-off-by: Jani Nikula Link: https://patchwork.freedesktop.org/patch/msgid/20191001140614.26909-1-jani.nikula@intel.com --- drivers/gpu/drm/drm_atomic_uapi.c | 2 +- drivers/gpu/drm/drm_dp_mst_topology.c | 6 +++--- drivers/gpu/drm/drm_edid.c | 2 +- drivers/gpu/drm/drm_edid_load.c | 2 +- drivers/gpu/drm/drm_mipi_dbi.c | 4 ++-- drivers/gpu/drm/drm_print.c | 4 ++-- drivers/gpu/drm/drm_vblank.c | 6 +++--- include/drm/drm_print.h | 5 +++++ 8 files changed, 18 insertions(+), 13 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 5a5b42db6f2a..b4b6b1f8e49d 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -1406,7 +1406,7 @@ retry: } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) { ret = drm_atomic_nonblocking_commit(state); } else { - if (unlikely(drm_debug & DRM_UT_STATE)) + if (drm_debug_enabled(DRM_UT_STATE)) drm_atomic_print_state(state); ret = drm_atomic_commit(state); diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index e6801db54d0f..6b14b63b8d62 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1179,7 +1179,7 @@ static int drm_dp_mst_wait_tx_reply(struct drm_dp_mst_branch *mstb, } } out: - if (unlikely(ret == -EIO && drm_debug & DRM_UT_DP)) { + if (unlikely(ret == -EIO) && drm_debug_enabled(DRM_UT_DP)) { struct drm_printer p = drm_debug_printer(DBG_PREFIX); drm_dp_mst_dump_sideband_msg_tx(&p, txmsg); @@ -2322,7 +2322,7 @@ static int process_single_tx_qlock(struct drm_dp_mst_topology_mgr *mgr, idx += tosend + 1; ret = drm_dp_send_sideband_msg(mgr, up, chunk, idx); - if (unlikely(ret && drm_debug & DRM_UT_DP)) { + if (unlikely(ret) && drm_debug_enabled(DRM_UT_DP)) { struct drm_printer p = drm_debug_printer(DBG_PREFIX); drm_printf(&p, "sideband msg failed to send\n"); @@ -2389,7 +2389,7 @@ static void drm_dp_queue_down_tx(struct drm_dp_mst_topology_mgr *mgr, mutex_lock(&mgr->qlock); list_add_tail(&txmsg->next, &mgr->tx_msg_downq); - if (unlikely(drm_debug & DRM_UT_DP)) { + if (drm_debug_enabled(DRM_UT_DP)) { struct drm_printer p = drm_debug_printer(DBG_PREFIX); drm_dp_mst_dump_sideband_msg_tx(&p, txmsg); diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 3c9703b08491..0552175313cb 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1651,7 +1651,7 @@ static void connector_bad_edid(struct drm_connector *connector, { int i; - if (connector->bad_edid_counter++ && !(drm_debug & DRM_UT_KMS)) + if (connector->bad_edid_counter++ && !drm_debug_enabled(DRM_UT_KMS)) return; dev_warn(connector->dev->dev, diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c index d38b3b255926..37d8ba3ddb46 100644 --- a/drivers/gpu/drm/drm_edid_load.c +++ b/drivers/gpu/drm/drm_edid_load.c @@ -175,7 +175,7 @@ static void *edid_load(struct drm_connector *connector, const char *name, u8 *edid; int fwsize, builtin; int i, valid_extensions = 0; - bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS); + bool print_bad_edid = !connector->bad_edid_counter || drm_debug_enabled(DRM_UT_KMS); builtin = match_string(generic_edid_name, GENERIC_EDIDS, name); if (builtin >= 0) { diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c index f8154316a3b0..ccfb5b33c5e3 100644 --- a/drivers/gpu/drm/drm_mipi_dbi.c +++ b/drivers/gpu/drm/drm_mipi_dbi.c @@ -783,7 +783,7 @@ static int mipi_dbi_spi1e_transfer(struct mipi_dbi *dbi, int dc, int i, ret; u8 *dst; - if (drm_debug & DRM_UT_DRIVER) + if (drm_debug_enabled(DRM_UT_DRIVER)) pr_debug("[drm:%s] dc=%d, max_chunk=%zu, transfers:\n", __func__, dc, max_chunk); @@ -907,7 +907,7 @@ static int mipi_dbi_spi1_transfer(struct mipi_dbi *dbi, int dc, max_chunk = dbi->tx_buf9_len; dst16 = dbi->tx_buf9; - if (drm_debug & DRM_UT_DRIVER) + if (drm_debug_enabled(DRM_UT_DRIVER)) pr_debug("[drm:%s] dc=%d, max_chunk=%zu, transfers:\n", __func__, dc, max_chunk); diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c index 1ade3a917c10..9a25d73c155c 100644 --- a/drivers/gpu/drm/drm_print.c +++ b/drivers/gpu/drm/drm_print.c @@ -262,7 +262,7 @@ void drm_dev_dbg(const struct device *dev, unsigned int category, struct va_format vaf; va_list args; - if (!(drm_debug & category)) + if (!drm_debug_enabled(category)) return; va_start(args, format); @@ -285,7 +285,7 @@ void drm_dbg(unsigned int category, const char *format, ...) struct va_format vaf; va_list args; - if (!(drm_debug & category)) + if (!drm_debug_enabled(category)) return; va_start(args, format); diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 9c6899758bc9..4f7962b6427b 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -332,7 +332,7 @@ u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc) u64 vblank; unsigned long flags; - WARN_ONCE(drm_debug & DRM_UT_VBL && !dev->driver->get_vblank_timestamp, + WARN_ONCE(drm_debug_enabled(DRM_UT_VBL) && !dev->driver->get_vblank_timestamp, "This function requires support for accurate vblank timestamps."); spin_lock_irqsave(&dev->vblank_time_lock, flags); @@ -706,7 +706,7 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, */ *vblank_time = ktime_sub_ns(etime, delta_ns); - if ((drm_debug & DRM_UT_VBL) == 0) + if (!drm_debug_enabled(DRM_UT_VBL)) return true; ts_etime = ktime_to_timespec64(etime); @@ -1352,7 +1352,7 @@ void drm_vblank_restore(struct drm_device *dev, unsigned int pipe) assert_spin_locked(&dev->vblank_time_lock); vblank = &dev->vblank[pipe]; - WARN_ONCE((drm_debug & DRM_UT_VBL) && !vblank->framedur_ns, + WARN_ONCE(drm_debug_enabled(DRM_UT_VBL) && !vblank->framedur_ns, "Cannot compute missed vblanks without frame duration\n"); framedur_ns = vblank->framedur_ns; diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h index fc9f218ab715..5b8049992c24 100644 --- a/include/drm/drm_print.h +++ b/include/drm/drm_print.h @@ -293,6 +293,11 @@ static inline struct drm_printer drm_err_printer(const char *prefix) #define DRM_UT_LEASE 0x80 #define DRM_UT_DP 0x100 +static inline bool drm_debug_enabled(unsigned int category) +{ + return unlikely(drm_debug & category); +} + __printf(3, 4) void drm_dev_printk(const struct device *dev, const char *level, const char *format, ...); -- cgit v1.2.3 From 83fa9842afe7d15b53267978f347676f9b5632d1 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Thu, 10 Oct 2019 00:41:13 +0200 Subject: drm/dp-mst: Drop connection_mutex check Private atomic objects have grown their own locking with commit b962a12050a387e4bbf3a48745afe1d29d396b0d Author: Rob Clark Date: Mon Oct 22 14:31:22 2018 +0200 drm/atomic: integrate modeset lock with private objects which means we're no longer relying on connection_mutex for mst state locking needs. Cc: Lyude Paul Cc: Sean Paul Reviewed-by: Lyude Paul Signed-off-by: Daniel Vetter Link: https://patchwork.freedesktop.org/patch/msgid/20191009224113.5432-1-daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_dp_mst_topology.c | 1 - 1 file changed, 1 deletion(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 6b14b63b8d62..9364e4f42975 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -4186,7 +4186,6 @@ struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_a { struct drm_device *dev = mgr->dev; - WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); return to_dp_mst_topology_state(drm_atomic_get_private_obj_state(state, &mgr->base)); } EXPORT_SYMBOL(drm_atomic_get_mst_topology_state); -- cgit v1.2.3 From 67c698fc5eb0f7471f28e10cd6960e5cf6426de1 Mon Sep 17 00:00:00 2001 From: Lucas De Marchi Date: Thu, 10 Oct 2019 18:09:07 -0700 Subject: drm/dp-mst: fix warning on unused var MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes: 83fa9842afe7 ("drm/dp-mst: Drop connection_mutex check") Signed-off-by: Lucas De Marchi Reviewed-by: José Roberto de Souza Signed-off-by: Daniel Vetter Link: https://patchwork.freedesktop.org/patch/msgid/20191011010907.103309-8-lucas.demarchi@intel.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 2 -- 1 file changed, 2 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 9364e4f42975..9cccc5e63309 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -4184,8 +4184,6 @@ EXPORT_SYMBOL(drm_dp_mst_topology_state_funcs); struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state, struct drm_dp_mst_topology_mgr *mgr) { - struct drm_device *dev = mgr->dev; - return to_dp_mst_topology_state(drm_atomic_get_private_obj_state(state, &mgr->base)); } EXPORT_SYMBOL(drm_atomic_get_mst_topology_state); -- cgit v1.2.3 From ddd9b54deebba64c37d04598c98035b5da037d38 Mon Sep 17 00:00:00 2001 From: Wambui Karuga Date: Sat, 19 Oct 2019 10:18:40 +0300 Subject: drm: remove unnecessary return variable Remove unnecessary variable `ret` in drm_dp_atomic_find_vcpi_slots() only used to hold the function return value and have the function return the value directly. Issue found by coccinelle: @@ local idexpression ret; expression e; @@ -ret = +return e; -return ret; Signed-off-by: Wambui Karuga Signed-off-by: Daniel Vetter Link: https://patchwork.freedesktop.org/patch/msgid/20191019071840.16877-1-wambui@karuga.xyz --- drivers/gpu/drm/drm_dp_mst_topology.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 9cccc5e63309..b854a422a523 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3540,7 +3540,7 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, { struct drm_dp_mst_topology_state *topology_state; struct drm_dp_vcpi_allocation *pos, *vcpi = NULL; - int prev_slots, req_slots, ret; + int prev_slots, req_slots; topology_state = drm_atomic_get_mst_topology_state(state, mgr); if (IS_ERR(topology_state)) @@ -3587,8 +3587,7 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, } vcpi->vcpi = req_slots; - ret = req_slots; - return ret; + return req_slots; } EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots); -- cgit v1.2.3 From 7cb12d48314eabdaaf30e4b3275f04811b458ed3 Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Tue, 19 Feb 2019 17:41:02 -0500 Subject: drm/dp_mst: Destroy MSTBs asynchronously MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When reprobing an MST topology during resume, we have to account for the fact that while we were suspended it's possible that mstbs may have been removed from any ports in the topology. Since iterating downwards in the topology requires that we hold &mgr->lock, destroying MSTBs from this context would result in attempting to lock &mgr->lock a second time and deadlocking. So, fix this by first moving destruction of MSTBs into destroy_connector_work, then rename destroy_connector_work and friends to reflect that they now destroy both ports and mstbs. Note that even though this means that MSTBs will still be accessible for a short period of time between their removal from the topology and delayed destruction, we are still protected against referencing a MSTB with a refcount of 0 since we use kref_get_unless_zero() in most places. Changes since v1: * s/destroy_connector_list/destroy_port_list/ s/connector_destroy_lock/delayed_destroy_lock/ s/connector_destroy_work/delayed_destroy_work/ s/drm_dp_finish_destroy_branch_device/drm_dp_delayed_destroy_mstb/ s/drm_dp_finish_destroy_port/drm_dp_delayed_destroy_port/ - danvet * Use two loops in drm_dp_delayed_destroy_work() - danvet * Better explain why we need to do this - danvet * Use cancel_work_sync() instead of flush_work() - flush_work() doesn't account for work requeing Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Cc: Daniel Vetter Reviewed-by: Sean Paul Signed-off-by: Lyude Paul Link: https://patchwork.freedesktop.org/patch/msgid/20191022023641.8026-2-lyude@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 164 ++++++++++++++++++++++------------ include/drm/drm_dp_mst_helper.h | 26 ++++-- 2 files changed, 128 insertions(+), 62 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 6b14b63b8d62..bbda316f5a0d 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1398,34 +1398,17 @@ static void drm_dp_destroy_mst_branch_device(struct kref *kref) struct drm_dp_mst_branch *mstb = container_of(kref, struct drm_dp_mst_branch, topology_kref); struct drm_dp_mst_topology_mgr *mgr = mstb->mgr; - struct drm_dp_mst_port *port, *tmp; - bool wake_tx = false; - mutex_lock(&mgr->lock); - list_for_each_entry_safe(port, tmp, &mstb->ports, next) { - list_del(&port->next); - drm_dp_mst_topology_put_port(port); - } - mutex_unlock(&mgr->lock); - - /* drop any tx slots msg */ - mutex_lock(&mstb->mgr->qlock); - if (mstb->tx_slots[0]) { - mstb->tx_slots[0]->state = DRM_DP_SIDEBAND_TX_TIMEOUT; - mstb->tx_slots[0] = NULL; - wake_tx = true; - } - if (mstb->tx_slots[1]) { - mstb->tx_slots[1]->state = DRM_DP_SIDEBAND_TX_TIMEOUT; - mstb->tx_slots[1] = NULL; - wake_tx = true; - } - mutex_unlock(&mstb->mgr->qlock); + INIT_LIST_HEAD(&mstb->destroy_next); - if (wake_tx) - wake_up_all(&mstb->mgr->tx_waitq); - - drm_dp_mst_put_mstb_malloc(mstb); + /* + * This can get called under mgr->mutex, so we need to perform the + * actual destruction of the mstb in another worker + */ + mutex_lock(&mgr->delayed_destroy_lock); + list_add(&mstb->destroy_next, &mgr->destroy_branch_device_list); + mutex_unlock(&mgr->delayed_destroy_lock); + schedule_work(&mgr->delayed_destroy_work); } /** @@ -1540,10 +1523,10 @@ static void drm_dp_destroy_port(struct kref *kref) * we might be holding the mode_config.mutex * from an EDID retrieval */ - mutex_lock(&mgr->destroy_connector_lock); - list_add(&port->next, &mgr->destroy_connector_list); - mutex_unlock(&mgr->destroy_connector_lock); - schedule_work(&mgr->destroy_connector_work); + mutex_lock(&mgr->delayed_destroy_lock); + list_add(&port->next, &mgr->destroy_port_list); + mutex_unlock(&mgr->delayed_destroy_lock); + schedule_work(&mgr->delayed_destroy_work); return; } /* no need to clean up vcpi @@ -3085,7 +3068,7 @@ void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr) DP_MST_EN | DP_UPSTREAM_IS_SRC); mutex_unlock(&mgr->lock); flush_work(&mgr->work); - flush_work(&mgr->destroy_connector_work); + flush_work(&mgr->delayed_destroy_work); } EXPORT_SYMBOL(drm_dp_mst_topology_mgr_suspend); @@ -3995,34 +3978,104 @@ static void drm_dp_tx_work(struct work_struct *work) mutex_unlock(&mgr->qlock); } -static void drm_dp_destroy_connector_work(struct work_struct *work) +static inline void +drm_dp_delayed_destroy_port(struct drm_dp_mst_port *port) { - struct drm_dp_mst_topology_mgr *mgr = container_of(work, struct drm_dp_mst_topology_mgr, destroy_connector_work); - struct drm_dp_mst_port *port; - bool send_hotplug = false; + port->mgr->cbs->destroy_connector(port->mgr, port->connector); + + drm_dp_port_teardown_pdt(port, port->pdt); + port->pdt = DP_PEER_DEVICE_NONE; + + drm_dp_mst_put_port_malloc(port); +} + +static inline void +drm_dp_delayed_destroy_mstb(struct drm_dp_mst_branch *mstb) +{ + struct drm_dp_mst_topology_mgr *mgr = mstb->mgr; + struct drm_dp_mst_port *port, *tmp; + bool wake_tx = false; + + mutex_lock(&mgr->lock); + list_for_each_entry_safe(port, tmp, &mstb->ports, next) { + list_del(&port->next); + drm_dp_mst_topology_put_port(port); + } + mutex_unlock(&mgr->lock); + + /* drop any tx slots msg */ + mutex_lock(&mstb->mgr->qlock); + if (mstb->tx_slots[0]) { + mstb->tx_slots[0]->state = DRM_DP_SIDEBAND_TX_TIMEOUT; + mstb->tx_slots[0] = NULL; + wake_tx = true; + } + if (mstb->tx_slots[1]) { + mstb->tx_slots[1]->state = DRM_DP_SIDEBAND_TX_TIMEOUT; + mstb->tx_slots[1] = NULL; + wake_tx = true; + } + mutex_unlock(&mstb->mgr->qlock); + + if (wake_tx) + wake_up_all(&mstb->mgr->tx_waitq); + + drm_dp_mst_put_mstb_malloc(mstb); +} + +static void drm_dp_delayed_destroy_work(struct work_struct *work) +{ + struct drm_dp_mst_topology_mgr *mgr = + container_of(work, struct drm_dp_mst_topology_mgr, + delayed_destroy_work); + bool send_hotplug = false, go_again; + /* * Not a regular list traverse as we have to drop the destroy - * connector lock before destroying the connector, to avoid AB->BA + * connector lock before destroying the mstb/port, to avoid AB->BA * ordering between this lock and the config mutex. */ - for (;;) { - mutex_lock(&mgr->destroy_connector_lock); - port = list_first_entry_or_null(&mgr->destroy_connector_list, struct drm_dp_mst_port, next); - if (!port) { - mutex_unlock(&mgr->destroy_connector_lock); - break; + do { + go_again = false; + + for (;;) { + struct drm_dp_mst_branch *mstb; + + mutex_lock(&mgr->delayed_destroy_lock); + mstb = list_first_entry_or_null(&mgr->destroy_branch_device_list, + struct drm_dp_mst_branch, + destroy_next); + if (mstb) + list_del(&mstb->destroy_next); + mutex_unlock(&mgr->delayed_destroy_lock); + + if (!mstb) + break; + + drm_dp_delayed_destroy_mstb(mstb); + go_again = true; } - list_del(&port->next); - mutex_unlock(&mgr->destroy_connector_lock); - mgr->cbs->destroy_connector(mgr, port->connector); + for (;;) { + struct drm_dp_mst_port *port; - drm_dp_port_teardown_pdt(port, port->pdt); - port->pdt = DP_PEER_DEVICE_NONE; + mutex_lock(&mgr->delayed_destroy_lock); + port = list_first_entry_or_null(&mgr->destroy_port_list, + struct drm_dp_mst_port, + next); + if (port) + list_del(&port->next); + mutex_unlock(&mgr->delayed_destroy_lock); + + if (!port) + break; + + drm_dp_delayed_destroy_port(port); + send_hotplug = true; + go_again = true; + } + } while (go_again); - drm_dp_mst_put_port_malloc(port); - send_hotplug = true; - } if (send_hotplug) drm_kms_helper_hotplug_event(mgr->dev); } @@ -4212,12 +4265,13 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr, mutex_init(&mgr->lock); mutex_init(&mgr->qlock); mutex_init(&mgr->payload_lock); - mutex_init(&mgr->destroy_connector_lock); + mutex_init(&mgr->delayed_destroy_lock); INIT_LIST_HEAD(&mgr->tx_msg_downq); - INIT_LIST_HEAD(&mgr->destroy_connector_list); + INIT_LIST_HEAD(&mgr->destroy_port_list); + INIT_LIST_HEAD(&mgr->destroy_branch_device_list); INIT_WORK(&mgr->work, drm_dp_mst_link_probe_work); INIT_WORK(&mgr->tx_work, drm_dp_tx_work); - INIT_WORK(&mgr->destroy_connector_work, drm_dp_destroy_connector_work); + INIT_WORK(&mgr->delayed_destroy_work, drm_dp_delayed_destroy_work); init_waitqueue_head(&mgr->tx_waitq); mgr->dev = dev; mgr->aux = aux; @@ -4258,7 +4312,7 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr) { drm_dp_mst_topology_mgr_set_mst(mgr, false); flush_work(&mgr->work); - flush_work(&mgr->destroy_connector_work); + cancel_work_sync(&mgr->delayed_destroy_work); mutex_lock(&mgr->payload_lock); kfree(mgr->payloads); mgr->payloads = NULL; @@ -4270,7 +4324,7 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr) drm_atomic_private_obj_fini(&mgr->base); mgr->funcs = NULL; - mutex_destroy(&mgr->destroy_connector_lock); + mutex_destroy(&mgr->delayed_destroy_lock); mutex_destroy(&mgr->payload_lock); mutex_destroy(&mgr->qlock); mutex_destroy(&mgr->lock); diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index 4a25e0577ae0..b2160c366fb7 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -143,6 +143,12 @@ struct drm_dp_mst_branch { */ struct kref malloc_kref; + /** + * @destroy_next: linked-list entry used by + * drm_dp_delayed_destroy_work() + */ + struct list_head destroy_next; + u8 rad[8]; u8 lct; int num_ports; @@ -571,18 +577,24 @@ struct drm_dp_mst_topology_mgr { struct work_struct tx_work; /** - * @destroy_connector_list: List of to be destroyed connectors. + * @destroy_port_list: List of to be destroyed connectors. + */ + struct list_head destroy_port_list; + /** + * @destroy_branch_device_list: List of to be destroyed branch + * devices. */ - struct list_head destroy_connector_list; + struct list_head destroy_branch_device_list; /** - * @destroy_connector_lock: Protects @connector_list. + * @delayed_destroy_lock: Protects @destroy_port_list and + * @destroy_branch_device_list. */ - struct mutex destroy_connector_lock; + struct mutex delayed_destroy_lock; /** - * @destroy_connector_work: Work item to destroy connectors. Needed to - * avoid locking inversion. + * @delayed_destroy_work: Work item to destroy MST port and branch + * devices, needed to avoid locking inversion. */ - struct work_struct destroy_connector_work; + struct work_struct delayed_destroy_work; }; int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr, -- cgit v1.2.3 From d29333cf5cd7594bd9a6bac10cbb4ec7079dc9a3 Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Sun, 10 Mar 2019 18:59:10 -0400 Subject: drm/dp_mst: Remove PDT teardown in drm_dp_destroy_port() and refactor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will allow us to add some locking for port->* members, in particular the PDT and ->connector, which can't be done from drm_dp_destroy_port() since we don't know what locks the caller might be holding. Note that we already do this in delayed_destroy_work (renamed from destroy_connector_work in this patch) for ports, we're just making it so mstbs are also destroyed in this worker. Changes since v2: * Clarify commit message Changes since v4: * Clarify commit message more Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Cc: Daniel Vetter Reviewed-by: Sean Paul Signed-off-by: Lyude Paul Link: https://patchwork.freedesktop.org/patch/msgid/20191022023641.8026-3-lyude@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 40 ++++++++++++++--------------------- 1 file changed, 16 insertions(+), 24 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index bbda316f5a0d..82d6c0f9395d 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1510,31 +1510,22 @@ static void drm_dp_destroy_port(struct kref *kref) container_of(kref, struct drm_dp_mst_port, topology_kref); struct drm_dp_mst_topology_mgr *mgr = port->mgr; - if (!port->input) { - kfree(port->cached_edid); + /* There's nothing that needs locking to destroy an input port yet */ + if (port->input) { + drm_dp_mst_put_port_malloc(port); + return; + } - /* - * The only time we don't have a connector - * on an output port is if the connector init - * fails. - */ - if (port->connector) { - /* we can't destroy the connector here, as - * we might be holding the mode_config.mutex - * from an EDID retrieval */ + kfree(port->cached_edid); - mutex_lock(&mgr->delayed_destroy_lock); - list_add(&port->next, &mgr->destroy_port_list); - mutex_unlock(&mgr->delayed_destroy_lock); - schedule_work(&mgr->delayed_destroy_work); - return; - } - /* no need to clean up vcpi - * as if we have no connector we never setup a vcpi */ - drm_dp_port_teardown_pdt(port, port->pdt); - port->pdt = DP_PEER_DEVICE_NONE; - } - drm_dp_mst_put_port_malloc(port); + /* + * we can't destroy the connector here, as we might be holding the + * mode_config.mutex from an EDID retrieval + */ + mutex_lock(&mgr->delayed_destroy_lock); + list_add(&port->next, &mgr->destroy_port_list); + mutex_unlock(&mgr->delayed_destroy_lock); + schedule_work(&mgr->delayed_destroy_work); } /** @@ -3981,7 +3972,8 @@ static void drm_dp_tx_work(struct work_struct *work) static inline void drm_dp_delayed_destroy_port(struct drm_dp_mst_port *port) { - port->mgr->cbs->destroy_connector(port->mgr, port->connector); + if (port->connector) + port->mgr->cbs->destroy_connector(port->mgr, port->connector); drm_dp_port_teardown_pdt(port, port->pdt); port->pdt = DP_PEER_DEVICE_NONE; -- cgit v1.2.3 From c485e2c97dae4e13f239ccad455070e99213dd4b Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Wed, 13 Mar 2019 13:51:41 -0400 Subject: drm/dp_mst: Refactor pdt setup/teardown, add more locking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since we're going to be implementing suspend/resume reprobing very soon, we need to make sure we are extra careful to ensure that our locking actually protects the topology state where we expect it to. Turns out this isn't the case with drm_dp_port_setup_pdt() and drm_dp_port_teardown_pdt(), both of which change port->mstb without grabbing &mgr->lock. Additionally, since most callers of these functions are just using it to teardown the port's previous PDT and setup a new one we can simplify things a bit and combine drm_dp_port_setup_pdt() and drm_dp_port_teardown_pdt() into a single function: drm_dp_port_set_pdt(). This function also handles actually ensuring that we grab the correct locks when we need to modify port->mstb. Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Cc: Daniel Vetter Reviewed-by: Sean Paul Signed-off-by: Lyude Paul Link: https://patchwork.freedesktop.org/patch/msgid/20191022023641.8026-4-lyude@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 181 ++++++++++++++++++++-------------- include/drm/drm_dp_mst_helper.h | 6 +- 2 files changed, 110 insertions(+), 77 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 82d6c0f9395d..f4390bba5372 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1486,24 +1486,6 @@ drm_dp_mst_topology_put_mstb(struct drm_dp_mst_branch *mstb) kref_put(&mstb->topology_kref, drm_dp_destroy_mst_branch_device); } -static void drm_dp_port_teardown_pdt(struct drm_dp_mst_port *port, int old_pdt) -{ - struct drm_dp_mst_branch *mstb; - - switch (old_pdt) { - case DP_PEER_DEVICE_DP_LEGACY_CONV: - case DP_PEER_DEVICE_SST_SINK: - /* remove i2c over sideband */ - drm_dp_mst_unregister_i2c_bus(&port->aux); - break; - case DP_PEER_DEVICE_MST_BRANCHING: - mstb = port->mstb; - port->mstb = NULL; - drm_dp_mst_topology_put_mstb(mstb); - break; - } -} - static void drm_dp_destroy_port(struct kref *kref) { struct drm_dp_mst_port *port = @@ -1713,38 +1695,79 @@ static u8 drm_dp_calculate_rad(struct drm_dp_mst_port *port, return parent_lct + 1; } -/* - * return sends link address for new mstb - */ -static bool drm_dp_port_setup_pdt(struct drm_dp_mst_port *port) +static int drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt) { - int ret; - u8 rad[6], lct; - bool send_link = false; + struct drm_dp_mst_topology_mgr *mgr = port->mgr; + struct drm_dp_mst_branch *mstb; + u8 rad[8], lct; + int ret = 0; + + if (port->pdt == new_pdt) + return 0; + + /* Teardown the old pdt, if there is one */ + switch (port->pdt) { + case DP_PEER_DEVICE_DP_LEGACY_CONV: + case DP_PEER_DEVICE_SST_SINK: + /* + * If the new PDT would also have an i2c bus, don't bother + * with reregistering it + */ + if (new_pdt == DP_PEER_DEVICE_DP_LEGACY_CONV || + new_pdt == DP_PEER_DEVICE_SST_SINK) { + port->pdt = new_pdt; + return 0; + } + + /* remove i2c over sideband */ + drm_dp_mst_unregister_i2c_bus(&port->aux); + break; + case DP_PEER_DEVICE_MST_BRANCHING: + mutex_lock(&mgr->lock); + drm_dp_mst_topology_put_mstb(port->mstb); + port->mstb = NULL; + mutex_unlock(&mgr->lock); + break; + } + + port->pdt = new_pdt; switch (port->pdt) { case DP_PEER_DEVICE_DP_LEGACY_CONV: case DP_PEER_DEVICE_SST_SINK: /* add i2c over sideband */ ret = drm_dp_mst_register_i2c_bus(&port->aux); break; + case DP_PEER_DEVICE_MST_BRANCHING: lct = drm_dp_calculate_rad(port, rad); + mstb = drm_dp_add_mst_branch_device(lct, rad); + if (!mstb) { + ret = -ENOMEM; + DRM_ERROR("Failed to create MSTB for port %p", port); + goto out; + } - port->mstb = drm_dp_add_mst_branch_device(lct, rad); - if (port->mstb) { - port->mstb->mgr = port->mgr; - port->mstb->port_parent = port; - /* - * Make sure this port's memory allocation stays - * around until its child MSTB releases it - */ - drm_dp_mst_get_port_malloc(port); + mutex_lock(&mgr->lock); + port->mstb = mstb; + mstb->mgr = port->mgr; + mstb->port_parent = port; - send_link = true; - } + /* + * Make sure this port's memory allocation stays + * around until its child MSTB releases it + */ + drm_dp_mst_get_port_malloc(port); + mutex_unlock(&mgr->lock); + + /* And make sure we send a link address for this */ + ret = 1; break; } - return send_link; + +out: + if (ret < 0) + port->pdt = DP_PEER_DEVICE_NONE; + return ret; } /** @@ -1881,10 +1904,9 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, struct drm_device *dev, struct drm_dp_link_addr_reply_port *port_msg) { + struct drm_dp_mst_topology_mgr *mgr = mstb->mgr; struct drm_dp_mst_port *port; - bool ret; bool created = false; - int old_pdt = 0; int old_ddps = 0; port = drm_dp_get_port(mstb, port_msg->port_number); @@ -1896,7 +1918,7 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, kref_init(&port->malloc_kref); port->parent = mstb; port->port_num = port_msg->port_number; - port->mgr = mstb->mgr; + port->mgr = mgr; port->aux.name = "DPMST"; port->aux.dev = dev->dev; port->aux.is_remote = true; @@ -1909,11 +1931,9 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, created = true; } else { - old_pdt = port->pdt; old_ddps = port->ddps; } - port->pdt = port_msg->peer_device_type; port->input = port_msg->input_port; port->mcs = port_msg->mcs; port->ddps = port_msg->ddps; @@ -1925,29 +1945,33 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, /* manage mstb port lists with mgr lock - take a reference for this list */ if (created) { - mutex_lock(&mstb->mgr->lock); + mutex_lock(&mgr->lock); drm_dp_mst_topology_get_port(port); list_add(&port->next, &mstb->ports); - mutex_unlock(&mstb->mgr->lock); + mutex_unlock(&mgr->lock); } if (old_ddps != port->ddps) { if (port->ddps) { if (!port->input) { - drm_dp_send_enum_path_resources(mstb->mgr, - mstb, port); + drm_dp_send_enum_path_resources(mgr, mstb, + port); } } else { port->available_pbn = 0; } } - if (old_pdt != port->pdt && !port->input) { - drm_dp_port_teardown_pdt(port, old_pdt); - - ret = drm_dp_port_setup_pdt(port); - if (ret == true) - drm_dp_send_link_address(mstb->mgr, port->mstb); + if (!port->input) { + int ret = drm_dp_port_set_pdt(port, + port_msg->peer_device_type); + if (ret == 1) { + drm_dp_send_link_address(mgr, port->mstb); + } else if (ret < 0) { + DRM_ERROR("Failed to change PDT on port %p: %d\n", + port, ret); + goto fail; + } } if (created && !port->input) { @@ -1955,18 +1979,11 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath)); - port->connector = (*mstb->mgr->cbs->add_connector)(mstb->mgr, - port, - proppath); - if (!port->connector) { - /* remove it from the port list */ - mutex_lock(&mstb->mgr->lock); - list_del(&port->next); - mutex_unlock(&mstb->mgr->lock); - /* drop port list reference */ - drm_dp_mst_topology_put_port(port); - goto out; - } + port->connector = (*mgr->cbs->add_connector)(mgr, port, + proppath); + if (!port->connector) + goto fail; + if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV || port->pdt == DP_PEER_DEVICE_SST_SINK) && port->port_num >= DP_MST_LOGICAL_PORT_0) { @@ -1974,12 +1991,24 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, &port->aux.ddc); drm_connector_set_tile_property(port->connector); } - (*mstb->mgr->cbs->register_connector)(port->connector); + + (*mgr->cbs->register_connector)(port->connector); } -out: /* put reference to this port */ drm_dp_mst_topology_put_port(port); + return; + +fail: + /* Remove it from the port list */ + mutex_lock(&mgr->lock); + list_del(&port->next); + mutex_unlock(&mgr->lock); + + /* Drop the port list reference */ + drm_dp_mst_topology_put_port(port); + /* And now drop our reference */ + drm_dp_mst_topology_put_port(port); } static void @@ -1987,16 +2016,14 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb, struct drm_dp_connection_status_notify *conn_stat) { struct drm_dp_mst_port *port; - int old_pdt; int old_ddps; bool dowork = false; + port = drm_dp_get_port(mstb, conn_stat->port_number); if (!port) return; old_ddps = port->ddps; - old_pdt = port->pdt; - port->pdt = conn_stat->peer_device_type; port->mcs = conn_stat->message_capability_status; port->ldps = conn_stat->legacy_device_plug_status; port->ddps = conn_stat->displayport_device_plug_status; @@ -2008,11 +2035,17 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb, port->available_pbn = 0; } } - if (old_pdt != port->pdt && !port->input) { - drm_dp_port_teardown_pdt(port, old_pdt); - if (drm_dp_port_setup_pdt(port)) + if (!port->input) { + int ret = drm_dp_port_set_pdt(port, + conn_stat->peer_device_type); + if (ret == 1) { dowork = true; + } else if (ret < 0) { + DRM_ERROR("Failed to change PDT for port %p: %d\n", + port, ret); + dowork = false; + } } drm_dp_mst_topology_put_port(port); @@ -3975,9 +4008,7 @@ drm_dp_delayed_destroy_port(struct drm_dp_mst_port *port) if (port->connector) port->mgr->cbs->destroy_connector(port->mgr, port->connector); - drm_dp_port_teardown_pdt(port, port->pdt); - port->pdt = DP_PEER_DEVICE_NONE; - + drm_dp_port_set_pdt(port, DP_PEER_DEVICE_NONE); drm_dp_mst_put_port_malloc(port); } diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index b2160c366fb7..8ba2a01324bb 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -55,8 +55,10 @@ struct drm_dp_vcpi { * @num_sdp_stream_sinks: Number of stream sinks * @available_pbn: Available bandwidth for this port. * @next: link to next port on this branch device - * @mstb: branch device attach below this port - * @aux: i2c aux transport to talk to device connected to this port. + * @mstb: branch device on this port, protected by + * &drm_dp_mst_topology_mgr.lock + * @aux: i2c aux transport to talk to device connected to this port, protected + * by &drm_dp_mst_topology_mgr.lock * @parent: branch device parent of this port * @vcpi: Virtual Channel Payload info for this port. * @connector: DRM connector this port is connected to. -- cgit v1.2.3 From 9408cc94eb041d0c2f9f00189a613b94c0449450 Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Mon, 17 Jun 2019 16:37:18 -0400 Subject: drm/dp_mst: Handle UP requests asynchronously MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Once upon a time, hotplugging devices on MST branches actually worked in DRM. Now, it only works in amdgpu (likely because of how it's hotplug handlers are implemented). On both i915 and nouveau, hotplug notifications from MST branches are noticed - but trying to respond to them causes messaging timeouts and causes the whole topology state to go out of sync with reality, usually resulting in the user needing to replug the entire topology in hopes that it actually fixes things. The reason for this is because the way we currently handle UP requests in MST is completely bogus. drm_dp_mst_handle_up_req() is called from drm_dp_mst_hpd_irq(), which is usually called from the driver's hotplug handler. Because we handle sending the hotplug event from this function, we actually cause the driver's hotplug handler (and in turn, all sideband transactions) to block on drm_device->mode_config.connection_mutex. This makes it impossible to send any sideband messages from the driver's connector probing functions, resulting in the aforementioned sideband message timeout. There's even more problems with this beyond breaking hotplugging on MST branch devices. It also makes it almost impossible to protect drm_dp_mst_port struct members under a lock because we then have to worry about dealing with all of the lock dependency issues that ensue. So, let's finally actually fix this issue by handling the processing of up requests asyncronously. This way we can send sideband messages from most contexts without having to deal with getting blocked if we hold connection_mutex. This also fixes MST branch device hotplugging on i915, finally! Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Cc: Daniel Vetter Reviewed-by: Sean Paul Signed-off-by: Lyude Paul Link: https://patchwork.freedesktop.org/patch/msgid/20191022023641.8026-5-lyude@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 146 ++++++++++++++++++++++++---------- include/drm/drm_dp_mst_helper.h | 16 ++++ 2 files changed, 122 insertions(+), 40 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index f4390bba5372..1aaeeb884b9c 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -45,6 +45,12 @@ * protocol. The helpers contain a topology manager and bandwidth manager. * The helpers encapsulate the sending and received of sideband msgs. */ +struct drm_dp_pending_up_req { + struct drm_dp_sideband_msg_hdr hdr; + struct drm_dp_sideband_msg_req_body msg; + struct list_head next; +}; + static bool dump_dp_payload_table(struct drm_dp_mst_topology_mgr *mgr, char *buf); @@ -3091,6 +3097,7 @@ void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr) drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, DP_MST_EN | DP_UPSTREAM_IS_SRC); mutex_unlock(&mgr->lock); + flush_work(&mgr->up_req_work); flush_work(&mgr->work); flush_work(&mgr->delayed_destroy_work); } @@ -3263,12 +3270,70 @@ clear_down_rep_recv: return 0; } +static inline void +drm_dp_mst_process_up_req(struct drm_dp_mst_topology_mgr *mgr, + struct drm_dp_pending_up_req *up_req) +{ + struct drm_dp_mst_branch *mstb = NULL; + struct drm_dp_sideband_msg_req_body *msg = &up_req->msg; + struct drm_dp_sideband_msg_hdr *hdr = &up_req->hdr; + + if (hdr->broadcast) { + const u8 *guid = NULL; + + if (msg->req_type == DP_CONNECTION_STATUS_NOTIFY) + guid = msg->u.conn_stat.guid; + else if (msg->req_type == DP_RESOURCE_STATUS_NOTIFY) + guid = msg->u.resource_stat.guid; + + mstb = drm_dp_get_mst_branch_device_by_guid(mgr, guid); + } else { + mstb = drm_dp_get_mst_branch_device(mgr, hdr->lct, hdr->rad); + } + + if (!mstb) { + DRM_DEBUG_KMS("Got MST reply from unknown device %d\n", + hdr->lct); + return; + } + + /* TODO: Add missing handler for DP_RESOURCE_STATUS_NOTIFY events */ + if (msg->req_type == DP_CONNECTION_STATUS_NOTIFY) { + drm_dp_mst_handle_conn_stat(mstb, &msg->u.conn_stat); + drm_kms_helper_hotplug_event(mgr->dev); + } + + drm_dp_mst_topology_put_mstb(mstb); +} + +static void drm_dp_mst_up_req_work(struct work_struct *work) +{ + struct drm_dp_mst_topology_mgr *mgr = + container_of(work, struct drm_dp_mst_topology_mgr, + up_req_work); + struct drm_dp_pending_up_req *up_req; + + while (true) { + mutex_lock(&mgr->up_req_lock); + up_req = list_first_entry_or_null(&mgr->up_req_list, + struct drm_dp_pending_up_req, + next); + if (up_req) + list_del(&up_req->next); + mutex_unlock(&mgr->up_req_lock); + + if (!up_req) + break; + + drm_dp_mst_process_up_req(mgr, up_req); + kfree(up_req); + } +} + static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr) { - struct drm_dp_sideband_msg_req_body msg; struct drm_dp_sideband_msg_hdr *hdr = &mgr->up_req_recv.initial_hdr; - struct drm_dp_mst_branch *mstb = NULL; - const u8 *guid; + struct drm_dp_pending_up_req *up_req; bool seqno; if (!drm_dp_get_one_sb_msg(mgr, true)) @@ -3277,56 +3342,53 @@ static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr) if (!mgr->up_req_recv.have_eomt) return 0; - if (!hdr->broadcast) { - mstb = drm_dp_get_mst_branch_device(mgr, hdr->lct, hdr->rad); - if (!mstb) { - DRM_DEBUG_KMS("Got MST reply from unknown device %d\n", - hdr->lct); - goto out; - } + up_req = kzalloc(sizeof(*up_req), GFP_KERNEL); + if (!up_req) { + DRM_ERROR("Not enough memory to process MST up req\n"); + return -ENOMEM; } + INIT_LIST_HEAD(&up_req->next); seqno = hdr->seqno; - drm_dp_sideband_parse_req(&mgr->up_req_recv, &msg); + drm_dp_sideband_parse_req(&mgr->up_req_recv, &up_req->msg); - if (msg.req_type == DP_CONNECTION_STATUS_NOTIFY) - guid = msg.u.conn_stat.guid; - else if (msg.req_type == DP_RESOURCE_STATUS_NOTIFY) - guid = msg.u.resource_stat.guid; - else + if (up_req->msg.req_type != DP_CONNECTION_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); goto out; - - drm_dp_send_up_ack_reply(mgr, mgr->mst_primary, msg.req_type, seqno, - false); - - if (!mstb) { - mstb = drm_dp_get_mst_branch_device_by_guid(mgr, guid); - if (!mstb) { - DRM_DEBUG_KMS("Got MST reply from unknown device %d\n", - hdr->lct); - goto out; - } } - if (msg.req_type == DP_CONNECTION_STATUS_NOTIFY) { - drm_dp_mst_handle_conn_stat(mstb, &msg.u.conn_stat); + drm_dp_send_up_ack_reply(mgr, mgr->mst_primary, up_req->msg.req_type, + seqno, false); + + if (up_req->msg.req_type == DP_CONNECTION_STATUS_NOTIFY) { + const struct drm_dp_connection_status_notify *conn_stat = + &up_req->msg.u.conn_stat; DRM_DEBUG_KMS("Got CSN: pn: %d ldps:%d ddps: %d mcs: %d ip: %d pdt: %d\n", - msg.u.conn_stat.port_number, - msg.u.conn_stat.legacy_device_plug_status, - msg.u.conn_stat.displayport_device_plug_status, - msg.u.conn_stat.message_capability_status, - msg.u.conn_stat.input_port, - msg.u.conn_stat.peer_device_type); + conn_stat->port_number, + conn_stat->legacy_device_plug_status, + conn_stat->displayport_device_plug_status, + conn_stat->message_capability_status, + conn_stat->input_port, + conn_stat->peer_device_type); + } else if (up_req->msg.req_type == DP_RESOURCE_STATUS_NOTIFY) { + const struct drm_dp_resource_status_notify *res_stat = + &up_req->msg.u.resource_stat; - drm_kms_helper_hotplug_event(mgr->dev); - } else if (msg.req_type == DP_RESOURCE_STATUS_NOTIFY) { DRM_DEBUG_KMS("Got RSN: pn: %d avail_pbn %d\n", - msg.u.resource_stat.port_number, - msg.u.resource_stat.available_pbn); + res_stat->port_number, + res_stat->available_pbn); } - drm_dp_mst_topology_put_mstb(mstb); + up_req->hdr = *hdr; + mutex_lock(&mgr->up_req_lock); + list_add_tail(&up_req->next, &mgr->up_req_list); + mutex_unlock(&mgr->up_req_lock); + queue_work(system_long_wq, &mgr->up_req_work); + out: memset(&mgr->up_req_recv, 0, sizeof(struct drm_dp_sideband_msg_rx)); return 0; @@ -4289,12 +4351,15 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr, mutex_init(&mgr->qlock); mutex_init(&mgr->payload_lock); mutex_init(&mgr->delayed_destroy_lock); + mutex_init(&mgr->up_req_lock); INIT_LIST_HEAD(&mgr->tx_msg_downq); INIT_LIST_HEAD(&mgr->destroy_port_list); INIT_LIST_HEAD(&mgr->destroy_branch_device_list); + INIT_LIST_HEAD(&mgr->up_req_list); INIT_WORK(&mgr->work, drm_dp_mst_link_probe_work); INIT_WORK(&mgr->tx_work, drm_dp_tx_work); INIT_WORK(&mgr->delayed_destroy_work, drm_dp_delayed_destroy_work); + INIT_WORK(&mgr->up_req_work, drm_dp_mst_up_req_work); init_waitqueue_head(&mgr->tx_waitq); mgr->dev = dev; mgr->aux = aux; @@ -4351,6 +4416,7 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr) mutex_destroy(&mgr->payload_lock); mutex_destroy(&mgr->qlock); mutex_destroy(&mgr->lock); + mutex_destroy(&mgr->up_req_lock); } EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy); diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index 8ba2a01324bb..7d80c38ee00e 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -597,6 +597,22 @@ struct drm_dp_mst_topology_mgr { * devices, needed to avoid locking inversion. */ struct work_struct delayed_destroy_work; + + /** + * @up_req_list: List of pending up requests from the topology that + * need to be processed, in chronological order. + */ + struct list_head up_req_list; + /** + * @up_req_lock: Protects @up_req_list + */ + struct mutex up_req_lock; + /** + * @up_req_work: Work item to process up requests received from the + * topology. Needed to avoid blocking hotplug handling and sideband + * transmissions. + */ + struct work_struct up_req_work; }; int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr, -- cgit v1.2.3 From 14692a3637d4f1cd8ccdb8c605222037b3ac3494 Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Wed, 16 Oct 2019 16:02:59 -0400 Subject: drm/dp_mst: Add probe_lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, MST lacks locking in a lot of places that really should have some sort of locking. Hotplugging and link address code paths are some of the offenders here, as there is actually nothing preventing us from running a link address probe while at the same time handling a connection status update request - something that's likely always been possible but never seen in the wild because hotplugging has been broken for ages now (with the exception of amdgpu, for reasons I don't think are worth digging into very far). Note: I'm going to start using the term "in-memory topology layout" here to refer to drm_dp_mst_port->mstb and drm_dp_mst_branch->ports. Locking in these places is a little tougher then it looks though. Generally we protect anything having to do with the in-memory topology layout under &mgr->lock. But this becomes nearly impossible to do from the context of link address probes due to the fact that &mgr->lock is usually grabbed under random various modesetting locks, meaning that there's no way we can just invert the &mgr->lock order and keep it locked throughout the whole process of updating the topology. Luckily there are only two workers which can modify the in-memory topology layout: drm_dp_mst_up_req_work() and drm_dp_mst_link_probe_work(), meaning as long as we prevent these two workers from traveling the topology layout in parallel with the intent of updating it we don't need to worry about grabbing &mgr->lock in these workers for reads. We only need to grab &mgr->lock in these workers for writes, so that readers outside these two workers are still protected from the topology layout changing beneath them. So, add the new &mgr->probe_lock and use it in both drm_dp_mst_link_probe_work() and drm_dp_mst_up_req_work(). Additionally, add some more detailed explanations for how this locking is intended to work to drm_dp_mst_port->mstb and drm_dp_mst_branch->ports. Signed-off-by: Lyude Paul Reviewed-by: Sean Paul Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Cc: Daniel Vetter Link: https://patchwork.freedesktop.org/patch/msgid/20191022023641.8026-6-lyude@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 28 ++++++++++++++++++---------- include/drm/drm_dp_mst_helper.h | 32 ++++++++++++++++++++++++++++---- 2 files changed, 46 insertions(+), 14 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 1aaeeb884b9c..2f87dec31c3c 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -2147,37 +2147,40 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m struct drm_dp_mst_branch *mstb) { struct drm_dp_mst_port *port; - struct drm_dp_mst_branch *mstb_child; + if (!mstb->link_address_sent) drm_dp_send_link_address(mgr, mstb); list_for_each_entry(port, &mstb->ports, next) { - if (port->input) - continue; + struct drm_dp_mst_branch *mstb_child = NULL; - if (!port->ddps) + if (port->input || !port->ddps) continue; if (!port->available_pbn) drm_dp_send_enum_path_resources(mgr, mstb, port); - if (port->mstb) { + if (port->mstb) mstb_child = drm_dp_mst_topology_get_mstb_validated( mgr, port->mstb); - if (mstb_child) { - drm_dp_check_and_send_link_address(mgr, mstb_child); - drm_dp_mst_topology_put_mstb(mstb_child); - } + + if (mstb_child) { + drm_dp_check_and_send_link_address(mgr, mstb_child); + drm_dp_mst_topology_put_mstb(mstb_child); } } } static void drm_dp_mst_link_probe_work(struct work_struct *work) { - struct drm_dp_mst_topology_mgr *mgr = container_of(work, struct drm_dp_mst_topology_mgr, work); + struct drm_dp_mst_topology_mgr *mgr = + container_of(work, struct drm_dp_mst_topology_mgr, work); + struct drm_device *dev = mgr->dev; struct drm_dp_mst_branch *mstb; int ret; + mutex_lock(&mgr->probe_lock); + mutex_lock(&mgr->lock); mstb = mgr->mst_primary; if (mstb) { @@ -2190,6 +2193,7 @@ static void drm_dp_mst_link_probe_work(struct work_struct *work) drm_dp_check_and_send_link_address(mgr, mstb); drm_dp_mst_topology_put_mstb(mstb); } + mutex_unlock(&mgr->probe_lock); } static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr, @@ -3313,6 +3317,7 @@ static void drm_dp_mst_up_req_work(struct work_struct *work) up_req_work); struct drm_dp_pending_up_req *up_req; + mutex_lock(&mgr->probe_lock); while (true) { mutex_lock(&mgr->up_req_lock); up_req = list_first_entry_or_null(&mgr->up_req_list, @@ -3328,6 +3333,7 @@ static void drm_dp_mst_up_req_work(struct work_struct *work) drm_dp_mst_process_up_req(mgr, up_req); kfree(up_req); } + mutex_unlock(&mgr->probe_lock); } static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr) @@ -4352,6 +4358,7 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr, mutex_init(&mgr->payload_lock); mutex_init(&mgr->delayed_destroy_lock); mutex_init(&mgr->up_req_lock); + mutex_init(&mgr->probe_lock); INIT_LIST_HEAD(&mgr->tx_msg_downq); INIT_LIST_HEAD(&mgr->destroy_port_list); INIT_LIST_HEAD(&mgr->destroy_branch_device_list); @@ -4417,6 +4424,7 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr) mutex_destroy(&mgr->qlock); mutex_destroy(&mgr->lock); mutex_destroy(&mgr->up_req_lock); + mutex_destroy(&mgr->probe_lock); } EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy); diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index 7d80c38ee00e..bccb5514e0ef 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -55,8 +55,6 @@ struct drm_dp_vcpi { * @num_sdp_stream_sinks: Number of stream sinks * @available_pbn: Available bandwidth for this port. * @next: link to next port on this branch device - * @mstb: branch device on this port, protected by - * &drm_dp_mst_topology_mgr.lock * @aux: i2c aux transport to talk to device connected to this port, protected * by &drm_dp_mst_topology_mgr.lock * @parent: branch device parent of this port @@ -92,7 +90,17 @@ struct drm_dp_mst_port { u8 num_sdp_stream_sinks; uint16_t available_pbn; struct list_head next; - struct drm_dp_mst_branch *mstb; /* pointer to an mstb if this port has one */ + /** + * @mstb: the branch device connected to this port, if there is one. + * This should be considered protected for reading by + * &drm_dp_mst_topology_mgr.lock. There are two exceptions to this: + * &drm_dp_mst_topology_mgr.up_req_work and + * &drm_dp_mst_topology_mgr.work, which do not grab + * &drm_dp_mst_topology_mgr.lock during reads but are the only + * updaters of this list and are protected from writing concurrently + * by &drm_dp_mst_topology_mgr.probe_lock. + */ + struct drm_dp_mst_branch *mstb; struct drm_dp_aux aux; /* i2c bus for this port? */ struct drm_dp_mst_branch *parent; @@ -118,7 +126,6 @@ struct drm_dp_mst_port { * @lct: Link count total to talk to this branch device. * @num_ports: number of ports on the branch. * @msg_slots: one bit per transmitted msg slot. - * @ports: linked list of ports on this branch. * @port_parent: pointer to the port parent, NULL if toplevel. * @mgr: topology manager for this branch device. * @tx_slots: transmission slots for this device. @@ -156,6 +163,16 @@ struct drm_dp_mst_branch { int num_ports; int msg_slots; + /** + * @ports: the list of ports on this branch device. This should be + * considered protected for reading by &drm_dp_mst_topology_mgr.lock. + * There are two exceptions to this: + * &drm_dp_mst_topology_mgr.up_req_work and + * &drm_dp_mst_topology_mgr.work, which do not grab + * &drm_dp_mst_topology_mgr.lock during reads but are the only + * updaters of this list and are protected from updating the list + * concurrently by @drm_dp_mst_topology_mgr.probe_lock + */ struct list_head ports; /* list of tx ops queue for this port */ @@ -502,6 +519,13 @@ struct drm_dp_mst_topology_mgr { */ struct mutex lock; + /** + * @probe_lock: Prevents @work and @up_req_work, the only writers of + * &drm_dp_mst_port.mstb and &drm_dp_mst_branch.ports, from racing + * while they update the topology. + */ + struct mutex probe_lock; + /** * @mst_state: If this manager is enabled for an MST capable port. False * if no MST sink/branch devices is connected. -- cgit v1.2.3 From 3f9b3f02dda501ea1889d773d547dcff12a3f7bb Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Mon, 17 Jun 2019 17:59:29 -0400 Subject: drm/dp_mst: Protect drm_dp_mst_port members with locking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a complicated one. Essentially, there's currently a problem in the MST core that hasn't really caused any issues that we're aware of (emphasis on "that we're aware of"): locking. When we go through and probe the link addresses and path resources in a topology, we hold no locks when updating ports with said information. The members I'm referring to in particular are: - ldps - ddps - mcs - pdt - dpcd_rev - num_sdp_streams - num_sdp_stream_sinks - available_pbn - input - connector Now that we're handling UP requests asynchronously and will be using some of the struct members mentioned above in atomic modesetting in the future for features such as PBN validation, this is going to become a lot more important. As well, the next few commits that prepare us for and introduce suspend/resume reprobing will also need clear locking in order to prevent from additional racing hilarities that we never could have hit in the past. So, let's solve this issue by using &mgr->base.lock, the modesetting lock which currently only protects &mgr->base.state. This works perfectly because it allows us to avoid blocking connection_mutex unnecessarily, and we can grab this in connector detection paths since it's a ww mutex. We start by having drm_dp_mst_handle_up_req() hold this when updating ports. For drm_dp_mst_handle_link_address_port() things are a bit more complicated. As I've learned the hard way, we can grab &mgr->lock.base for everything except for port->connector. See, our normal driver probing paths end up generating this rather obvious lockdep chain: &drm->mode_config.mutex -> crtc_ww_class_mutex/crtc_ww_class_acquire -> &connector->mutex However, sysfs grabs &drm->mode_config.mutex in order to protect itself from connector state changing under it. Because this entails grabbing kn->count, e.g. the lock that the kernel provides for protecting sysfs contexts, we end up grabbing kn->count followed by &drm->mode_config.mutex. This ends up creating an extremely rude chain: &kn->count -> &drm->mode_config.mutex -> crtc_ww_class_mutex/crtc_ww_class_acquire -> &connector->mutex I mean, look at that thing! It's just evil!!! This gross thing ends up making any calls to drm_connector_register()/drm_connector_unregister() impossible when holding any kind of modesetting lock. This is annoying because ideally, we always want to ensure that drm_dp_mst_port->connector never changes when doing an atomic commit or check that would affect the atomic topology state so that it can reliably and easily be used from future DRM DP MST helpers to assist with tasks such as scanning through the current VCPI allocations and adding connectors which need to have their allocations updated in response to a bandwidth change or the like. Being able to hold &mgr->base.lock throughout the entire link probe process would have been _great_, since we could prevent userspace from ever seeing any states in-between individual port changes and as a result likely end up with a much faster probe and more consistent results from said probes. But without some rework of how we handle connector probing in sysfs it's not at all currently possible. In the future, maybe we can try using the sysfs locks to protect updates to connector probing state and fix this mess. So for now, to protect everything other than port->connector under &mgr->base.lock and ensure that we still have the guarantee that atomic check/commit contexts will never see port->connector change we use a silly trick. See: port->connector only needs to change in order to ensure that input ports (see the MST spec) never have a ghost connector associated with them. But, there's nothing stopping us from simply throwing the entire port out and creating a new one in order to maintain that requirement while still keeping port->connector consistent across the lifetime of the port in atomic check/commit contexts. For all intended purposes this works fine, as we validate ports in any contexts we care about before using them and as such will end up reporting the connector as disconnected until it's port's destruction finalizes. So, we just do that in cases where we detect port->input has transitioned from true->false. We don't need to worry about the other direction, since a port without a connector isn't visible to userspace and as such doesn't need to be protected by &mgr->base.lock until we finish registering a connector for it. For updating members of drm_dp_mst_port other than port->connector, we simply grab &mgr->base.lock in drm_dp_mst_link_probe_work() for already registered ports, update said members and drop the lock before potentially registering a connector and probing the link address of it's children. Finally, we modify drm_dp_mst_detect_port() to take a modesetting lock acquisition context in order to acquire &mgr->base.lock under &connection_mutex and convert all it's users over to using the .detect_ctx probe hooks. With that, we finally have well defined locking. Changes since v4: * Get rid of port->mutex, stop using connection_mutex and just use our own modesetting lock - mgr->base.lock. Also, add a probe_lock that comes before this patch. * Just throw out ports that get changed from an output to an input, and replace them with new ports. This lets us ensure that modesetting contexts never see port->connector go from having a connector to being NULL. * Write an extremely detailed explanation of what problems this is trying to fix, since there's a _lot_ of context here and I honestly forgot some of it myself a couple times. * Don't grab mgr->lock when reading port->mstb in drm_dp_mst_handle_link_address_port(). It's not needed. Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Cc: Daniel Vetter Reviewed-by: Sean Paul Signed-off-by: Lyude Paul Link: https://patchwork.freedesktop.org/patch/msgid/20191022023641.8026-7-lyude@redhat.com --- .../amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 28 ++- drivers/gpu/drm/drm_dp_mst_topology.c | 230 ++++++++++++++------- drivers/gpu/drm/i915/display/intel_dp_mst.c | 28 +-- drivers/gpu/drm/nouveau/dispnv50/disp.c | 32 +-- drivers/gpu/drm/radeon/radeon_dp_mst.c | 24 ++- include/drm/drm_dp_mst_helper.h | 38 ++-- 6 files changed, 240 insertions(+), 140 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c index 5ec14efd4d8c..f8214acf70b3 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c @@ -123,21 +123,6 @@ static ssize_t dm_dp_aux_transfer(struct drm_dp_aux *aux, return result; } -static enum drm_connector_status -dm_dp_mst_detect(struct drm_connector *connector, bool force) -{ - struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector); - struct amdgpu_dm_connector *master = aconnector->mst_port; - - enum drm_connector_status status = - drm_dp_mst_detect_port( - connector, - &master->mst_mgr, - aconnector->port); - - return status; -} - static void dm_dp_mst_connector_destroy(struct drm_connector *connector) { @@ -177,7 +162,6 @@ amdgpu_dm_mst_connector_early_unregister(struct drm_connector *connector) } static const struct drm_connector_funcs dm_dp_mst_connector_funcs = { - .detect = dm_dp_mst_detect, .fill_modes = drm_helper_probe_single_connector_modes, .destroy = dm_dp_mst_connector_destroy, .reset = amdgpu_dm_connector_funcs_reset, @@ -252,10 +236,22 @@ static struct drm_encoder *dm_mst_best_encoder(struct drm_connector *connector) return &amdgpu_dm_connector->mst_encoder->base; } +static int +dm_dp_mst_detect(struct drm_connector *connector, + struct drm_modeset_acquire_ctx *ctx, bool force) +{ + struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector); + struct amdgpu_dm_connector *master = aconnector->mst_port; + + return drm_dp_mst_detect_port(connector, ctx, &master->mst_mgr, + aconnector->port); +} + static const struct drm_connector_helper_funcs dm_dp_mst_connector_helper_funcs = { .get_modes = dm_dp_mst_get_modes, .mode_valid = amdgpu_dm_connector_mode_valid, .best_encoder = dm_mst_best_encoder, + .detect_ctx = dm_dp_mst_detect, }; static void amdgpu_dm_encoder_destroy(struct drm_encoder *encoder) diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 2f87dec31c3c..55f289eebea7 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1905,6 +1905,78 @@ void drm_dp_mst_connector_early_unregister(struct drm_connector *connector, } EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister); +static void +drm_dp_mst_port_add_connector(struct drm_dp_mst_branch *mstb, + struct drm_dp_mst_port *port) +{ + struct drm_dp_mst_topology_mgr *mgr = port->mgr; + char proppath[255]; + int ret; + + build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath)); + port->connector = mgr->cbs->add_connector(mgr, port, proppath); + if (!port->connector) { + ret = -ENOMEM; + goto error; + } + + if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV || + port->pdt == DP_PEER_DEVICE_SST_SINK) && + port->port_num >= DP_MST_LOGICAL_PORT_0) { + port->cached_edid = drm_get_edid(port->connector, + &port->aux.ddc); + drm_connector_set_tile_property(port->connector); + } + + mgr->cbs->register_connector(port->connector); + return; + +error: + DRM_ERROR("Failed to create connector for port %p: %d\n", port, ret); +} + +/* + * Drop a topology reference, and unlink the port from the in-memory topology + * layout + */ +static void +drm_dp_mst_topology_unlink_port(struct drm_dp_mst_topology_mgr *mgr, + struct drm_dp_mst_port *port) +{ + mutex_lock(&mgr->lock); + list_del(&port->next); + mutex_unlock(&mgr->lock); + drm_dp_mst_topology_put_port(port); +} + +static struct drm_dp_mst_port * +drm_dp_mst_add_port(struct drm_device *dev, + struct drm_dp_mst_topology_mgr *mgr, + struct drm_dp_mst_branch *mstb, u8 port_number) +{ + struct drm_dp_mst_port *port = kzalloc(sizeof(*port), GFP_KERNEL); + + if (!port) + return NULL; + + kref_init(&port->topology_kref); + kref_init(&port->malloc_kref); + port->parent = mstb; + port->port_num = port_number; + port->mgr = mgr; + port->aux.name = "DPMST"; + port->aux.dev = dev->dev; + port->aux.is_remote = true; + + /* + * Make sure the memory allocation for our parent branch stays + * around until our own memory allocation is released + */ + drm_dp_mst_get_mstb_malloc(mstb); + + return port; +} + static void drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, struct drm_device *dev, @@ -1912,35 +1984,40 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, { struct drm_dp_mst_topology_mgr *mgr = mstb->mgr; struct drm_dp_mst_port *port; - bool created = false; - int old_ddps = 0; + int old_ddps = 0, ret; + u8 new_pdt = DP_PEER_DEVICE_NONE; + bool created = false, send_link_addr = false; port = drm_dp_get_port(mstb, port_msg->port_number); if (!port) { - port = kzalloc(sizeof(*port), GFP_KERNEL); + port = drm_dp_mst_add_port(dev, mgr, mstb, + port_msg->port_number); if (!port) return; - kref_init(&port->topology_kref); - kref_init(&port->malloc_kref); - port->parent = mstb; - port->port_num = port_msg->port_number; - port->mgr = mgr; - port->aux.name = "DPMST"; - port->aux.dev = dev->dev; - port->aux.is_remote = true; - - /* - * Make sure the memory allocation for our parent branch stays - * around until our own memory allocation is released + created = true; + } else if (port_msg->input_port && !port->input && port->connector) { + /* Destroying the connector is impossible in this context, so + * replace the port with a new one */ - drm_dp_mst_get_mstb_malloc(mstb); + drm_dp_mst_topology_unlink_port(mgr, port); + drm_dp_mst_topology_put_port(port); + port = drm_dp_mst_add_port(dev, mgr, mstb, + port_msg->port_number); + if (!port) + return; created = true; } else { + /* Locking is only needed when the port has a connector + * exposed to userspace + */ + drm_modeset_lock(&mgr->base.lock, NULL); old_ddps = port->ddps; } port->input = port_msg->input_port; + if (!port->input) + new_pdt = port_msg->peer_device_type; port->mcs = port_msg->mcs; port->ddps = port_msg->ddps; port->ldps = port_msg->legacy_device_plug_status; @@ -1968,59 +2045,39 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, } } - if (!port->input) { - int ret = drm_dp_port_set_pdt(port, - port_msg->peer_device_type); - if (ret == 1) { - drm_dp_send_link_address(mgr, port->mstb); - } else if (ret < 0) { - DRM_ERROR("Failed to change PDT on port %p: %d\n", - port, ret); - goto fail; - } + ret = drm_dp_port_set_pdt(port, new_pdt); + if (ret == 1) { + send_link_addr = true; + } else if (ret < 0) { + DRM_ERROR("Failed to change PDT on port %p: %d\n", + port, ret); + goto fail; } - if (created && !port->input) { - char proppath[255]; - - build_mst_prop_path(mstb, port->port_num, proppath, - sizeof(proppath)); - port->connector = (*mgr->cbs->add_connector)(mgr, port, - proppath); - if (!port->connector) - goto fail; - - if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV || - port->pdt == DP_PEER_DEVICE_SST_SINK) && - port->port_num >= DP_MST_LOGICAL_PORT_0) { - port->cached_edid = drm_get_edid(port->connector, - &port->aux.ddc); - drm_connector_set_tile_property(port->connector); - } + if (!created) + drm_modeset_unlock(&mgr->base.lock); + else if (!port->connector && !port->input) + drm_dp_mst_port_add_connector(mstb, port); - (*mgr->cbs->register_connector)(port->connector); - } + if (send_link_addr && port->mstb) + drm_dp_send_link_address(mgr, port->mstb); /* put reference to this port */ drm_dp_mst_topology_put_port(port); return; fail: - /* Remove it from the port list */ - mutex_lock(&mgr->lock); - list_del(&port->next); - mutex_unlock(&mgr->lock); - - /* Drop the port list reference */ - drm_dp_mst_topology_put_port(port); - /* And now drop our reference */ + drm_dp_mst_topology_unlink_port(mgr, port); drm_dp_mst_topology_put_port(port); + if (!created) + drm_modeset_unlock(&mgr->base.lock); } static void drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb, struct drm_dp_connection_status_notify *conn_stat) { + struct drm_dp_mst_topology_mgr *mgr = mstb->mgr; struct drm_dp_mst_port *port; int old_ddps; bool dowork = false; @@ -2029,6 +2086,10 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb, if (!port) return; + /* Locking is only needed if the port's exposed to userspace */ + if (port->connector) + drm_modeset_lock(&mgr->base.lock, NULL); + old_ddps = port->ddps; port->mcs = conn_stat->message_capability_status; port->ldps = conn_stat->legacy_device_plug_status; @@ -2054,10 +2115,12 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb, } } + if (port->connector) + drm_modeset_unlock(&mgr->base.lock); + drm_dp_mst_topology_put_port(port); if (dowork) queue_work(system_long_wq, &mstb->mgr->work); - } static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct drm_dp_mst_topology_mgr *mgr, @@ -2157,8 +2220,11 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m if (port->input || !port->ddps) continue; - if (!port->available_pbn) + if (!port->available_pbn) { + drm_modeset_lock(&mgr->base.lock, NULL); drm_dp_send_enum_path_resources(mgr, mstb, port); + drm_modeset_unlock(&mgr->base.lock); + } if (port->mstb) mstb_child = drm_dp_mst_topology_get_mstb_validated( @@ -2189,11 +2255,16 @@ static void drm_dp_mst_link_probe_work(struct work_struct *work) mstb = NULL; } mutex_unlock(&mgr->lock); - if (mstb) { - drm_dp_check_and_send_link_address(mgr, mstb); - drm_dp_mst_topology_put_mstb(mstb); + if (!mstb) { + mutex_unlock(&mgr->probe_lock); + return; } + + drm_dp_check_and_send_link_address(mgr, mstb); + drm_dp_mst_topology_put_mstb(mstb); + mutex_unlock(&mgr->probe_lock); + drm_kms_helper_hotplug_event(dev); } static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr, @@ -2478,8 +2549,6 @@ static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, drm_dp_mst_handle_link_address_port(mstb, mgr->dev, &reply->ports[i]); - drm_kms_helper_hotplug_event(mgr->dev); - out: if (ret <= 0) mstb->link_address_sent = false; @@ -3274,13 +3343,14 @@ clear_down_rep_recv: return 0; } -static inline void +static inline bool drm_dp_mst_process_up_req(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_pending_up_req *up_req) { struct drm_dp_mst_branch *mstb = NULL; struct drm_dp_sideband_msg_req_body *msg = &up_req->msg; struct drm_dp_sideband_msg_hdr *hdr = &up_req->hdr; + bool hotplug = false; if (hdr->broadcast) { const u8 *guid = NULL; @@ -3298,16 +3368,17 @@ drm_dp_mst_process_up_req(struct drm_dp_mst_topology_mgr *mgr, if (!mstb) { DRM_DEBUG_KMS("Got MST reply from unknown device %d\n", hdr->lct); - return; + return false; } /* TODO: Add missing handler for DP_RESOURCE_STATUS_NOTIFY events */ if (msg->req_type == DP_CONNECTION_STATUS_NOTIFY) { drm_dp_mst_handle_conn_stat(mstb, &msg->u.conn_stat); - drm_kms_helper_hotplug_event(mgr->dev); + hotplug = true; } drm_dp_mst_topology_put_mstb(mstb); + return hotplug; } static void drm_dp_mst_up_req_work(struct work_struct *work) @@ -3316,6 +3387,7 @@ static void drm_dp_mst_up_req_work(struct work_struct *work) container_of(work, struct drm_dp_mst_topology_mgr, up_req_work); struct drm_dp_pending_up_req *up_req; + bool send_hotplug = false; mutex_lock(&mgr->probe_lock); while (true) { @@ -3330,10 +3402,13 @@ static void drm_dp_mst_up_req_work(struct work_struct *work) if (!up_req) break; - drm_dp_mst_process_up_req(mgr, up_req); + send_hotplug |= drm_dp_mst_process_up_req(mgr, up_req); kfree(up_req); } mutex_unlock(&mgr->probe_lock); + + if (send_hotplug) + drm_kms_helper_hotplug_event(mgr->dev); } static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr) @@ -3441,22 +3516,31 @@ EXPORT_SYMBOL(drm_dp_mst_hpd_irq); /** * drm_dp_mst_detect_port() - get connection status for an MST port * @connector: DRM connector for this port + * @ctx: The acquisition context to use for grabbing locks * @mgr: manager for this port - * @port: unverified pointer to a port + * @port: pointer to a port * - * This returns the current connection state for a port. It validates the - * port pointer still exists so the caller doesn't require a reference + * This returns the current connection state for a port. */ -enum drm_connector_status drm_dp_mst_detect_port(struct drm_connector *connector, - struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port) +int +drm_dp_mst_detect_port(struct drm_connector *connector, + struct drm_modeset_acquire_ctx *ctx, + struct drm_dp_mst_topology_mgr *mgr, + struct drm_dp_mst_port *port) { - enum drm_connector_status status = connector_status_disconnected; + int ret; /* we need to search for the port in the mgr in case it's gone */ port = drm_dp_mst_topology_get_port_validated(mgr, port); if (!port) return connector_status_disconnected; + ret = drm_modeset_lock(&mgr->base.lock, ctx); + if (ret) + goto out; + + ret = connector_status_disconnected; + if (!port->ddps) goto out; @@ -3466,7 +3550,7 @@ enum drm_connector_status drm_dp_mst_detect_port(struct drm_connector *connector break; case DP_PEER_DEVICE_SST_SINK: - status = connector_status_connected; + ret = connector_status_connected; /* for logical ports - cache the EDID */ if (port->port_num >= 8 && !port->cached_edid) { port->cached_edid = drm_get_edid(connector, &port->aux.ddc); @@ -3474,12 +3558,12 @@ enum drm_connector_status drm_dp_mst_detect_port(struct drm_connector *connector break; case DP_PEER_DEVICE_DP_LEGACY_CONV: if (port->ldps) - status = connector_status_connected; + ret = connector_status_connected; break; } out: drm_dp_mst_topology_put_port(port); - return status; + return ret; } EXPORT_SYMBOL(drm_dp_mst_detect_port); diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c index bbcab27644dc..a9962846a503 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -391,20 +391,7 @@ static int intel_dp_mst_get_ddc_modes(struct drm_connector *connector) return ret; } -static enum drm_connector_status -intel_dp_mst_detect(struct drm_connector *connector, bool force) -{ - struct intel_connector *intel_connector = to_intel_connector(connector); - struct intel_dp *intel_dp = intel_connector->mst_port; - - if (drm_connector_is_unregistered(connector)) - return connector_status_disconnected; - return drm_dp_mst_detect_port(connector, &intel_dp->mst_mgr, - intel_connector->port); -} - static const struct drm_connector_funcs intel_dp_mst_connector_funcs = { - .detect = intel_dp_mst_detect, .fill_modes = drm_helper_probe_single_connector_modes, .atomic_get_property = intel_digital_connector_atomic_get_property, .atomic_set_property = intel_digital_connector_atomic_set_property, @@ -465,11 +452,26 @@ static struct drm_encoder *intel_mst_atomic_best_encoder(struct drm_connector *c return &intel_dp->mst_encoders[crtc->pipe]->base.base; } +static int +intel_dp_mst_detect(struct drm_connector *connector, + struct drm_modeset_acquire_ctx *ctx, bool force) +{ + struct intel_connector *intel_connector = to_intel_connector(connector); + struct intel_dp *intel_dp = intel_connector->mst_port; + + if (drm_connector_is_unregistered(connector)) + return connector_status_disconnected; + + return drm_dp_mst_detect_port(connector, ctx, &intel_dp->mst_mgr, + intel_connector->port); +} + static const struct drm_connector_helper_funcs intel_dp_mst_connector_helper_funcs = { .get_modes = intel_dp_mst_get_modes, .mode_valid = intel_dp_mst_mode_valid, .atomic_best_encoder = intel_mst_atomic_best_encoder, .atomic_check = intel_dp_mst_atomic_check, + .detect_ctx = intel_dp_mst_detect, }; static void intel_dp_mst_encoder_destroy(struct drm_encoder *encoder) diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index a13924ae1992..a9d6aa110cfd 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -986,20 +986,11 @@ nv50_mstc_atomic_check(struct drm_connector *connector, return drm_dp_atomic_release_vcpi_slots(state, mgr, mstc->port); } -static const struct drm_connector_helper_funcs -nv50_mstc_help = { - .get_modes = nv50_mstc_get_modes, - .mode_valid = nv50_mstc_mode_valid, - .best_encoder = nv50_mstc_best_encoder, - .atomic_best_encoder = nv50_mstc_atomic_best_encoder, - .atomic_check = nv50_mstc_atomic_check, -}; - -static enum drm_connector_status -nv50_mstc_detect(struct drm_connector *connector, bool force) +static int +nv50_mstc_detect(struct drm_connector *connector, + struct drm_modeset_acquire_ctx *ctx, bool force) { struct nv50_mstc *mstc = nv50_mstc(connector); - enum drm_connector_status conn_status; int ret; if (drm_connector_is_unregistered(connector)) @@ -1009,14 +1000,24 @@ nv50_mstc_detect(struct drm_connector *connector, bool force) if (ret < 0 && ret != -EACCES) return connector_status_disconnected; - conn_status = drm_dp_mst_detect_port(connector, mstc->port->mgr, - mstc->port); + ret = drm_dp_mst_detect_port(connector, ctx, mstc->port->mgr, + mstc->port); pm_runtime_mark_last_busy(connector->dev->dev); pm_runtime_put_autosuspend(connector->dev->dev); - return conn_status; + return ret; } +static const struct drm_connector_helper_funcs +nv50_mstc_help = { + .get_modes = nv50_mstc_get_modes, + .mode_valid = nv50_mstc_mode_valid, + .best_encoder = nv50_mstc_best_encoder, + .atomic_best_encoder = nv50_mstc_atomic_best_encoder, + .atomic_check = nv50_mstc_atomic_check, + .detect_ctx = nv50_mstc_detect, +}; + static void nv50_mstc_destroy(struct drm_connector *connector) { @@ -1031,7 +1032,6 @@ nv50_mstc_destroy(struct drm_connector *connector) static const struct drm_connector_funcs nv50_mstc = { .reset = nouveau_conn_reset, - .detect = nv50_mstc_detect, .fill_modes = drm_helper_probe_single_connector_modes, .destroy = nv50_mstc_destroy, .atomic_duplicate_state = nouveau_conn_atomic_duplicate_state, diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c index 2994f07fbad9..ee28f5b3785e 100644 --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c @@ -233,21 +233,26 @@ drm_encoder *radeon_mst_best_encoder(struct drm_connector *connector) return &radeon_connector->mst_encoder->base; } +static int +radeon_dp_mst_detect(struct drm_connector *connector, + struct drm_modeset_acquire_ctx *ctx, + bool force) +{ + struct radeon_connector *radeon_connector = + to_radeon_connector(connector); + struct radeon_connector *master = radeon_connector->mst_port; + + return drm_dp_mst_detect_port(connector, ctx, &master->mst_mgr, + radeon_connector->port); +} + static const struct drm_connector_helper_funcs radeon_dp_mst_connector_helper_funcs = { .get_modes = radeon_dp_mst_get_modes, .mode_valid = radeon_dp_mst_mode_valid, .best_encoder = radeon_mst_best_encoder, + .detect_ctx = radeon_dp_mst_detect, }; -static enum drm_connector_status -radeon_dp_mst_detect(struct drm_connector *connector, bool force) -{ - struct radeon_connector *radeon_connector = to_radeon_connector(connector); - struct radeon_connector *master = radeon_connector->mst_port; - - return drm_dp_mst_detect_port(connector, &master->mst_mgr, radeon_connector->port); -} - static void radeon_dp_mst_connector_destroy(struct drm_connector *connector) { @@ -262,7 +267,6 @@ radeon_dp_mst_connector_destroy(struct drm_connector *connector) static const struct drm_connector_funcs radeon_dp_mst_connector_funcs = { .dpms = drm_helper_connector_dpms, - .detect = radeon_dp_mst_detect, .fill_modes = drm_helper_probe_single_connector_modes, .destroy = radeon_dp_mst_connector_destroy, }; diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index bccb5514e0ef..fd142db42cb0 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -45,21 +45,31 @@ struct drm_dp_vcpi { /** * struct drm_dp_mst_port - MST port * @port_num: port number - * @input: if this port is an input port. - * @mcs: message capability status - DP 1.2 spec. - * @ddps: DisplayPort Device Plug Status - DP 1.2 - * @pdt: Peer Device Type - * @ldps: Legacy Device Plug Status - * @dpcd_rev: DPCD revision of device on this port - * @num_sdp_streams: Number of simultaneous streams - * @num_sdp_stream_sinks: Number of stream sinks - * @available_pbn: Available bandwidth for this port. + * @input: if this port is an input port. Protected by + * &drm_dp_mst_topology_mgr.base.lock. + * @mcs: message capability status - DP 1.2 spec. Protected by + * &drm_dp_mst_topology_mgr.base.lock. + * @ddps: DisplayPort Device Plug Status - DP 1.2. Protected by + * &drm_dp_mst_topology_mgr.base.lock. + * @pdt: Peer Device Type. Protected by + * &drm_dp_mst_topology_mgr.base.lock. + * @ldps: Legacy Device Plug Status. Protected by + * &drm_dp_mst_topology_mgr.base.lock. + * @dpcd_rev: DPCD revision of device on this port. Protected by + * &drm_dp_mst_topology_mgr.base.lock. + * @num_sdp_streams: Number of simultaneous streams. Protected by + * &drm_dp_mst_topology_mgr.base.lock. + * @num_sdp_stream_sinks: Number of stream sinks. Protected by + * &drm_dp_mst_topology_mgr.base.lock. + * @available_pbn: Available bandwidth for this port. Protected by + * &drm_dp_mst_topology_mgr.base.lock. * @next: link to next port on this branch device * @aux: i2c aux transport to talk to device connected to this port, protected - * by &drm_dp_mst_topology_mgr.lock + * by &drm_dp_mst_topology_mgr.base.lock. * @parent: branch device parent of this port * @vcpi: Virtual Channel Payload info for this port. - * @connector: DRM connector this port is connected to. + * @connector: DRM connector this port is connected to. Protected by + * &drm_dp_mst_topology_mgr.base.lock. * @mgr: topology manager this port lives under. * * This structure represents an MST port endpoint on a device somewhere @@ -653,7 +663,11 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handled); -enum drm_connector_status drm_dp_mst_detect_port(struct drm_connector *connector, struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port); +int +drm_dp_mst_detect_port(struct drm_connector *connector, + struct drm_modeset_acquire_ctx *ctx, + struct drm_dp_mst_topology_mgr *mgr, + struct drm_dp_mst_port *port); bool drm_dp_mst_port_has_audio(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port); -- cgit v1.2.3 From dad7d84f883571dddb686c426cfd2533f58e7c27 Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Wed, 16 Oct 2019 17:50:15 -0400 Subject: drm/dp_mst: Don't forget to update port->input in drm_dp_mst_handle_conn_stat() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This probably hasn't caused any problems up until now since it's probably nearly impossible to encounter this in the wild, however if we were to receive a connection status notification from the MST hub after resume while we're in the middle of reprobing the link addresses for a topology then there's a much larger chance that a port could have changed from being an output port to input port (or vice versa). If we forget to update this bit of information, we'll potentially ignore a valid PDT change on a downstream port because we think it's an input port. So, make sure we read the input_port field in connection status notifications in drm_dp_mst_handle_conn_stat() to prevent this from happening once we've implemented suspend/resume reprobing. Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Cc: Daniel Vetter Reviewed-by: Sean Paul Signed-off-by: Lyude Paul Link: https://patchwork.freedesktop.org/patch/msgid/20191022023641.8026-8-lyude@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 50 +++++++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 14 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 55f289eebea7..5e031b0ca726 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -2079,18 +2079,38 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb, { struct drm_dp_mst_topology_mgr *mgr = mstb->mgr; struct drm_dp_mst_port *port; - int old_ddps; - bool dowork = false; + int old_ddps, ret; + u8 new_pdt; + bool dowork = false, create_connector = false; port = drm_dp_get_port(mstb, conn_stat->port_number); if (!port) return; - /* Locking is only needed if the port's exposed to userspace */ - if (port->connector) + if (port->connector) { + if (!port->input && conn_stat->input_port) { + /* + * We can't remove a connector from an already exposed + * port, so just throw the port out and make sure we + * reprobe the link address of it's parent MSTB + */ + drm_dp_mst_topology_unlink_port(mgr, port); + mstb->link_address_sent = false; + dowork = true; + goto out; + } + + /* Locking is only needed if the port's exposed to userspace */ drm_modeset_lock(&mgr->base.lock, NULL); + } else if (port->input && !conn_stat->input_port) { + create_connector = true; + /* Reprobe link address so we get num_sdp_streams */ + mstb->link_address_sent = false; + dowork = true; + } old_ddps = port->ddps; + port->input = conn_stat->input_port; port->mcs = conn_stat->message_capability_status; port->ldps = conn_stat->legacy_device_plug_status; port->ddps = conn_stat->displayport_device_plug_status; @@ -2103,21 +2123,23 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb, } } - if (!port->input) { - int ret = drm_dp_port_set_pdt(port, - conn_stat->peer_device_type); - if (ret == 1) { - dowork = true; - } else if (ret < 0) { - DRM_ERROR("Failed to change PDT for port %p: %d\n", - port, ret); - dowork = false; - } + new_pdt = port->input ? DP_PEER_DEVICE_NONE : conn_stat->peer_device_type; + + ret = drm_dp_port_set_pdt(port, new_pdt); + if (ret == 1) { + dowork = true; + } else if (ret < 0) { + DRM_ERROR("Failed to change PDT for port %p: %d\n", + port, ret); + dowork = false; } if (port->connector) drm_modeset_unlock(&mgr->base.lock); + else if (create_connector) + drm_dp_mst_port_add_connector(mstb, port); +out: drm_dp_mst_topology_put_port(port); if (dowork) queue_work(system_long_wq, &mstb->mgr->work); -- cgit v1.2.3 From 79413ed4a1b7c9c19f9caa375a06d86c9c045ffb Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Wed, 9 Oct 2019 15:14:59 -0400 Subject: drm/dp_mst: Lessen indenting in drm_dp_mst_topology_mgr_resume() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Does what it says on the tin. Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Cc: Daniel Vetter Reviewed-by: Sean Paul Signed-off-by: Lyude Paul Link: https://patchwork.freedesktop.org/patch/msgid/20191022023641.8026-9-lyude@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 59 +++++++++++++++++------------------ 1 file changed, 29 insertions(+), 30 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 5e031b0ca726..588b67cea052 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3210,45 +3210,44 @@ EXPORT_SYMBOL(drm_dp_mst_topology_mgr_suspend); */ int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr) { - int ret = 0; + int ret; + u8 guid[16]; mutex_lock(&mgr->lock); + if (!mgr->mst_primary) + goto out_fail; - if (mgr->mst_primary) { - int sret; - u8 guid[16]; + ret = drm_dp_dpcd_read(mgr->aux, DP_DPCD_REV, mgr->dpcd, + DP_RECEIVER_CAP_SIZE); + if (ret != DP_RECEIVER_CAP_SIZE) { + DRM_DEBUG_KMS("dpcd read failed - undocked during suspend?\n"); + goto out_fail; + } - sret = drm_dp_dpcd_read(mgr->aux, DP_DPCD_REV, mgr->dpcd, DP_RECEIVER_CAP_SIZE); - if (sret != DP_RECEIVER_CAP_SIZE) { - DRM_DEBUG_KMS("dpcd read failed - undocked during suspend?\n"); - ret = -1; - goto out_unlock; - } + ret = drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, + DP_MST_EN | + DP_UP_REQ_EN | + DP_UPSTREAM_IS_SRC); + if (ret < 0) { + DRM_DEBUG_KMS("mst write failed - undocked during suspend?\n"); + goto out_fail; + } - ret = drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, - DP_MST_EN | DP_UP_REQ_EN | DP_UPSTREAM_IS_SRC); - if (ret < 0) { - DRM_DEBUG_KMS("mst write failed - undocked during suspend?\n"); - ret = -1; - goto out_unlock; - } + /* Some hubs forget their guids after they resume */ + ret = drm_dp_dpcd_read(mgr->aux, DP_GUID, guid, 16); + if (ret != 16) { + DRM_DEBUG_KMS("dpcd read failed - undocked during suspend?\n"); + goto out_fail; + } + drm_dp_check_mstb_guid(mgr->mst_primary, guid); - /* Some hubs forget their guids after they resume */ - sret = drm_dp_dpcd_read(mgr->aux, DP_GUID, guid, 16); - if (sret != 16) { - DRM_DEBUG_KMS("dpcd read failed - undocked during suspend?\n"); - ret = -1; - goto out_unlock; - } - drm_dp_check_mstb_guid(mgr->mst_primary, guid); + mutex_unlock(&mgr->lock); - ret = 0; - } else - ret = -1; + return 0; -out_unlock: +out_fail: mutex_unlock(&mgr->lock); - return ret; + return -1; } EXPORT_SYMBOL(drm_dp_mst_topology_mgr_resume); -- cgit v1.2.3 From 6f85f73821f6af4de4429ab2f2f7958dbd81cb90 Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Mon, 17 Jun 2019 19:57:33 -0400 Subject: drm/dp_mst: Add basic topology reprobing when resuming MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finally! For a very long time, our MST helpers have had one very annoying issue: They don't know how to reprobe the topology state when coming out of suspend. This means that if a user has a machine connected to an MST topology and decides to suspend their machine, we lose all topology changes that happened during that period. That can be a big problem if the machine was connected to a different topology on the same port before resuming, as we won't bother reprobing any of the ports and likely cause the user's monitors not to come back up as expected. So, we start fixing this by teaching our MST helpers how to reprobe the link addresses of each connected topology when resuming. As it turns out, the behavior that we want here is identical to the behavior we want when initially probing a newly connected MST topology, with a couple of important differences: - We need to be more careful about handling the potential races between events from the MST hub that could change the topology state as we're performing the link address reprobe - We need to be more careful about handling unlikely state changes on ports - such as an input port turning into an output port, something that would be far more likely to happen in situations like the MST hub we're connected to being changed while we're suspend Both of which have been solved by previous commits. That leaves one requirement: - We need to prune any MST ports in our in-memory topology state that were present when suspending, but have not appeared in the post-resume link address response from their parent branch device Which we can now handle in this commit by modifying drm_dp_send_link_address(). We then introduce suspend/resume reprobing by introducing drm_dp_mst_topology_mgr_invalidate_mstb(), which we call in drm_dp_mst_topology_mgr_suspend() to traverse the in-memory topology state to indicate that each mstb needs it's link address resent and PBN resources reprobed. On resume, we start back up &mgr->work and have it reprobe the topology in the same way we would on a hotplug, removing any leftover ports that no longer appear in the topology state. Changes since v4: * Split indenting changes in drm_dp_mst_topology_mgr_resume() into a separate patch * Only fire hotplugs when something has actually changed after a link address probe * Don't try to change port->connector at all on ports, just throw out ports that need their connectors removed to make things easier. Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Cc: Daniel Vetter Reviewed-by: Sean Paul Signed-off-by: Lyude Paul Link: https://patchwork.freedesktop.org/patch/msgid/20191022023641.8026-14-lyude@redhat.com --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- drivers/gpu/drm/drm_dp_mst_topology.c | 182 ++++++++++++++++++---- drivers/gpu/drm/i915/display/intel_dp.c | 3 +- drivers/gpu/drm/nouveau/dispnv50/disp.c | 6 +- include/drm/drm_dp_mst_helper.h | 3 +- 5 files changed, 156 insertions(+), 40 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 8f67d301ad81..6c34f640f419 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -974,7 +974,7 @@ static void s3_handle_mst(struct drm_device *dev, bool suspend) if (suspend) { drm_dp_mst_topology_mgr_suspend(mgr); } else { - ret = drm_dp_mst_topology_mgr_resume(mgr); + ret = drm_dp_mst_topology_mgr_resume(mgr, true); if (ret < 0) { drm_dp_mst_topology_mgr_set_mst(mgr, false); need_hotplug = true; diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 588b67cea052..42eac2a59a4b 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -67,8 +67,8 @@ static int drm_dp_send_dpcd_write(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port, int offset, int size, u8 *bytes); -static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, - struct drm_dp_mst_branch *mstb); +static int drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, + struct drm_dp_mst_branch *mstb); static int drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_branch *mstb, struct drm_dp_mst_port *port); @@ -1977,7 +1977,7 @@ drm_dp_mst_add_port(struct drm_device *dev, return port; } -static void +static int drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, struct drm_device *dev, struct drm_dp_link_addr_reply_port *port_msg) @@ -1986,33 +1986,45 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, struct drm_dp_mst_port *port; int old_ddps = 0, ret; u8 new_pdt = DP_PEER_DEVICE_NONE; - bool created = false, send_link_addr = false; + bool created = false, send_link_addr = false, changed = false; port = drm_dp_get_port(mstb, port_msg->port_number); if (!port) { port = drm_dp_mst_add_port(dev, mgr, mstb, port_msg->port_number); if (!port) - return; + return -ENOMEM; created = true; - } else if (port_msg->input_port && !port->input && port->connector) { - /* Destroying the connector is impossible in this context, so - * replace the port with a new one + changed = true; + } else if (!port->input && port_msg->input_port && port->connector) { + /* Since port->connector can't be changed here, we create a + * new port if input_port changes from 0 to 1 */ drm_dp_mst_topology_unlink_port(mgr, port); drm_dp_mst_topology_put_port(port); - port = drm_dp_mst_add_port(dev, mgr, mstb, port_msg->port_number); if (!port) - return; + return -ENOMEM; + changed = true; created = true; - } else { - /* Locking is only needed when the port has a connector - * exposed to userspace + } else if (port->input && !port_msg->input_port) { + changed = true; + } else if (port->connector) { + /* We're updating a port that's exposed to userspace, so do it + * under lock */ drm_modeset_lock(&mgr->base.lock, NULL); + old_ddps = port->ddps; + changed = port->ddps != port_msg->ddps || + (port->ddps && + (port->ldps != port_msg->legacy_device_plug_status || + port->dpcd_rev != port_msg->dpcd_revision || + port->mcs != port_msg->mcs || + port->pdt != port_msg->peer_device_type || + port->num_sdp_stream_sinks != + port_msg->num_sdp_stream_sinks)); } port->input = port_msg->input_port; @@ -2054,23 +2066,38 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, goto fail; } - if (!created) + /* + * If this port wasn't just created, then we're reprobing because + * we're coming out of suspend. In this case, always resend the link + * address if there's an MSTB on this port + */ + if (!created && port->pdt == DP_PEER_DEVICE_MST_BRANCHING) + send_link_addr = true; + + if (port->connector) drm_modeset_unlock(&mgr->base.lock); - else if (!port->connector && !port->input) + else if (!port->input) drm_dp_mst_port_add_connector(mstb, port); - if (send_link_addr && port->mstb) - drm_dp_send_link_address(mgr, port->mstb); + if (send_link_addr && port->mstb) { + ret = drm_dp_send_link_address(mgr, port->mstb); + if (ret == 1) /* MSTB below us changed */ + changed = true; + else if (ret < 0) + goto fail_put; + } /* put reference to this port */ drm_dp_mst_topology_put_port(port); - return; + return changed; fail: drm_dp_mst_topology_unlink_port(mgr, port); - drm_dp_mst_topology_put_port(port); - if (!created) + if (port->connector) drm_modeset_unlock(&mgr->base.lock); +fail_put: + drm_dp_mst_topology_put_port(port); + return ret; } static void @@ -2228,13 +2255,20 @@ drm_dp_get_mst_branch_device_by_guid(struct drm_dp_mst_topology_mgr *mgr, return mstb; } -static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *mgr, +static int drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_branch *mstb) { struct drm_dp_mst_port *port; + int ret; + bool changed = false; - if (!mstb->link_address_sent) - drm_dp_send_link_address(mgr, mstb); + if (!mstb->link_address_sent) { + ret = drm_dp_send_link_address(mgr, mstb); + if (ret == 1) + changed = true; + else if (ret < 0) + return ret; + } list_for_each_entry(port, &mstb->ports, next) { struct drm_dp_mst_branch *mstb_child = NULL; @@ -2246,6 +2280,7 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m drm_modeset_lock(&mgr->base.lock, NULL); drm_dp_send_enum_path_resources(mgr, mstb, port); drm_modeset_unlock(&mgr->base.lock); + changed = true; } if (port->mstb) @@ -2253,10 +2288,17 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m mgr, port->mstb); if (mstb_child) { - drm_dp_check_and_send_link_address(mgr, mstb_child); + ret = drm_dp_check_and_send_link_address(mgr, + mstb_child); drm_dp_mst_topology_put_mstb(mstb_child); + if (ret == 1) + changed = true; + else if (ret < 0) + return ret; } } + + return changed; } static void drm_dp_mst_link_probe_work(struct work_struct *work) @@ -2282,11 +2324,12 @@ static void drm_dp_mst_link_probe_work(struct work_struct *work) return; } - drm_dp_check_and_send_link_address(mgr, mstb); + ret = drm_dp_check_and_send_link_address(mgr, mstb); drm_dp_mst_topology_put_mstb(mstb); mutex_unlock(&mgr->probe_lock); - drm_kms_helper_hotplug_event(dev); + if (ret) + drm_kms_helper_hotplug_event(dev); } static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr, @@ -2532,16 +2575,18 @@ drm_dp_dump_link_address(struct drm_dp_link_address_ack_reply *reply) } } -static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, +static int drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_branch *mstb) { struct drm_dp_sideband_msg_tx *txmsg; struct drm_dp_link_address_ack_reply *reply; - int i, len, ret; + struct drm_dp_mst_port *port, *tmp; + int i, len, ret, port_mask = 0; + bool changed = false; txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL); if (!txmsg) - return; + return -ENOMEM; txmsg->dst = mstb; len = build_link_address(txmsg); @@ -2567,14 +2612,39 @@ static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, drm_dp_check_mstb_guid(mstb, reply->guid); - for (i = 0; i < reply->nports; i++) - drm_dp_mst_handle_link_address_port(mstb, mgr->dev, - &reply->ports[i]); + for (i = 0; i < reply->nports; i++) { + port_mask |= BIT(reply->ports[i].port_number); + ret = drm_dp_mst_handle_link_address_port(mstb, mgr->dev, + &reply->ports[i]); + if (ret == 1) + changed = true; + else if (ret < 0) + goto out; + } + + /* Prune any ports that are currently a part of mstb in our in-memory + * topology, but were not seen in this link address. Usually this + * means that they were removed while the topology was out of sync, + * e.g. during suspend/resume + */ + mutex_lock(&mgr->lock); + list_for_each_entry_safe(port, tmp, &mstb->ports, next) { + if (port_mask & BIT(port->port_num)) + continue; + + DRM_DEBUG_KMS("port %d was not in link address, removing\n", + port->port_num); + list_del(&port->next); + drm_dp_mst_topology_put_port(port); + changed = true; + } + mutex_unlock(&mgr->lock); out: if (ret <= 0) mstb->link_address_sent = false; kfree(txmsg); + return ret < 0 ? ret : changed; } static int @@ -3179,6 +3249,23 @@ out_unlock: } EXPORT_SYMBOL(drm_dp_mst_topology_mgr_set_mst); +static void +drm_dp_mst_topology_mgr_invalidate_mstb(struct drm_dp_mst_branch *mstb) +{ + struct drm_dp_mst_port *port; + + /* The link address will need to be re-sent on resume */ + mstb->link_address_sent = false; + + list_for_each_entry(port, &mstb->ports, next) { + /* The PBN for each port will also need to be re-probed */ + port->available_pbn = 0; + + if (port->mstb) + drm_dp_mst_topology_mgr_invalidate_mstb(port->mstb); + } +} + /** * drm_dp_mst_topology_mgr_suspend() - suspend the MST manager * @mgr: manager to suspend @@ -3195,20 +3282,36 @@ void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr) flush_work(&mgr->up_req_work); flush_work(&mgr->work); flush_work(&mgr->delayed_destroy_work); + + mutex_lock(&mgr->lock); + if (mgr->mst_state && mgr->mst_primary) + drm_dp_mst_topology_mgr_invalidate_mstb(mgr->mst_primary); + mutex_unlock(&mgr->lock); } EXPORT_SYMBOL(drm_dp_mst_topology_mgr_suspend); /** * drm_dp_mst_topology_mgr_resume() - resume the MST manager * @mgr: manager to resume + * @sync: whether or not to perform topology reprobing synchronously * * This will fetch DPCD and see if the device is still there, * if it is, it will rewrite the MSTM control bits, and return. * - * if the device fails this returns -1, and the driver should do + * If the device fails this returns -1, and the driver should do * a full MST reprobe, in case we were undocked. + * + * During system resume (where it is assumed that the driver will be calling + * drm_atomic_helper_resume()) this function should be called beforehand with + * @sync set to true. In contexts like runtime resume where the driver is not + * expected to be calling drm_atomic_helper_resume(), this function should be + * called with @sync set to false in order to avoid deadlocking. + * + * Returns: -1 if the MST topology was removed while we were suspended, 0 + * otherwise. */ -int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr) +int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr, + bool sync) { int ret; u8 guid[16]; @@ -3241,8 +3344,19 @@ int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr) } drm_dp_check_mstb_guid(mgr->mst_primary, guid); + /* + * For the final step of resuming the topology, we need to bring the + * state of our in-memory topology back into sync with reality. So, + * restart the probing process as if we're probing a new hub + */ + queue_work(system_long_wq, &mgr->work); mutex_unlock(&mgr->lock); + if (sync) { + DRM_DEBUG_KMS("Waiting for link probe work to finish re-syncing topology...\n"); + flush_work(&mgr->work); + } + return 0; out_fail: diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 5eeafa45831a..403b593a3eb4 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -7625,7 +7625,8 @@ void intel_dp_mst_resume(struct drm_i915_private *dev_priv) if (!intel_dp->can_mst) continue; - ret = drm_dp_mst_topology_mgr_resume(&intel_dp->mst_mgr); + ret = drm_dp_mst_topology_mgr_resume(&intel_dp->mst_mgr, + true); if (ret) { intel_dp->is_mst = false; drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index a9d6aa110cfd..549486f1d937 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -1309,14 +1309,14 @@ nv50_mstm_fini(struct nv50_mstm *mstm) } static void -nv50_mstm_init(struct nv50_mstm *mstm) +nv50_mstm_init(struct nv50_mstm *mstm, bool runtime) { int ret; if (!mstm || !mstm->mgr.mst_state) return; - ret = drm_dp_mst_topology_mgr_resume(&mstm->mgr); + ret = drm_dp_mst_topology_mgr_resume(&mstm->mgr, !runtime); if (ret == -1) { drm_dp_mst_topology_mgr_set_mst(&mstm->mgr, false); drm_kms_helper_hotplug_event(mstm->mgr.dev); @@ -2263,7 +2263,7 @@ nv50_display_init(struct drm_device *dev, bool resume, bool runtime) if (encoder->encoder_type != DRM_MODE_ENCODER_DPMST) { struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder); - nv50_mstm_init(nv_encoder->dp.mstm); + nv50_mstm_init(nv_encoder->dp.mstm, runtime); } } diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index fd142db42cb0..144027e27464 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -706,7 +706,8 @@ void drm_dp_mst_dump_topology(struct seq_file *m, void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr); int __must_check -drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr); +drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr, + bool sync); ssize_t drm_dp_mst_dpcd_read(struct drm_dp_aux *aux, unsigned int offset, void *buffer, size_t size); -- cgit v1.2.3 From 12a280c7286857119cf0d88c487f695e3a1c0912 Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Thu, 20 Jun 2019 17:59:25 -0400 Subject: drm/dp_mst: Add topology ref history tracking for debugging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For very subtle mistakes with topology refs, it can be rather difficult to trace them down with the debugging info that we already have. I had one such issue recently while trying to implement suspend/resume reprobing for MST, and ended up coming up with this. Inspired by Chris Wilson's wakeref tracking for i915, this adds a very similar feature to the DP MST helpers, which allows for partial tracking of topology refs for both ports and branch devices. This is a lot less advanced then wakeref tracking: we merely keep a count of all of the spots where a topology ref has been grabbed or dropped, then dump out that history in chronological order when a port or branch device's topology refcount reaches 0. So far, I've found this incredibly useful for debugging topology refcount errors. Since this has the potential to be somewhat slow and loud, we add an expert kernel config option to enable or disable this feature, CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS. Changes since v1: * Don't forget to destroy topology_ref_history_lock Changes since v4: * Correct order of kref_put()/topology_ref_history_unlock - we can't unlock the history after kref_put() since the memory might have been freed by that point * Don't print message on allocation error failures, the kernel already does this for us Changes since v5: * Get rid of some leftover usages of %px * Remove a leftover empty return; statement Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Cc: Daniel Vetter Reviewed-by: Sean Paul Signed-off-by: Lyude Paul Link: https://patchwork.freedesktop.org/patch/msgid/20191022023641.8026-15-lyude@redhat.com --- drivers/gpu/drm/Kconfig | 14 ++ drivers/gpu/drm/drm_dp_mst_topology.c | 234 ++++++++++++++++++++++++++++++++-- include/drm/drm_dp_mst_helper.h | 45 +++++++ 3 files changed, 285 insertions(+), 8 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index caeef11976e8..933b916a99e3 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -93,6 +93,20 @@ config DRM_KMS_FB_HELPER help FBDEV helpers for KMS drivers. +config DRM_DEBUG_DP_MST_TOPOLOGY_REFS + bool "Enable refcount backtrace history in the DP MST helpers" + select STACKDEPOT + depends on DRM_KMS_HELPER + depends on DEBUG_KERNEL + depends on EXPERT + help + Enables debug tracing for topology refs in DRM's DP MST helpers. A + history of each topology reference/dereference will be printed to the + kernel log once a port or branch device's topology refcount reaches 0. + + This has the potential to use a lot of memory and print some very + large kernel messages. If in doubt, say "N". + config DRM_FBDEV_EMULATION bool "Enable legacy fbdev support for your modesetting driver" depends on DRM diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 42eac2a59a4b..0a402d58fc5d 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -28,6 +28,13 @@ #include #include +#if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS) +#include +#include +#include +#include +#endif + #include #include #include @@ -1399,12 +1406,184 @@ drm_dp_mst_put_port_malloc(struct drm_dp_mst_port *port) } EXPORT_SYMBOL(drm_dp_mst_put_port_malloc); +#if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS) + +#define STACK_DEPTH 8 + +static noinline void +__topology_ref_save(struct drm_dp_mst_topology_mgr *mgr, + struct drm_dp_mst_topology_ref_history *history, + enum drm_dp_mst_topology_ref_type type) +{ + struct drm_dp_mst_topology_ref_entry *entry = NULL; + depot_stack_handle_t backtrace; + ulong stack_entries[STACK_DEPTH]; + uint n; + int i; + + n = stack_trace_save(stack_entries, ARRAY_SIZE(stack_entries), 1); + backtrace = stack_depot_save(stack_entries, n, GFP_KERNEL); + if (!backtrace) + return; + + /* Try to find an existing entry for this backtrace */ + for (i = 0; i < history->len; i++) { + if (history->entries[i].backtrace == backtrace) { + entry = &history->entries[i]; + break; + } + } + + /* Otherwise add one */ + if (!entry) { + struct drm_dp_mst_topology_ref_entry *new; + int new_len = history->len + 1; + + new = krealloc(history->entries, sizeof(*new) * new_len, + GFP_KERNEL); + if (!new) + return; + + entry = &new[history->len]; + history->len = new_len; + history->entries = new; + + entry->backtrace = backtrace; + entry->type = type; + entry->count = 0; + } + entry->count++; + entry->ts_nsec = ktime_get_ns(); +} + +static int +topology_ref_history_cmp(const void *a, const void *b) +{ + const struct drm_dp_mst_topology_ref_entry *entry_a = a, *entry_b = b; + + if (entry_a->ts_nsec > entry_b->ts_nsec) + return 1; + else if (entry_a->ts_nsec < entry_b->ts_nsec) + return -1; + else + return 0; +} + +static inline const char * +topology_ref_type_to_str(enum drm_dp_mst_topology_ref_type type) +{ + if (type == DRM_DP_MST_TOPOLOGY_REF_GET) + return "get"; + else + return "put"; +} + +static void +__dump_topology_ref_history(struct drm_dp_mst_topology_ref_history *history, + void *ptr, const char *type_str) +{ + struct drm_printer p = drm_debug_printer(DBG_PREFIX); + char *buf = kzalloc(PAGE_SIZE, GFP_KERNEL); + int i; + + if (!buf) + return; + + if (!history->len) + goto out; + + /* First, sort the list so that it goes from oldest to newest + * reference entry + */ + sort(history->entries, history->len, sizeof(*history->entries), + topology_ref_history_cmp, NULL); + + drm_printf(&p, "%s (%p) topology count reached 0, dumping history:\n", + type_str, ptr); + + for (i = 0; i < history->len; i++) { + const struct drm_dp_mst_topology_ref_entry *entry = + &history->entries[i]; + ulong *entries; + uint nr_entries; + u64 ts_nsec = entry->ts_nsec; + u64 rem_nsec = do_div(ts_nsec, 1000000000); + + nr_entries = stack_depot_fetch(entry->backtrace, &entries); + stack_trace_snprint(buf, PAGE_SIZE, entries, nr_entries, 4); + + drm_printf(&p, " %d %ss (last at %5llu.%06llu):\n%s", + entry->count, + topology_ref_type_to_str(entry->type), + ts_nsec, rem_nsec / 1000, buf); + } + + /* Now free the history, since this is the only time we expose it */ + kfree(history->entries); +out: + kfree(buf); +} + +static __always_inline void +drm_dp_mst_dump_mstb_topology_history(struct drm_dp_mst_branch *mstb) +{ + __dump_topology_ref_history(&mstb->topology_ref_history, mstb, + "MSTB"); +} + +static __always_inline void +drm_dp_mst_dump_port_topology_history(struct drm_dp_mst_port *port) +{ + __dump_topology_ref_history(&port->topology_ref_history, port, + "Port"); +} + +static __always_inline void +save_mstb_topology_ref(struct drm_dp_mst_branch *mstb, + enum drm_dp_mst_topology_ref_type type) +{ + __topology_ref_save(mstb->mgr, &mstb->topology_ref_history, type); +} + +static __always_inline void +save_port_topology_ref(struct drm_dp_mst_port *port, + enum drm_dp_mst_topology_ref_type type) +{ + __topology_ref_save(port->mgr, &port->topology_ref_history, type); +} + +static inline void +topology_ref_history_lock(struct drm_dp_mst_topology_mgr *mgr) +{ + mutex_lock(&mgr->topology_ref_history_lock); +} + +static inline void +topology_ref_history_unlock(struct drm_dp_mst_topology_mgr *mgr) +{ + mutex_unlock(&mgr->topology_ref_history_lock); +} +#else +static inline void +topology_ref_history_lock(struct drm_dp_mst_topology_mgr *mgr) {} +static inline void +topology_ref_history_unlock(struct drm_dp_mst_topology_mgr *mgr) {} +static inline void +drm_dp_mst_dump_mstb_topology_history(struct drm_dp_mst_branch *mstb) {} +static inline void +drm_dp_mst_dump_port_topology_history(struct drm_dp_mst_port *port) {} +#define save_mstb_topology_ref(mstb, type) +#define save_port_topology_ref(port, type) +#endif + static void drm_dp_destroy_mst_branch_device(struct kref *kref) { struct drm_dp_mst_branch *mstb = container_of(kref, struct drm_dp_mst_branch, topology_kref); struct drm_dp_mst_topology_mgr *mgr = mstb->mgr; + drm_dp_mst_dump_mstb_topology_history(mstb); + INIT_LIST_HEAD(&mstb->destroy_next); /* @@ -1442,11 +1621,17 @@ static void drm_dp_destroy_mst_branch_device(struct kref *kref) static int __must_check drm_dp_mst_topology_try_get_mstb(struct drm_dp_mst_branch *mstb) { - int ret = kref_get_unless_zero(&mstb->topology_kref); + int ret; - if (ret) - DRM_DEBUG("mstb %p (%d)\n", mstb, - kref_read(&mstb->topology_kref)); + topology_ref_history_lock(mstb->mgr); + ret = kref_get_unless_zero(&mstb->topology_kref); + if (ret) { + DRM_DEBUG("mstb %p (%d)\n", + mstb, kref_read(&mstb->topology_kref)); + save_mstb_topology_ref(mstb, DRM_DP_MST_TOPOLOGY_REF_GET); + } + + topology_ref_history_unlock(mstb->mgr); return ret; } @@ -1467,9 +1652,14 @@ drm_dp_mst_topology_try_get_mstb(struct drm_dp_mst_branch *mstb) */ static void drm_dp_mst_topology_get_mstb(struct drm_dp_mst_branch *mstb) { + topology_ref_history_lock(mstb->mgr); + + save_mstb_topology_ref(mstb, DRM_DP_MST_TOPOLOGY_REF_GET); WARN_ON(kref_read(&mstb->topology_kref) == 0); kref_get(&mstb->topology_kref); DRM_DEBUG("mstb %p (%d)\n", mstb, kref_read(&mstb->topology_kref)); + + topology_ref_history_unlock(mstb->mgr); } /** @@ -1487,8 +1677,13 @@ static void drm_dp_mst_topology_get_mstb(struct drm_dp_mst_branch *mstb) static void drm_dp_mst_topology_put_mstb(struct drm_dp_mst_branch *mstb) { + topology_ref_history_lock(mstb->mgr); + DRM_DEBUG("mstb %p (%d)\n", mstb, kref_read(&mstb->topology_kref) - 1); + save_mstb_topology_ref(mstb, DRM_DP_MST_TOPOLOGY_REF_PUT); + + topology_ref_history_unlock(mstb->mgr); kref_put(&mstb->topology_kref, drm_dp_destroy_mst_branch_device); } @@ -1498,6 +1693,8 @@ static void drm_dp_destroy_port(struct kref *kref) container_of(kref, struct drm_dp_mst_port, topology_kref); struct drm_dp_mst_topology_mgr *mgr = port->mgr; + drm_dp_mst_dump_port_topology_history(port); + /* There's nothing that needs locking to destroy an input port yet */ if (port->input) { drm_dp_mst_put_port_malloc(port); @@ -1541,12 +1738,17 @@ static void drm_dp_destroy_port(struct kref *kref) static int __must_check drm_dp_mst_topology_try_get_port(struct drm_dp_mst_port *port) { - int ret = kref_get_unless_zero(&port->topology_kref); + int ret; - if (ret) - DRM_DEBUG("port %p (%d)\n", port, - kref_read(&port->topology_kref)); + topology_ref_history_lock(port->mgr); + ret = kref_get_unless_zero(&port->topology_kref); + if (ret) { + DRM_DEBUG("port %p (%d)\n", + port, kref_read(&port->topology_kref)); + save_port_topology_ref(port, DRM_DP_MST_TOPOLOGY_REF_GET); + } + topology_ref_history_unlock(port->mgr); return ret; } @@ -1565,9 +1767,14 @@ drm_dp_mst_topology_try_get_port(struct drm_dp_mst_port *port) */ static void drm_dp_mst_topology_get_port(struct drm_dp_mst_port *port) { + topology_ref_history_lock(port->mgr); + WARN_ON(kref_read(&port->topology_kref) == 0); kref_get(&port->topology_kref); DRM_DEBUG("port %p (%d)\n", port, kref_read(&port->topology_kref)); + save_port_topology_ref(port, DRM_DP_MST_TOPOLOGY_REF_GET); + + topology_ref_history_unlock(port->mgr); } /** @@ -1583,8 +1790,13 @@ static void drm_dp_mst_topology_get_port(struct drm_dp_mst_port *port) */ static void drm_dp_mst_topology_put_port(struct drm_dp_mst_port *port) { + topology_ref_history_lock(port->mgr); + DRM_DEBUG("port %p (%d)\n", port, kref_read(&port->topology_kref) - 1); + save_port_topology_ref(port, DRM_DP_MST_TOPOLOGY_REF_PUT); + + topology_ref_history_unlock(port->mgr); kref_put(&port->topology_kref, drm_dp_destroy_port); } @@ -4578,6 +4790,9 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr, mutex_init(&mgr->delayed_destroy_lock); mutex_init(&mgr->up_req_lock); mutex_init(&mgr->probe_lock); +#if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS) + mutex_init(&mgr->topology_ref_history_lock); +#endif INIT_LIST_HEAD(&mgr->tx_msg_downq); INIT_LIST_HEAD(&mgr->destroy_port_list); INIT_LIST_HEAD(&mgr->destroy_branch_device_list); @@ -4644,6 +4859,9 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr) mutex_destroy(&mgr->lock); mutex_destroy(&mgr->up_req_lock); mutex_destroy(&mgr->probe_lock); +#if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS) + mutex_destroy(&mgr->topology_ref_history_lock); +#endif } EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy); diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index 144027e27464..d5fc90b30487 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -26,6 +26,26 @@ #include #include +#if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS) +#include +#include + +enum drm_dp_mst_topology_ref_type { + DRM_DP_MST_TOPOLOGY_REF_GET, + DRM_DP_MST_TOPOLOGY_REF_PUT, +}; + +struct drm_dp_mst_topology_ref_history { + struct drm_dp_mst_topology_ref_entry { + enum drm_dp_mst_topology_ref_type type; + int count; + ktime_t ts_nsec; + depot_stack_handle_t backtrace; + } *entries; + int len; +}; +#endif /* IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS) */ + struct drm_dp_mst_branch; /** @@ -89,6 +109,14 @@ struct drm_dp_mst_port { */ struct kref malloc_kref; +#if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS) + /** + * @topology_ref_history: A history of each topology + * reference/dereference. See CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS. + */ + struct drm_dp_mst_topology_ref_history topology_ref_history; +#endif + u8 port_num; bool input; bool mcs; @@ -162,6 +190,14 @@ struct drm_dp_mst_branch { */ struct kref malloc_kref; +#if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS) + /** + * @topology_ref_history: A history of each topology + * reference/dereference. See CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS. + */ + struct drm_dp_mst_topology_ref_history topology_ref_history; +#endif + /** * @destroy_next: linked-list entry used by * drm_dp_delayed_destroy_work() @@ -647,6 +683,15 @@ struct drm_dp_mst_topology_mgr { * transmissions. */ struct work_struct up_req_work; + +#if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS) + /** + * @topology_ref_history_lock: protects + * &drm_dp_mst_port.topology_ref_history and + * &drm_dp_mst_branch.topology_ref_history. + */ + struct mutex topology_ref_history_lock; +#endif }; int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr, -- cgit v1.2.3 From 68acde7629d75d460760a3124c52f358eecc6d26 Mon Sep 17 00:00:00 2001 From: Chenwandun Date: Thu, 31 Oct 2019 17:43:49 +0800 Subject: drm/dp_mst: fix gcc compile error drivers/gpu/drm/drm_dp_mst_topology.c: In function __topology_ref_save: drivers/gpu/drm/drm_dp_mst_topology.c:1424:6: error: implicit declaration of function stack_trace_save; did you mean stack_depot_save? [-Werror=implicit-function-declaration] n = stack_trace_save(stack_entries, ARRAY_SIZE(stack_entries), 1); ^~~~~~~~~~~~~~~~ stack_depot_save drivers/gpu/drm/drm_dp_mst_topology.c: In function __dump_topology_ref_history: drivers/gpu/drm/drm_dp_mst_topology.c:1513:3: error: implicit declaration of function stack_trace_snprint; did you mean acpi_trace_point? [-Werror=implicit-function-declaration] stack_trace_snprint(buf, PAGE_SIZE, entries, nr_entries, 4); ^~~~~~~~~~~~~~~~~~~ acpi_trace_point stack_trace_save and stack_trace_snprint are declared in , so there is need to include it, and is already included by practices, so just replace by . Signed-off-by: Chenwandun Reviewed-by: Lyude Paul Acked-by: Daniel Vetter Signed-off-by: Dave Airlie Link: https://patchwork.freedesktop.org/patch/msgid/1572515029-42087-1-git-send-email-chenwandun@huawei.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 85bef73a6763..11adc4b6ccfe 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -29,7 +29,7 @@ #include #if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS) -#include +#include #include #include #include -- cgit v1.2.3 From b149cbfeecaa2b2870d2c4f878382a024e6e4975 Mon Sep 17 00:00:00 2001 From: Sean Paul Date: Wed, 6 Nov 2019 14:41:15 -0500 Subject: drm/mst: Fix up u64 division MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change rem_nsec to u32 since that's what do_div returns, this avoids the u64 divide in the drm_print args. Changes in v2: - Instead of doing do_div in drm_print, make rem_nsec u32 (Ville) Link to v1: https://patchwork.freedesktop.org/patch/msgid/20191106173622.15573-1-sean@poorly.run Fixes: 12a280c72868 ("drm/dp_mst: Add topology ref history tracking for debugging") Cc: Juston Li Cc: Imre Deak Cc: Ville Syrjälä Cc: Harry Wentland Cc: Daniel Vetter Cc: Sean Paul Cc: Lyude Paul Cc: Maarten Lankhorst Cc: Maxime Ripard Cc: David Airlie Cc: Daniel Vetter Cc: dri-devel@lists.freedesktop.org Reviewed-by: Ville Syrjälä Signed-off-by: Sean Paul Link: https://patchwork.freedesktop.org/patch/msgid/20191106194121.164458-1-sean@poorly.run --- drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 11adc4b6ccfe..ae5809a1f19a 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1507,12 +1507,12 @@ __dump_topology_ref_history(struct drm_dp_mst_topology_ref_history *history, ulong *entries; uint nr_entries; u64 ts_nsec = entry->ts_nsec; - u64 rem_nsec = do_div(ts_nsec, 1000000000); + u32 rem_nsec = do_div(ts_nsec, 1000000000); nr_entries = stack_depot_fetch(entry->backtrace, &entries); stack_trace_snprint(buf, PAGE_SIZE, entries, nr_entries, 4); - drm_printf(&p, " %d %ss (last at %5llu.%06llu):\n%s", + drm_printf(&p, " %d %ss (last at %5llu.%06u):\n%s", entry->count, topology_ref_type_to_str(entry->type), ts_nsec, rem_nsec / 1000, buf); -- cgit v1.2.3 From e5a6ca27eb72c67533ddfc11c06df84beaa167fa Mon Sep 17 00:00:00 2001 From: Wayne Lin Date: Tue, 3 Dec 2019 12:24:23 +0800 Subject: drm/dp_mst: Correct the bug in drm_dp_update_payload_part1() [Why] If the payload_state is DP_PAYLOAD_DELETE_LOCAL in series, current code doesn't delete the payload at current index and just move the index to next one after shuffling payloads. [How] Drop the i++ increasing part in for loop head and decide whether to increase the index or not according to payload_state of current payload. Changes since v1: * Refine the code to have it easy reading * Amend the commit message to meet the way code is modified now. Signed-off-by: Wayne Lin Reviewed-by: Lyude Paul Fixes: 706246c761dd ("drm/dp_mst: Refactor drm_dp_update_payload_part1()") Cc: Daniel Vetter Cc: Juston Li Cc: Maarten Lankhorst Cc: Maxime Ripard Cc: Sean Paul Cc: David Airlie Cc: Daniel Vetter Cc: dri-devel@lists.freedesktop.org Cc: # v5.1+ [Added cc for stable] Signed-off-by: Lyude Paul Link: https://patchwork.freedesktop.org/patch/msgid/20191203042423.5961-1-Wayne.Lin@amd.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'drivers/gpu/drm/drm_dp_mst_topology.c') diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index ae5809a1f19a..273dd80fabf3 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3176,9 +3176,11 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr) drm_dp_mst_topology_put_port(port); } - for (i = 0; i < mgr->max_payloads; i++) { - if (mgr->payloads[i].payload_state != DP_PAYLOAD_DELETE_LOCAL) + for (i = 0; i < mgr->max_payloads; /* do nothing */) { + if (mgr->payloads[i].payload_state != DP_PAYLOAD_DELETE_LOCAL) { + i++; continue; + } DRM_DEBUG_KMS("removing payload %d\n", i); for (j = i; j < mgr->max_payloads - 1; j++) { -- cgit v1.2.3