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

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

[Daniel Vetter]

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);
 }
 
 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.
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/nouveau: Use drm_vblank_on/off consistently

[Mario Kleiner]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/nouveau: Use drm_vblank_on/off consistently

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/nouveau: Use drm_vblank_on/off consistently

[Mario Kleiner]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/nouveau: Use drm_vblank_on/off consistently

[Daniel Vetter]

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

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) {
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/nouveau: Use drm_vblank_on/off consistently

[Mario Kleiner]

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) {
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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.

v2: Don't forget about nv50+, reported by Mario.

Tested-by: Mario Kleiner <mario.kleiner.de@gmail.com>
Tested-by: poma <pomidorabelisima@gmail.com>
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 | 6 +-----
 drivers/gpu/drm/nouveau/nv50_display.c    | 8 ++++++++
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
index 6f04397d43a7..bb9e9cb14b9d 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 24be27d3cd18..b1fd86f83d69 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -599,7 +599,7 @@ nouveau_display_resume(struct drm_device *dev, bool runtime)
 {
 	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct drm_crtc *crtc;
-	int ret, head;
+	int ret;
 
 	/* re-pin fb/cursors */
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
@@ -635,10 +635,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.
diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c
index ea3921652449..2ba2a145379b 100644
--- a/drivers/gpu/drm/nouveau/nv50_display.c
+++ b/drivers/gpu/drm/nouveau/nv50_display.c
@@ -1023,6 +1023,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
@@ -1062,6 +1066,8 @@ nv50_crtc_prepare(struct drm_crtc *crtc)
 	}
 
 	nv50_crtc_cursor_show_hide(nv_crtc, false, false);
+
+	drm_crtc_vblank_on(crtc);
 }
 
 static void
@@ -1071,6 +1077,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) {
-- 
2.7.0.rc3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm/radeon: Switch to drm_vblank_on/off

[Daniel Vetter]

These should be functionally equivalent to the older per/post modeset
functions, except that they block out drm_vblank_get right away.
There's only the clock adjusting code (outside of pageflips) in
readone which uses drm_vblank_get. But that code doesn't synchronize
against concurrent modesets and instead handles any such races by
waiting for the right vblank to arrive with a short timetout.

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 with this patch Michel uncovered a bug in the dri3
implementation of the DDX (it does vblank waits when the pipe is off),
which had to be fixed first.

Cc: Michel Dänzer <michel.daenzer@amd.com>
Acked-and-tested-by: Michel Dänzer <michel.daenzer@amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/radeon/atombios_crtc.c      | 4 ++--
 drivers/gpu/drm/radeon/radeon_legacy_crtc.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c
index 801dd60ac192..e187beca38f7 100644
--- a/drivers/gpu/drm/radeon/atombios_crtc.c
+++ b/drivers/gpu/drm/radeon/atombios_crtc.c
@@ -275,13 +275,13 @@ void atombios_crtc_dpms(struct drm_crtc *crtc, int mode)
 		if (ASIC_IS_DCE3(rdev) && !ASIC_IS_DCE6(rdev))
 			atombios_enable_crtc_memreq(crtc, ATOM_ENABLE);
 		atombios_blank_crtc(crtc, ATOM_DISABLE);
-		drm_vblank_post_modeset(dev, radeon_crtc->crtc_id);
+		drm_vblank_on(dev, radeon_crtc->crtc_id);
 		radeon_crtc_load_lut(crtc);
 		break;
 	case DRM_MODE_DPMS_STANDBY:
 	case DRM_MODE_DPMS_SUSPEND:
 	case DRM_MODE_DPMS_OFF:
-		drm_vblank_pre_modeset(dev, radeon_crtc->crtc_id);
+		drm_vblank_off(dev, radeon_crtc->crtc_id);
 		if (radeon_crtc->enabled)
 			atombios_blank_crtc(crtc, ATOM_ENABLE);
 		if (ASIC_IS_DCE3(rdev) && !ASIC_IS_DCE6(rdev))
diff --git a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c
index 32b338ff436b..24152dfef199 100644
--- a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c
+++ b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c
@@ -331,13 +331,13 @@ static void radeon_crtc_dpms(struct drm_crtc *crtc, int mode)
 									 RADEON_CRTC_DISP_REQ_EN_B));
 			WREG32_P(RADEON_CRTC_EXT_CNTL, crtc_ext_cntl, ~(mask | crtc_ext_cntl));
 		}
