*sigh* finally have some time to go through these reviews

jfyi: I realized after looking over this patch that it's not actually needed - I had been planning on using drm_dp_dump_link_address() for other things, but ended up deciding to make the final plan to use something that dumps into a format that's identical to the one we're using for dumping DOWN requests. IMHO though, this patch does make things look nicer so I'll probably keep it.

Assuming I can still count your r-b as valid with a change to the commit description?

On Thu, 2019-08-08 at 21:53 +0200, Daniel Vetter wrote:

On Wed, Jul 17, 2019 at 09:42:24PM -0400, Lyude Paul wrote:

...

Since we're about to be calling this from multiple places. Also it makes things easier to read!

Cc: Juston Li juston.li@intel.com Cc: Imre Deak imre.deak@intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Harry Wentland hwentlan@amd.com Signed-off-by: Lyude Paul lyude@redhat.com

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

...

---

drivers/gpu/drm/drm_dp_mst_topology.c | 35 ++++++++++++++++++--------- 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 0984b9a34d55..998081b9b205 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -2013,6 +2013,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) { @@ -2038,18 +2060,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);

-- 2.21.0

On Tue, 2019-08-13 at 15:00 +0200, Daniel Vetter wrote:

On Wed, Jul 17, 2019 at 09:42:25PM -0400, Lyude Paul wrote:

...

Currently we remove MST branch devices from the in-memory topology immediately when their topology refcount reaches 0. This works just fine at the moment, but since we're about to add suspend/resume reprobing for MST topologies we're going to need to be able to travel through the topology and drop topology refs on branch devices while holding mgr->mutex. Since we currently can't do this due to the circular locking dependencies that would incur, move all of the actual work for drm_dp_destroy_mst_branch_device() into drm_dp_destroy_connector_work() so we can drop topology refs on MSTBs in any locking context.

Would be good to point at where exactly the problem is here, maybe also mentioned the exact future patch that causes the problem. I did go look around a bit, but didn't spot it.

Ah, I didn't do a great job of explaining whoops! Basically, the issue is that when reprobing the state during suspend/resume, there's of course going to be a chance that any MST ports in the topology could have had their PDT changed while we were suspended. This in turn means that while we're iterating through each mstb's ports in drm_dp_send_link_address() we want to be able to remove MSTBs from while under &mgr->lock, something we can't do without handling MSTB destruction in another thread. I'll mention this in the next version of this patch

...

Cc: Juston Li juston.li@intel.com Cc: Imre Deak imre.deak@intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Harry Wentland hwentlan@amd.com Signed-off-by: Lyude Paul lyude@redhat.com

---

drivers/gpu/drm/drm_dp_mst_topology.c | 121 +++++++++++++++++--------- include/drm/drm_dp_mst_helper.h | 10 +++ 2 files changed, 90 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 998081b9b205..d7c3d9233834 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1108,34 +1108,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);

• if (wake_tx)

• wake_up_all(&mstb->mgr->tx_waitq);
  

• INIT_LIST_HEAD(&mstb->destroy_next);

• 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->destroy_connector_lock);
• list_add(&mstb->destroy_next, &mgr->destroy_branch_device_list);
• mutex_unlock(&mgr->destroy_connector_lock);
• schedule_work(&mgr->destroy_connector_work);

}

/** @@ -3618,10 +3601,56 @@ static void drm_dp_tx_work(struct work_struct *work) mutex_unlock(&mgr->qlock); }

+static inline void +drm_dp_finish_destroy_port(struct drm_dp_mst_port *port)

Bikeshed: I'd call this _delayed_destroy, I think that makes a bit clearer why there's 2 stages to destroying stuff.

...

+{

• INIT_LIST_HEAD(&port->next);
• 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_finish_destroy_mst_branch_device(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_destroy_connector_work(struct work_struct *work) { 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; /*
  • Not a regular list traverse as we have to drop the destroy

@@ -3629,24 +3658,33 @@ static void drm_dp_destroy_connector_work(struct work_struct *work) * ordering between this lock and the config mutex. */ for (;;) {

• struct drm_dp_mst_branch *mstb = NULL;
  

• struct drm_dp_mst_port *port = NULL;
  

• /* Destroy any MSTBs first, and then their ports second */
  

  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;
  

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

• } else {
  

• 	port = list_first_entry_or_null(&mgr-
  

...

destroy_connector_list,

• 					struct
  

drm_dp_mst_port,

• 					next);
  

• 	if (port)
  

• 		list_del(&port->next);
  

  }

