[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 at linux.intel.com wrote:
> > From: Ville Syrjälä 
> >
> > 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 at 
> 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ä 
> > ---
> >  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(, [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, );
> > +
> > +   /*
> > +* Reset msg.request in case in case it got
> > +* changed into a WRITE_STATUS_UPDATE.
> > +*/
> > +   drm_dp_i2c_msg_set_request(, [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, );
> > +
> > +   /*
> > +* Reset msg.request in case in case it got
> > +* changed into a WRITE_STATUS_UPDATE.
> > +*/
> > +   drm_dp_i2c_msg_set_request(, [i]);
> > +
> > if (err < 0)
> >

[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 at linux.intel.com wrote:
> From: Ville Syrjälä 
>
> 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ä 
> ---
>  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 



> +}
> +
>  /*
>   * 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(, [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, );
> +
> + /*
> +  * Reset msg.request in case in case it got
> +  * changed into a WRITE_STATUS_UPDATE.
> +  */
> + drm_dp_i2c_msg_set_request(, [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, );
> +
> + /*
> +  * Reset msg.request in case in case it got
> +  * changed into a WRITE_STATUS_UPDATE.
> +  */
> + drm_dp_i2c_msg_set_request(, [i]);
> +
>   if (err < 0)
>   break;
>   transfer_size = err;
> -- 
> 2.0.5
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center

[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 at linux.intel.com wrote:
> From: Ville Syrjälä 
>
> 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 at 
intel.com

>
> Signed-off-by: Ville Syrjälä 
> ---
>  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(, [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, );
> +
> + /*
> +  * Reset msg.request in case in case it got
> +  * changed into a WRITE_STATUS_UPDATE.
> +  */
> + drm_dp_i2c_msg_set_request(, [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, );
> +
> + /*
> +  * Reset msg.request in case in case it got
> +  * changed into a WRITE_STATUS_UPDATE.
> +  */
> + drm_dp_i2c_msg_set_request(, [i]);
> +
>   if (err < 0)
>   break;
>   transfer_size = err;
> -- 
> 2.0.5
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center

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

[ville.syrj...@linux.intel.com]

From: Ville Syrjälä 

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ä 
---
 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(, [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, );
+
+   /*
+* Reset msg.request in case in case it got
+* changed into a WRITE_STATUS_UPDATE.
+*/
+   drm_dp_i2c_msg_set_request(, [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, );
+
+   /*
+* Reset msg.request in case in case it got
+* changed into a WRITE_STATUS_UPDATE.
+*/
+   drm_dp_i2c_msg_set_request(, [i]);
+
if (err < 0)
break;
transfer_size = err;
-- 
2.0.5