Commit d308a881 authored by Lyude Paul's avatar Lyude Paul

drm/dp_mst: Kill the second sideband tx slot, save the world

While we support using both tx slots for sideband transmissions, it
appears that DisplayPort devices in the field didn't end up doing a very
good job of supporting it. From section 5.2.1 of the DP 2.0
specification:

  There are MST Sink/Branch devices in the field that do not handle
  interleaved message transactions.

  To facilitate message transaction handling by downstream devices, an
  MST Source device shall generate message transactions in an atomic
  manner (i.e., the MST Source device shall not concurrently interleave
  multiple message transactions). Therefore, an MST Source device shall
  clear the Message_Sequence_No value in the Sideband_MSG_Header to 0.

This might come as a bit of a surprise since the vast majority of hubs
will support using both tx slots even if they don't support interleaved
message transactions, and we've also been using both tx slots since MST
was introduced into the kernel.

However, there is one device we've had trouble getting working
consistently with MST for so long that we actually assumed it was just
broken: the infamous Dell P2415Qb. Previously this monitor would appear
to work sometimes, but in most situations would end up timing out
LINK_ADDRESS messages almost at random until you power cycled the whole
display. After reading section 5.2.1 in the DP 2.0 spec, some closer
investigation into this infamous display revealed it was only ever
timing out on sideband messages in the second TX slot.

Sure enough, avoiding the second TX slot has suddenly made this monitor
function perfectly for the first time in five years. And since they
explicitly mention this in the specification, I doubt this is the only
monitor out there with this issue. This might even explain explain the
seemingly harmless garbage sideband responses we would occasionally see
with MST hubs!

So - rewrite our sideband TX handlers to only support one TX slot. In
order to simplify our sideband handling now that we don't support
transmitting to multiple MSTBs at once, we also move all state tracking
for down replies from mstbs to the topology manager.
Signed-off-by: default avatarLyude Paul <lyude@redhat.com>
Fixes: ad7f8a1f ("drm/helper: add Displayport multi-stream helper (v0.6)")
Cc: Sean Paul <sean@poorly.run>
Cc: "Lin, Wayne" <Wayne.Lin@amd.com>
Cc: <stable@vger.kernel.org> # v3.17+
Reviewed-by: default avatarSean Paul <sean@poorly.run>
Link: https://patchwork.freedesktop.org/patch/msgid/20200424181308.770749-1-lyude@redhat.com
parent 611e22b1
This diff is collapsed.
...@@ -194,11 +194,8 @@ struct drm_dp_sideband_msg_rx { ...@@ -194,11 +194,8 @@ struct drm_dp_sideband_msg_rx {
* @rad: Relative Address to talk to this branch device. * @rad: Relative Address to talk to this branch device.
* @lct: Link count total to talk to this branch device. * @lct: Link count total to talk to this branch device.
* @num_ports: number of ports on the branch. * @num_ports: number of ports on the branch.
* @msg_slots: one bit per transmitted msg slot.
* @port_parent: pointer to the port parent, NULL if toplevel. * @port_parent: pointer to the port parent, NULL if toplevel.
* @mgr: topology manager for this branch device. * @mgr: topology manager for this branch device.
* @tx_slots: transmission slots for this device.
* @last_seqno: last sequence number used to talk to this.
* @link_address_sent: if a link address message has been sent to this device yet. * @link_address_sent: if a link address message has been sent to this device yet.
* @guid: guid for DP 1.2 branch device. port under this branch can be * @guid: guid for DP 1.2 branch device. port under this branch can be
* identified by port #. * identified by port #.
...@@ -239,7 +236,6 @@ struct drm_dp_mst_branch { ...@@ -239,7 +236,6 @@ struct drm_dp_mst_branch {
u8 lct; u8 lct;
int num_ports; int num_ports;
int msg_slots;
/** /**
* @ports: the list of ports on this branch device. This should be * @ports: the list of ports on this branch device. This should be
* considered protected for reading by &drm_dp_mst_topology_mgr.lock. * considered protected for reading by &drm_dp_mst_topology_mgr.lock.
...@@ -252,20 +248,11 @@ struct drm_dp_mst_branch { ...@@ -252,20 +248,11 @@ struct drm_dp_mst_branch {
*/ */
struct list_head ports; struct list_head ports;
/* list of tx ops queue for this port */
struct drm_dp_mst_port *port_parent; struct drm_dp_mst_port *port_parent;
struct drm_dp_mst_topology_mgr *mgr; struct drm_dp_mst_topology_mgr *mgr;
/* slots are protected by mstb->mgr->qlock */
struct drm_dp_sideband_msg_tx *tx_slots[2];
int last_seqno;
bool link_address_sent; bool link_address_sent;
/**
* @down_rep_recv: Message receiver state for down replies.
*/
struct drm_dp_sideband_msg_rx down_rep_recv[2];
/* global unique identifier to identify branch devices */ /* global unique identifier to identify branch devices */
u8 guid[16]; u8 guid[16];
}; };
...@@ -567,6 +554,12 @@ struct drm_dp_mst_topology_mgr { ...@@ -567,6 +554,12 @@ struct drm_dp_mst_topology_mgr {
*/ */
struct drm_dp_sideband_msg_rx up_req_recv; struct drm_dp_sideband_msg_rx up_req_recv;
/**
* @down_rep_recv: Message receiver state for replies to down
* requests.
*/
struct drm_dp_sideband_msg_rx down_rep_recv;
/** /**
* @lock: protects @mst_state, @mst_primary, @dpcd, and * @lock: protects @mst_state, @mst_primary, @dpcd, and
* @payload_id_table_cleared. * @payload_id_table_cleared.
...@@ -592,11 +585,6 @@ struct drm_dp_mst_topology_mgr { ...@@ -592,11 +585,6 @@ struct drm_dp_mst_topology_mgr {
*/ */
bool payload_id_table_cleared : 1; bool payload_id_table_cleared : 1;
/**
* @is_waiting_for_dwn_reply: whether we're waiting for a down reply.
*/
bool is_waiting_for_dwn_reply : 1;
/** /**
* @mst_primary: Pointer to the primary/first branch device. * @mst_primary: Pointer to the primary/first branch device.
*/ */
...@@ -621,13 +609,12 @@ struct drm_dp_mst_topology_mgr { ...@@ -621,13 +609,12 @@ struct drm_dp_mst_topology_mgr {
const struct drm_private_state_funcs *funcs; const struct drm_private_state_funcs *funcs;
/** /**
* @qlock: protects @tx_msg_downq, the &drm_dp_mst_branch.txslost and * @qlock: protects @tx_msg_downq and &drm_dp_sideband_msg_tx.state
* &drm_dp_sideband_msg_tx.state once they are queued
*/ */
struct mutex qlock; struct mutex qlock;
/** /**
* @tx_msg_downq: List of pending down replies. * @tx_msg_downq: List of pending down requests
*/ */
struct list_head tx_msg_downq; struct list_head tx_msg_downq;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment