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

[Ville Syrjälä]

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