[PATCH 0/5] drm/dp: i2c-over-aux short write support

[PATCH 0/5] drm/dp: i2c-over-aux short write support

[ville.syrjala]

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

This series tries to implement support for short i2c-over-aux writes.

I did notice that my monitor (HP ZR24w) does support DDC/CI so I was
able to do some writes, but I only saw native defers instead of i2c
defers/short acks. So I've not actually been able to test this.

Another complication with DDC/CI seems to be that the monitor doesn't
seem to like the default 100kHz bus speed. I had to drop it to 10kHz
to make it reliable (my dongle supports bus speed control fortunately).
Currently we have no standard way to configure the bus speed AFAICS,
so at least with this monitor DDC/CI is a bit useless.

I tried to fix up radeon and tegra too. gma500 I left alone since it's
not using dp i2c helper.

Ville Syrjälä (5):
  drm/dp: s/I2C_STATUS/I2C_WRITE_STATUS_UPDATE/
  drm/i915: Handle DP_AUX_I2C_WRITE_STATUS_UPDATE
  drm/radeon: Handle DP_AUX_I2C_WRITE_STATUS_UPDATE
  drm/tegra: Handle I2C_WRITE_STATUS_UPDATE for address only writes
  drm/dp: Use I2C_WRITE_STATUS_UPDATE to drain partial I2C_WRITE
    requests

 drivers/gpu/drm/drm_dp_helper.c      | 42 ++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_dp.c      |  1 +
 drivers/gpu/drm/radeon/atombios_dp.c |  1 +
 drivers/gpu/drm/tegra/dpaux.c        |  3 ++-
 include/drm/drm_dp_helper.h          |  2 +-
 5 files changed, 43 insertions(+), 6 deletions(-)

-- 
2.0.5

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

[PATCH 1/5] drm/dp: s/I2C_STATUS/I2C_WRITE_STATUS_UPDATE/

[ville.syrjala]

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

Rename the I2C_STATUS request to I2C_WRITE_STATUS_UPDATE to match the
spec.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/tegra/dpaux.c | 2 +-
 include/drm/drm_dp_helper.h   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c
