From patchwork Tue Mar 22 10:53:53 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Maarten Lankhorst X-Patchwork-Id: 8640091 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 31FC99F3D1 for ; Tue, 22 Mar 2016 10:54:11 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 70F4420398 for ; Tue, 22 Mar 2016 10:54:08 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id F10AE2038A for ; Tue, 22 Mar 2016 10:54:06 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id E505D6E1D3; Tue, 22 Mar 2016 10:54:00 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 768066E1D3; Tue, 22 Mar 2016 10:53:58 +0000 (UTC) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga102.jf.intel.com with ESMTP; 22 Mar 2016 03:53:57 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,376,1455004800"; d="scan'208";a="929298633" Received: from ndduggan-mobl.ger.corp.intel.com (HELO patser.lan) ([10.252.4.147]) by fmsmga001.fm.intel.com with ESMTP; 22 Mar 2016 03:53:55 -0700 Subject: Re: [PATCH] drm/core: Do not preserve framebuffer on rmfb. To: Daniel Vetter References: <1458569477-13364-1-git-send-email-maarten.lankhorst@linux.intel.com> <56F11130.5020506@linux.intel.com> <20160322105047.GU28483@phenom.ffwll.local> From: Maarten Lankhorst Message-ID: <56F12441.3000803@linux.intel.com> Date: Tue, 22 Mar 2016 11:53:53 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <20160322105047.GU28483@phenom.ffwll.local> Cc: stable , intel-gfx , Thomas Hellstrom , dri-devel 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: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Op 22-03-16 om 11:50 schreef Daniel Vetter: > On Tue, Mar 22, 2016 at 10:32:32AM +0100, Maarten Lankhorst wrote: >> Op 21-03-16 om 18:37 schreef Daniel Vetter: >>> On Mon, Mar 21, 2016 at 03:11:17PM +0100, Maarten Lankhorst wrote: >>>> It turns out that preserving framebuffers after the rmfb call breaks >>>> vmwgfx userspace. This was originally introduced because it was thought >>>> nobody relied on the behavior, but unfortunately it seems there are >>>> exceptions. >>>> >>>> drm_framebuffer_remove may fail with -EINTR now, so a straight revert >>>> is impossible. There is no way to remove the framebuffer from the lists >>>> and active planes without introducing a race because of the different >>>> locking requirements. Instead call drm_framebuffer_remove from a >>>> workqueue, which is unaffected by signals. >>>> >>>> Cc: stable@vger.kernel.org #v4.4+ >>>> Fixes: 13803132818c ("drm/core: Preserve the framebuffer after removing it.") >>>> Testcase: kms_flip.flip-vs-rmfb-interruptible >>>> References: https://lists.freedesktop.org/archives/dri-devel/2016-March/102876.html >>>> Cc: Thomas Hellstrom >>>> Cc: David Herrmann >>>> --- >>>> drivers/gpu/drm/drm_crtc.c | 20 +++++++++++++++++++- >>>> 1 file changed, 19 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >>>> index e08f962288d9..b7d0b959f088 100644 >>>> --- a/drivers/gpu/drm/drm_crtc.c >>>> +++ b/drivers/gpu/drm/drm_crtc.c >>>> @@ -3434,6 +3434,18 @@ int drm_mode_addfb2(struct drm_device *dev, >>>> return 0; >>>> } >>>> >>>> +struct drm_mode_rmfb_work { >>>> + struct work_struct work; >>>> + struct drm_framebuffer *fb; >>>> +}; >>>> + >>>> +static void drm_mode_rmfb_work_fn(struct work_struct *w) >>>> +{ >>>> + struct drm_mode_rmfb_work *arg = container_of(w, typeof(*arg), work); >>>> + >>>> + drm_framebuffer_remove(arg->fb); >>> drm_framebuffer_remove still has the problem of not working correctly with >>> atomic since atomic commit will complain if we try to do more than 1 >>> commit per ww_acquire_ctx. I think we still need an atomic version of >>> this. Also probably a more nasty igt testcase which uses the same fb on >>> more than one plane to be able to hit this case. >> That's true, but a separate bug. :) > Atm we only use drm_framebuffer_remove in atomic drivers to nuke the fbdev > one at unload. With your patch userspace can't get there easily, and hence > it must be fixed. Maybe separate prep patch (also cc: stable) upfront? > Something like this? Unfortunately I need to collect acks first. commit ed242f92c2e7571a6a5f649c2a67031debc73e44 Author: Maarten Lankhorst Date: Thu Mar 17 13:42:08 2016 +0100 drm/atomic: Add remove_fb function pointer. Use this in drm_framebuffer_remove, this is to remove the fb in an atomic way. Signed-off-by: Maarten Lankhorst diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c index 56b829f97699..50ba6adb74e8 100644 --- a/drivers/gpu/drm/arm/hdlcd_drv.c +++ b/drivers/gpu/drm/arm/hdlcd_drv.c @@ -324,6 +324,7 @@ static struct drm_driver hdlcd_driver = { .dumb_create = drm_gem_cma_dumb_create, .dumb_map_offset = drm_gem_cma_dumb_map_offset, .dumb_destroy = drm_gem_dumb_destroy, + .remove_fb = drm_atomic_helper_remove_fb, .prime_handle_to_fd = drm_gem_prime_handle_to_fd, .prime_fd_to_handle = drm_gem_prime_fd_to_handle, .gem_prime_export = drm_gem_prime_export, diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c index 3d8d16402d07..5d357f729114 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c @@ -711,6 +711,7 @@ static struct drm_driver atmel_hlcdc_dc_driver = { .dumb_create = drm_gem_cma_dumb_create, .dumb_map_offset = drm_gem_cma_dumb_map_offset, .dumb_destroy = drm_gem_dumb_destroy, + .remove_fb = drm_atomic_helper_remove_fb, .fops = &fops, .name = "atmel-hlcdc", .desc = "Atmel HLCD Controller DRM", diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 4befe25c81c7..eb3b413560df 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1960,6 +1960,72 @@ commit: return 0; } +int drm_atomic_helper_remove_fb(struct drm_framebuffer *fb) +{ + struct drm_modeset_acquire_ctx ctx; + struct drm_device *dev = fb->dev; + struct drm_atomic_state *state; + struct drm_plane *plane; + int ret = 0; + unsigned plane_mask; + + state = drm_atomic_state_alloc(dev); + if (!state) + return -ENOMEM; + + drm_modeset_acquire_init(&ctx, 0); + +retry: + plane_mask = 0; + ret = drm_modeset_lock_all_ctx(dev, &ctx); + if (ret) + goto unlock; + + drm_for_each_plane(plane, dev) { + struct drm_plane_state *plane_state; + + if (plane->state->fb != fb) + continue; + + plane_state = drm_atomic_get_plane_state(state, plane); + if (IS_ERR(plane_state)) { + ret = PTR_ERR(plane_state); + goto unlock; + } + + drm_atomic_set_fb_for_plane(plane_state, NULL); + ret = drm_atomic_set_crtc_for_plane(plane_state, NULL); + if (ret) + goto unlock; + + plane_mask |= BIT(drm_plane_index(plane)); + + plane->old_fb = plane->fb; + plane->fb = NULL; + } + + if (plane_mask) + ret = drm_atomic_commit(state); + +unlock: + if (plane_mask) + drm_atomic_clean_old_fb(dev, plane_mask, ret); + + if (ret == -EDEADLK) { + drm_modeset_backoff(&ctx); + goto retry; + } + + if (ret || !plane_mask) + drm_atomic_state_free(state); + + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx); + + return ret; +} +EXPORT_SYMBOL(drm_atomic_helper_remove_fb); + /** * drm_atomic_helper_disable_all - disable all currently active outputs * @dev: DRM device diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index aaf6ab42f2c1..51c5a00ffdff 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -577,6 +577,42 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb) } EXPORT_SYMBOL(drm_framebuffer_cleanup); +static int +legacy_remove_fb(struct drm_framebuffer *fb) +{ + struct drm_device *dev = fb->dev; + struct drm_crtc *crtc; + struct drm_plane *plane; + struct drm_mode_set set; + int ret = 0; + + /* atomic drivers must use drm_atomic_helper_remove_fb */ + WARN_ON(drm_core_check_feature(dev, DRIVER_ATOMIC)); + + drm_modeset_lock_all(dev); + /* remove from any CRTC */ + drm_for_each_crtc(crtc, dev) { + if (crtc->primary->fb == fb) { + /* should turn off the crtc */ + memset(&set, 0, sizeof(struct drm_mode_set)); + set.crtc = crtc; + set.fb = NULL; + ret = drm_mode_set_config_internal(&set); + if (ret) + goto out; + } + } + + drm_for_each_plane(plane, dev) { + if (plane->fb == fb) + drm_plane_force_disable(plane); + } + +out: + drm_modeset_unlock_all(dev); + return ret; +} + /** * drm_framebuffer_remove - remove and unreference a framebuffer object * @fb: framebuffer to remove @@ -592,9 +628,6 @@ EXPORT_SYMBOL(drm_framebuffer_cleanup); void drm_framebuffer_remove(struct drm_framebuffer *fb) { struct drm_device *dev; - struct drm_crtc *crtc; - struct drm_plane *plane; - struct drm_mode_set set; int ret; if (!fb) @@ -620,25 +653,13 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) * in this manner. */ if (atomic_read(&fb->refcount.refcount) > 1) { - drm_modeset_lock_all(dev); - /* remove from any CRTC */ - drm_for_each_crtc(crtc, dev) { - if (crtc->primary->fb == fb) { - /* should turn off the crtc */ - memset(&set, 0, sizeof(struct drm_mode_set)); - set.crtc = crtc; - set.fb = NULL; - ret = drm_mode_set_config_internal(&set); - if (ret) - DRM_ERROR("failed to reset crtc %p when fb was deleted\n", crtc); - } - } + if (dev->driver->remove_fb) + ret = dev->driver->remove_fb(fb); + else + ret = legacy_remove_fb(fb); - drm_for_each_plane(plane, dev) { - if (plane->fb == fb) - drm_plane_force_disable(plane); - } - drm_modeset_unlock_all(dev); + if (ret) + DRM_ERROR("failed to remove fb %i/%p with %i\n", fb->base.id, fb, ret); } drm_framebuffer_unreference(fb); diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 5344940c8a07..2705a315f1ec 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c @@ -423,6 +423,7 @@ static struct drm_driver exynos_drm_driver = { .dumb_create = exynos_drm_gem_dumb_create, .dumb_map_offset = exynos_drm_gem_dumb_map_offset, .dumb_destroy = drm_gem_dumb_destroy, + .remove_fb = drm_atomic_helper_remove_fb, .prime_handle_to_fd = drm_gem_prime_handle_to_fd, .prime_fd_to_handle = drm_gem_prime_fd_to_handle, .gem_prime_export = drm_gem_prime_export, diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c index e8d9337a66d8..f7562b178819 100644 --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c @@ -24,6 +24,7 @@ #include #include #include +#include #include "fsl_dcu_drm_crtc.h" #include "fsl_dcu_drm_drv.h" @@ -194,6 +195,7 @@ static struct drm_driver fsl_dcu_drm_driver = { .dumb_create = drm_gem_cma_dumb_create, .dumb_map_offset = drm_gem_cma_dumb_map_offset, .dumb_destroy = drm_gem_dumb_destroy, + .remove_fb = drm_atomic_helper_remove_fb, .fops = &fsl_dcu_drm_fops, .name = "fsl-dcu-drm", .desc = "Freescale DCU DRM", diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 2a076b005af9..9cbf88adf280 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -42,6 +42,7 @@ #include #include #include +#include static struct drm_driver driver; @@ -1712,6 +1713,7 @@ static struct drm_driver driver = { .dumb_create = i915_gem_dumb_create, .dumb_map_offset = i915_gem_mmap_gtt, .dumb_destroy = drm_gem_dumb_destroy, + .remove_fb = drm_atomic_helper_remove_fb, .ioctls = i915_ioctls, .fops = &i915_driver_fops, .name = DRIVER_NAME, diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index d52910e2c26c..fab3d7f036ae 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -973,6 +973,7 @@ static struct drm_driver msm_driver = { .dumb_create = msm_gem_dumb_create, .dumb_map_offset = msm_gem_dumb_map_offset, .dumb_destroy = drm_gem_dumb_destroy, + .remove_fb = drm_atomic_helper_remove_fb, .prime_handle_to_fd = drm_gem_prime_handle_to_fd, .prime_fd_to_handle = drm_gem_prime_fd_to_handle, .gem_prime_export = drm_gem_prime_export, diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 80398a684cae..9f3bacbad118 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -817,6 +817,7 @@ static struct drm_driver omap_drm_driver = { .dumb_create = omap_gem_dumb_create, .dumb_map_offset = omap_gem_dumb_map_offset, .dumb_destroy = drm_gem_dumb_destroy, + .remove_fb = drm_atomic_helper_remove_fb, .ioctls = ioctls, .num_ioctls = DRM_OMAP_NUM_IOCTLS, .fops = &omapdriver_fops, diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index ed6006bf6bd8..3b7388d87815 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -25,6 +25,7 @@ #include #include #include +#include #include "rcar_du_crtc.h" #include "rcar_du_drv.h" @@ -231,6 +232,7 @@ static struct drm_driver rcar_du_driver = { .dumb_create = rcar_du_dumb_create, .dumb_map_offset = drm_gem_cma_dumb_map_offset, .dumb_destroy = drm_gem_dumb_destroy, + .remove_fb = drm_atomic_helper_remove_fb, .fops = &rcar_du_fops, .name = "rcar-du", .desc = "Renesas R-Car Display Unit", diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c index 896da09e49ee..bf2162e4b131 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -290,6 +291,7 @@ static struct drm_driver rockchip_drm_driver = { .dumb_create = rockchip_gem_dumb_create, .dumb_map_offset = rockchip_gem_dumb_map_offset, .dumb_destroy = drm_gem_dumb_destroy, + .remove_fb = drm_atomic_helper_remove_fb, .prime_handle_to_fd = drm_gem_prime_handle_to_fd, .prime_fd_to_handle = drm_gem_prime_fd_to_handle, .gem_prime_import = drm_gem_prime_import, diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index 8e6b18caa706..c60f86c8f73d 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -944,6 +944,8 @@ static struct drm_driver tegra_drm_driver = { .dumb_map_offset = tegra_bo_dumb_map_offset, .dumb_destroy = drm_gem_dumb_destroy, + .remove_fb = drm_atomic_helper_remove_fb, + .ioctls = tegra_drm_ioctls, .num_ioctls = ARRAY_SIZE(tegra_drm_ioctls), .fops = &tegra_drm_fops, diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c index b7d2ff0e6e1f..4dde5d924d51 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.c +++ b/drivers/gpu/drm/vc4/vc4_drv.c @@ -15,6 +15,7 @@ #include #include #include "drm_fb_cma_helper.h" +#include "drm/drm_atomic_helper.h" #include "uapi/drm/vc4_drm.h" #include "vc4_drv.h" @@ -115,6 +116,8 @@ static struct drm_driver vc4_drm_driver = { .dumb_map_offset = drm_gem_cma_dumb_map_offset, .dumb_destroy = drm_gem_dumb_destroy, + .remove_fb = drm_atomic_helper_remove_fb, + .ioctls = vc4_drm_ioctls, .num_ioctls = ARRAY_SIZE(vc4_drm_ioctls), .fops = &vc4_drm_fops, diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c index 7f898cfdc746..56912941eaf8 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drv.c +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c @@ -31,6 +31,7 @@ #include #include "drmP.h" #include "drm/drm.h" +#include "drm/drm_atomic_helper.h" #include "virtgpu_drv.h" static struct drm_driver driver; @@ -129,6 +130,8 @@ static struct drm_driver driver = { .dumb_map_offset = virtio_gpu_mode_dumb_mmap, .dumb_destroy = virtio_gpu_mode_dumb_destroy, + .remove_fb = drm_atomic_helper_remove_fb, + #if defined(CONFIG_DEBUG_FS) .debugfs_init = virtio_gpu_debugfs_init, .debugfs_cleanup = virtio_gpu_debugfs_takedown, diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 3c8422c69572..31483c2fef51 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -638,6 +638,9 @@ struct drm_driver { struct drm_device *dev, uint32_t handle); + /* rmfb and drm_framebuffer_remove hook */ + int (*remove_fb)(struct drm_framebuffer *fb); + /* Driver private ops for this object */ const struct vm_operations_struct *gem_vm_ops; diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 9054598c9a7a..2d5ff5c80c76 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -87,6 +87,8 @@ int drm_atomic_helper_set_config(struct drm_mode_set *set); int __drm_atomic_helper_set_config(struct drm_mode_set *set, struct drm_atomic_state *state); +int drm_atomic_helper_remove_fb(struct drm_framebuffer *fb); + int drm_atomic_helper_disable_all(struct drm_device *dev, struct drm_modeset_acquire_ctx *ctx); struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev);