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 <[email protected]>
Cc: Imre Deak <[email protected]>
Cc: Ville Syrjälä <[email protected]>
Cc: Harry Wentland <[email protected]>
Reviewed-by: Daniel Vetter <[email protected]>
Signed-off-by: Lyude Paul <[email protected]>
---
drivers/gpu/drm/drm_dp_mst_topology.c | 309 +++++++++++++++++-
.../gpu/drm/drm_dp_mst_topology_internal.h | 24 ++
.../gpu/drm/selftests/drm_modeset_selftests.h | 1 +
.../drm/selftests/test-drm_dp_mst_helper.c | 204 ++++++++++++
.../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
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 1c862749cb63..f5f1d8b50fb6 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -37,6 +37,7 @@
#include <drm/drm_probe_helper.h>
#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, "<failed to decode sideband req: %d>\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;
@@ -2013,8 +2304,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;
}
@@ -2076,6 +2370,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 <[email protected]>
+ */
+
+#ifndef _DRM_DP_MST_HELPER_INTERNAL_H_
+#define _DRM_DP_MST_HELPER_INTERNAL_H_
+
+#include <drm/drm_dp_mst_helper.h>
+
+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 <drm/drm_dp_mst_helper.h>
#include <drm/drm_print.h>
+#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 4a4507fe928d..5423a8adda78 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -293,7 +293,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;
--
2.21.0
On Tue, 03 Sep 2019, Lyude Paul <[email protected]> wrote:
> 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 <[email protected]>
> Cc: Imre Deak <[email protected]>
> Cc: Ville Syrjälä <[email protected]>
> Cc: Harry Wentland <[email protected]>
> Reviewed-by: Daniel Vetter <[email protected]>
> Signed-off-by: Lyude Paul <[email protected]>
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 309 +++++++++++++++++-
> .../gpu/drm/drm_dp_mst_topology_internal.h | 24 ++
> .../gpu/drm/selftests/drm_modeset_selftests.h | 1 +
> .../drm/selftests/test-drm_dp_mst_helper.c | 204 ++++++++++++
> .../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
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 1c862749cb63..f5f1d8b50fb6 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -37,6 +37,7 @@
> #include <drm/drm_probe_helper.h>
>
> #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, "<failed to decode sideband req: %d>\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;
> @@ -2013,8 +2304,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;
> }
>
> @@ -2076,6 +2370,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 <[email protected]>
> + */
> +
> +#ifndef _DRM_DP_MST_HELPER_INTERNAL_H_
> +#define _DRM_DP_MST_HELPER_INTERNAL_H_
> +
> +#include <drm/drm_dp_mst_helper.h>
> +
> +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 <drm/drm_dp_mst_helper.h>
> #include <drm/drm_print.h>
>
> +#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);
Is this the sole reason of adding drm_err_printer()? Where do you intend
to take error printing next, in general? Why isn't DRM_ERROR() the right
thing to do here?
It's just that I think debug and error printing from drm is a mess, and
instead of cleaning it up, we seem to be piling on new stuff without
direction.
BR,
Jani.
> + 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 4a4507fe928d..5423a8adda78 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -293,7 +293,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;
--
Jani Nikula, Intel Open Source Graphics Center