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@xxxxxxxxxxxx> > --- > > 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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
