* [PATCH 1/3] drm/i915/bxt,glk: Increase PCODE timeouts during CDCLK freq changing
@ 2018-01-30 11:47 Imre Deak
2018-01-30 11:47 ` [PATCH 2/3] drm/i915: Fold sandybridge_pcode_read() into snb_pcode_request() Imre Deak
` (5 more replies)
0 siblings, 6 replies; 13+ messages in thread
From: Imre Deak @ 2018-01-30 11:47 UTC (permalink / raw)
To: intel-gfx; +Cc: Chris Wilson, Ville Syrjälä, stable, #, v4.4+
Currently we see sporadic timeouts during CDCLK changing both on BXT and
GLK as reported by the Bugzilla: ticket. It's easy to reproduce this by
changing the frequency in a tight loop after blanking the display. The
upper bound for the completion time is 800us based on my tests, so
increase it from the current 500us to 2ms; with that I couldn't trigger
the problem either on BXT or GLK.
Note that timeouts happened during both the change notification and the
voltage level setting PCODE request. (For the latter one BSpec doesn't
require us to wait for completion before further HW programming.)
This issue is similar to
2c7d0602c815 ("drm/i915/gen9: Fix PCODE polling during CDCLK change
notification")
but there the PCODE request does complete (as shown by the mbox
busy flag), only the reply we get from PCODE indicates a failure.
So there we keep resending the request until a success reply, here we
just have to increase the timeout for the one PCODE request we send.
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: stable@vger.kernel.org # v4.4+
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103326
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 6 +++++-
drivers/gpu/drm/i915/intel_cdclk.c | 20 +++++++++++++++-----
drivers/gpu/drm/i915/intel_pm.c | 6 +++---
3 files changed, 23 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 454d8f937fae..5e293be4e51d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3723,7 +3723,11 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e,
struct intel_display_error_state *error);
int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val);
-int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val);
+int snb_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 val,
+ int timeout_us);
+#define sandybridge_pcode_write(dev_priv, mbox, val) \
+ snb_pcode_request(dev_priv, mbox, val, 500)
+
int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request,
u32 reply_mask, u32 reply, int timeout_base_ms);
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index c4392ea34a3d..5057336c40ba 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1370,10 +1370,14 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
break;
}
- /* Inform power controller of upcoming frequency change */
+ /*
+ * Inform power controller of upcoming frequency change. BSpec
+ * requires us to wait up to 150usec, but that leads to timeouts;
+ * the 2ms used here is based on experiment.
+ */
mutex_lock(&dev_priv->pcu_lock);
- ret = sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
- 0x80000000);
+ ret = snb_pcode_request(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
+ 0x80000000, 2000);
mutex_unlock(&dev_priv->pcu_lock);
if (ret) {
@@ -1404,8 +1408,14 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
I915_WRITE(CDCLK_CTL, val);
mutex_lock(&dev_priv->pcu_lock);
- ret = sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
- cdclk_state->voltage_level);
+ /*
+ * The timeout isn't specified, the 2ms used here is based on
+ * experiment.
+ * FIXME: Waiting for the request completion could be delayed until
+ * the next PCODE request based on BSpec.
+ */
+ ret = snb_pcode_request(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
+ cdclk_state->voltage_level, 2000);
mutex_unlock(&dev_priv->pcu_lock);
if (ret) {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0b92ea1dbd40..f6f4dbacb9af 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -9169,8 +9169,8 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
return 0;
}
-int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
- u32 mbox, u32 val)
+int snb_pcode_request(struct drm_i915_private *dev_priv,
+ u32 mbox, u32 val, int timeout_us)
{
int status;
@@ -9193,7 +9193,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
if (__intel_wait_for_register_fw(dev_priv,
GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
- 500, 0, NULL)) {
+ timeout_us, 0, NULL)) {
DRM_ERROR("timeout waiting for pcode write of 0x%08x to mbox %x to finish for %ps\n",
val, mbox, __builtin_return_address(0));
return -ETIMEDOUT;
--
2.13.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/3] drm/i915: Fold sandybridge_pcode_read() into snb_pcode_request()
2018-01-30 11:47 [PATCH 1/3] drm/i915/bxt,glk: Increase PCODE timeouts during CDCLK freq changing Imre Deak
@ 2018-01-30 11:47 ` Imre Deak
2018-01-30 11:57 ` Chris Wilson
2018-01-30 11:47 ` [PATCH 3/3] drm/i915/bxt, glk: Avoid long atomic poll during CDCLK change Imre Deak
` (4 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Imre Deak @ 2018-01-30 11:47 UTC (permalink / raw)
To: intel-gfx
These two functions are very similar so simplify things by removing the
duplication.
Add a seperate sleeping poll timeout parameter, useful for longer polls
like the CDCLK change on BXT/GLK. The next patch will take that into use.
While at it document snb_pcode_request() and clean up a bit the
error/debug prints. Other than that no functional changes.
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 10 ++--
drivers/gpu/drm/i915/intel_cdclk.c | 4 +-
drivers/gpu/drm/i915/intel_pm.c | 97 ++++++++++++++------------------------
3 files changed, 44 insertions(+), 67 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5e293be4e51d..1e9911f66339 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3722,11 +3722,13 @@ intel_display_capture_error_state(struct drm_i915_private *dev_priv);
extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e,
struct intel_display_error_state *error);
-int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val);
-int snb_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 val,
- int timeout_us);
+int snb_pcode_request(struct drm_i915_private *dev_priv, u32 mbox,
+ u32 send_val, u32 *recv_val,
+ int fast_timeout_us, int slow_timeout_ms);
+#define sandybridge_pcode_read(dev_priv, mbox, val) \
+ snb_pcode_request(dev_priv, mbox, *val, val, 500, 0)
#define sandybridge_pcode_write(dev_priv, mbox, val) \
- snb_pcode_request(dev_priv, mbox, val, 500)
+ snb_pcode_request(dev_priv, mbox, val, NULL, 500, 0)
int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request,
u32 reply_mask, u32 reply, int timeout_base_ms);
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index 5057336c40ba..8d06a6f66f29 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1377,7 +1377,7 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
*/
mutex_lock(&dev_priv->pcu_lock);
ret = snb_pcode_request(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
- 0x80000000, 2000);
+ 0x80000000, NULL, 2000, 0);
mutex_unlock(&dev_priv->pcu_lock);
if (ret) {
@@ -1415,7 +1415,7 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
* the next PCODE request based on BSpec.
*/
ret = snb_pcode_request(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
- cdclk_state->voltage_level, 2000);
+ cdclk_state->voltage_level, NULL, 2000, 0);
mutex_unlock(&dev_priv->pcu_lock);
if (ret) {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f6f4dbacb9af..5a6e5dcb6ff8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -9123,7 +9123,29 @@ static inline int gen7_check_mailbox_status(struct drm_i915_private *dev_priv)
}
}
-int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val)
+/**
+ * snb_pcode_request - send PCODE request
+ * @dev_priv: device private
+ * @mbox: PCODE mailbox ID the request is targeted for
+ * @send_val: request-ID or data to send
+ * @reply_val: buffer to store the reply in
+ * @fast_timeout_us: timeout for the request completion atomic wait
+ * @slow_timeout_ms: timeout for the request completion sleeping wait
+ *
+ * Send a PCODE request with @send_val as the request-ID or data as parameter.
+ * Wait @fast_timeout_us atomically then @slow_timeout_ms with sleeping wait
+ * for the request completion then if @reply_val is not NULL store the reply
+ * returned by PCODE in it.
+ *
+ * Returns
+ * - 0 on success
+ * - %-EAGAIN if PCODE is still busy with the previous request
+ * - %-ETIMEDOUT in case of a timeout
+ * - <0 in case of some other error as reported by PCODE
+ */
+int snb_pcode_request(struct drm_i915_private *dev_priv, u32 mbox,
+ u32 send_val, u32 *recv_val,
+ int fast_timeout_us, int slow_timeout_ms)
{
int status;
@@ -9133,26 +9155,27 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
* use te fw I915_READ variants to reduce the amount of work
* required when reading/writing.
*/
-
if (I915_READ_FW(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) {
- DRM_DEBUG_DRIVER("warning: pcode (read from mbox %x) mailbox access failed for %ps\n",
- mbox, __builtin_return_address(0));
+ DRM_DEBUG_DRIVER("warning: pcode not ready for request 0x%08x to mbox 0x%x for %ps\n",
+ send_val, mbox, __builtin_return_address(0));
return -EAGAIN;
}
- I915_WRITE_FW(GEN6_PCODE_DATA, *val);
+ I915_WRITE_FW(GEN6_PCODE_DATA, send_val);
I915_WRITE_FW(GEN6_PCODE_DATA1, 0);
I915_WRITE_FW(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
if (__intel_wait_for_register_fw(dev_priv,
GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
- 500, 0, NULL)) {
- DRM_ERROR("timeout waiting for pcode read (from mbox %x) to finish for %ps\n",
- mbox, __builtin_return_address(0));
+ fast_timeout_us, slow_timeout_ms,
+ NULL)) {
+ DRM_ERROR("timeout waiting for pcode request 0x%08x to mbox 0x%x to finish for %ps\n",
+ send_val, mbox, __builtin_return_address(0));
return -ETIMEDOUT;
}
- *val = I915_READ_FW(GEN6_PCODE_DATA);
+ if (recv_val)
+ *recv_val = I915_READ_FW(GEN6_PCODE_DATA);
I915_WRITE_FW(GEN6_PCODE_DATA, 0);
if (INTEL_GEN(dev_priv) > 6)
@@ -9160,59 +9183,11 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
else
status = gen6_check_mailbox_status(dev_priv);
- if (status) {
- DRM_DEBUG_DRIVER("warning: pcode (read from mbox %x) mailbox access failed for %ps: %d\n",
- mbox, __builtin_return_address(0), status);
- return status;
- }
+ if (status)
+ DRM_DEBUG_DRIVER("warning: pcode request %08x to mbox 0x%x failed for %ps: %d\n",
+ send_val, mbox, __builtin_return_address(0), status);
- return 0;
-}
-
-int snb_pcode_request(struct drm_i915_private *dev_priv,
- u32 mbox, u32 val, int timeout_us)
-{
- int status;
-
- WARN_ON(!mutex_is_locked(&dev_priv->pcu_lock));
-
- /* GEN6_PCODE_* are outside of the forcewake domain, we can
- * use te fw I915_READ variants to reduce the amount of work
- * required when reading/writing.
- */
-
- if (I915_READ_FW(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) {
- DRM_DEBUG_DRIVER("warning: pcode (write of 0x%08x to mbox %x) mailbox access failed for %ps\n",
- val, mbox, __builtin_return_address(0));
- return -EAGAIN;
- }
-
- I915_WRITE_FW(GEN6_PCODE_DATA, val);
- I915_WRITE_FW(GEN6_PCODE_DATA1, 0);
- I915_WRITE_FW(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
-
- if (__intel_wait_for_register_fw(dev_priv,
- GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
- timeout_us, 0, NULL)) {
- DRM_ERROR("timeout waiting for pcode write of 0x%08x to mbox %x to finish for %ps\n",
- val, mbox, __builtin_return_address(0));
- return -ETIMEDOUT;
- }
-
- I915_WRITE_FW(GEN6_PCODE_DATA, 0);
-
- if (INTEL_GEN(dev_priv) > 6)
- status = gen7_check_mailbox_status(dev_priv);
- else
- status = gen6_check_mailbox_status(dev_priv);
-
- if (status) {
- DRM_DEBUG_DRIVER("warning: pcode (write of 0x%08x to mbox %x) mailbox access failed for %ps: %d\n",
- val, mbox, __builtin_return_address(0), status);
- return status;
- }
-
- return 0;
+ return status;
}
static bool skl_pcode_try_request(struct drm_i915_private *dev_priv, u32 mbox,
--
2.13.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/3] drm/i915/bxt, glk: Avoid long atomic poll during CDCLK change
2018-01-30 11:47 [PATCH 1/3] drm/i915/bxt,glk: Increase PCODE timeouts during CDCLK freq changing Imre Deak
2018-01-30 11:47 ` [PATCH 2/3] drm/i915: Fold sandybridge_pcode_read() into snb_pcode_request() Imre Deak
@ 2018-01-30 11:47 ` Imre Deak
2018-01-30 12:00 ` [PATCH 1/3] drm/i915/bxt, glk: Increase PCODE timeouts during CDCLK freq changing Chris Wilson
` (3 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Imre Deak @ 2018-01-30 11:47 UTC (permalink / raw)
To: intel-gfx
There is no requirement for doing the PCODE request polling atomically,
so do that only for a short time switching to sleeping poll afterwards.
The specification requires a 150usec timeout for the change notification,
so let's use that for the atomic poll. Do the extra 2ms poll - needed as
a workaround on BXT/GLK - in sleeping mode.
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/intel_cdclk.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index 8d06a6f66f29..f94e14c8cd0a 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1377,7 +1377,7 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
*/
mutex_lock(&dev_priv->pcu_lock);
ret = snb_pcode_request(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
- 0x80000000, NULL, 2000, 0);
+ 0x80000000, NULL, 150, 2);
mutex_unlock(&dev_priv->pcu_lock);
if (ret) {
@@ -1415,7 +1415,7 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
* the next PCODE request based on BSpec.
*/
ret = snb_pcode_request(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
- cdclk_state->voltage_level, NULL, 2000, 0);
+ cdclk_state->voltage_level, NULL, 150, 2);
mutex_unlock(&dev_priv->pcu_lock);
if (ret) {
--
2.13.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] drm/i915: Fold sandybridge_pcode_read() into snb_pcode_request()
2018-01-30 11:47 ` [PATCH 2/3] drm/i915: Fold sandybridge_pcode_read() into snb_pcode_request() Imre Deak
@ 2018-01-30 11:57 ` Chris Wilson
2018-01-30 12:25 ` Imre Deak
0 siblings, 1 reply; 13+ messages in thread
From: Chris Wilson @ 2018-01-30 11:57 UTC (permalink / raw)
To: Imre Deak, intel-gfx
Quoting Imre Deak (2018-01-30 11:47:11)
> These two functions are very similar so simplify things by removing the
> duplication.
>
> Add a seperate sleeping poll timeout parameter, useful for longer polls
> like the CDCLK change on BXT/GLK. The next patch will take that into use.
>
> While at it document snb_pcode_request() and clean up a bit the
> error/debug prints. Other than that no functional changes.
In my patches to do the same (and move it to intel_sideband.c) I kept
the sandybridge_pcode_read/sandybridge_pcode_write functions to both
take the sb_lock and to provide imo clearer debug messages.
https://patchwork.freedesktop.org/series/36469/
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] drm/i915/bxt, glk: Increase PCODE timeouts during CDCLK freq changing
2018-01-30 11:47 [PATCH 1/3] drm/i915/bxt,glk: Increase PCODE timeouts during CDCLK freq changing Imre Deak
2018-01-30 11:47 ` [PATCH 2/3] drm/i915: Fold sandybridge_pcode_read() into snb_pcode_request() Imre Deak
2018-01-30 11:47 ` [PATCH 3/3] drm/i915/bxt, glk: Avoid long atomic poll during CDCLK change Imre Deak
@ 2018-01-30 12:00 ` Chris Wilson
2018-01-30 13:42 ` Ville Syrjälä
` (2 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Chris Wilson @ 2018-01-30 12:00 UTC (permalink / raw)
To: Imre Deak, intel-gfx; +Cc: Ville Syrjälä, stable, #, v4.4+
Quoting Imre Deak (2018-01-30 11:47:10)
> Currently we see sporadic timeouts during CDCLK changing both on BXT and
> GLK as reported by the Bugzilla: ticket. It's easy to reproduce this by
> changing the frequency in a tight loop after blanking the display. The
> upper bound for the completion time is 800us based on my tests, so
> increase it from the current 500us to 2ms; with that I couldn't trigger
> the problem either on BXT or GLK.
>
> Note that timeouts happened during both the change notification and the
> voltage level setting PCODE request. (For the latter one BSpec doesn't
> require us to wait for completion before further HW programming.)
>
> This issue is similar to
> 2c7d0602c815 ("drm/i915/gen9: Fix PCODE polling during CDCLK change
> notification")
> but there the PCODE request does complete (as shown by the mbox
> busy flag), only the reply we get from PCODE indicates a failure.
> So there we keep resending the request until a success reply, here we
> just have to increase the timeout for the one PCODE request we send.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: stable@vger.kernel.org # v4.4+
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103326
> Signed-off-by: Imre Deak <imre.deak@intel.com>
Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] drm/i915: Fold sandybridge_pcode_read() into snb_pcode_request()
2018-01-30 11:57 ` Chris Wilson
@ 2018-01-30 12:25 ` Imre Deak
2018-01-30 12:50 ` Chris Wilson
0 siblings, 1 reply; 13+ messages in thread
From: Imre Deak @ 2018-01-30 12:25 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Tue, Jan 30, 2018 at 11:57:49AM +0000, Chris Wilson wrote:
> Quoting Imre Deak (2018-01-30 11:47:11)
> > These two functions are very similar so simplify things by removing the
> > duplication.
> >
> > Add a seperate sleeping poll timeout parameter, useful for longer polls
> > like the CDCLK change on BXT/GLK. The next patch will take that into use.
> >
> > While at it document snb_pcode_request() and clean up a bit the
> > error/debug prints. Other than that no functional changes.
>
> In my patches to do the same (and move it to intel_sideband.c) I kept
> the sandybridge_pcode_read/sandybridge_pcode_write functions to both
> take the sb_lock and to provide imo clearer debug messages.
> https://patchwork.freedesktop.org/series/36469/
Ah didn't notice it, will drop mine. Does it make sense to pass the
fast/slow timeouts to sandybridge_pcode_read/write? Imo it'd document
things better and could avoid the long atomic poll on BXT/GLK.
--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] drm/i915: Fold sandybridge_pcode_read() into snb_pcode_request()
2018-01-30 12:25 ` Imre Deak
@ 2018-01-30 12:50 ` Chris Wilson
0 siblings, 0 replies; 13+ messages in thread
From: Chris Wilson @ 2018-01-30 12:50 UTC (permalink / raw)
To: imre.deak; +Cc: intel-gfx
Quoting Imre Deak (2018-01-30 12:25:39)
> On Tue, Jan 30, 2018 at 11:57:49AM +0000, Chris Wilson wrote:
> > Quoting Imre Deak (2018-01-30 11:47:11)
> > > These two functions are very similar so simplify things by removing the
> > > duplication.
> > >
> > > Add a seperate sleeping poll timeout parameter, useful for longer polls
> > > like the CDCLK change on BXT/GLK. The next patch will take that into use.
> > >
> > > While at it document snb_pcode_request() and clean up a bit the
> > > error/debug prints. Other than that no functional changes.
> >
> > In my patches to do the same (and move it to intel_sideband.c) I kept
> > the sandybridge_pcode_read/sandybridge_pcode_write functions to both
> > take the sb_lock and to provide imo clearer debug messages.
> > https://patchwork.freedesktop.org/series/36469/
>
> Ah didn't notice it, will drop mine. Does it make sense to pass the
> fast/slow timeouts to sandybridge_pcode_read/write? Imo it'd document
> things better and could avoid the long atomic poll on BXT/GLK.
It does. You've demonstrated a need, so just do it :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] drm/i915/bxt,glk: Increase PCODE timeouts during CDCLK freq changing
2018-01-30 11:47 [PATCH 1/3] drm/i915/bxt,glk: Increase PCODE timeouts during CDCLK freq changing Imre Deak
@ 2018-01-30 13:42 ` Ville Syrjälä
2018-01-30 11:47 ` [PATCH 3/3] drm/i915/bxt, glk: Avoid long atomic poll during CDCLK change Imre Deak
` (4 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Ville Syrjälä @ 2018-01-30 13:42 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx, Chris Wilson, stable, #, v4.4+
On Tue, Jan 30, 2018 at 01:47:10PM +0200, Imre Deak wrote:
> Currently we see sporadic timeouts during CDCLK changing both on BXT and
> GLK as reported by the Bugzilla: ticket. It's easy to reproduce this by
> changing the frequency in a tight loop after blanking the display. The
> upper bound for the completion time is 800us based on my tests, so
> increase it from the current 500us to 2ms; with that I couldn't trigger
> the problem either on BXT or GLK.
>
> Note that timeouts happened during both the change notification and the
> voltage level setting PCODE request. (For the latter one BSpec doesn't
> require us to wait for completion before further HW programming.)
>
> This issue is similar to
> 2c7d0602c815 ("drm/i915/gen9: Fix PCODE polling during CDCLK change
> notification")
> but there the PCODE request does complete (as shown by the mbox
> busy flag), only the reply we get from PCODE indicates a failure.
> So there we keep resending the request until a success reply, here we
> just have to increase the timeout for the one PCODE request we send.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> Cc: stable@vger.kernel.org # v4.4+
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103326
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 6 +++++-
> drivers/gpu/drm/i915/intel_cdclk.c | 20 +++++++++++++++-----
> drivers/gpu/drm/i915/intel_pm.c | 6 +++---
> 3 files changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 454d8f937fae..5e293be4e51d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3723,7 +3723,11 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e,
> struct intel_display_error_state *error);
>
> int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val);
> -int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val);
> +int snb_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 val,
> + int timeout_us);
> +#define sandybridge_pcode_write(dev_priv, mbox, val) \
> + snb_pcode_request(dev_priv, mbox, val, 500)
The naming feels a bit inconsistent. snb_pcode_request() is nothing
like skl_pcode_request(), rather it's just an improved
sandybridge_pcode_write().
> +
> int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request,
> u32 reply_mask, u32 reply, int timeout_base_ms);
>
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index c4392ea34a3d..5057336c40ba 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1370,10 +1370,14 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> break;
> }
>
> - /* Inform power controller of upcoming frequency change */
> + /*
> + * Inform power controller of upcoming frequency change. BSpec
> + * requires us to wait up to 150usec, but that leads to timeouts;
> + * the 2ms used here is based on experiment.
> + */
> mutex_lock(&dev_priv->pcu_lock);
> - ret = sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> - 0x80000000);
> + ret = snb_pcode_request(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> + 0x80000000, 2000);
> mutex_unlock(&dev_priv->pcu_lock);
>
> if (ret) {
> @@ -1404,8 +1408,14 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> I915_WRITE(CDCLK_CTL, val);
>
> mutex_lock(&dev_priv->pcu_lock);
> - ret = sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> - cdclk_state->voltage_level);
> + /*
> + * The timeout isn't specified, the 2ms used here is based on
> + * experiment.
> + * FIXME: Waiting for the request completion could be delayed until
> + * the next PCODE request based on BSpec.
> + */
> + ret = snb_pcode_request(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> + cdclk_state->voltage_level, 2000);
> mutex_unlock(&dev_priv->pcu_lock);
>
> if (ret) {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 0b92ea1dbd40..f6f4dbacb9af 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -9169,8 +9169,8 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
> return 0;
> }
>
> -int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
> - u32 mbox, u32 val)
> +int snb_pcode_request(struct drm_i915_private *dev_priv,
> + u32 mbox, u32 val, int timeout_us)
> {
> int status;
>
> @@ -9193,7 +9193,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
>
> if (__intel_wait_for_register_fw(dev_priv,
> GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
> - 500, 0, NULL)) {
> + timeout_us, 0, NULL)) {
> DRM_ERROR("timeout waiting for pcode write of 0x%08x to mbox %x to finish for %ps\n",
> val, mbox, __builtin_return_address(0));
> return -ETIMEDOUT;
> --
> 2.13.2
--
Ville Syrj�l�
Intel OTC
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] drm/i915/bxt,glk: Increase PCODE timeouts during CDCLK freq changing
@ 2018-01-30 13:42 ` Ville Syrjälä
0 siblings, 0 replies; 13+ messages in thread
From: Ville Syrjälä @ 2018-01-30 13:42 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx, Chris Wilson, stable, #, v4.4+
On Tue, Jan 30, 2018 at 01:47:10PM +0200, Imre Deak wrote:
> Currently we see sporadic timeouts during CDCLK changing both on BXT and
> GLK as reported by the Bugzilla: ticket. It's easy to reproduce this by
> changing the frequency in a tight loop after blanking the display. The
> upper bound for the completion time is 800us based on my tests, so
> increase it from the current 500us to 2ms; with that I couldn't trigger
> the problem either on BXT or GLK.
>
> Note that timeouts happened during both the change notification and the
> voltage level setting PCODE request. (For the latter one BSpec doesn't
> require us to wait for completion before further HW programming.)
>
> This issue is similar to
> 2c7d0602c815 ("drm/i915/gen9: Fix PCODE polling during CDCLK change
> notification")
> but there the PCODE request does complete (as shown by the mbox
> busy flag), only the reply we get from PCODE indicates a failure.
> So there we keep resending the request until a success reply, here we
> just have to increase the timeout for the one PCODE request we send.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: stable@vger.kernel.org # v4.4+
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103326
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 6 +++++-
> drivers/gpu/drm/i915/intel_cdclk.c | 20 +++++++++++++++-----
> drivers/gpu/drm/i915/intel_pm.c | 6 +++---
> 3 files changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 454d8f937fae..5e293be4e51d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3723,7 +3723,11 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e,
> struct intel_display_error_state *error);
>
> int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val);
> -int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val);
> +int snb_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 val,
> + int timeout_us);
> +#define sandybridge_pcode_write(dev_priv, mbox, val) \
> + snb_pcode_request(dev_priv, mbox, val, 500)
The naming feels a bit inconsistent. snb_pcode_request() is nothing
like skl_pcode_request(), rather it's just an improved
sandybridge_pcode_write().
> +
> int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request,
> u32 reply_mask, u32 reply, int timeout_base_ms);
>
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index c4392ea34a3d..5057336c40ba 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1370,10 +1370,14 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> break;
> }
>
> - /* Inform power controller of upcoming frequency change */
> + /*
> + * Inform power controller of upcoming frequency change. BSpec
> + * requires us to wait up to 150usec, but that leads to timeouts;
> + * the 2ms used here is based on experiment.
> + */
> mutex_lock(&dev_priv->pcu_lock);
> - ret = sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> - 0x80000000);
> + ret = snb_pcode_request(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> + 0x80000000, 2000);
> mutex_unlock(&dev_priv->pcu_lock);
>
> if (ret) {
> @@ -1404,8 +1408,14 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> I915_WRITE(CDCLK_CTL, val);
>
> mutex_lock(&dev_priv->pcu_lock);
> - ret = sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> - cdclk_state->voltage_level);
> + /*
> + * The timeout isn't specified, the 2ms used here is based on
> + * experiment.
> + * FIXME: Waiting for the request completion could be delayed until
> + * the next PCODE request based on BSpec.
> + */
> + ret = snb_pcode_request(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> + cdclk_state->voltage_level, 2000);
> mutex_unlock(&dev_priv->pcu_lock);
>
> if (ret) {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 0b92ea1dbd40..f6f4dbacb9af 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -9169,8 +9169,8 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
> return 0;
> }
>
> -int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
> - u32 mbox, u32 val)
> +int snb_pcode_request(struct drm_i915_private *dev_priv,
> + u32 mbox, u32 val, int timeout_us)
> {
> int status;
>
> @@ -9193,7 +9193,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
>
> if (__intel_wait_for_register_fw(dev_priv,
> GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
> - 500, 0, NULL)) {
> + timeout_us, 0, NULL)) {
> DRM_ERROR("timeout waiting for pcode write of 0x%08x to mbox %x to finish for %ps\n",
> val, mbox, __builtin_return_address(0));
> return -ETIMEDOUT;
> --
> 2.13.2
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] drm/i915/bxt,glk: Increase PCODE timeouts during CDCLK freq changing
2018-01-30 13:42 ` Ville Syrjälä
@ 2018-01-30 14:17 ` Imre Deak
-1 siblings, 0 replies; 13+ messages in thread
From: Imre Deak @ 2018-01-30 14:17 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, Chris Wilson, stable, #, v4.4+
On Tue, Jan 30, 2018 at 03:42:45PM +0200, Ville Syrj�l� wrote:
> On Tue, Jan 30, 2018 at 01:47:10PM +0200, Imre Deak wrote:
> > Currently we see sporadic timeouts during CDCLK changing both on BXT and
> > GLK as reported by the Bugzilla: ticket. It's easy to reproduce this by
> > changing the frequency in a tight loop after blanking the display. The
> > upper bound for the completion time is 800us based on my tests, so
> > increase it from the current 500us to 2ms; with that I couldn't trigger
> > the problem either on BXT or GLK.
> >
> > Note that timeouts happened during both the change notification and the
> > voltage level setting PCODE request. (For the latter one BSpec doesn't
> > require us to wait for completion before further HW programming.)
> >
> > This issue is similar to
> > 2c7d0602c815 ("drm/i915/gen9: Fix PCODE polling during CDCLK change
> > notification")
> > but there the PCODE request does complete (as shown by the mbox
> > busy flag), only the reply we get from PCODE indicates a failure.
> > So there we keep resending the request until a success reply, here we
> > just have to increase the timeout for the one PCODE request we send.
> >
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > Cc: stable@vger.kernel.org # v4.4+
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103326
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 6 +++++-
> > drivers/gpu/drm/i915/intel_cdclk.c | 20 +++++++++++++++-----
> > drivers/gpu/drm/i915/intel_pm.c | 6 +++---
> > 3 files changed, 23 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 454d8f937fae..5e293be4e51d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3723,7 +3723,11 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e,
> > struct intel_display_error_state *error);
> >
> > int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val);
> > -int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val);
> > +int snb_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 val,
> > + int timeout_us);
> > +#define sandybridge_pcode_write(dev_priv, mbox, val) \
> > + snb_pcode_request(dev_priv, mbox, val, 500)
>
> The naming feels a bit inconsistent. snb_pcode_request() is nothing
> like skl_pcode_request(), rather it's just an improved
> sandybridge_pcode_write().
The idea was to keep in then end (in drm-tip) only two pcode helpers
snb_pcode_request() and skl_pcode_request(). But yes, they are different
so probably the name should reflect this. I'll use
sandybridge_pcode_write_timeout().
>
> > +
> > int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request,
> > u32 reply_mask, u32 reply, int timeout_base_ms);
> >
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > index c4392ea34a3d..5057336c40ba 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -1370,10 +1370,14 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > break;
> > }
> >
> > - /* Inform power controller of upcoming frequency change */
> > + /*
> > + * Inform power controller of upcoming frequency change. BSpec
> > + * requires us to wait up to 150usec, but that leads to timeouts;
> > + * the 2ms used here is based on experiment.
> > + */
> > mutex_lock(&dev_priv->pcu_lock);
> > - ret = sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> > - 0x80000000);
> > + ret = snb_pcode_request(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> > + 0x80000000, 2000);
> > mutex_unlock(&dev_priv->pcu_lock);
> >
> > if (ret) {
> > @@ -1404,8 +1408,14 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > I915_WRITE(CDCLK_CTL, val);
> >
> > mutex_lock(&dev_priv->pcu_lock);
> > - ret = sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> > - cdclk_state->voltage_level);
> > + /*
> > + * The timeout isn't specified, the 2ms used here is based on
> > + * experiment.
> > + * FIXME: Waiting for the request completion could be delayed until
> > + * the next PCODE request based on BSpec.
> > + */
> > + ret = snb_pcode_request(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> > + cdclk_state->voltage_level, 2000);
> > mutex_unlock(&dev_priv->pcu_lock);
> >
> > if (ret) {
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 0b92ea1dbd40..f6f4dbacb9af 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -9169,8 +9169,8 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
> > return 0;
> > }
> >
> > -int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
> > - u32 mbox, u32 val)
> > +int snb_pcode_request(struct drm_i915_private *dev_priv,
> > + u32 mbox, u32 val, int timeout_us)
> > {
> > int status;
> >
> > @@ -9193,7 +9193,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
> >
> > if (__intel_wait_for_register_fw(dev_priv,
> > GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
> > - 500, 0, NULL)) {
> > + timeout_us, 0, NULL)) {
> > DRM_ERROR("timeout waiting for pcode write of 0x%08x to mbox %x to finish for %ps\n",
> > val, mbox, __builtin_return_address(0));
> > return -ETIMEDOUT;
> > --
> > 2.13.2
>
> --
> Ville Syrj�l�
> Intel OTC
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] drm/i915/bxt, glk: Increase PCODE timeouts during CDCLK freq changing
@ 2018-01-30 14:17 ` Imre Deak
0 siblings, 0 replies; 13+ messages in thread
From: Imre Deak @ 2018-01-30 14:17 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: v4.4+, intel-gfx, #, stable
On Tue, Jan 30, 2018 at 03:42:45PM +0200, Ville Syrjälä wrote:
> On Tue, Jan 30, 2018 at 01:47:10PM +0200, Imre Deak wrote:
> > Currently we see sporadic timeouts during CDCLK changing both on BXT and
> > GLK as reported by the Bugzilla: ticket. It's easy to reproduce this by
> > changing the frequency in a tight loop after blanking the display. The
> > upper bound for the completion time is 800us based on my tests, so
> > increase it from the current 500us to 2ms; with that I couldn't trigger
> > the problem either on BXT or GLK.
> >
> > Note that timeouts happened during both the change notification and the
> > voltage level setting PCODE request. (For the latter one BSpec doesn't
> > require us to wait for completion before further HW programming.)
> >
> > This issue is similar to
> > 2c7d0602c815 ("drm/i915/gen9: Fix PCODE polling during CDCLK change
> > notification")
> > but there the PCODE request does complete (as shown by the mbox
> > busy flag), only the reply we get from PCODE indicates a failure.
> > So there we keep resending the request until a success reply, here we
> > just have to increase the timeout for the one PCODE request we send.
> >
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: stable@vger.kernel.org # v4.4+
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103326
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 6 +++++-
> > drivers/gpu/drm/i915/intel_cdclk.c | 20 +++++++++++++++-----
> > drivers/gpu/drm/i915/intel_pm.c | 6 +++---
> > 3 files changed, 23 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 454d8f937fae..5e293be4e51d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3723,7 +3723,11 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e,
> > struct intel_display_error_state *error);
> >
> > int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val);
> > -int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val);
> > +int snb_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 val,
> > + int timeout_us);
> > +#define sandybridge_pcode_write(dev_priv, mbox, val) \
> > + snb_pcode_request(dev_priv, mbox, val, 500)
>
> The naming feels a bit inconsistent. snb_pcode_request() is nothing
> like skl_pcode_request(), rather it's just an improved
> sandybridge_pcode_write().
The idea was to keep in then end (in drm-tip) only two pcode helpers
snb_pcode_request() and skl_pcode_request(). But yes, they are different
so probably the name should reflect this. I'll use
sandybridge_pcode_write_timeout().
>
> > +
> > int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request,
> > u32 reply_mask, u32 reply, int timeout_base_ms);
> >
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > index c4392ea34a3d..5057336c40ba 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -1370,10 +1370,14 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > break;
> > }
> >
> > - /* Inform power controller of upcoming frequency change */
> > + /*
> > + * Inform power controller of upcoming frequency change. BSpec
> > + * requires us to wait up to 150usec, but that leads to timeouts;
> > + * the 2ms used here is based on experiment.
> > + */
> > mutex_lock(&dev_priv->pcu_lock);
> > - ret = sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> > - 0x80000000);
> > + ret = snb_pcode_request(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> > + 0x80000000, 2000);
> > mutex_unlock(&dev_priv->pcu_lock);
> >
> > if (ret) {
> > @@ -1404,8 +1408,14 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > I915_WRITE(CDCLK_CTL, val);
> >
> > mutex_lock(&dev_priv->pcu_lock);
> > - ret = sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> > - cdclk_state->voltage_level);
> > + /*
> > + * The timeout isn't specified, the 2ms used here is based on
> > + * experiment.
> > + * FIXME: Waiting for the request completion could be delayed until
> > + * the next PCODE request based on BSpec.
> > + */
> > + ret = snb_pcode_request(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> > + cdclk_state->voltage_level, 2000);
> > mutex_unlock(&dev_priv->pcu_lock);
> >
> > if (ret) {
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 0b92ea1dbd40..f6f4dbacb9af 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -9169,8 +9169,8 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
> > return 0;
> > }
> >
> > -int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
> > - u32 mbox, u32 val)
> > +int snb_pcode_request(struct drm_i915_private *dev_priv,
> > + u32 mbox, u32 val, int timeout_us)
> > {
> > int status;
> >
> > @@ -9193,7 +9193,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
> >
> > if (__intel_wait_for_register_fw(dev_priv,
> > GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
> > - 500, 0, NULL)) {
> > + timeout_us, 0, NULL)) {
> > DRM_ERROR("timeout waiting for pcode write of 0x%08x to mbox %x to finish for %ps\n",
> > val, mbox, __builtin_return_address(0));
> > return -ETIMEDOUT;
> > --
> > 2.13.2
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/bxt, glk: Increase PCODE timeouts during CDCLK freq changing
2018-01-30 11:47 [PATCH 1/3] drm/i915/bxt,glk: Increase PCODE timeouts during CDCLK freq changing Imre Deak
` (3 preceding siblings ...)
2018-01-30 13:42 ` Ville Syrjälä
@ 2018-01-30 17:06 ` Patchwork
2018-01-30 19:57 ` ✓ Fi.CI.IGT: " Patchwork
5 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2018-01-30 17:06 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/3] drm/i915/bxt, glk: Increase PCODE timeouts during CDCLK freq changing
URL : https://patchwork.freedesktop.org/series/37338/
State : success
== Summary ==
Series 37338v1 series starting with [1/3] drm/i915/bxt, glk: Increase PCODE timeouts during CDCLK freq changing
https://patchwork.freedesktop.org/api/1.0/series/37338/revisions/1/mbox/
Test debugfs_test:
Subgroup read_all_entries:
pass -> INCOMPLETE (fi-snb-2520m) fdo#103713
Test gem_mmap_gtt:
Subgroup basic-small-bo-tiledx:
fail -> PASS (fi-gdg-551) fdo#102575
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:421s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:427s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:373s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:485s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:281s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:482s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:488s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:465s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:457s
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:571s
fi-elk-e7500 total:224 pass:168 dwarn:10 dfail:0 fail:0 skip:45
fi-gdg-551 total:288 pass:180 dwarn:0 dfail:0 fail:0 skip:108 time:279s
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:508s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:391s
fi-hsw-4770r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:405s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:410s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:461s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:415s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:457s
fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:499s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:456s
fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:508s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:428s
fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:513s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:531s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:484s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:476s
fi-skl-guc total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:417s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:429s
fi-snb-2520m total:3 pass:2 dwarn:0 dfail:0 fail:0 skip:0
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:402s
Blacklisted hosts:
fi-glk-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:471s
040c6384a6d2a3c5fbb578824775e940a73c6021 drm-tip: 2018y-01m-30d-16h-00m-34s UTC integration manifest
c41894128b7c drm/i915/bxt, glk: Avoid long atomic poll during CDCLK change
8b290e06c92b drm/i915: Fold sandybridge_pcode_read() into snb_pcode_request()
f5f7c68d73a4 drm/i915/bxt, glk: Increase PCODE timeouts during CDCLK freq changing
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7821/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
* ✓ Fi.CI.IGT: success for series starting with [1/3] drm/i915/bxt, glk: Increase PCODE timeouts during CDCLK freq changing
2018-01-30 11:47 [PATCH 1/3] drm/i915/bxt,glk: Increase PCODE timeouts during CDCLK freq changing Imre Deak
` (4 preceding siblings ...)
2018-01-30 17:06 ` ✓ Fi.CI.BAT: success for series starting with [1/3] " Patchwork
@ 2018-01-30 19:57 ` Patchwork
5 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2018-01-30 19:57 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/3] drm/i915/bxt, glk: Increase PCODE timeouts during CDCLK freq changing
URL : https://patchwork.freedesktop.org/series/37338/
State : success
== Summary ==
Warning: bzip CI_DRM_3701/shard-glkb6/results1.json.bz2 wasn't in correct JSON format
Test gem_eio:
Subgroup in-flight-contexts:
dmesg-warn -> PASS (shard-snb) fdo#104058
Test perf_pmu:
Subgroup frequency:
pass -> FAIL (shard-apl) fdo#104829
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-a:
skip -> PASS (shard-snb) fdo#103375 +1
fdo#104058 https://bugs.freedesktop.org/show_bug.cgi?id=104058
fdo#104829 https://bugs.freedesktop.org/show_bug.cgi?id=104829
fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
shard-apl total:2838 pass:1750 dwarn:1 dfail:0 fail:23 skip:1064 time:12589s
shard-hsw total:2825 pass:1726 dwarn:1 dfail:0 fail:12 skip:1084 time:11674s
shard-snb total:2838 pass:1328 dwarn:1 dfail:0 fail:12 skip:1497 time:6666s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7821/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-01-30 19:57 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-30 11:47 [PATCH 1/3] drm/i915/bxt,glk: Increase PCODE timeouts during CDCLK freq changing Imre Deak
2018-01-30 11:47 ` [PATCH 2/3] drm/i915: Fold sandybridge_pcode_read() into snb_pcode_request() Imre Deak
2018-01-30 11:57 ` Chris Wilson
2018-01-30 12:25 ` Imre Deak
2018-01-30 12:50 ` Chris Wilson
2018-01-30 11:47 ` [PATCH 3/3] drm/i915/bxt, glk: Avoid long atomic poll during CDCLK change Imre Deak
2018-01-30 12:00 ` [PATCH 1/3] drm/i915/bxt, glk: Increase PCODE timeouts during CDCLK freq changing Chris Wilson
2018-01-30 13:42 ` [PATCH 1/3] drm/i915/bxt,glk: " Ville Syrjälä
2018-01-30 13:42 ` Ville Syrjälä
2018-01-30 14:17 ` Imre Deak
2018-01-30 14:17 ` [PATCH 1/3] drm/i915/bxt, glk: " Imre Deak
2018-01-30 17:06 ` ✓ Fi.CI.BAT: success for series starting with [1/3] " Patchwork
2018-01-30 19:57 ` ✓ Fi.CI.IGT: " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.