-		drm_vblank_post_modeset(dev, radeon_crtc->crtc_id);
+		drm_vblank_on(dev, radeon_crtc->crtc_id);
 		radeon_crtc_load_lut(crtc);
 		break;
 	case DRM_MODE_DPMS_STANDBY:
 	case DRM_MODE_DPMS_SUSPEND:
 	case DRM_MODE_DPMS_OFF:
-		drm_vblank_pre_modeset(dev, radeon_crtc->crtc_id);
+		drm_vblank_off(dev, radeon_crtc->crtc_id);
 		if (radeon_crtc->crtc_id)
 			WREG32_P(RADEON_CRTC2_GEN_CNTL, mask, ~(RADEON_CRTC2_EN | mask));
 		else {
-- 
2.7.0.rc3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/amdgpu: Switch to drm_vblank_on/off

[Daniel Vetter]

Equivalent change to the radeon driver.

Note that with radeon this caught a bug in the dri3 DDX
implementation, which asked for vblank interrupts when the pipe is
off. That bug needs to be fixed before we can merge this patch (if
amdgpu is affected too). Michel discovered this one.

Cc: Michel Dänzer <michel.daenzer@amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 4 ++--
 drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index 093599aba64b..d3926d193018 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -2701,13 +2701,13 @@ static void dce_v10_0_crtc_dpms(struct drm_crtc *crtc, int mode)
 		type = amdgpu_crtc_idx_to_irq_type(adev, amdgpu_crtc->crtc_id);
 		amdgpu_irq_update(adev, &adev->crtc_irq, type);
 		amdgpu_irq_update(adev, &adev->pageflip_irq, type);
-		drm_vblank_post_modeset(dev, amdgpu_crtc->crtc_id);
+		drm_vblank_on(dev, amdgpu_crtc->crtc_id);
 		dce_v10_0_crtc_load_lut(crtc);
 		break;
 	case DRM_MODE_DPMS_STANDBY:
 	case DRM_MODE_DPMS_SUSPEND:
 	case DRM_MODE_DPMS_OFF:
-		drm_vblank_pre_modeset(dev, amdgpu_crtc->crtc_id);
+		drm_vblank_off(dev, amdgpu_crtc->crtc_id);
 		if (amdgpu_crtc->enabled) {
 			dce_v10_0_vga_enable(crtc, true);
 			amdgpu_atombios_crtc_blank(crtc, ATOM_ENABLE);
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
index 8e67249d4367..789c7f9f623a 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
@@ -2692,13 +2692,13 @@ static void dce_v11_0_crtc_dpms(struct drm_crtc *crtc, int mode)
 		type = amdgpu_crtc_idx_to_irq_type(adev, amdgpu_crtc->crtc_id);
 		amdgpu_irq_update(adev, &adev->crtc_irq, type);
 		amdgpu_irq_update(adev, &adev->pageflip_irq, type);
-		drm_vblank_post_modeset(dev, amdgpu_crtc->crtc_id);
+		drm_vblank_on(dev, amdgpu_crtc->crtc_id);
 		dce_v11_0_crtc_load_lut(crtc);
 		break;
 	case DRM_MODE_DPMS_STANDBY:
 	case DRM_MODE_DPMS_SUSPEND:
 	case DRM_MODE_DPMS_OFF:
-		drm_vblank_pre_modeset(dev, amdgpu_crtc->crtc_id);
+		drm_vblank_off(dev, amdgpu_crtc->crtc_id);
 		if (amdgpu_crtc->enabled) {
 			dce_v11_0_vga_enable(crtc, true);
 			amdgpu_atombios_crtc_blank(crtc, ATOM_ENABLE);
-- 
2.7.0.rc3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/nouveau: Use drm_vblank_on/off consistently

[Michel Dänzer]

On 21.01.2016 19:08, 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.
> 
> v2: Don't forget about nv50+, reported by Mario.

I may be blind, but I can't see the DRI2 code in xf86-video-nouveau
dealing with DRM_IOCTL_WAIT_VBLANK returning an error during DPMS off /
modeset. Does it?

If not, AFAICT it would result in the xserver DRI2 code returning
protocol errors to clients, which I'm not sure they'll survive.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/radeon: Switch to drm_vblank_on/off

[Michel Dänzer]

On 21.01.2016 19:08, Daniel Vetter wrote:
> These should be functionally equivalent to the older per/post modeset
> functions, except that they block out drm_vblank_get right away.
> There's only the clock adjusting code (outside of pageflips) in
> readone which uses drm_vblank_get. But that code doesn't synchronize
> against concurrent modesets and instead handles any such races by
> waiting for the right vblank to arrive with a short timetout.
> 
> 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 with this patch Michel uncovered a bug in the dri3
> implementation of the DDX (it does vblank waits when the pipe is off),
> which had to be fixed first.
> 
> Cc: Michel Dänzer <michel.daenzer@amd.com>
> Acked-and-tested-by: Michel Dänzer <michel.daenzer@amd.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Note that this should be rebased onto Mario's PM fixes, but those might
go to 4.5 / stable anyway.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/amdgpu: Switch to drm_vblank_on/off

[Michel Dänzer]

On 21.01.2016 19:08, Daniel Vetter wrote:
> Equivalent change to the radeon driver.
> 
> Note that with radeon this caught a bug in the dri3 DDX
> implementation, which asked for vblank interrupts when the pipe is
> off. That bug needs to be fixed before we can merge this patch (if
> amdgpu is affected too). Michel discovered this one.

This is fixed in xf86-video-amdgpu as well.


> Cc: Michel Dänzer <michel.daenzer@amd.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 4 ++--
>  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> index 093599aba64b..d3926d193018 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> @@ -2701,13 +2701,13 @@ static void dce_v10_0_crtc_dpms(struct drm_crtc *crtc, int mode)
>  		type = amdgpu_crtc_idx_to_irq_type(adev, amdgpu_crtc->crtc_id);
>  		amdgpu_irq_update(adev, &adev->crtc_irq, type);
>  		amdgpu_irq_update(adev, &adev->pageflip_irq, type);
> -		drm_vblank_post_modeset(dev, amdgpu_crtc->crtc_id);
> +		drm_vblank_on(dev, amdgpu_crtc->crtc_id);
>  		dce_v10_0_crtc_load_lut(crtc);
>  		break;
>  	case DRM_MODE_DPMS_STANDBY:
>  	case DRM_MODE_DPMS_SUSPEND:
>  	case DRM_MODE_DPMS_OFF:
> -		drm_vblank_pre_modeset(dev, amdgpu_crtc->crtc_id);
> +		drm_vblank_off(dev, amdgpu_crtc->crtc_id);
>  		if (amdgpu_crtc->enabled) {
>  			dce_v10_0_vga_enable(crtc, true);
>  			amdgpu_atombios_crtc_blank(crtc, ATOM_ENABLE);
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> index 8e67249d4367..789c7f9f623a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> @@ -2692,13 +2692,13 @@ static void dce_v11_0_crtc_dpms(struct drm_crtc *crtc, int mode)
>  		type = amdgpu_crtc_idx_to_irq_type(adev, amdgpu_crtc->crtc_id);
>  		amdgpu_irq_update(adev, &adev->crtc_irq, type);
>  		amdgpu_irq_update(adev, &adev->pageflip_irq, type);
> -		drm_vblank_post_modeset(dev, amdgpu_crtc->crtc_id);
> +		drm_vblank_on(dev, amdgpu_crtc->crtc_id);
>  		dce_v11_0_crtc_load_lut(crtc);
>  		break;
>  	case DRM_MODE_DPMS_STANDBY:
>  	case DRM_MODE_DPMS_SUSPEND:
>  	case DRM_MODE_DPMS_OFF:
> -		drm_vblank_pre_modeset(dev, amdgpu_crtc->crtc_id);
> +		drm_vblank_off(dev, amdgpu_crtc->crtc_id);
>  		if (amdgpu_crtc->enabled) {
>  			dce_v11_0_vga_enable(crtc, true);
>  			amdgpu_atombios_crtc_blank(crtc, ATOM_ENABLE);
> 

You missed dce_v8_0.c. With that fixed,

Acked-by: Michel Dänzer <michel.daenzer@amd.com>


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel