[PATCH v4 1/3] drm/i915/gen6+: Clear upper data byte during PCODE write

[PATCH v4 1/3] drm/i915/gen6+: Clear upper data byte during PCODE write

[Imre Deak]

The spec calls for the upper data byte to be cleared before most of the
PCODE write commands, for others like IPS control it doesn't say
anything about this byte. Let's clear it in case it's clobbered somehow,
especially that there are places where we only do a PCODE write without
a preceeding PCODE read.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 29b6653..66c62f3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7838,6 +7838,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
 	}
 
 	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,
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 2/3] drm/i915/gen9: Fix PCODE polling during CDCLK change notification

[Imre Deak]

commit 848496e5902833600f7992f4faa82dc1546051ba
Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
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.
v4:
- Pass reply_mask, reply to skl_pcode_request(), instead of assuming the
  reply is generic. (Ville)

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
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@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 ++
 drivers/gpu/drm/i915/intel_display.c | 31 ++++++++---------------
 drivers/gpu/drm/i915/intel_pm.c      | 49 ++++++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 01f5067..1be5bab 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3593,6 +3593,8 @@ 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,
+		      u32 reply_mask, u32 reply);
 
 /* intel_sideband.c */
 u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5d11002..3d220da 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6245,35 +6245,24 @@ 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,
