Re: [PATCH] drm/core: Do not preserve framebuffer on rmfb.

On Tue, Mar 22, 2016 at 11:53:53AM +0100, Maarten Lankhorst wrote:

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 thellstrom@vmware.com Cc: David Herrmann dh.herrmann@gmail.com

---

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.

Oh dear, we're back to that discussion about how to pull this off :( I forgot about all that, silly me ...

For a much more minimal fix, what about a default rmfb helper which implements the right thing automatically depending upon DRIVER_ATOMIC, plus the hook only for i915 to get atomic behaviour? With that we only need ack for drm core + i915, which we can get fast ;-)

Would then also mean that drm_atomic_remove_framebuffer() would need to be in drm_atomic.c probably, so that drm_crtc.c can call it.

if (dev->driver->remove_fb) ret = dev->driver->remove_fb(fb); else if (drm_core_check_feature(dev, DRIVER_ATOMIC)) ret = drm_atomic_remove_fb(fb); else ret = legacy_remove_fb(fb);

Besides this, need some minimal kerneldoc from drm_atomic_remove_framebuffer().

Cheers, Daniel

commit ed242f92c2e7571a6a5f649c2a67031debc73e44 Author: Maarten Lankhorst maarten.lankhorst@linux.intel.com 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 <maarten.lankhorst@linux.intel.com>

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 <drm/drmP.h> #include <drm/drm_crtc_helper.h> #include <drm/drm_gem_cma_helper.h> +#include <drm/drm_atomic_helper.h>

#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 <linux/vgaarb.h> #include <linux/vga_switcheroo.h> #include <drm/drm_crtc_helper.h> +#include <drm/drm_atomic_helper.h>

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 <drm/drm_crtc_helper.h> #include <drm/drm_fb_cma_helper.h> #include <drm/drm_gem_cma_helper.h> +#include <drm/drm_atomic_helper.h>

#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 <drm/drmP.h> #include <drm/drm_crtc_helper.h> #include <drm/drm_fb_helper.h> +#include <drm/drm_atomic_helper.h> #include <linux/dma-mapping.h> #include <linux/pm_runtime.h> #include <linux/module.h> @@ -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 <linux/of_platform.h> #include <linux/platform_device.h> #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 <linux/pci.h> #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);