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

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

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@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 <brainn@gmail.com>
References: https://bugs.freedesktop.org/show_bug.cgi?id=108081
Signed-off-by: Ville Syrjälä <ville.syrjala@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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/5] drm/dp/mst: Validate REMOTE_I2C_READ harder

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@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ä <ville.syrjala@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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/dp/mst: Validate REMOTE_I2C_READ harder

[Dhinakaran Pandiyan]

On Fri, 2018-09-28 at 21:04 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@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.

Right, the syntax for i2c_remote_read allows only 8 bits for length.
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>


> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@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;

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/5] drm/dp: Implement I2C_M_STOP for i2c-over-aux

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@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ä <ville.syrjala@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

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

Re: [PATCH 3/5] drm/dp: Implement I2C_M_STOP for i2c-over-aux

[Dhinakaran Pandiyan]

On Fri, 2018-09-28 at 21:04 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@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.
> 
Don't see the I2C_M_STOP flag anywhere in drm_edid.c, but the change
introduced here does make sense.

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@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;
>  }
>  
>  /*

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/dp: Implement I2C_M_STOP for i2c-over-aux

[Daniel Vetter]

On Mon, Dec 10, 2018 at 06:47:00PM -0800, Dhinakaran Pandiyan wrote:
> On Fri, 2018-09-28 at 21:04 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@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.
> > 
> Don't see the I2C_M_STOP flag anywhere in drm_edid.c, but the change
> introduced here does make sense.

Iirc it's the i2c core library which takes an entire transaction, splits
it up, and sets the stop flag only on the very last one. Or something like
that.
-Daniel

> 
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@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;
> >  }
> >  
> >  /*
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/dp: Implement I2C_M_STOP for i2c-over-aux

[Ville Syrjälä]

On Wed, Dec 12, 2018 at 11:30:30AM +0100, Daniel Vetter wrote:
> On Mon, Dec 10, 2018 at 06:47:00PM -0800, Dhinakaran Pandiyan wrote:
> > On Fri, 2018-09-28 at 21:04 +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@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.
> > > 
> > Don't see the I2C_M_STOP flag anywhere in drm_edid.c, but the change
> > introduced here does make sense.
> 
> Iirc it's the i2c core library which takes an entire transaction, splits
> it up, and sets the stop flag only on the very last one. Or something like
> that.

The last msg of the transfer has an implicit stop even without
the flag. The core won't add the flag for you. So the flag
is purely meant to force a stop+start between two messages
of the same transfer.

Well, it's not really specific anywhere IIRC but that's how
i2c-algo-bit behaves, and I tend to think of that one as the
defacto specification.


> -Daniel
> 
> > 
> > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@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;
> > >  }
> > >  
> > >  /*
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] drm/dp/mst: Provide defines for ACK vs. NAK reply type

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@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ä <ville.syrjala@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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/5] drm/dp/mst: Provide defines for ACK vs. NAK reply type

[Dhinakaran Pandiyan]

On Fri, 2018-09-28 at 21:04 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@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ä <ville.syrjala@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
> +
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,
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>


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

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

[PATCH 5/5] drm/dp/mst: Provide better debugs for NAK replies

[Ville Syrjala]

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

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

Signed-off-by: Ville Syrjälä <ville.syrjala@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

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

Re: [PATCH 5/5] drm/dp/mst: Provide better debugs for NAK replies

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1873 bytes --]

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 

All warnings (new ones prefixed by >>):

>> 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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 45022 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/5] drm/dp/mst: Provide better debugs for NAK replies

[Dhinakaran Pandiyan]

On Fri, 2018-09-28 at 21:04 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 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, 
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@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

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

Re: [PATCH 5/5] drm/dp/mst: Provide better debugs for NAK replies

[Dhinakaran Pandiyan]

On Fri, 2018-12-07 at 16:57 -0800, Dhinakaran Pandiyan wrote:
> On Fri, 2018-09-28 at 21:04 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > 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, 
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@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];
> > +}

drm_dp_sideband_parse_reply() could also use the decoded string.

-DK

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/5] drm/dp/mst: Configure no_stop_bit correctly for remote i2c xfers

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/dp/mst: Configure no_stop_bit correctly for remote i2c xfers
URL   : https://patchwork.freedesktop.org/series/50341/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
3c513d17e3fc drm/dp/mst: Configure no_stop_bit correctly for remote i2c xfers
12c216b98bf5 drm/dp/mst: Validate REMOTE_I2C_READ harder
732333e972e3 drm/dp: Implement I2C_M_STOP for i2c-over-aux
4fcf4b69c01d drm/dp/mst: Provide defines for ACK vs. NAK reply type
-:99: WARNING:BRACES: braces {} are not necessary for single statement blocks
#99: FILE: drivers/gpu/drm/drm_dp_mst_topology.c:2351:
+		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);
 		}

total: 0 errors, 1 warnings, 0 checks, 94 lines checked
3ce151fed736 drm/dp/mst: Provide better debugs for NAK replies
-:22: ERROR:BRACKET_SPACE: space prohibited before open square bracket '['
#22: FILE: drivers/gpu/drm/drm_dp_mst_topology.c:70:
+#define STR(x) [DP_ ## x] = #x

-:53: ERROR:BRACKET_SPACE: space prohibited before open square bracket '['
#53: FILE: drivers/gpu/drm/drm_dp_mst_topology.c:101:
+#define STR(x) [DP_NAK_ ## x] = #x

total: 2 errors, 0 warnings, 0 checks, 84 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/5] drm/dp/mst: Configure no_stop_bit correctly for remote i2c xfers

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/dp/mst: Configure no_stop_bit correctly for remote i2c xfers
URL   : https://patchwork.freedesktop.org/series/50341/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4906 -> Patchwork_10304 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/50341/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_10304 that come from known issues:

  === IGT changes ===

    ==== Possible fixes ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      fi-byt-clapper:     FAIL (fdo#103191, fdo#107362) -> PASS

    
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362


== Participating hosts (50 -> 47) ==

  Additional (2): fi-skl-caroline fi-snb-2520m 
  Missing    (5): fi-ctg-p8600 fi-bsw-cyan fi-ilk-m540 fi-byt-squawks fi-icl-u2 


== Build changes ==

    * Linux: CI_DRM_4906 -> Patchwork_10304

  CI_DRM_4906: 187637a6495f71dd240d02badbf2fecc1e3c1bb2 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4658: cab89ce2c5da684d01deff402d4e8e11441beadb @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10304: 3ce151fed736e40a6011327e4c320782c6c00c9d @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

3ce151fed736 drm/dp/mst: Provide better debugs for NAK replies
4fcf4b69c01d drm/dp/mst: Provide defines for ACK vs. NAK reply type
732333e972e3 drm/dp: Implement I2C_M_STOP for i2c-over-aux
12c216b98bf5 drm/dp/mst: Validate REMOTE_I2C_READ harder
3c513d17e3fc drm/dp/mst: Configure no_stop_bit correctly for remote i2c xfers

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10304/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/5] drm/dp/mst: Configure no_stop_bit correctly for remote i2c xfers

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/dp/mst: Configure no_stop_bit correctly for remote i2c xfers
URL   : https://patchwork.freedesktop.org/series/50341/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4906_full -> Patchwork_10304_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

  Here are the changes found in Patchwork_10304_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_await@wide-contexts:
      shard-kbl:          PASS -> FAIL (fdo#106680)

    igt@gem_exec_schedule@wide-bsd:
      shard-snb:          SKIP -> INCOMPLETE (fdo#105411)

    igt@gem_exec_suspend@basic-s3-devices:
      shard-glk:          PASS -> INCOMPLETE (fdo#103359, k.org#198133)

    igt@kms_setmode@basic:
      shard-apl:          PASS -> FAIL (fdo#99912)

    igt@perf@blocking:
      shard-hsw:          PASS -> FAIL (fdo#102252)

    
    ==== Possible fixes ====

    igt@kms_color@pipe-c-ctm-blue-to-red:
      shard-kbl:          DMESG-WARN (fdo#103558, fdo#105602) -> PASS +24

    igt@kms_draw_crc@draw-method-xrgb8888-render-ytiled:
      shard-glk:          FAIL (fdo#103232) -> PASS

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#105363) -> PASS

    igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    
    ==== Warnings ====

    igt@kms_available_modes_crc@available_mode_test_crc:
      shard-kbl:          DMESG-WARN (fdo#103558, fdo#105602) -> FAIL (fdo#106641)

    
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#106641 https://bugs.freedesktop.org/show_bug.cgi?id=106641
  fdo#106680 https://bugs.freedesktop.org/show_bug.cgi?id=106680
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (6 -> 5) ==

  Missing    (1): shard-skl 


== Build changes ==

    * Linux: CI_DRM_4906 -> Patchwork_10304

  CI_DRM_4906: 187637a6495f71dd240d02badbf2fecc1e3c1bb2 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4658: cab89ce2c5da684d01deff402d4e8e11441beadb @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10304: 3ce151fed736e40a6011327e4c320782c6c00c9d @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10304/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Dhinakaran Pandiyan]

On Fri, 2018-09-28 at 21:03 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@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."
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.

> 
> 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?


> 
> Cc: Brian Vincent <brainn@gmail.com>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=108081
> Signed-off-by: Ville Syrjälä <ville.syrjala@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;

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Dhinakaran Pandiyan]

On Fri, 2018-12-07 at 12:45 -0800, Dhinakaran Pandiyan wrote:
> On Fri, 2018-09-28 at 21:03 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@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."
> 
> 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.
> 
> > 
> > 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.
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> 
> > 
> > Cc: Brian Vincent <brainn@gmail.com>
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=108081
> > Signed-off-by: Ville Syrjälä <ville.syrjala@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;

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

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

[Ville Syrjälä]

On Fri, Dec 07, 2018 at 12:45:25PM -0800, Dhinakaran Pandiyan wrote:
> On Fri, 2018-09-28 at 21:03 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@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."
> 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'.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Dhinakaran Pandiyan]

On Mon, 2018-12-10 at 18:39 +0200, Ville Syrjälä wrote:
> On Fri, Dec 07, 2018 at 12:45:25PM -0800, Dhinakaran Pandiyan wrote:
> > On Fri, 2018-09-28 at 21:03 +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@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."
> > 
> > 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

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