From patchwork Wed Dec 12 13:06:55 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 1867001 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 537F0DF2EE for ; Wed, 12 Dec 2012 14:25:01 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 386EAE65AA for ; Wed, 12 Dec 2012 06:25:01 -0800 (PST) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-ee0-f49.google.com (mail-ee0-f49.google.com [74.125.83.49]) by gabe.freedesktop.org (Postfix) with ESMTP id 0F1D3E5CD4 for ; Wed, 12 Dec 2012 05:14:48 -0800 (PST) Received: by mail-ee0-f49.google.com with SMTP id c4so430107eek.36 for ; Wed, 12 Dec 2012 05:14:48 -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=BzFoU+VR+iuYs6vwEzH5oavZRpGUgMAD/bVoJqUKQOY=; b=Bb8sn2C7AXmOVsL6hEU7mgznIsATpgG/nh5eU2ileEHMPjOxFRmWwXqqHMfocsTnT6 0oPgtp3l/c5HV0EG9c2VB8iptr22Z0zH8bKnnehFEvRn0IPA5cqdGxQ5lmYVLcl3M/jD yaoW14l4G5C6vSH4syjqT2jWKZqFwdUq310hk= 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=BzFoU+VR+iuYs6vwEzH5oavZRpGUgMAD/bVoJqUKQOY=; b=pnsX60X0euCwYDXSMYuvEUbjl8A4BnZQ2sfCk1AAHFTqnCntgKNOde9onsfZxper1D srrsZDPQOM33clKigRHbx3VST+NL9ESEg8+Viqik/AdbghuPTEQwe/k8LQC6/DDboeos ncnGuRJ/0K8yMHjcWEJ+EuQlptTKk7axpW5AeAh7+f/cRG6XU2b8J/nBjPpDI/w1dKag OWsPvUX7isF0ICOyuA7d5CUJEQLCkzcKZUN7S7pfOvbmS/WdAJ5nl0nbfDmINKN15lDp FbfWiUjonlQ+gvc+KlbmKbO84/QkowVeZfj4/NSn0UK3xlLrNtE19qpaubRREFtAjTMr NjGA== Received: by 10.14.213.134 with SMTP id a6mr2716942eep.45.1355317659253; Wed, 12 Dec 2012 05:07:39 -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 r1sm55868541eeo.2.2012.12.12.05.07.38 (version=TLSv1/SSLv3 cipher=OTHER); Wed, 12 Dec 2012 05:07:38 -0800 (PST) From: Daniel Vetter To: DRI Development Subject: [PATCH 15/37] drm: only take the crtc lock for ->cursor_move Date: Wed, 12 Dec 2012 14:06:55 +0100 Message-Id: <1355317637-16742-16-git-send-email-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 1.7.10.4 In-Reply-To: <1355317637-16742-1-git-send-email-daniel.vetter@ffwll.ch> References: <1355317637-16742-1-git-send-email-daniel.vetter@ffwll.ch> X-Gm-Message-State: ALoCoQngY+TusMs/x07JlGzTpCaoPYmmF5eHw9qA08+p7Z24+Nnsn21+XpIsyh75RDPXG6owkcPH Cc: Nouveau Dev , Intel Graphics Development , Radeon Dev , 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. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_crtc.c | 6 ++---- drivers/gpu/drm/radeon/radeon_cursor.c | 8 +++++++- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 13 +++++++++++++ 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 62b5002..3b2f25d 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2031,28 +2031,26 @@ 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; } } + mutex_unlock(&crtc->mutex); out: 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; }