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@xxxxxxxxxxxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx # v4.4+
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103326
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > ---
> > 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