+				SKL_CDCLK_READY_FOR_CHANGE,
+				SKL_CDCLK_READY_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..aed88e0 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7864,6 +7864,55 @@ 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 reply_mask, u32 reply,
+				  u32 *status)
+{
+	u32 val = request;
+
+	*status = sandybridge_pcode_read(dev_priv, mbox, &val);
+
+	return *status || ((val & reply_mask) == reply);
+}
+
+/**
+ * skl_pcode_request - send PCODE request until acknowledgment
+ * @dev_priv: device private
+ * @mbox: PCODE mailbox ID the request is targeted for
+ * @request: request ID
+ * @reply_mask: mask used to check for request acknowledgment
+ * @reply: value used to check for request acknowledgement
+ *
+ * Keep resending the @request to @mbox until PCODE acknowledges it, PCODE
+ * reports an error or a 10ms timeout expires. The request is acknowledged
+ * once the PCODE reply dword equals @reply after aplying @reply_mask.
+ *
+ * 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 reply_mask, u32 reply)
+{
+	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,
+						    reply_mask, reply, &status),
+			      10);
+	preempt_enable();
+
+	return ret ? ret : status;
+}
+
 static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
 {
 	/*
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 2/3] drm/i915/gen9: Fix PCODE polling during CDCLK change notification

[Ville Syrjälä]

On Mon, Nov 28, 2016 at 05:29:28PM +0200, Imre Deak wrote:
> commit 848496e5902833600f7992f4faa82dc1546051ba
> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 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.
> v4:
> - Pass reply_mask, reply to skl_pcode_request(), instead of assuming the
>   reply is generic. (Ville)
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> 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@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  2 ++
>  drivers/gpu/drm/i915/intel_display.c | 31 ++++++++---------------
>  drivers/gpu/drm/i915/intel_pm.c      | 49 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 61 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 01f5067..1be5bab 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3593,6 +3593,8 @@ 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,
> +		      u32 reply_mask, u32 reply);
>  
>  /* intel_sideband.c */
>  u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5d11002..3d220da 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6245,35 +6245,24 @@ 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,
> +				SKL_CDCLK_READY_FOR_CHANGE,
> +				SKL_CDCLK_READY_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..aed88e0 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7864,6 +7864,55 @@ 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 reply_mask, u32 reply,
> +				  u32 *status)
> +{
> +	u32 val = request;
> +
> +	*status = sandybridge_pcode_read(dev_priv, mbox, &val);
> +
> +	return *status || ((val & reply_mask) == reply);
> +}
> +
> +/**
> + * skl_pcode_request - send PCODE request until acknowledgment
> + * @dev_priv: device private
> + * @mbox: PCODE mailbox ID the request is targeted for
> + * @request: request ID
> + * @reply_mask: mask used to check for request acknowledgment
> + * @reply: value used to check for request acknowledgement
> + *
> + * Keep resending the @request to @mbox until PCODE acknowledges it, PCODE
> + * reports an error or a 10ms timeout expires. The request is acknowledged
> + * once the PCODE reply dword equals @reply after aplying @reply_mask.
> + *
> + * 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 reply_mask, u32 reply)
> +{
> +	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

This comment seems a little misplaced. The 3ms is specified for the
cdclk prep request, SAGV will have a different value.

If we don't want callers to pass in the timeout as well, I guess we'll
at least want to reword this comment to say which callers need what
kind of timeouts. Though I kinda like the idea of callers passing that
in as well.

> +	 * 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,
> +						    reply_mask, reply, &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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 2/3] drm/i915/gen9: Fix PCODE polling during CDCLK change notification

[Imre Deak]

On ma, 2016-11-28 at 17:58 +0200, Ville Syrjälä wrote:
> On Mon, Nov 28, 2016 at 05:29:28PM +0200, Imre Deak wrote:
> > commit 848496e5902833600f7992f4faa82dc1546051ba
> > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 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.
> > v4:
> > - Pass reply_mask, reply to skl_pcode_request(), instead of assuming the
> >   reply is generic. (Ville)
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > 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@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  2 ++
> >  drivers/gpu/drm/i915/intel_display.c | 31 ++++++++---------------
> >  drivers/gpu/drm/i915/intel_pm.c      | 49 ++++++++++++++++++++++++++++++++++++
> >  3 files changed, 61 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 01f5067..1be5bab 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3593,6 +3593,8 @@ 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,
> > +		      u32 reply_mask, u32 reply);
> >  
> >  /* intel_sideband.c */
> >  u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr);
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 5d11002..3d220da 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6245,35 +6245,24 @@ 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,
> > +				SKL_CDCLK_READY_FOR_CHANGE,
> > +				SKL_CDCLK_READY_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..aed88e0 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -7864,6 +7864,55 @@ 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 reply_mask, u32 reply,
> > +				  u32 *status)
> > +{
> > +	u32 val = request;
> > +
> > +	*status = sandybridge_pcode_read(dev_priv, mbox, &val);
> > +
> > +	return *status || ((val & reply_mask) == reply);
> > +}
> > +
> > +/**
> > + * skl_pcode_request - send PCODE request until acknowledgment
> > + * @dev_priv: device private
> > + * @mbox: PCODE mailbox ID the request is targeted for
> > + * @request: request ID
> > + * @reply_mask: mask used to check for request acknowledgment
> > + * @reply: value used to check for request acknowledgement
> > + *
> > + * Keep resending the @request to @mbox until PCODE acknowledges it, PCODE
> > + * reports an error or a 10ms timeout expires. The request is acknowledged
> > + * once the PCODE reply dword equals @reply after aplying @reply_mask.
> > + *
> > + * 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 reply_mask, u32 reply)
> > +{
> > +	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
> 
> This comment seems a little misplaced. The 3ms is specified for the
> cdclk prep request, SAGV will have a different value.
> If we don't want callers to pass in the timeout as well, I guess we'll
> at least want to reword this comment to say which callers need what
> kind of timeouts.

How about "at most 3ms - which can be less depending on the given
request"?

> Though I kinda like the idea of callers passing that
> in as well.

The 10ms timeout I chose here is anyway an upper bound, considering
that interrupts could reduce the number of requests we can send. Hence
thought to make it a generic value.

> 
> > +	 * 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,
> > +						    reply_mask, reply, &status),
> > +			      10);
> > +	preempt_enable();
> > +
> > +	return ret ? ret : status;
> > +}
> > +
> >  static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
> >  {
> >  	/*
> > -- 
> > 2.5.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 2/3] drm/i915/gen9: Fix PCODE polling during CDCLK change notification

[Ville Syrjälä]

On Mon, Nov 28, 2016 at 06:12:51PM +0200, Imre Deak wrote:
> On ma, 2016-11-28 at 17:58 +0200, Ville Syrjälä wrote:
> > On Mon, Nov 28, 2016 at 05:29:28PM +0200, Imre Deak wrote:
> > > commit 848496e5902833600f7992f4faa82dc1546051ba
> > > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 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.
> > > v4:
> > > - Pass reply_mask, reply to skl_pcode_request(), instead of assuming the
> > >   reply is generic. (Ville)
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > 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@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h      |  2 ++
> > >  drivers/gpu/drm/i915/intel_display.c | 31 ++++++++---------------
> > >  drivers/gpu/drm/i915/intel_pm.c      | 49 ++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 61 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 01f5067..1be5bab 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -3593,6 +3593,8 @@ 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,
> > > +		      u32 reply_mask, u32 reply);
> > >  
> > >  /* intel_sideband.c */
> > >  u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr);
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 5d11002..3d220da 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -6245,35 +6245,24 @@ 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,
> > > +				SKL_CDCLK_READY_FOR_CHANGE,
> > > +				SKL_CDCLK_READY_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..aed88e0 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -7864,6 +7864,55 @@ 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 reply_mask, u32 reply,
> > > +				  u32 *status)
> > > +{
> > > +	u32 val = request;
> > > +
> > > +	*status = sandybridge_pcode_read(dev_priv, mbox, &val);
> > > +
> > > +	return *status || ((val & reply_mask) == reply);
> > > +}
> > > +
> > > +/**
> > > + * skl_pcode_request - send PCODE request until acknowledgment
> > > + * @dev_priv: device private
> > > + * @mbox: PCODE mailbox ID the request is targeted for
> > > + * @request: request ID
> > > + * @reply_mask: mask used to check for request acknowledgment
> > > + * @reply: value used to check for request acknowledgement
> > > + *
> > > + * Keep resending the @request to @mbox until PCODE acknowledges it, PCODE
> > > + * reports an error or a 10ms timeout expires. The request is acknowledged
> > > + * once the PCODE reply dword equals @reply after aplying @reply_mask.
> > > + *
> > > + * 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 reply_mask, u32 reply)
> > > +{
> > > +	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
> > 
> > This comment seems a little misplaced. The 3ms is specified for the
> > cdclk prep request, SAGV will have a different value.
> > If we don't want callers to pass in the timeout as well, I guess we'll
> > at least want to reword this comment to say which callers need what
> > kind of timeouts.
> 
> How about "at most 3ms - which can be less depending on the given
> request"?

I'd rather want explicit "cdclk thing needs 3ms, sagv thing needs
1ms" (or whatever the sagv timeout was). Otherwise it's hard to see
whether it's even doing the correct thing.

> 
> > Though I kinda like the idea of callers passing that
> > in as well.
> 
> The 10ms timeout I chose here is anyway an upper bound, considering
> that interrupts could reduce the number of requests we can send. Hence
> thought to make it a generic value.
> 
> > 
> > > +	 * 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,
> > > +						    reply_mask, reply, &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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v5 2/3] drm/i915/gen9: Fix PCODE polling during CDCLK change notification

[Imre Deak]

commit 848496e5902833600f7992f4faa82dc1546051ba
Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
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.
v4:
- Pass reply_mask, reply to skl_pcode_request(), instead of assuming the
  reply is generic. (Ville)
v5:
- List the request specific timeout values as code comment. (Ville)

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
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@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 ++
 drivers/gpu/drm/i915/intel_display.c | 31 +++++++--------------
 drivers/gpu/drm/i915/intel_pm.c      | 53 ++++++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 01f5067..1be5bab 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3593,6 +3593,8 @@ 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,
+		      u32 reply_mask, u32 reply);
 
 /* intel_sideband.c */
 u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5d11002..3d220da 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6245,35 +6245,24 @@ 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,
