| Message ID | 1406876761-15814-1-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 01, 2014 at 12:34:56PM +0530, sagar.a.kamble@intel.com wrote: > From: Sagar Kamble <sagar.a.kamble@intel.com> > > Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of gunit registers need to be followed in > PM suspend and resume path similar to runtime suspend and resume. > > v2: > 1. Keeping GT access, wake, gunit save/restore related helpers static. > 2. Moved GT access check, Wake Control, Gunit state save to end of i915_drm_freeze. > 3. Reusing the sequence in runtime_suspend/resume path at macro level. > > Cc: Imre Deak <imre.deak at intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch> > Cc: Jani Nikula <jani.nikula at linux.intel.com> > Cc: Goel, Akash <akash.goel@intel.com> > Change-Id: I15cfdeeec9c976d9839bb281f809664f4a0c78a2 > Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 39 +++++++++++++++++++++++++++++++++------ > drivers/gpu/drm/i915/i915_drv.h | 1 + > 2 files changed, 34 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 6c4b25c..385dc74 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -490,11 +490,16 @@ bool i915_semaphore_is_enabled(struct drm_device *dev) > return true; > } > > +static int vlv_runtime_suspend(struct drm_i915_private *dev_priv); > +static int vlv_runtime_resume(struct drm_i915_private *dev_priv, > + bool resume_from_s0ix); > + > static int i915_drm_freeze(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_crtc *crtc; > pci_power_t opregion_target_state; > + int ret = 0; > > /* ignore lid events during suspend */ > mutex_lock(&dev_priv->modeset_restore_lock); > @@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev) > > intel_display_set_init_power(dev_priv, false); > > - return 0; > + /* Save Gunit State and clear wake - Need to make sure > + * changes in vlv_runtime_suspend path don't impact this path */ > + if (IS_VALLEYVIEW(dev)) > + ret = vlv_runtime_suspend(dev_priv); Maybe I wasn't clear, but I absolutely don't want any IS_VLV additions to core resume/thaw code. This should be shovelled into the runtime pm handling code, which should be reused in the suspend/resume code. > + > + return ret; > } > > int i915_suspend(struct drm_device *dev, pm_message_t state) > @@ -610,6 +620,12 @@ void intel_console_resume(struct work_struct *work) > static int i915_drm_thaw_early(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > + int ret = 0; > + > + /* Restore Gunit State and allow wake - Need to make sure > + * changes in vlv_runtime_resume path don't impact this path */ > + if (IS_VALLEYVIEW(dev)) > + ret = vlv_runtime_resume(dev_priv, true); > > if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > hsw_disable_pc8(dev_priv); > @@ -618,7 +634,7 @@ static int i915_drm_thaw_early(struct drm_device *dev) > intel_uncore_sanitize(dev); > intel_power_domains_init_hw(dev_priv); > > - return 0; > + return ret; > } > > static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) > @@ -1098,6 +1114,7 @@ static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv) > s->gu_ctl0 = I915_READ(VLV_GU_CTL0); > s->gu_ctl1 = I915_READ(VLV_GU_CTL1); > s->clock_gate_dis2 = I915_READ(VLV_GUNIT_CLOCK_GATE2); > + s->dpio_cfg_data = I915_READ(DPIO_CTL); > > /* > * Not saving any of: > @@ -1192,6 +1209,7 @@ static void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv) > I915_WRITE(VLV_GU_CTL0, s->gu_ctl0); > I915_WRITE(VLV_GU_CTL1, s->gu_ctl1); > I915_WRITE(VLV_GUNIT_CLOCK_GATE2, s->clock_gate_dis2); > + I915_WRITE(DPIO_CTL, s->dpio_cfg_data); > } > > int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on) > @@ -1291,6 +1309,8 @@ static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv) > I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR); > } > > +/* This function is used in system suspend path as well to utilize > + * Gfx clock, Wake control, Gunit state save related functionaility */ > static int vlv_runtime_suspend(struct drm_i915_private *dev_priv) > { > u32 mask; > @@ -1331,7 +1351,12 @@ err1: > return err; > } > > -static int vlv_runtime_resume(struct drm_i915_private *dev_priv) > +/* This function is used in system resume path as well to utilize > + * Gfx clock, Wake control, Gunit state restore related functionaility. > + * GEM and other initialization will differ which will be controlled by > + * resume_from_s0ix variable */ > +static int vlv_runtime_resume(struct drm_i915_private *dev_priv, > + bool resume_from_s0ix) > { > struct drm_device *dev = dev_priv->dev; > int err; > @@ -1356,8 +1381,10 @@ static int vlv_runtime_resume(struct drm_i915_private *dev_priv) > > vlv_check_no_gt_access(dev_priv); > > - intel_init_clock_gating(dev); > - i915_gem_restore_fences(dev); > + if (!resume_from_s0ix) { > + intel_init_clock_gating(dev); > + i915_gem_restore_fences(dev); > + } This essentially amounts to another IS_VLV block. I might be able to live with a generic "supports runtime pm check". > > return ret; > } > @@ -1462,7 +1489,7 @@ static int intel_runtime_resume(struct device *device) > } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { > ret = hsw_runtime_resume(dev_priv); > } else if (IS_VALLEYVIEW(dev)) { > - ret = vlv_runtime_resume(dev_priv); > + ret = vlv_runtime_resume(dev_priv, false); > } else { > WARN_ON(1); > ret = -ENODEV; > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index d604f4f..3a836c9 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -910,6 +910,7 @@ struct vlv_s0ix_state { > u32 gu_ctl0; > u32 gu_ctl1; > u32 clock_gate_dis2; > + u32 dpio_cfg_data; Register save/restore files considered evil. I've let vlv slip through, but I really want people to try harder to avoid these. The correct fix is to pimp the clock_gating_init functions and similar places to make sure we don't just keep the right value around, but also reinit in all places correctly. -Daniel
Hi Daniel, On Mon, 2014-08-04 at 10:07 +0200, Daniel Vetter wrote: > On Fri, Aug 01, 2014 at 12:34:56PM +0530, sagar.a.kamble@intel.com wrote: > > From: Sagar Kamble <sagar.a.kamble@intel.com> > > > > Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of gunit registers need to be followed in > > PM suspend and resume path similar to runtime suspend and resume. > > > > v2: > > 1. Keeping GT access, wake, gunit save/restore related helpers static. > > 2. Moved GT access check, Wake Control, Gunit state save to end of i915_drm_freeze. > > 3. Reusing the sequence in runtime_suspend/resume path at macro level. > > > > Cc: Imre Deak <imre.deak at intel.com> > > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com> > > Cc: Daniel Vetter <daniel.vetter at ffwll.ch> > > Cc: Jani Nikula <jani.nikula at linux.intel.com> > > Cc: Goel, Akash <akash.goel@intel.com> > > Change-Id: I15cfdeeec9c976d9839bb281f809664f4a0c78a2 > > Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.c | 39 +++++++++++++++++++++++++++++++++------ > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > 2 files changed, 34 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index 6c4b25c..385dc74 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -490,11 +490,16 @@ bool i915_semaphore_is_enabled(struct drm_device *dev) > > return true; > > } > > > > +static int vlv_runtime_suspend(struct drm_i915_private *dev_priv); > > +static int vlv_runtime_resume(struct drm_i915_private *dev_priv, > > + bool resume_from_s0ix); > > + > > static int i915_drm_freeze(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > struct drm_crtc *crtc; > > pci_power_t opregion_target_state; > > + int ret = 0; > > > > /* ignore lid events during suspend */ > > mutex_lock(&dev_priv->modeset_restore_lock); > > @@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev) > > > > intel_display_set_init_power(dev_priv, false); > > > > - return 0; > > + /* Save Gunit State and clear wake - Need to make sure > > + * changes in vlv_runtime_suspend path don't impact this path */ > > + if (IS_VALLEYVIEW(dev)) > > + ret = vlv_runtime_suspend(dev_priv); > > Maybe I wasn't clear, but I absolutely don't want any IS_VLV additions to > core resume/thaw code. This should be shovelled into the runtime pm > handling code, which should be reused in the suspend/resume code. This piece of code does not fit into any of the power well get/put path. Its specific sequence that need to be followed in VLV when Gunit gets power gated. So we have to keep this IS_VLV related functionality in both runtime and pm suspend/resume. > > > + > > + return ret; > > } > > > > int i915_suspend(struct drm_device *dev, pm_message_t state) > > @@ -610,6 +620,12 @@ void intel_console_resume(struct work_struct *work) > > static int i915_drm_thaw_early(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > + int ret = 0; > > + > > + /* Restore Gunit State and allow wake - Need to make sure > > + * changes in vlv_runtime_resume path don't impact this path */ > > + if (IS_VALLEYVIEW(dev)) > > + ret = vlv_runtime_resume(dev_priv, true); > > > > if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > > hsw_disable_pc8(dev_priv); > > @@ -618,7 +634,7 @@ static int i915_drm_thaw_early(struct drm_device *dev) > > intel_uncore_sanitize(dev); > > intel_power_domains_init_hw(dev_priv); > > > > - return 0; > > + return ret; > > } > > > > static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) > > @@ -1098,6 +1114,7 @@ static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv) > > s->gu_ctl0 = I915_READ(VLV_GU_CTL0); > > s->gu_ctl1 = I915_READ(VLV_GU_CTL1); > > s->clock_gate_dis2 = I915_READ(VLV_GUNIT_CLOCK_GATE2); > > + s->dpio_cfg_data = I915_READ(DPIO_CTL); > > > > /* > > * Not saving any of: > > @@ -1192,6 +1209,7 @@ static void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv) > > I915_WRITE(VLV_GU_CTL0, s->gu_ctl0); > > I915_WRITE(VLV_GU_CTL1, s->gu_ctl1); > > I915_WRITE(VLV_GUNIT_CLOCK_GATE2, s->clock_gate_dis2); > > + I915_WRITE(DPIO_CTL, s->dpio_cfg_data); > > } > > > > int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on) > > @@ -1291,6 +1309,8 @@ static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv) > > I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR); > > } > > > > +/* This function is used in system suspend path as well to utilize > > + * Gfx clock, Wake control, Gunit state save related functionaility */ > > static int vlv_runtime_suspend(struct drm_i915_private *dev_priv) > > { > > u32 mask; > > @@ -1331,7 +1351,12 @@ err1: > > return err; > > } > > > > -static int vlv_runtime_resume(struct drm_i915_private *dev_priv) > > +/* This function is used in system resume path as well to utilize > > + * Gfx clock, Wake control, Gunit state restore related functionaility. > > + * GEM and other initialization will differ which will be controlled by > > + * resume_from_s0ix variable */ > > +static int vlv_runtime_resume(struct drm_i915_private *dev_priv, > > + bool resume_from_s0ix) > > { > > struct drm_device *dev = dev_priv->dev; > > int err; > > @@ -1356,8 +1381,10 @@ static int vlv_runtime_resume(struct drm_i915_private *dev_priv) > > > > vlv_check_no_gt_access(dev_priv); > > > > - intel_init_clock_gating(dev); > > - i915_gem_restore_fences(dev); > > + if (!resume_from_s0ix) { > > + intel_init_clock_gating(dev); > > + i915_gem_restore_fences(dev); > > + } > > This essentially amounts to another IS_VLV block. I might be able to live > with a generic "supports runtime pm check". > > > > > return ret; > > } > > @@ -1462,7 +1489,7 @@ static int intel_runtime_resume(struct device *device) > > } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { > > ret = hsw_runtime_resume(dev_priv); > > } else if (IS_VALLEYVIEW(dev)) { > > - ret = vlv_runtime_resume(dev_priv); > > + ret = vlv_runtime_resume(dev_priv, false); > > } else { > > WARN_ON(1); > > ret = -ENODEV; > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index d604f4f..3a836c9 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -910,6 +910,7 @@ struct vlv_s0ix_state { > > u32 gu_ctl0; > > u32 gu_ctl1; > > u32 clock_gate_dis2; > > + u32 dpio_cfg_data; > > Register save/restore files considered evil. I've let vlv slip through, > but I really want people to try harder to avoid these. The correct fix is > to pimp the clock_gating_init functions and similar places to make sure we > don't just keep the right value around, but also reinit in all places > correctly. > -Daniel
On Fri, Aug 08, 2014 at 12:22:44PM +0530, Sagar Arun Kamble wrote: > Hi Daniel, > On Mon, 2014-08-04 at 10:07 +0200, Daniel Vetter wrote: > > On Fri, Aug 01, 2014 at 12:34:56PM +0530, sagar.a.kamble@intel.com wrote: > > > From: Sagar Kamble <sagar.a.kamble@intel.com> > > > @@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev) > > > > > > intel_display_set_init_power(dev_priv, false); > > > > > > - return 0; > > > + /* Save Gunit State and clear wake - Need to make sure > > > + * changes in vlv_runtime_suspend path don't impact this path */ > > > + if (IS_VALLEYVIEW(dev)) > > > + ret = vlv_runtime_suspend(dev_priv); > > > > Maybe I wasn't clear, but I absolutely don't want any IS_VLV additions to > > core resume/thaw code. This should be shovelled into the runtime pm > > handling code, which should be reused in the suspend/resume code. > This piece of code does not fit into any of the power well get/put path. > Its specific sequence that need to be followed in VLV when Gunit gets > power gated. So we have to keep this IS_VLV related functionality in > both runtime and pm suspend/resume. Well we support S0ix now. Which means our system suspend/resume code actually calls into the runtime pm code. So either that design is broken (and we need to fix this) or something else is amiss. Or we don't need this code any more. But duplicating it is not the right approach. And yeah the power domain stuff might not be the right place, I've just used that as a place-holder for all the runtime pm code we have. -Daniel
On Fri, 2014-08-08 at 09:42 +0200, Daniel Vetter wrote: > On Fri, Aug 08, 2014 at 12:22:44PM +0530, Sagar Arun Kamble wrote: > > Hi Daniel, > > On Mon, 2014-08-04 at 10:07 +0200, Daniel Vetter wrote: > > > On Fri, Aug 01, 2014 at 12:34:56PM +0530, sagar.a.kamble@intel.com wrote: > > > > From: Sagar Kamble <sagar.a.kamble@intel.com> > > > > @@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev) > > > > > > > > intel_display_set_init_power(dev_priv, false); > > > > > > > > - return 0; > > > > + /* Save Gunit State and clear wake - Need to make sure > > > > + * changes in vlv_runtime_suspend path don't impact this path */ > > > > + if (IS_VALLEYVIEW(dev)) > > > > + ret = vlv_runtime_suspend(dev_priv); > > > > > > Maybe I wasn't clear, but I absolutely don't want any IS_VLV additions to > > > core resume/thaw code. This should be shovelled into the runtime pm > > > handling code, which should be reused in the suspend/resume code. > > This piece of code does not fit into any of the power well get/put path. > > Its specific sequence that need to be followed in VLV when Gunit gets > > power gated. So we have to keep this IS_VLV related functionality in > > both runtime and pm suspend/resume. > > Well we support S0ix now. Which means our system suspend/resume code > actually calls into the runtime pm code. So either that design is broken > (and we need to fix this) or something else is amiss. Or we don't need > this code any more. > > But duplicating it is not the right approach. And yeah the power domain > stuff might not be the right place, I've just used that as a place-holder > for all the runtime pm code we have. > -Daniel While entering S0iX we are getting following call flow: intel_runtime_resume followed by i915_drm_freeze and While resuming back from S0iX: i915_drm_thaw How can we share that wake control, gunit save/restore functionality? I am observing issue given below: Here is how I am achieving S0ix: 1. echo mem > /sys/power/state 2. (Assuming we have removed all wake_locks in /sys/powe/wake_lock) If device is runtime suspend it is getting resumed with runtime_resume 3. PM core triggers this sequence for each device (prepare, suspend, suspend_late, suspend_noirq) 4. Then pm_suspend happens for Gfx. After all devices are suspended S0i0->S0iR->S0i1->S0i2->S0i3 happens. Gunit gets power gated. While resuming back (with user action/power button press) 1. PM core triggers following sequenc for each device: (resume_noirq, resume_early, resume, complete) After call to i915_drm_thaw_early(called through resume_early), during i915_drm_thaw(called through resume) we were seeing following issue during ring initialization. This is happening since wake is not enabled and Gunit registers are not restored(which is done in runtime_resume but that path is not taken here since this is resume from pm_suspend) <6>[ 3152.991399] PM: device[controlD64] driver[drm] resume enter <3>[ 3153.976759] [drm:stop_ring] *ERROR* render ring :timed out trying to stop ring <3>[ 3154.980061] [drm:stop_ring] *ERROR* render ring :timed out trying to stop ring <3>[ 3154.980067] [drm:init_ring_common] *ERROR* failed to set render ring head to zero ctl 00000000 head 00000000 tail 00000000 start 00000000 <3>[ 3155.983364] [drm:stop_ring] *ERROR* bsd ring :timed out trying to stop ring <3>[ 3156.986668] [drm:stop_ring] *ERROR* bsd ring :timed out trying to stop ring <3>[ 3156.986673] [drm:init_ring_common] *ERROR* failed to set bsd ring head to zero ctl 00000000 head 00000000 tail 00000000 start 00000000 <3>[ 3157.989982] [drm:stop_ring] *ERROR* render ring :timed out trying to stop ring thanks, Sagar
On Fri, 2014-08-08 at 14:29 +0530, Sagar Arun Kamble wrote: > On Fri, 2014-08-08 at 09:42 +0200, Daniel Vetter wrote: > > On Fri, Aug 08, 2014 at 12:22:44PM +0530, Sagar Arun Kamble wrote: > > > Hi Daniel, > > > On Mon, 2014-08-04 at 10:07 +0200, Daniel Vetter wrote: > > > > On Fri, Aug 01, 2014 at 12:34:56PM +0530, sagar.a.kamble@intel.com wrote: > > > > > From: Sagar Kamble <sagar.a.kamble@intel.com> > > > > > @@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev) > > > > > > > > > > intel_display_set_init_power(dev_priv, false); > > > > > > > > > > - return 0; > > > > > + /* Save Gunit State and clear wake - Need to make sure > > > > > + * changes in vlv_runtime_suspend path don't impact this path */ > > > > > + if (IS_VALLEYVIEW(dev)) > > > > > + ret = vlv_runtime_suspend(dev_priv); > > > > > > > > Maybe I wasn't clear, but I absolutely don't want any IS_VLV additions to > > > > core resume/thaw code. This should be shovelled into the runtime pm > > > > handling code, which should be reused in the suspend/resume code. > > > This piece of code does not fit into any of the power well get/put path. > > > Its specific sequence that need to be followed in VLV when Gunit gets > > > power gated. So we have to keep this IS_VLV related functionality in > > > both runtime and pm suspend/resume. > > > > Well we support S0ix now. Which means our system suspend/resume code > > actually calls into the runtime pm code. So either that design is broken > > (and we need to fix this) or something else is amiss. Or we don't need > > this code any more. > > > > But duplicating it is not the right approach. And yeah the power domain > > stuff might not be the right place, I've just used that as a place-holder > > for all the runtime pm code we have. > > -Daniel > While entering S0iX we are getting following call flow: > intel_runtime_resume followed by i915_drm_freeze > and While resuming back from S0iX: > i915_drm_thaw > > How can we share that wake control, gunit save/restore functionality? > > I am observing issue given below: > > > Here is how I am achieving S0ix: > 1. echo mem > /sys/power/state > 2. (Assuming we have removed all wake_locks in /sys/powe/wake_lock) > If device is runtime suspend it is getting resumed with > runtime_resume > 3. PM core triggers this sequence for each device > (prepare, suspend, suspend_late, suspend_noirq) > 4. Then pm_suspend happens for Gfx. > > After all devices are suspended S0i0->S0iR->S0i1->S0i2->S0i3 happens. > Gunit gets power gated. > > While resuming back (with user action/power button press) > 1. PM core triggers following sequenc for each device: > (resume_noirq, resume_early, resume, complete) > > After call to i915_drm_thaw_early(called through resume_early), > during i915_drm_thaw(called through resume) we were seeing following > issue during ring initialization. > This is happening since wake is not enabled and Gunit registers are not > restored(which is done in runtime_resume but that path is not taken here > since this is resume from pm_suspend) Yes, this is correct. The DPM core takes a runtime PM reference before calling the system suspend handler. So you have to call any needed parts explicitly from the system suspend handler that is shared with the runtime PM code. But the platform checks could be contained all within the called common handler and you'd call this handler from the system suspend handler unconditionally. I think this would be closer what Daniel suggested. --Imre > <6>[ 3152.991399] PM: device[controlD64] driver[drm] resume enter > <3>[ 3153.976759] [drm:stop_ring] *ERROR* render ring :timed out trying > to stop ring > <3>[ 3154.980061] [drm:stop_ring] *ERROR* render ring :timed out trying > to stop ring > <3>[ 3154.980067] [drm:init_ring_common] *ERROR* failed to set render > ring head to zero ctl 00000000 head 00000000 tail 00000000 start > 00000000 > <3>[ 3155.983364] [drm:stop_ring] *ERROR* bsd ring :timed out trying to > stop ring > <3>[ 3156.986668] [drm:stop_ring] *ERROR* bsd ring :timed out trying to > stop ring > <3>[ 3156.986673] [drm:init_ring_common] *ERROR* failed to set bsd ring > head to zero ctl 00000000 head 00000000 tail 00000000 start 00000000 > <3>[ 3157.989982] [drm:stop_ring] *ERROR* render ring :timed out trying > to stop ring > > > thanks, > Sagar > >
On Fri, Aug 08, 2014 at 02:29:38PM +0530, Sagar Arun Kamble wrote: > On Fri, 2014-08-08 at 09:42 +0200, Daniel Vetter wrote: > > On Fri, Aug 08, 2014 at 12:22:44PM +0530, Sagar Arun Kamble wrote: > > > Hi Daniel, > > > On Mon, 2014-08-04 at 10:07 +0200, Daniel Vetter wrote: > > > > On Fri, Aug 01, 2014 at 12:34:56PM +0530, sagar.a.kamble@intel.com wrote: > > > > > From: Sagar Kamble <sagar.a.kamble@intel.com> > > > > > @@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev) > > > > > > > > > > intel_display_set_init_power(dev_priv, false); > > > > > > > > > > - return 0; > > > > > + /* Save Gunit State and clear wake - Need to make sure > > > > > + * changes in vlv_runtime_suspend path don't impact this path */ > > > > > + if (IS_VALLEYVIEW(dev)) > > > > > + ret = vlv_runtime_suspend(dev_priv); > > > > > > > > Maybe I wasn't clear, but I absolutely don't want any IS_VLV additions to > > > > core resume/thaw code. This should be shovelled into the runtime pm > > > > handling code, which should be reused in the suspend/resume code. > > > This piece of code does not fit into any of the power well get/put path. > > > Its specific sequence that need to be followed in VLV when Gunit gets > > > power gated. So we have to keep this IS_VLV related functionality in > > > both runtime and pm suspend/resume. > > > > Well we support S0ix now. Which means our system suspend/resume code > > actually calls into the runtime pm code. So either that design is broken > > (and we need to fix this) or something else is amiss. Or we don't need > > this code any more. > > > > But duplicating it is not the right approach. And yeah the power domain > > stuff might not be the right place, I've just used that as a place-holder > > for all the runtime pm code we have. > > -Daniel > While entering S0iX we are getting following call flow: > intel_runtime_resume followed by i915_drm_freeze > and While resuming back from S0iX: > i915_drm_thaw > > How can we share that wake control, gunit save/restore functionality? > > I am observing issue given below: > > > Here is how I am achieving S0ix: > 1. echo mem > /sys/power/state > 2. (Assuming we have removed all wake_locks in /sys/powe/wake_lock) > If device is runtime suspend it is getting resumed with > runtime_resume > 3. PM core triggers this sequence for each device > (prepare, suspend, suspend_late, suspend_noirq) > 4. Then pm_suspend happens for Gfx. > > After all devices are suspended S0i0->S0iR->S0i1->S0i2->S0i3 happens. > Gunit gets power gated. > > While resuming back (with user action/power button press) > 1. PM core triggers following sequenc for each device: > (resume_noirq, resume_early, resume, complete) > > After call to i915_drm_thaw_early(called through resume_early), > during i915_drm_thaw(called through resume) we were seeing following > issue during ring initialization. > This is happening since wake is not enabled and Gunit registers are not > restored(which is done in runtime_resume but that path is not taken here > since this is resume from pm_suspend) We have intel_runtime_pm_get/put calls in the suspend/resume paths which should achieve this. Maybe they're not at the right place or not in the right ordering, but they're there. So on platforms with runtime pm support we _do_ call all the relevant runtime pm callbacks from system suspend/resume. -Daniel > <6>[ 3152.991399] PM: device[controlD64] driver[drm] resume enter > <3>[ 3153.976759] [drm:stop_ring] *ERROR* render ring :timed out trying > to stop ring > <3>[ 3154.980061] [drm:stop_ring] *ERROR* render ring :timed out trying > to stop ring > <3>[ 3154.980067] [drm:init_ring_common] *ERROR* failed to set render > ring head to zero ctl 00000000 head 00000000 tail 00000000 start > 00000000 > <3>[ 3155.983364] [drm:stop_ring] *ERROR* bsd ring :timed out trying to > stop ring > <3>[ 3156.986668] [drm:stop_ring] *ERROR* bsd ring :timed out trying to > stop ring > <3>[ 3156.986673] [drm:init_ring_common] *ERROR* failed to set bsd ring > head to zero ctl 00000000 head 00000000 tail 00000000 start 00000000 > <3>[ 3157.989982] [drm:stop_ring] *ERROR* render ring :timed out trying > to stop ring
On Fri, 2014-08-08 at 11:15 +0200, Daniel Vetter wrote: > On Fri, Aug 08, 2014 at 02:29:38PM +0530, Sagar Arun Kamble wrote: > > On Fri, 2014-08-08 at 09:42 +0200, Daniel Vetter wrote: > > > On Fri, Aug 08, 2014 at 12:22:44PM +0530, Sagar Arun Kamble wrote: > > > > Hi Daniel, > > > > On Mon, 2014-08-04 at 10:07 +0200, Daniel Vetter wrote: > > > > > On Fri, Aug 01, 2014 at 12:34:56PM +0530, sagar.a.kamble@intel.com wrote: > > > > > > From: Sagar Kamble <sagar.a.kamble@intel.com> > > > > > > @@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev) > > > > > > > > > > > > intel_display_set_init_power(dev_priv, false); > > > > > > > > > > > > - return 0; > > > > > > + /* Save Gunit State and clear wake - Need to make sure > > > > > > + * changes in vlv_runtime_suspend path don't impact this path */ > > > > > > + if (IS_VALLEYVIEW(dev)) > > > > > > + ret = vlv_runtime_suspend(dev_priv); > > > > > > > > > > Maybe I wasn't clear, but I absolutely don't want any IS_VLV additions to > > > > > core resume/thaw code. This should be shovelled into the runtime pm > > > > > handling code, which should be reused in the suspend/resume code. > > > > This piece of code does not fit into any of the power well get/put path. > > > > Its specific sequence that need to be followed in VLV when Gunit gets > > > > power gated. So we have to keep this IS_VLV related functionality in > > > > both runtime and pm suspend/resume. > > > > > > Well we support S0ix now. Which means our system suspend/resume code > > > actually calls into the runtime pm code. So either that design is broken > > > (and we need to fix this) or something else is amiss. Or we don't need > > > this code any more. > > > > > > But duplicating it is not the right approach. And yeah the power domain > > > stuff might not be the right place, I've just used that as a place-holder > > > for all the runtime pm code we have. > > > -Daniel > > While entering S0iX we are getting following call flow: > > intel_runtime_resume followed by i915_drm_freeze > > and While resuming back from S0iX: > > i915_drm_thaw > > > > How can we share that wake control, gunit save/restore functionality? > > > > I am observing issue given below: > > > > > > Here is how I am achieving S0ix: > > 1. echo mem > /sys/power/state > > 2. (Assuming we have removed all wake_locks in /sys/powe/wake_lock) > > If device is runtime suspend it is getting resumed with > > runtime_resume > > 3. PM core triggers this sequence for each device > > (prepare, suspend, suspend_late, suspend_noirq) > > 4. Then pm_suspend happens for Gfx. > > > > After all devices are suspended S0i0->S0iR->S0i1->S0i2->S0i3 happens. > > Gunit gets power gated. > > > > While resuming back (with user action/power button press) > > 1. PM core triggers following sequenc for each device: > > (resume_noirq, resume_early, resume, complete) > > > > After call to i915_drm_thaw_early(called through resume_early), > > during i915_drm_thaw(called through resume) we were seeing following > > issue during ring initialization. > > This is happening since wake is not enabled and Gunit registers are not > > restored(which is done in runtime_resume but that path is not taken here > > since this is resume from pm_suspend) > > We have intel_runtime_pm_get/put calls in the suspend/resume paths which > should achieve this. Maybe they're not at the right place or not in the > right ordering, but they're there. So on platforms with runtime pm support > we _do_ call all the relevant runtime pm callbacks from system > suspend/resume. > -Daniel I am seeing following commit removed get/put calls from suspend resume paths: commit 395a5abbd97d7d76a7a26da52f33daebe279b3b3 Author: Jesse Barnes <jbarnes@virtuousgeek.org> Date: Wed Jun 18 09:52:56 2014 -0700 drm/i915: don't take runtime PM reference around freeze/thaw That ordering of get/put was also incorrect probably. Even though we do rpm_get before freeze after S0i3 we need to explicitely do wake contro/gunit save restore. In the current patch, the rpm handlers are called directly instead of (intel_runtime_pm_get/put). valleyview_runtime_suspend at the end of i915_drm_freeze and valleyview_runtime_resume at the beginning of i915_drm_thaw_early. Do I need to replace them with intel_display_power_get/put? Will DPM core allow rpm calls when system pm suspend/resume is in progress? > > <6>[ 3152.991399] PM: device[controlD64] driver[drm] resume enter > > <3>[ 3153.976759] [drm:stop_ring] *ERROR* render ring :timed out trying > > to stop ring > > <3>[ 3154.980061] [drm:stop_ring] *ERROR* render ring :timed out trying > > to stop ring > > <3>[ 3154.980067] [drm:init_ring_common] *ERROR* failed to set render > > ring head to zero ctl 00000000 head 00000000 tail 00000000 start > > 00000000 > > <3>[ 3155.983364] [drm:stop_ring] *ERROR* bsd ring :timed out trying to > > stop ring > > <3>[ 3156.986668] [drm:stop_ring] *ERROR* bsd ring :timed out trying to > > stop ring > > <3>[ 3156.986673] [drm:init_ring_common] *ERROR* failed to set bsd ring > > head to zero ctl 00000000 head 00000000 tail 00000000 start 00000000 > > <3>[ 3157.989982] [drm:stop_ring] *ERROR* render ring :timed out trying > > to stop ring
On Fri, 2014-08-08 at 15:54 +0530, Sagar Arun Kamble wrote: > On Fri, 2014-08-08 at 11:15 +0200, Daniel Vetter wrote: > > On Fri, Aug 08, 2014 at 02:29:38PM +0530, Sagar Arun Kamble wrote: > > > On Fri, 2014-08-08 at 09:42 +0200, Daniel Vetter wrote: > > > > On Fri, Aug 08, 2014 at 12:22:44PM +0530, Sagar Arun Kamble wrote: > > > > > Hi Daniel, > > > > > On Mon, 2014-08-04 at 10:07 +0200, Daniel Vetter wrote: > > > > > > On Fri, Aug 01, 2014 at 12:34:56PM +0530, sagar.a.kamble@intel.com wrote: > > > > > > > From: Sagar Kamble <sagar.a.kamble@intel.com> > > > > > > > @@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev) > > > > > > > > > > > > > > intel_display_set_init_power(dev_priv, false); > > > > > > > > > > > > > > - return 0; > > > > > > > + /* Save Gunit State and clear wake - Need to make sure > > > > > > > + * changes in vlv_runtime_suspend path don't impact this path */ > > > > > > > + if (IS_VALLEYVIEW(dev)) > > > > > > > + ret = vlv_runtime_suspend(dev_priv); > > > > > > > > > > > > Maybe I wasn't clear, but I absolutely don't want any IS_VLV additions to > > > > > > core resume/thaw code. This should be shovelled into the runtime pm > > > > > > handling code, which should be reused in the suspend/resume code. > > > > > This piece of code does not fit into any of the power well get/put path. > > > > > Its specific sequence that need to be followed in VLV when Gunit gets > > > > > power gated. So we have to keep this IS_VLV related functionality in > > > > > both runtime and pm suspend/resume. > > > > > > > > Well we support S0ix now. Which means our system suspend/resume code > > > > actually calls into the runtime pm code. So either that design is broken > > > > (and we need to fix this) or something else is amiss. Or we don't need > > > > this code any more. > > > > > > > > But duplicating it is not the right approach. And yeah the power domain > > > > stuff might not be the right place, I've just used that as a place-holder > > > > for all the runtime pm code we have. > > > > -Daniel > > > While entering S0iX we are getting following call flow: > > > intel_runtime_resume followed by i915_drm_freeze > > > and While resuming back from S0iX: > > > i915_drm_thaw > > > > > > How can we share that wake control, gunit save/restore functionality? > > > > > > I am observing issue given below: > > > > > > > > > Here is how I am achieving S0ix: > > > 1. echo mem > /sys/power/state > > > 2. (Assuming we have removed all wake_locks in /sys/powe/wake_lock) > > > If device is runtime suspend it is getting resumed with > > > runtime_resume > > > 3. PM core triggers this sequence for each device > > > (prepare, suspend, suspend_late, suspend_noirq) > > > 4. Then pm_suspend happens for Gfx. > > > > > > After all devices are suspended S0i0->S0iR->S0i1->S0i2->S0i3 happens. > > > Gunit gets power gated. > > > > > > While resuming back (with user action/power button press) > > > 1. PM core triggers following sequenc for each device: > > > (resume_noirq, resume_early, resume, complete) > > > > > > After call to i915_drm_thaw_early(called through resume_early), > > > during i915_drm_thaw(called through resume) we were seeing following > > > issue during ring initialization. > > > This is happening since wake is not enabled and Gunit registers are not > > > restored(which is done in runtime_resume but that path is not taken here > > > since this is resume from pm_suspend) > > > > We have intel_runtime_pm_get/put calls in the suspend/resume paths which > > should achieve this. Maybe they're not at the right place or not in the > > right ordering, but they're there. So on platforms with runtime pm support > > we _do_ call all the relevant runtime pm callbacks from system > > suspend/resume. > > -Daniel > I am seeing following commit removed get/put calls from suspend resume > paths: > commit 395a5abbd97d7d76a7a26da52f33daebe279b3b3 > Author: Jesse Barnes <jbarnes@virtuousgeek.org> > Date: Wed Jun 18 09:52:56 2014 -0700 > > drm/i915: don't take runtime PM reference around freeze/thaw > > > That ordering of get/put was also incorrect probably. Even though we do > rpm_get before freeze after S0i3 we need to explicitely do wake > contro/gunit save restore. > > > In the current patch, the rpm handlers are called directly instead of > (intel_runtime_pm_get/put). > > valleyview_runtime_suspend at the end of i915_drm_freeze and > valleyview_runtime_resume at the beginning of i915_drm_thaw_early. > > Do I need to replace them with intel_display_power_get/put? Will DPM > core allow rpm calls when system pm suspend/resume is in progress? No, you can't make the device runtime suspend while the system suspend handler runs since DPM takes an RPM reference before calling this handler. The reference will be dropped only after returning from the corresponding system resume handler. > > > <6>[ 3152.991399] PM: device[controlD64] driver[drm] resume enter > > > <3>[ 3153.976759] [drm:stop_ring] *ERROR* render ring :timed out trying > > > to stop ring > > > <3>[ 3154.980061] [drm:stop_ring] *ERROR* render ring :timed out trying > > > to stop ring > > > <3>[ 3154.980067] [drm:init_ring_common] *ERROR* failed to set render > > > ring head to zero ctl 00000000 head 00000000 tail 00000000 start > > > 00000000 > > > <3>[ 3155.983364] [drm:stop_ring] *ERROR* bsd ring :timed out trying to > > > stop ring > > > <3>[ 3156.986668] [drm:stop_ring] *ERROR* bsd ring :timed out trying to > > > stop ring > > > <3>[ 3156.986673] [drm:init_ring_common] *ERROR* failed to set bsd ring > > > head to zero ctl 00000000 head 00000000 tail 00000000 start 00000000 > > > <3>[ 3157.989982] [drm:stop_ring] *ERROR* render ring :timed out trying > > > to stop ring > >
On Fri, Aug 08, 2014 at 02:34:37PM +0300, Imre Deak wrote: > On Fri, 2014-08-08 at 15:54 +0530, Sagar Arun Kamble wrote: > > On Fri, 2014-08-08 at 11:15 +0200, Daniel Vetter wrote: > > > On Fri, Aug 08, 2014 at 02:29:38PM +0530, Sagar Arun Kamble wrote: > > > > On Fri, 2014-08-08 at 09:42 +0200, Daniel Vetter wrote: > > > > > On Fri, Aug 08, 2014 at 12:22:44PM +0530, Sagar Arun Kamble wrote: > > > > > > Hi Daniel, > > > > > > On Mon, 2014-08-04 at 10:07 +0200, Daniel Vetter wrote: > > > > > > > On Fri, Aug 01, 2014 at 12:34:56PM +0530, sagar.a.kamble@intel.com wrote: > > > > > > > > From: Sagar Kamble <sagar.a.kamble@intel.com> > > > > > > > > @@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev) > > > > > > > > > > > > > > > > intel_display_set_init_power(dev_priv, false); > > > > > > > > > > > > > > > > - return 0; > > > > > > > > + /* Save Gunit State and clear wake - Need to make sure > > > > > > > > + * changes in vlv_runtime_suspend path don't impact this path */ > > > > > > > > + if (IS_VALLEYVIEW(dev)) > > > > > > > > + ret = vlv_runtime_suspend(dev_priv); > > > > > > > > > > > > > > Maybe I wasn't clear, but I absolutely don't want any IS_VLV additions to > > > > > > > core resume/thaw code. This should be shovelled into the runtime pm > > > > > > > handling code, which should be reused in the suspend/resume code. > > > > > > This piece of code does not fit into any of the power well get/put path. > > > > > > Its specific sequence that need to be followed in VLV when Gunit gets > > > > > > power gated. So we have to keep this IS_VLV related functionality in > > > > > > both runtime and pm suspend/resume. > > > > > > > > > > Well we support S0ix now. Which means our system suspend/resume code > > > > > actually calls into the runtime pm code. So either that design is broken > > > > > (and we need to fix this) or something else is amiss. Or we don't need > > > > > this code any more. > > > > > > > > > > But duplicating it is not the right approach. And yeah the power domain > > > > > stuff might not be the right place, I've just used that as a place-holder > > > > > for all the runtime pm code we have. > > > > > -Daniel > > > > While entering S0iX we are getting following call flow: > > > > intel_runtime_resume followed by i915_drm_freeze > > > > and While resuming back from S0iX: > > > > i915_drm_thaw > > > > > > > > How can we share that wake control, gunit save/restore functionality? > > > > > > > > I am observing issue given below: > > > > > > > > > > > > Here is how I am achieving S0ix: > > > > 1. echo mem > /sys/power/state > > > > 2. (Assuming we have removed all wake_locks in /sys/powe/wake_lock) > > > > If device is runtime suspend it is getting resumed with > > > > runtime_resume > > > > 3. PM core triggers this sequence for each device > > > > (prepare, suspend, suspend_late, suspend_noirq) > > > > 4. Then pm_suspend happens for Gfx. > > > > > > > > After all devices are suspended S0i0->S0iR->S0i1->S0i2->S0i3 happens. > > > > Gunit gets power gated. > > > > > > > > While resuming back (with user action/power button press) > > > > 1. PM core triggers following sequenc for each device: > > > > (resume_noirq, resume_early, resume, complete) > > > > > > > > After call to i915_drm_thaw_early(called through resume_early), > > > > during i915_drm_thaw(called through resume) we were seeing following > > > > issue during ring initialization. > > > > This is happening since wake is not enabled and Gunit registers are not > > > > restored(which is done in runtime_resume but that path is not taken here > > > > since this is resume from pm_suspend) > > > > > > We have intel_runtime_pm_get/put calls in the suspend/resume paths which > > > should achieve this. Maybe they're not at the right place or not in the > > > right ordering, but they're there. So on platforms with runtime pm support > > > we _do_ call all the relevant runtime pm callbacks from system > > > suspend/resume. > > > -Daniel > > I am seeing following commit removed get/put calls from suspend resume > > paths: > > commit 395a5abbd97d7d76a7a26da52f33daebe279b3b3 > > Author: Jesse Barnes <jbarnes@virtuousgeek.org> > > Date: Wed Jun 18 09:52:56 2014 -0700 > > > > drm/i915: don't take runtime PM reference around freeze/thaw > > > > > > That ordering of get/put was also incorrect probably. Even though we do > > rpm_get before freeze after S0i3 we need to explicitely do wake > > contro/gunit save restore. > > > > > > In the current patch, the rpm handlers are called directly instead of > > (intel_runtime_pm_get/put). > > > > valleyview_runtime_suspend at the end of i915_drm_freeze and > > valleyview_runtime_resume at the beginning of i915_drm_thaw_early. > > > > Do I need to replace them with intel_display_power_get/put? Will DPM > > core allow rpm calls when system pm suspend/resume is in progress? > > No, you can't make the device runtime suspend while the system suspend > handler runs since DPM takes an RPM reference before calling this > handler. The reference will be dropped only after returning from the > corresponding system resume handler. Blergh, I always forget that little piece of nonsense. Then we just need to call the relevant stuff directly, but not the individual platform functions but the generic interfaces. -Daniel
Gn Fri, Aug 08, 2014 at 03:43:49PM +0200, Daniel Vetter wrote: > On Fri, Aug 08, 2014 at 02:34:37PM +0300, Imre Deak wrote: > > On Fri, 2014-08-08 at 15:54 +0530, Sagar Arun Kamble wrote: > > > On Fri, 2014-08-08 at 11:15 +0200, Daniel Vetter wrote: > > > > On Fri, Aug 08, 2014 at 02:29:38PM +0530, Sagar Arun Kamble wrote: > > > > > On Fri, 2014-08-08 at 09:42 +0200, Daniel Vetter wrote: > > > > > > On Fri, Aug 08, 2014 at 12:22:44PM +0530, Sagar Arun Kamble wrote: > > > > > > > Hi Daniel, > > > > > > > On Mon, 2014-08-04 at 10:07 +0200, Daniel Vetter wrote: > > > > > > > > On Fri, Aug 01, 2014 at 12:34:56PM +0530, sagar.a.kamble@intel.com wrote: > > > > > > > > > From: Sagar Kamble <sagar.a.kamble@intel.com> > > > > > > > > > @@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev) > > > > > > > > > > > > > > > > > > intel_display_set_init_power(dev_priv, false); > > > > > > > > > > > > > > > > > > - return 0; > > > > > > > > > + /* Save Gunit State and clear wake - Need to make sure > > > > > > > > > + * changes in vlv_runtime_suspend path don't impact this path */ > > > > > > > > > + if (IS_VALLEYVIEW(dev)) > > > > > > > > > + ret = vlv_runtime_suspend(dev_priv); > > > > > > > > > > > > > > > > Maybe I wasn't clear, but I absolutely don't want any IS_VLV additions to > > > > > > > > core resume/thaw code. This should be shovelled into the runtime pm > > > > > > > > handling code, which should be reused in the suspend/resume code. > > > > > > > This piece of code does not fit into any of the power well get/put path. > > > > > > > Its specific sequence that need to be followed in VLV when Gunit gets > > > > > > > power gated. So we have to keep this IS_VLV related functionality in > > > > > > > both runtime and pm suspend/resume. > > > > > > > > > > > > Well we support S0ix now. Which means our system suspend/resume code > > > > > > actually calls into the runtime pm code. So either that design is broken > > > > > > (and we need to fix this) or something else is amiss. Or we don't need > > > > > > this code any more. > > > > > > > > > > > > But duplicating it is not the right approach. And yeah the power domain > > > > > > stuff might not be the right place, I've just used that as a place-holder > > > > > > for all the runtime pm code we have. > > > > > > -Daniel > > > > > While entering S0iX we are getting following call flow: > > > > > intel_runtime_resume followed by i915_drm_freeze > > > > > and While resuming back from S0iX: > > > > > i915_drm_thaw > > > > > > > > > > How can we share that wake control, gunit save/restore functionality? > > > > > > > > > > I am observing issue given below: > > > > > > > > > > > > > > > Here is how I am achieving S0ix: > > > > > 1. echo mem > /sys/power/state > > > > > 2. (Assuming we have removed all wake_locks in /sys/powe/wake_lock) > > > > > If device is runtime suspend it is getting resumed with > > > > > runtime_resume > > > > > 3. PM core triggers this sequence for each device > > > > > (prepare, suspend, suspend_late, suspend_noirq) > > > > > 4. Then pm_suspend happens for Gfx. > > > > > > > > > > After all devices are suspended S0i0->S0iR->S0i1->S0i2->S0i3 happens. > > > > > Gunit gets power gated. > > > > > > > > > > While resuming back (with user action/power button press) > > > > > 1. PM core triggers following sequenc for each device: > > > > > (resume_noirq, resume_early, resume, complete) > > > > > > > > > > After call to i915_drm_thaw_early(called through resume_early), > > > > > during i915_drm_thaw(called through resume) we were seeing following > > > > > issue during ring initialization. > > > > > This is happening since wake is not enabled and Gunit registers are not > > > > > restored(which is done in runtime_resume but that path is not taken here > > > > > since this is resume from pm_suspend) > > > > > > > > We have intel_runtime_pm_get/put calls in the suspend/resume paths which > > > > should achieve this. Maybe they're not at the right place or not in the > > > > right ordering, but they're there. So on platforms with runtime pm support > > > > we _do_ call all the relevant runtime pm callbacks from system > > > > suspend/resume. > > > > -Daniel > > > I am seeing following commit removed get/put calls from suspend resume > > > paths: > > > commit 395a5abbd97d7d76a7a26da52f33daebe279b3b3 > > > Author: Jesse Barnes <jbarnes@virtuousgeek.org> > > > Date: Wed Jun 18 09:52:56 2014 -0700 > > > > > > drm/i915: don't take runtime PM reference around freeze/thaw > > > > > > > > > That ordering of get/put was also incorrect probably. Even though we do > > > rpm_get before freeze after S0i3 we need to explicitely do wake > > > contro/gunit save restore. > > > > > > > > > In the current patch, the rpm handlers are called directly instead of > > > (intel_runtime_pm_get/put). > > > > > > valleyview_runtime_suspend at the end of i915_drm_freeze and > > > valleyview_runtime_resume at the beginning of i915_drm_thaw_early. > > > > > > Do I need to replace them with intel_display_power_get/put? Will DPM > > > core allow rpm calls when system pm suspend/resume is in progress? > > > > No, you can't make the device runtime suspend while the system suspend > > handler runs since DPM takes an RPM reference before calling this > > handler. The reference will be dropped only after returning from the > > corresponding system resume handler. > > Blergh, I always forget that little piece of nonsense. Then we just need > to call the relevant stuff directly, but not the individual platform > functions but the generic interfaces. Ok, so I guess I have to unlazy to avoid looking like an incompetent idiot any longer ;-) So when Jesse added basic soix support in commit 8abdc17941c71b37311bb93876ac83dce58160c8 Author: Kristen Carlson Accardi <kristen@linux.intel.com> Date: Thu Jun 12 08:35:48 2014 -0700 drm/i915: make sure PC8 is enabled on suspend and disabled on resume v4 he just added a direct call to the relevant runtime pm function. Which isn't too cool really, but I merged it. The next platform that wants this (i.e. you) gets to refactor this so that we can get rid of the HSW/BDW checks in there and don't need to add anything else either. So essentially instead of calling platform hooks directly you shoulc call the relevant driver-internal interfaces/vfuncs. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 6c4b25c..385dc74 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -490,11 +490,16 @@ bool i915_semaphore_is_enabled(struct drm_device *dev) return true; } +static int vlv_runtime_suspend(struct drm_i915_private *dev_priv); +static int vlv_runtime_resume(struct drm_i915_private *dev_priv, + bool resume_from_s0ix); + static int i915_drm_freeze(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; struct drm_crtc *crtc; pci_power_t opregion_target_state; + int ret = 0; /* ignore lid events during suspend */ mutex_lock(&dev_priv->modeset_restore_lock); @@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev) intel_display_set_init_power(dev_priv, false); - return 0; + /* Save Gunit State and clear wake - Need to make sure + * changes in vlv_runtime_suspend path don't impact this path */ + if (IS_VALLEYVIEW(dev)) + ret = vlv_runtime_suspend(dev_priv); + + return ret; } int i915_suspend(struct drm_device *dev, pm_message_t state) @@ -610,6 +620,12 @@ void intel_console_resume(struct work_struct *work) static int i915_drm_thaw_early(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + int ret = 0; + + /* Restore Gunit State and allow wake - Need to make sure + * changes in vlv_runtime_resume path don't impact this path */ + if (IS_VALLEYVIEW(dev)) + ret = vlv_runtime_resume(dev_priv, true); if (IS_HASWELL(dev) || IS_BROADWELL(dev)) hsw_disable_pc8(dev_priv); @@ -618,7 +634,7 @@ static int i915_drm_thaw_early(struct drm_device *dev) intel_uncore_sanitize(dev); intel_power_domains_init_hw(dev_priv); - return 0; + return ret; } static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) @@ -1098,6 +1114,7 @@ static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv) s->gu_ctl0 = I915_READ(VLV_GU_CTL0); s->gu_ctl1 = I915_READ(VLV_GU_CTL1); s->clock_gate_dis2 = I915_READ(VLV_GUNIT_CLOCK_GATE2); + s->dpio_cfg_data = I915_READ(DPIO_CTL); /* * Not saving any of: @@ -1192,6 +1209,7 @@ static void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv) I915_WRITE(VLV_GU_CTL0, s->gu_ctl0); I915_WRITE(VLV_GU_CTL1, s->gu_ctl1); I915_WRITE(VLV_GUNIT_CLOCK_GATE2, s->clock_gate_dis2); + I915_WRITE(DPIO_CTL, s->dpio_cfg_data); } int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on) @@ -1291,6 +1309,8 @@ static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv) I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR); } +/* This function is used in system suspend path as well to utilize + * Gfx clock, Wake control, Gunit state save related functionaility */ static int vlv_runtime_suspend(struct drm_i915_private *dev_priv) { u32 mask; @@ -1331,7 +1351,12 @@ err1: return err; } -static int vlv_runtime_resume(struct drm_i915_private *dev_priv) +/* This function is used in system resume path as well to utilize + * Gfx clock, Wake control, Gunit state restore related functionaility. + * GEM and other initialization will differ which will be controlled by + * resume_from_s0ix variable */ +static int vlv_runtime_resume(struct drm_i915_private *dev_priv, + bool resume_from_s0ix) { struct drm_device *dev = dev_priv->dev; int err; @@ -1356,8 +1381,10 @@ static int vlv_runtime_resume(struct drm_i915_private *dev_priv) vlv_check_no_gt_access(dev_priv); - intel_init_clock_gating(dev); - i915_gem_restore_fences(dev); + if (!resume_from_s0ix) { + intel_init_clock_gating(dev); + i915_gem_restore_fences(dev); + } return ret; } @@ -1462,7 +1489,7 @@ static int intel_runtime_resume(struct device *device) } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { ret = hsw_runtime_resume(dev_priv); } else if (IS_VALLEYVIEW(dev)) { - ret = vlv_runtime_resume(dev_priv); + ret = vlv_runtime_resume(dev_priv, false); } else { WARN_ON(1); ret = -ENODEV; diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index d604f4f..3a836c9 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -910,6 +910,7 @@ struct vlv_s0ix_state { u32 gu_ctl0; u32 gu_ctl1; u32 clock_gate_dis2; + u32 dpio_cfg_data; }; struct intel_rps_ei {