| Message ID | 1396641519-18529-3-git-send-email-alexander.deucher@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 04, 2014 at 03:58:37PM -0400, Alex Deucher wrote: > We need bare address packets at the start and end of > each i2c over aux transaction to properly reset the connection > between transactions. This mirrors what the existing dp i2c > over aux algo currently does. > > This fixes EDID fetches on certain monitors especially with > dp bridges. > > v2: update as per Ville's comments > - Set buffer to NULL for zero sized packets > - abort the entre transaction if one of the messages fails > v3: drop leftover debugging code > > Signed-off-by: Alex Deucher <alexander.deucher@amd.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Thierry Reding <treding@nvidia.com> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/drm_dp_helper.c | 52 +++++++++++++++++++++++------------------ > 1 file changed, 29 insertions(+), 23 deletions(-) Can we please document that zero-sized messages specify address-only transactions? Perhaps it would also be useful to mention that these can only happen for I2C-over-AUX messages. > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > index 74724aa..dfe4cf4 100644 > --- a/drivers/gpu/drm/drm_dp_helper.c > +++ b/drivers/gpu/drm/drm_dp_helper.c > @@ -664,12 +664,23 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, > int num) > { > struct drm_dp_aux *aux = adapter->algo_data; > - unsigned int i, j; > + unsigned int m, b; I don't see why these would need to be changed. i and j are perfectly fine loop variable names. > - for (i = 0; i < num; i++) { > - struct drm_dp_aux_msg msg; > - int err; > + memset(&msg, 0, sizeof(msg)); > > + for (m = 0; m < num; m++) { > + msg.address = msgs[m].addr; > + msg.request = (msgs[m].flags & I2C_M_RD) ? > + DP_AUX_I2C_READ : > + DP_AUX_I2C_WRITE; > + msg.request |= DP_AUX_I2C_MOT; > + msg.buffer = NULL; > + msg.size = 0; > + err = drm_dp_i2c_do_msg(aux, &msg); > + if (err < 0) > + break; This seems somewhat brittle to me. Even though I notice that patch 3/4 updates a comment that documents these assumptions, I don't see a reason for these assumptions in the first place. I'd prefer if we simply provided the complete message rather than rely on drivers not to touch anything but the reply field. Thierry
On Fri, Apr 04, 2014 at 03:58:37PM -0400, Alex Deucher wrote: [...] > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c [...] > + /* send a bare address packet to close out the connection */ Missed this one earlier. Perhaps s/connection/transaction/? Thierry
On Mon, Apr 7, 2014 at 3:49 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Fri, Apr 04, 2014 at 03:58:37PM -0400, Alex Deucher wrote: >> We need bare address packets at the start and end of >> each i2c over aux transaction to properly reset the connection >> between transactions. This mirrors what the existing dp i2c >> over aux algo currently does. >> >> This fixes EDID fetches on certain monitors especially with >> dp bridges. >> >> v2: update as per Ville's comments >> - Set buffer to NULL for zero sized packets >> - abort the entre transaction if one of the messages fails >> v3: drop leftover debugging code >> >> Signed-off-by: Alex Deucher <alexander.deucher@amd.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Cc: Thierry Reding <treding@nvidia.com> >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> --- >> drivers/gpu/drm/drm_dp_helper.c | 52 +++++++++++++++++++++++------------------ >> 1 file changed, 29 insertions(+), 23 deletions(-) > > Can we please document that zero-sized messages specify address-only > transactions? Perhaps it would also be useful to mention that these can > only happen for I2C-over-AUX messages. Can do. I don't know of any uses for bare address packets with regular aux off hand, but there may be cases I'm not familiar with. Does anyone know of any use for a bare address packet with regular aux? > >> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c >> index 74724aa..dfe4cf4 100644 >> --- a/drivers/gpu/drm/drm_dp_helper.c >> +++ b/drivers/gpu/drm/drm_dp_helper.c >> @@ -664,12 +664,23 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, >> int num) >> { >> struct drm_dp_aux *aux = adapter->algo_data; >> - unsigned int i, j; >> + unsigned int m, b; > > I don't see why these would need to be changed. i and j are perfectly > fine loop variable names. It was easier for me to follow the code since the variables matched the objects they were iterating, but I can change them back if you'd prefer. > >> - for (i = 0; i < num; i++) { >> - struct drm_dp_aux_msg msg; >> - int err; >> + memset(&msg, 0, sizeof(msg)); >> >> + for (m = 0; m < num; m++) { >> + msg.address = msgs[m].addr; >> + msg.request = (msgs[m].flags & I2C_M_RD) ? >> + DP_AUX_I2C_READ : >> + DP_AUX_I2C_WRITE; >> + msg.request |= DP_AUX_I2C_MOT; >> + msg.buffer = NULL; >> + msg.size = 0; >> + err = drm_dp_i2c_do_msg(aux, &msg); >> + if (err < 0) >> + break; > > This seems somewhat brittle to me. Even though I notice that patch 3/4 > updates a comment that documents these assumptions, I don't see a reason > for these assumptions in the first place. We already assume that in drm_dp_i2c_do_msg() for the retry loop. > > I'd prefer if we simply provided the complete message rather than rely > on drivers not to touch anything but the reply field. Are you suggesting we re-write drm_dp_i2c_do_msg() or move the retry logic into drm_dp_i2c_xfer()? Do you mind if we do that in a follow up patch so we can keep it separate from the addition of the bare address packets? Alex
On Mon, Apr 07, 2014 at 09:44:06AM -0400, Alex Deucher wrote: > On Mon, Apr 7, 2014 at 3:49 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > > On Fri, Apr 04, 2014 at 03:58:37PM -0400, Alex Deucher wrote: > >> We need bare address packets at the start and end of > >> each i2c over aux transaction to properly reset the connection > >> between transactions. This mirrors what the existing dp i2c > >> over aux algo currently does. > >> > >> This fixes EDID fetches on certain monitors especially with > >> dp bridges. > >> > >> v2: update as per Ville's comments > >> - Set buffer to NULL for zero sized packets > >> - abort the entre transaction if one of the messages fails > >> v3: drop leftover debugging code > >> > >> Signed-off-by: Alex Deucher <alexander.deucher@amd.com> > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> Cc: Jani Nikula <jani.nikula@intel.com> > >> Cc: Thierry Reding <treding@nvidia.com> > >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> --- > >> drivers/gpu/drm/drm_dp_helper.c | 52 +++++++++++++++++++++++------------------ > >> 1 file changed, 29 insertions(+), 23 deletions(-) > > > > Can we please document that zero-sized messages specify address-only > > transactions? Perhaps it would also be useful to mention that these can > > only happen for I2C-over-AUX messages. > > Can do. I don't know of any uses for bare address packets with > regular aux off hand, but there may be cases I'm not familiar with. > Does anyone know of any use for a bare address packet with regular > aux? > > > > >> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > >> index 74724aa..dfe4cf4 100644 > >> --- a/drivers/gpu/drm/drm_dp_helper.c > >> +++ b/drivers/gpu/drm/drm_dp_helper.c > >> @@ -664,12 +664,23 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, > >> int num) > >> { > >> struct drm_dp_aux *aux = adapter->algo_data; > >> - unsigned int i, j; > >> + unsigned int m, b; > > > > I don't see why these would need to be changed. i and j are perfectly > > fine loop variable names. > > It was easier for me to follow the code since the variables matched > the objects they were iterating, but I can change them back if you'd > prefer. I think there's enough context in the surrounding code to make it obvious what they are used for. I also think that keeping the names makes the diff easier to follow. But if you feel really strongly about keeping m and b I can live with it. > >> - for (i = 0; i < num; i++) { > >> - struct drm_dp_aux_msg msg; > >> - int err; > >> + memset(&msg, 0, sizeof(msg)); > >> > >> + for (m = 0; m < num; m++) { > >> + msg.address = msgs[m].addr; > >> + msg.request = (msgs[m].flags & I2C_M_RD) ? > >> + DP_AUX_I2C_READ : > >> + DP_AUX_I2C_WRITE; > >> + msg.request |= DP_AUX_I2C_MOT; > >> + msg.buffer = NULL; > >> + msg.size = 0; > >> + err = drm_dp_i2c_do_msg(aux, &msg); > >> + if (err < 0) > >> + break; > > > > This seems somewhat brittle to me. Even though I notice that patch 3/4 > > updates a comment that documents these assumptions, I don't see a reason > > for these assumptions in the first place. > > We already assume that in drm_dp_i2c_do_msg() for the retry loop. Ugh... you're right. > > I'd prefer if we simply provided the complete message rather than rely > > on drivers not to touch anything but the reply field. > > Are you suggesting we re-write drm_dp_i2c_do_msg() or move the retry > logic into drm_dp_i2c_xfer()? Do you mind if we do that in a follow > up patch so we can keep it separate from the addition of the bare > address packets? No, looking at the code again, I don't see how we can do much better without sacrificing much of the clarity of the code. But perhaps this could be documented somewhere else than the I2C helpers. Perhaps the struct drm_dp_aux comment should be updated, which would cause the DRM docbook to automatically pick up that change as well. Thierry
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 74724aa..dfe4cf4 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -664,12 +664,23 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num) { struct drm_dp_aux *aux = adapter->algo_data; - unsigned int i, j; + unsigned int m, b; + struct drm_dp_aux_msg msg; + int err = 0; - for (i = 0; i < num; i++) { - struct drm_dp_aux_msg msg; - int err; + memset(&msg, 0, sizeof(msg)); + for (m = 0; m < num; m++) { + msg.address = msgs[m].addr; + msg.request = (msgs[m].flags & I2C_M_RD) ? + DP_AUX_I2C_READ : + DP_AUX_I2C_WRITE; + msg.request |= DP_AUX_I2C_MOT; + msg.buffer = NULL; + msg.size = 0; + err = drm_dp_i2c_do_msg(aux, &msg); + if (err < 0) + break; /* * Many hardware implementations support FIFOs larger than a * single byte, but it has been empirically determined that @@ -677,31 +688,26 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, * decreased performance. Therefore each message is simply * transferred byte-by-byte. */ - for (j = 0; j < msgs[i].len; j++) { - memset(&msg, 0, sizeof(msg)); - msg.address = msgs[i].addr; - - msg.request = (msgs[i].flags & I2C_M_RD) ? - DP_AUX_I2C_READ : - DP_AUX_I2C_WRITE; - - /* - * All messages except the last one are middle-of- - * transfer messages. - */ - if ((i < num - 1) || (j < msgs[i].len - 1)) - msg.request |= DP_AUX_I2C_MOT; - - msg.buffer = msgs[i].buf + j; + for (b = 0; b < msgs[m].len; b++) { + msg.buffer = msgs[m].buf + b; msg.size = 1; err = drm_dp_i2c_do_msg(aux, &msg); if (err < 0) - return err; + break; } + if (err < 0) + break; } - - return num; + if (err >= 0) + err = num; + /* send a bare address packet to close out the connection */ + msg.request &= ~DP_AUX_I2C_MOT; + msg.buffer = NULL; + msg.size = 0; + (void)drm_dp_i2c_do_msg(aux, &msg); + + return err; } static const struct i2c_algorithm drm_dp_i2c_algo = {
We need bare address packets at the start and end of each i2c over aux transaction to properly reset the connection between transactions. This mirrors what the existing dp i2c over aux algo currently does. This fixes EDID fetches on certain monitors especially with dp bridges. v2: update as per Ville's comments - Set buffer to NULL for zero sized packets - abort the entre transaction if one of the messages fails v3: drop leftover debugging code Signed-off-by: Alex Deucher <alexander.deucher@amd.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Cc: Thierry Reding <treding@nvidia.com> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/drm_dp_helper.c | 52 +++++++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 23 deletions(-)