Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: eOHY9B5zg/kc/lXGfOPjdW80MuYe+1hgajN5+oSspELrGu6zn1guw+/mjWoyCSkl5eXTRQuVPE
 /BTfWRH9zwSw==
IronPort-SDR: RqSVLiw9qETRT6Gh2XJn1KfmSOca1/BnR6hMR3+0aIkTMBITSqT3qEufK96iVz8N0/FpOy0Sfa
 N9NWZ+gGm1Tg==
Date:   Tue, 1 Sep 2020 12:31:20 +0300
From:   Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@linux.intel.com>
To:     Sam McNally <sammc@chromium.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        David Airlie <airlied@linux.ie>,
        dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/dp_mst: Support remote i2c writes
Message-ID: <20200901093120.GD6112@intel.com>
References: <20200727160225.1.I4e95a534de051551cd143e6cb83d4c5a9b0ad1cd@changeid>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200727160225.1.I4e95a534de051551cd143e6cb83d4c5a9b0ad1cd@changeid>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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

-- 
Ville Syrj?l?
Intel