Hi,
This small series introduces some infrastructure to support AUX channels in a generic way. Drivers make use of it by embedding and filling in a struct drm_dp_aux. Various helpers can then be used to for example read from or write to the DPCD.
Patch 1 adds the basic infrastructure as well as a couple of helpers to access the DPCD.
The helper introduced in patch 2 can be used to obtain the link status as expected by various existing DP helpers.
More convenience helpers are added in patch 3, which can come in handy during DP initialization.
An AUX channel can also be used to implement I2C-over-AUX and patch 4 implements an I2C adapter that can be used with the DRM EDID helpers.
Changes in v2: - reimplement I2C-over-AUX functionality to get rid of the additional layer - extract retry logic from existing drivers - add more kerneldoc comments
Thierry
Thierry Reding (4): drm/dp: Add AUX channel infrastructure drm/dp: Add drm_dp_dpcd_read_link_status() drm/dp: Add DisplayPort link helpers drm/dp: Allow registering AUX channels as I2C busses
drivers/gpu/drm/drm_dp_helper.c | 387 ++++++++++++++++++++++++++++++++++++++++ include/drm/drm_dp_helper.h | 101 +++++++++++ 2 files changed, 488 insertions(+)
This is a superset of the current i2c_dp_aux bus functionality and can be used to transfer native AUX in addition to I2C-over-AUX messages.
Helpers are provided to read and write the DPCD, either blockwise or byte-wise. Many of the existing helpers for DisplayPort take a copy of a portion of the DPCD and operate on that, without a way to write data back to the DPCD (e.g. for configuration of the link).
Subsequent patches will build upon this infrastructure to provide common functionality in a generic way.
Signed-off-by: Thierry Reding treding@nvidia.com --- drivers/gpu/drm/drm_dp_helper.c | 116 ++++++++++++++++++++++++++++++++++++++++ include/drm/drm_dp_helper.h | 64 ++++++++++++++++++++++ 2 files changed, 180 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 9e978aae8972..bd622d462ca7 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -346,3 +346,119 @@ int drm_dp_bw_code_to_link_rate(u8 link_bw) } } EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate); + +/** + * drm_dp_dpcd_read() - read a series of bytes from the DPCD + * @aux: DisplayPort AUX channel + * @offset: address of the (first) register to read + * @buffer: buffer to store the register values + * @size: number of bytes in @buffer + * + * Returns the number of bytes transferred on success, or a negative error + * code on failure. + */ +ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset, + void *buffer, size_t size) +{ + struct drm_dp_aux_msg msg; + unsigned int retry; + int err; + + memset(&msg, 0, sizeof(msg)); + msg.address = offset; + msg.request = DP_AUX_NATIVE_READ; + msg.buffer = buffer; + msg.size = size; + + /* + * The specification doesn't give any recommendation on how often to + * retry native transactions, so retry 7 times like for I2C-over-AUX + * transactions. + */ + for (retry = 0; retry < 7; retry++) { + err = aux->transfer(aux, &msg); + if (err < 0) { + if (err == -EBUSY) + continue; + + return err; + } + + if (err == 0) + return -EPROTO; + + switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) { + case DP_AUX_NATIVE_REPLY_ACK: + return err; + + case DP_AUX_NATIVE_REPLY_NACK: + return -EIO; + + case DP_AUX_NATIVE_REPLY_DEFER: + usleep_range(400, 500); + break; + } + } + + DRM_ERROR("too many retries, giving up\n"); + return -EIO; +} +EXPORT_SYMBOL(drm_dp_dpcd_read); + +/** + * drm_dp_dpcd_write() - write a series of bytes to the DPCD + * @aux: DisplayPort AUX channel + * @offset: address of the (first) register to write + * @buffer: buffer containing the values to write + * @size: number of bytes in @buffer + * + * Returns the number of bytes transferred on success, or a negative error + * code on failure. + */ +ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset, + void *buffer, size_t size) +{ + struct drm_dp_aux_msg msg; + unsigned int retry; + int err; + + memset(&msg, 0, sizeof(msg)); + msg.address = offset; + msg.request = DP_AUX_NATIVE_WRITE; + msg.buffer = buffer; + msg.size = size; + + /* + * The specification doesn't give any recommendation on how often to + * retry native transactions, so retry 7 times like for I2C-over-AUX + * transactions. + */ + for (retry = 0; retry < 7; retry++) { + err = aux->transfer(aux, &msg); + if (err < 0) { + if (err == -EBUSY) + continue; + + return err; + } + + if (err == 0) + return -EPROTO; + + switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) { + case DP_AUX_NATIVE_REPLY_ACK: + return 0; + + case DP_AUX_NATIVE_REPLY_NACK: + return -EIO; + + case DP_AUX_NATIVE_REPLY_DEFER: + usleep_range(400, 500); + break; + } + } + + DRM_ERROR("too many retries, giving up\n"); + return -EIO; +} +EXPORT_SYMBOL(drm_dp_dpcd_write); diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index c873f9ce5871..0b14e799cd98 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -397,4 +397,68 @@ drm_dp_enhanced_frame_cap(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) (dpcd[DP_MAX_LANE_COUNT] & DP_ENHANCED_FRAME_CAP); }
+/* + * DisplayPort AUX channel + */ + +/** + * struct drm_dp_aux_msg - DisplayPort AUX channel transaction + * @address: address of the (first) register to access + * @flags: contains the type of transaction as well as flags (see above) + * @reply: upon completion, contains the reply type of the transaction + * @buffer: pointer to a transmission or reception buffer + * @size: size of @buffer + */ +struct drm_dp_aux_msg { + unsigned int address; + u8 request; + u8 reply; + void *buffer; + size_t size; +}; + +/** + * struct drm_dp_aux - DisplayPort AUX channel + * @transfer: transfers a message representing a single AUX transaction + */ +struct drm_dp_aux { + ssize_t (*transfer)(struct drm_dp_aux *aux, + struct drm_dp_aux_msg *msg); +}; + +ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset, + void *buffer, size_t size); +ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset, + void *buffer, size_t size); + +/** + * drm_dp_dpcd_readb() - read a single byte from the DPCD + * @aux: DisplayPort AUX channel + * @offset: address of the register to read + * @valuep: location where the value of the register will be stored + * + * Returns the number of bytes transferred (1) on success, or a negative + * error code on failure. + */ +static inline ssize_t drm_dp_dpcd_readb(struct drm_dp_aux *aux, + unsigned int offset, u8 *valuep) +{ + return drm_dp_dpcd_read(aux, offset, valuep, 1); +} + +/** + * drm_dp_dpcd_writeb() - write a single byte to the DPCD + * @aux: DisplayPort AUX channel + * @offset: address of the register to write + * @valuep: value to write to the register + * + * Returns the number of bytes transferred (1) on success, or a negative + * error code on failure. + */ +static inline ssize_t drm_dp_dpcd_writeb(struct drm_dp_aux *aux, u8 value, + unsigned int offset) +{ + return drm_dp_dpcd_write(aux, offset, &value, 1); +} + #endif /* _DRM_DP_HELPER_H_ */
On Tue, 17 Dec 2013, Thierry Reding thierry.reding@gmail.com wrote:
This is a superset of the current i2c_dp_aux bus functionality and can be used to transfer native AUX in addition to I2C-over-AUX messages.
Helpers are provided to read and write the DPCD, either blockwise or byte-wise. Many of the existing helpers for DisplayPort take a copy of a portion of the DPCD and operate on that, without a way to write data back to the DPCD (e.g. for configuration of the link).
Subsequent patches will build upon this infrastructure to provide common functionality in a generic way.
Signed-off-by: Thierry Reding treding@nvidia.com
drivers/gpu/drm/drm_dp_helper.c | 116 ++++++++++++++++++++++++++++++++++++++++ include/drm/drm_dp_helper.h | 64 ++++++++++++++++++++++ 2 files changed, 180 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 9e978aae8972..bd622d462ca7 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -346,3 +346,119 @@ int drm_dp_bw_code_to_link_rate(u8 link_bw) } } EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);
+/**
- drm_dp_dpcd_read() - read a series of bytes from the DPCD
- @aux: DisplayPort AUX channel
- @offset: address of the (first) register to read
- @buffer: buffer to store the register values
- @size: number of bytes in @buffer
- Returns the number of bytes transferred on success, or a negative error
- code on failure.
- */
+ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
void *buffer, size_t size)+{
- struct drm_dp_aux_msg msg;
- unsigned int retry;
- int err;
- memset(&msg, 0, sizeof(msg));
- msg.address = offset;
- msg.request = DP_AUX_NATIVE_READ;
- msg.buffer = buffer;
- msg.size = size;
- /*
* The specification doesn't give any recommendation on how often to* retry native transactions, so retry 7 times like for I2C-over-AUX* transactions.*/- for (retry = 0; retry < 7; retry++) {
err = aux->transfer(aux, &msg);if (err < 0) {if (err == -EBUSY)continue;return err;}if (err == 0)return -EPROTO;switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {case DP_AUX_NATIVE_REPLY_ACK:return err;case DP_AUX_NATIVE_REPLY_NACK:return -EIO;case DP_AUX_NATIVE_REPLY_DEFER:usleep_range(400, 500);break;}- }
- DRM_ERROR("too many retries, giving up\n");
- return -EIO;
+} +EXPORT_SYMBOL(drm_dp_dpcd_read);
+/**
- drm_dp_dpcd_write() - write a series of bytes to the DPCD
- @aux: DisplayPort AUX channel
- @offset: address of the (first) register to write
- @buffer: buffer containing the values to write
- @size: number of bytes in @buffer
- Returns the number of bytes transferred on success, or a negative error
- code on failure.
- */
+ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
void *buffer, size_t size)+{
- struct drm_dp_aux_msg msg;
- unsigned int retry;
- int err;
- memset(&msg, 0, sizeof(msg));
- msg.address = offset;
- msg.request = DP_AUX_NATIVE_WRITE;
- msg.buffer = buffer;
- msg.size = size;
- /*
* The specification doesn't give any recommendation on how often to* retry native transactions, so retry 7 times like for I2C-over-AUX* transactions.*/- for (retry = 0; retry < 7; retry++) {
err = aux->transfer(aux, &msg);if (err < 0) {if (err == -EBUSY)continue;return err;}if (err == 0)return -EPROTO;switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {case DP_AUX_NATIVE_REPLY_ACK:return 0;
"Returns the number of bytes transferred on success, or a negative error code on failure." Compare drm_dp_dpcd_read.
You could add an internal helper to do both read and write, it's mostly the same code.
BR, Jani.
case DP_AUX_NATIVE_REPLY_NACK:return -EIO;case DP_AUX_NATIVE_REPLY_DEFER:usleep_range(400, 500);break;}- }
- DRM_ERROR("too many retries, giving up\n");
- return -EIO;
+} +EXPORT_SYMBOL(drm_dp_dpcd_write); diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index c873f9ce5871..0b14e799cd98 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -397,4 +397,68 @@ drm_dp_enhanced_frame_cap(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) (dpcd[DP_MAX_LANE_COUNT] & DP_ENHANCED_FRAME_CAP); }
+/*
- DisplayPort AUX channel
- */
+/**
- struct drm_dp_aux_msg - DisplayPort AUX channel transaction
- @address: address of the (first) register to access
- @flags: contains the type of transaction as well as flags (see above)
- @reply: upon completion, contains the reply type of the transaction
- @buffer: pointer to a transmission or reception buffer
- @size: size of @buffer
- */
+struct drm_dp_aux_msg {
- unsigned int address;
- u8 request;
- u8 reply;
- void *buffer;
- size_t size;
+};
+/**
- struct drm_dp_aux - DisplayPort AUX channel
- @transfer: transfers a message representing a single AUX transaction
- */
+struct drm_dp_aux {
- ssize_t (*transfer)(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg);+};
+ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
void *buffer, size_t size);+ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
void *buffer, size_t size);+/**
- drm_dp_dpcd_readb() - read a single byte from the DPCD
- @aux: DisplayPort AUX channel
- @offset: address of the register to read
- @valuep: location where the value of the register will be stored
- Returns the number of bytes transferred (1) on success, or a negative
- error code on failure.
- */
+static inline ssize_t drm_dp_dpcd_readb(struct drm_dp_aux *aux,
unsigned int offset, u8 *valuep)+{
- return drm_dp_dpcd_read(aux, offset, valuep, 1);
+}
+/**
- drm_dp_dpcd_writeb() - write a single byte to the DPCD
- @aux: DisplayPort AUX channel
- @offset: address of the register to write
- @valuep: value to write to the register
- Returns the number of bytes transferred (1) on success, or a negative
- error code on failure.
- */
+static inline ssize_t drm_dp_dpcd_writeb(struct drm_dp_aux *aux, u8 value,
unsigned int offset)+{
- return drm_dp_dpcd_write(aux, offset, &value, 1);
+}
#endif /* _DRM_DP_HELPER_H_ */
1.8.4.2
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Dec 17, 2013 at 06:44:30PM +0200, Jani Nikula wrote:
On Tue, 17 Dec 2013, Thierry Reding thierry.reding@gmail.com wrote:
[...]
+/**
- drm_dp_dpcd_write() - write a series of bytes to the DPCD
- @aux: DisplayPort AUX channel
- @offset: address of the (first) register to write
- @buffer: buffer containing the values to write
- @size: number of bytes in @buffer
- Returns the number of bytes transferred on success, or a negative error
- code on failure.
- */
+ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
void *buffer, size_t size)
[...]
switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {case DP_AUX_NATIVE_REPLY_ACK:return 0;"Returns the number of bytes transferred on success, or a negative error code on failure." Compare drm_dp_dpcd_read.
Good catch!
You could add an internal helper to do both read and write, it's mostly the same code.
Yes, I've factored out drm_dp_dpcd_access() which takes an additional request parameter. That's the only difference between both functions.
Thanks, Thierry
On Tue, 17 Dec 2013, Thierry Reding thierry.reding@gmail.com wrote:
This is a superset of the current i2c_dp_aux bus functionality and can be used to transfer native AUX in addition to I2C-over-AUX messages.
Helpers are provided to read and write the DPCD, either blockwise or byte-wise. Many of the existing helpers for DisplayPort take a copy of a portion of the DPCD and operate on that, without a way to write data back to the DPCD (e.g. for configuration of the link).
Subsequent patches will build upon this infrastructure to provide common functionality in a generic way.
Signed-off-by: Thierry Reding treding@nvidia.com
drivers/gpu/drm/drm_dp_helper.c | 116 ++++++++++++++++++++++++++++++++++++++++ include/drm/drm_dp_helper.h | 64 ++++++++++++++++++++++ 2 files changed, 180 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 9e978aae8972..bd622d462ca7 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -346,3 +346,119 @@ int drm_dp_bw_code_to_link_rate(u8 link_bw) } } EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);
+/**
- drm_dp_dpcd_read() - read a series of bytes from the DPCD
- @aux: DisplayPort AUX channel
- @offset: address of the (first) register to read
- @buffer: buffer to store the register values
- @size: number of bytes in @buffer
- Returns the number of bytes transferred on success, or a negative error
- code on failure.
- */
+ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
void *buffer, size_t size)+{
- struct drm_dp_aux_msg msg;
- unsigned int retry;
- int err;
- memset(&msg, 0, sizeof(msg));
- msg.address = offset;
- msg.request = DP_AUX_NATIVE_READ;
- msg.buffer = buffer;
- msg.size = size;
- /*
* The specification doesn't give any recommendation on how often to* retry native transactions, so retry 7 times like for I2C-over-AUX* transactions.*/- for (retry = 0; retry < 7; retry++) {
err = aux->transfer(aux, &msg);if (err < 0) {if (err == -EBUSY)continue;return err;}if (err == 0)return -EPROTO;switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {case DP_AUX_NATIVE_REPLY_ACK:return err;case DP_AUX_NATIVE_REPLY_NACK:return -EIO;case DP_AUX_NATIVE_REPLY_DEFER:usleep_range(400, 500);break;}- }
- DRM_ERROR("too many retries, giving up\n");
- return -EIO;
+} +EXPORT_SYMBOL(drm_dp_dpcd_read);
+/**
- drm_dp_dpcd_write() - write a series of bytes to the DPCD
- @aux: DisplayPort AUX channel
- @offset: address of the (first) register to write
- @buffer: buffer containing the values to write
- @size: number of bytes in @buffer
- Returns the number of bytes transferred on success, or a negative error
- code on failure.
- */
+ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
void *buffer, size_t size)+{
- struct drm_dp_aux_msg msg;
- unsigned int retry;
- int err;
- memset(&msg, 0, sizeof(msg));
- msg.address = offset;
- msg.request = DP_AUX_NATIVE_WRITE;
- msg.buffer = buffer;
- msg.size = size;
- /*
* The specification doesn't give any recommendation on how often to* retry native transactions, so retry 7 times like for I2C-over-AUX* transactions.*/- for (retry = 0; retry < 7; retry++) {
err = aux->transfer(aux, &msg);if (err < 0) {if (err == -EBUSY)continue;return err;}if (err == 0)return -EPROTO;switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {case DP_AUX_NATIVE_REPLY_ACK:return 0;case DP_AUX_NATIVE_REPLY_NACK:return -EIO;case DP_AUX_NATIVE_REPLY_DEFER:usleep_range(400, 500);break;}- }
- DRM_ERROR("too many retries, giving up\n");
- return -EIO;
+} +EXPORT_SYMBOL(drm_dp_dpcd_write); diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index c873f9ce5871..0b14e799cd98 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -397,4 +397,68 @@ drm_dp_enhanced_frame_cap(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) (dpcd[DP_MAX_LANE_COUNT] & DP_ENHANCED_FRAME_CAP); }
+/*
- DisplayPort AUX channel
- */
+/**
- struct drm_dp_aux_msg - DisplayPort AUX channel transaction
- @address: address of the (first) register to access
- @flags: contains the type of transaction as well as flags (see above)
- @reply: upon completion, contains the reply type of the transaction
- @buffer: pointer to a transmission or reception buffer
- @size: size of @buffer
- */
+struct drm_dp_aux_msg {
- unsigned int address;
- u8 request;
- u8 reply;
- void *buffer;
- size_t size;
+};
+/**
- struct drm_dp_aux - DisplayPort AUX channel
- @transfer: transfers a message representing a single AUX transaction
- */
+struct drm_dp_aux {
- ssize_t (*transfer)(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg);+};
+ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
void *buffer, size_t size);+ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
void *buffer, size_t size);+/**
- drm_dp_dpcd_readb() - read a single byte from the DPCD
- @aux: DisplayPort AUX channel
- @offset: address of the register to read
- @valuep: location where the value of the register will be stored
- Returns the number of bytes transferred (1) on success, or a negative
- error code on failure.
- */
+static inline ssize_t drm_dp_dpcd_readb(struct drm_dp_aux *aux,
unsigned int offset, u8 *valuep)+{
- return drm_dp_dpcd_read(aux, offset, valuep, 1);
+}
+/**
- drm_dp_dpcd_writeb() - write a single byte to the DPCD
- @aux: DisplayPort AUX channel
- @offset: address of the register to write
- @valuep: value to write to the register
- Returns the number of bytes transferred (1) on success, or a negative
- error code on failure.
- */
+static inline ssize_t drm_dp_dpcd_writeb(struct drm_dp_aux *aux, u8 value,
unsigned int offset)
I'd much prefer offset before value parameter. Why would you have these the other way around here than in all the other functions?
BR, Jani.
+{
- return drm_dp_dpcd_write(aux, offset, &value, 1);
+}
#endif /* _DRM_DP_HELPER_H_ */
1.8.4.2
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Dec 20, 2013 at 03:08:21PM +0200, Jani Nikula wrote:
On Tue, 17 Dec 2013, Thierry Reding thierry.reding@gmail.com wrote:
[...]
+/**
- drm_dp_dpcd_writeb() - write a single byte to the DPCD
- @aux: DisplayPort AUX channel
- @offset: address of the register to write
- @valuep: value to write to the register
- Returns the number of bytes transferred (1) on success, or a negative
- error code on failure.
- */
+static inline ssize_t drm_dp_dpcd_writeb(struct drm_dp_aux *aux, u8 value,
unsigned int offset)I'd much prefer offset before value parameter. Why would you have these the other way around here than in all the other functions?
I guess I thought this would mirror the convention seen with readl() and writel(), but I suppose since drm_dp_dpcd_readb() doesn't actually look anything like readl() there's no consistency here anyway. I'll change it around.
Thierry
The function reads the link status (6 bytes starting at offset 0x202) from the DPCD so that it can be conveniently passed to other DPCD helpers.
Signed-off-by: Thierry Reding treding@nvidia.com --- drivers/gpu/drm/drm_dp_helper.c | 16 ++++++++++++++++ include/drm/drm_dp_helper.h | 3 +++ 2 files changed, 19 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index bd622d462ca7..8111b8e5a8bc 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -462,3 +462,19 @@ ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset, return -EIO; } EXPORT_SYMBOL(drm_dp_dpcd_write); + +/** + * drm_dp_dpcd_read_link_status() - read DPCD link status (bytes 0x202-0x207) + * @aux: DisplayPort AUX channel + * @status: buffer to store the link status in (must be at least 6 bytes) + * + * Returns the number of bytes transferred on success or a negative error + * code on failure. + */ +int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux, + u8 status[DP_LINK_STATUS_SIZE]) +{ + return drm_dp_dpcd_read(aux, DP_LANE0_1_STATUS, status, + DP_LINK_STATUS_SIZE); +} +EXPORT_SYMBOL(drm_dp_dpcd_read_link_status); diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index 0b14e799cd98..38cbaef05005 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -461,4 +461,7 @@ static inline ssize_t drm_dp_dpcd_writeb(struct drm_dp_aux *aux, u8 value, return drm_dp_dpcd_write(aux, offset, &value, 1); }
+int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux, + u8 status[DP_LINK_STATUS_SIZE]); + #endif /* _DRM_DP_HELPER_H_ */
Add a helper to probe a DP link (reading out the maximum rate, link count and capabilities) as well as configuring it accordingly.
Signed-off-by: Thierry Reding treding@nvidia.com --- drivers/gpu/drm/drm_dp_helper.c | 77 +++++++++++++++++++++++++++++++++++++++++ include/drm/drm_dp_helper.h | 30 ++++++++++++++++ 2 files changed, 107 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 8111b8e5a8bc..01a8173c6e83 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -478,3 +478,80 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux, DP_LINK_STATUS_SIZE); } EXPORT_SYMBOL(drm_dp_dpcd_read_link_status); + +/** + * drm_dp_link_probe() - probe a DisplayPort link for capabilities + * @aux: DisplayPort AUX channel + * @link: pointer to structure in which to return link capabilities + * + * The structure filled in by this function can usually be passed directly + * into drm_dp_link_power_up() configure the link based on the link's + * capabilities. + * + * Returns 0 on success or a negative error code on failure. + */ +int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link) +{ + u8 value; + int err; + + memset(link, 0, sizeof(*link)); + + err = drm_dp_dpcd_readb(aux, DP_MAX_LINK_RATE, &value); + if (err < 0) + return err; + + link->rate = drm_dp_bw_code_to_link_rate(value); + + err = drm_dp_dpcd_readb(aux, DP_MAX_LANE_COUNT, &value); + if (err < 0) + return err; + + link->num_lanes = value & DP_MAX_LANE_COUNT_MASK; + + if (value & DP_ENHANCED_FRAME_CAP) + link->capabilities |= DP_LINK_CAP_ENHANCED_FRAMING; + + return 0; +} + +/** + * drm_dp_link_power_up() - power up and configure a DisplayPort link + * @aux: DisplayPort AUX channel + * @link: pointer to a structure containing the link configuration + * + * Returns 0 on success or a negative error code on failure. + */ +int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link) +{ + u8 value; + int err; + + err = drm_dp_dpcd_readb(aux, DP_SET_POWER, &value); + if (err < 0) + return err; + + value &= ~DP_SET_POWER_MASK; + value |= DP_SET_POWER_D0; + + err = drm_dp_dpcd_writeb(aux, value, DP_SET_POWER); + if (err < 0) + return err; + + value = drm_dp_link_rate_to_bw_code(link->rate); + + err = drm_dp_dpcd_writeb(aux, value, DP_LINK_BW_SET); + if (err < 0) + return err; + + value = link->num_lanes; + + if (link->capabilities & DP_LINK_CAP_ENHANCED_FRAMING) + value |= DP_LANE_COUNT_ENHANCED_FRAME_EN; + + err = drm_dp_dpcd_writeb(aux, value, DP_LANE_COUNT_SET); + if (err < 0) + return err; + + return 0; +} diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index 38cbaef05005..bad9ee11a2e2 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -290,6 +290,7 @@ #define DP_SET_POWER 0x600 # define DP_SET_POWER_D0 0x1 # define DP_SET_POWER_D3 0x2 +# define DP_SET_POWER_MASK 0x3
#define DP_PSR_ERROR_STATUS 0x2006 /* XXX 1.2? */ # define DP_PSR_LINK_CRC_ERROR (1 << 0) @@ -464,4 +465,33 @@ static inline ssize_t drm_dp_dpcd_writeb(struct drm_dp_aux *aux, u8 value, int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux, u8 status[DP_LINK_STATUS_SIZE]);
+/* + * DisplayPort link + */ +#define DP_LINK_CAP_ENHANCED_FRAMING (1 << 0) + +struct drm_dp_link { + unsigned int rate; + unsigned int num_lanes; + unsigned long capabilities; +}; + +/** + * drm_dp_link_probe() - probe link properties and capabilities + * @aux: DisplayPort AUX channel + * @link: DisplayPort link + * + * Returns 0 on success or a negative error code on failure. + */ +int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link); + +/** + * drm_dp_link_power_up() - power up a link + * @aux: DisplayPort AUX channel + * @link: DisplayPort link + * + * Returns 0 on success or a negative error code on failure. + */ +int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link); + #endif /* _DRM_DP_HELPER_H_ */
On Tue, 17 Dec 2013, Thierry Reding thierry.reding@gmail.com wrote:
Add a helper to probe a DP link (reading out the maximum rate, link count and capabilities) as well as configuring it accordingly.
Signed-off-by: Thierry Reding treding@nvidia.com
drivers/gpu/drm/drm_dp_helper.c | 77 +++++++++++++++++++++++++++++++++++++++++ include/drm/drm_dp_helper.h | 30 ++++++++++++++++ 2 files changed, 107 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 8111b8e5a8bc..01a8173c6e83 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -478,3 +478,80 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux, DP_LINK_STATUS_SIZE); } EXPORT_SYMBOL(drm_dp_dpcd_read_link_status);
+/**
- drm_dp_link_probe() - probe a DisplayPort link for capabilities
- @aux: DisplayPort AUX channel
- @link: pointer to structure in which to return link capabilities
- The structure filled in by this function can usually be passed directly
- into drm_dp_link_power_up() configure the link based on the link's
- capabilities.
- Returns 0 on success or a negative error code on failure.
- */
+int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link) +{
- u8 value;
- int err;
- memset(link, 0, sizeof(*link));
- err = drm_dp_dpcd_readb(aux, DP_MAX_LINK_RATE, &value);
- if (err < 0)
return err;- link->rate = drm_dp_bw_code_to_link_rate(value);
- err = drm_dp_dpcd_readb(aux, DP_MAX_LANE_COUNT, &value);
- if (err < 0)
return err;- link->num_lanes = value & DP_MAX_LANE_COUNT_MASK;
- if (value & DP_ENHANCED_FRAME_CAP)
link->capabilities |= DP_LINK_CAP_ENHANCED_FRAMING;- return 0;
+}
Okay, I'm biased due to the i915 implementation, but I think it's better to read the first 16 bytes of DPCD in one block, cache that, and provide accessors to the information. Keep the DPCD reads and writes to a minimum.
+/**
- drm_dp_link_power_up() - power up and configure a DisplayPort link
- @aux: DisplayPort AUX channel
- @link: pointer to a structure containing the link configuration
- Returns 0 on success or a negative error code on failure.
- */
+int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link) +{
- u8 value;
- int err;
- err = drm_dp_dpcd_readb(aux, DP_SET_POWER, &value);
If this is turning on the sink, we'll need to give the sink some time to wake up, and retry, per the DP spec.
Maybe I'd leave setting DP_SET_POWER up to the drivers altogether, at least for now. (Again, I'm biased, and this would make it easier for i915 to adopt this stuff. ;)
BR, Jani.
- if (err < 0)
return err;- value &= ~DP_SET_POWER_MASK;
- value |= DP_SET_POWER_D0;
- err = drm_dp_dpcd_writeb(aux, value, DP_SET_POWER);
- if (err < 0)
return err;- value = drm_dp_link_rate_to_bw_code(link->rate);
- err = drm_dp_dpcd_writeb(aux, value, DP_LINK_BW_SET);
- if (err < 0)
return err;- value = link->num_lanes;
- if (link->capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
value |= DP_LANE_COUNT_ENHANCED_FRAME_EN;- err = drm_dp_dpcd_writeb(aux, value, DP_LANE_COUNT_SET);
- if (err < 0)
return err;
IMO you should always combine DPCD reads and writes as much as possible. DP_LINK_BW_SET and DP_LANE_COUNT_SET are back to back.
- return 0;
+} diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index 38cbaef05005..bad9ee11a2e2 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -290,6 +290,7 @@ #define DP_SET_POWER 0x600 # define DP_SET_POWER_D0 0x1 # define DP_SET_POWER_D3 0x2 +# define DP_SET_POWER_MASK 0x3
#define DP_PSR_ERROR_STATUS 0x2006 /* XXX 1.2? */ # define DP_PSR_LINK_CRC_ERROR (1 << 0) @@ -464,4 +465,33 @@ static inline ssize_t drm_dp_dpcd_writeb(struct drm_dp_aux *aux, u8 value, int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux, u8 status[DP_LINK_STATUS_SIZE]);
+/*
- DisplayPort link
- */
+#define DP_LINK_CAP_ENHANCED_FRAMING (1 << 0)
+struct drm_dp_link {
- unsigned int rate;
- unsigned int num_lanes;
- unsigned long capabilities;
+};
+/**
- drm_dp_link_probe() - probe link properties and capabilities
- @aux: DisplayPort AUX channel
- @link: DisplayPort link
- Returns 0 on success or a negative error code on failure.
- */
+int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link);
+/**
- drm_dp_link_power_up() - power up a link
- @aux: DisplayPort AUX channel
- @link: DisplayPort link
- Returns 0 on success or a negative error code on failure.
- */
+int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link);
#endif /* _DRM_DP_HELPER_H_ */
1.8.4.2
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Implements an I2C-over-AUX I2C adapter on top of the generic drm_dp_aux infrastructure. It extracts the retry logic from existing drivers, which should help in porting those drivers to this new helper.
Signed-off-by: Thierry Reding treding@nvidia.com --- drivers/gpu/drm/drm_dp_helper.c | 178 ++++++++++++++++++++++++++++++++++++++++ include/drm/drm_dp_helper.h | 4 + 2 files changed, 182 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 01a8173c6e83..8a64cf8ac8cc 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -555,3 +555,181 @@ int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link)
return 0; } + +/* + * I2C-over-AUX implementation + */ + +struct drm_dp_i2c_adapter { + struct i2c_adapter adapter; + struct drm_dp_aux *aux; +}; + +static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter) +{ + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | + I2C_FUNC_SMBUS_READ_BLOCK_DATA | + I2C_FUNC_SMBUS_BLOCK_PROC_CALL | + I2C_FUNC_10BIT_ADDR; +} + +/* + * Transfer a single I2C-over-AUX message and handle various error conditions, + * retrying the transaction as appropriate. + */ +static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) +{ + unsigned int retry; + int err; + + /* + * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device + * is required to retry at least seven times upon receiving AUX_DEFER + * before giving up the AUX transaction. + */ + for (retry = 0; retry < 7; retry++) { + err = aux->transfer(aux, msg); + if (err < 0) + return err; + + switch (msg->reply & DP_AUX_NATIVE_REPLY_MASK) { + case DP_AUX_NATIVE_REPLY_ACK: + /* + * For I2C-over-AUX transactions this isn't enough, we + * need to check for the I2C ACK reply. + */ + break; + + case DP_AUX_NATIVE_REPLY_NACK: + return -EREMOTEIO; + + case DP_AUX_NATIVE_REPLY_DEFER: + /* + * We could check for I2C bitrate capabilities and if + * available adjust this interval. We could also be + * more careful with DP-to-legacy adapters where a + * long legacy cable may forc very low I2C bit rates. + * + * For now just defer for long enough to hopefully be + * safe for all use-cases. + */ + usleep_range(500, 600); + continue; + + default: + DRM_ERROR("invalid native reply %#04x\n", msg->reply); + return -EREMOTEIO; + } + + switch (msg->reply & DP_AUX_I2C_REPLY_MASK) { + case DP_AUX_I2C_REPLY_ACK: + /* + * Both native ACK and I2C ACK replies received. We + * can assume the transfer was successful. + */ + return 0; + + case DP_AUX_I2C_REPLY_NACK: + return -EREMOTEIO; + + case DP_AUX_I2C_REPLY_DEFER: + usleep_range(400, 500); + continue; + + default: + DRM_ERROR("invalid I2C reply %#04x\n", msg->reply); + return -EREMOTEIO; + } + } + + DRM_ERROR("too many retries, giving up\n"); + return -EREMOTEIO; +} + +static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, + int num) +{ + struct drm_dp_i2c_adapter *dp = adapter->algo_data; + unsigned int i, j; + + for (i = 0; i < num; i++) { + struct drm_dp_aux_msg msg; + int err; + + /* + * Many hardware implementations support FIFOs larger than a + * single byte, but it has been empirically determined that + * transferring data in larger chunks can actually lead to + * 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 (j < msgs[i].len - 1) + msg.request |= DP_AUX_I2C_MOT; + + msg.buffer = msgs[i].buf + j; + msg.size = 1; + + err = drm_dp_i2c_do_msg(dp->aux, &msg); + if (err < 0) + return err; + } + } + + return num; +} + +static const struct i2c_algorithm drm_dp_i2c_algo = { + .functionality = drm_dp_i2c_functionality, + .master_xfer = drm_dp_i2c_xfer, +}; + +/** + * drm_dp_add_i2c_bus() - register an I2C adapter for I2C-over-AUX access + * @aux: DisplayPort AUX channel + * + * Returns a pointer to an I2C adapter on success or an ERR_PTR()-encoded + * error code on failure. + */ +struct i2c_adapter *drm_dp_add_i2c_bus(struct drm_dp_aux *aux) +{ + struct drm_dp_i2c_adapter *adapter; + int err; + + adapter = kzalloc(sizeof(*adapter), GFP_KERNEL); + if (!adapter) + return ERR_PTR(-ENOMEM); + + adapter->adapter.algo = &drm_dp_i2c_algo; + adapter->adapter.algo_data = adapter; + adapter->adapter.retries = 3; + adapter->aux = aux; + + adapter->adapter.class = I2C_CLASS_DDC; + adapter->adapter.owner = THIS_MODULE; + adapter->adapter.dev.parent = aux->dev; + adapter->adapter.dev.of_node = aux->dev->of_node; + + strncpy(adapter->adapter.name, dev_name(aux->dev), + sizeof(adapter->adapter.name)); + + err = i2c_add_adapter(&adapter->adapter); + if (err < 0) { + kfree(adapter); + return ERR_PTR(err); + } + + return &adapter->adapter; +} +EXPORT_SYMBOL(drm_dp_add_i2c_bus); diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index bad9ee11a2e2..c4acdbc2a172 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -423,6 +423,8 @@ struct drm_dp_aux_msg { * @transfer: transfers a message representing a single AUX transaction */ struct drm_dp_aux { + struct device *dev; + ssize_t (*transfer)(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg); }; @@ -494,4 +496,6 @@ int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link); */ int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link);
+struct i2c_adapter *drm_dp_add_i2c_bus(struct drm_dp_aux *aux); + #endif /* _DRM_DP_HELPER_H_ */
On Tue, 17 Dec 2013, Thierry Reding thierry.reding@gmail.com wrote:
Implements an I2C-over-AUX I2C adapter on top of the generic drm_dp_aux infrastructure. It extracts the retry logic from existing drivers, which should help in porting those drivers to this new helper.
Signed-off-by: Thierry Reding treding@nvidia.com
drivers/gpu/drm/drm_dp_helper.c | 178 ++++++++++++++++++++++++++++++++++++++++ include/drm/drm_dp_helper.h | 4 + 2 files changed, 182 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 01a8173c6e83..8a64cf8ac8cc 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -555,3 +555,181 @@ int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link)
return 0; }
+/*
- I2C-over-AUX implementation
- */
+struct drm_dp_i2c_adapter {
- struct i2c_adapter adapter;
- struct drm_dp_aux *aux;
+};
+static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter) +{
- return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
I2C_FUNC_SMBUS_READ_BLOCK_DATA |I2C_FUNC_SMBUS_BLOCK_PROC_CALL |I2C_FUNC_10BIT_ADDR;+}
+/*
- Transfer a single I2C-over-AUX message and handle various error conditions,
- retrying the transaction as appropriate.
- */
+static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) +{
- unsigned int retry;
- int err;
- /*
* DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device* is required to retry at least seven times upon receiving AUX_DEFER* before giving up the AUX transaction.*/- for (retry = 0; retry < 7; retry++) {
err = aux->transfer(aux, msg);if (err < 0)return err;switch (msg->reply & DP_AUX_NATIVE_REPLY_MASK) {case DP_AUX_NATIVE_REPLY_ACK:/** For I2C-over-AUX transactions this isn't enough, we* need to check for the I2C ACK reply.*/break;case DP_AUX_NATIVE_REPLY_NACK:return -EREMOTEIO;case DP_AUX_NATIVE_REPLY_DEFER:/** We could check for I2C bitrate capabilities and if* available adjust this interval. We could also be* more careful with DP-to-legacy adapters where a* long legacy cable may forc very low I2C bit rates.
^^^^ force
* For now just defer for long enough to hopefully be* safe for all use-cases.*/usleep_range(500, 600);
I've banged my head against the wall a bit with this (the original i915 comment similar to the above was mine) and I'd be hesitant to switch to this as-is in i915.
First, I'd like to retain the DRM_DEBUG_KMS messages all around, as otherwise issues here will be hard to debug. It's a pain to have to ask to patch the kernel just get the debug info. At least we've had a fair share of DP aux issues in our driver.
Second, I fear one size does not fit all wrt the delays. What would you say about making all the DP_AUX_NATIVE_REPLY_DEFER and DP_AUX_I2C_REPLY_DEFER checks do callbacks if defined, and fallback to the default delays otherwise? I think there'll be some more churn in the error handling, particularly for the more exotic sinks, and IMO having them per driver would make development and bug fixing faster. I'm not saying you need to do this now, it can be a follow-up; just asking what you'd think of it.
I hope to be able to do more review of the series later this week.
BR, Jani.
continue;default:DRM_ERROR("invalid native reply %#04x\n", msg->reply);return -EREMOTEIO;}switch (msg->reply & DP_AUX_I2C_REPLY_MASK) {case DP_AUX_I2C_REPLY_ACK:/** Both native ACK and I2C ACK replies received. We* can assume the transfer was successful.*/return 0;case DP_AUX_I2C_REPLY_NACK:return -EREMOTEIO;case DP_AUX_I2C_REPLY_DEFER:usleep_range(400, 500);continue;default:DRM_ERROR("invalid I2C reply %#04x\n", msg->reply);return -EREMOTEIO;}- }
- DRM_ERROR("too many retries, giving up\n");
- return -EREMOTEIO;
+}
+static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
int num)+{
- struct drm_dp_i2c_adapter *dp = adapter->algo_data;
- unsigned int i, j;
- for (i = 0; i < num; i++) {
struct drm_dp_aux_msg msg;int err;/** Many hardware implementations support FIFOs larger than a* single byte, but it has been empirically determined that* transferring data in larger chunks can actually lead to* 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 (j < msgs[i].len - 1)msg.request |= DP_AUX_I2C_MOT;msg.buffer = msgs[i].buf + j;msg.size = 1;err = drm_dp_i2c_do_msg(dp->aux, &msg);if (err < 0)return err;}- }
- return num;
+}
+static const struct i2c_algorithm drm_dp_i2c_algo = {
- .functionality = drm_dp_i2c_functionality,
- .master_xfer = drm_dp_i2c_xfer,
+};
+/**
- drm_dp_add_i2c_bus() - register an I2C adapter for I2C-over-AUX access
- @aux: DisplayPort AUX channel
- Returns a pointer to an I2C adapter on success or an ERR_PTR()-encoded
- error code on failure.
- */
+struct i2c_adapter *drm_dp_add_i2c_bus(struct drm_dp_aux *aux) +{
- struct drm_dp_i2c_adapter *adapter;
- int err;
- adapter = kzalloc(sizeof(*adapter), GFP_KERNEL);
- if (!adapter)
return ERR_PTR(-ENOMEM);- adapter->adapter.algo = &drm_dp_i2c_algo;
- adapter->adapter.algo_data = adapter;
- adapter->adapter.retries = 3;
- adapter->aux = aux;
- adapter->adapter.class = I2C_CLASS_DDC;
- adapter->adapter.owner = THIS_MODULE;
- adapter->adapter.dev.parent = aux->dev;
- adapter->adapter.dev.of_node = aux->dev->of_node;
- strncpy(adapter->adapter.name, dev_name(aux->dev),
sizeof(adapter->adapter.name));- err = i2c_add_adapter(&adapter->adapter);
- if (err < 0) {
kfree(adapter);return ERR_PTR(err);- }
- return &adapter->adapter;
+} +EXPORT_SYMBOL(drm_dp_add_i2c_bus); diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index bad9ee11a2e2..c4acdbc2a172 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -423,6 +423,8 @@ struct drm_dp_aux_msg {
- @transfer: transfers a message representing a single AUX transaction
*/ struct drm_dp_aux {
- struct device *dev;
- ssize_t (*transfer)(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg);
}; @@ -494,4 +496,6 @@ int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link); */ int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link);
+struct i2c_adapter *drm_dp_add_i2c_bus(struct drm_dp_aux *aux);
#endif /* _DRM_DP_HELPER_H_ */
1.8.4.2
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Dec 17, 2013 at 07:11:21PM +0200, Jani Nikula wrote:
On Tue, 17 Dec 2013, Thierry Reding thierry.reding@gmail.com wrote:
[...]
case DP_AUX_NATIVE_REPLY_DEFER:/** We could check for I2C bitrate capabilities and if* available adjust this interval. We could also be* more careful with DP-to-legacy adapters where a* long legacy cable may forc very low I2C bit rates.^^^^ force
Fixed.
* For now just defer for long enough to hopefully be* safe for all use-cases.*/usleep_range(500, 600);I've banged my head against the wall a bit with this (the original i915 comment similar to the above was mine) and I'd be hesitant to switch to this as-is in i915.
First, I'd like to retain the DRM_DEBUG_KMS messages all around, as otherwise issues here will be hard to debug. It's a pain to have to ask to patch the kernel just get the debug info. At least we've had a fair share of DP aux issues in our driver.
Okay, I'll add those back in. I'll assume the same goes for DRM_ERROR messages.
Second, I fear one size does not fit all wrt the delays. What would you say about making all the DP_AUX_NATIVE_REPLY_DEFER and DP_AUX_I2C_REPLY_DEFER checks do callbacks if defined, and fallback to the default delays otherwise? I think there'll be some more churn in the error handling, particularly for the more exotic sinks, and IMO having them per driver would make development and bug fixing faster. I'm not saying you need to do this now, it can be a follow-up; just asking what you'd think of it.
Sure, we should be able to easily do that. One possible alternative to this would be to handle in in the drivers by handling DEFER explicitly and then returning -EBUSY. -EBUSY will cause native AUX transactions to be retried and I just noticed that radeon does the same for I2C-over-AUX transactions, so I'll add that special case there as well.
The benefit of doing that would be that the helper stays relatively simple. On the downside it may not be as explicit as a driver-specific callback.
Thanks, Thierry
On Tue, Dec 17, 2013 at 05:20:07PM +0100, Thierry Reding wrote:
Implements an I2C-over-AUX I2C adapter on top of the generic drm_dp_aux infrastructure. It extracts the retry logic from existing drivers, which should help in porting those drivers to this new helper.
Signed-off-by: Thierry Reding treding@nvidia.com
drivers/gpu/drm/drm_dp_helper.c | 178 ++++++++++++++++++++++++++++++++++++++++ include/drm/drm_dp_helper.h | 4 + 2 files changed, 182 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 01a8173c6e83..8a64cf8ac8cc 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -555,3 +555,181 @@ int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link)
return 0; }
+/*
- I2C-over-AUX implementation
- */
+struct drm_dp_i2c_adapter {
- struct i2c_adapter adapter;
- struct drm_dp_aux *aux;
+};
I'd just embedded an i2c adapter into the drm_dp_aux structure - I think drivers always want such a thing. Then maybe rename the setup function from add to register_i2c_bus or so. For smoother transition drivers can always store a pointer to this i2c_adapter somewhere more convenient for them. -Daniel
+static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter) +{
- return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
I2C_FUNC_SMBUS_READ_BLOCK_DATA |I2C_FUNC_SMBUS_BLOCK_PROC_CALL |I2C_FUNC_10BIT_ADDR;+}
+/*
- Transfer a single I2C-over-AUX message and handle various error conditions,
- retrying the transaction as appropriate.
- */
+static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) +{
- unsigned int retry;
- int err;
- /*
* DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device* is required to retry at least seven times upon receiving AUX_DEFER* before giving up the AUX transaction.*/- for (retry = 0; retry < 7; retry++) {
err = aux->transfer(aux, msg);if (err < 0)return err;switch (msg->reply & DP_AUX_NATIVE_REPLY_MASK) {case DP_AUX_NATIVE_REPLY_ACK:/** For I2C-over-AUX transactions this isn't enough, we* need to check for the I2C ACK reply.*/break;case DP_AUX_NATIVE_REPLY_NACK:return -EREMOTEIO;case DP_AUX_NATIVE_REPLY_DEFER:/** We could check for I2C bitrate capabilities and if* available adjust this interval. We could also be* more careful with DP-to-legacy adapters where a* long legacy cable may forc very low I2C bit rates.** For now just defer for long enough to hopefully be* safe for all use-cases.*/usleep_range(500, 600);continue;default:DRM_ERROR("invalid native reply %#04x\n", msg->reply);return -EREMOTEIO;}switch (msg->reply & DP_AUX_I2C_REPLY_MASK) {case DP_AUX_I2C_REPLY_ACK:/** Both native ACK and I2C ACK replies received. We* can assume the transfer was successful.*/return 0;case DP_AUX_I2C_REPLY_NACK:return -EREMOTEIO;case DP_AUX_I2C_REPLY_DEFER:usleep_range(400, 500);continue;default:DRM_ERROR("invalid I2C reply %#04x\n", msg->reply);return -EREMOTEIO;}- }
- DRM_ERROR("too many retries, giving up\n");
- return -EREMOTEIO;
+}
+static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
int num)+{
- struct drm_dp_i2c_adapter *dp = adapter->algo_data;
- unsigned int i, j;
- for (i = 0; i < num; i++) {
struct drm_dp_aux_msg msg;int err;/** Many hardware implementations support FIFOs larger than a* single byte, but it has been empirically determined that* transferring data in larger chunks can actually lead to* 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 (j < msgs[i].len - 1)msg.request |= DP_AUX_I2C_MOT;msg.buffer = msgs[i].buf + j;msg.size = 1;err = drm_dp_i2c_do_msg(dp->aux, &msg);if (err < 0)return err;}- }
- return num;
+}
+static const struct i2c_algorithm drm_dp_i2c_algo = {
- .functionality = drm_dp_i2c_functionality,
- .master_xfer = drm_dp_i2c_xfer,
+};
+/**
- drm_dp_add_i2c_bus() - register an I2C adapter for I2C-over-AUX access
- @aux: DisplayPort AUX channel
- Returns a pointer to an I2C adapter on success or an ERR_PTR()-encoded
- error code on failure.
- */
+struct i2c_adapter *drm_dp_add_i2c_bus(struct drm_dp_aux *aux) +{
- struct drm_dp_i2c_adapter *adapter;
- int err;
- adapter = kzalloc(sizeof(*adapter), GFP_KERNEL);
- if (!adapter)
return ERR_PTR(-ENOMEM);- adapter->adapter.algo = &drm_dp_i2c_algo;
- adapter->adapter.algo_data = adapter;
- adapter->adapter.retries = 3;
- adapter->aux = aux;
- adapter->adapter.class = I2C_CLASS_DDC;
- adapter->adapter.owner = THIS_MODULE;
- adapter->adapter.dev.parent = aux->dev;
- adapter->adapter.dev.of_node = aux->dev->of_node;
- strncpy(adapter->adapter.name, dev_name(aux->dev),
sizeof(adapter->adapter.name));- err = i2c_add_adapter(&adapter->adapter);
- if (err < 0) {
kfree(adapter);return ERR_PTR(err);- }
- return &adapter->adapter;
+} +EXPORT_SYMBOL(drm_dp_add_i2c_bus); diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index bad9ee11a2e2..c4acdbc2a172 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -423,6 +423,8 @@ struct drm_dp_aux_msg {
- @transfer: transfers a message representing a single AUX transaction
*/ struct drm_dp_aux {
- struct device *dev;
- ssize_t (*transfer)(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg);
}; @@ -494,4 +496,6 @@ int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link); */ int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link);
+struct i2c_adapter *drm_dp_add_i2c_bus(struct drm_dp_aux *aux);
#endif /* _DRM_DP_HELPER_H_ */
1.8.4.2
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Dec 18, 2013 at 09:52:47AM +0100, Daniel Vetter wrote:
On Tue, Dec 17, 2013 at 05:20:07PM +0100, Thierry Reding wrote:
Implements an I2C-over-AUX I2C adapter on top of the generic drm_dp_aux infrastructure. It extracts the retry logic from existing drivers, which should help in porting those drivers to this new helper.
Signed-off-by: Thierry Reding treding@nvidia.com
drivers/gpu/drm/drm_dp_helper.c | 178 ++++++++++++++++++++++++++++++++++++++++ include/drm/drm_dp_helper.h | 4 + 2 files changed, 182 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 01a8173c6e83..8a64cf8ac8cc 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -555,3 +555,181 @@ int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link)
return 0; }
+/*
- I2C-over-AUX implementation
- */
+struct drm_dp_i2c_adapter {
- struct i2c_adapter adapter;
- struct drm_dp_aux *aux;
+};
I'd just embedded an i2c adapter into the drm_dp_aux structure - I think drivers always want such a thing. Then maybe rename the setup function from add to register_i2c_bus or so. For smoother transition drivers can always store a pointer to this i2c_adapter somewhere more convenient for them.
Okay, I've made that change. I was slightly worried that it might cause lifetime issues, but looking somewhat more closely perhaps that's not an issue since i2c_adapters seem to be carefully designed to easily allow being embedded.
Thierry
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Jani Nikula -
Thierry Reding