| Message ID | 1386490966-20678-1-git-send-email-deepak.s@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Dec 08, 2013 at 01:52:46PM +0530, deepak.s@intel.com wrote: > From: Deepak S <deepak.s@intel.com> > > Instead of waiting for HW to bringup the wells, We force the wells up > before disabling RC6. This is to avoid any register access when wells > are down. > > Signed-off-by: Deepak S <deepak.s@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 2e1340f..089712a 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3661,6 +3661,12 @@ static void gen6_disable_rps(struct drm_device *dev) > static void valleyview_disable_rps(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > + unsigned long irqflags; > + > + /* We need to bring up the wells before disabling the RC6 */ > + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > + dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL); > + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); What precisely does this fix? All our register access is already wrapped in get/put calls, so I'm a bit confused ... Is this to work around hw issues, or is this what the bios expects when going into suspend? -Daniel > > I915_WRITE(GEN6_RC_CONTROL, 0); > > -- > 1.8.4.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>What precisely does this fix? All our register access is already wrapped in get/put calls, so I'm a bit confused ... Is this to work around hw issues, or is this what the bios expects when going into suspend? Yes Daniel, this was sequence recommended when going into suspend/Resume path. -----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Monday, December 9, 2013 1:31 PM To: S, Deepak Cc: intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915: Bring UP Power Wells before disabling RC6. On Sun, Dec 08, 2013 at 01:52:46PM +0530, deepak.s@intel.com wrote: > From: Deepak S <deepak.s@intel.com> > > Instead of waiting for HW to bringup the wells, We force the wells up > before disabling RC6. This is to avoid any register access when wells > are down. > > Signed-off-by: Deepak S <deepak.s@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > b/drivers/gpu/drm/i915/intel_pm.c index 2e1340f..089712a 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3661,6 +3661,12 @@ static void gen6_disable_rps(struct drm_device > *dev) static void valleyview_disable_rps(struct drm_device *dev) { > struct drm_i915_private *dev_priv = dev->dev_private; > + unsigned long irqflags; > + > + /* We need to bring up the wells before disabling the RC6 */ > + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > + dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL); > + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); What precisely does this fix? All our register access is already wrapped in get/put calls, so I'm a bit confused ... Is this to work around hw issues, or is this what the bios expects when going into suspend? -Daniel > > I915_WRITE(GEN6_RC_CONTROL, 0); > > -- > 1.8.4.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, Dec 09, 2013 at 02:14:03PM +0000, S, Deepak wrote: > >What precisely does this fix? All our register access is already wrapped in get/put calls, so I'm a bit confused ... Is this to work around hw issues, or is this what the bios expects when going into suspend? > > Yes Daniel, this was sequence recommended when going into suspend/Resume path. So the BIOS falls over if we don't do this? In that case I think we need to reword the code comment to say that we're doing this for the BIOS. Otherwise someone will remove this again, since our own code surely doesn't need it. -Daniel > > -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter > Sent: Monday, December 9, 2013 1:31 PM > To: S, Deepak > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915: Bring UP Power Wells before disabling RC6. > > On Sun, Dec 08, 2013 at 01:52:46PM +0530, deepak.s@intel.com wrote: > > From: Deepak S <deepak.s@intel.com> > > > > Instead of waiting for HW to bringup the wells, We force the wells up > > before disabling RC6. This is to avoid any register access when wells > > are down. > > > > Signed-off-by: Deepak S <deepak.s@intel.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > b/drivers/gpu/drm/i915/intel_pm.c index 2e1340f..089712a 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3661,6 +3661,12 @@ static void gen6_disable_rps(struct drm_device > > *dev) static void valleyview_disable_rps(struct drm_device *dev) { > > struct drm_i915_private *dev_priv = dev->dev_private; > > + unsigned long irqflags; > > + > > + /* We need to bring up the wells before disabling the RC6 */ > > + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > + dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL); > > + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > > What precisely does this fix? All our register access is already wrapped in get/put calls, so I'm a bit confused ... Is this to work around hw issues, or is this what the bios expects when going into suspend? > -Daniel > > > > I915_WRITE(GEN6_RC_CONTROL, 0); > > > > -- > > 1.8.4.2 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
We faced some issue for not following the sequence. I will add proper commit message and send it for review. -Deepak -----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Monday, December 9, 2013 10:49 PM To: S, Deepak Cc: Daniel Vetter; intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915: Bring UP Power Wells before disabling RC6. On Mon, Dec 09, 2013 at 02:14:03PM +0000, S, Deepak wrote: > >What precisely does this fix? All our register access is already wrapped in get/put calls, so I'm a bit confused ... Is this to work around hw issues, or is this what the bios expects when going into suspend? > > Yes Daniel, this was sequence recommended when going into suspend/Resume path. So the BIOS falls over if we don't do this? In that case I think we need to reword the code comment to say that we're doing this for the BIOS. Otherwise someone will remove this again, since our own code surely doesn't need it. -Daniel > > -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of > Daniel Vetter > Sent: Monday, December 9, 2013 1:31 PM > To: S, Deepak > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915: Bring UP Power Wells before disabling RC6. > > On Sun, Dec 08, 2013 at 01:52:46PM +0530, deepak.s@intel.com wrote: > > From: Deepak S <deepak.s@intel.com> > > > > Instead of waiting for HW to bringup the wells, We force the wells > > up before disabling RC6. This is to avoid any register access when > > wells are down. > > > > Signed-off-by: Deepak S <deepak.s@intel.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > b/drivers/gpu/drm/i915/intel_pm.c index 2e1340f..089712a 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3661,6 +3661,12 @@ static void gen6_disable_rps(struct > > drm_device > > *dev) static void valleyview_disable_rps(struct drm_device *dev) { > > struct drm_i915_private *dev_priv = dev->dev_private; > > + unsigned long irqflags; > > + > > + /* We need to bring up the wells before disabling the RC6 */ > > + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > + dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL); > > + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > > What precisely does this fix? All our register access is already wrapped in get/put calls, so I'm a bit confused ... Is this to work around hw issues, or is this what the bios expects when going into suspend? > -Daniel > > > > I915_WRITE(GEN6_RC_CONTROL, 0); > > > > -- > > 1.8.4.2 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 2e1340f..089712a 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3661,6 +3661,12 @@ static void gen6_disable_rps(struct drm_device *dev) static void valleyview_disable_rps(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + unsigned long irqflags; + + /* We need to bring up the wells before disabling the RC6 */ + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); + dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL); + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); I915_WRITE(GEN6_RC_CONTROL, 0);