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@xxxxxxxxxxxxxxx #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@xxxxxxxxxx> > >>>> Cc: David Herrmann <dh.herrmann@xxxxxxxxx> > >>>> --- > >>>> 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@xxxxxxxxxxxxxxx> > 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@xxxxxxxxxxxxxxx> > > 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); > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html