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: 1871741 Return-Path: X-Original-To: patchwork-intel-gfx@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 569B4DFB7B for ; Thu, 13 Dec 2012 11:04:12 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 2A537E5F40 for ; Thu, 13 Dec 2012 03:04:12 -0800 (PST) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@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 B63BAE5DE7 for ; Thu, 13 Dec 2012 03:03:58 -0800 (PST) Received: by mail-ea0-f177.google.com with SMTP id c10so654831eaa.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=QJJbL6aZw0SNBO/HepTTerVL38mHZ1M0aFyoWoExd6/Y6ra3v+F/5drHNf8VVAT6iU HiaS6867EP+dSqcOeut+bopq1MzrM7kV8Xi5l948Fyao4Ketlk1gQYHhyMOqv81QltAU X7mLOrTL7wdkwZwvWdM3rmCsTfe4ygzGkksTLhlWXkkp2F6BtFRGJtilFiflgwoc/k5k c/MJu9n2/Y2aWsFpiqKFvqaeMgl3HZt0ojhNwwkH+19cNsf0rLezxHVHrZkPzbrfORph 4xX2XuNhD8CLsBSeiJhOfhXSlDfSG1JxXsub5FyrefJ8e5rvmnBuRWcD1nHQ1b/88vRR VSvA== 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 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: ALoCoQm82gsFPl0OneDo2LYYkjYvvogOLItnWB1q2+aG4NKCi93ITbyu7HVHrXK/J5Q3OXOm3JnQ Cc: Nouveau Dev , Intel Graphics Development , Daniel Vetter Subject: [Intel-gfx] [PATCH] drm: only take the crtc lock for ->cursor_move X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: intel-gfx-bounces+patchwork-intel-gfx=patchwork.kernel.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+patchwork-intel-gfx=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; }