On Thu, Mar 23, 2017 at 07:22:31AM +0100, Thomas Hellstrom wrote:
On 03/22/2017 10:50 PM, Daniel Vetter wrote:
It's been around forever, no one bothered to address the FIXME, so I presume it's all fine.
Cc: Sinclair Yeh syeh@vmware.com Cc: Thomas Hellstrom thellstrom@vmware.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
NAK. We need to properly address this. Probably as part of the atomic update.
So could someone with vmwgfx understanding explain this? Note that the FIXME was originally added by me years ago, because I wasn't sure (only about 90%) that this is safe, and was essentially pleading for a vmwgfx expert to review this?
Since it didn't happen I presume it's not that terribly and probably safe ...
I'm still 90% sure that this is correct, but I'd love for a vmwgfx to audit it. Replying with a NAK is kinda not the response I was hoping for (and yes I guess I should have explained what's going on here better, but it's just a git blame of the FIXME comment away).
Thanks,
Daniel
/Thomas
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 25 ------------------------- 1 file changed, 25 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index d492d57d5309..424b3fc57203 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -148,15 +148,6 @@ int vmw_du_crtc_cursor_set2(struct drm_crtc *crtc, struct drm_file *file_priv, s32 hotspot_x, hotspot_y; int ret;
- /*
* FIXME: Unclear whether there's any global state touched by the* cursor_set function, especially vmw_cursor_update_position looks* suspicious. For now take the easy route and reacquire all locks. We* can do this since the caller in the drm core doesn't check anything* which is protected by any looks.*/- drm_modeset_unlock_crtc(crtc);
- drm_modeset_lock_all(dev_priv->dev); hotspot_x = hot_x + du->hotspot_x; hotspot_y = hot_y + du->hotspot_y;
@@ -224,9 +215,6 @@ int vmw_du_crtc_cursor_set2(struct drm_crtc *crtc, struct drm_file *file_priv, }
out:
- drm_modeset_unlock_all(dev_priv->dev);
- drm_modeset_lock_crtc(crtc, crtc->cursor);
- return ret;
}
@@ -239,25 +227,12 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) du->cursor_x = x + du->set_gui_x; du->cursor_y = y + du->set_gui_y;
/*
* FIXME: Unclear whether there's any global state touched by the* cursor_set function, especially vmw_cursor_update_position looks* suspicious. For now take the easy route and reacquire all locks. We* can do this since the caller in the drm core doesn't check anything* which is protected by any looks.*/drm_modeset_unlock_crtc(crtc);
drm_modeset_lock_all(dev_priv->dev);
vmw_cursor_update_position(dev_priv, shown, du->cursor_x + du->hotspot_x + du->core_hotspot_x, du->cursor_y + du->hotspot_y + du->core_hotspot_y);
drm_modeset_unlock_all(dev_priv->dev);
drm_modeset_lock_crtc(crtc, crtc->cursor);
return 0;
}