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@xxxxxxxxx wrote:
> > From: Sagar Kamble <sagar.a.kamble@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Change-Id: I15cfdeeec9c976d9839bb281f809664f4a0c78a2
> > Signed-off-by: Sagar Kamble <sagar.a.kamble@xxxxxxxxx>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
