From c4749c9a4a9ddc16200ce46a19078357727bf4b1 Mon Sep 17 00:00:00 2001 From: Maarten Lankhorst Date: Mon, 31 Aug 2015 12:25:04 +0200 Subject: drm/atomic: Fix bookkeeping with TEST_ONLY, v3. Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e "drm/atomic: Cleanup on error properly in the atomic ioctl." cleaned up some error paths, but didn't fix the TEST_ONLY path. In the check only case plane->fb shouldn't be updated, and the vblank events should be cleared as on failure. Changes since v1: - Fix -EDEADLK handling of vblank events too. - Free state last with CHECK_ONLY. Changes since v2: - Add comment about freeing crtc_state->event with TEST_ONLY. (Daniel Stone) Signed-off-by: Maarten Lankhorst Reviewed-by: Daniel Stone Signed-off-by: Jani Nikula --- drivers/gpu/drm/drm_atomic.c | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 434915448ea0..f7d5166f89b2 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1515,7 +1515,8 @@ retry: copied_props++; } - if (obj->type == DRM_MODE_OBJECT_PLANE && count_props) { + if (obj->type == DRM_MODE_OBJECT_PLANE && count_props && + !(arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)) { plane = obj_to_plane(obj); plane_mask |= (1 << drm_plane_index(plane)); plane->old_fb = plane->fb; @@ -1537,10 +1538,11 @@ retry: } if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) { + /* + * Unlike commit, check_only does not clean up state. + * Below we call drm_atomic_state_free for it. + */ ret = drm_atomic_check_only(state); - /* _check_only() does not free state, unlike _commit() */ - if (!ret) - drm_atomic_state_free(state); } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) { ret = drm_atomic_async_commit(state); } else { @@ -1567,25 +1569,30 @@ out: plane->old_fb = NULL; } + if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { + /* + * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive, + * if they weren't, this code should be called on success + * for TEST_ONLY too. + */ + + for_each_crtc_in_state(state, crtc, crtc_state, i) { + if (!crtc_state->event) + continue; + + destroy_vblank_event(dev, file_priv, + crtc_state->event); + } + } + if (ret == -EDEADLK) { drm_atomic_state_clear(state); drm_modeset_backoff(&ctx); goto retry; } - if (ret) { - if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { - for_each_crtc_in_state(state, crtc, crtc_state, i) { - if (!crtc_state->event) - continue; - - destroy_vblank_event(dev, file_priv, - crtc_state->event); - } - } - + if (ret || arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) drm_atomic_state_free(state); - } drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx); -- cgit v1.2.3 From 79a2b161c12a1b751f53a7698494cb722530bca5 Mon Sep 17 00:00:00 2001 From: Ville Syrjälä Date: Wed, 26 Aug 2015 22:55:05 +0300 Subject: drm/dp: Define AUX_RETRY_INTERVAL as 500 us MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently we react to native and i2c defers by waiting either 400-500 us or 500-600 us, depending on which code path we take. Consolidate them all to one define AUX_RETRY_INTERVAL which defines the minimum interval. Since we've been using two different intervals pick the longer of them and define AUX_RETRY_INTERVAL as 500 us. For the maximum just use AUX_RETRY_INTERVAL+100 us. I want to have a define for this so that I can use it when calculating the estimated duration of i2c-over-aux transfers. Without a define it would be very easy to change the sleep duration and neglect to update the i2c-over-aux estimates. Cc: Simon Farnsworth Cc: moosotc@gmail.com Signed-off-by: Ville Syrjälä Reviewed-by: Simon Farnsworth Reviewed-by: Daniel Vetter Signed-off-by: Jani Nikula --- drivers/gpu/drm/drm_dp_helper.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 80a02a412607..7069e54e5b43 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -159,6 +159,8 @@ int drm_dp_bw_code_to_link_rate(u8 link_bw) } EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate); +#define AUX_RETRY_INTERVAL 500 /* us */ + /** * DOC: dp helpers * @@ -213,7 +215,7 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request, return -EIO; case DP_AUX_NATIVE_REPLY_DEFER: - usleep_range(400, 500); + usleep_range(AUX_RETRY_INTERVAL, AUX_RETRY_INTERVAL + 100); break; } } @@ -476,7 +478,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) * For now just defer for long enough to hopefully be * safe for all use-cases. */ - usleep_range(500, 600); + usleep_range(AUX_RETRY_INTERVAL, AUX_RETRY_INTERVAL + 100); continue; default: @@ -506,7 +508,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) aux->i2c_defer_count++; if (defer_i2c < 7) defer_i2c++; - usleep_range(400, 500); + usleep_range(AUX_RETRY_INTERVAL, AUX_RETRY_INTERVAL + 100); continue; default: -- cgit v1.2.3 From 4efa83c8c786ab7ec7982e3dd348cb7e7ecbeb04 Mon Sep 17 00:00:00 2001 From: Ville Syrjälä Date: Tue, 1 Sep 2015 20:12:54 +0300 Subject: drm/dp: Adjust i2c-over-aux retry count based on message size and i2c bus speed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Calculate the number of retries we should do for each i2c-over-aux message based on the time it takes to perform the i2c transfer vs. the aux transfer. We assume the shortest possible length for the aux transfer, and the longest possible (exluding clock stretching) for the i2c transfer. The DP spec has some examples on how to calculate this, but we don't calculate things quite the same way. The spec doesn't account for the retry interval (assumes immediate retry on defer), and doesn't assume the best/worst case behaviour as we do. Note that currently we assume 10 kHz speed for the i2c bus. Some real world devices (eg. some Apple DP->VGA dongle) fails with less than 16 retries. and that would correspond to something close to 15 kHz (with our method of calculating things) But let's just go for 10 kHz to be on the safe side. Ideally we should query/set the i2c bus speed via DPCD but for now this should at leaast remove the regression from the 1->16 byte trasnfer size change. And of course if the sink completes the transfer quicker this shouldn't slow things down since we don't change the interval between retries. I did a few experiments with a DP->DVI dongle I have that allows you to change the i2c bus speed. Here are the results of me changing the actual bus speed and the assumed bus speed and seeing when we start to fail the operation: actual i2c khz assumed i2c khz max retries 1 1 ok -> 2 fail 211 ok -> 106 fail 5 8 ok -> 9 fail 27 ok -> 24 fail 10 17 ok -> 18 fail 13 ok -> 12 fail 100 210 ok -> 211 fail 2 ok -> 1 fail So based on that we have a fairly decent safety margin baked into the formula to calculate the max number of retries. Fixes a regression with some DP dongles from: commit 1d002fa720738bcd0bddb9178e9ea0773288e1dd Author: Simon Farnsworth Date: Tue Feb 10 18:38:08 2015 +0000 drm/dp: Use large transactions for I2C over AUX v2: Use best case for AUX and worst case for i2c (Simon Farnsworth) Add a define our AUX retry interval and account for it v3: Make everything usecs to avoid confusion about units (Daniel) Add a comment reminding people about the AUX bitrate (Daniel) Use DIV_ROUND_UP() since we're after the "worst" case for i2c Cc: Simon Farnsworth Cc: moosotc@gmail.com Tested-by: moosotc@gmail.com Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91451 Reviewed-by: Simon Farnsworth Reviewed-by: Daniel Vetter Signed-off-by: Ville Syrjälä Signed-off-by: Jani Nikula --- drivers/gpu/drm/drm_dp_helper.c | 84 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 82 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 7069e54e5b43..214a4c649d56 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -424,6 +424,81 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter) I2C_FUNC_10BIT_ADDR; } +#define AUX_PRECHARGE_LEN 10 /* 10 to 16 */ +#define AUX_SYNC_LEN (16 + 4) /* preamble + AUX_SYNC_END */ +#define AUX_STOP_LEN 4 +#define AUX_CMD_LEN 4 +#define AUX_ADDRESS_LEN 20 +#define AUX_REPLY_PAD_LEN 4 +#define AUX_LENGTH_LEN 8 + +/* + * Calculate the duration of the AUX request/reply in usec. Gives the + * "best" case estimate, ie. successful while as short as possible. + */ +static int drm_dp_aux_req_duration(const struct drm_dp_aux_msg *msg) +{ + int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN + + AUX_CMD_LEN + AUX_ADDRESS_LEN + AUX_LENGTH_LEN; + + if ((msg->request & DP_AUX_I2C_READ) == 0) + len += msg->size * 8; + + return len; +} + +static int drm_dp_aux_reply_duration(const struct drm_dp_aux_msg *msg) +{ + int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN + + AUX_CMD_LEN + AUX_REPLY_PAD_LEN; + + /* + * For read we expect what was asked. For writes there will + * be 0 or 1 data bytes. Assume 0 for the "best" case. + */ + if (msg->request & DP_AUX_I2C_READ) + len += msg->size * 8; + + return len; +} + +#define I2C_START_LEN 1 +#define I2C_STOP_LEN 1 +#define I2C_ADDR_LEN 9 /* ADDRESS + R/W + ACK/NACK */ +#define I2C_DATA_LEN 9 /* DATA + ACK/NACK */ + +/* + * Calculate the length of the i2c transfer in usec, assuming + * the i2c bus speed is as specified. Gives the the "worst" + * case estimate, ie. successful while as long as possible. + * Doesn't account the the "MOT" bit, and instead assumes each + * message includes a START, ADDRESS and STOP. Neither does it + * account for additional random variables such as clock stretching. + */ +static int drm_dp_i2c_msg_duration(const struct drm_dp_aux_msg *msg, + int i2c_speed_khz) +{ + /* AUX bitrate is 1MHz, i2c bitrate as specified */ + return DIV_ROUND_UP((I2C_START_LEN + I2C_ADDR_LEN + + msg->size * I2C_DATA_LEN + + I2C_STOP_LEN) * 1000, i2c_speed_khz); +} + +/* + * Deterine how many retries should be attempted to successfully transfer + * the specified message, based on the estimated durations of the + * i2c and AUX transfers. + */ +static int drm_dp_i2c_retry_count(const struct drm_dp_aux_msg *msg, + int i2c_speed_khz) +{ + int aux_time_us = drm_dp_aux_req_duration(msg) + + drm_dp_aux_reply_duration(msg); + int i2c_time_us = drm_dp_i2c_msg_duration(msg, i2c_speed_khz); + + return DIV_ROUND_UP(i2c_time_us, aux_time_us + AUX_RETRY_INTERVAL); +} + /* * Transfer a single I2C-over-AUX message and handle various error conditions, * retrying the transaction as appropriate. It is assumed that the @@ -436,13 +511,18 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) { unsigned int retry, defer_i2c; int ret; - /* * 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. + * + * We also try to account for the i2c bus speed. + * FIXME currently assumes 10 kHz as some real world devices seem + * to require it. We should query/set the speed via DPCD if supported. */ - for (retry = 0, defer_i2c = 0; retry < (7 + defer_i2c); retry++) { + int max_retries = max(7, drm_dp_i2c_retry_count(msg, 10)); + + for (retry = 0, defer_i2c = 0; retry < (max_retries + defer_i2c); retry++) { mutex_lock(&aux->hw_mutex); ret = aux->transfer(aux, msg); mutex_unlock(&aux->hw_mutex); -- cgit v1.2.3 From f36203be608a38a5b5523a7aa52cc72f757b9679 Mon Sep 17 00:00:00 2001 From: Ville Syrjälä Date: Wed, 26 Aug 2015 22:55:07 +0300 Subject: drm/dp: Add dp_aux_i2c_speed_khz module param to set the assume i2c bus speed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To help with debugging i2c-over-aux issues, add a module parameter than can be used to tweak the assumed i2c bus speed, and thus the maximum number of retries we will do for each aux message. Cc: Simon Farnsworth Cc: moosotc@gmail.com Signed-off-by: Ville Syrjälä Reviewed-by: Simon Farnsworth Reviewed-by: Daniel Vetter Signed-off-by: Jani Nikula --- drivers/gpu/drm/drm_dp_helper.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 214a4c649d56..291734e87fca 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -499,6 +499,15 @@ static int drm_dp_i2c_retry_count(const struct drm_dp_aux_msg *msg, return DIV_ROUND_UP(i2c_time_us, aux_time_us + AUX_RETRY_INTERVAL); } +/* + * FIXME currently assumes 10 kHz as some real world devices seem + * to require it. We should query/set the speed via DPCD if supported. + */ +static int dp_aux_i2c_speed_khz __read_mostly = 10; +module_param_unsafe(dp_aux_i2c_speed_khz, int, 0644); +MODULE_PARM_DESC(dp_aux_i2c_speed_khz, + "Assumed speed of the i2c bus in kHz, (1-400, default 10)"); + /* * Transfer a single I2C-over-AUX message and handle various error conditions, * retrying the transaction as appropriate. It is assumed that the @@ -517,10 +526,8 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) * before giving up the AUX transaction. * * We also try to account for the i2c bus speed. - * FIXME currently assumes 10 kHz as some real world devices seem - * to require it. We should query/set the speed via DPCD if supported. */ - int max_retries = max(7, drm_dp_i2c_retry_count(msg, 10)); + int max_retries = max(7, drm_dp_i2c_retry_count(msg, dp_aux_i2c_speed_khz)); for (retry = 0, defer_i2c = 0; retry < (max_retries + defer_i2c); retry++) { mutex_lock(&aux->hw_mutex); -- cgit v1.2.3