From patchwork Mon Feb 27 20:43:16 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Gabriel Krisman Bertazi X-Patchwork-Id: 9594173 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id D4F2D60471 for ; Mon, 27 Feb 2017 20:43:53 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id C737027F9F for ; Mon, 27 Feb 2017 20:43:53 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id BC0AB2832D; Mon, 27 Feb 2017 20:43:53 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 60318282E8 for ; Mon, 27 Feb 2017 20:43:52 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 9977C6E58B; Mon, 27 Feb 2017 20:43:51 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from bhuna.collabora.co.uk (bhuna.collabora.co.uk [IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3]) by gabe.freedesktop.org (Postfix) with ESMTPS id 085096E58B for ; Mon, 27 Feb 2017 20:43:50 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: krisman) with ESMTPSA id 6997326A23B From: Gabriel Krisman Bertazi To: dri-devel@lists.freedesktop.org Subject: [RESEND PATCH 02/14] drm: qxl: Consolidate bo reservation when pinning Date: Mon, 27 Feb 2017 17:43:16 -0300 Message-Id: <20170227204328.18761-3-krisman@collabora.co.uk> X-Mailer: git-send-email 2.11.0 In-Reply-To: <20170227204328.18761-1-krisman@collabora.co.uk> References: <20170227204328.18761-1-krisman@collabora.co.uk> Cc: kraxel@redhat.com, Gabriel Krisman Bertazi X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP Every attempt to pin/unpin objects in memory requires qxl_bo_reserve/unreserve calls around the pinning operation to protect the object from concurrent access, which causes that call sequence to be reproduced every place where pinning is needed. In some cases, that sequence was not executed correctly, resulting in potential unprotected pinning operations. This commit encapsulates the reservation inside a new wrapper to make sure it is always handled properly. In cases where reservation must be done beforehand, for some reason, one can use the unprotected version __qxl_bo_pin/unpin. Signed-off-by: Gabriel Krisman Bertazi Reviewed-by: Gustavo Padovan --- drivers/gpu/drm/qxl/qxl_display.c | 52 ++++++--------------------------------- drivers/gpu/drm/qxl/qxl_fb.c | 25 ++++++------------- drivers/gpu/drm/qxl/qxl_object.c | 41 ++++++++++++++++++++++++++++-- 3 files changed, 54 insertions(+), 64 deletions(-) diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 2ce805a7ce5e..0957db17de4d 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -286,11 +286,7 @@ static int qxl_crtc_page_flip(struct drm_crtc *crtc, bo_old->is_primary = false; bo->is_primary = true; - ret = qxl_bo_reserve(bo, false); - if (ret) - return ret; ret = qxl_bo_pin(bo, bo->type, NULL); - qxl_bo_unreserve(bo); if (ret) return ret; @@ -306,11 +302,7 @@ static int qxl_crtc_page_flip(struct drm_crtc *crtc, } drm_crtc_vblank_put(crtc); - ret = qxl_bo_reserve(bo, false); - if (!ret) { - qxl_bo_unpin(bo); - qxl_bo_unreserve(bo); - } + qxl_bo_unpin(bo); return 0; } @@ -417,12 +409,7 @@ static int qxl_crtc_cursor_set2(struct drm_crtc *crtc, user_bo = gem_to_qxl_bo(obj); - ret = qxl_bo_reserve(user_bo, false); - if (ret) - goto out_unref; - ret = qxl_bo_pin(user_bo, QXL_GEM_DOMAIN_CPU, NULL); - qxl_bo_unreserve(user_bo); if (ret) goto out_unref; @@ -485,11 +472,8 @@ static int qxl_crtc_cursor_set2(struct drm_crtc *crtc, qxl_release_fence_buffer_objects(release); /* finish with the userspace bo */ - ret = qxl_bo_reserve(user_bo, false); - if (!ret) { - qxl_bo_unpin(user_bo); - qxl_bo_unreserve(user_bo); - } + qxl_bo_unpin(user_bo); + drm_gem_object_unreference_unlocked(obj); qxl_bo_unref (&qcrtc->cursor_bo); @@ -747,15 +731,10 @@ static int qxl_crtc_mode_set(struct drm_crtc *crtc, return -EINVAL; } - ret = qxl_bo_reserve(bo, false); - if (ret != 0) - return ret; ret = qxl_bo_pin(bo, bo->type, NULL); - if (ret != 0) { - qxl_bo_unreserve(bo); + if (ret != 0) return -EINVAL; - } - qxl_bo_unreserve(bo); + if (recreate_primary) { qxl_io_destroy_primary(qdev); qxl_io_log(qdev, @@ -781,9 +760,7 @@ static int qxl_crtc_mode_set(struct drm_crtc *crtc, if (old_bo && old_bo != bo) { old_bo->is_primary = false; - ret = qxl_bo_reserve(old_bo, false); qxl_bo_unpin(old_bo); - qxl_bo_unreserve(old_bo); } qxl_monitors_config_set(qdev, qcrtc->index, x, y, @@ -812,10 +789,8 @@ static void qxl_crtc_disable(struct drm_crtc *crtc) if (crtc->primary->fb) { struct qxl_framebuffer *qfb = to_qxl_framebuffer(crtc->primary->fb); struct qxl_bo *bo = gem_to_qxl_bo(qfb->obj); - int ret; - ret = qxl_bo_reserve(bo, false); + qxl_bo_unpin(bo); - qxl_bo_unreserve(bo); crtc->primary->fb = NULL; } @@ -1144,17 +1119,9 @@ int qxl_create_monitors_object(struct qxl_device *qdev) } qdev->monitors_config_bo = gem_to_qxl_bo(gobj); - ret = qxl_bo_reserve(qdev->monitors_config_bo, false); - if (ret) - return ret; - ret = qxl_bo_pin(qdev->monitors_config_bo, QXL_GEM_DOMAIN_VRAM, NULL); - if (ret) { - qxl_bo_unreserve(qdev->monitors_config_bo); + if (ret) return ret; - } - - qxl_bo_unreserve(qdev->monitors_config_bo); qxl_bo_kmap(qdev->monitors_config_bo, NULL); @@ -1175,13 +1142,10 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev) qdev->ram_header->monitors_config = 0; qxl_bo_kunmap(qdev->monitors_config_bo); - ret = qxl_bo_reserve(qdev->monitors_config_bo, false); + ret = qxl_bo_unpin(qdev->monitors_config_bo); if (ret) return ret; - qxl_bo_unpin(qdev->monitors_config_bo); - qxl_bo_unreserve(qdev->monitors_config_bo); - qxl_bo_unref(&qdev->monitors_config_bo); return 0; } diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c index 44e0619083fc..35124737666e 100644 --- a/drivers/gpu/drm/qxl/qxl_fb.c +++ b/drivers/gpu/drm/qxl/qxl_fb.c @@ -90,14 +90,10 @@ static struct fb_ops qxlfb_ops = { static void qxlfb_destroy_pinned_object(struct drm_gem_object *gobj) { struct qxl_bo *qbo = gem_to_qxl_bo(gobj); - int ret; - ret = qxl_bo_reserve(qbo, false); - if (likely(ret == 0)) { - qxl_bo_kunmap(qbo); - qxl_bo_unpin(qbo); - qxl_bo_unreserve(qbo); - } + qxl_bo_kunmap(qbo); + qxl_bo_unpin(qbo); + drm_gem_object_unreference_unlocked(gobj); } @@ -148,16 +144,13 @@ static int qxlfb_create_pinned_object(struct qxl_fbdev *qfbdev, qbo->surf.height = mode_cmd->height; qbo->surf.stride = mode_cmd->pitches[0]; qbo->surf.format = SPICE_SURFACE_FMT_32_xRGB; - ret = qxl_bo_reserve(qbo, false); - if (unlikely(ret != 0)) - goto out_unref; + ret = qxl_bo_pin(qbo, QXL_GEM_DOMAIN_SURFACE, NULL); if (ret) { - qxl_bo_unreserve(qbo); goto out_unref; } ret = qxl_bo_kmap(qbo, NULL); - qxl_bo_unreserve(qbo); /* unreserve, will be mmaped */ + if (ret) goto out_unref; @@ -322,12 +315,8 @@ static int qxlfb_create(struct qxl_fbdev *qfbdev, out_unref: if (qbo) { - ret = qxl_bo_reserve(qbo, false); - if (likely(ret == 0)) { - qxl_bo_kunmap(qbo); - qxl_bo_unpin(qbo); - qxl_bo_unreserve(qbo); - } + qxl_bo_kunmap(qbo); + qxl_bo_unpin(qbo); } if (fb && ret) { drm_gem_object_unreference_unlocked(gobj); diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c index dbc13510a1f8..9a7eef7dd604 100644 --- a/drivers/gpu/drm/qxl/qxl_object.c +++ b/drivers/gpu/drm/qxl/qxl_object.c @@ -221,7 +221,7 @@ struct qxl_bo *qxl_bo_ref(struct qxl_bo *bo) return bo; } -int qxl_bo_pin(struct qxl_bo *bo, u32 domain, u64 *gpu_addr) +int __qxl_bo_pin(struct qxl_bo *bo, u32 domain, u64 *gpu_addr) { struct drm_device *ddev = bo->gem_base.dev; int r; @@ -244,7 +244,7 @@ int qxl_bo_pin(struct qxl_bo *bo, u32 domain, u64 *gpu_addr) return r; } -int qxl_bo_unpin(struct qxl_bo *bo) +int __qxl_bo_unpin(struct qxl_bo *bo) { struct drm_device *ddev = bo->gem_base.dev; int r, i; @@ -264,6 +264,43 @@ int qxl_bo_unpin(struct qxl_bo *bo) return r; } + +/* + * Reserve the BO before pinning the object. If the BO was reserved + * beforehand, use the internal version directly __qxl_bo_pin. + * + */ +int qxl_bo_pin(struct qxl_bo *bo, u32 domain, u64 *gpu_addr) +{ + int r; + + r = qxl_bo_reserve(bo, false); + if (r) + return r; + + r = __qxl_bo_pin(bo, bo->type, NULL); + qxl_bo_unreserve(bo); + return r; +} + +/* + * Reserve the BO before pinning the object. If the BO was reserved + * beforehand, use the internal version directly __qxl_bo_unpin. + * + */ +int qxl_bo_unpin(struct qxl_bo *bo) +{ + int r; + + r = qxl_bo_reserve(bo, false); + if (r) + return r; + + r = __qxl_bo_unpin(bo); + qxl_bo_unreserve(bo); + return r; +} + void qxl_bo_force_delete(struct qxl_device *qdev) { struct qxl_bo *bo, *n;