Control flow looks rather awkward here. I'd do either two loops, or if you want to have just one, rename it to ->delayed_destroy_list and have a ->delayed_destry_cb next to it?

Cheers, Daniel

...

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

• port->pdt = DP_PEER_DEVICE_NONE;
  

• drm_dp_mst_put_port_malloc(port);
  

• send_hotplug = true;
  

• if (mstb) {
  

• 	drm_dp_finish_destroy_mst_branch_device(mstb);
  

• } else if (port) {
  

• 	drm_dp_finish_destroy_port(port);
  

• 	send_hotplug = true;
  

• } else {
  

• 	break;
  

• }
  

  } if (send_hotplug) drm_kms_helper_hotplug_event(mgr->dev);

@@ -3840,6 +3878,7 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr, mutex_init(&mgr->destroy_connector_lock); INIT_LIST_HEAD(&mgr->tx_msg_downq); INIT_LIST_HEAD(&mgr->destroy_connector_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); diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index 8c97a5f92c47..c01f3ea72756 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_destroy_connector_work()
  

• */
  

• struct list_head destroy_next;
• u8 rad[8]; u8 lct; int num_ports;

@@ -578,6 +584,10 @@ struct drm_dp_mst_topology_mgr { * @destroy_connector_list: List of to be destroyed connectors. */ struct list_head destroy_connector_list;

• /**

• * @destroy_branch_device_list: List of to be destroyed branch devices
  

• */
  

• struct list_head destroy_branch_device_list; /**
  • @destroy_connector_lock: Protects @connector_list.
  */

-- 2.21.0

On Wed, Jul 17, 2019 at 09:42:26PM -0400, Lyude Paul wrote:

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 juston.li@intel.com Cc: Imre Deak imre.deak@intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Harry Wentland hwentlan@amd.com Signed-off-by: Lyude Paul lyude@redhat.com

More official unit tests, yay!

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/drm_dp_mst_topology.c | 27 -------------- drivers/gpu/drm/selftests/Makefile | 2 +- .../gpu/drm/selftests/drm_modeset_selftests.h | 1 + .../drm/selftests/test-drm_dp_mst_helper.c | 36 +++++++++++++++++++ .../drm/selftests/test-drm_modeset_common.h | 1 + 5 files changed, 39 insertions(+), 28 deletions(-) create mode 100644 drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index d7c3d9233834..9e382117896d 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -45,7 +45,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);

@@ -3439,30 +3438,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) { @@ -3898,8 +3873,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..51b2486ec917 --- /dev/null +++ b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c @@ -0,0 +1,36 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*

• • Test cases for for the DRM DP MST helpers
• */

+#define pr_fmt(fmt) "drm_dp_mst_helper: " fmt

+#include <drm/drm_dp_mst_helper.h> +#include <drm/drm_print.h>

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

2.21.0

On Wed, Jul 17, 2019 at 09:42:27PM -0400, Lyude Paul wrote:

A simple convienence function that returns a drm_printer which prints using pr_err()

Cc: Juston Li juston.li@intel.com Cc: Imre Deak imre.deak@intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Harry Wentland hwentlan@amd.com Signed-off-by: Lyude Paul lyude@redhat.com

---

drivers/gpu/drm/drm_print.c | 6 ++++++ include/drm/drm_print.h | 17 +++++++++++++++++ 2 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c index a17c8a14dba4..6112be358769 100644 --- a/drivers/gpu/drm/drm_print.c +++ b/drivers/gpu/drm/drm_print.c @@ -147,6 +147,12 @@ void __drm_printfn_debug(struct drm_printer *p, struct va_format *vaf) } EXPORT_SYMBOL(__drm_printfn_debug);

