[PATCH 1/5] drm/dp/mst: Configure no_stop_bit correctly for remote i2c xfers

Discussion:

Ville Syrjala

2018-09-28 18:03:59 UTC

From: Ville Syrjälä <***@linux.intel.com>

We aren't supposed to force a stop+start between every i2c msg
when performing multi message transfers. This should eg. cause
the DDC segment address to be reset back to 0 between writing
the segment address and reading the actual EDID extension block.

To quote the E-DDC spec:
"... this standard requires that the segment pointer be
reset to 00h when a NO ACK or a STOP condition is received."

Since we're going to touch this might as well consult the
I2C_M_STOP flag to determine whether we want to force the stop
or not.

Cc: Brian Vincent <***@gmail.com>
References: https://bugs.freedesktop.org/show_bug.cgi?id=108081
Signed-off-by: Ville Syrjälä <***@linux.intel.com>
---
drivers/gpu/drm/drm_dp_mst_topology.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 5ff1d79b86c4..3b400eab18a2 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -3276,6 +3276,7 @@ static int drm_dp_mst_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs
msg.u.i2c_read.transactions[i].i2c_dev_id = msgs[i].addr;
msg.u.i2c_read.transactions[i].num_bytes = msgs[i].len;
msg.u.i2c_read.transactions[i].bytes = msgs[i].buf;
+ msg.u.i2c_read.transactions[i].no_stop_bit = !(msgs[i].flags & I2C_M_STOP);
}
msg.u.i2c_read.read_i2c_device_id = msgs[num - 1].addr;
msg.u.i2c_read.num_bytes_read = msgs[num - 1].len;

--
2.16.4

Ville Syrjala

2018-09-28 18:04:01 UTC

Permalink

From: Ville Syrjälä <***@linux.intel.com>

Consult the I2C_M_STOP flag to determine whether to set the MOT bit or
not. Makes it possible to send multiple messages in one go with
stop+start generated between the messages (as opposed nothing or
repstart depending on whether thr address/rw changed).

Not sure anyone has actual use for this but figured I'd handle it
since I started to look at that flag for MST remote i2c xfers.