+				SKL_CDCLK_READY_FOR_CHANGE,
+				SKL_CDCLK_READY_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..4e06e92 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7864,6 +7864,59 @@ 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 reply_mask, u32 reply,
+				  u32 *status)
+{
+	u32 val = request;
+
+	*status = sandybridge_pcode_read(dev_priv, mbox, &val);
+
+	return *status || ((val & reply_mask) == reply);
+}
+
+/**
+ * skl_pcode_request - send PCODE request until acknowledgment
+ * @dev_priv: device private
+ * @mbox: PCODE mailbox ID the request is targeted for
+ * @request: request ID
+ * @reply_mask: mask used to check for request acknowledgment
+ * @reply: value used to check for request acknowledgement
+ *
+ * Keep resending the @request to @mbox until PCODE acknowledges it, PCODE
+ * reports an error or a 10ms timeout expires. The request is acknowledged
+ * once the PCODE reply dword equals @reply after applying @reply_mask.
+ *
+ * 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 reply_mask, u32 reply)
+{
+	u32 status;
+	int ret;
+
+	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
+
+	/*
+	 * The spec says to keep retrying the request until a request specific
+	 * timeout is reached or the request is acknowledged. The specific
+	 * timeout values for each request:
+	 * - SKL_CDCLK_PREPARE_FOR_CHANGE: 3ms
+	 * - GEN9_SAGV_DISABLE: 1ms
+	 * To meet the above disable pre-emption to maximize the number of
+	 * attempts within the timeout period. 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,
+						    reply_mask, reply, &status),
+			      10);
+	preempt_enable();
+
+	return ret ? ret : status;
+}
+
 static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
 {
 	/*
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 2/3] drm/i915/gen9: Fix PCODE polling during CDCLK change notification

[Chris Wilson]

On Mon, Nov 28, 2016 at 06:40:33PM +0200, Imre Deak wrote:
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 66c62f3..4e06e92 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7864,6 +7864,59 @@ 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 reply_mask, u32 reply,
> +				  u32 *status)
> +{
> +	u32 val = request;
> +
> +	*status = sandybridge_pcode_read(dev_priv, mbox, &val);

Hmm. This in turn uses a plain wait_for() that we want to stop relying
on drm_can_sleep() in future. Right now, it's ok but we're just making
things harder for ourselves later. I'm not keen on the alternative
though (passing around I am atomic flags, or making more things atomic
by default). :|
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 2/3] drm/i915/gen9: Fix PCODE polling during CDCLK change notification

[Imre Deak]

On ma, 2016-11-28 at 17:13 +0000, Chris Wilson wrote:
> On Mon, Nov 28, 2016 at 06:40:33PM +0200, Imre Deak wrote:
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 66c62f3..4e06e92 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -7864,6 +7864,59 @@ 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 reply_mask, u32 reply,
> > +				  u32 *status)
> > +{
> > +	u32 val = request;
> > +
> > +	*status = sandybridge_pcode_read(dev_priv, mbox, &val);
> 
> Hmm. This in turn uses a plain wait_for() that we want to stop relying
> on drm_can_sleep() in future. Right now, it's ok but we're just making
> things harder for ourselves later. I'm not keen on the alternative
> though (passing around I am atomic flags, or making more things atomic
> by default). :|

Another way would be to factor out common parts of
sandybridge_pcode_read/write and implement skl_pcode_try_request() in
terms of those parts. But not sure if that would look better than just
passing an is_atomic flag to sandybridge_pcode_read().

> -Chris
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 3/3] drm/i915/gen9: Fix PCODE polling during SAGV disabling

[Imre Deak]

According to the previous patch, it's possible atm that we call
intel_do_sagv_disable() only once during the 1ms period and time out if
that call fails. As opposed to this the spec says that we need to keep
retrying this request for a 1ms duration, so let's do this similarly to
the CDCLK change notification request.

v4:
- Rebased on the reply_mask, reply change.

Cc: Lyude <cpaul@redhat.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 34 ++++++++--------------------------
 1 file changed, 8 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index aed88e0..61b7108 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2954,24 +2954,10 @@ intel_enable_sagv(struct drm_i915_private *dev_priv)
 	return 0;
 }
 
-static int
-intel_do_sagv_disable(struct drm_i915_private *dev_priv)
-{
-	int ret;
-	uint32_t temp = GEN9_SAGV_DISABLE;
-
-	ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL,
-				     &temp);
-	if (ret)
-		return ret;
-	else
-		return temp & GEN9_SAGV_IS_DISABLED;
-}
-
 int
 intel_disable_sagv(struct drm_i915_private *dev_priv)
 {
-	int ret, result;
+	int ret;
 
 	if (!intel_has_sagv(dev_priv))
 		return 0;
@@ -2981,27 +2967,23 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
 
 	DRM_DEBUG_KMS("Disabling the SAGV\n");
 	mutex_lock(&dev_priv->rps.hw_lock);
-
 	/* bspec says to keep retrying for at least 1 ms */