+void __drm_printfn_err(struct drm_printer *p, struct va_format *vaf) +{

• pr_err("%s %pV", p->prefix, vaf);

DRM printing is a huge bikeshad (or tire fire?). We can't call DRM_ERROR, but for consistency mabye emulate the layout?

Either way: Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

+} +EXPORT_SYMBOL(__drm_printfn_err);

/**

• drm_puts - print a const string to a &drm_printer stream
• @p: the &drm printer

diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h index a5d6f2f3e430..112165d3195d 100644 --- a/include/drm/drm_print.h +++ b/include/drm/drm_print.h @@ -83,6 +83,7 @@ void __drm_printfn_seq_file(struct drm_printer *p, struct va_format *vaf); void __drm_puts_seq_file(struct drm_printer *p, const char *str); void __drm_printfn_info(struct drm_printer *p, struct va_format *vaf); void __drm_printfn_debug(struct drm_printer *p, struct va_format *vaf); +void __drm_printfn_err(struct drm_printer *p, struct va_format *vaf);

__printf(2, 3) void drm_printf(struct drm_printer *p, const char *f, ...); @@ -227,6 +228,22 @@ static inline struct drm_printer drm_debug_printer(const char *prefix) return p; }

+/**

• • drm_err_printer - construct a &drm_printer that outputs to pr_err()
• • @prefix: debug output prefix
• • RETURNS:
• • The &drm_printer object
• */

+static inline struct drm_printer drm_err_printer(const char *prefix) +{

• struct drm_printer p = {

• .printfn = __drm_printfn_err,
  

• .prefix = prefix
  

• };
• return p;

+}

/*

• The following categories are defined:

-- 2.21.0

On Wed, Jul 17, 2019 at 09:42:28PM -0400, Lyude Paul 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. Note that with this commit, we'll be dumping out sideband requests under both of the following conditions:

• When the user has enabled DRM_UT_DP output, of course.
• When the user has enabled DRM_UT_KMS or DRM_UT_DP, and a sideband request has failed in some way. This will allow for developers to get a basic idea of what's actually happening with failed modesets on MST, without needing to have DRM_UT_DP explicitly enabled.

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!

Cc: Juston Li juston.li@intel.com Cc: Imre Deak imre.deak@intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Harry Wentland hwentlan@amd.com Signed-off-by: Lyude Paul lyude@redhat.com

---

drivers/gpu/drm/drm_dp_mst_topology.c | 308 +++++++++++++++++- .../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 | 212 ++++++++++++ .../drm/selftests/test-drm_modeset_common.h | 1 + include/drm/drm_dp_mst_helper.h | 2 +- 6 files changed, 543 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 9e382117896d..defc5e09fb9a 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -36,6 +36,8 @@ #include <drm/drm_print.h> #include <drm/drm_probe_helper.h>

+#include "drm_dp_mst_topology_internal.h"

/**

• DOC: dp mst helper

@@ -68,6 +70,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) @@ -124,6 +128,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
  

• */
  

lct *=2;

missing here? And yeah the todo would be sweet, but quite a bit more typing I guess.

• 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) @@ -256,8 +297,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; @@ -362,6 +404,249 @@ 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:

OCD warning: the enum isn't ordered the same way here and in drm_dp_encode_sideband_req().

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

• 	a->vcpi = buf[++idx] & 0x7f;
  

Complain here if 0x80 is set?

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

• req->u.query_payload.vcpi = buf[++idx] & 0x7f;
  

Same here for the highest bit?

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

But if we go really strict on validation we'd need to make sure we don't walk past raw->cur_len ... probably not worth it?

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

• }

For safety: Should we validate cur_len here? Or return it, and let the caller deal with any mismatches.

But I'm kinda leaning towards no safety here, since it's not dealing with any replies we get from the hw/externally, which might be used for attacking us. If we later on have the same thing but for drm_dp_sideband_parse_reply() then a lot more fun.

• 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) { @@ -893,6 +1178,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 | DRM_UT_KMS))) {

Hm I'd only check for DRM_UT_DP here.

• struct drm_printer p = drm_debug_printer(DBG_PREFIX);
  

• drm_dp_mst_dump_sideband_msg_tx(&p, txmsg);
  

• } mutex_unlock(&mgr->qlock);

  return ret;

@@ -1927,8 +2217,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 (ret && unlikely(drm_debug & (DRM_UT_DP | DRM_UT_KMS))) {

Same.

• 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; }

@@ -1990,6 +2283,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)) {

Like you do here.

• 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@redhat.com>
    

• */

+#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 51b2486ec917..ceca89babd65 100644 --- a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c +++ b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c @@ -8,6 +8,7 @@ #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) @@ -34,3 +35,214 @@ 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) {
• case DP_ENUM_PATH_RESOURCES:
• case DP_POWER_UP_PHY:
• case DP_POWER_DOWN_PHY:
• case DP_ALLOCATE_PAYLOAD:
• case DP_QUERY_PAYLOAD:
• case DP_REMOTE_DPCD_READ:

• return memcmp(in, out, sizeof(*in)) == 0;
  

Just memcmp the entire thing for everyrone (we assume kzalloc anyway), and then only have the additional checks for the few cases we need it? Would slightly reduce the code.

• 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

• }
• return true;

+}

+static int +__sideband_msg_req_encode_decode(int line,

• 		 struct drm_dp_sideband_msg_req_body *in,
  

• 		 struct drm_dp_sideband_msg_req_body *out)
  

+{

• struct drm_printer p = drm_err_printer(pr_fmt());
• struct drm_dp_sideband_msg_tx txmsg;
• int i, ret;
• bool eq;
• drm_dp_encode_sideband_req(in, &txmsg);
• ret = drm_dp_decode_sideband_req(&txmsg, out);
• if (ret < 0) {

• pr_err(pr_fmt("Failed to decode sideband request @ line %d: %d\n"),
  

•        line, ret);
  

• return ret;
  

• }
• eq = sideband_msg_req_equal(in, out);
• if (!eq) {

• pr_err(pr_fmt("Encode/decode @ line %d failed, expected:\n"),
  

•        line);
  

• drm_dp_dump_sideband_msg_req_body(in, 1, &p);
  

• pr_err(pr_fmt("Got:\n"));
  

• drm_dp_dump_sideband_msg_req_body(out, 1, &p);
  

• }
• 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));
• /* Clear the output entirely */
• memset(out, 0, sizeof(*out));
• return eq ? 0 : -EINVAL;

+}

+int igt_dp_mst_sideband_msg_req_decode(void *unused) +{

• struct drm_dp_sideband_msg_req_body in = { 0 }, out = { 0 };
• u8 data[] = { 0xff, 0x0, 0xdd };
• int i;

+#define DO_TEST() \

• do { \

• if (__sideband_msg_req_encode_decode(__LINE__, &in, &out)) \
  

• 	return -EINVAL;                                    \
  

• } while (0)

I had a tiny wtf moment here until I realized this is a macro ... maybe put the do on the first line and indent everything? Pure bikeshed to help the blind ...

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

+}

Extremely nice, more unit tests ftw!

With the nits somehow figured out: Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

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 c01f3ea72756..4c8d177e83e5 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, 2019-08-13 at 16:50 +0200, Daniel Vetter wrote:

On Wed, Jul 17, 2019 at 09:42:28PM -0400, Lyude Paul 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. Note that with this commit, we'll be dumping out sideband requests under both of the following conditions:

• When the user has enabled DRM_UT_DP output, of course.
• When the user has enabled DRM_UT_KMS or DRM_UT_DP, and a sideband request has failed in some way. This will allow for developers to get a basic idea of what's actually happening with failed modesets on MST, without needing to have DRM_UT_DP explicitly enabled.

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!

Cc: Juston Li juston.li@intel.com Cc: Imre Deak imre.deak@intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Harry Wentland hwentlan@amd.com Signed-off-by: Lyude Paul lyude@redhat.com

---

drivers/gpu/drm/drm_dp_mst_topology.c | 308 +++++++++++++++++- .../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 | 212 ++++++++++++ .../drm/selftests/test-drm_modeset_common.h | 1 + include/drm/drm_dp_mst_helper.h | 2 +- 6 files changed, 543 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 9e382117896d..defc5e09fb9a 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -36,6 +36,8 @@ #include <drm/drm_print.h> #include <drm/drm_probe_helper.h>

