[4/4] drm/radeon: unpin cursor BOs before suspend
diff mbox

Message ID 1435881253-4912-4-git-send-email-greg@chown.ath.cx
State New
Headers show

Commit Message

Grigori Goronzy July 2, 2015, 11:54 p.m. UTC
Everything is evicted from VRAM before suspend, so we need to make
sure all BOs are unpinned and re-pinned after resume. Fixes broken
mouse cursor after resume introduced by commit b9729b17.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=100541
Cc: stable@vger.kernel.org
Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
---
 drivers/gpu/drm/radeon/radeon_device.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Michel Dänzer July 3, 2015, 3:30 a.m. UTC | #1
[ Dropping stable@vger.kernel.org from Cc; the patch will be picked up
for stable after it lands in Linus' tree ]

On 03.07.2015 08:54, Grigori Goronzy wrote:
> Everything is evicted from VRAM before suspend, so we need to make
> sure all BOs are unpinned and re-pinned after resume. Fixes broken
> mouse cursor after resume introduced by commit b9729b17.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=100541
> Cc: stable@vger.kernel.org
> Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
> ---
>  drivers/gpu/drm/radeon/radeon_device.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
> index 14deeae..dddd5df 100644
> --- a/drivers/gpu/drm/radeon/radeon_device.c
> +++ b/drivers/gpu/drm/radeon/radeon_device.c
> @@ -1578,6 +1578,20 @@ int radeon_suspend_kms(struct drm_device *dev, bool suspend, bool fbcon)
>  		drm_helper_connector_dpms(connector, DRM_MODE_DPMS_OFF);
>  	}
>  
> +	/* unpin cursors */
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +		struct radeon_crtc *radeon_crtc = to_radeon_crtc(crtc);
> +
> +		if (radeon_crtc->cursor_bo) {
> +			struct radeon_bo *robj = gem_to_radeon_bo(radeon_crtc->cursor_bo);
> +			r = radeon_bo_reserve(robj, false);
> +			if (r == 0) {
> +				radeon_bo_unpin(robj);
> +				radeon_bo_unreserve(robj);
> +			}
> +		}
> +	}
> +
>  	/* unpin the front buffers */
>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>  		struct radeon_framebuffer *rfb = to_radeon_framebuffer(crtc->primary->fb);
> 

This could be done in the same loop as the front buffers.

On resume, the cursor BO is currently pinned again by
radeon_cursor_reset -> radeon_set_cursor. However, radeon_cursor_reset
is also called when changing the video mode, in which case it causes the
cursor BO pin count to increase by 1 for each CRTC using it. Presumably,
the mouse cursor would end up broken again on suspend/resume after that
for you.

We need a solution which pins the BO again on resume but doesn't
increase the pin count during a mode change. I'm not sure right now what
the best way is to achieve that, I'll think about it more later.
Grigori Goronzy July 3, 2015, 8:16 a.m. UTC | #2
On 2015-07-03 05:30, Michel Dänzer wrote:
> 
> This could be done in the same loop as the front buffers.
> 

Sure, I just thought it looks cleaner this way.

> On resume, the cursor BO is currently pinned again by
> radeon_cursor_reset -> radeon_set_cursor. However, radeon_cursor_reset
> is also called when changing the video mode, in which case it causes 
> the
> cursor BO pin count to increase by 1 for each CRTC using it. 
> Presumably,
> the mouse cursor would end up broken again on suspend/resume after that
> for you.
> 

Indeed. It seems to be problematic overall that radeon_cursor_reset does 
unconditionally increment the pin count. As soon as a mode is switched 
with cursor enabled, the cursor BO will stay pinned forever.

> We need a solution which pins the BO again on resume but doesn't
> increase the pin count during a mode change. I'm not sure right now 
> what
> the best way is to achieve that, I'll think about it more later.

How about this:

Never let radeon_set_cursor mess with the pin count, do that in 
radeon_crtc_cursor_set2 only, and make sure that the reference^Wpin 
count is updated accordingly (i.e. exactly one pin per crtc). Then add 
some explicit cursor resume code that traverses the crtc list and 
re-pins as needed. Maybe that that nice, but should work.

Grigori
Michel Dänzer July 3, 2015, 10:01 a.m. UTC | #3
On 03.07.2015 17:16, Grigori Goronzy wrote:
> On 2015-07-03 05:30, Michel Dänzer wrote:
>>
>> On resume, the cursor BO is currently pinned again by
>> radeon_cursor_reset -> radeon_set_cursor. However, radeon_cursor_reset
>> is also called when changing the video mode, in which case it causes the
>> cursor BO pin count to increase by 1 for each CRTC using it. Presumably,
>> the mouse cursor would end up broken again on suspend/resume after that
>> for you.
>>
> 
> Indeed. It seems to be problematic overall that radeon_cursor_reset does
> unconditionally increment the pin count. As soon as a mode is switched
> with cursor enabled, the cursor BO will stay pinned forever.

Exactly, so we can't ignore that for this fix.


>> We need a solution which pins the BO again on resume but doesn't
>> increase the pin count during a mode change. I'm not sure right now what
>> the best way is to achieve that, I'll think about it more later.
> 
> How about this:
> 
> Never let radeon_set_cursor mess with the pin count, do that in
> radeon_crtc_cursor_set2 only, and make sure that the reference^Wpin
> count is updated accordingly (i.e. exactly one pin per crtc). Then add
> some explicit cursor resume code that traverses the crtc list and
> re-pins as needed. Maybe that that nice, but should work.

Sounds good. I probably won't get around to playing with this before
next week, feel free to give it a try in the meantime.

Patch
diff mbox

diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index 14deeae..dddd5df 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -1578,6 +1578,20 @@  int radeon_suspend_kms(struct drm_device *dev, bool suspend, bool fbcon)
 		drm_helper_connector_dpms(connector, DRM_MODE_DPMS_OFF);
 	}
 
+	/* unpin cursors */
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		struct radeon_crtc *radeon_crtc = to_radeon_crtc(crtc);
+
+		if (radeon_crtc->cursor_bo) {
+			struct radeon_bo *robj = gem_to_radeon_bo(radeon_crtc->cursor_bo);
+			r = radeon_bo_reserve(robj, false);
+			if (r == 0) {
+				radeon_bo_unpin(robj);
+				radeon_bo_unreserve(robj);
+			}
+		}
+	}
+
 	/* unpin the front buffers */
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
 		struct radeon_framebuffer *rfb = to_radeon_framebuffer(crtc->primary->fb);