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@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? > > > > > > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
