On Mon, Nov 28, 2016 at 01:12:57PM +0200, Imre Deak wrote: > commit 848496e5902833600f7992f4faa82dc1546051ba > Author: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > Date: Wed Jul 13 16:32:03 2016 +0300 > > drm/i915: Wait up to 3ms for the pcu to ack the cdclk change request on SKL > > increased the timeout to match the spec, but we still see a timeout on > at least one SKL. A CDCLK change request following the failed one will > succeed nevertheless. > > I could reproduce this problem easily by running kms_pipe_crc_basic in a > loop. In all failure cases _wait_for() was pre-empted for >3ms and so in > the worst case - when the pre-emption happened right after calculating > timeout__ in _wait_for() - we called skl_cdclk_wait_for_pcu_ready() only > once which failed and so _wait_for() timed out. As opposed to this the > spec says to keep retrying the request for at most a 3ms period. > > To fix this disable pre-emption to maximize the number of times we retry > the request. Also increase the timeout to 10ms to account for interrupts > that could reduce the number of these attempts. With this change I > couldn't trigger the problem. > > v2: > - Use 1ms poll period instead of 10us. (Chris) > v3: > - Poll with pre-emption disabled to increase the number of request > attempts. (Ville, Chris) > - Factor out a helper to poll, it's also needed by the next patch. > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > Reference: https://bugs.freedesktop.org/show_bug.cgi?id=97929 > Testcase: igt/kms_pipe_crc_basic/suspend-read-crc-pipe-B > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_reg.h | 2 +- > drivers/gpu/drm/i915/intel_display.c | 29 +++++++----------------- > drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++++++++++++++++ > 4 files changed, 53 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 01f5067..f618807 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3593,6 +3593,7 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e, > > 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 skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request); > > /* intel_sideband.c */ > u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr); > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 6747d68..f542cbc 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -7424,7 +7424,6 @@ enum { > #define GEN9_MEM_LATENCY_LEVEL_3_7_SHIFT 24 > #define SKL_PCODE_CDCLK_CONTROL 0x7 > #define SKL_CDCLK_PREPARE_FOR_CHANGE 0x3 > -#define SKL_CDCLK_READY_FOR_CHANGE 0x1 > #define GEN6_PCODE_WRITE_MIN_FREQ_TABLE 0x8 > #define GEN6_PCODE_READ_MIN_FREQ_TABLE 0x9 > #define GEN6_READ_OC_PARAMS 0xc > @@ -7437,6 +7436,7 @@ enum { > #define GEN9_SAGV_DISABLE 0x0 > #define GEN9_SAGV_IS_DISABLED 0x1 > #define GEN9_SAGV_ENABLE 0x3 > +#define GEN9_PCODE_REQUEST_DONE 0x1 Is that really a generic thing? > #define GEN6_PCODE_DATA _MMIO(0x138128) > #define GEN6_PCODE_FREQ_IA_RATIO_SHIFT 8 > #define GEN6_PCODE_FREQ_RING_RATIO_SHIFT 16 > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 5d11002..46c0e42 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6245,35 +6245,22 @@ skl_dpll0_disable(struct drm_i915_private *dev_priv) > dev_priv->cdclk_pll.vco = 0; > } > > -static bool skl_cdclk_pcu_ready(struct drm_i915_private *dev_priv) > -{ > - int ret; > - u32 val; > - > - /* inform PCU we want to change CDCLK */ > - val = SKL_CDCLK_PREPARE_FOR_CHANGE; > - mutex_lock(&dev_priv->rps.hw_lock); > - ret = sandybridge_pcode_read(dev_priv, SKL_PCODE_CDCLK_CONTROL, &val); > - mutex_unlock(&dev_priv->rps.hw_lock); > - > - return ret == 0 && (val & SKL_CDCLK_READY_FOR_CHANGE); > -} > - > -static bool skl_cdclk_wait_for_pcu_ready(struct drm_i915_private *dev_priv) > -{ > - return _wait_for(skl_cdclk_pcu_ready(dev_priv), 3000, 10) == 0; > -} > - > static void skl_set_cdclk(struct drm_i915_private *dev_priv, int cdclk, int vco) > { > u32 freq_select, pcu_ack; > + int ret; > > WARN_ON((cdclk == 24000) != (vco == 0)); > > DRM_DEBUG_DRIVER("Changing CDCLK to %d kHz (VCO %d kHz)\n", cdclk, vco); > > - if (!skl_cdclk_wait_for_pcu_ready(dev_priv)) { > - DRM_ERROR("failed to inform PCU about cdclk change\n"); > + mutex_lock(&dev_priv->rps.hw_lock); > + ret = skl_pcode_request(dev_priv, SKL_PCODE_CDCLK_CONTROL, > + SKL_CDCLK_PREPARE_FOR_CHANGE); > + mutex_unlock(&dev_priv->rps.hw_lock); > + if (ret) { > + DRM_ERROR("Failed to inform PCU about cdclk change (%d)\n", > + ret); > return; > } > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 66c62f3..edd68f3 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -7864,6 +7864,49 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, > return 0; > } > > +static bool skl_pcode_try_request(struct drm_i915_private *dev_priv, u32 mbox, > + u32 request, u32 *status) > +{ > + u32 val = request; > + > + *status = sandybridge_pcode_read(dev_priv, mbox, &val); > + > + return *status || (val & GEN9_PCODE_REQUEST_DONE); > +} > + > +/** > + * skl_pcode_request - send PCODE request until acknowledgment > + * @dev_priv: device private > + * @mbox: PCODE mailbox ID the request is targeted for > + * @request: request ID > + * > + * Keep resending the @request to @mbox until PCODE acknowledges it, or a 10ms > + * timeout expires. > + * > + * Returns 0 on success, %-ETIMEDOUT in case of a timeout, <0 in case of some > + * other error as reported by PCODE. > + */ > +int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request) > +{ > + u32 status; > + int ret; > + > + WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock)); > + > + /* > + * The spec says to keep retrying the request for at most 3ms until > + * acknowledgement, so disable pre-emption to maximize the number of > + * attempts within this duration. Use a 10ms overall timeout to > + * account for interrupts that could reduce the number of attempts. > + */ > + preempt_disable(); > + ret = wait_for_atomic(skl_pcode_try_request(dev_priv, mbox, request, > + &status), 10); > + preempt_enable(); > + > + return ret ? ret : status; > +} > + > static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val) > { > /* > -- > 2.5.0 -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
