Re: [PATCH v2 07/27] drm/dp_mst: Add sideband down request tracing + selftests
From: Jani Nikula
Date: Tue Sep 10 2019 - 05:01:30 EST
On Tue, 03 Sep 2019, Lyude Paul <lyude@xxxxxxxxxx> 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 <juston.li@xxxxxxxxx>
> Cc: Imre Deak <imre.deak@xxxxxxxxx>
> Cc: Ville SyrjÃlà <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Harry Wentland <hwentlan@xxxxxxx>
> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>
> ---
> 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 <lyude@xxxxxxxxxx>
> + */
> +
> +#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