-	ret = wait_for(result = intel_do_sagv_disable(dev_priv), 1);
+	ret = skl_pcode_request(dev_priv, GEN9_PCODE_SAGV_CONTROL,
+				GEN9_SAGV_DISABLE,
+				GEN9_SAGV_IS_DISABLED, GEN9_SAGV_IS_DISABLED);
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
-	if (ret == -ETIMEDOUT) {
-		DRM_ERROR("Request to disable SAGV timed out\n");
-		return -ETIMEDOUT;
-	}
-
 	/*
 	 * Some skl systems, pre-release machines in particular,
 	 * don't actually have an SAGV.
 	 */
-	if (IS_SKYLAKE(dev_priv) && result == -ENXIO) {
+	if (IS_SKYLAKE(dev_priv) && ret == -ENXIO) {
 		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
 		dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED;
 		return 0;
-	} else if (result < 0) {
-		DRM_ERROR("Failed to disable the SAGV\n");
-		return result;
+	} else if (ret < 0) {
+		DRM_ERROR("Failed to disable the SAGV (%d)\n", ret);
+		return ret;
 	}
 
 	dev_priv->sagv_status = I915_SAGV_DISABLED;
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [v4,1/3] drm/i915/gen6+: Clear upper data byte during PCODE write

[Patchwork]

== Series Details ==

Series: series starting with [v4,1/3] drm/i915/gen6+: Clear upper data byte during PCODE write
URL   : https://patchwork.freedesktop.org/series/16046/
State : success

== Summary ==

Series 16046v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/16046/revisions/1/mbox/


fi-bdw-5557u     total:245  pass:230  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:245  pass:205  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:245  pass:217  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:245  pass:217  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:245  pass:213  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:245  pass:225  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:245  pass:225  dwarn:0   dfail:0   fail:0   skip:20 
fi-ivb-3520m     total:245  pass:223  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:245  pass:223  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7500u     total:245  pass:223  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:245  pass:231  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:245  pass:224  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:245  pass:223  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:245  pass:231  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:245  pass:213  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:245  pass:212  dwarn:0   dfail:0   fail:0   skip:33 

8f02d2103390e48e9e76c9e75d16dbb4396484c8 drm-tip: 2016y-11m-28d-12h-38m-14s UTC integration manifest
3344d31 drm/i915/gen9: Fix PCODE polling during SAGV disabling
eb49123 drm/i915/gen9: Fix PCODE polling during CDCLK change notification
9296bd4 drm/i915/gen6+: Clear upper data byte during PCODE write

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3127/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [v4,1/3] drm/i915/gen6+: Clear upper data byte during PCODE write (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v4,1/3] drm/i915/gen6+: Clear upper data byte during PCODE write (rev2)
URL   : https://patchwork.freedesktop.org/series/16046/
State : success

== Summary ==

Series 16046v2 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/16046/revisions/2/mbox/


fi-bdw-5557u     total:245  pass:230  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:245  pass:205  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:245  pass:217  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:245  pass:217  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:245  pass:213  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:245  pass:225  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:245  pass:225  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:245  pass:192  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:245  pass:223  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:245  pass:223  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7500u     total:245  pass:223  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:245  pass:231  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:245  pass:224  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:245  pass:223  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:245  pass:231  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:245  pass:213  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:245  pass:212  dwarn:0   dfail:0   fail:0   skip:33 

477cc06f6ef4842fd63e28ff30d721462af199a6 drm-tip: 2016y-11m-28d-16h-17m-39s UTC integration manifest
b82f611 drm/i915/gen9: Fix PCODE polling during SAGV disabling
97b0290 drm/i915/gen9: Fix PCODE polling during CDCLK change notification
50aec7a4c drm/i915/gen6+: Clear upper data byte during PCODE write

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3128/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✓ Fi.CI.BAT: success for series starting with [v4,1/3] drm/i915/gen6+: Clear upper data byte during PCODE write (rev2)

[Imre Deak]

On ma, 2016-11-28 at 17:15 +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [v4,1/3] drm/i915/gen6+: Clear upper data byte during PCODE write (rev2)
> URL   : https://patchwork.freedesktop.org/series/16046/
> State : success
> 
> == Summary ==
> 
> Series 16046v2 Series without cover letter
> https://patchwork.freedesktop.org/api/1.0/series/16046/revisions/2/mbox/

I pushed 1/3 to -dinq, thanks for the review. I'll send an updated
version of the other 2, doing the polling first w/o disabling
preemption. According to Art and my tests that should succeed most of
the time, so we'd need the preempt-disabled poll only as a fallback.

> fi-bdw-5557u     total:245  pass:230  dwarn:0   dfail:0   fail:0   skip:15 
> fi-bsw-n3050     total:245  pass:205  dwarn:0   dfail:0   fail:0   skip:40 
> fi-bxt-t5700     total:245  pass:217  dwarn:0   dfail:0   fail:0   skip:28 
> fi-byt-j1900     total:245  pass:217  dwarn:0   dfail:0   fail:0   skip:28 
> fi-byt-n2820     total:245  pass:213  dwarn:0   dfail:0   fail:0   skip:32 
> fi-hsw-4770      total:245  pass:225  dwarn:0   dfail:0   fail:0   skip:20 
> fi-hsw-4770r     total:245  pass:225  dwarn:0   dfail:0   fail:0   skip:20 
> fi-ilk-650       total:245  pass:192  dwarn:0   dfail:0   fail:0   skip:53 
> fi-ivb-3520m     total:245  pass:223  dwarn:0   dfail:0   fail:0   skip:22 
> fi-ivb-3770      total:245  pass:223  dwarn:0   dfail:0   fail:0   skip:22 
> fi-kbl-7500u     total:245  pass:223  dwarn:0   dfail:0   fail:0   skip:22 
> fi-skl-6260u     total:245  pass:231  dwarn:0   dfail:0   fail:0   skip:14 
> fi-skl-6700hq    total:245  pass:224  dwarn:0   dfail:0   fail:0   skip:21 
> fi-skl-6700k     total:245  pass:223  dwarn:1   dfail:0   fail:0   skip:21 
> fi-skl-6770hq    total:245  pass:231  dwarn:0   dfail:0   fail:0   skip:14 
> fi-snb-2520m     total:245  pass:213  dwarn:0   dfail:0   fail:0   skip:32 
> fi-snb-2600      total:245  pass:212  dwarn:0   dfail:0   fail:0   skip:33 
> 
> 477cc06f6ef4842fd63e28ff30d721462af199a6 drm-tip: 2016y-11m-28d-16h-17m-39s UTC integration manifest
> b82f611 drm/i915/gen9: Fix PCODE polling during SAGV disabling
> 97b0290 drm/i915/gen9: Fix PCODE polling during CDCLK change notification
> 50aec7a4c drm/i915/gen6+: Clear upper data byte during PCODE write
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3128/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx