V3 just drops a left over debug statement.
Alex Deucher (4): drm/radeon/dp: handle zero sized i2c over aux transactions drm/dp/i2c: send bare addresses to properly reset i2c connections (v3) drm/dp/i2c: Update comments about common i2c over dp assumptions drm/radeon/dp: switch to the common i2c over aux code
drivers/gpu/drm/drm_dp_helper.c | 56 ++++++------ drivers/gpu/drm/radeon/atombios_dp.c | 132 ++++++----------------------- drivers/gpu/drm/radeon/radeon_connectors.c | 44 ++-------- drivers/gpu/drm/radeon/radeon_display.c | 11 ++- drivers/gpu/drm/radeon/radeon_i2c.c | 60 +++---------- drivers/gpu/drm/radeon/radeon_mode.h | 12 +-- 6 files changed, 87 insertions(+), 228 deletions(-)
Needed for proper i2c over aux handling for certain monitors and configurations (e.g., dp bridges or adapters).
Signed-off-by: Alex Deucher alexander.deucher@amd.com --- drivers/gpu/drm/radeon/atombios_dp.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c index 8b0ab17..e914008 100644 --- a/drivers/gpu/drm/radeon/atombios_dp.c +++ b/drivers/gpu/drm/radeon/atombios_dp.c @@ -143,6 +143,7 @@ static int radeon_process_aux_ch(struct radeon_i2c_chan *chan, }
#define HEADER_SIZE 4 +#define BARE_ADDRESS_SIZE 3
static ssize_t radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) @@ -160,13 +161,16 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) tx_buf[0] = msg->address & 0xff; tx_buf[1] = msg->address >> 8; tx_buf[2] = msg->request << 4; - tx_buf[3] = msg->size - 1; + tx_buf[3] = msg->size ? (msg->size - 1) : 0;
switch (msg->request & ~DP_AUX_I2C_MOT) { case DP_AUX_NATIVE_WRITE: case DP_AUX_I2C_WRITE: tx_size = HEADER_SIZE + msg->size; - tx_buf[3] |= tx_size << 4; + if (msg->size == 0) + tx_buf[3] |= BARE_ADDRESS_SIZE << 4; + else + tx_buf[3] |= tx_size << 4; memcpy(tx_buf + HEADER_SIZE, msg->buffer, msg->size); ret = radeon_process_aux_ch(chan, tx_buf, tx_size, NULL, 0, delay, &ack); @@ -177,7 +181,10 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) case DP_AUX_NATIVE_READ: case DP_AUX_I2C_READ: tx_size = HEADER_SIZE; - tx_buf[3] |= tx_size << 4; + if (msg->size == 0) + tx_buf[3] |= BARE_ADDRESS_SIZE << 4; + else + tx_buf[3] |= tx_size << 4; ret = radeon_process_aux_ch(chan, tx_buf, tx_size, msg->buffer, msg->size, delay, &ack); break; @@ -186,7 +193,7 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) break; }
- if (ret > 0) + if (ret >= 0) msg->reply = ack >> 4;
return ret;
Am 04.04.2014 21:58, schrieb Alex Deucher:
Needed for proper i2c over aux handling for certain monitors and configurations (e.g., dp bridges or adapters).
Signed-off-by: Alex Deucher alexander.deucher@amd.com
My planning was that yesterdays pull request is the last one with new features. I can't judge how invasive this series is, so should I add it to my 3.15 branch and send Dave another pull request or should we wait for 3.16?
Christian.
drivers/gpu/drm/radeon/atombios_dp.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c index 8b0ab17..e914008 100644 --- a/drivers/gpu/drm/radeon/atombios_dp.c +++ b/drivers/gpu/drm/radeon/atombios_dp.c @@ -143,6 +143,7 @@ static int radeon_process_aux_ch(struct radeon_i2c_chan *chan, }
#define HEADER_SIZE 4 +#define BARE_ADDRESS_SIZE 3
static ssize_t radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) @@ -160,13 +161,16 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) tx_buf[0] = msg->address & 0xff; tx_buf[1] = msg->address >> 8; tx_buf[2] = msg->request << 4;
- tx_buf[3] = msg->size - 1;
tx_buf[3] = msg->size ? (msg->size - 1) : 0;
switch (msg->request & ~DP_AUX_I2C_MOT) { case DP_AUX_NATIVE_WRITE: case DP_AUX_I2C_WRITE: tx_size = HEADER_SIZE + msg->size;
tx_buf[3] |= tx_size << 4;
if (msg->size == 0)tx_buf[3] |= BARE_ADDRESS_SIZE << 4;elsetx_buf[3] |= tx_size << 4;@@ -177,7 +181,10 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) case DP_AUX_NATIVE_READ: case DP_AUX_I2C_READ: tx_size = HEADER_SIZE;
tx_buf[3] |= tx_size << 4;
if (msg->size == 0)tx_buf[3] |= BARE_ADDRESS_SIZE << 4;elsetx_buf[3] |= tx_size << 4;@@ -186,7 +193,7 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) break; }
- if (ret > 0)
if (ret >= 0) msg->reply = ack >> 4;
return ret;
On Sat, Apr 05, 2014 at 11:25:01AM +0200, Christian König wrote:
Am 04.04.2014 21:58, schrieb Alex Deucher:
Needed for proper i2c over aux handling for certain monitors and configurations (e.g., dp bridges or adapters).
Signed-off-by: Alex Deucher alexander.deucher@amd.com
My planning was that yesterdays pull request is the last one with new features. I can't judge how invasive this series is, so should I add it to my 3.15 branch and send Dave another pull request or should we wait for 3.16?
At least the two patches for the dp aux helper code should go into 3.15 since they fix a behavioral regression in i915, too. -Daniel
On Sat, Apr 5, 2014 at 5:25 AM, Christian König deathsimple@vodafone.de wrote:
Am 04.04.2014 21:58, schrieb Alex Deucher:
Needed for proper i2c over aux handling for certain monitors and configurations (e.g., dp bridges or adapters).
Signed-off-by: Alex Deucher alexander.deucher@amd.com
My planning was that yesterdays pull request is the last one with new features. I can't judge how invasive this series is, so should I add it to my 3.15 branch and send Dave another pull request or should we wait for 3.16?
I'd like to get them into 3.15. They provide a really nice cleanup for the radeon dp i2c handling.
Alex
Christian.
drivers/gpu/drm/radeon/atombios_dp.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c index 8b0ab17..e914008 100644 --- a/drivers/gpu/drm/radeon/atombios_dp.c +++ b/drivers/gpu/drm/radeon/atombios_dp.c @@ -143,6 +143,7 @@ static int radeon_process_aux_ch(struct radeon_i2c_chan *chan, } #define HEADER_SIZE 4 +#define BARE_ADDRESS_SIZE 3 static ssize_t radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) @@ -160,13 +161,16 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) tx_buf[0] = msg->address & 0xff; tx_buf[1] = msg->address >> 8; tx_buf[2] = msg->request << 4;
tx_buf[3] = msg->size - 1;
tx_buf[3] = msg->size ? (msg->size - 1) : 0; switch (msg->request & ~DP_AUX_I2C_MOT) { case DP_AUX_NATIVE_WRITE: case DP_AUX_I2C_WRITE: tx_size = HEADER_SIZE + msg->size;
tx_buf[3] |= tx_size << 4;
if (msg->size == 0)tx_buf[3] |= BARE_ADDRESS_SIZE << 4;elsetx_buf[3] |= tx_size << 4; memcpy(tx_buf + HEADER_SIZE, msg->buffer, msg->size); ret = radeon_process_aux_ch(chan, tx_buf, tx_size, NULL, 0,delay, &ack); @@ -177,7 +181,10 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) case DP_AUX_NATIVE_READ: case DP_AUX_I2C_READ: tx_size = HEADER_SIZE;
tx_buf[3] |= tx_size << 4;
if (msg->size == 0)tx_buf[3] |= BARE_ADDRESS_SIZE << 4;elsetx_buf[3] |= tx_size << 4; ret = radeon_process_aux_ch(chan, tx_buf, tx_size, msg->buffer,msg->size, delay, &ack); break; @@ -186,7 +193,7 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) break; }
if (ret > 0)
if (ret >= 0) msg->reply = ack >> 4; return ret;
On Fri, 04 Apr 2014, Alex Deucher alexdeucher@gmail.com wrote:
Needed for proper i2c over aux handling for certain monitors and configurations (e.g., dp bridges or adapters).
Signed-off-by: Alex Deucher alexander.deucher@amd.com
drivers/gpu/drm/radeon/atombios_dp.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c index 8b0ab17..e914008 100644 --- a/drivers/gpu/drm/radeon/atombios_dp.c +++ b/drivers/gpu/drm/radeon/atombios_dp.c @@ -143,6 +143,7 @@ static int radeon_process_aux_ch(struct radeon_i2c_chan *chan, }
#define HEADER_SIZE 4 +#define BARE_ADDRESS_SIZE 3
static ssize_t radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) @@ -160,13 +161,16 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) tx_buf[0] = msg->address & 0xff; tx_buf[1] = msg->address >> 8; tx_buf[2] = msg->request << 4;
- tx_buf[3] = msg->size - 1;
tx_buf[3] = msg->size ? (msg->size - 1) : 0;
switch (msg->request & ~DP_AUX_I2C_MOT) { case DP_AUX_NATIVE_WRITE: case DP_AUX_I2C_WRITE: tx_size = HEADER_SIZE + msg->size;
tx_buf[3] |= tx_size << 4;
if (msg->size == 0)tx_buf[3] |= BARE_ADDRESS_SIZE << 4;elsetx_buf[3] |= tx_size << 4;
I think your tz_size and tx_buf[3] are confused. Shouldn't you only send 3 bytes of tx_buf when msg->size == 0?
Disclaimer, I don't know anything about your hw and all that... :)
BR, Jani.
@@ -177,7 +181,10 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) case DP_AUX_NATIVE_READ: case DP_AUX_I2C_READ: tx_size = HEADER_SIZE;
tx_buf[3] |= tx_size << 4;
if (msg->size == 0)tx_buf[3] |= BARE_ADDRESS_SIZE << 4;elsetx_buf[3] |= tx_size << 4;@@ -186,7 +193,7 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) break; }
- if (ret > 0)
if (ret >= 0) msg->reply = ack >> 4;
return ret;
-- 1.8.3.1
On Mon, Apr 7, 2014 at 3:57 AM, Jani Nikula jani.nikula@linux.intel.com wrote:
On Fri, 04 Apr 2014, Alex Deucher alexdeucher@gmail.com wrote:
Needed for proper i2c over aux handling for certain monitors and configurations (e.g., dp bridges or adapters).
Signed-off-by: Alex Deucher alexander.deucher@amd.com
drivers/gpu/drm/radeon/atombios_dp.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c index 8b0ab17..e914008 100644 --- a/drivers/gpu/drm/radeon/atombios_dp.c +++ b/drivers/gpu/drm/radeon/atombios_dp.c @@ -143,6 +143,7 @@ static int radeon_process_aux_ch(struct radeon_i2c_chan *chan, }
#define HEADER_SIZE 4 +#define BARE_ADDRESS_SIZE 3
static ssize_t radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) @@ -160,13 +161,16 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) tx_buf[0] = msg->address & 0xff; tx_buf[1] = msg->address >> 8; tx_buf[2] = msg->request << 4;
tx_buf[3] = msg->size - 1;
tx_buf[3] = msg->size ? (msg->size - 1) : 0; switch (msg->request & ~DP_AUX_I2C_MOT) { case DP_AUX_NATIVE_WRITE: case DP_AUX_I2C_WRITE: tx_size = HEADER_SIZE + msg->size;
tx_buf[3] |= tx_size << 4;
if (msg->size == 0)tx_buf[3] |= BARE_ADDRESS_SIZE << 4;elsetx_buf[3] |= tx_size << 4; memcpy(tx_buf + HEADER_SIZE, msg->buffer, msg->size); ret = radeon_process_aux_ch(chan, tx_buf, tx_size, NULL, 0, delay, &ack);I think your tz_size and tx_buf[3] are confused. Shouldn't you only send 3 bytes of tx_buf when msg->size == 0?
Disclaimer, I don't know anything about your hw and all that... :)
It doesn't really matter. The hw only cares about the size specified in tx_buf[3]. tx_size is only used to determine how much data to the buffer used by the atom table. We end up copying an extra byte that never gets used. I suppose I should fix it up for clarify.
Alex
BR, Jani.
@@ -177,7 +181,10 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) case DP_AUX_NATIVE_READ: case DP_AUX_I2C_READ: tx_size = HEADER_SIZE;
tx_buf[3] |= tx_size << 4;
if (msg->size == 0)tx_buf[3] |= BARE_ADDRESS_SIZE << 4;elsetx_buf[3] |= tx_size << 4; ret = radeon_process_aux_ch(chan, tx_buf, tx_size, msg->buffer, msg->size, delay, &ack); break;@@ -186,7 +193,7 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) break; }
if (ret > 0)
if (ret >= 0) msg->reply = ack >> 4; return ret;-- 1.8.3.1
-- Jani Nikula, Intel Open Source Technology Center
On Mon, Apr 7, 2014 at 9:24 AM, Alex Deucher alexdeucher@gmail.com wrote:
On Mon, Apr 7, 2014 at 3:57 AM, Jani Nikula jani.nikula@linux.intel.com wrote:
On Fri, 04 Apr 2014, Alex Deucher alexdeucher@gmail.com wrote:
Needed for proper i2c over aux handling for certain monitors and configurations (e.g., dp bridges or adapters).
Signed-off-by: Alex Deucher alexander.deucher@amd.com
drivers/gpu/drm/radeon/atombios_dp.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c index 8b0ab17..e914008 100644 --- a/drivers/gpu/drm/radeon/atombios_dp.c +++ b/drivers/gpu/drm/radeon/atombios_dp.c @@ -143,6 +143,7 @@ static int radeon_process_aux_ch(struct radeon_i2c_chan *chan, }
#define HEADER_SIZE 4 +#define BARE_ADDRESS_SIZE 3
static ssize_t radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) @@ -160,13 +161,16 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) tx_buf[0] = msg->address & 0xff; tx_buf[1] = msg->address >> 8; tx_buf[2] = msg->request << 4;
tx_buf[3] = msg->size - 1;
tx_buf[3] = msg->size ? (msg->size - 1) : 0; switch (msg->request & ~DP_AUX_I2C_MOT) { case DP_AUX_NATIVE_WRITE: case DP_AUX_I2C_WRITE: tx_size = HEADER_SIZE + msg->size;
tx_buf[3] |= tx_size << 4;
if (msg->size == 0)tx_buf[3] |= BARE_ADDRESS_SIZE << 4;elsetx_buf[3] |= tx_size << 4; memcpy(tx_buf + HEADER_SIZE, msg->buffer, msg->size); ret = radeon_process_aux_ch(chan, tx_buf, tx_size, NULL, 0, delay, &ack);I think your tz_size and tx_buf[3] are confused. Shouldn't you only send 3 bytes of tx_buf when msg->size == 0?
Disclaimer, I don't know anything about your hw and all that... :)
It doesn't really matter. The hw only cares about the size specified in tx_buf[3]. tx_size is only used to determine how much data to the buffer used by the atom table. We end up copying an extra byte that never gets used. I suppose I should fix it up for clarify.
Actually, I was wrong. We need all 4 bytes of the tx_buf, but the hw only sends 3 based on the size specified in tx_buf[3]. so the code is correct as is.
Alex
Alex
BR, Jani.
@@ -177,7 +181,10 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) case DP_AUX_NATIVE_READ: case DP_AUX_I2C_READ: tx_size = HEADER_SIZE;
tx_buf[3] |= tx_size << 4;
if (msg->size == 0)tx_buf[3] |= BARE_ADDRESS_SIZE << 4;elsetx_buf[3] |= tx_size << 4; ret = radeon_process_aux_ch(chan, tx_buf, tx_size, msg->buffer, msg->size, delay, &ack); break;@@ -186,7 +193,7 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) break; }
if (ret > 0)
if (ret >= 0) msg->reply = ack >> 4; return ret;-- 1.8.3.1
-- Jani Nikula, Intel Open Source Technology Center
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(-)
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 = {
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 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
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
If you are using the common dp over i2c functionality, it is asumed that the aux transfer function does not modify the any of the msg structure other than the reply field. Doing so breaks the logic in the common 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 | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index dfe4cf4..948de4f 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -577,7 +577,9 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
/* * Transfer a single I2C-over-AUX message and handle various error conditions, - * retrying the transaction as appropriate. + * retrying the transaction as appropriate. It is assumed that the + * aux->transfer function does not modify anything in the msg other than the + * reply field. */ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) {
Provides a nice cleanup in radeon.
Signed-off-by: Alex Deucher alexander.deucher@amd.com --- drivers/gpu/drm/radeon/atombios_dp.c | 117 +++++------------------------ drivers/gpu/drm/radeon/radeon_connectors.c | 44 ++--------- drivers/gpu/drm/radeon/radeon_display.c | 11 ++- drivers/gpu/drm/radeon/radeon_i2c.c | 60 ++++----------- drivers/gpu/drm/radeon/radeon_mode.h | 12 +-- 5 files changed, 44 insertions(+), 200 deletions(-)
diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c index e914008..d545769 100644 --- a/drivers/gpu/drm/radeon/atombios_dp.c +++ b/drivers/gpu/drm/radeon/atombios_dp.c @@ -201,98 +201,15 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
void radeon_dp_aux_init(struct radeon_connector *radeon_connector) { - struct radeon_connector_atom_dig *dig_connector = radeon_connector->con_priv; - - dig_connector->dp_i2c_bus->aux.dev = radeon_connector->base.kdev; - dig_connector->dp_i2c_bus->aux.transfer = radeon_dp_aux_transfer; -} - -int radeon_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode, - u8 write_byte, u8 *read_byte) -{ - struct i2c_algo_dp_aux_data *algo_data = adapter->algo_data; - struct radeon_i2c_chan *auxch = i2c_get_adapdata(adapter); - u16 address = algo_data->address; - u8 msg[5]; - u8 reply[2]; - unsigned retry; - int msg_bytes; - int reply_bytes = 1; int ret; - u8 ack; - - /* Set up the address */ - msg[0] = address; - msg[1] = address >> 8; - - /* Set up the command byte */ - if (mode & MODE_I2C_READ) { - msg[2] = DP_AUX_I2C_READ << 4; - msg_bytes = 4; - msg[3] = msg_bytes << 4; - } else { - msg[2] = DP_AUX_I2C_WRITE << 4; - msg_bytes = 5; - msg[3] = msg_bytes << 4; - msg[4] = write_byte; - }
- /* special handling for start/stop */ - if (mode & (MODE_I2C_START | MODE_I2C_STOP)) - msg[3] = 3 << 4; - - /* Set MOT bit for all but stop */ - if ((mode & MODE_I2C_STOP) == 0) - msg[2] |= DP_AUX_I2C_MOT << 4; - - for (retry = 0; retry < 7; retry++) { - ret = radeon_process_aux_ch(auxch, - msg, msg_bytes, reply, reply_bytes, 0, &ack); - if (ret == -EBUSY) - continue; - else if (ret < 0) { - DRM_DEBUG_KMS("aux_ch failed %d\n", ret); - return ret; - } + radeon_connector->ddc_bus->aux.dev = radeon_connector->base.kdev; + radeon_connector->ddc_bus->aux.transfer = radeon_dp_aux_transfer; + ret = drm_dp_aux_register_i2c_bus(&radeon_connector->ddc_bus->aux); + if (!ret) + radeon_connector->ddc_bus->has_aux = true;
- switch ((ack >> 4) & DP_AUX_NATIVE_REPLY_MASK) { - case DP_AUX_NATIVE_REPLY_ACK: - /* I2C-over-AUX Reply field is only valid - * when paired with AUX ACK. - */ - break; - case DP_AUX_NATIVE_REPLY_NACK: - DRM_DEBUG_KMS("aux_ch native nack\n"); - return -EREMOTEIO; - case DP_AUX_NATIVE_REPLY_DEFER: - DRM_DEBUG_KMS("aux_ch native defer\n"); - usleep_range(500, 600); - continue; - default: - DRM_ERROR("aux_ch invalid native reply 0x%02x\n", ack); - return -EREMOTEIO; - } - - switch ((ack >> 4) & DP_AUX_I2C_REPLY_MASK) { - case DP_AUX_I2C_REPLY_ACK: - if (mode == MODE_I2C_READ) - *read_byte = reply[0]; - return ret; - case DP_AUX_I2C_REPLY_NACK: - DRM_DEBUG_KMS("aux_i2c nack\n"); - return -EREMOTEIO; - case DP_AUX_I2C_REPLY_DEFER: - DRM_DEBUG_KMS("aux_i2c defer\n"); - usleep_range(400, 500); - break; - default: - DRM_ERROR("aux_i2c invalid reply 0x%02x\n", ack); - return -EREMOTEIO; - } - } - - DRM_DEBUG_KMS("aux i2c too many retries, giving up\n"); - return -EREMOTEIO; + WARN(ret, "drm_dp_aux_register_i2c_bus() failed with error %d\n", ret); }
/***** general DP utility functions *****/ @@ -427,12 +344,11 @@ static u8 radeon_dp_encoder_service(struct radeon_device *rdev,
u8 radeon_dp_getsinktype(struct radeon_connector *radeon_connector) { - struct radeon_connector_atom_dig *dig_connector = radeon_connector->con_priv; struct drm_device *dev = radeon_connector->base.dev; struct radeon_device *rdev = dev->dev_private;
return radeon_dp_encoder_service(rdev, ATOM_DP_ACTION_GET_SINK_TYPE, 0, - dig_connector->dp_i2c_bus->rec.i2c_id, 0); + radeon_connector->ddc_bus->rec.i2c_id, 0); }
static void radeon_dp_probe_oui(struct radeon_connector *radeon_connector) @@ -443,11 +359,11 @@ static void radeon_dp_probe_oui(struct radeon_connector *radeon_connector) if (!(dig_connector->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT)) return;
- if (drm_dp_dpcd_read(&dig_connector->dp_i2c_bus->aux, DP_SINK_OUI, buf, 3)) + if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux, DP_SINK_OUI, buf, 3)) DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n", buf[0], buf[1], buf[2]);
- if (drm_dp_dpcd_read(&dig_connector->dp_i2c_bus->aux, DP_BRANCH_OUI, buf, 3)) + if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux, DP_BRANCH_OUI, buf, 3)) DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n", buf[0], buf[1], buf[2]); } @@ -458,7 +374,7 @@ bool radeon_dp_getdpcd(struct radeon_connector *radeon_connector) u8 msg[DP_DPCD_SIZE]; int ret, i;
- ret = drm_dp_dpcd_read(&dig_connector->dp_i2c_bus->aux, DP_DPCD_REV, msg, + ret = drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux, DP_DPCD_REV, msg, DP_DPCD_SIZE); if (ret > 0) { memcpy(dig_connector->dpcd, msg, DP_DPCD_SIZE); @@ -496,7 +412,7 @@ int radeon_dp_get_panel_mode(struct drm_encoder *encoder,
if (dp_bridge != ENCODER_OBJECT_ID_NONE) { /* DP bridge chips */ - drm_dp_dpcd_readb(&dig_connector->dp_i2c_bus->aux, + drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux, DP_EDP_CONFIGURATION_CAP, &tmp); if (tmp & 1) panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE; @@ -507,7 +423,7 @@ int radeon_dp_get_panel_mode(struct drm_encoder *encoder, panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE; } else if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) { /* eDP */ - drm_dp_dpcd_readb(&dig_connector->dp_i2c_bus->aux, + drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux, DP_EDP_CONFIGURATION_CAP, &tmp); if (tmp & 1) panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE; @@ -561,7 +477,8 @@ bool radeon_dp_needs_link_train(struct radeon_connector *radeon_connector) u8 link_status[DP_LINK_STATUS_SIZE]; struct radeon_connector_atom_dig *dig = radeon_connector->con_priv;
- if (drm_dp_dpcd_read_link_status(&dig->dp_i2c_bus->aux, link_status) <= 0) + if (drm_dp_dpcd_read_link_status(&radeon_connector->ddc_bus->aux, link_status) + <= 0) return false; if (drm_dp_channel_eq_ok(link_status, dig->dp_lane_count)) return false; @@ -581,7 +498,7 @@ void radeon_dp_set_rx_power_state(struct drm_connector *connector,
/* power up/down the sink */ if (dig_connector->dpcd[0] >= 0x11) { - drm_dp_dpcd_writeb(&dig_connector->dp_i2c_bus->aux, + drm_dp_dpcd_writeb(&radeon_connector->ddc_bus->aux, DP_SET_POWER, power_state); usleep_range(1000, 2000); } @@ -885,7 +802,7 @@ void radeon_dp_link_train(struct drm_encoder *encoder, else dp_info.enc_id |= ATOM_DP_CONFIG_LINK_A;
- drm_dp_dpcd_readb(&dig_connector->dp_i2c_bus->aux, DP_MAX_LANE_COUNT, &tmp); + drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux, DP_MAX_LANE_COUNT, &tmp); if (ASIC_IS_DCE5(rdev) && (tmp & DP_TPS3_SUPPORTED)) dp_info.tp3_supported = true; else @@ -897,7 +814,7 @@ void radeon_dp_link_train(struct drm_encoder *encoder, dp_info.connector = connector; dp_info.dp_lane_count = dig_connector->dp_lane_count; dp_info.dp_clock = dig_connector->dp_clock; - dp_info.aux = &dig_connector->dp_i2c_bus->aux; + dp_info.aux = &radeon_connector->ddc_bus->aux;
if (radeon_dp_link_train_init(&dp_info)) goto done; diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c index ec958e86..e1388cd 100644 --- a/drivers/gpu/drm/radeon/radeon_connectors.c +++ b/drivers/gpu/drm/radeon/radeon_connectors.c @@ -1261,21 +1261,6 @@ static const struct drm_connector_funcs radeon_dvi_connector_funcs = { .force = radeon_dvi_force, };
-static void radeon_dp_connector_destroy(struct drm_connector *connector) -{ - struct radeon_connector *radeon_connector = to_radeon_connector(connector); - struct radeon_connector_atom_dig *radeon_dig_connector = radeon_connector->con_priv; - - if (radeon_connector->edid) - kfree(radeon_connector->edid); - if (radeon_dig_connector->dp_i2c_bus) - radeon_i2c_destroy(radeon_dig_connector->dp_i2c_bus); - kfree(radeon_connector->con_priv); - drm_sysfs_connector_remove(connector); - drm_connector_cleanup(connector); - kfree(connector); -} - static int radeon_dp_get_modes(struct drm_connector *connector) { struct radeon_connector *radeon_connector = to_radeon_connector(connector); @@ -1553,7 +1538,7 @@ static const struct drm_connector_funcs radeon_dp_connector_funcs = { .detect = radeon_dp_detect, .fill_modes = drm_helper_probe_single_connector_modes, .set_property = radeon_connector_set_property, - .destroy = radeon_dp_connector_destroy, + .destroy = radeon_connector_destroy, .force = radeon_dvi_force, };
@@ -1562,7 +1547,7 @@ static const struct drm_connector_funcs radeon_edp_connector_funcs = { .detect = radeon_dp_detect, .fill_modes = drm_helper_probe_single_connector_modes, .set_property = radeon_lvds_set_property, - .destroy = radeon_dp_connector_destroy, + .destroy = radeon_connector_destroy, .force = radeon_dvi_force, };
@@ -1571,7 +1556,7 @@ static const struct drm_connector_funcs radeon_lvds_bridge_connector_funcs = { .detect = radeon_dp_detect, .fill_modes = drm_helper_probe_single_connector_modes, .set_property = radeon_lvds_set_property, - .destroy = radeon_dp_connector_destroy, + .destroy = radeon_connector_destroy, .force = radeon_dvi_force, };
@@ -1668,17 +1653,10 @@ radeon_add_atom_connector(struct drm_device *dev, radeon_dig_connector->igp_lane_info = igp_lane_info; radeon_connector->con_priv = radeon_dig_connector; if (i2c_bus->valid) { - /* add DP i2c bus */ - if (connector_type == DRM_MODE_CONNECTOR_eDP) - radeon_dig_connector->dp_i2c_bus = radeon_i2c_create_dp(dev, i2c_bus, "eDP-auxch"); - else - radeon_dig_connector->dp_i2c_bus = radeon_i2c_create_dp(dev, i2c_bus, "DP-auxch"); - if (radeon_dig_connector->dp_i2c_bus) + radeon_connector->ddc_bus = radeon_i2c_lookup(rdev, i2c_bus); + if (radeon_connector->ddc_bus) has_aux = true; else - DRM_ERROR("DP: Failed to assign dp ddc bus! Check dmesg for i2c errors.\n"); - radeon_connector->ddc_bus = radeon_i2c_lookup(rdev, i2c_bus); - if (!radeon_connector->ddc_bus) DRM_ERROR("DP: Failed to assign ddc bus! Check dmesg for i2c errors.\n"); } switch (connector_type) { @@ -1893,10 +1871,6 @@ radeon_add_atom_connector(struct drm_device *dev, drm_connector_init(dev, &radeon_connector->base, &radeon_dp_connector_funcs, connector_type); drm_connector_helper_add(&radeon_connector->base, &radeon_dp_connector_helper_funcs); if (i2c_bus->valid) { - /* add DP i2c bus */ - radeon_dig_connector->dp_i2c_bus = radeon_i2c_create_dp(dev, i2c_bus, "DP-auxch"); - if (!radeon_dig_connector->dp_i2c_bus) - DRM_ERROR("DP: Failed to assign dp ddc bus! Check dmesg for i2c errors.\n"); radeon_connector->ddc_bus = radeon_i2c_lookup(rdev, i2c_bus); if (radeon_connector->ddc_bus) has_aux = true; @@ -1942,14 +1916,10 @@ radeon_add_atom_connector(struct drm_device *dev, drm_connector_init(dev, &radeon_connector->base, &radeon_edp_connector_funcs, connector_type); drm_connector_helper_add(&radeon_connector->base, &radeon_dp_connector_helper_funcs); if (i2c_bus->valid) { - /* add DP i2c bus */ - radeon_dig_connector->dp_i2c_bus = radeon_i2c_create_dp(dev, i2c_bus, "eDP-auxch"); - if (radeon_dig_connector->dp_i2c_bus) + radeon_connector->ddc_bus = radeon_i2c_lookup(rdev, i2c_bus); + if (radeon_connector->ddc_bus) has_aux = true; else - DRM_ERROR("DP: Failed to assign dp ddc bus! Check dmesg for i2c errors.\n"); - radeon_connector->ddc_bus = radeon_i2c_lookup(rdev, i2c_bus); - if (!radeon_connector->ddc_bus) DRM_ERROR("DP: Failed to assign ddc bus! Check dmesg for i2c errors.\n"); } drm_object_attach_property(&radeon_connector->base.base, diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c index fbd8b93..04bf503 100644 --- a/drivers/gpu/drm/radeon/radeon_display.c +++ b/drivers/gpu/drm/radeon/radeon_display.c @@ -757,19 +757,18 @@ int radeon_ddc_get_modes(struct radeon_connector *radeon_connector)
if (radeon_connector_encoder_get_dp_bridge_encoder_id(&radeon_connector->base) != ENCODER_OBJECT_ID_NONE) { - struct radeon_connector_atom_dig *dig = radeon_connector->con_priv; - - if (dig->dp_i2c_bus) + if (radeon_connector->ddc_bus->has_aux) radeon_connector->edid = drm_get_edid(&radeon_connector->base, - &dig->dp_i2c_bus->adapter); + &radeon_connector->ddc_bus->aux.ddc); } else if ((radeon_connector->base.connector_type == DRM_MODE_CONNECTOR_DisplayPort) || (radeon_connector->base.connector_type == DRM_MODE_CONNECTOR_eDP)) { struct radeon_connector_atom_dig *dig = radeon_connector->con_priv;
if ((dig->dp_sink_type == CONNECTOR_OBJECT_ID_DISPLAYPORT || - dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) && dig->dp_i2c_bus) + dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) && + radeon_connector->ddc_bus->has_aux) radeon_connector->edid = drm_get_edid(&radeon_connector->base, - &dig->dp_i2c_bus->adapter); + &radeon_connector->ddc_bus->aux.ddc); else if (radeon_connector->ddc_bus && !radeon_connector->edid) radeon_connector->edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter); diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c b/drivers/gpu/drm/radeon/radeon_i2c.c index e24ca6a..7b94414 100644 --- a/drivers/gpu/drm/radeon/radeon_i2c.c +++ b/drivers/gpu/drm/radeon/radeon_i2c.c @@ -64,8 +64,7 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector, bool use_aux) radeon_router_select_ddc_port(radeon_connector);
if (use_aux) { - struct radeon_connector_atom_dig *dig = radeon_connector->con_priv; - ret = i2c_transfer(&dig->dp_i2c_bus->adapter, msgs, 2); + ret = i2c_transfer(&radeon_connector->ddc_bus->aux.ddc, msgs, 2); } else { ret = i2c_transfer(&radeon_connector->ddc_bus->adapter, msgs, 2); } @@ -950,16 +949,16 @@ struct radeon_i2c_chan *radeon_i2c_create(struct drm_device *dev, /* set the radeon bit adapter */ snprintf(i2c->adapter.name, sizeof(i2c->adapter.name), "Radeon i2c bit bus %s", name); - i2c->adapter.algo_data = &i2c->algo.bit; - i2c->algo.bit.pre_xfer = pre_xfer; - i2c->algo.bit.post_xfer = post_xfer; - i2c->algo.bit.setsda = set_data; - i2c->algo.bit.setscl = set_clock; - i2c->algo.bit.getsda = get_data; - i2c->algo.bit.getscl = get_clock; - i2c->algo.bit.udelay = 10; - i2c->algo.bit.timeout = usecs_to_jiffies(2200); /* from VESA */ - i2c->algo.bit.data = i2c; + i2c->adapter.algo_data = &i2c->bit; + i2c->bit.pre_xfer = pre_xfer; + i2c->bit.post_xfer = post_xfer; + i2c->bit.setsda = set_data; + i2c->bit.setscl = set_clock; + i2c->bit.getsda = get_data; + i2c->bit.getscl = get_clock; + i2c->bit.udelay = 10; + i2c->bit.timeout = usecs_to_jiffies(2200); /* from VESA */ + i2c->bit.data = i2c; ret = i2c_bit_add_bus(&i2c->adapter); if (ret) { DRM_ERROR("Failed to register bit i2c %s\n", name); @@ -974,46 +973,13 @@ out_free:
}
-struct radeon_i2c_chan *radeon_i2c_create_dp(struct drm_device *dev, - struct radeon_i2c_bus_rec *rec, - const char *name) -{ - struct radeon_i2c_chan *i2c; - int ret; - - i2c = kzalloc(sizeof(struct radeon_i2c_chan), GFP_KERNEL); - if (i2c == NULL) - return NULL; - - i2c->rec = *rec; - i2c->adapter.owner = THIS_MODULE; - i2c->adapter.class = I2C_CLASS_DDC; - i2c->adapter.dev.parent = &dev->pdev->dev; - i2c->dev = dev; - snprintf(i2c->adapter.name, sizeof(i2c->adapter.name), - "Radeon aux bus %s", name); - i2c_set_adapdata(&i2c->adapter, i2c); - i2c->adapter.algo_data = &i2c->algo.dp; - i2c->algo.dp.aux_ch = radeon_dp_i2c_aux_ch; - i2c->algo.dp.address = 0; - ret = i2c_dp_aux_add_bus(&i2c->adapter); - if (ret) { - DRM_INFO("Failed to register i2c %s\n", name); - goto out_free; - } - - return i2c; -out_free: - kfree(i2c); - return NULL; - -} - void radeon_i2c_destroy(struct radeon_i2c_chan *i2c) { if (!i2c) return; i2c_del_adapter(&i2c->adapter); + if (i2c->has_aux) + drm_dp_aux_unregister_i2c_bus(&i2c->aux); kfree(i2c); }
diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h index 832d9fa..6ddf31a 100644 --- a/drivers/gpu/drm/radeon/radeon_mode.h +++ b/drivers/gpu/drm/radeon/radeon_mode.h @@ -187,12 +187,10 @@ struct radeon_pll { struct radeon_i2c_chan { struct i2c_adapter adapter; struct drm_device *dev; - union { - struct i2c_algo_bit_data bit; - struct i2c_algo_dp_aux_data dp; - } algo; + struct i2c_algo_bit_data bit; struct radeon_i2c_bus_rec rec; struct drm_dp_aux aux; + bool has_aux; };
/* mostly for macs, but really any system without connector tables */ @@ -440,7 +438,6 @@ struct radeon_encoder { struct radeon_connector_atom_dig { uint32_t igp_lane_info; /* displayport */ - struct radeon_i2c_chan *dp_i2c_bus; u8 dpcd[DP_RECEIVER_CAP_SIZE]; u8 dp_sink_type; int dp_clock; @@ -702,8 +699,6 @@ extern void atombios_dig_transmitter_setup(struct drm_encoder *encoder, uint8_t lane_set); extern void radeon_atom_ext_encoder_setup_ddc(struct drm_encoder *encoder); extern struct drm_encoder *radeon_get_external_encoder(struct drm_encoder *encoder); -extern int radeon_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode, - u8 write_byte, u8 *read_byte); void radeon_atom_copy_swap(u8 *dst, u8 *src, u8 num_bytes, bool to_le);
extern void radeon_i2c_init(struct radeon_device *rdev); @@ -715,9 +710,6 @@ extern void radeon_i2c_add(struct radeon_device *rdev, const char *name); extern struct radeon_i2c_chan *radeon_i2c_lookup(struct radeon_device *rdev, struct radeon_i2c_bus_rec *i2c_bus); -extern struct radeon_i2c_chan *radeon_i2c_create_dp(struct drm_device *dev, - struct radeon_i2c_bus_rec *rec, - const char *name); extern struct radeon_i2c_chan *radeon_i2c_create(struct drm_device *dev, struct radeon_i2c_bus_rec *rec, const char *name);
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Christian König -
Daniel Vetter -
Jani Nikula -
Thierry Reding