| Message ID | 1361304703-2869-5-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 19, 2013 at 12:11:41PM -0800, Jesse Barnes wrote: > With the other bits in place, we can do this safely. > > v2: disable backlight on suspend to prevent premature enablement on resume > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/i915_drv.c | 12 +++++++++--- > drivers/gpu/drm/i915/intel_fb.c | 3 +++ > 2 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index c5b8c81..e76b038 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -492,9 +492,10 @@ static int i915_drm_freeze(struct drm_device *dev) > > cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work); > > - intel_modeset_disable(dev); As discussed in person last week, simply dropping this will probably kill S0i3 support. -Daniel > - > drm_irq_uninstall(dev); > + > + if (dev_priv->backlight) > + intel_panel_disable_backlight(dev); > } > > i915_save_state(dev); > @@ -569,9 +570,14 @@ static int __i915_drm_thaw(struct drm_device *dev) > mutex_unlock(&dev->struct_mutex); > > intel_modeset_init_hw(dev); > - intel_modeset_setup_hw_state(dev, false); > + > drm_irq_install(dev); > intel_hpd_init(dev); > + > + /* Resume the modeset for every activated CRTC */ > + drm_modeset_lock_all(dev); > + intel_modeset_setup_hw_state(dev, true); > + drm_modeset_unlock_all(dev); > } > > intel_opregion_init(dev); > diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c > index 1c510da..987bc33 100644 > --- a/drivers/gpu/drm/i915/intel_fb.c > +++ b/drivers/gpu/drm/i915/intel_fb.c > @@ -149,6 +149,9 @@ static int intelfb_create(struct intel_fbdev *ifbdev, > } > info->screen_size = size; > > + /* This driver doesn't need a VT switch to restore the mode on resume */ > + info->skip_vt_switch = true; > + > // memset(info->screen_base, 0, size); > > drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth); > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 18 Mar 2013 08:49:07 +0100 Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Feb 19, 2013 at 12:11:41PM -0800, Jesse Barnes wrote: > > With the other bits in place, we can do this safely. > > > > v2: disable backlight on suspend to prevent premature enablement on resume > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > --- > > drivers/gpu/drm/i915/i915_drv.c | 12 +++++++++--- > > drivers/gpu/drm/i915/intel_fb.c | 3 +++ > > 2 files changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index c5b8c81..e76b038 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -492,9 +492,10 @@ static int i915_drm_freeze(struct drm_device *dev) > > > > cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work); > > > > - intel_modeset_disable(dev); > > As discussed in person last week, simply dropping this will probably kill > S0i3 support. Not really, since DPMS will be off in that case too generally, but it does make testing harder. I think it just needs to be a low level call to crtc disable on each pipe, otherwise we'll zap the state we're trying to save.
On Mon, Mar 18, 2013 at 6:42 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > On Mon, 18 Mar 2013 08:49:07 +0100 > Daniel Vetter <daniel@ffwll.ch> wrote: > >> On Tue, Feb 19, 2013 at 12:11:41PM -0800, Jesse Barnes wrote: >> > With the other bits in place, we can do this safely. >> > >> > v2: disable backlight on suspend to prevent premature enablement on resume >> > >> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> >> > --- >> > drivers/gpu/drm/i915/i915_drv.c | 12 +++++++++--- >> > drivers/gpu/drm/i915/intel_fb.c | 3 +++ >> > 2 files changed, 12 insertions(+), 3 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> > index c5b8c81..e76b038 100644 >> > --- a/drivers/gpu/drm/i915/i915_drv.c >> > +++ b/drivers/gpu/drm/i915/i915_drv.c >> > @@ -492,9 +492,10 @@ static int i915_drm_freeze(struct drm_device *dev) >> > >> > cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work); >> > >> > - intel_modeset_disable(dev); >> >> As discussed in person last week, simply dropping this will probably kill >> S0i3 support. > > Not really, since DPMS will be off in that case too generally, but it > does make testing harder. Hm, where do we do that currently? Without fbcon I don't see it, and we can't really rely on userspace to get this right I guess ... > I think it just needs to be a low level call to crtc disable on each > pipe, otherwise we'll zap the state we're trying to save. That just reminded me that we also should restore the right dpms state I think. At least I'm not too sure whether we'll currently do that (and whether the modeset state tracker would catch it). Otoh dpms standby/suspend died with gen4 ;-) -Daniel
On Mon, 18 Mar 2013 19:50:37 +0100 Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Mar 18, 2013 at 6:42 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > On Mon, 18 Mar 2013 08:49:07 +0100 > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > >> On Tue, Feb 19, 2013 at 12:11:41PM -0800, Jesse Barnes wrote: > >> > With the other bits in place, we can do this safely. > >> > > >> > v2: disable backlight on suspend to prevent premature enablement on resume > >> > > >> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > >> > --- > >> > drivers/gpu/drm/i915/i915_drv.c | 12 +++++++++--- > >> > drivers/gpu/drm/i915/intel_fb.c | 3 +++ > >> > 2 files changed, 12 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > >> > index c5b8c81..e76b038 100644 > >> > --- a/drivers/gpu/drm/i915/i915_drv.c > >> > +++ b/drivers/gpu/drm/i915/i915_drv.c > >> > @@ -492,9 +492,10 @@ static int i915_drm_freeze(struct drm_device *dev) > >> > > >> > cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work); > >> > > >> > - intel_modeset_disable(dev); > >> > >> As discussed in person last week, simply dropping this will probably kill > >> S0i3 support. > > > > Not really, since DPMS will be off in that case too generally, but it > > does make testing harder. > > Hm, where do we do that currently? Without fbcon I don't see it, and > we can't really rely on userspace to get this right I guess ... I was assuming userspace would do it, but yes we shouldn't require that. > > I think it just needs to be a low level call to crtc disable on each > > pipe, otherwise we'll zap the state we're trying to save. > > That just reminded me that we also should restore the right dpms state > I think. At least I'm not too sure whether we'll currently do that > (and whether the modeset state tracker would catch it). Otoh dpms > standby/suspend died with gen4 ;-) Hm yeah haven't tested that at all. One typical kind of suspend will happen after DPMS off when the machine has been idle for some period. When it comes back up the user will probably want to see the display. But we don't have to enforce that on the kernel side; we can leave it to userspace.
On Tue, Mar 19, 2013 at 5:56 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: >> > I think it just needs to be a low level call to crtc disable on each >> > pipe, otherwise we'll zap the state we're trying to save. >> >> That just reminded me that we also should restore the right dpms state >> I think. At least I'm not too sure whether we'll currently do that >> (and whether the modeset state tracker would catch it). Otoh dpms >> standby/suspend died with gen4 ;-) > > Hm yeah haven't tested that at all. One typical kind of suspend will > happen after DPMS off when the machine has been idle for some period. > When it comes back up the user will probably want to see the display. > But we don't have to enforce that on the kernel side; we can leave it > to userspace. Note that this isn't about dpms state in general, we'll take care of that. The problem is with intermediate dpms levels, which requires us to keep the pipe partially running. If you force-restore such a thing we'll end up at dpms ON. Which isn't quite what we want. Otoh it's old hw, so I don't think we need to spill too many brain cycles on this issue. But if we do fix it, I think we should implement proper support to read out that hw state and cross-check it - otherwise it's pretty much guaranteed to be broken. -Daniel
On Tue, Mar 19, 2013 at 06:13:09PM +0100, Daniel Vetter wrote: > On Tue, Mar 19, 2013 at 5:56 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > >> > I think it just needs to be a low level call to crtc disable on each > >> > pipe, otherwise we'll zap the state we're trying to save. > >> > >> That just reminded me that we also should restore the right dpms state > >> I think. At least I'm not too sure whether we'll currently do that > >> (and whether the modeset state tracker would catch it). Otoh dpms > >> standby/suspend died with gen4 ;-) > > > > Hm yeah haven't tested that at all. One typical kind of suspend will > > happen after DPMS off when the machine has been idle for some period. > > When it comes back up the user will probably want to see the display. > > But we don't have to enforce that on the kernel side; we can leave it > > to userspace. > > Note that this isn't about dpms state in general, we'll take care of > that. The problem is with intermediate dpms levels, which requires us > to keep the pipe partially running. If you force-restore such a thing > we'll end up at dpms ON. Which isn't quite what we want. > > Otoh it's old hw, so I don't think we need to spill too many brain > cycles on this issue. But if we do fix it, I think we should implement > proper support to read out that hw state and cross-check it - > otherwise it's pretty much guaranteed to be broken. Ajax just submitted a patch to fix restoring of intermediate dpms levels, so we should be good here. Another idea for safe/restore around suspend which should just work is loop over all connectors and adjust the dpms state (and safe just that piece somewhere). That way we get nice implicit coverag of our dpms code while doing suspends and suspend doesn't need anything in addition infrastructure wise. The only downside is that we need to make the power wells stuff work with dpms, too. But that's a requirement, anyway. Now the real reason for writing this mail: 3.10 feature merge is drawing to a close (in about a month I think) and imo we should get switchless resume in a few weeks ahead. That way we can give it a decent beating before it reaches Linus' upstream git. And I like switchless resume a lot. So what's your plan here? -Daniel
On Tue, 26 Mar 2013 13:14:48 +0100 Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Mar 19, 2013 at 06:13:09PM +0100, Daniel Vetter wrote: > > On Tue, Mar 19, 2013 at 5:56 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > >> > I think it just needs to be a low level call to crtc disable on each > > >> > pipe, otherwise we'll zap the state we're trying to save. > > >> > > >> That just reminded me that we also should restore the right dpms state > > >> I think. At least I'm not too sure whether we'll currently do that > > >> (and whether the modeset state tracker would catch it). Otoh dpms > > >> standby/suspend died with gen4 ;-) > > > > > > Hm yeah haven't tested that at all. One typical kind of suspend will > > > happen after DPMS off when the machine has been idle for some period. > > > When it comes back up the user will probably want to see the display. > > > But we don't have to enforce that on the kernel side; we can leave it > > > to userspace. > > > > Note that this isn't about dpms state in general, we'll take care of > > that. The problem is with intermediate dpms levels, which requires us > > to keep the pipe partially running. If you force-restore such a thing > > we'll end up at dpms ON. Which isn't quite what we want. > > > > Otoh it's old hw, so I don't think we need to spill too many brain > > cycles on this issue. But if we do fix it, I think we should implement > > proper support to read out that hw state and cross-check it - > > otherwise it's pretty much guaranteed to be broken. > > Ajax just submitted a patch to fix restoring of intermediate dpms levels, > so we should be good here. Another idea for safe/restore around suspend > which should just work is loop over all connectors and adjust the dpms > state (and safe just that piece somewhere). That way we get nice implicit > coverag of our dpms code while doing suspends and suspend doesn't need > anything in addition infrastructure wise. > > The only downside is that we need to make the power wells stuff work with > dpms, too. But that's a requirement, anyway. > > Now the real reason for writing this mail: 3.10 feature merge is drawing > to a close (in about a month I think) and imo we should get switchless > resume in a few weeks ahead. That way we can give it a decent beating > before it reaches Linus' upstream git. And I like switchless resume a lot. > > So what's your plan here? What's left? Just dealing with the shut down at suspend time (so that suspend testing and RTD3 works) and Paulo's issue? I'm not sure what to do about the latter, but I can spin up a new patch with the shutdown bits in place.
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index c5b8c81..e76b038 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -492,9 +492,10 @@ static int i915_drm_freeze(struct drm_device *dev) cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work); - intel_modeset_disable(dev); - drm_irq_uninstall(dev); + + if (dev_priv->backlight) + intel_panel_disable_backlight(dev); } i915_save_state(dev); @@ -569,9 +570,14 @@ static int __i915_drm_thaw(struct drm_device *dev) mutex_unlock(&dev->struct_mutex); intel_modeset_init_hw(dev); - intel_modeset_setup_hw_state(dev, false); + drm_irq_install(dev); intel_hpd_init(dev); + + /* Resume the modeset for every activated CRTC */ + drm_modeset_lock_all(dev); + intel_modeset_setup_hw_state(dev, true); + drm_modeset_unlock_all(dev); } intel_opregion_init(dev); diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c index 1c510da..987bc33 100644 --- a/drivers/gpu/drm/i915/intel_fb.c +++ b/drivers/gpu/drm/i915/intel_fb.c @@ -149,6 +149,9 @@ static int intelfb_create(struct intel_fbdev *ifbdev, } info->screen_size = size; + /* This driver doesn't need a VT switch to restore the mode on resume */ + info->skip_vt_switch = true; + // memset(info->screen_base, 0, size); drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
With the other bits in place, we can do this safely. v2: disable backlight on suspend to prevent premature enablement on resume Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/i915_drv.c | 12 +++++++++--- drivers/gpu/drm/i915/intel_fb.c | 3 +++ 2 files changed, 12 insertions(+), 3 deletions(-)