| Message ID | 1383249135-11986-2-git-send-email-jeff.mcgee@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 31, 2013 at 8:52 PM, <jeff.mcgee@intel.com> wrote: > From: Jeff McGee <jeff.mcgee@intel.com> > > A check of rps/rc6 state after i915_reset determined that the ring > MAX_IDLE registers were returned to their hardware defaults and that > the GEN6_PMIMR register was set to mask all interrupts. This change > restores those values to their pre-reset states by re-initializing > rps/rc6 in i915_reset. A full re-initialization was opted for versus > a targeted set of restore operations for simplicity and maintain- > ability. Note that the re-initialization is not done for Ironlake, > due to a past comment that it causes problems. > > Also updated the rps initialization sequence to preserve existing > min/max values in the case of a re-init. We assume the values were > validated upon being set and do not do further range checking. The > debugfs interface for changing min/max was updated with range > checking to ensure this condition (already present in sysfs > interface). > > Issue: VIZ-3142 > Issue: AXIA-4676 > OTC-Tracker: VIZ-3143 > Signed-off-by: Jeff McGee <jeff.mcgee@intel.com> Can I have a testcase in i-g-t for this please? I think the following should work: 1. Throw a dummy load onto the gpu, check that cagf goes up. 2. Limit min/max to a non-default value (and install an igt atexit handler to undo this). 3. Throw a dummy load onto the gpu, check that cagf jumps from the idle freq to the selected one directly. 4. Inject a gpu hang with the stop_rings stuff (see e.g. kms_flip.c or ZZ_hangman). 5. Reject that the limts still work as in step 3. Cheers, Daniel
On Mon, 2013-11-04 at 09:44 +0100, Daniel Vetter wrote: > On Thu, Oct 31, 2013 at 8:52 PM, <jeff.mcgee@intel.com> wrote: > > From: Jeff McGee <jeff.mcgee@intel.com> > > > > A check of rps/rc6 state after i915_reset determined that the ring > > MAX_IDLE registers were returned to their hardware defaults and that > > the GEN6_PMIMR register was set to mask all interrupts. This change > > restores those values to their pre-reset states by re-initializing > > rps/rc6 in i915_reset. A full re-initialization was opted for versus > > a targeted set of restore operations for simplicity and maintain- > > ability. Note that the re-initialization is not done for Ironlake, > > due to a past comment that it causes problems. > > > > Also updated the rps initialization sequence to preserve existing > > min/max values in the case of a re-init. We assume the values were > > validated upon being set and do not do further range checking. The > > debugfs interface for changing min/max was updated with range > > checking to ensure this condition (already present in sysfs > > interface). > > > > Issue: VIZ-3142 > > Issue: AXIA-4676 > > OTC-Tracker: VIZ-3143 > > Signed-off-by: Jeff McGee <jeff.mcgee@intel.com> > > Can I have a testcase in i-g-t for this please? I think the following > should work: > > 1. Throw a dummy load onto the gpu, check that cagf goes up. > > 2. Limit min/max to a non-default value (and install an igt atexit > handler to undo this). > > 3. Throw a dummy load onto the gpu, check that cagf jumps from the > idle freq to the selected one directly. > > 4. Inject a gpu hang with the stop_rings stuff (see e.g. kms_flip.c or > ZZ_hangman). > > 5. Reject that the limts still work as in step 3. > > Cheers, Daniel I'll see what can be done. I understand the emphasis on adding formalized tests. There will have to be some resourcing discussions on the product side if this is now a requirement for upstream patch acceptance. Jeff
On Tue, 2013-11-05 at 20:38 +0000, Mcgee, Jeff wrote: > On Mon, 2013-11-04 at 09:44 +0100, Daniel Vetter wrote: > > On Thu, Oct 31, 2013 at 8:52 PM, <jeff.mcgee@intel.com> wrote: > > > From: Jeff McGee <jeff.mcgee@intel.com> > > > > > > A check of rps/rc6 state after i915_reset determined that the ring > > > MAX_IDLE registers were returned to their hardware defaults and that > > > the GEN6_PMIMR register was set to mask all interrupts. This change > > > restores those values to their pre-reset states by re-initializing > > > rps/rc6 in i915_reset. A full re-initialization was opted for versus > > > a targeted set of restore operations for simplicity and maintain- > > > ability. Note that the re-initialization is not done for Ironlake, > > > due to a past comment that it causes problems. > > > > > > Also updated the rps initialization sequence to preserve existing > > > min/max values in the case of a re-init. We assume the values were > > > validated upon being set and do not do further range checking. The > > > debugfs interface for changing min/max was updated with range > > > checking to ensure this condition (already present in sysfs > > > interface). > > > > > > Issue: VIZ-3142 > > > Issue: AXIA-4676 > > > OTC-Tracker: VIZ-3143 > > > Signed-off-by: Jeff McGee <jeff.mcgee@intel.com> > > > > Can I have a testcase in i-g-t for this please? I think the following > > should work: > > > > 1. Throw a dummy load onto the gpu, check that cagf goes up. > > > > 2. Limit min/max to a non-default value (and install an igt atexit > > handler to undo this). > > > > 3. Throw a dummy load onto the gpu, check that cagf jumps from the > > idle freq to the selected one directly. > > > > 4. Inject a gpu hang with the stop_rings stuff (see e.g. kms_flip.c or > > ZZ_hangman). > > > > 5. Reject that the limts still work as in step 3. > > > > Cheers, Daniel > > I'll see what can be done. I understand the emphasis on adding > formalized tests. There will have to be some resourcing discussions on > the product side if this is now a requirement for upstream patch > acceptance. > > Jeff This discussion is starting in VPG Android, but it may be a while until test code gets submitted. This is a fairly serious bug and easy to reproduce manually. So please consider the patch without the test case for now. Thanks Jeff
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 5c45e9e..68710f3 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2565,6 +2565,7 @@ i915_max_freq_set(void *data, u64 val) { struct drm_device *dev = data; struct drm_i915_private *dev_priv = dev->dev_private; + u32 rp_state_cap, hw_max, hw_min; int ret; if (!(IS_GEN6(dev) || IS_GEN7(dev))) @@ -2583,14 +2584,29 @@ i915_max_freq_set(void *data, u64 val) */ if (IS_VALLEYVIEW(dev)) { val = vlv_freq_opcode(dev_priv->mem_freq, val); - dev_priv->rps.max_delay = val; - gen6_set_rps(dev, val); + + hw_max = valleyview_rps_max_freq(dev_priv); + hw_min = valleyview_rps_min_freq(dev_priv); } else { do_div(val, GT_FREQUENCY_MULTIPLIER); - dev_priv->rps.max_delay = val; - gen6_set_rps(dev, val); + + rp_state_cap = I915_READ(GEN6_RP_STATE_CAP); + hw_max = dev_priv->rps.hw_max; + hw_min = (rp_state_cap >> 16) & 0xff; + } + + if (val < hw_min || val > hw_max || val < dev_priv->rps.min_delay) { + mutex_unlock(&dev_priv->rps.hw_lock); + return -EINVAL; } + dev_priv->rps.max_delay = val; + + if (IS_VALLEYVIEW(dev)) + valleyview_set_rps(dev, val); + else + gen6_set_rps(dev, val); + mutex_unlock(&dev_priv->rps.hw_lock); return 0; @@ -2631,6 +2647,7 @@ i915_min_freq_set(void *data, u64 val) { struct drm_device *dev = data; struct drm_i915_private *dev_priv = dev->dev_private; + u32 rp_state_cap, hw_max, hw_min; int ret; if (!(IS_GEN6(dev) || IS_GEN7(dev))) @@ -2649,13 +2666,29 @@ i915_min_freq_set(void *data, u64 val) */ if (IS_VALLEYVIEW(dev)) { val = vlv_freq_opcode(dev_priv->mem_freq, val); - dev_priv->rps.min_delay = val; - valleyview_set_rps(dev, val); + + hw_max = valleyview_rps_max_freq(dev_priv); + hw_min = valleyview_rps_min_freq(dev_priv); } else { do_div(val, GT_FREQUENCY_MULTIPLIER); - dev_priv->rps.min_delay = val; - gen6_set_rps(dev, val); + + rp_state_cap = I915_READ(GEN6_RP_STATE_CAP); + hw_max = dev_priv->rps.hw_max; + hw_min = (rp_state_cap >> 16) & 0xff; + } + + if (val < hw_min || val > hw_max || val > dev_priv->rps.max_delay) { + mutex_unlock(&dev_priv->rps.hw_lock); + return -EINVAL; } + + dev_priv->rps.min_delay = val; + + if (IS_VALLEYVIEW(dev)) + valleyview_set_rps(dev, val); + else + gen6_set_rps(dev, val); + mutex_unlock(&dev_priv->rps.hw_lock); return 0; diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index a0804fa..7137ae7 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -769,6 +769,17 @@ int i915_reset(struct drm_device *dev) drm_irq_uninstall(dev); drm_irq_install(dev); + + /* rps/rc6 re-init is necessary to restore state lost after the + * reset and the re-install of drm irq. Skip for ironlake per + * previous concerns that it doesn't respond well to some forms + * of re-init after reset. */ + if (INTEL_INFO(dev)->gen > 5) { + mutex_lock(&dev->struct_mutex); + intel_enable_gt_powersave(dev); + mutex_unlock(&dev->struct_mutex); + } + intel_hpd_init(dev); } else { mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index a0c907f..b079ab8 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3751,7 +3751,7 @@ static void gen6_enable_rps(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; struct intel_ring_buffer *ring; - u32 rp_state_cap; + u32 rp_state_cap, hw_max, hw_min; u32 gt_perf_status; u32 rc6vids, pcu_mbox, rc6_mask = 0; u32 gtfifodbg; @@ -3780,13 +3780,20 @@ static void gen6_enable_rps(struct drm_device *dev) gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS); /* In units of 50MHz */ - dev_priv->rps.hw_max = dev_priv->rps.max_delay = rp_state_cap & 0xff; - dev_priv->rps.min_delay = (rp_state_cap >> 16) & 0xff; + dev_priv->rps.hw_max = hw_max = rp_state_cap & 0xff; + hw_min = (rp_state_cap >> 16) & 0xff; dev_priv->rps.rp1_delay = (rp_state_cap >> 8) & 0xff; dev_priv->rps.rp0_delay = (rp_state_cap >> 0) & 0xff; dev_priv->rps.rpe_delay = dev_priv->rps.rp1_delay; dev_priv->rps.cur_delay = 0; + /* Preserve min/max settings in case of re-init */ + if (dev_priv->rps.max_delay == 0) + dev_priv->rps.max_delay = hw_max; + + if (dev_priv->rps.min_delay == 0) + dev_priv->rps.min_delay = hw_min; + /* disable the counters and set deterministic thresholds */ I915_WRITE(GEN6_RC_CONTROL, 0); @@ -4012,7 +4019,7 @@ static void valleyview_enable_rps(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; struct intel_ring_buffer *ring; - u32 gtfifodbg, val, rc6_mode = 0; + u32 gtfifodbg, val, hw_max, hw_min, rc6_mode = 0; int i; WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock)); @@ -4087,12 +4094,11 @@ static void valleyview_enable_rps(struct drm_device *dev) dev_priv->rps.cur_delay), dev_priv->rps.cur_delay); - dev_priv->rps.max_delay = valleyview_rps_max_freq(dev_priv); - dev_priv->rps.hw_max = dev_priv->rps.max_delay; + dev_priv->rps.hw_max = hw_max = valleyview_rps_max_freq(dev_priv); DRM_DEBUG_DRIVER("max GPU freq: %d MHz (%u)\n", vlv_gpu_freq(dev_priv->mem_freq, dev_priv->rps.max_delay), - dev_priv->rps.max_delay); + hw_max); dev_priv->rps.rpe_delay = valleyview_rps_rpe_freq(dev_priv); DRM_DEBUG_DRIVER("RPe GPU freq: %d MHz (%u)\n", @@ -4100,11 +4106,18 @@ static void valleyview_enable_rps(struct drm_device *dev) dev_priv->rps.rpe_delay), dev_priv->rps.rpe_delay); - dev_priv->rps.min_delay = valleyview_rps_min_freq(dev_priv); + hw_min = valleyview_rps_min_freq(dev_priv); DRM_DEBUG_DRIVER("min GPU freq: %d MHz (%u)\n", vlv_gpu_freq(dev_priv->mem_freq, dev_priv->rps.min_delay), - dev_priv->rps.min_delay); + hw_min); + + /* Preserve min/max settings in case of re-init */ + if (dev_priv->rps.max_delay == 0) + dev_priv->rps.max_delay = hw_max; + + if (dev_priv->rps.min_delay == 0) + dev_priv->rps.min_delay = hw_min; DRM_DEBUG_DRIVER("setting GPU freq to %d MHz (%u)\n", vlv_gpu_freq(dev_priv->mem_freq,