Re: [Intel-gfx] [PATCH 4/4] drm/i915: make sure PC8 is enabled on suspend and disabled on resume
On Thu, 2014-05-29 at 14:11 -0700, Jesse Barnes wrote:
> From: Kristen Carlson Accardi
>
> This matches the runtime suspend paths and allows the system to enter
> the lowest power mode at freeze time.
>
> Signed-off-by: Kristen Carlson Accardi
> Signed-off-by: Jesse Barnes
> ---
> drivers/gpu/drm/i915/i915_drv.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b6211d7..24dc856 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev)
>
> intel_display_set_init_power(dev_priv, false);
>
> + if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> + hsw_enable_pc8(dev_priv);
> +
> return 0;
> }
>
> @@ -618,6 +621,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool
> restore_gtt_mappings)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> + if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> + hsw_disable_pc8(dev_priv);
I would put this before we access any of the HW regs in thaw_early() and
correspondingly the above call to hsw_enable_pc8() to suspend_late()
before we call pci_disable_device().
With that change this is:
Reviewed-by: Imre Deak
> +
> if (drm_core_check_feature(dev, DRIVER_MODESET) &&
> restore_gtt_mappings) {
> mutex_lock(&dev->struct_mutex);
signature.asc
Description: This is a digitally signed message part
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: make sure PC8 is enabled on suspend and disabled on resume
On Fri, 30 May 2014 16:37:53 +0300
Imre Deak wrote:
> On Thu, 2014-05-29 at 14:11 -0700, Jesse Barnes wrote:
> > From: Kristen Carlson Accardi
> >
> > This matches the runtime suspend paths and allows the system to enter
> > the lowest power mode at freeze time.
> >
> > Signed-off-by: Kristen Carlson Accardi
> > Signed-off-by: Jesse Barnes
> > ---
> > drivers/gpu/drm/i915/i915_drv.c | 6 ++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index b6211d7..24dc856 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev)
> >
> > intel_display_set_init_power(dev_priv, false);
> >
> > + if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > + hsw_enable_pc8(dev_priv);
> > +
> > return 0;
> > }
> >
> > @@ -618,6 +621,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool
> > restore_gtt_mappings)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> >
> > + if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > + hsw_disable_pc8(dev_priv);
>
> I would put this before we access any of the HW regs in thaw_early() and
> correspondingly the above call to hsw_enable_pc8() to suspend_late()
> before we call pci_disable_device().
>
> With that change this is:
> Reviewed-by: Imre Deak
For the thaw side I think that makes sense.
But for the freeze side, putting it in suspend_late won't get us the
freeze behavior we want. I think Rafael and Kristen are thinking of
re-using the freeze infrastructure for some kind of connected standby
feature, where most stuff is frozen, but the system isn't in S3 or S4,
so we need the enable_pc8 call in the _freeze path as well.
Rafael, is that correct?
I'll add a late_freeze and put it there instead, so it doesn't pollute
the S3 suspend path.
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: make sure PC8 is enabled on suspend and disabled on resume
On Friday, May 30, 2014 11:29:15 AM Jesse Barnes wrote:
> On Fri, 30 May 2014 16:37:53 +0300
> Imre Deak wrote:
>
> > On Thu, 2014-05-29 at 14:11 -0700, Jesse Barnes wrote:
> > > From: Kristen Carlson Accardi
> > >
> > > This matches the runtime suspend paths and allows the system to enter
> > > the lowest power mode at freeze time.
> > >
> > > Signed-off-by: Kristen Carlson Accardi
> > > Signed-off-by: Jesse Barnes
> > > ---
> > > drivers/gpu/drm/i915/i915_drv.c | 6 ++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > b/drivers/gpu/drm/i915/i915_drv.c
> > > index b6211d7..24dc856 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev)
> > >
> > > intel_display_set_init_power(dev_priv, false);
> > >
> > > + if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > > + hsw_enable_pc8(dev_priv);
> > > +
> > > return 0;
> > > }
> > >
> > > @@ -618,6 +621,9 @@ static int __i915_drm_thaw(struct drm_device *dev,
> > > bool restore_gtt_mappings)
> > > {
> > > struct drm_i915_private *dev_priv = dev->dev_private;
> > >
> > > + if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > > + hsw_disable_pc8(dev_priv);
> >
> > I would put this before we access any of the HW regs in thaw_early() and
> > correspondingly the above call to hsw_enable_pc8() to suspend_late()
> > before we call pci_disable_device().
> >
> > With that change this is:
> > Reviewed-by: Imre Deak
>
> For the thaw side I think that makes sense.
>
> But for the freeze side, putting it in suspend_late won't get us the
> freeze behavior we want. I think Rafael and Kristen are thinking of
> re-using the freeze infrastructure for some kind of connected standby
> feature, where most stuff is frozen, but the system isn't in S3 or S4,
> so we need the enable_pc8 call in the _freeze path as well.
>
> Rafael, is that correct?
No, it isn't. The .freeze()/.thaw() callbacks are hibernation-specific.
There are no plans for using this in PM beyond hibernation.
What we're going to use are .suspend/_late/_noirq() and the corresponding
resume callbacks and runtime PM.
> I'll add a late_freeze and put it there instead, so it doesn't pollute
> the S3 suspend path.
The freeze/thaw stuff need not do any PM BTW, it just needs to quiesce the
device to prevent it from doing DMA etc and then bring it back to life.
Rafael
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: make sure PC8 is enabled on suspend and disabled on resume
On Fri, 30 May 2014 23:12:45 +0200
"Rafael J. Wysocki" wrote:
> On Friday, May 30, 2014 11:29:15 AM Jesse Barnes wrote:
> > On Fri, 30 May 2014 16:37:53 +0300
> > Imre Deak wrote:
> >
> > > On Thu, 2014-05-29 at 14:11 -0700, Jesse Barnes wrote:
> > > > From: Kristen Carlson Accardi
> > > >
> > > > This matches the runtime suspend paths and allows the system to enter
> > > > the lowest power mode at freeze time.
> > > >
> > > > Signed-off-by: Kristen Carlson Accardi
> > > > Signed-off-by: Jesse Barnes
> > > > ---
> > > > drivers/gpu/drm/i915/i915_drv.c | 6 ++
> > > > 1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > > b/drivers/gpu/drm/i915/i915_drv.c
> > > > index b6211d7..24dc856 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev)
> > > >
> > > > intel_display_set_init_power(dev_priv, false);
> > > >
> > > > + if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > > > + hsw_enable_pc8(dev_priv);
> > > > +
> > > > return 0;
> > > > }
> > > >
> > > > @@ -618,6 +621,9 @@ static int __i915_drm_thaw(struct drm_device *dev,
> > > > bool restore_gtt_mappings)
> > > > {
> > > > struct drm_i915_private *dev_priv = dev->dev_private;
> > > >
> > > > + if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > > > + hsw_disable_pc8(dev_priv);
> > >
> > > I would put this before we access any of the HW regs in thaw_early() and
> > > correspondingly the above call to hsw_enable_pc8() to suspend_late()
> > > before we call pci_disable_device().
> > >
> > > With that change this is:
> > > Reviewed-by: Imre Deak
> >
> > For the thaw side I think that makes sense.
> >
> > But for the freeze side, putting it in suspend_late won't get us the
> > freeze behavior we want. I think Rafael and Kristen are thinking of
> > re-using the freeze infrastructure for some kind of connected standby
> > feature, where most stuff is frozen, but the system isn't in S3 or S4,
> > so we need the enable_pc8 call in the _freeze path as well.
> >
> > Rafael, is that correct?
>
> No, it isn't. The .freeze()/.thaw() callbacks are hibernation-specific.
> There are no plans for using this in PM beyond hibernation.
>
> What we're going to use are .suspend/_late/_noirq() and the corresponding
> resume callbacks and runtime PM.
>
> > I'll add a late_freeze and put it there instead, so it doesn't pollute
> > the S3 suspend path.
>
> The freeze/thaw stuff need not do any PM BTW, it just needs to quiesce the
> device to prevent it from doing DMA etc and then bring it back to life.
Ok thanks. Kristen corrected me on IRC too. The latest patch I sent
should do what we want then, now that I've removed the freeze_late
function and put our PC8 enable in suspend_late like Imre suggested
initially.
--
Jesse Barnes, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: make sure PC8 is enabled on suspend and disabled on resume
On Thu, May 29, 2014 at 02:11:37PM -0700, Jesse Barnes wrote:
> From: Kristen Carlson Accardi
>
> This matches the runtime suspend paths and allows the system to enter
> the lowest power mode at freeze time.
>
> Signed-off-by: Kristen Carlson Accardi
> Signed-off-by: Jesse Barnes
pc8 is fully subsumed into runtime pm by now. Do we _really_ still need
this?
-Daniel
> ---
> drivers/gpu/drm/i915/i915_drv.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b6211d7..24dc856 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev)
>
> intel_display_set_init_power(dev_priv, false);
>
> + if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> + hsw_enable_pc8(dev_priv);
> +
> return 0;
> }
>
> @@ -618,6 +621,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool
> restore_gtt_mappings)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> + if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> + hsw_disable_pc8(dev_priv);
> +
> if (drm_core_check_feature(dev, DRIVER_MODESET) &&
> restore_gtt_mappings) {
> mutex_lock(&dev->struct_mutex);
> --
> 1.9.1
>
> ___
> 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: make sure PC8 is enabled on suspend and disabled on resume
On Mon, 2014-06-02 at 10:45 +0200, Daniel Vetter wrote:
> On Thu, May 29, 2014 at 02:11:37PM -0700, Jesse Barnes wrote:
> > From: Kristen Carlson Accardi
> >
> > This matches the runtime suspend paths and allows the system to enter
> > the lowest power mode at freeze time.
> >
> > Signed-off-by: Kristen Carlson Accardi
> > Signed-off-by: Jesse Barnes
>
> pc8 is fully subsumed into runtime pm by now. Do we _really_ still need
> this?
Yes, since the system suspend/resume handlers are called with an RPM ref
held and thus PC8 disabled.
--Imre
> -Daniel
>
> > ---
> > drivers/gpu/drm/i915/i915_drv.c | 6 ++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index b6211d7..24dc856 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev)
> >
> > intel_display_set_init_power(dev_priv, false);
> >
> > + if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > + hsw_enable_pc8(dev_priv);
> > +
> > return 0;
> > }
> >
> > @@ -618,6 +621,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool
> > restore_gtt_mappings)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> >
> > + if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > + hsw_disable_pc8(dev_priv);
> > +
> > if (drm_core_check_feature(dev, DRIVER_MODESET) &&
> > restore_gtt_mappings) {
> > mutex_lock(&dev->struct_mutex);
> > --
> > 1.9.1
> >
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
signature.asc
Description: This is a digitally signed message part
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: make sure PC8 is enabled on suspend and disabled on resume
On Mon, Jun 02, 2014 at 02:37:35PM +0300, Imre Deak wrote: > On Mon, 2014-06-02 at 10:45 +0200, Daniel Vetter wrote: > > On Thu, May 29, 2014 at 02:11:37PM -0700, Jesse Barnes wrote: > > > From: Kristen Carlson Accardi > > > > > > This matches the runtime suspend paths and allows the system to enter > > > the lowest power mode at freeze time. > > > > > > Signed-off-by: Kristen Carlson Accardi > > > Signed-off-by: Jesse Barnes > > > > pc8 is fully subsumed into runtime pm by now. Do we _really_ still need > > this? > > Yes, since the system suspend/resume handlers are called with an RPM ref > held and thus PC8 disabled. But doesn't patch 1 try to fix that? Imo we should have this here but instead go through highl-level the runtime pm functions to shut off the chip on all platforms. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: make sure PC8 is enabled on suspend and disabled on resume
On Mon, 2014-06-02 at 17:32 +0200, Daniel Vetter wrote: > On Mon, Jun 02, 2014 at 02:37:35PM +0300, Imre Deak wrote: > > On Mon, 2014-06-02 at 10:45 +0200, Daniel Vetter wrote: > > > On Thu, May 29, 2014 at 02:11:37PM -0700, Jesse Barnes wrote: > > > > From: Kristen Carlson Accardi > > > > > > > > This matches the runtime suspend paths and allows the system to enter > > > > the lowest power mode at freeze time. > > > > > > > > Signed-off-by: Kristen Carlson Accardi > > > > Signed-off-by: Jesse Barnes > > > > > > pc8 is fully subsumed into runtime pm by now. Do we _really_ still need > > > this? > > > > Yes, since the system suspend/resume handlers are called with an RPM ref > > held and thus PC8 disabled. > > But doesn't patch 1 try to fix that? That only disables the display side, but we won't disable PC8 until the RPM suspend handler is called. And that won't happen because the last RPM ref is held by the DPM framework for the duration of system suspend/resume handlers. > Imo we should have this here but instead go through highl-level the runtime > pm functions to shut off the chip on all platforms. After the planned refactoring we could have a low-level function that we can call both from here and the runtime PM path, but until that happens we need to do this here directly. --Imre signature.asc Description: This is a digitally signed message part ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: make sure PC8 is enabled on suspend and disabled on resume
On Mon, Jun 02, 2014 at 06:57:18PM +0300, Imre Deak wrote: > On Mon, 2014-06-02 at 17:32 +0200, Daniel Vetter wrote: > > On Mon, Jun 02, 2014 at 02:37:35PM +0300, Imre Deak wrote: > > > On Mon, 2014-06-02 at 10:45 +0200, Daniel Vetter wrote: > > > > On Thu, May 29, 2014 at 02:11:37PM -0700, Jesse Barnes wrote: > > > > > From: Kristen Carlson Accardi > > > > > > > > > > This matches the runtime suspend paths and allows the system to enter > > > > > the lowest power mode at freeze time. > > > > > > > > > > Signed-off-by: Kristen Carlson Accardi > > > > > Signed-off-by: Jesse Barnes > > > > > > > > pc8 is fully subsumed into runtime pm by now. Do we _really_ still need > > > > this? > > > > > > Yes, since the system suspend/resume handlers are called with an RPM ref > > > held and thus PC8 disabled. > > > > But doesn't patch 1 try to fix that? > > That only disables the display side, but we won't disable PC8 until the > RPM suspend handler is called. And that won't happen because the last > RPM ref is held by the DPM framework for the duration of system > suspend/resume handlers. Yeah, there's discussion going on to make system suspend be optionally based upon runtime pm in the core. Atm that's not possible since the system wakes up everyone to suspend them. > > Imo we should have this here but instead go through highl-level the runtime > > pm functions to shut off the chip on all platforms. > > After the planned refactoring we could have a low-level function that we > can call both from here and the runtime PM path, but until that happens > we need to do this here directly. Yeah, that's what I'm actually aiming for - we should be able to call the runtime pm suspend code from the system suspend code and share pretty much all the code. The sequence I'm thinking of would be for system suspend: 1. Stop everything we need to stop (gpu, display, rps, ...) to be able to enter runtime pm. 2. Check for leaked references. This might be tricky because audio. 3. Call runtime pm suspend hooks. Resume would be the same, but inverted. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
