From 80f0b5aff8f49f63eaf08bdae243de3a8ea3f77e Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Thu, 13 Dec 2012 23:39:01 +0100 Subject: drm/vmwgfx: reorder framebuffer init sequence vmwgfx has an oddity, when failing to reference the surface it'll return 0, since that's what the successfull drm_framebuffer_init will leave behind in ret. Fix this up by returning -EINVAL. Split out from all the other driver updates due to the above tiny semantic change. Shouldn't matter though since the reference grabbing seemingly can't fail. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) (limited to 'drivers/gpu/drm/vmwgfx/vmwgfx_kms.c') diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 54743943d8b3..edc97929c9a3 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -681,14 +681,10 @@ static int vmw_kms_new_framebuffer_surface(struct vmw_private *dev_priv, goto out_err1; } - ret = drm_framebuffer_init(dev, &vfbs->base.base, - &vmw_framebuffer_surface_funcs); - if (ret) - goto out_err2; - if (!vmw_surface_reference(surface)) { DRM_ERROR("failed to reference surface %p\n", surface); - goto out_err3; + ret = -EINVAL; + goto out_err2; } /* XXX get the first 3 from the surface info */ @@ -707,10 +703,15 @@ static int vmw_kms_new_framebuffer_surface(struct vmw_private *dev_priv, *out = &vfbs->base; + ret = drm_framebuffer_init(dev, &vfbs->base.base, + &vmw_framebuffer_surface_funcs); + if (ret) + goto out_err3; + return 0; out_err3: - drm_framebuffer_cleanup(&vfbs->base.base); + vmw_surface_unreference(&surface); out_err2: kfree(vfbs); out_err1: @@ -1053,14 +1054,10 @@ static int vmw_kms_new_framebuffer_dmabuf(struct vmw_private *dev_priv, goto out_err1; } - ret = drm_framebuffer_init(dev, &vfbd->base.base, - &vmw_framebuffer_dmabuf_funcs); - if (ret) - goto out_err2; - if (!vmw_dmabuf_reference(dmabuf)) { DRM_ERROR("failed to reference dmabuf %p\n", dmabuf); - goto out_err3; + ret = -EINVAL; + goto out_err2; } vfbd->base.base.bits_per_pixel = mode_cmd->bpp; @@ -1077,10 +1074,15 @@ static int vmw_kms_new_framebuffer_dmabuf(struct vmw_private *dev_priv, vfbd->base.user_handle = mode_cmd->handle; *out = &vfbd->base; + ret = drm_framebuffer_init(dev, &vfbd->base.base, + &vmw_framebuffer_dmabuf_funcs); + if (ret) + goto out_err3; + return 0; out_err3: - drm_framebuffer_cleanup(&vfbd->base.base); + vmw_dmabuf_unreference(&dmabuf); out_err2: kfree(vfbd); out_err1: -- cgit v1.2.3 From af26ef3b3978349cfbd864163a6ebb4906b733b5 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Thu, 13 Dec 2012 23:07:50 +0100 Subject: drm/: Unified handling of unimplemented fb->create_handle Some drivers don't have real ->create_handle callbacks. - cirrus/ast/mga200: Returns either 0 or -EINVAL. - udl: Didn't even bother with a callback, leading to a nice userspace-triggerable OOPS. - vmwgfx: This driver bothered with an implementation to return 0 as the handle (which is the canonical no-obj gem handle). All have in common that ->create_handle doesn't really make too much sense for them - that ioctl is used only for seamless fb takeover in the radeon/nouveau/i915 ddx drivers. So allow drivers to not implement this and return a consistent -ENODEV. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 12 ------------ 1 file changed, 12 deletions(-) (limited to 'drivers/gpu/drm/vmwgfx/vmwgfx_kms.c') diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index edc97929c9a3..9c0876b908ae 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -373,16 +373,6 @@ void vmw_kms_cursor_post_execbuf(struct vmw_private *dev_priv) * Generic framebuffer code */ -int vmw_framebuffer_create_handle(struct drm_framebuffer *fb, - struct drm_file *file_priv, - unsigned int *handle) -{ - if (handle) - *handle = 0; - - return 0; -} - /* * Surface framebuffer code */ @@ -610,7 +600,6 @@ int vmw_framebuffer_surface_dirty(struct drm_framebuffer *framebuffer, static struct drm_framebuffer_funcs vmw_framebuffer_surface_funcs = { .destroy = vmw_framebuffer_surface_destroy, .dirty = vmw_framebuffer_surface_dirty, - .create_handle = vmw_framebuffer_create_handle, }; static int vmw_kms_new_framebuffer_surface(struct vmw_private *dev_priv, @@ -961,7 +950,6 @@ int vmw_framebuffer_dmabuf_dirty(struct drm_framebuffer *framebuffer, static struct drm_framebuffer_funcs vmw_framebuffer_dmabuf_funcs = { .destroy = vmw_framebuffer_dmabuf_destroy, .dirty = vmw_framebuffer_dmabuf_dirty, - .create_handle = vmw_framebuffer_create_handle, }; /** -- cgit v1.2.3 From bfb899282f500eeb9dff2600729904aad0fd39e7 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Sun, 2 Dec 2012 13:48:21 +0100 Subject: drm: only take the crtc lock for ->cursor_set First convert ->cursor_set to only take the crtc lock, since that seems to be the function with the least amount of state - the core ioctl function doesn't check anything which can change at runtime, so we don't have any object lifetime issues to contend. The only thing which is important is that the driver's implementation doesn't touch any state outside of that single crtc which is not yet properly protected by other locking: - ast: access the global ast->cache_kmap. Luckily we only have on crtc on this driver, so this is fine. Add a comment. - gma500: calls gma_power_begin|and and psb_gtt_pin|unpin, both which have their own locking to protect their state. Everything else is crtc-local. - i915: touches a bit of global gem state, all protected by the One Lock to Rule Them All (dev->struct_mutex). - nouveau: Pre-nv50 is all nice, nv50+ uses the evo channels to queue up all display changes. And some of these channels are device global. But this is fine now since the previous patch introduced an evo channel mutex. - radeon: Uses some indirect register access for cursor updates, but with the previous patches to protect these indirect 2-register access patterns with a spinlock, this should be fine now, too. - vmwgfx: I have no idea how that works - update_cursor_position doesn't take any per-crtc argument and I haven't figured out any other place where this could be set in some form of a side-channel. But vmwgfx definitely has more than one crtc (or at least can register more than one), so I have no idea how this is supposed to not fail with the current code already. Hence take the easy way out and simply acquire all locks (which requires dropping the crtc lock the core acquired for us). That way it's not worse off for consistency than the old code. Reviewed-by: Rob Clark Signed-off-by: Daniel Vetter --- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) (limited to 'drivers/gpu/drm/vmwgfx/vmwgfx_kms.c') diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 9c0876b908ae..8d82e631c305 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -180,16 +180,29 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv, struct vmw_dma_buffer *dmabuf = NULL; 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. + */ + mutex_unlock(&crtc->mutex); + drm_modeset_lock_all(dev_priv->dev); + /* A lot of the code assumes this */ - if (handle && (width != 64 || height != 64)) - return -EINVAL; + if (handle && (width != 64 || height != 64)) { + ret = -EINVAL; + goto out; + } if (handle) { ret = vmw_user_lookup_handle(dev_priv, tfile, handle, &surface, &dmabuf); if (ret) { DRM_ERROR("failed to find surface or dmabuf: %i\n", ret); - return -EINVAL; + ret = -EINVAL; + goto out; } } @@ -197,7 +210,8 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv, if (surface && !surface->snooper.image) { DRM_ERROR("surface not suitable for cursor\n"); vmw_surface_unreference(&surface); - return -EINVAL; + ret = -EINVAL; + goto out; } /* takedown old cursor */ @@ -225,14 +239,20 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv, du->hotspot_x, du->hotspot_y); } else { vmw_cursor_update_position(dev_priv, false, 0, 0); - return 0; + ret = 0; + goto out; } vmw_cursor_update_position(dev_priv, true, du->cursor_x + du->hotspot_x, du->cursor_y + du->hotspot_y); - return 0; + ret = 0; +out: + drm_modeset_unlock_all(dev_priv->dev); + mutex_lock(&crtc->mutex); + + return ret; } int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) -- cgit v1.2.3 From dac35663cef4ca7f572d430bb54b14be8f03cb10 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Sun, 2 Dec 2012 15:24:10 +0100 Subject: drm: only take the crtc lock for ->cursor_move ->cursor_move uses mostly the same facilities in drivers as ->cursor_set, so pretty much nothing to fix up: - ast/gma500/i915: They all use per-crtc registers to update the cursor position. ast again touches the global cursor cache, but that's ok since there's only one crtc. - nouveau: nv50+ is again special, updates happen through the per-crtc channel (without pushbufs), so it's not protected by the new evo lock introduced earlier. But since this channel is per-crtc, we should be fine anyway. - radeon: A bit a mess: avivo asics need a workaround when both output pipes are enabled, which means it'll access the crtc list. Just reading that flag is ok though as long as radeon _always_ grabs all locks when changing the crtc configuration. Which means with the current scheme it cannot do an optimized modeset which only locks the relevant crtcs. This can be fixed though by introducing a bit of global state with separate locks and ensure in the modeset code that the cursor will be updated appropriately when enabling the 2nd pipe (on affected asics). - vmwgfx: I still don't understand what it's doing exactly, so apply the same trick for now. v2: Fixup unlocking for the error cases, spotted by Richard Wilbur. v3: Another error-case fixup. Reviewed-by: Rob Clark Signed-off-by: Daniel Vetter --- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) (limited to 'drivers/gpu/drm/vmwgfx/vmwgfx_kms.c') diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 8d82e631c305..3e3c7ab33ca2 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -264,10 +264,23 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) du->cursor_x = x + crtc->x; du->cursor_y = y + crtc->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. + */ + mutex_unlock(&crtc->mutex); + drm_modeset_lock_all(dev_priv->dev); + vmw_cursor_update_position(dev_priv, shown, du->cursor_x + du->hotspot_x, du->cursor_y + du->hotspot_y); + drm_modeset_unlock_all(dev_priv->dev); + mutex_lock(&crtc->mutex); + return 0; } -- cgit v1.2.3