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

Message ID 20150529173521.GY8341@phenom.ffwll.local
State New
Headers show

Commit Message

Daniel Vetter May 29, 2015, 5:35 p.m. UTC
On Fri, May 29, 2015 at 07:23:35PM +0200, Mario Kleiner wrote:
> 
> 
> 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.

Oh right totally missed the nv50+ code. I only grepped for
pre/post_modeset ...

Below untested diff should help. I also realized that the pre-nv50 code
lacks drm_vblank_on/off in the dpms callback, so there's more work to do
anyway for this one here.

Thanks, Daniel

Comments

Mario Kleiner June 5, 2015, 7:40 p.m. UTC | #1
On 05/29/2015 07:35 PM, Daniel Vetter wrote:
> On Fri, May 29, 2015 at 07:23:35PM +0200, Mario Kleiner wrote:
>>
>>
>> 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.
>
> Oh right totally missed the nv50+ code. I only grepped for
> pre/post_modeset ...
>
> Below untested diff should help. I also realized that the pre-nv50 code
> lacks drm_vblank_on/off in the dpms callback, so there's more work to do
> anyway for this one here.
>
> Thanks, Daniel
>

The diff on top of your patch is now tested and helps. suspend->resume 
is now fine on nv50. In your patch, nouveau_display_resume() would also 
need to get a now unused "int head" removed to make the compiler happy.

-mario

> diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c
> index 7da7958556a3..a16c37d8f7e1 100644
> --- a/drivers/gpu/drm/nouveau/nv50_display.c
> +++ b/drivers/gpu/drm/nouveau/nv50_display.c
> @@ -997,6 +997,10 @@ nv50_crtc_cursor_show_hide(struct nouveau_crtc *nv_crtc, bool show, bool update)
>   static void
>   nv50_crtc_dpms(struct drm_crtc *crtc, int mode)
>   {
> +	if (mode == DRM_MODE_DPMS_ON)
> +		drm_crtc_vblank_on(crtc);
> +	else
> +		drm_crtc_vblank_off(crtc);
>   }
>
>   static void
> @@ -1036,6 +1040,8 @@ nv50_crtc_prepare(struct drm_crtc *crtc)
>   	}
>
>   	nv50_crtc_cursor_show_hide(nv_crtc, false, false);
> +
> +	drm_crtc_vblank_on(crtc);
>   }
>
>   static void
> @@ -1045,6 +1051,8 @@ nv50_crtc_commit(struct drm_crtc *crtc)
>   	struct nv50_mast *mast = nv50_mast(crtc->dev);
>   	u32 *push;
>
> +	drm_crtc_vblank_on(crtc);
> +
>   	push = evo_wait(mast, 32);
>   	if (push) {
>   		if (nv50_vers(mast) < G82_DISP_CORE_CHANNEL_DMA) {
>

Patch
diff mbox

diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c
index 7da7958556a3..a16c37d8f7e1 100644
--- a/drivers/gpu/drm/nouveau/nv50_display.c
+++ b/drivers/gpu/drm/nouveau/nv50_display.c
@@ -997,6 +997,10 @@  nv50_crtc_cursor_show_hide(struct nouveau_crtc *nv_crtc, bool show, bool update)
 static void
 nv50_crtc_dpms(struct drm_crtc *crtc, int mode)
 {
+	if (mode == DRM_MODE_DPMS_ON)
+		drm_crtc_vblank_on(crtc);
+	else
+		drm_crtc_vblank_off(crtc);
 }
 
 static void
@@ -1036,6 +1040,8 @@  nv50_crtc_prepare(struct drm_crtc *crtc)
 	}
 
 	nv50_crtc_cursor_show_hide(nv_crtc, false, false);
+
+	drm_crtc_vblank_on(crtc);
 }
 
 static void
@@ -1045,6 +1051,8 @@  nv50_crtc_commit(struct drm_crtc *crtc)
 	struct nv50_mast *mast = nv50_mast(crtc->dev);
 	u32 *push;
 
+	drm_crtc_vblank_on(crtc);
+
 	push = evo_wait(mast, 32);
 	if (push) {
 		if (nv50_vers(mast) < G82_DISP_CORE_CHANNEL_DMA) {