+#include "drm_dp_mst_topology_internal.h"

/**

• DOC: dp mst helper

@@ -68,6 +70,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) @@ -124,6 +128,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
  

• */
  

lct *=2;

missing here? And yeah the todo would be sweet, but quite a bit more typing I guess.

...

• 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) @@ -256,8 +297,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; @@ -362,6 +404,249 @@ 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:

OCD warning: the enum isn't ordered the same way here and in drm_dp_encode_sideband_req().

...

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

• 	a->vcpi = buf[++idx] & 0x7f;
  

Complain here if 0x80 is set?

...

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

• req->u.query_payload.vcpi = buf[++idx] & 0x7f;
  

Same here for the highest bit?

...

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

But if we go really strict on validation we'd need to make sure we don't walk past raw->cur_len ... probably not worth it?

...

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

• }

For safety: Should we validate cur_len here? Or return it, and let the caller deal with any mismatches.

But I'm kinda leaning towards no safety here, since it's not dealing with any replies we get from the hw/externally, which might be used for attacking us. If we later on have the same thing but for drm_dp_sideband_parse_reply() then a lot more fun.

...

• 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) { @@ -893,6 +1178,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 | DRM_UT_KMS))) {

Hm I'd only check for DRM_UT_DP here.

...

• struct drm_printer p = drm_debug_printer(DBG_PREFIX);
  

• drm_dp_mst_dump_sideband_msg_tx(&p, txmsg);
  

• } mutex_unlock(&mgr->qlock);

  return ret;

@@ -1927,8 +2217,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 (ret && unlikely(drm_debug & (DRM_UT_DP | DRM_UT_KMS))) {

Same.

...

• 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; }

@@ -1990,6 +2283,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)) {

Like you do here.

...

• 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@redhat.com>
    

• */

+#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 51b2486ec917..ceca89babd65 100644 --- a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c +++ b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c @@ -8,6 +8,7 @@ #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) @@ -34,3 +35,214 @@ 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) {
• case DP_ENUM_PATH_RESOURCES:
• case DP_POWER_UP_PHY:
• case DP_POWER_DOWN_PHY:
• case DP_ALLOCATE_PAYLOAD:
• case DP_QUERY_PAYLOAD:
• case DP_REMOTE_DPCD_READ:

• return memcmp(in, out, sizeof(*in)) == 0;
  

Just memcmp the entire thing for everyrone (we assume kzalloc anyway), and then only have the additional checks for the few cases we need it? Would slightly reduce the code.

Just realized also - this wouldn't work: remember that I2C_READ, DPCD_WRITE, and I2C_WRITE contain pointers which of course will be different even if the contents are the same. Unfortunately I don't think there's really a good solution for this, unless we want to just remove the pointers and store things within the down request struct itself

...

• 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

• }
• return true;

+}

+static int +__sideband_msg_req_encode_decode(int line,

• 		 struct drm_dp_sideband_msg_req_body *in,
  

• 		 struct drm_dp_sideband_msg_req_body *out)
  

+{

• struct drm_printer p = drm_err_printer(pr_fmt());
• struct drm_dp_sideband_msg_tx txmsg;
• int i, ret;
• bool eq;
• drm_dp_encode_sideband_req(in, &txmsg);
• ret = drm_dp_decode_sideband_req(&txmsg, out);
• if (ret < 0) {

• pr_err(pr_fmt("Failed to decode sideband request @ line %d:
  

%d\n"),

•        line, ret);
  

• return ret;
  

• }
• eq = sideband_msg_req_equal(in, out);
• if (!eq) {

• pr_err(pr_fmt("Encode/decode @ line %d failed, expected:\n"),
  

•        line);
  

• drm_dp_dump_sideband_msg_req_body(in, 1, &p);
  

• pr_err(pr_fmt("Got:\n"));
  

• drm_dp_dump_sideband_msg_req_body(out, 1, &p);
  

• }
• 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));
• /* Clear the output entirely */
• memset(out, 0, sizeof(*out));
• return eq ? 0 : -EINVAL;