index d6b55e3..b1617af 100644
--- a/drivers/gpu/drm/tegra/dpaux.c
+++ b/drivers/gpu/drm/tegra/dpaux.c
@@ -152,7 +152,7 @@ static ssize_t tegra_dpaux_transfer(struct drm_dp_aux *aux,
 
 		break;
 
-	case DP_AUX_I2C_STATUS:
+	case DP_AUX_I2C_WRITE_STATUS_UPDATE:
 		if (msg->request & DP_AUX_I2C_MOT)
 			value |= DPAUX_DP_AUXCTL_CMD_MOT_RQ;
 		else
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 523f04c..27301f5 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -46,7 +46,7 @@
 
 #define DP_AUX_I2C_WRITE		0x0
 #define DP_AUX_I2C_READ			0x1
-#define DP_AUX_I2C_STATUS		0x2
+#define DP_AUX_I2C_WRITE_STATUS_UPDATE	0x2
 #define DP_AUX_I2C_MOT			0x4
 #define DP_AUX_NATIVE_WRITE		0x8
 #define DP_AUX_NATIVE_READ		0x9
-- 
2.0.5

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

[PATCH 2/5] drm/i915: Handle DP_AUX_I2C_WRITE_STATUS_UPDATE

[ville.syrjala]

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

When we get an i2c defer or short ack for i2c-over-aux write we need
to switch to WRITE_STATUS_UPDATE to poll for the completion of the
original request.

i915 doesn't try to interpret wht request type apart from separating
reads from writes, and so we should be able to treat this the same as
a normal i2c write.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 33d5877..aed5f26 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -956,6 +956,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 	switch (msg->request & ~DP_AUX_I2C_MOT) {
 	case DP_AUX_NATIVE_WRITE:
 	case DP_AUX_I2C_WRITE:
+	case DP_AUX_I2C_WRITE_STATUS_UPDATE:
 		txsize = msg->size ? HEADER_SIZE + msg->size : BARE_ADDRESS_SIZE;
 		rxsize = 1;
 
-- 
2.0.5

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

[PATCH 3/5] drm/radeon: Handle DP_AUX_I2C_WRITE_STATUS_UPDATE

[ville.syrjala]

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

When we get an i2c defer or short ack for i2c-over-aux write we need
to switch to WRITE_STATUS_UPDATE to poll for the completion of the
original request.

Looks like radeon doesn't do anything special with the request type,
so hopefully just treating it the same as a i2c write is enough.

Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: "Christian König" <christian.koenig@amd.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/radeon/atombios_dp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c
index 8d74de8..9960b69 100644
--- a/drivers/gpu/drm/radeon/atombios_dp.c
+++ b/drivers/gpu/drm/radeon/atombios_dp.c
@@ -178,6 +178,7 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 	switch (msg->request & ~DP_AUX_I2C_MOT) {
 	case DP_AUX_NATIVE_WRITE:
 	case DP_AUX_I2C_WRITE:
+	case DP_AUX_I2C_WRITE_STATUS_UPDATE:
 		/* The atom implementation only supports writes with a max payload of
 		 * 12 bytes since it uses 4 bits for the total count (header + payload)
 		 * in the parameter space.  The atom interface supports 16 byte
-- 
2.0.5

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

[PATCH 4/5] drm/tegra: Handle I2C_WRITE_STATUS_UPDATE for address only writes

[ville.syrjala]

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

A address-only I2C_WRITE can't be replied with a short i2c ack, but I
suppose it could be replied with an i2c defer. So the code should be
prepared for an address-only I2C_WRITE_STATUS_UPDATE.

Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: "Terje Bergström" <tbergstrom@nvidia.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/tegra/dpaux.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c
index b1617af..a48687a 100644
--- a/drivers/gpu/drm/tegra/dpaux.c
+++ b/drivers/gpu/drm/tegra/dpaux.c
@@ -122,6 +122,7 @@ static ssize_t tegra_dpaux_transfer(struct drm_dp_aux *aux,
 	 */
 	if (msg->size < 1) {
 		switch (msg->request & ~DP_AUX_I2C_MOT) {
+		case DP_AUX_I2C_WRITE_STATUS_UPDATE:
 		case DP_AUX_I2C_WRITE:
 		case DP_AUX_I2C_READ:
 			value = DPAUX_DP_AUXCTL_CMD_ADDRESS_ONLY;
-- 
2.0.5

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

[PATCH 5/5] drm/dp: Use I2C_WRITE_STATUS_UPDATE to drain partial I2C_WRITE requests

[ville.syrjala]

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

When an i2c WRITE gets an i2c defer or short i2c ack reply, we are
supposed to switch the request from I2C_WRITE to I2C_WRITE_STATUS_UPDATE
when we continue to poll for the completion of the request.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 41 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 37 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index d5368ea..4db81a6 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -422,6 +422,25 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
 	       I2C_FUNC_10BIT_ADDR;
 }
 
+static void drm_dp_i2c_msg_set_request(struct drm_dp_aux_msg *msg,
+				       const struct i2c_msg *i2c_msg)
+{
+	msg->request = (i2c_msg->flags & I2C_M_RD) ?
+		DP_AUX_I2C_READ : DP_AUX_I2C_WRITE;
+	msg->request |= DP_AUX_I2C_MOT;
+}
+
+static void drm_dp_i2c_msg_write_status_update(struct drm_dp_aux_msg *msg)
+{
+	/*
+	 * In case of i2c defer or short i2c ack reply to a write,
+	 * we need to switch to WRITE_STATUS_UPDATE to drain the
+	 * rest of the message
+	 */
+	if ((msg->request & ~DP_AUX_I2C_MOT) == DP_AUX_I2C_WRITE)
+		msg->request |= DP_AUX_I2C_WRITE_STATUS_UPDATE;
+}
+
 /*
  * Transfer a single I2C-over-AUX message and handle various error conditions,
  * retrying the transaction as appropriate.  It is assumed that the
@@ -490,6 +509,8 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 			 * Both native ACK and I2C ACK replies received. We
 			 * can assume the transfer was successful.
 			 */
+			if (ret != msg->size)
+				drm_dp_i2c_msg_write_status_update(msg);
 			return ret;
 
 		case DP_AUX_I2C_REPLY_NACK:
@@ -501,6 +522,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 			DRM_DEBUG_KMS("I2C defer\n");
 			aux->i2c_defer_count++;
 			usleep_range(400, 500);
+			drm_dp_i2c_msg_write_status_update(msg);
 			continue;
 
 		default:
@@ -566,10 +588,7 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
 
 	for (i = 0; i < num; i++) {
 		msg.address = msgs[i].addr;
-		msg.request = (msgs[i].flags & I2C_M_RD) ?
-			DP_AUX_I2C_READ :
-			DP_AUX_I2C_WRITE;
-		msg.request |= DP_AUX_I2C_MOT;
+		drm_dp_i2c_msg_set_request(&msg, &msgs[i]);
 		/* Send a bare address packet to start the transaction.
 		 * Zero sized messages specify an address only (bare
 		 * address) transaction.
@@ -577,6 +596,13 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
 		msg.buffer = NULL;
 		msg.size = 0;
 		err = drm_dp_i2c_do_msg(aux, &msg);
+
+		/*
+		 * Reset msg.request in case in case it got
+		 * changed into a WRITE_STATUS_UPDATE.
+		 */
+		drm_dp_i2c_msg_set_request(&msg, &msgs[i]);
+
 		if (err < 0)
 			break;
 		/* We want each transaction to be as large as possible, but
@@ -589,6 +615,13 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
 			msg.size = min(transfer_size, msgs[i].len - j);
 
 			err = drm_dp_i2c_drain_msg(aux, &msg);
+
+			/*
+			 * Reset msg.request in case in case it got
+			 * changed into a WRITE_STATUS_UPDATE.
+			 */
+			drm_dp_i2c_msg_set_request(&msg, &msgs[i]);
+
 			if (err < 0)
 				break;
 			transfer_size = err;
-- 
2.0.5

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

Re: [PATCH 5/5] drm/dp: Use I2C_WRITE_STATUS_UPDATE to drain partial I2C_WRITE requests

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5948
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  303/303              303/303
SNB                 -1              279/279              278/279
IVB                                  343/343              343/343
BYT                                  287/287              287/287
HSW                                  363/363              363/363
BDW                                  308/308              308/308
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*SNB  igt_gem_persistent_relocs_forked-interruptible-faulting-reloc-thrash-inactive      PASS(2)      DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/dp: Use I2C_WRITE_STATUS_UPDATE to drain partial I2C_WRITE requests

[Jani Nikula]

On Fri, 13 Mar 2015, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> When an i2c WRITE gets an i2c defer or short i2c ack reply, we are
> supposed to switch the request from I2C_WRITE to I2C_WRITE_STATUS_UPDATE
> when we continue to poll for the completion of the request.

As I said IRL, it's a bit unfortunate that setting and resetting of
DP_AUX_I2C_WRITE_STATUS_UPDATE happen at different levels of the
abstraction. But I can live with that.

We also have another problem with reporting short writes in i915,
hopefully fixed by [1]. Before that gets fixed, can't really r-b.

BR,
Jani.


[1] http://mid.gmane.org/1426605534-5780-1-git-send-email-jani.nikula@intel.com

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 41 +++++++++++++++++++++++++++++++++++++----
>  1 file changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index d5368ea..4db81a6 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -422,6 +422,25 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
>  	       I2C_FUNC_10BIT_ADDR;
>  }
>  
> +static void drm_dp_i2c_msg_set_request(struct drm_dp_aux_msg *msg,
> +				       const struct i2c_msg *i2c_msg)
> +{
> +	msg->request = (i2c_msg->flags & I2C_M_RD) ?
> +		DP_AUX_I2C_READ : DP_AUX_I2C_WRITE;
> +	msg->request |= DP_AUX_I2C_MOT;
> +}
> +
> +static void drm_dp_i2c_msg_write_status_update(struct drm_dp_aux_msg *msg)
> +{
> +	/*
> +	 * In case of i2c defer or short i2c ack reply to a write,
> +	 * we need to switch to WRITE_STATUS_UPDATE to drain the
> +	 * rest of the message
> +	 */
> +	if ((msg->request & ~DP_AUX_I2C_MOT) == DP_AUX_I2C_WRITE)
> +		msg->request |= DP_AUX_I2C_WRITE_STATUS_UPDATE;
> +}
> +
>  /*
>   * Transfer a single I2C-over-AUX message and handle various error conditions,
>   * retrying the transaction as appropriate.  It is assumed that the
> @@ -490,6 +509,8 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>  			 * Both native ACK and I2C ACK replies received. We
>  			 * can assume the transfer was successful.
>  			 */
> +			if (ret != msg->size)
> +				drm_dp_i2c_msg_write_status_update(msg);
>  			return ret;
>  
>  		case DP_AUX_I2C_REPLY_NACK:
> @@ -501,6 +522,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>  			DRM_DEBUG_KMS("I2C defer\n");
>  			aux->i2c_defer_count++;
>  			usleep_range(400, 500);
> +			drm_dp_i2c_msg_write_status_update(msg);
>  			continue;
>  
>  		default:
> @@ -566,10 +588,7 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>  
>  	for (i = 0; i < num; i++) {
>  		msg.address = msgs[i].addr;
> -		msg.request = (msgs[i].flags & I2C_M_RD) ?
> -			DP_AUX_I2C_READ :
> -			DP_AUX_I2C_WRITE;
> -		msg.request |= DP_AUX_I2C_MOT;
> +		drm_dp_i2c_msg_set_request(&msg, &msgs[i]);
>  		/* Send a bare address packet to start the transaction.
>  		 * Zero sized messages specify an address only (bare
>  		 * address) transaction.
> @@ -577,6 +596,13 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>  		msg.buffer = NULL;
>  		msg.size = 0;
>  		err = drm_dp_i2c_do_msg(aux, &msg);
> +
> +		/*
> +		 * Reset msg.request in case in case it got
> +		 * changed into a WRITE_STATUS_UPDATE.
> +		 */
> +		drm_dp_i2c_msg_set_request(&msg, &msgs[i]);
> +
>  		if (err < 0)
>  			break;
>  		/* We want each transaction to be as large as possible, but
> @@ -589,6 +615,13 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>  			msg.size = min(transfer_size, msgs[i].len - j);
>  
>  			err = drm_dp_i2c_drain_msg(aux, &msg);
> +
> +			/*
> +			 * Reset msg.request in case in case it got
> +			 * changed into a WRITE_STATUS_UPDATE.
> +			 */
> +			drm_dp_i2c_msg_set_request(&msg, &msgs[i]);
> +
>  			if (err < 0)
>  				break;
>  			transfer_size = err;
> -- 
> 2.0.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 1/5] drm/dp: s/I2C_STATUS/I2C_WRITE_STATUS_UPDATE/

[Jani Nikula]

On Fri, 13 Mar 2015, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Rename the I2C_STATUS request to I2C_WRITE_STATUS_UPDATE to match the
> spec.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/tegra/dpaux.c | 2 +-
>  include/drm/drm_dp_helper.h   | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c
> index d6b55e3..b1617af 100644
> --- a/drivers/gpu/drm/tegra/dpaux.c
> +++ b/drivers/gpu/drm/tegra/dpaux.c
> @@ -152,7 +152,7 @@ static ssize_t tegra_dpaux_transfer(struct drm_dp_aux *aux,
>  
>  		break;
>  
> -	case DP_AUX_I2C_STATUS:
> +	case DP_AUX_I2C_WRITE_STATUS_UPDATE:
>  		if (msg->request & DP_AUX_I2C_MOT)
>  			value |= DPAUX_DP_AUXCTL_CMD_MOT_RQ;
>  		else
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 523f04c..27301f5 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -46,7 +46,7 @@
>  
>  #define DP_AUX_I2C_WRITE		0x0
>  #define DP_AUX_I2C_READ			0x1
> -#define DP_AUX_I2C_STATUS		0x2
> +#define DP_AUX_I2C_WRITE_STATUS_UPDATE	0x2
>  #define DP_AUX_I2C_MOT			0x4
>  #define DP_AUX_NATIVE_WRITE		0x8
>  #define DP_AUX_NATIVE_READ		0x9
> -- 
> 2.0.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/5] drm/i915: Handle DP_AUX_I2C_WRITE_STATUS_UPDATE

[Jani Nikula]

On Fri, 13 Mar 2015, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> When we get an i2c defer or short ack for i2c-over-aux write we need
> to switch to WRITE_STATUS_UPDATE to poll for the completion of the
> original request.
>
> i915 doesn't try to interpret wht request type apart from separating
> reads from writes, and so we should be able to treat this the same as
> a normal i2c write.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Probably needs a refresh due to context change due to my short writes
patch though.

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 33d5877..aed5f26 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -956,6 +956,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>  	switch (msg->request & ~DP_AUX_I2C_MOT) {
>  	case DP_AUX_NATIVE_WRITE:
>  	case DP_AUX_I2C_WRITE:
> +	case DP_AUX_I2C_WRITE_STATUS_UPDATE:
>  		txsize = msg->size ? HEADER_SIZE + msg->size : BARE_ADDRESS_SIZE;
>  		rxsize = 1;
>  
> -- 
> 2.0.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/dp: Use I2C_WRITE_STATUS_UPDATE to drain partial I2C_WRITE requests

[Jani Nikula]

On Fri, 13 Mar 2015, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> When an i2c WRITE gets an i2c defer or short i2c ack reply, we are
> supposed to switch the request from I2C_WRITE to I2C_WRITE_STATUS_UPDATE
> when we continue to poll for the completion of the request.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 41 +++++++++++++++++++++++++++++++++++++----
>  1 file changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index d5368ea..4db81a6 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -422,6 +422,25 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
>  	       I2C_FUNC_10BIT_ADDR;
>  }
>  
> +static void drm_dp_i2c_msg_set_request(struct drm_dp_aux_msg *msg,
> +				       const struct i2c_msg *i2c_msg)
> +{
> +	msg->request = (i2c_msg->flags & I2C_M_RD) ?
> +		DP_AUX_I2C_READ : DP_AUX_I2C_WRITE;
> +	msg->request |= DP_AUX_I2C_MOT;
> +}
> +
> +static void drm_dp_i2c_msg_write_status_update(struct drm_dp_aux_msg *msg)
> +{
> +	/*
> +	 * In case of i2c defer or short i2c ack reply to a write,
> +	 * we need to switch to WRITE_STATUS_UPDATE to drain the
> +	 * rest of the message
> +	 */
> +	if ((msg->request & ~DP_AUX_I2C_MOT) == DP_AUX_I2C_WRITE)
> +		msg->request |= DP_AUX_I2C_WRITE_STATUS_UPDATE;

This only works because DP_AUX_I2C_WRITE == 0, and it may not be obvious
to the casual reader that DP_AUX_I2C_WRITE_STATUS_UPDATE is a
replacement not an addition to DP_AUX_I2C_WRITE.

Whether you fix that or not, this is

Reviewed-by: Jani Nikula <jani.nikula@intel.com>



> +}
> +
>  /*
>   * Transfer a single I2C-over-AUX message and handle various error conditions,
>   * retrying the transaction as appropriate.  It is assumed that the
> @@ -490,6 +509,8 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>  			 * Both native ACK and I2C ACK replies received. We
>  			 * can assume the transfer was successful.
>  			 */
> +			if (ret != msg->size)
> +				drm_dp_i2c_msg_write_status_update(msg);
>  			return ret;
>  
>  		case DP_AUX_I2C_REPLY_NACK:
> @@ -501,6 +522,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>  			DRM_DEBUG_KMS("I2C defer\n");
>  			aux->i2c_defer_count++;
>  			usleep_range(400, 500);
> +			drm_dp_i2c_msg_write_status_update(msg);
>  			continue;
>  
>  		default:
> @@ -566,10 +588,7 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>  
>  	for (i = 0; i < num; i++) {
>  		msg.address = msgs[i].addr;
> -		msg.request = (msgs[i].flags & I2C_M_RD) ?
> -			DP_AUX_I2C_READ :
> -			DP_AUX_I2C_WRITE;
> -		msg.request |= DP_AUX_I2C_MOT;
> +		drm_dp_i2c_msg_set_request(&msg, &msgs[i]);
>  		/* Send a bare address packet to start the transaction.
>  		 * Zero sized messages specify an address only (bare
>  		 * address) transaction.
> @@ -577,6 +596,13 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>  		msg.buffer = NULL;
>  		msg.size = 0;
>  		err = drm_dp_i2c_do_msg(aux, &msg);
> +
> +		/*
> +		 * Reset msg.request in case in case it got
> +		 * changed into a WRITE_STATUS_UPDATE.
> +		 */
> +		drm_dp_i2c_msg_set_request(&msg, &msgs[i]);
> +
>  		if (err < 0)
>  			break;
>  		/* We want each transaction to be as large as possible, but
> @@ -589,6 +615,13 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>  			msg.size = min(transfer_size, msgs[i].len - j);
>  
>  			err = drm_dp_i2c_drain_msg(aux, &msg);
> +
> +			/*
> +			 * Reset msg.request in case in case it got
> +			 * changed into a WRITE_STATUS_UPDATE.
> +			 */
> +			drm_dp_i2c_msg_set_request(&msg, &msgs[i]);
> +
>  			if (err < 0)
>  				break;
>  			transfer_size = err;
> -- 
> 2.0.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Fwd: [PATCH 0/5] drm/dp: i2c-over-aux short write support

[Alex Deucher]

fwd to the list.


---------- Forwarded message ----------
From: Alex Deucher <alexdeucher@gmail.com>
Date: Mon, Mar 16, 2015 at 9:49 AM
Subject: Re: [PATCH 0/5] drm/dp: i2c-over-aux short write support
To: Ville Syrjälä <ville.syrjala@linux.intel.com>


On Fri, Mar 13, 2015 at 12:13 PM,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> This series tries to implement support for short i2c-over-aux writes.
>
> I did notice that my monitor (HP ZR24w) does support DDC/CI so I was
> able to do some writes, but I only saw native defers instead of i2c
> defers/short acks. So I've not actually been able to test this.
>
> Another complication with DDC/CI seems to be that the monitor doesn't
> seem to like the default 100kHz bus speed. I had to drop it to 10kHz
> to make it reliable (my dongle supports bus speed control fortunately).
> Currently we have no standard way to configure the bus speed AFAICS,
> so at least with this monitor DDC/CI is a bit useless.
>
> I tried to fix up radeon and tegra too. gma500 I left alone since it's
> not using dp i2c helper.
>
> Ville Syrjälä (5):
>   drm/dp: s/I2C_STATUS/I2C_WRITE_STATUS_UPDATE/
>   drm/i915: Handle DP_AUX_I2C_WRITE_STATUS_UPDATE
>   drm/radeon: Handle DP_AUX_I2C_WRITE_STATUS_UPDATE
>   drm/tegra: Handle I2C_WRITE_STATUS_UPDATE for address only writes
>   drm/dp: Use I2C_WRITE_STATUS_UPDATE to drain partial I2C_WRITE
>     requests

Patches 1, 3, 5 are:
Acked-by: Alex Deucher <alexander.deucher@amd.com>


>
>  drivers/gpu/drm/drm_dp_helper.c      | 42 ++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_dp.c      |  1 +
>  drivers/gpu/drm/radeon/atombios_dp.c |  1 +
>  drivers/gpu/drm/tegra/dpaux.c        |  3 ++-
>  include/drm/drm_dp_helper.h          |  2 +-
>  5 files changed, 43 insertions(+), 6 deletions(-)
>
> --
> 2.0.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/dp: Use I2C_WRITE_STATUS_UPDATE to drain partial I2C_WRITE requests

[Ville Syrjälä]

On Tue, Mar 17, 2015 at 05:22:08PM +0200, Jani Nikula wrote:
> On Fri, 13 Mar 2015, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > When an i2c WRITE gets an i2c defer or short i2c ack reply, we are
> > supposed to switch the request from I2C_WRITE to I2C_WRITE_STATUS_UPDATE
> > when we continue to poll for the completion of the request.
> 
> As I said IRL, it's a bit unfortunate that setting and resetting of
> DP_AUX_I2C_WRITE_STATUS_UPDATE happen at different levels of the
> abstraction. But I can live with that.
> 
> We also have another problem with reporting short writes in i915,
> hopefully fixed by [1]. Before that gets fixed, can't really r-b.
> 
> BR,
> Jani.
> 
> 
> [1] http://mid.gmane.org/1426605534-5780-1-git-send-email-jani.nikula@intel.com

BTW I notice now that radeon seems to have that same problem. I won't
try to fix that, so just a FYI for the radeon folks...

> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_dp_helper.c | 41 +++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 37 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > index d5368ea..4db81a6 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -422,6 +422,25 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
> >  	       I2C_FUNC_10BIT_ADDR;
> >  }
> >  
> > +static void drm_dp_i2c_msg_set_request(struct drm_dp_aux_msg *msg,
> > +				       const struct i2c_msg *i2c_msg)
> > +{
> > +	msg->request = (i2c_msg->flags & I2C_M_RD) ?
> > +		DP_AUX_I2C_READ : DP_AUX_I2C_WRITE;
> > +	msg->request |= DP_AUX_I2C_MOT;
> > +}
> > +
> > +static void drm_dp_i2c_msg_write_status_update(struct drm_dp_aux_msg *msg)
> > +{
> > +	/*
> > +	 * In case of i2c defer or short i2c ack reply to a write,
> > +	 * we need to switch to WRITE_STATUS_UPDATE to drain the
> > +	 * rest of the message
> > +	 */
> > +	if ((msg->request & ~DP_AUX_I2C_MOT) == DP_AUX_I2C_WRITE)
> > +		msg->request |= DP_AUX_I2C_WRITE_STATUS_UPDATE;
> > +}
> > +
> >  /*
> >   * Transfer a single I2C-over-AUX message and handle various error conditions,
> >   * retrying the transaction as appropriate.  It is assumed that the
> > @@ -490,6 +509,8 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> >  			 * Both native ACK and I2C ACK replies received. We
> >  			 * can assume the transfer was successful.
> >  			 */
> > +			if (ret != msg->size)
> > +				drm_dp_i2c_msg_write_status_update(msg);
> >  			return ret;
> >  
> >  		case DP_AUX_I2C_REPLY_NACK:
> > @@ -501,6 +522,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> >  			DRM_DEBUG_KMS("I2C defer\n");
> >  			aux->i2c_defer_count++;
> >  			usleep_range(400, 500);
> > +			drm_dp_i2c_msg_write_status_update(msg);
> >  			continue;
> >  
> >  		default:
> > @@ -566,10 +588,7 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
> >  
> >  	for (i = 0; i < num; i++) {
> >  		msg.address = msgs[i].addr;
> > -		msg.request = (msgs[i].flags & I2C_M_RD) ?
> > -			DP_AUX_I2C_READ :
> > -			DP_AUX_I2C_WRITE;
> > -		msg.request |= DP_AUX_I2C_MOT;
> > +		drm_dp_i2c_msg_set_request(&msg, &msgs[i]);
> >  		/* Send a bare address packet to start the transaction.
> >  		 * Zero sized messages specify an address only (bare
> >  		 * address) transaction.
> > @@ -577,6 +596,13 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
> >  		msg.buffer = NULL;
> >  		msg.size = 0;
> >  		err = drm_dp_i2c_do_msg(aux, &msg);
> > +
> > +		/*
> > +		 * Reset msg.request in case in case it got
> > +		 * changed into a WRITE_STATUS_UPDATE.
> > +		 */
> > +		drm_dp_i2c_msg_set_request(&msg, &msgs[i]);
> > +
> >  		if (err < 0)
> >  			break;
> >  		/* We want each transaction to be as large as possible, but
> > @@ -589,6 +615,13 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
> >  			msg.size = min(transfer_size, msgs[i].len - j);
> >  
> >  			err = drm_dp_i2c_drain_msg(aux, &msg);
> > +
> > +			/*
> > +			 * Reset msg.request in case in case it got
> > +			 * changed into a WRITE_STATUS_UPDATE.
> > +			 */
> > +			drm_dp_i2c_msg_set_request(&msg, &msgs[i]);
> > +
> >  			if (err < 0)
> >  				break;
> >  			transfer_size = err;
> > -- 
> > 2.0.5
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel