From patchwork Thu Dec 13 11:03:33 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 1871751 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by patchwork2.kernel.org (Postfix) with ESMTP id 6C6CEDF2EF for ; Thu, 13 Dec 2012 11:04:43 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 4AABFE6879 for ; Thu, 13 Dec 2012 03:04:43 -0800 (PST) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-ea0-f177.google.com (mail-ea0-f177.google.com [209.85.215.177]) by gabe.freedesktop.org (Postfix) with ESMTP id BB957E5F34 for ; Thu, 13 Dec 2012 03:03:58 -0800 (PST) Received: by mail-ea0-f177.google.com with SMTP id c10so654832eaa.36 for ; Thu, 13 Dec 2012 03:03:58 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=from:to:cc:subject:date:message-id:x-mailer:in-reply-to:references; bh=1UzDzV6duMBGNN1VRa8VJv18phJhhkrvwmDTfdxPVsI=; b=M0C25Vu/TbALoPfmicWxDp5u2nXBaG9wN4v2q4j591m0SVa9apUHW9XsYCvU7BBH84 wAw17EFt/wI6Sd1NlKSfLxkms1leN/bUMUK6R6bAwjT1ycEgVgYl2i/9W2pcX9TSp+8b lwhvnTM3qgNfKnhJOs2PNxvmp1FlFv2hn6I+c= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=from:to:cc:subject:date:message-id:x-mailer:in-reply-to:references :x-gm-message-state; bh=1UzDzV6duMBGNN1VRa8VJv18phJhhkrvwmDTfdxPVsI=; b=feI8jYd+gG3Fu04QmtmcfSX8WH5k0dk36CsVFIEwl5/xr+3zSrwBDAX19LUjENkCSA 563Cm/pDZQUYogFxMnpBlN8E2XuqKKHFznMCdn1O84dOlEXT64fIeaqDV4NDweQnu4+W BegL2EMJow6Gs+PjmMrXTOTNAlgM5PWHXoUgDnJ7FGOPBU6mnU2nO8vTvf3r0IUKcr8i vX9XfB5hjGeAu7AkHEfwrDg9wZ7srnXar5LfIL6Cl/MU3hZIf0y72bepONgty9rl22zx fmI2zwv8Gp0yMwrAbKpz/wEmRXBzYyaUX2RD8QYnds5Jqw5T7CpCoqlbr1dPHvxnM3QC a28A== Received: by 10.14.194.195 with SMTP id m43mr4186084een.44.1355396637897; Thu, 13 Dec 2012 03:03:57 -0800 (PST) Received: from biers.ffwll.local (178-83-130-250.dynamic.hispeed.ch. [178.83.130.250]) by mx.google.com with ESMTPS id l3sm2280259eem.14.2012.12.13.03.03.56 (version=TLSv1/SSLv3 cipher=OTHER); Thu, 13 Dec 2012 03:03:57 -0800 (PST) From: Daniel Vetter To: DRI Development Subject: [PATCH] drm: only take the crtc lock for ->cursor_move Date: Thu, 13 Dec 2012 12:03:33 +0100 Message-Id: <1355396613-25224-1-git-send-email-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 1.7.10.4 In-Reply-To: <1355317637-16742-16-git-send-email-daniel.vetter@ffwll.ch> References: <1355317637-16742-16-git-send-email-daniel.vetter@ffwll.ch> X-Gm-Message-State: ALoCoQnQ4CbJTP/yD8YokDCpRmGgVKkQqRZstcm3J9ZV+KOmqs7Xw1w9LzZMRfWB9KxSqc68lUMt Cc: Nouveau Dev , Intel Graphics Development , Daniel Vetter X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org Errors-To: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org ->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. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_crtc.c | 7 +++---- drivers/gpu/drm/radeon/radeon_cursor.c | 8 +++++++- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 13 +++++++++++++ 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 62b5002..83de97f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2031,29 +2031,28 @@ int drm_mode_cursor_ioctl(struct drm_device *dev, } crtc = obj_to_crtc(obj); + mutex_lock(&crtc->mutex); if (req->flags & DRM_MODE_CURSOR_BO) { if (!crtc->funcs->cursor_set) { ret = -ENXIO; goto out; } /* Turns off the cursor if handle is 0 */ - mutex_lock(&crtc->mutex); ret = crtc->funcs->cursor_set(crtc, file_priv, req->handle, req->width, req->height); - mutex_unlock(&crtc->mutex); } if (req->flags & DRM_MODE_CURSOR_MOVE) { if (crtc->funcs->cursor_move) { - drm_modeset_lock_all(dev); ret = crtc->funcs->cursor_move(crtc, req->x, req->y); - drm_modeset_unlock_all(dev); } else { ret = -EFAULT; goto out; } } out: + mutex_unlock(&crtc->mutex); + return ret; } diff --git a/drivers/gpu/drm/radeon/radeon_cursor.c b/drivers/gpu/drm/radeon/radeon_cursor.c index ad6df62..c1680e6 100644 --- a/drivers/gpu/drm/radeon/radeon_cursor.c +++ b/drivers/gpu/drm/radeon/radeon_cursor.c @@ -245,8 +245,14 @@ int radeon_crtc_cursor_move(struct drm_crtc *crtc, int i = 0; struct drm_crtc *crtc_p; - /* avivo cursor image can't end on 128 pixel boundary or + /* + * avivo cursor image can't end on 128 pixel boundary or * go past the end of the frame if both crtcs are enabled + * + * NOTE: It is safe to access crtc->enabled of other crtcs + * without holding either the mode_config lock or the other + * crtc's lock as long as write access to this flag _always_ + * grabs all locks. */ list_for_each_entry(crtc_p, &crtc->dev->mode_config.crtc_list, head) { if (crtc_p->enabled) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 74b6734..385b8849 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; }