Here's a few patches that simplify the aux handling code and bubble up
timeouts and nacks to the upper DRM layers. The goal is to get DRM to
know that the other side isn't there or that there's been a timeout,
instead of saying that everything is fine and putting some error message
into the logs.
Cc: Dmitry Baryshkov <[email protected]>
Cc: Abhinav Kumar <[email protected]>
Cc: Kuogee Hsieh <[email protected]>
Cc: [email protected]
Cc: Sean Paul <[email protected]>
Stephen Boyd (3):
drm/msm/dp: Simplify aux irq handling code
drm/msm/dp: Shrink locking area of dp_aux_transfer()
drm/msm/dp: Handle aux timeouts, nacks, defers
drivers/gpu/drm/msm/dp/dp_aux.c | 181 ++++++++++++----------------
drivers/gpu/drm/msm/dp/dp_aux.h | 8 --
drivers/gpu/drm/msm/dp/dp_catalog.c | 2 +-
drivers/gpu/drm/msm/dp/dp_catalog.h | 2 +-
4 files changed, 80 insertions(+), 113 deletions(-)
base-commit: 51595e3b4943b0079638b2657f603cf5c8ea3a66
--
https://chromeos.dev
Let's look at the irq status bits after a transfer and see if we got a
nack or a defer or a timeout, instead of telling drm layers that
everything was fine, while still printing an error message. I wasn't
sure about NACK+DEFER so I lumped all those various errors along with a
nack so that the drm core can figure out that things are just not going
well. The important thing is that we're now returning -ETIMEDOUT when
the message times out and nacks for bad addresses.
Cc: Dmitry Baryshkov <[email protected]>
Cc: Abhinav Kumar <[email protected]>
Cc: Kuogee Hsieh <[email protected]>
Cc: [email protected]
Cc: Sean Paul <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/msm/dp/dp_aux.c | 140 ++++++++++++++------------------
drivers/gpu/drm/msm/dp/dp_aux.h | 8 --
2 files changed, 61 insertions(+), 87 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index b49810396513..4a3293b590b0 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -9,7 +9,15 @@
#include "dp_reg.h"
#include "dp_aux.h"
-#define DP_AUX_ENUM_STR(x) #x
+enum msm_dp_aux_err {
+ DP_AUX_ERR_NONE,
+ DP_AUX_ERR_ADDR,
+ DP_AUX_ERR_TOUT,
+ DP_AUX_ERR_NACK,
+ DP_AUX_ERR_DEFER,
+ DP_AUX_ERR_NACK_DEFER,
+ DP_AUX_ERR_PHY,
+};
struct dp_aux_private {
struct device *dev;
@@ -18,7 +26,7 @@ struct dp_aux_private {
struct mutex mutex;
struct completion comp;
- u32 aux_error_num;
+ enum msm_dp_aux_err aux_error_num;
u32 retry_cnt;
bool cmd_busy;
bool native;
@@ -33,62 +41,45 @@ struct dp_aux_private {
#define MAX_AUX_RETRIES 5
-static const char *dp_aux_get_error(u32 aux_error)
-{
- switch (aux_error) {
- case DP_AUX_ERR_NONE:
- return DP_AUX_ENUM_STR(DP_AUX_ERR_NONE);
- case DP_AUX_ERR_ADDR:
- return DP_AUX_ENUM_STR(DP_AUX_ERR_ADDR);
- case DP_AUX_ERR_TOUT:
- return DP_AUX_ENUM_STR(DP_AUX_ERR_TOUT);
- case DP_AUX_ERR_NACK:
- return DP_AUX_ENUM_STR(DP_AUX_ERR_NACK);
- case DP_AUX_ERR_DEFER:
- return DP_AUX_ENUM_STR(DP_AUX_ERR_DEFER);
- case DP_AUX_ERR_NACK_DEFER:
- return DP_AUX_ENUM_STR(DP_AUX_ERR_NACK_DEFER);
- default:
- return "unknown";
- }
-}
-
-static u32 dp_aux_write(struct dp_aux_private *aux,
+static ssize_t dp_aux_write(struct dp_aux_private *aux,
struct drm_dp_aux_msg *msg)
{
- u32 data[4], reg, len;
+ u8 data[4];
+ u32 reg;
+ ssize_t len;
u8 *msgdata = msg->buffer;
int const AUX_CMD_FIFO_LEN = 128;
int i = 0;
if (aux->read)
- len = 4;
+ len = 0;
else
- len = msg->size + 4;
+ len = msg->size;
/*
* cmd fifo only has depth of 144 bytes
* limit buf length to 128 bytes here
*/
- if (len > AUX_CMD_FIFO_LEN) {
+ if (len > AUX_CMD_FIFO_LEN - 4) {
DRM_ERROR("buf size greater than allowed size of 128 bytes\n");
- return 0;
+ return -EINVAL;
}
/* Pack cmd and write to HW */
- data[0] = (msg->address >> 16) & 0xf; /* addr[19:16] */
+ data[0] = (msg->address >> 16) & 0xf; /* addr[19:16] */
if (aux->read)
- data[0] |= BIT(4); /* R/W */
+ data[0] |= BIT(4); /* R/W */
- data[1] = (msg->address >> 8) & 0xff; /* addr[15:8] */
- data[2] = msg->address & 0xff; /* addr[7:0] */
- data[3] = (msg->size - 1) & 0xff; /* len[7:0] */
+ data[1] = msg->address >> 8; /* addr[15:8] */
+ data[2] = msg->address; /* addr[7:0] */
+ data[3] = msg->size - 1; /* len[7:0] */
- for (i = 0; i < len; i++) {
+ for (i = 0; i < len + 4; i++) {
reg = (i < 4) ? data[i] : msgdata[i - 4];
+ reg <<= DP_AUX_DATA_OFFSET;
+ reg &= DP_AUX_DATA_MASK;
+ reg |= DP_AUX_DATA_WRITE;
/* index = 0, write */
- reg = (((reg) << DP_AUX_DATA_OFFSET)
- & DP_AUX_DATA_MASK) | DP_AUX_DATA_WRITE;
if (i == 0)
reg |= DP_AUX_DATA_INDEX_WRITE;
aux->catalog->aux_data = reg;
@@ -116,39 +107,27 @@ static u32 dp_aux_write(struct dp_aux_private *aux,
return len;
}
-static int dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
+static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
struct drm_dp_aux_msg *msg)
{
- u32 ret, len, timeout;
- int aux_timeout_ms = HZ/4;
+ ssize_t ret;
+ unsigned long time_left;
reinit_completion(&aux->comp);
- len = dp_aux_write(aux, msg);
- if (len == 0) {
- DRM_ERROR("DP AUX write failed\n");
- return -EINVAL;
- }
+ ret = dp_aux_write(aux, msg);
+ if (ret < 0)
+ return ret;
- timeout = wait_for_completion_timeout(&aux->comp, aux_timeout_ms);
- if (!timeout) {
- DRM_ERROR("aux %s timeout\n", (aux->read ? "read" : "write"));
+ time_left = wait_for_completion_timeout(&aux->comp,
+ msecs_to_jiffies(250));
+ if (!time_left)
return -ETIMEDOUT;
- }
-
- if (aux->aux_error_num == DP_AUX_ERR_NONE) {
- ret = len;
- } else {
- DRM_ERROR_RATELIMITED("aux err: %s\n",
- dp_aux_get_error(aux->aux_error_num));
-
- ret = -EINVAL;
- }
return ret;
}
-static void dp_aux_cmd_fifo_rx(struct dp_aux_private *aux,
+static ssize_t dp_aux_cmd_fifo_rx(struct dp_aux_private *aux,
struct drm_dp_aux_msg *msg)
{
u32 data;
@@ -175,9 +154,10 @@ static void dp_aux_cmd_fifo_rx(struct dp_aux_private *aux,
actual_i = (data >> DP_AUX_DATA_INDEX_OFFSET) & 0xFF;
if (i != actual_i)
- DRM_ERROR("Index mismatch: expected %d, found %d\n",
- i, actual_i);
+ break;
}
+
+ return i;
}
static void dp_aux_native_handler(struct dp_aux_private *aux, u32 isr)
@@ -367,36 +347,38 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
}
ret = dp_aux_cmd_fifo_tx(aux, msg);
-
if (ret < 0) {
if (aux->native) {
aux->retry_cnt++;
if (!(aux->retry_cnt % MAX_AUX_RETRIES))
dp_catalog_aux_update_cfg(aux->catalog);
}
- usleep_range(400, 500); /* at least 400us to next try */
- goto unlock_exit;
- }
-
- if (aux->aux_error_num == DP_AUX_ERR_NONE) {
- if (aux->read)
- dp_aux_cmd_fifo_rx(aux, msg);
-
- msg->reply = aux->native ?
- DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
} else {
- /* Reply defer to retry */
- msg->reply = aux->native ?
- DP_AUX_NATIVE_REPLY_DEFER : DP_AUX_I2C_REPLY_DEFER;
+ aux->retry_cnt = 0;
+ switch (aux->aux_error_num) {
+ case DP_AUX_ERR_NONE:
+ if (aux->read)
+ ret = dp_aux_cmd_fifo_rx(aux, msg);
+ msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
+ break;
+ case DP_AUX_ERR_DEFER:
+ msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_DEFER : DP_AUX_I2C_REPLY_DEFER;
+ break;
+ case DP_AUX_ERR_PHY:
+ case DP_AUX_ERR_ADDR:
+ case DP_AUX_ERR_NACK:
+ case DP_AUX_ERR_NACK_DEFER:
+ msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_NACK : DP_AUX_I2C_REPLY_NACK;
+ break;
+ case DP_AUX_ERR_TOUT:
+ ret = -ETIMEDOUT;
+ break;
+ }
}
- /* Return requested size for success or retry */
- ret = msg->size;
- aux->retry_cnt = 0;
-
-unlock_exit:
aux->cmd_busy = false;
mutex_unlock(&aux->mutex);
+
return ret;
}
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h b/drivers/gpu/drm/msm/dp/dp_aux.h
index f8b8ba919465..0728cc09c9ec 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.h
+++ b/drivers/gpu/drm/msm/dp/dp_aux.h
@@ -9,14 +9,6 @@
#include "dp_catalog.h"
#include <drm/drm_dp_helper.h>
-#define DP_AUX_ERR_NONE 0
-#define DP_AUX_ERR_ADDR -1
-#define DP_AUX_ERR_TOUT -2
-#define DP_AUX_ERR_NACK -3
-#define DP_AUX_ERR_DEFER -4
-#define DP_AUX_ERR_NACK_DEFER -5
-#define DP_AUX_ERR_PHY -6
-
int dp_aux_register(struct drm_dp_aux *dp_aux);
void dp_aux_unregister(struct drm_dp_aux *dp_aux);
void dp_aux_isr(struct drm_dp_aux *dp_aux);
--
https://chromeos.dev
We don't need to hold the lock to inspect the message we're going to
transfer, and we don't need to clear the busy flag either. Take the lock
later and bail out earlier if conditions aren't met.
Cc: Dmitry Baryshkov <[email protected]>
Cc: Abhinav Kumar <[email protected]>
Cc: Kuogee Hsieh <[email protected]>
Cc: [email protected]
Cc: Sean Paul <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/msm/dp/dp_aux.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 91188466cece..b49810396513 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -329,30 +329,29 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
ssize_t ret;
int const aux_cmd_native_max = 16;
int const aux_cmd_i2c_max = 128;
- struct dp_aux_private *aux = container_of(dp_aux,
- struct dp_aux_private, dp_aux);
+ struct dp_aux_private *aux;
- mutex_lock(&aux->mutex);
+ aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
aux->native = msg->request & (DP_AUX_NATIVE_WRITE & DP_AUX_NATIVE_READ);
/* Ignore address only message */
- if ((msg->size == 0) || (msg->buffer == NULL)) {
+ if (msg->size == 0 || !msg->buffer) {
msg->reply = aux->native ?
DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
- ret = msg->size;
- goto unlock_exit;
+ return msg->size;
}
/* msg sanity check */
- if ((aux->native && (msg->size > aux_cmd_native_max)) ||
- (msg->size > aux_cmd_i2c_max)) {
+ if ((aux->native && msg->size > aux_cmd_native_max) ||
+ msg->size > aux_cmd_i2c_max) {
DRM_ERROR("%s: invalid msg: size(%zu), request(%x)\n",
__func__, msg->size, msg->request);
- ret = -EINVAL;
- goto unlock_exit;
+ return -EINVAL;
}
+ mutex_lock(&aux->mutex);
+
dp_aux_update_offset_and_segment(aux, msg);
dp_aux_transfer_helper(aux, msg, true);
--
https://chromeos.dev
Quoting Stephen Boyd (2021-05-07 14:25:02)
> Here's a few patches that simplify the aux handling code and bubble up
> timeouts and nacks to the upper DRM layers. The goal is to get DRM to
> know that the other side isn't there or that there's been a timeout,
> instead of saying that everything is fine and putting some error message
> into the logs.
>
> Cc: Dmitry Baryshkov <[email protected]>
> Cc: Abhinav Kumar <[email protected]>
> Cc: Kuogee Hsieh <[email protected]>
> Cc: [email protected]
> Cc: Sean Paul <[email protected]>
>
Kuogee, have you had a change to review this series?
> Stephen Boyd (3):
> drm/msm/dp: Simplify aux irq handling code
> drm/msm/dp: Shrink locking area of dp_aux_transfer()
> drm/msm/dp: Handle aux timeouts, nacks, defers
>
> drivers/gpu/drm/msm/dp/dp_aux.c | 181 ++++++++++++----------------
> drivers/gpu/drm/msm/dp/dp_aux.h | 8 --
> drivers/gpu/drm/msm/dp/dp_catalog.c | 2 +-
> drivers/gpu/drm/msm/dp/dp_catalog.h | 2 +-
> 4 files changed, 80 insertions(+), 113 deletions(-)
>
>
> base-commit: 51595e3b4943b0079638b2657f603cf5c8ea3a66
> --
> https://chromeos.dev
>
On 2021-05-21 14:57, Stephen Boyd wrote:
> Quoting Stephen Boyd (2021-05-07 14:25:02)
>> Here's a few patches that simplify the aux handling code and bubble up
>> timeouts and nacks to the upper DRM layers. The goal is to get DRM to
>> know that the other side isn't there or that there's been a timeout,
>> instead of saying that everything is fine and putting some error
>> message
>> into the logs.
>>
>> Cc: Dmitry Baryshkov <[email protected]>
>> Cc: Abhinav Kumar <[email protected]>
>> Cc: Kuogee Hsieh <[email protected]>
>> Cc: [email protected]
>> Cc: Sean Paul <[email protected]>
>>
>
> Kuogee, have you had a change to review this series?
>
Sorry missed this one.
Will review it now.
>> Stephen Boyd (3):
>> drm/msm/dp: Simplify aux irq handling code
>> drm/msm/dp: Shrink locking area of dp_aux_transfer()
>> drm/msm/dp: Handle aux timeouts, nacks, defers
>>
>> drivers/gpu/drm/msm/dp/dp_aux.c | 181
>> ++++++++++++----------------
>> drivers/gpu/drm/msm/dp/dp_aux.h | 8 --
>> drivers/gpu/drm/msm/dp/dp_catalog.c | 2 +-
>> drivers/gpu/drm/msm/dp/dp_catalog.h | 2 +-
>> 4 files changed, 80 insertions(+), 113 deletions(-)
>>
>>
>> base-commit: 51595e3b4943b0079638b2657f603cf5c8ea3a66
>> --
>> https://chromeos.dev
>>
On 2021-05-07 14:25, Stephen Boyd wrote:
> We don't need to hold the lock to inspect the message we're going to
> transfer, and we don't need to clear the busy flag either. Take the
> lock
> later and bail out earlier if conditions aren't met.
>
> Cc: Dmitry Baryshkov <[email protected]>
> Cc: Abhinav Kumar <[email protected]>
> Cc: Kuogee Hsieh <[email protected]>
> Cc: [email protected]
> Cc: Sean Paul <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
Reviewed-by: Kuogee Hsieh <[email protected]>
> ---
> drivers/gpu/drm/msm/dp/dp_aux.c | 19 +++++++++----------
> 1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
> b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 91188466cece..b49810396513 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -329,30 +329,29 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
> *dp_aux,
> ssize_t ret;
> int const aux_cmd_native_max = 16;
> int const aux_cmd_i2c_max = 128;
> - struct dp_aux_private *aux = container_of(dp_aux,
> - struct dp_aux_private, dp_aux);
> + struct dp_aux_private *aux;
>
> - mutex_lock(&aux->mutex);
> + aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
>
> aux->native = msg->request & (DP_AUX_NATIVE_WRITE &
> DP_AUX_NATIVE_READ);
>
> /* Ignore address only message */
> - if ((msg->size == 0) || (msg->buffer == NULL)) {
> + if (msg->size == 0 || !msg->buffer) {
> msg->reply = aux->native ?
> DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
> - ret = msg->size;
> - goto unlock_exit;
> + return msg->size;
> }
>
> /* msg sanity check */
> - if ((aux->native && (msg->size > aux_cmd_native_max)) ||
> - (msg->size > aux_cmd_i2c_max)) {
> + if ((aux->native && msg->size > aux_cmd_native_max) ||
> + msg->size > aux_cmd_i2c_max) {
> DRM_ERROR("%s: invalid msg: size(%zu), request(%x)\n",
> __func__, msg->size, msg->request);
> - ret = -EINVAL;
> - goto unlock_exit;
> + return -EINVAL;
> }
>
> + mutex_lock(&aux->mutex);
> +
> dp_aux_update_offset_and_segment(aux, msg);
> dp_aux_transfer_helper(aux, msg, true);
On 2021-05-07 14:25, Stephen Boyd wrote:
> Let's look at the irq status bits after a transfer and see if we got a
> nack or a defer or a timeout, instead of telling drm layers that
> everything was fine, while still printing an error message. I wasn't
> sure about NACK+DEFER so I lumped all those various errors along with a
> nack so that the drm core can figure out that things are just not going
> well. The important thing is that we're now returning -ETIMEDOUT when
> the message times out and nacks for bad addresses.
>
> Cc: Dmitry Baryshkov <[email protected]>
> Cc: Abhinav Kumar <[email protected]>
> Cc: Kuogee Hsieh <[email protected]>
> Cc: [email protected]
> Cc: Sean Paul <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> drivers/gpu/drm/msm/dp/dp_aux.c | 140 ++++++++++++++------------------
> drivers/gpu/drm/msm/dp/dp_aux.h | 8 --
> 2 files changed, 61 insertions(+), 87 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
> b/drivers/gpu/drm/msm/dp/dp_aux.c
> index b49810396513..4a3293b590b0 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -9,7 +9,15 @@
> #include "dp_reg.h"
> #include "dp_aux.h"
>
> -#define DP_AUX_ENUM_STR(x) #x
> +enum msm_dp_aux_err {
> + DP_AUX_ERR_NONE,
> + DP_AUX_ERR_ADDR,
> + DP_AUX_ERR_TOUT,
> + DP_AUX_ERR_NACK,
> + DP_AUX_ERR_DEFER,
> + DP_AUX_ERR_NACK_DEFER,
> + DP_AUX_ERR_PHY,
> +};
>
> struct dp_aux_private {
> struct device *dev;
> @@ -18,7 +26,7 @@ struct dp_aux_private {
> struct mutex mutex;
> struct completion comp;
>
> - u32 aux_error_num;
> + enum msm_dp_aux_err aux_error_num;
> u32 retry_cnt;
> bool cmd_busy;
> bool native;
> @@ -33,62 +41,45 @@ struct dp_aux_private {
>
> #define MAX_AUX_RETRIES 5
>
> -static const char *dp_aux_get_error(u32 aux_error)
> -{
> - switch (aux_error) {
> - case DP_AUX_ERR_NONE:
> - return DP_AUX_ENUM_STR(DP_AUX_ERR_NONE);
> - case DP_AUX_ERR_ADDR:
> - return DP_AUX_ENUM_STR(DP_AUX_ERR_ADDR);
> - case DP_AUX_ERR_TOUT:
> - return DP_AUX_ENUM_STR(DP_AUX_ERR_TOUT);
> - case DP_AUX_ERR_NACK:
> - return DP_AUX_ENUM_STR(DP_AUX_ERR_NACK);
> - case DP_AUX_ERR_DEFER:
> - return DP_AUX_ENUM_STR(DP_AUX_ERR_DEFER);
> - case DP_AUX_ERR_NACK_DEFER:
> - return DP_AUX_ENUM_STR(DP_AUX_ERR_NACK_DEFER);
> - default:
> - return "unknown";
> - }
> -}
> -
> -static u32 dp_aux_write(struct dp_aux_private *aux,
> +static ssize_t dp_aux_write(struct dp_aux_private *aux,
> struct drm_dp_aux_msg *msg)
> {
> - u32 data[4], reg, len;
> + u8 data[4];
> + u32 reg;
> + ssize_t len;
> u8 *msgdata = msg->buffer;
> int const AUX_CMD_FIFO_LEN = 128;
> int i = 0;
>
> if (aux->read)
> - len = 4;
> + len = 0;
> else
> - len = msg->size + 4;
> + len = msg->size;
>
> /*
> * cmd fifo only has depth of 144 bytes
> * limit buf length to 128 bytes here
> */
> - if (len > AUX_CMD_FIFO_LEN) {
> + if (len > AUX_CMD_FIFO_LEN - 4) {
> DRM_ERROR("buf size greater than allowed size of 128 bytes\n");
> - return 0;
> + return -EINVAL;
> }
>
> /* Pack cmd and write to HW */
> - data[0] = (msg->address >> 16) & 0xf; /* addr[19:16] */
> + data[0] = (msg->address >> 16) & 0xf; /* addr[19:16] */
> if (aux->read)
> - data[0] |= BIT(4); /* R/W */
> + data[0] |= BIT(4); /* R/W */
>
> - data[1] = (msg->address >> 8) & 0xff; /* addr[15:8] */
> - data[2] = msg->address & 0xff; /* addr[7:0] */
> - data[3] = (msg->size - 1) & 0xff; /* len[7:0] */
> + data[1] = msg->address >> 8; /* addr[15:8] */
> + data[2] = msg->address; /* addr[7:0] */
> + data[3] = msg->size - 1; /* len[7:0] */
>
> - for (i = 0; i < len; i++) {
> + for (i = 0; i < len + 4; i++) {
> reg = (i < 4) ? data[i] : msgdata[i - 4];
> + reg <<= DP_AUX_DATA_OFFSET;
> + reg &= DP_AUX_DATA_MASK;
> + reg |= DP_AUX_DATA_WRITE;
> /* index = 0, write */
> - reg = (((reg) << DP_AUX_DATA_OFFSET)
> - & DP_AUX_DATA_MASK) | DP_AUX_DATA_WRITE;
> if (i == 0)
> reg |= DP_AUX_DATA_INDEX_WRITE;
> aux->catalog->aux_data = reg;
> @@ -116,39 +107,27 @@ static u32 dp_aux_write(struct dp_aux_private
> *aux,
> return len;
> }
>
> -static int dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
> +static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
> struct drm_dp_aux_msg *msg)
> {
> - u32 ret, len, timeout;
> - int aux_timeout_ms = HZ/4;
> + ssize_t ret;
> + unsigned long time_left;
>
> reinit_completion(&aux->comp);
>
> - len = dp_aux_write(aux, msg);
> - if (len == 0) {
> - DRM_ERROR("DP AUX write failed\n");
> - return -EINVAL;
> - }
> + ret = dp_aux_write(aux, msg);
> + if (ret < 0)
> + return ret;
>
> - timeout = wait_for_completion_timeout(&aux->comp, aux_timeout_ms);
> - if (!timeout) {
> - DRM_ERROR("aux %s timeout\n", (aux->read ? "read" : "write"));
> + time_left = wait_for_completion_timeout(&aux->comp,
> + msecs_to_jiffies(250));
> + if (!time_left)
> return -ETIMEDOUT;
> - }
> -
> - if (aux->aux_error_num == DP_AUX_ERR_NONE) {
> - ret = len;
> - } else {
> - DRM_ERROR_RATELIMITED("aux err: %s\n",
> - dp_aux_get_error(aux->aux_error_num));
> -
> - ret = -EINVAL;
> - }
>
> return ret;
> }
>
> -static void dp_aux_cmd_fifo_rx(struct dp_aux_private *aux,
> +static ssize_t dp_aux_cmd_fifo_rx(struct dp_aux_private *aux,
> struct drm_dp_aux_msg *msg)
> {
> u32 data;
> @@ -175,9 +154,10 @@ static void dp_aux_cmd_fifo_rx(struct
> dp_aux_private *aux,
>
> actual_i = (data >> DP_AUX_DATA_INDEX_OFFSET) & 0xFF;
> if (i != actual_i)
> - DRM_ERROR("Index mismatch: expected %d, found %d\n",
> - i, actual_i);
> + break;
> }
> +
> + return i;
> }
>
> static void dp_aux_native_handler(struct dp_aux_private *aux, u32 isr)
> @@ -367,36 +347,38 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
> *dp_aux,
> }
>
> ret = dp_aux_cmd_fifo_tx(aux, msg);
> -
> if (ret < 0) {
> if (aux->native) {
> aux->retry_cnt++;
> if (!(aux->retry_cnt % MAX_AUX_RETRIES))
> dp_catalog_aux_update_cfg(aux->catalog);
> }
> - usleep_range(400, 500); /* at least 400us to next try */
> - goto unlock_exit;
> - }
1) dp_catalog_aux_update_cfg(aux->catalog) will not work without
dp_catalog_aux_reset(aux->catalog);
dp_catalog_aux_reset(aux->catalog) will reset hpd control block and
potentially cause pending hpd interrupts got lost.
Therefore I think we should not do
dp_catalog_aux_update_cfg(aux->catalog) for now.
reset aux controller will reset hpd control block probolem will be fixed
at next chipset.
after that we can add dp_catalog_aux_update_cfg(aux->catalog) followed
by dp_catalog_aux_reset(aux->catalog) back at next chipset.
2) according to DP specification, aux read/write failed have to wait at
least 400us before next try can start.
Otherwise, DP compliant test will failed
> -
> - if (aux->aux_error_num == DP_AUX_ERR_NONE) {
> - if (aux->read)
> - dp_aux_cmd_fifo_rx(aux, msg);
> -
> - msg->reply = aux->native ?
> - DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
> } else {
> - /* Reply defer to retry */
> - msg->reply = aux->native ?
> - DP_AUX_NATIVE_REPLY_DEFER : DP_AUX_I2C_REPLY_DEFER;
> + aux->retry_cnt = 0;
> + switch (aux->aux_error_num) {
> + case DP_AUX_ERR_NONE:
> + if (aux->read)
> + ret = dp_aux_cmd_fifo_rx(aux, msg);
> + msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_ACK :
> DP_AUX_I2C_REPLY_ACK;
> + break;
> + case DP_AUX_ERR_DEFER:
> + msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_DEFER :
> DP_AUX_I2C_REPLY_DEFER;
> + break;
> + case DP_AUX_ERR_PHY:
> + case DP_AUX_ERR_ADDR:
> + case DP_AUX_ERR_NACK:
> + case DP_AUX_ERR_NACK_DEFER:
> + msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_NACK :
> DP_AUX_I2C_REPLY_NACK;
> + break;
> + case DP_AUX_ERR_TOUT:
> + ret = -ETIMEDOUT;
> + break;
> + }
> }
>
> - /* Return requested size for success or retry */
> - ret = msg->size;
> - aux->retry_cnt = 0;
> -
> -unlock_exit:
> aux->cmd_busy = false;
> mutex_unlock(&aux->mutex);
> +
> return ret;
> }
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h
> b/drivers/gpu/drm/msm/dp/dp_aux.h
> index f8b8ba919465..0728cc09c9ec 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.h
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.h
> @@ -9,14 +9,6 @@
> #include "dp_catalog.h"
> #include <drm/drm_dp_helper.h>
>
> -#define DP_AUX_ERR_NONE 0
> -#define DP_AUX_ERR_ADDR -1
> -#define DP_AUX_ERR_TOUT -2
> -#define DP_AUX_ERR_NACK -3
> -#define DP_AUX_ERR_DEFER -4
> -#define DP_AUX_ERR_NACK_DEFER -5
> -#define DP_AUX_ERR_PHY -6
> -
> int dp_aux_register(struct drm_dp_aux *dp_aux);
> void dp_aux_unregister(struct drm_dp_aux *dp_aux);
> void dp_aux_isr(struct drm_dp_aux *dp_aux);
Quoting [email protected] (2021-05-24 09:33:49)
> On 2021-05-07 14:25, Stephen Boyd wrote:
> > @@ -367,36 +347,38 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
> > *dp_aux,
> > }
> >
> > ret = dp_aux_cmd_fifo_tx(aux, msg);
> > -
> > if (ret < 0) {
> > if (aux->native) {
> > aux->retry_cnt++;
> > if (!(aux->retry_cnt % MAX_AUX_RETRIES))
> > dp_catalog_aux_update_cfg(aux->catalog);
> > }
> > - usleep_range(400, 500); /* at least 400us to next try */
> > - goto unlock_exit;
> > - }
>
> 1) dp_catalog_aux_update_cfg(aux->catalog) will not work without
> dp_catalog_aux_reset(aux->catalog);
> dp_catalog_aux_reset(aux->catalog) will reset hpd control block and
> potentially cause pending hpd interrupts got lost.
> Therefore I think we should not do
> dp_catalog_aux_update_cfg(aux->catalog) for now.
> reset aux controller will reset hpd control block probolem will be fixed
> at next chipset.
> after that we can add dp_catalog_aux_update_cfg(aux->catalog) followed
> by dp_catalog_aux_reset(aux->catalog) back at next chipset.
Hmm ok. So the phy calibration logic that tweaks the tuning values is
never used? Why can't the phy be tuned while it is active? I don't
understand why we would ever want to reset the aux phy when changing the
settings for a retry. Either way, this is not actually changing in this
patch so it would be another patch to remove this code.
>
> 2) according to DP specification, aux read/write failed have to wait at
> least 400us before next try can start.
> Otherwise, DP compliant test will failed
Yes. The caller of this function, drm_dp_dpcd_access(), has the delay
already
if (ret != 0 && ret != -ETIMEDOUT) {
usleep_range(AUX_RETRY_INTERVAL,
AUX_RETRY_INTERVAL + 100);
}
so this delay here is redundant.
On 2021-05-07 14:25, Stephen Boyd wrote:
> Let's look at the irq status bits after a transfer and see if we got a
> nack or a defer or a timeout, instead of telling drm layers that
> everything was fine, while still printing an error message. I wasn't
> sure about NACK+DEFER so I lumped all those various errors along with a
> nack so that the drm core can figure out that things are just not going
> well. The important thing is that we're now returning -ETIMEDOUT when
> the message times out and nacks for bad addresses.
>
> Cc: Dmitry Baryshkov <[email protected]>
> Cc: Abhinav Kumar <[email protected]>
> Cc: Kuogee Hsieh <[email protected]>
> Cc: [email protected]
> Cc: Sean Paul <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
Reviewed-by: Kuogee Hsieh <[email protected]>
> ---
> drivers/gpu/drm/msm/dp/dp_aux.c | 140 ++++++++++++++------------------
> drivers/gpu/drm/msm/dp/dp_aux.h | 8 --
> 2 files changed, 61 insertions(+), 87 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
> b/drivers/gpu/drm/msm/dp/dp_aux.c
> index b49810396513..4a3293b590b0 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -9,7 +9,15 @@
> #include "dp_reg.h"
> #include "dp_aux.h"
>
> -#define DP_AUX_ENUM_STR(x) #x
> +enum msm_dp_aux_err {
> + DP_AUX_ERR_NONE,
> + DP_AUX_ERR_ADDR,
> + DP_AUX_ERR_TOUT,
> + DP_AUX_ERR_NACK,
> + DP_AUX_ERR_DEFER,
> + DP_AUX_ERR_NACK_DEFER,
> + DP_AUX_ERR_PHY,
> +};
>
> struct dp_aux_private {
> struct device *dev;
> @@ -18,7 +26,7 @@ struct dp_aux_private {
> struct mutex mutex;
> struct completion comp;
>
> - u32 aux_error_num;
> + enum msm_dp_aux_err aux_error_num;
> u32 retry_cnt;
> bool cmd_busy;
> bool native;
> @@ -33,62 +41,45 @@ struct dp_aux_private {
>
> #define MAX_AUX_RETRIES 5
>
> -static const char *dp_aux_get_error(u32 aux_error)
> -{
> - switch (aux_error) {
> - case DP_AUX_ERR_NONE:
> - return DP_AUX_ENUM_STR(DP_AUX_ERR_NONE);
> - case DP_AUX_ERR_ADDR:
> - return DP_AUX_ENUM_STR(DP_AUX_ERR_ADDR);
> - case DP_AUX_ERR_TOUT:
> - return DP_AUX_ENUM_STR(DP_AUX_ERR_TOUT);
> - case DP_AUX_ERR_NACK:
> - return DP_AUX_ENUM_STR(DP_AUX_ERR_NACK);
> - case DP_AUX_ERR_DEFER:
> - return DP_AUX_ENUM_STR(DP_AUX_ERR_DEFER);
> - case DP_AUX_ERR_NACK_DEFER:
> - return DP_AUX_ENUM_STR(DP_AUX_ERR_NACK_DEFER);
> - default:
> - return "unknown";
> - }
> -}
> -
> -static u32 dp_aux_write(struct dp_aux_private *aux,
> +static ssize_t dp_aux_write(struct dp_aux_private *aux,
> struct drm_dp_aux_msg *msg)
> {
> - u32 data[4], reg, len;
> + u8 data[4];
> + u32 reg;
> + ssize_t len;
> u8 *msgdata = msg->buffer;
> int const AUX_CMD_FIFO_LEN = 128;
> int i = 0;
>
> if (aux->read)
> - len = 4;
> + len = 0;
> else
> - len = msg->size + 4;
> + len = msg->size;
>
> /*
> * cmd fifo only has depth of 144 bytes
> * limit buf length to 128 bytes here
> */
> - if (len > AUX_CMD_FIFO_LEN) {
> + if (len > AUX_CMD_FIFO_LEN - 4) {
> DRM_ERROR("buf size greater than allowed size of 128 bytes\n");
> - return 0;
> + return -EINVAL;
> }
>
> /* Pack cmd and write to HW */
> - data[0] = (msg->address >> 16) & 0xf; /* addr[19:16] */
> + data[0] = (msg->address >> 16) & 0xf; /* addr[19:16] */
> if (aux->read)
> - data[0] |= BIT(4); /* R/W */
> + data[0] |= BIT(4); /* R/W */
>
> - data[1] = (msg->address >> 8) & 0xff; /* addr[15:8] */
> - data[2] = msg->address & 0xff; /* addr[7:0] */
> - data[3] = (msg->size - 1) & 0xff; /* len[7:0] */
> + data[1] = msg->address >> 8; /* addr[15:8] */
> + data[2] = msg->address; /* addr[7:0] */
> + data[3] = msg->size - 1; /* len[7:0] */
>
> - for (i = 0; i < len; i++) {
> + for (i = 0; i < len + 4; i++) {
> reg = (i < 4) ? data[i] : msgdata[i - 4];
> + reg <<= DP_AUX_DATA_OFFSET;
> + reg &= DP_AUX_DATA_MASK;
> + reg |= DP_AUX_DATA_WRITE;
> /* index = 0, write */
> - reg = (((reg) << DP_AUX_DATA_OFFSET)
> - & DP_AUX_DATA_MASK) | DP_AUX_DATA_WRITE;
> if (i == 0)
> reg |= DP_AUX_DATA_INDEX_WRITE;
> aux->catalog->aux_data = reg;
> @@ -116,39 +107,27 @@ static u32 dp_aux_write(struct dp_aux_private
> *aux,
> return len;
> }
>
> -static int dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
> +static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
> struct drm_dp_aux_msg *msg)
> {
> - u32 ret, len, timeout;
> - int aux_timeout_ms = HZ/4;
> + ssize_t ret;
> + unsigned long time_left;
>
> reinit_completion(&aux->comp);
>
> - len = dp_aux_write(aux, msg);
> - if (len == 0) {
> - DRM_ERROR("DP AUX write failed\n");
> - return -EINVAL;
> - }
> + ret = dp_aux_write(aux, msg);
> + if (ret < 0)
> + return ret;
>
> - timeout = wait_for_completion_timeout(&aux->comp, aux_timeout_ms);
> - if (!timeout) {
> - DRM_ERROR("aux %s timeout\n", (aux->read ? "read" : "write"));
> + time_left = wait_for_completion_timeout(&aux->comp,
> + msecs_to_jiffies(250));
> + if (!time_left)
> return -ETIMEDOUT;
> - }
> -
> - if (aux->aux_error_num == DP_AUX_ERR_NONE) {
> - ret = len;
> - } else {
> - DRM_ERROR_RATELIMITED("aux err: %s\n",
> - dp_aux_get_error(aux->aux_error_num));
> -
> - ret = -EINVAL;
> - }
>
> return ret;
> }
>
> -static void dp_aux_cmd_fifo_rx(struct dp_aux_private *aux,
> +static ssize_t dp_aux_cmd_fifo_rx(struct dp_aux_private *aux,
> struct drm_dp_aux_msg *msg)
> {
> u32 data;
> @@ -175,9 +154,10 @@ static void dp_aux_cmd_fifo_rx(struct
> dp_aux_private *aux,
>
> actual_i = (data >> DP_AUX_DATA_INDEX_OFFSET) & 0xFF;
> if (i != actual_i)
> - DRM_ERROR("Index mismatch: expected %d, found %d\n",
> - i, actual_i);
> + break;
> }
> +
> + return i;
> }
>
> static void dp_aux_native_handler(struct dp_aux_private *aux, u32 isr)
> @@ -367,36 +347,38 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
> *dp_aux,
> }
>
> ret = dp_aux_cmd_fifo_tx(aux, msg);
> -
> if (ret < 0) {
> if (aux->native) {
> aux->retry_cnt++;
> if (!(aux->retry_cnt % MAX_AUX_RETRIES))
> dp_catalog_aux_update_cfg(aux->catalog);
> }
> - usleep_range(400, 500); /* at least 400us to next try */
> - goto unlock_exit;
> - }
> -
> - if (aux->aux_error_num == DP_AUX_ERR_NONE) {
> - if (aux->read)
> - dp_aux_cmd_fifo_rx(aux, msg);
> -
> - msg->reply = aux->native ?
> - DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
> } else {
> - /* Reply defer to retry */
> - msg->reply = aux->native ?
> - DP_AUX_NATIVE_REPLY_DEFER : DP_AUX_I2C_REPLY_DEFER;
> + aux->retry_cnt = 0;
> + switch (aux->aux_error_num) {
> + case DP_AUX_ERR_NONE:
> + if (aux->read)
> + ret = dp_aux_cmd_fifo_rx(aux, msg);
> + msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_ACK :
> DP_AUX_I2C_REPLY_ACK;
> + break;
> + case DP_AUX_ERR_DEFER:
> + msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_DEFER :
> DP_AUX_I2C_REPLY_DEFER;
> + break;
> + case DP_AUX_ERR_PHY:
> + case DP_AUX_ERR_ADDR:
> + case DP_AUX_ERR_NACK:
> + case DP_AUX_ERR_NACK_DEFER:
> + msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_NACK :
> DP_AUX_I2C_REPLY_NACK;
> + break;
> + case DP_AUX_ERR_TOUT:
> + ret = -ETIMEDOUT;
> + break;
> + }
> }
>
> - /* Return requested size for success or retry */
> - ret = msg->size;
> - aux->retry_cnt = 0;
> -
> -unlock_exit:
> aux->cmd_busy = false;
> mutex_unlock(&aux->mutex);
> +
> return ret;
> }
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h
> b/drivers/gpu/drm/msm/dp/dp_aux.h
> index f8b8ba919465..0728cc09c9ec 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.h
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.h
> @@ -9,14 +9,6 @@
> #include "dp_catalog.h"
> #include <drm/drm_dp_helper.h>
>
> -#define DP_AUX_ERR_NONE 0
> -#define DP_AUX_ERR_ADDR -1
> -#define DP_AUX_ERR_TOUT -2
> -#define DP_AUX_ERR_NACK -3
> -#define DP_AUX_ERR_DEFER -4
> -#define DP_AUX_ERR_NACK_DEFER -5
> -#define DP_AUX_ERR_PHY -6
> -
> int dp_aux_register(struct drm_dp_aux *dp_aux);
> void dp_aux_unregister(struct drm_dp_aux *dp_aux);
> void dp_aux_isr(struct drm_dp_aux *dp_aux);
On 2021-05-24 12:19, Stephen Boyd wrote:
> Quoting [email protected] (2021-05-24 09:33:49)
>> On 2021-05-07 14:25, Stephen Boyd wrote:
>> > @@ -367,36 +347,38 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
>> > *dp_aux,
>> > }
>> >
>> > ret = dp_aux_cmd_fifo_tx(aux, msg);
>> > -
>> > if (ret < 0) {
>> > if (aux->native) {
>> > aux->retry_cnt++;
>> > if (!(aux->retry_cnt % MAX_AUX_RETRIES))
>> > dp_catalog_aux_update_cfg(aux->catalog);
>> > }
>> > - usleep_range(400, 500); /* at least 400us to next try */
>> > - goto unlock_exit;
>> > - }
>>
>> 1) dp_catalog_aux_update_cfg(aux->catalog) will not work without
>> dp_catalog_aux_reset(aux->catalog);
>> dp_catalog_aux_reset(aux->catalog) will reset hpd control block and
>> potentially cause pending hpd interrupts got lost.
>> Therefore I think we should not do
>> dp_catalog_aux_update_cfg(aux->catalog) for now.
>> reset aux controller will reset hpd control block probolem will be
>> fixed
>> at next chipset.
>> after that we can add dp_catalog_aux_update_cfg(aux->catalog) followed
>> by dp_catalog_aux_reset(aux->catalog) back at next chipset.
>
> Hmm ok. So the phy calibration logic that tweaks the tuning values is
> never used? Why can't the phy be tuned while it is active? I don't
> understand why we would ever want to reset the aux phy when changing
> the
> settings for a retry. Either way, this is not actually changing in this
> patch so it would be another patch to remove this code.
ok,
>
>>
>> 2) according to DP specification, aux read/write failed have to wait
>> at
>> least 400us before next try can start.
>> Otherwise, DP compliant test will failed
>
> Yes. The caller of this function, drm_dp_dpcd_access(), has the delay
> already
>
> if (ret != 0 && ret != -ETIMEDOUT) {
> usleep_range(AUX_RETRY_INTERVAL,
> AUX_RETRY_INTERVAL + 100);
> }
>
> so this delay here is redundant.
yes, you are right. This is enough.