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

[Imre Deak <imre.deak@intel.com>]

On ma, 2016-11-28 at 16:11 +0200, Ville Syrjälä wrote:
> On Mon, Nov 28, 2016 at 04:06:05PM +0200, Ville Syrjälä wrote:
> > On Mon, Nov 28, 2016 at 03:54:08PM +0200, Imre Deak wrote:
> > > On ma, 2016-11-28 at 15:51 +0200, Ville Syrjälä wrote:
> > > > On Mon, Nov 28, 2016 at 01:12:57PM +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.
> > > > > 
> > > > > 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      |  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?
> > > 
> > > At least SAGV uses the same and there is no other request I know of
> > > that would need this polling request. It will be used in the next patch
> > > for SAGV too.
> > 
> > At least it's a bit mispaced since now it looks like it would be
> > a pcode command, but command 0x1 == CMD_CONFIG according to the spec.
> 
> If we don't want to assume it's all that generic, I guess we could
> always have the caller pass in the mask+value to wait for?

Ok, will change that.

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