Message ID | 1432717471-7879-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 05/27/2015 11:04 AM, Daniel Vetter wrote: > In > > commit 9cba5efab5a8145ae6c52ea273553f069c294482 > Author: Mario Kleiner <mario.kleiner.de@gmail.com> > Date: Tue Jul 29 02:36:44 2014 +0200 > > drm/nouveau: Dis/Enable vblank irqs during suspend/resume > > drm_vblank_on/off calls where added around suspend/resume to make sure > vblank stay doesn't go boom over that transition. But nouveau already > used drm_vblank_pre/post_modeset over modesets. Instead use > drm_vblank_on/off everyhwere. The slight change here is that after > _off drm_vblank_get will refuse to work right away, but nouveau > doesn't seem to depend upon that anywhere outside of the pageflip > paths. > > The longer-term plan here is to switch all kms drivers to > drm_vblank_on/off so that common code like pending event cleanup can > be done there, while drm_vblank_pre/post_modeset will be purely > drm internal for the old UMS ioctl. > > Note that the drm_vblank_off still seems required in the suspend path > since nouveau doesn't explicitly disable crtcs. But on the resume side > drm_helper_resume_force_mode should end up calling drm_vblank_on > through the nouveau crtc hooks already. Hence remove the call in the > resume code. > > Cc: Mario Kleiner <mario.kleiner.de@gmail.com> > Cc: Ben Skeggs <bskeggs@redhat.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/nouveau/dispnv04/crtc.c | 4 ++-- > drivers/gpu/drm/nouveau/nouveau_display.c | 4 ---- > 2 files changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c > index 3d96b49fe662..dab24066fa21 100644 > --- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c > +++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c > @@ -708,7 +708,7 @@ static void nv_crtc_prepare(struct drm_crtc *crtc) > if (nv_two_heads(dev)) > NVSetOwner(dev, nv_crtc->index); > > - drm_vblank_pre_modeset(dev, nv_crtc->index); > + drm_vblank_off(dev, nv_crtc->index); > funcs->dpms(crtc, DRM_MODE_DPMS_OFF); > > NVBlankScreen(dev, nv_crtc->index, true); > @@ -740,7 +740,7 @@ static void nv_crtc_commit(struct drm_crtc *crtc) > #endif > > funcs->dpms(crtc, DRM_MODE_DPMS_ON); > - drm_vblank_post_modeset(dev, nv_crtc->index); > + drm_vblank_on(dev, nv_crtc->index); > } The above hunk is probably correct, but i couldn't test it without sufficiently old pre-nv 50 hardware. > > static void nv_crtc_destroy(struct drm_crtc *crtc) > diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c > index 8670d90cdc11..d824023f9fc6 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_display.c > +++ b/drivers/gpu/drm/nouveau/nouveau_display.c > @@ -620,10 +620,6 @@ nouveau_display_resume(struct drm_device *dev, bool runtime) > nv_crtc->lut.depth = 0; > } > > - /* Make sure that drm and hw vblank irqs get resumed if needed. */ > - for (head = 0; head < dev->mode_config.num_crtc; head++) > - drm_vblank_on(dev, head); > - > /* This should ensure we don't hit a locking problem when someone > * wakes us up via a connector. We should never go into suspend > * while the display is on anyways. > Tested this one and this hunk breaks suspend/resume. After a suspend/resume cycle, all OpenGL apps and composited desktop are dead, as the core can't get any vblank irq's enabled anymore. So the drm_vblank_on() is still needed here. thanks, -mario
On Fri, May 29, 2015 at 06:50:06PM +0200, Mario Kleiner wrote: > On 05/27/2015 11:04 AM, Daniel Vetter wrote: > >In > > > >commit 9cba5efab5a8145ae6c52ea273553f069c294482 > >Author: Mario Kleiner <mario.kleiner.de@gmail.com> > >Date: Tue Jul 29 02:36:44 2014 +0200 > > > > drm/nouveau: Dis/Enable vblank irqs during suspend/resume > > > >drm_vblank_on/off calls where added around suspend/resume to make sure > >vblank stay doesn't go boom over that transition. But nouveau already > >used drm_vblank_pre/post_modeset over modesets. Instead use > >drm_vblank_on/off everyhwere. The slight change here is that after > >_off drm_vblank_get will refuse to work right away, but nouveau > >doesn't seem to depend upon that anywhere outside of the pageflip > >paths. > > > >The longer-term plan here is to switch all kms drivers to > >drm_vblank_on/off so that common code like pending event cleanup can > >be done there, while drm_vblank_pre/post_modeset will be purely > >drm internal for the old UMS ioctl. > > > >Note that the drm_vblank_off still seems required in the suspend path > >since nouveau doesn't explicitly disable crtcs. But on the resume side > >drm_helper_resume_force_mode should end up calling drm_vblank_on > >through the nouveau crtc hooks already. Hence remove the call in the > >resume code. > > > >Cc: Mario Kleiner <mario.kleiner.de@gmail.com> > >Cc: Ben Skeggs <bskeggs@redhat.com> > >Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > >--- > > drivers/gpu/drm/nouveau/dispnv04/crtc.c | 4 ++-- > > drivers/gpu/drm/nouveau/nouveau_display.c | 4 ---- > > 2 files changed, 2 insertions(+), 6 deletions(-) > > > >diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c > >index 3d96b49fe662..dab24066fa21 100644 > >--- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c > >+++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c > >@@ -708,7 +708,7 @@ static void nv_crtc_prepare(struct drm_crtc *crtc) > > if (nv_two_heads(dev)) > > NVSetOwner(dev, nv_crtc->index); > > > >- drm_vblank_pre_modeset(dev, nv_crtc->index); > >+ drm_vblank_off(dev, nv_crtc->index); > > funcs->dpms(crtc, DRM_MODE_DPMS_OFF); > > > > NVBlankScreen(dev, nv_crtc->index, true); > >@@ -740,7 +740,7 @@ static void nv_crtc_commit(struct drm_crtc *crtc) > > #endif > > > > funcs->dpms(crtc, DRM_MODE_DPMS_ON); > >- drm_vblank_post_modeset(dev, nv_crtc->index); > >+ drm_vblank_on(dev, nv_crtc->index); > > } > > The above hunk is probably correct, but i couldn't test it without > sufficiently old pre-nv 50 hardware. > > > > > static void nv_crtc_destroy(struct drm_crtc *crtc) > >diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c > >index 8670d90cdc11..d824023f9fc6 100644 > >--- a/drivers/gpu/drm/nouveau/nouveau_display.c > >+++ b/drivers/gpu/drm/nouveau/nouveau_display.c > >@@ -620,10 +620,6 @@ nouveau_display_resume(struct drm_device *dev, bool runtime) > > nv_crtc->lut.depth = 0; > > } > > > >- /* Make sure that drm and hw vblank irqs get resumed if needed. */ > >- for (head = 0; head < dev->mode_config.num_crtc; head++) > >- drm_vblank_on(dev, head); > >- > > /* This should ensure we don't hit a locking problem when someone > > * wakes us up via a connector. We should never go into suspend > > * while the display is on anyways. > > > > Tested this one and this hunk breaks suspend/resume. After a suspend/resume > cycle, all OpenGL apps and composited desktop are dead, as the core can't > get any vblank irq's enabled anymore. > > So the drm_vblank_on() is still needed here. Hm that's very surprising. As mentioned above the force_mode_restore should be calling nv_crtc_prepare already and fix this all up for us. I guess I need to dig out my nv card and trace what's really going on here. Enabling interrupts when the crtc is off isn't a good idea. -Daniel
On 05/29/2015 07:19 PM, Daniel Vetter wrote: > On Fri, May 29, 2015 at 06:50:06PM +0200, Mario Kleiner wrote: >> On 05/27/2015 11:04 AM, Daniel Vetter wrote: >>> In >>> >>> commit 9cba5efab5a8145ae6c52ea273553f069c294482 >>> Author: Mario Kleiner <mario.kleiner.de@gmail.com> >>> Date: Tue Jul 29 02:36:44 2014 +0200 >>> >>> drm/nouveau: Dis/Enable vblank irqs during suspend/resume >>> >>> drm_vblank_on/off calls where added around suspend/resume to make sure >>> vblank stay doesn't go boom over that transition. But nouveau already >>> used drm_vblank_pre/post_modeset over modesets. Instead use >>> drm_vblank_on/off everyhwere. The slight change here is that after >>> _off drm_vblank_get will refuse to work right away, but nouveau >>> doesn't seem to depend upon that anywhere outside of the pageflip >>> paths. >>> >>> The longer-term plan here is to switch all kms drivers to >>> drm_vblank_on/off so that common code like pending event cleanup can >>> be done there, while drm_vblank_pre/post_modeset will be purely >>> drm internal for the old UMS ioctl. >>> >>> Note that the drm_vblank_off still seems required in the suspend path >>> since nouveau doesn't explicitly disable crtcs. But on the resume side >>> drm_helper_resume_force_mode should end up calling drm_vblank_on >>> through the nouveau crtc hooks already. Hence remove the call in the >>> resume code. >>> >>> Cc: Mario Kleiner <mario.kleiner.de@gmail.com> >>> Cc: Ben Skeggs <bskeggs@redhat.com> >>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >>> --- >>> drivers/gpu/drm/nouveau/dispnv04/crtc.c | 4 ++-- >>> drivers/gpu/drm/nouveau/nouveau_display.c | 4 ---- >>> 2 files changed, 2 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c >>> index 3d96b49fe662..dab24066fa21 100644 >>> --- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c >>> +++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c >>> @@ -708,7 +708,7 @@ static void nv_crtc_prepare(struct drm_crtc *crtc) >>> if (nv_two_heads(dev)) >>> NVSetOwner(dev, nv_crtc->index); >>> >>> - drm_vblank_pre_modeset(dev, nv_crtc->index); >>> + drm_vblank_off(dev, nv_crtc->index); >>> funcs->dpms(crtc, DRM_MODE_DPMS_OFF); >>> >>> NVBlankScreen(dev, nv_crtc->index, true); >>> @@ -740,7 +740,7 @@ static void nv_crtc_commit(struct drm_crtc *crtc) >>> #endif >>> >>> funcs->dpms(crtc, DRM_MODE_DPMS_ON); >>> - drm_vblank_post_modeset(dev, nv_crtc->index); >>> + drm_vblank_on(dev, nv_crtc->index); >>> } >> >> The above hunk is probably correct, but i couldn't test it without >> sufficiently old pre-nv 50 hardware. >> >>> >>> static void nv_crtc_destroy(struct drm_crtc *crtc) >>> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c >>> index 8670d90cdc11..d824023f9fc6 100644 >>> --- a/drivers/gpu/drm/nouveau/nouveau_display.c >>> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c >>> @@ -620,10 +620,6 @@ nouveau_display_resume(struct drm_device *dev, bool runtime) >>> nv_crtc->lut.depth = 0; >>> } >>> >>> - /* Make sure that drm and hw vblank irqs get resumed if needed. */ >>> - for (head = 0; head < dev->mode_config.num_crtc; head++) >>> - drm_vblank_on(dev, head); >>> - >>> /* This should ensure we don't hit a locking problem when someone >>> * wakes us up via a connector. We should never go into suspend >>> * while the display is on anyways. >>> >> >> Tested this one and this hunk breaks suspend/resume. After a suspend/resume >> cycle, all OpenGL apps and composited desktop are dead, as the core can't >> get any vblank irq's enabled anymore. >> >> So the drm_vblank_on() is still needed here. > > Hm that's very surprising. As mentioned above the force_mode_restore > should be calling nv_crtc_prepare already and fix this all up for us. I > guess I need to dig out my nv card and trace what's really going on here. > > Enabling interrupts when the crtc is off isn't a good idea. > -Daniel > I think the nv_crtc_prepare() path modified in your first hunk is only for the original nv04 display engine for very old cards. nv50+ (GeForce-8 and later) take different paths. -mario
diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c index 3d96b49fe662..dab24066fa21 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c +++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c @@ -708,7 +708,7 @@ static void nv_crtc_prepare(struct drm_crtc *crtc) if (nv_two_heads(dev)) NVSetOwner(dev, nv_crtc->index); - drm_vblank_pre_modeset(dev, nv_crtc->index); + drm_vblank_off(dev, nv_crtc->index); funcs->dpms(crtc, DRM_MODE_DPMS_OFF); NVBlankScreen(dev, nv_crtc->index, true); @@ -740,7 +740,7 @@ static void nv_crtc_commit(struct drm_crtc *crtc) #endif funcs->dpms(crtc, DRM_MODE_DPMS_ON); - drm_vblank_post_modeset(dev, nv_crtc->index); + drm_vblank_on(dev, nv_crtc->index); } static void nv_crtc_destroy(struct drm_crtc *crtc) diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c index 8670d90cdc11..d824023f9fc6 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.c +++ b/drivers/gpu/drm/nouveau/nouveau_display.c @@ -620,10 +620,6 @@ nouveau_display_resume(struct drm_device *dev, bool runtime) nv_crtc->lut.depth = 0; } - /* Make sure that drm and hw vblank irqs get resumed if needed. */ - for (head = 0; head < dev->mode_config.num_crtc; head++) - drm_vblank_on(dev, head); - /* This should ensure we don't hit a locking problem when someone * wakes us up via a connector. We should never go into suspend * while the display is on anyways.
In commit 9cba5efab5a8145ae6c52ea273553f069c294482 Author: Mario Kleiner <mario.kleiner.de@gmail.com> Date: Tue Jul 29 02:36:44 2014 +0200 drm/nouveau: Dis/Enable vblank irqs during suspend/resume drm_vblank_on/off calls where added around suspend/resume to make sure vblank stay doesn't go boom over that transition. But nouveau already used drm_vblank_pre/post_modeset over modesets. Instead use drm_vblank_on/off everyhwere. The slight change here is that after _off drm_vblank_get will refuse to work right away, but nouveau doesn't seem to depend upon that anywhere outside of the pageflip paths. The longer-term plan here is to switch all kms drivers to drm_vblank_on/off so that common code like pending event cleanup can be done there, while drm_vblank_pre/post_modeset will be purely drm internal for the old UMS ioctl. Note that the drm_vblank_off still seems required in the suspend path since nouveau doesn't explicitly disable crtcs. But on the resume side drm_helper_resume_force_mode should end up calling drm_vblank_on through the nouveau crtc hooks already. Hence remove the call in the resume code. Cc: Mario Kleiner <mario.kleiner.de@gmail.com> Cc: Ben Skeggs <bskeggs@redhat.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/nouveau/dispnv04/crtc.c | 4 ++-- drivers/gpu/drm/nouveau/nouveau_display.c | 4 ---- 2 files changed, 2 insertions(+), 6 deletions(-)