+}

+int igt_dp_mst_sideband_msg_req_decode(void *unused) +{

• struct drm_dp_sideband_msg_req_body in = { 0 }, out = { 0 };
• u8 data[] = { 0xff, 0x0, 0xdd };
• int i;

+#define DO_TEST() \

• do { \

• if (__sideband_msg_req_encode_decode(__LINE__, &in, &out)) \
  

• 	return -EINVAL;                                    \
  

• } while (0)

I had a tiny wtf moment here until I realized this is a macro ... maybe put the do on the first line and indent everything? Pure bikeshed to help the blind ...

...

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

+}

Extremely nice, more unit tests ftw!

With the nits somehow figured out: Reviewed-by: Daniel Vetter < daniel.vetter@ffwll.ch>

...

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 c01f3ea72756..4c8d177e83e5 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

Two things below

On Tue, 2019-08-27 at 19:15 +0200, Daniel Vetter wrote:

On Tue, Aug 27, 2019 at 12:49:17PM -0400, Lyude Paul wrote:

...

On Tue, 2019-08-13 at 16:50 +0200, Daniel Vetter wrote:

...

On Wed, Jul 17, 2019 at 09:42:28PM -0400, Lyude Paul 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. Note that with this commit, we'll be dumping out sideband requests under both of the following conditions:

• When the user has enabled DRM_UT_DP output, of course.
• When the user has enabled DRM_UT_KMS or DRM_UT_DP, and a sideband request has failed in some way. This will allow for developers to

get a basic idea of what's actually happening with failed modesets on MST, without needing to have DRM_UT_DP explicitly enabled.

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!

Cc: Juston Li juston.li@intel.com Cc: Imre Deak imre.deak@intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Harry Wentland hwentlan@amd.com Signed-off-by: Lyude Paul lyude@redhat.com

---

drivers/gpu/drm/drm_dp_mst_topology.c | 308 +++++++++++++++++- .../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 | 212 ++++++++++++ .../drm/selftests/test-drm_modeset_common.h | 1 + include/drm/drm_dp_mst_helper.h | 2 +- 6 files changed, 543 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 9e382117896d..defc5e09fb9a 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -36,6 +36,8 @@ #include <drm/drm_print.h> #include <drm/drm_probe_helper.h>

+#include "drm_dp_mst_topology_internal.h"

/**

• DOC: dp mst helper

@@ -68,6 +70,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) @@ -124,6 +128,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
  

• */
  

lct *=2;

missing here? And yeah the todo would be sweet, but quite a bit more typing I guess.

...

• 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) @@ -256,8 +297,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; @@ -362,6 +404,249 @@ 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:

OCD warning: the enum isn't ordered the same way here and in drm_dp_encode_sideband_req().

...

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

• 	a->vcpi = buf[++idx] & 0x7f;
  

Complain here if 0x80 is set?

...

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

• req->u.query_payload.vcpi = buf[++idx] & 0x7f;
  

Same here for the highest bit?

...

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

But if we go really strict on validation we'd need to make sure we don't walk past raw->cur_len ... probably not worth it?

...

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

• }

For safety: Should we validate cur_len here? Or return it, and let the caller deal with any mismatches.

But I'm kinda leaning towards no safety here, since it's not dealing with any replies we get from the hw/externally, which might be used for attacking us. If we later on have the same thing but for drm_dp_sideband_parse_reply() then a lot more fun.

...