Signed-off-by: Ville Syrjälä <***@linux.intel.com>
---
drivers/gpu/drm/drm_dp_helper.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 37c01b6076ec..e85cea299d2a 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -884,7 +884,8 @@ static void drm_dp_i2c_msg_set_request(struct drm_dp_aux_msg *msg,
{
msg->request = (i2c_msg->flags & I2C_M_RD) ?
DP_AUX_I2C_READ : DP_AUX_I2C_WRITE;
- msg->request |= DP_AUX_I2C_MOT;
+ if (!(i2c_msg->flags & I2C_M_STOP))
+ msg->request |= DP_AUX_I2C_MOT;
}

/*

--
2.16.4

Dhinakaran Pandiyan

2018-12-11 02:47:00 UTC

Permalink

Post by Ville Syrjala
Consult the I2C_M_STOP flag to determine whether to set the MOT bit or
not. Makes it possible to send multiple messages in one go with
stop+start generated between the messages (as opposed nothing or
repstart depending on whether thr address/rw changed).
Not sure anyone has actual use for this but figured I'd handle it
since I started to look at that flag for MST remote i2c xfers.

Don't see the I2C_M_STOP flag anywhere in drm_edid.c, but the change
introduced here does make sense.

Post by Ville Syrjala
---
drivers/gpu/drm/drm_dp_helper.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_dp_helper.c
b/drivers/gpu/drm/drm_dp_helper.c
index 37c01b6076ec..e85cea299d2a 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -884,7 +884,8 @@ static void drm_dp_i2c_msg_set_request(struct drm_dp_aux_msg *msg,
{
msg->request = (i2c_msg->flags & I2C_M_RD) ?
DP_AUX_I2C_READ : DP_AUX_I2C_WRITE;
- msg->request |= DP_AUX_I2C_MOT;
+ if (!(i2c_msg->flags & I2C_M_STOP))
+ msg->request |= DP_AUX_I2C_MOT;
}
/*

Ville Syrjala

2018-09-28 18:04:02 UTC

Permalink

From: Ville Syrjälä <***@linux.intel.com>

Make the code a bit easier to read by providing symbolic names
for the reply_type (ACK vs. NAK). Also clean up some brace stuff
while at it.

Signed-off-by: Ville Syrjälä <***@linux.intel.com>
---
drivers/gpu/drm/drm_dp_mst_topology.c | 26 +++++++++++++-------------
include/drm/drm_dp_helper.h | 4 ++++
2 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index a0652fc166c6..c0f754364cc7 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -567,7 +567,7 @@ static bool drm_dp_sideband_parse_reply(struct drm_dp_sideband_msg_rx *raw,
msg->reply_type = (raw->msg[0] & 0x80) >> 7;
msg->req_type = (raw->msg[0] & 0x7f);

- if (msg->reply_type) {
+ if (msg->reply_type == DP_REPLY_NAK) {
memcpy(msg->u.nak.guid, &raw->msg[1], 16);
msg->u.nak.reason = raw->msg[17];
msg->u.nak.nak_data = raw->msg[18];
@@ -1614,9 +1614,9 @@ static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
if (ret > 0) {
int i;

- if (txmsg->reply.reply_type == 1)
+ if (txmsg->reply.reply_type == DP_REPLY_NAK) {
DRM_DEBUG_KMS("link address nak received\n");
- else {
+ } 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,
@@ -1665,9 +1665,9 @@ static int drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr,

ret = drm_dp_mst_wait_tx_reply(mstb, txmsg);
if (ret > 0) {
- if (txmsg->reply.reply_type == 1)
+ if (txmsg->reply.reply_type == DP_REPLY_NAK) {
DRM_DEBUG_KMS("enum path resources nak received\n");
- else {
+ } else {
if (port->port_num != txmsg->reply.u.path_resources.port_number)
DRM_ERROR("got incorrect port in response\n");
DRM_DEBUG_KMS("enum path resources %d: %d %d\n", txmsg->reply.u.path_resources.port_number, txmsg->reply.u.path_resources.full_payload_bw_number,
@@ -1755,9 +1755,9 @@ static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr,

ret = drm_dp_mst_wait_tx_reply(mstb, txmsg);
if (ret > 0) {
- if (txmsg->reply.reply_type == 1) {
+ if (txmsg->reply.reply_type == DP_REPLY_NAK)
ret = -EINVAL;
- } else
+ else
ret = 0;
}
kfree(txmsg);
@@ -1789,7 +1789,7 @@ int drm_dp_send_power_updown_phy(struct drm_dp_mst_topology_mgr *mgr,

ret = drm_dp_mst_wait_tx_reply(port->parent, txmsg);
if (ret > 0) {
- if (txmsg->reply.reply_type == 1)
+ if (txmsg->reply.reply_type == DP_REPLY_NAK)
ret = -EINVAL;
else
ret = 0;
@@ -2026,9 +2026,9 @@ static int drm_dp_send_dpcd_write(struct drm_dp_mst_topology_mgr *mgr,

ret = drm_dp_mst_wait_tx_reply(mstb, txmsg);
if (ret > 0) {
- if (txmsg->reply.reply_type == 1) {
+ if (txmsg->reply.reply_type == DP_REPLY_NAK)
ret = -EINVAL;
- } else
+ else
ret = 0;
}
kfree(txmsg);
@@ -2041,7 +2041,7 @@ static int drm_dp_encode_up_ack_reply(struct drm_dp_sideband_msg_tx *msg, u8 req
{
struct drm_dp_sideband_msg_reply_body reply;

- reply.reply_type = 0;
+ reply.reply_type = DP_REPLY_ACK;
reply.req_type = req_type;
drm_dp_encode_sideband_reply(&reply, msg);
return 0;
@@ -2348,7 +2348,7 @@ static int drm_dp_mst_handle_down_rep(struct drm_dp_mst_topology_mgr *mgr)
}

drm_dp_sideband_parse_reply(&mgr->down_rep_recv, &txmsg->reply);
- if (txmsg->reply.reply_type == 1) {
+ if (txmsg->reply.reply_type == DP_REPLY_NAK) {
DRM_DEBUG_KMS("Got NAK reply: req 0x%02x, reason 0x%02x, nak data 0x%02x\n", txmsg->reply.req_type, txmsg->reply.u.nak.reason, txmsg->reply.u.nak.nak_data);
}

@@ -3306,7 +3306,7 @@ static int drm_dp_mst_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs
ret = drm_dp_mst_wait_tx_reply(mstb, txmsg);
if (ret > 0) {

- if (txmsg->reply.reply_type == 1) { /* got a NAK back */
+ if (txmsg->reply.reply_type == DP_REPLY_NAK) {
ret = -EREMOTEIO;
goto out;
}
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 2a3843f248cf..2a0fd9d7066e 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -905,6 +905,10 @@
#define DP_AUX_HDCP_KSV_FIFO 0x6802C
#define DP_AUX_HDCP_AINFO 0x6803B

+/* DP 1.2 MST sideband reply types */
+#define DP_REPLY_ACK 0x00
+#define DP_REPLY_NAK 0x01
+
/* DP 1.2 Sideband message defines */
/* peer device type - DP 1.2a Table 2-92 */
#define DP_PEER_DEVICE_NONE 0x0

--
2.16.4

Dhinakaran Pandiyan

2018-12-08 00:22:59 UTC

Permalink

Post by Ville Syrjala
Make the code a bit easier to read by providing symbolic names
for the reply_type (ACK vs. NAK). Also clean up some brace stuff
while at it.
---
drivers/gpu/drm/drm_dp_mst_topology.c | 26 +++++++++++++----------
---
include/drm/drm_dp_helper.h | 4 ++++
2 files changed, 17 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index a0652fc166c6..c0f754364cc7 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -567,7 +567,7 @@ static bool drm_dp_sideband_parse_reply(struct
drm_dp_sideband_msg_rx *raw,
msg->reply_type = (raw->msg[0] & 0x80) >> 7;
msg->req_type = (raw->msg[0] & 0x7f);
- if (msg->reply_type) {
+ if (msg->reply_type == DP_REPLY_NAK) {
memcpy(msg->u.nak.guid, &raw->msg[1], 16);
msg->u.nak.reason = raw->msg[17];
msg->u.nak.nak_data = raw->msg[18];
@@ -1614,9 +1614,9 @@ static void drm_dp_send_link_address(struct
drm_dp_mst_topology_mgr *mgr,
if (ret > 0) {
int i;
- if (txmsg->reply.reply_type == 1)
+ if (txmsg->reply.reply_type == DP_REPLY_NAK) {
DRM_DEBUG_KMS("link address nak received\n");
- else {
+ } else {
DRM_DEBUG_KMS("link address reply: %d\n",
txmsg->reply.u.link_addr.nports);
for (i = 0; i < txmsg-

Post by Ville Syrjala
reply.u.link_addr.nports; i++) {

%d, pn: %d, dpcd_rev: %02x, mcs: %d, ddps: %d, ldps %d, sdp %d/%d\n",
i,
@@ -1665,9 +1665,9 @@ static int
drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr,
ret = drm_dp_mst_wait_tx_reply(mstb, txmsg);
if (ret > 0) {
- if (txmsg->reply.reply_type == 1)
+ if (txmsg->reply.reply_type == DP_REPLY_NAK) {
DRM_DEBUG_KMS("enum path resources nak
received\n");
- else {
+ } else {
if (port->port_num != txmsg-

Post by Ville Syrjala
reply.u.path_resources.port_number)

DRM_ERROR("got incorrect port in
response\n");
DRM_DEBUG_KMS("enum path resources %d: %d
%d\n", txmsg->reply.u.path_resources.port_number, txmsg-

Post by Ville Syrjala
reply.u.path_resources.full_payload_bw_number,

@@ -1755,9 +1755,9 @@ static int drm_dp_payload_send_msg(struct
drm_dp_mst_topology_mgr *mgr,
ret = drm_dp_mst_wait_tx_reply(mstb, txmsg);
if (ret > 0) {
- if (txmsg->reply.reply_type == 1) {
+ if (txmsg->reply.reply_type == DP_REPLY_NAK)
ret = -EINVAL;
- } else
+ else
ret = 0;
}
kfree(txmsg);
@@ -1789,7 +1789,7 @@ int drm_dp_send_power_updown_phy(struct
drm_dp_mst_topology_mgr *mgr,
ret = drm_dp_mst_wait_tx_reply(port->parent, txmsg);
if (ret > 0) {
- if (txmsg->reply.reply_type == 1)
+ if (txmsg->reply.reply_type == DP_REPLY_NAK)
ret = -EINVAL;
else
ret = 0;
@@ -2026,9 +2026,9 @@ static int drm_dp_send_dpcd_write(struct
drm_dp_mst_topology_mgr *mgr,
ret = drm_dp_mst_wait_tx_reply(mstb, txmsg);
if (ret > 0) {
- if (txmsg->reply.reply_type == 1) {
+ if (txmsg->reply.reply_type == DP_REPLY_NAK)
ret = -EINVAL;
- } else
+ else
ret = 0;
}
kfree(txmsg);
@@ -2041,7 +2041,7 @@ static int drm_dp_encode_up_ack_reply(struct
drm_dp_sideband_msg_tx *msg, u8 req
{
struct drm_dp_sideband_msg_reply_body reply;
- reply.reply_type = 0;
+ reply.reply_type = DP_REPLY_ACK;
reply.req_type = req_type;
drm_dp_encode_sideband_reply(&reply, msg);
return 0;
@@ -2348,7 +2348,7 @@ static int drm_dp_mst_handle_down_rep(struct
drm_dp_mst_topology_mgr *mgr)
}
drm_dp_sideband_parse_reply(&mgr->down_rep_recv,
&txmsg->reply);
- if (txmsg->reply.reply_type == 1) {
+ if (txmsg->reply.reply_type == DP_REPLY_NAK) {
DRM_DEBUG_KMS("Got NAK reply: req 0x%02x,
reason 0x%02x, nak data 0x%02x\n", txmsg->reply.req_type, txmsg-

Post by Ville Syrjala
reply.u.nak.reason, txmsg->reply.u.nak.nak_data);

}
@@ -3306,7 +3306,7 @@ static int drm_dp_mst_i2c_xfer(struct
i2c_adapter *adapter, struct i2c_msg *msgs
ret = drm_dp_mst_wait_tx_reply(mstb, txmsg);
if (ret > 0) {
- if (txmsg->reply.reply_type == 1) { /* got a NAK back
*/
+ if (txmsg->reply.reply_type == DP_REPLY_NAK) {
ret = -EREMOTEIO;
goto out;
}
diff --git a/include/drm/drm_dp_helper.h
b/include/drm/drm_dp_helper.h
index 2a3843f248cf..2a0fd9d7066e 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -905,6 +905,10 @@
#define DP_AUX_HDCP_KSV_FIFO 0x6802C
#define DP_AUX_HDCP_AINFO 0x6803B
+/* DP 1.2 MST sideband reply types */
+#define DP_REPLY_ACK 0x00
+#define DP_REPLY_NAK 0x01
+

bikeshed: How about calling these DP_SIDEBAND_ACK or DP_SIDEBAND_NAK to
differentiate from native AUX replies? And also move it right next to
the NAK reason definition.

Will leave it to you if you want to implement those bikesheds,

Post by Ville Syrjala
/* DP 1.2 Sideband message defines */
/* peer device type - DP 1.2a Table 2-92 */
#define DP_PEER_DEVICE_NONE 0x0

Ville Syrjala

2018-09-28 18:04:03 UTC

Permalink

From: Ville Syrjälä <***@linux.intel.com>

Decode the NAK reply fields to make it easier to parse the logs.

Signed-off-by: Ville Syrjälä <***@linux.intel.com>
---
drivers/gpu/drm/drm_dp_mst_topology.c | 65 ++++++++++++++++++++++++++++++++++-
include/drm/drm_dp_helper.h | 1 +
2 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index c0f754364cc7..1178c1655f9a 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -66,6 +66,64 @@ static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr,
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 STR(x) [DP_ ## x] = #x
+
+static const char *drm_dp_mst_req_type_str(u8 req_type)
+{
+ static const char * const req_type_str[] = {
+ STR(GET_MSG_TRANSACTION_VERSION),
+ STR(LINK_ADDRESS),
+ STR(CONNECTION_STATUS_NOTIFY),
+ STR(ENUM_PATH_RESOURCES),
+ STR(ALLOCATE_PAYLOAD),
+ STR(QUERY_PAYLOAD),
+ STR(RESOURCE_STATUS_NOTIFY),
+ STR(CLEAR_PAYLOAD_ID_TABLE),
+ STR(REMOTE_DPCD_READ),
+ STR(REMOTE_DPCD_WRITE),
+ STR(REMOTE_I2C_READ),
+ STR(REMOTE_I2C_WRITE),
+ STR(POWER_UP_PHY),
+ STR(POWER_DOWN_PHY),
+ STR(SINK_EVENT_NOTIFY),
+ STR(QUERY_STREAM_ENC_STATUS),
+ };
+
+ if (req_type >= ARRAY_SIZE(req_type_str) ||
+ !req_type_str[req_type])
+ return "unknown";
+
+ return req_type_str[req_type];
+}
+
+#undef STR
+#define STR(x) [DP_NAK_ ## x] = #x
+
+static const char *drm_dp_mst_nak_reason_str(u8 nak_reason)
+{
+ static const char * const nak_reason_str[] = {
+ STR(WRITE_FAILURE),
+ STR(INVALID_READ),
+ STR(CRC_FAILURE),
+ STR(BAD_PARAM),
+ STR(DEFER),
+ STR(LINK_FAILURE),
+ STR(NO_RESOURCES),
+ STR(DPCD_FAIL),
+ STR(I2C_NAK),
+ STR(ALLOCATE_FAIL),
+ };
+
+ if (nak_reason >= ARRAY_SIZE(nak_reason_str) ||
+ !nak_reason_str[nak_reason])
+ return "unknown";
+
+ return nak_reason_str[nak_reason];
+}
+
+#undef STR
+
/* sideband msg handling */
static u8 drm_dp_msg_header_crc4(const uint8_t *data, size_t num_nibbles)
{
@@ -2349,7 +2407,12 @@ static int drm_dp_mst_handle_down_rep(struct drm_dp_mst_topology_mgr *mgr)

drm_dp_sideband_parse_reply(&mgr->down_rep_recv, &txmsg->reply);
if (txmsg->reply.reply_type == DP_REPLY_NAK) {
- DRM_DEBUG_KMS("Got NAK reply: req 0x%02x, reason 0x%02x, nak data 0x%02x\n", txmsg->reply.req_type, txmsg->reply.u.nak.reason, txmsg->reply.u.nak.nak_data);
+ DRM_DEBUG_KMS("Got NAK reply: req 0x%02x (%s), reason 0x%02x (%s), nak data 0x%02x\n",
+ txmsg->reply.req_type,
+ drm_dp_mst_req_type_str(txmsg->reply.req_type),
+ txmsg->reply.u.nak.reason,
+ drm_dp_mst_nak_reason_str(txmsg->reply.u.nak.reason),
+ txmsg->reply.u.nak.nak_data);
}

memset(&mgr->down_rep_recv, 0, sizeof(struct drm_dp_sideband_msg_rx));
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 2a0fd9d7066e..2453767246fb 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -918,6 +918,7 @@
#define DP_PEER_DEVICE_DP_LEGACY_CONV 0x4

/* DP 1.2 MST sideband request names DP 1.2a Table 2-80 */
+#define DP_GET_MSG_TRANSACTION_VERSION 0x00 /* DP 1.3 */
#define DP_LINK_ADDRESS 0x01
#define DP_CONNECTION_STATUS_NOTIFY 0x02
#define DP_ENUM_PATH_RESOURCES 0x10

--
2.16.4

kbuild test robot

2018-09-28 21:55:57 UTC

Permalink

Hi Ville,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on sof-driver-fuweitax/master]
[also build test WARNING on v4.19-rc5 next-20180928]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Ville-Syrjala/drm-dp-mst-Configure-no_stop_bit-correctly-for-remote-i2c-xfers/20180929-040153
base: https://github.com/fuweitax/linux master
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=m68k

drivers/gpu/drm/drm_dp_mst_topology.c:70:0: warning: "STR" redefined

#define STR(x) [DP_ ## x] = #x

In file included from arch/m68k/include/asm/irqflags.h:8:0,
from include/linux/irqflags.h:16,
from arch/m68k/include/asm/atomic.h:6,
from include/linux/atomic.h:5,
from include/linux/rcupdate.h:38,
from include/linux/rculist.h:11,
from include/linux/pid.h:5,
from include/linux/sched.h:14,
from drivers/gpu/drm/drm_dp_mst_topology.c:27:
arch/m68k/include/asm/entry.h:244:0: note: this is the location of the previous definition
#define STR(X) STR1(X)

vim +/STR +70 drivers/gpu/drm/drm_dp_mst_topology.c

69

70 #define STR(x) [DP_ ## x] = #x

71

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Dhinakaran Pandiyan

2018-12-08 00:57:54 UTC

Permalink

Post by Ville Syrjala
Decode the NAK reply fields to make it easier to parse the logs.

A lot better than seeing the error codes.

0-day's found a conflicting definition that's missing an undef. With
that addressed,

Post by Ville Syrjala
---
drivers/gpu/drm/drm_dp_mst_topology.c | 65
++++++++++++++++++++++++++++++++++-
include/drm/drm_dp_helper.h | 1 +
2 files changed, 65 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index c0f754364cc7..1178c1655f9a 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -66,6 +66,64 @@ static bool drm_dp_validate_guid(struct
drm_dp_mst_topology_mgr *mgr,
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 STR(x) [DP_ ## x] = #x
+
+static const char *drm_dp_mst_req_type_str(u8 req_type)
+{
+ static const char * const req_type_str[] = {
+ STR(GET_MSG_TRANSACTION_VERSION),
+ STR(LINK_ADDRESS),
+ STR(CONNECTION_STATUS_NOTIFY),
+ STR(ENUM_PATH_RESOURCES),
+ STR(ALLOCATE_PAYLOAD),
+ STR(QUERY_PAYLOAD),
+ STR(RESOURCE_STATUS_NOTIFY),
+ STR(CLEAR_PAYLOAD_ID_TABLE),
+ STR(REMOTE_DPCD_READ),
+ STR(REMOTE_DPCD_WRITE),
+ STR(REMOTE_I2C_READ),
+ STR(REMOTE_I2C_WRITE),
+ STR(POWER_UP_PHY),
+ STR(POWER_DOWN_PHY),
+ STR(SINK_EVENT_NOTIFY),
+ STR(QUERY_STREAM_ENC_STATUS),
+ };
+
+ if (req_type >= ARRAY_SIZE(req_type_str) ||
+ !req_type_str[req_type])
+ return "unknown";
+
+ return req_type_str[req_type];
+}
+
+#undef STR
+#define STR(x) [DP_NAK_ ## x] = #x
+
+static const char *drm_dp_mst_nak_reason_str(u8 nak_reason)
+{
+ static const char * const nak_reason_str[] = {
+ STR(WRITE_FAILURE),
+ STR(INVALID_READ),
+ STR(CRC_FAILURE),
+ STR(BAD_PARAM),
+ STR(DEFER),
+ STR(LINK_FAILURE),
+ STR(NO_RESOURCES),
+ STR(DPCD_FAIL),
+ STR(I2C_NAK),
+ STR(ALLOCATE_FAIL),
+ };
+
+ if (nak_reason >= ARRAY_SIZE(nak_reason_str) ||
+ !nak_reason_str[nak_reason])
+ return "unknown";
+
+ return nak_reason_str[nak_reason];
+}
+
+#undef STR
+
/* sideband msg handling */
static u8 drm_dp_msg_header_crc4(const uint8_t *data, size_t
num_nibbles)
{
@@ -2349,7 +2407,12 @@ static int drm_dp_mst_handle_down_rep(struct
drm_dp_mst_topology_mgr *mgr)
drm_dp_sideband_parse_reply(&mgr->down_rep_recv,
&txmsg->reply);
if (txmsg->reply.reply_type == DP_REPLY_NAK) {
- DRM_DEBUG_KMS("Got NAK reply: req 0x%02x,
reason 0x%02x, nak data 0x%02x\n", txmsg->reply.req_type, txmsg-

Post by Ville Syrjala
reply.u.nak.reason, txmsg->reply.u.nak.nak_data);

+ DRM_DEBUG_KMS("Got NAK reply: req 0x%02x (%s),
reason 0x%02x (%s), nak data 0x%02x\n",
+ txmsg->reply.req_type,
+ drm_dp_mst_req_type_str(txmsg-

Post by Ville Syrjala
reply.req_type),

+ txmsg->reply.u.nak.reason,
+ drm_dp_mst_nak_reason_str(txmsg-

Post by Ville Syrjala
reply.u.nak.reason),

+ txmsg->reply.u.nak.nak_data);
}
memset(&mgr->down_rep_recv, 0, sizeof(struct
drm_dp_sideband_msg_rx));
diff --git a/include/drm/drm_dp_helper.h
b/include/drm/drm_dp_helper.h
index 2a0fd9d7066e..2453767246fb 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -918,6 +918,7 @@
#define DP_PEER_DEVICE_DP_LEGACY_CONV 0x4
/* DP 1.2 MST sideband request names DP 1.2a Table 2-80 */
+#define DP_GET_MSG_TRANSACTION_VERSION 0x00 /* DP 1.3 */
#define DP_LINK_ADDRESS 0x01
#define DP_CONNECTION_STATUS_NOTIFY 0x02
#define DP_ENUM_PATH_RESOURCES 0x10

Dhinakaran Pandiyan

2018-12-08 01:05:09 UTC

Permalink

Post by Dhinakaran Pandiyan

Post by Ville Syrjala
Decode the NAK reply fields to make it easier to parse the logs.

A lot better than seeing the error codes.
0-day's found a conflicting definition that's missing an undef. With
that addressed,

drm_dp_sideband_parse_reply() could also use the decoded string.

-DK

Ville Syrjala

2018-09-28 18:04:00 UTC

Permalink

From: Ville Syrjälä <***@linux.intel.com>

Make sure i2c msgs we're asked to transfer conform to the
requirements of REMOTE_I2C_READ. We were only checking that the
last message is a read, but we must also check that the preceding
messages are all writes. Also check that the length of each
message isn't too long.

Signed-off-by: Ville Syrjälä <***@linux.intel.com>
---
drivers/gpu/drm/drm_dp_mst_topology.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 3b400eab18a2..a0652fc166c6 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -3239,6 +3239,23 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr)
}
EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy);

+static bool remote_i2c_read_ok(const struct i2c_msg msgs[], int num)
+{
+ int i;
+
+ if (num - 1 > DP_REMOTE_I2C_READ_MAX_TRANSACTIONS)
+ return false;
+
+ for (i = 0; i < num - 1; i++) {
+ if (msgs[i].flags & I2C_M_RD ||
+ msgs[i].len > 0xff)
+ return false;
+ }
+
+ return msgs[num - 1].flags & I2C_M_RD &&
+ msgs[num - 1].len <= 0xff;
+}
+
/* I2C device */
static int drm_dp_mst_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
int num)
@@ -3248,7 +3265,6 @@ static int drm_dp_mst_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs
struct drm_dp_mst_branch *mstb;
struct drm_dp_mst_topology_mgr *mgr = port->mgr;
unsigned int i;
- bool reading = false;
struct drm_dp_sideband_msg_req_body msg;
struct drm_dp_sideband_msg_tx *txmsg = NULL;
int ret;
@@ -3257,12 +3273,7 @@ static int drm_dp_mst_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs
if (!mstb)
return -EREMOTEIO;

- /* construct i2c msg */
- /* see if last msg is a read */
- if (msgs[num - 1].flags & I2C_M_RD)
- reading = true;
-
- if (!reading || (num - 1 > DP_REMOTE_I2C_READ_MAX_TRANSACTIONS)) {
+ if (!remote_i2c_read_ok(msgs, num)) {
DRM_DEBUG_KMS("Unsupported I2C transaction for MST device\n");
ret = -EIO;
goto out;

--
2.16.4

Dhinakaran Pandiyan

2018-12-07 23:11:15 UTC

Permalink

Post by Ville Syrjala
Make sure i2c msgs we're asked to transfer conform to the
requirements of REMOTE_I2C_READ. We were only checking that the
last message is a read, but we must also check that the preceding
messages are all writes. Also check that the length of each
message isn't too long.

Right, the syntax for i2c_remote_read allows only 8 bits for length.

Post by Ville Syrjala
---
drivers/gpu/drm/drm_dp_mst_topology.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 3b400eab18a2..a0652fc166c6 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -3239,6 +3239,23 @@ void drm_dp_mst_topology_mgr_destroy(struct
drm_dp_mst_topology_mgr *mgr)
}
EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy);
+static bool remote_i2c_read_ok(const struct i2c_msg msgs[], int num)
+{
+ int i;
+
+ if (num - 1 > DP_REMOTE_I2C_READ_MAX_TRANSACTIONS)
+ return false;
+
+ for (i = 0; i < num - 1; i++) {
+ if (msgs[i].flags & I2C_M_RD ||
+ msgs[i].len > 0xff)
+ return false;
+ }
+
+ return msgs[num - 1].flags & I2C_M_RD &&
+ msgs[num - 1].len <= 0xff;
+}
+
/* I2C device */
static int drm_dp_mst_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
int num)
@@ -3248,7 +3265,6 @@ static int drm_dp_mst_i2c_xfer(struct
i2c_adapter *adapter, struct i2c_msg *msgs
struct drm_dp_mst_branch *mstb;
struct drm_dp_mst_topology_mgr *mgr = port->mgr;
unsigned int i;
- bool reading = false;
struct drm_dp_sideband_msg_req_body msg;
struct drm_dp_sideband_msg_tx *txmsg = NULL;
int ret;
@@ -3257,12 +3273,7 @@ static int drm_dp_mst_i2c_xfer(struct
i2c_adapter *adapter, struct i2c_msg *msgs
if (!mstb)
return -EREMOTEIO;
- /* construct i2c msg */
- /* see if last msg is a read */
- if (msgs[num - 1].flags & I2C_M_RD)
- reading = true;
-
- if (!reading || (num - 1 >
DP_REMOTE_I2C_READ_MAX_TRANSACTIONS)) {
+ if (!remote_i2c_read_ok(msgs, num)) {
DRM_DEBUG_KMS("Unsupported I2C transaction for MST
device\n");
ret = -EIO;
goto out;

Dhinakaran Pandiyan

2018-12-07 20:45:25 UTC

Permalink

Post by Ville Syrjala
We aren't supposed to force a stop+start between every i2c msg
when performing multi message transfers. This should eg. cause
the DDC segment address to be reset back to 0 between writing
the segment address and reading the actual EDID extension block.
"... this standard requires that the segment pointer be
reset to 00h when a NO ACK or a STOP condition is received."

Related question, do you know why the segment and ddc addresses are
defined as 0x30 and 0x50? The E-DDC spec says they should be at 0x60
and 0xA0/0xA1.

Post by Ville Syrjala
Since we're going to touch this might as well consult the
I2C_M_STOP flag to determine whether we want to force the stop
or not.

Reviewing this took a lot of spec reading than I expected.

Setting the no_stop_bit after writing the segment address makes sense.
I have one concern though. drm_do_probe_ddc_edid does not make use of
the I2C_M_STOP flag, which in turn means we won't reset the no_stop_bit
at the end of edid read. Pass the i2c stop flag from the caller?

Post by Ville Syrjala
References: https://bugs.freedesktop.org/show_bug.cgi?id=108081
---
drivers/gpu/drm/drm_dp_mst_topology.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 5ff1d79b86c4..3b400eab18a2 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -3276,6 +3276,7 @@ static int drm_dp_mst_i2c_xfer(struct
i2c_adapter *adapter, struct i2c_msg *msgs
msg.u.i2c_read.transactions[i].i2c_dev_id =
msgs[i].addr;
msg.u.i2c_read.transactions[i].num_bytes = msgs[i].len;
msg.u.i2c_read.transactions[i].bytes = msgs[i].buf;
+ msg.u.i2c_read.transactions[i].no_stop_bit =
!(msgs[i].flags & I2C_M_STOP);
}
msg.u.i2c_read.read_i2c_device_id = msgs[num - 1].addr;
msg.u.i2c_read.num_bytes_read = msgs[num - 1].len;

Dhinakaran Pandiyan

2018-12-07 22:52:35 UTC

Permalink

Post by Dhinakaran Pandiyan

Related question, do you know why the segment and ddc addresses are
defined as 0x30 and 0x50? The E-DDC spec says they should be at 0x60
and 0xA0/0xA1.

Post by Ville Syrjala
Since we're going to touch this might as well consult the
I2C_M_STOP flag to determine whether we want to force the stop
or not.

Reviewing this took a lot of spec reading than I expected.
Setting the no_stop_bit after writing the segment address makes sense.
I have one concern though. drm_do_probe_ddc_edid does not make use of
the I2C_M_STOP flag, which in turn means we won't reset the
no_stop_bit
at the end of edid read. Pass the i2c stop flag from the caller?

Never mind, the no_stop_bit is relevant only between i2c writes.

Post by Dhinakaran Pandiyan

Dhinakaran Pandiyan

2018-12-10 20:09:25 UTC

Permalink

Post by Dhinakaran Pandiyan

Related question, do you know why the segment and ddc addresses are
defined as 0x30 and 0x50? The E-DDC spec says they should be at 0x60
and 0xA0/0xA1.

The spec uses 'slave_address << 1 | r/w'.

Got it, thanks.

-DK