| Message ID | 1435881253-4912-4-git-send-email-greg@chown.ath.cx (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[ 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.
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
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.
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);
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(+)