• 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) { @@ -893,6 +1178,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 |

DRM_UT_KMS))) {

Hm I'd only check for DRM_UT_DP here.

Was going to go ahead with this, but realized it might not be a great idea. One of the reasons I went with DRM_UT_DP | DRM_UT_KMS here is because generally, I'd assume that (this is the case for me at least) DRM_UT_DP serves as a way to trace what's going on with DP, and DRM_UT_KMS is for non-atomic specific modesetting debug info. That being said, if we fail sending a message like PAYLOAD_ALLOCATE then the modeset itself is going to not work correctly and thus - the contents of failure would still be relevant to someone working on modesetting issues even if they're not interested in the entirity of what's going on between the sink and the source.

That, and we were already printing when messages timed out under DRM_UT_KMS. Granted though, that printing was added well before I introduced DRM_UT_DP. However, I suppose I could just move that back to using DRM_DEBUG_KMS() and dump the sideband message seperately.

Maybe I'm overthinking this a bit, thoughts?

...

...

...

• struct drm_printer p = drm_debug_printer(DBG_PREFIX);
  

• drm_dp_mst_dump_sideband_msg_tx(&p, txmsg);
  

• } mutex_unlock(&mgr->qlock);

  return ret;

@@ -1927,8 +2217,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 (ret && unlikely(drm_debug & (DRM_UT_DP | DRM_UT_KMS))) {

Same.

...

• 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; }

@@ -1990,6 +2283,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)) {

Like you do here.

...

• 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@redhat.com>
    

• */

+#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 51b2486ec917..ceca89babd65 100644 --- a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c +++ b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c @@ -8,6 +8,7 @@ #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) @@ -34,3 +35,214 @@ 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) {
• case DP_ENUM_PATH_RESOURCES:
• case DP_POWER_UP_PHY:
• case DP_POWER_DOWN_PHY:
• case DP_ALLOCATE_PAYLOAD:
• case DP_QUERY_PAYLOAD:
• case DP_REMOTE_DPCD_READ:

• return memcmp(in, out, sizeof(*in)) == 0;
  

Just memcmp the entire thing for everyrone (we assume kzalloc anyway), and then only have the additional checks for the few cases we need it? Would slightly reduce the code.

Just realized also - this wouldn't work: remember that I2C_READ, DPCD_WRITE, and I2C_WRITE contain pointers which of course will be different even if the contents are the same. Unfortunately I don't think there's really a good solution for this, unless we want to just remove the pointers and store things within the down request struct itself

Uh ... disappoint :-/

Then at least put that into a comment, and maybe make this the default: handler?

...

...

...

• 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

• }
• return true;

+}

+static int +__sideband_msg_req_encode_decode(int line,

• 		 struct drm_dp_sideband_msg_req_body
  

*in,

• 		 struct drm_dp_sideband_msg_req_body
  

*out) +{

• struct drm_printer p = drm_err_printer(pr_fmt());
• struct drm_dp_sideband_msg_tx txmsg;
• int i, ret;
• bool eq;
• drm_dp_encode_sideband_req(in, &txmsg);
• ret = drm_dp_decode_sideband_req(&txmsg, out);
• if (ret < 0) {

• pr_err(pr_fmt("Failed to decode sideband request @
  

line %d: %d\n"),

•        line, ret);
  

• return ret;
  

• }
• eq = sideband_msg_req_equal(in, out);
• if (!eq) {

• pr_err(pr_fmt("Encode/decode @ line %d failed,
  

expected:\n"),

•        line);
  

• drm_dp_dump_sideband_msg_req_body(in, 1, &p);
  

• pr_err(pr_fmt("Got:\n"));
  

• drm_dp_dump_sideband_msg_req_body(out, 1, &p);
  

• }
• 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));
• /* Clear the output entirely */
• memset(out, 0, sizeof(*out));
• return eq ? 0 : -EINVAL;

+}

+int igt_dp_mst_sideband_msg_req_decode(void *unused) +{

• struct drm_dp_sideband_msg_req_body in = { 0 }, out = { 0 };
• u8 data[] = { 0xff, 0x0, 0xdd };
• int i;

+#define DO_TEST() \

• do

{ \

• if (__sideband_msg_req_encode_decode(__LINE__, &in,
  

&out)) \

• 	return
  

-EINVAL; \

• } while (0)

I had a tiny wtf moment here until I realized this is a macro ... maybe put the do on the first line and indent everything? Pure bikeshed to help the blind ...

...

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

+}

Extremely nice, more unit tests ftw!

With the nits somehow figured out: Reviewed-by: Daniel Vetter < daniel.vetter@ffwll.ch>

...

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 c01f3ea72756..4c8d177e83e5 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

-- Cheers, Lyude Paul