Re: [PATCH] drm/dp_mst: Support remote i2c writes

On Mon, Jul 27, 2020 at 04:03:37PM +1000, Sam McNally wrote:

For DP MST outputs, the i2c device currently only supports transfers that can be implemented using remote i2c reads. Such transfers must consist of zero or more write transactions followed by one read transaction. DDC/CI commands require standalone write transactions and hence aren't supported.

Since each remote i2c write is handled as a separate transfer, remote i2c writes can support transfers consisting of write transactions, where all but the last have I2C_M_STOP set. According to the DDC/CI 1.1 standard, DDC/CI commands only require a single write or read transaction in a transfer, so this is sufficient.

For i2c transfers meeting the above criteria, generate and send a remote i2c write message for each transaction. Add the trivial remote i2c write reply parsing support so remote i2c write acks bubble up correctly.

Looks great.

For good measure I bounced this to intel-gfx so we got the CI to check it. Seems to have passed.

Amended with Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/37 and pushed to drm-misc-next. Thanks!

Signed-off-by: Sam McNally sammc@chromium.org

drivers/gpu/drm/drm_dp_mst_topology.c | 106 ++++++++++++++++++++++---- 1 file changed, 90 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 09b32289497e..1ac874e4e7a1 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -961,6 +961,8 @@ static bool drm_dp_sideband_parse_reply(struct drm_dp_sideband_msg_rx *raw, return drm_dp_sideband_parse_remote_dpcd_write(raw, msg); case DP_REMOTE_I2C_READ: return drm_dp_sideband_parse_remote_i2c_read_ack(raw, msg);

• case DP_REMOTE_I2C_WRITE:

• return true; /* since there's nothing to parse */
  

  case DP_ENUM_PATH_RESOURCES: return drm_dp_sideband_parse_enum_path_resources_ack(raw, msg); case DP_ALLOCATE_PAYLOAD:

@@ -5326,29 +5328,29 @@ static bool remote_i2c_read_ok(const struct i2c_msg msgs[], int num) msgs[num - 1].len <= 0xff; }

-/* I2C device */ -static int drm_dp_mst_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,

• 	       int num)
  

+static bool remote_i2c_write_ok(const struct i2c_msg msgs[], int num) +{

• int i;
• for (i = 0; i < num - 1; i++) {

• if (msgs[i].flags & I2C_M_RD || !(msgs[i].flags & I2C_M_STOP) ||
  

•     msgs[i].len > 0xff)
  

• 	return false;
  

• }
• return !(msgs[num - 1].flags & I2C_M_RD) && msgs[num - 1].len <= 0xff;

+}

+static int drm_dp_mst_i2c_read(struct drm_dp_mst_branch *mstb,

• 	       struct drm_dp_mst_port *port,
  

• 	       struct i2c_msg *msgs, int num)
  

{

• struct drm_dp_aux *aux = adapter->algo_data;

• struct drm_dp_mst_port *port = container_of(aux, struct drm_dp_mst_port, aux);

• struct drm_dp_mst_branch *mstb; struct drm_dp_mst_topology_mgr *mgr = port->mgr; unsigned int i; struct drm_dp_sideband_msg_req_body msg; struct drm_dp_sideband_msg_tx *txmsg = NULL; int ret;

• mstb = drm_dp_mst_topology_get_mstb_validated(mgr, port->parent);

• if (!mstb)

• return -EREMOTEIO;
  

• if (!remote_i2c_read_ok(msgs, num)) {

• DRM_DEBUG_KMS("Unsupported I2C transaction for MST device\n");
  

• ret = -EIO;
  

• goto out;
  

• }

• memset(&msg, 0, sizeof(msg)); msg.req_type = DP_REMOTE_I2C_READ; msg.u.i2c_read.num_transactions = num - 1;

@@ -5389,6 +5391,78 @@ static int drm_dp_mst_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs } out: kfree(txmsg);

• return ret;

+}

+static int drm_dp_mst_i2c_write(struct drm_dp_mst_branch *mstb,

• 		struct drm_dp_mst_port *port,
  

• 		struct i2c_msg *msgs, int num)
  

+{

• struct drm_dp_mst_topology_mgr *mgr = port->mgr;
• unsigned int i;
• struct drm_dp_sideband_msg_req_body msg;
• struct drm_dp_sideband_msg_tx *txmsg = NULL;
• int ret;
• txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
• if (!txmsg) {

• ret = -ENOMEM;
  

• goto out;
  

• }
• for (i = 0; i < num; i++) {

• memset(&msg, 0, sizeof(msg));
  

• msg.req_type = DP_REMOTE_I2C_WRITE;
  

• msg.u.i2c_write.port_number = port->port_num;
  

• msg.u.i2c_write.write_i2c_device_id = msgs[i].addr;
  

• msg.u.i2c_write.num_bytes = msgs[i].len;
  

• msg.u.i2c_write.bytes = msgs[i].buf;
  

• memset(txmsg, 0, sizeof(*txmsg));
  

• txmsg->dst = mstb;
  

• drm_dp_encode_sideband_req(&msg, txmsg);
  

• drm_dp_queue_down_tx(mgr, txmsg);
  

• ret = drm_dp_mst_wait_tx_reply(mstb, txmsg);
  

• if (ret > 0) {
  

• 	if (txmsg->reply.reply_type == DP_SIDEBAND_REPLY_NAK) {
  

• 		ret = -EREMOTEIO;
  

• 		goto out;
  

• 	}
  

• } else {
  

• 	goto out;
  

• }
  

• }
• ret = num;

+out:

• kfree(txmsg);
• return ret;

+}

+/* I2C device */ +static int drm_dp_mst_i2c_xfer(struct i2c_adapter *adapter,

• 	       struct i2c_msg *msgs, int num)
  

+{

• struct drm_dp_aux *aux = adapter->algo_data;
• struct drm_dp_mst_port *port =

• container_of(aux, struct drm_dp_mst_port, aux);
  

• struct drm_dp_mst_branch *mstb;
• struct drm_dp_mst_topology_mgr *mgr = port->mgr;
• int ret;
• mstb = drm_dp_mst_topology_get_mstb_validated(mgr, port->parent);
• if (!mstb)

• return -EREMOTEIO;
  

• if (remote_i2c_read_ok(msgs, num)) {

• ret = drm_dp_mst_i2c_read(mstb, port, msgs, num);
  

• } else if (remote_i2c_write_ok(msgs, num)) {

• ret = drm_dp_mst_i2c_write(mstb, port, msgs, num);
  

• } else {

• DRM_DEBUG_KMS("Unsupported I2C transaction for MST device\n");
  

• ret = -EIO;
  

• }
• drm_dp_mst_topology_put_mstb(mstb); return ret;

}

2.28.0.rc0.142.g3c755180ce-goog

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel