[1/3] drm/nouveau: Use drm_vblank_on/off consistently
diff mbox

Message ID 1432717471-7879-1-git-send-email-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter May 27, 2015, 9:04 a.m. UTC
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(-)

Comments

Mario Kleiner May 29, 2015, 4:50 p.m. UTC | #1
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
Daniel Vetter May 29, 2015, 5:19 p.m. UTC | #2
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
Mario Kleiner May 29, 2015, 5:23 p.m. UTC | #3
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

Patch
diff mbox

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.