drm/i915: Fix fbdev unload sequence

First thing we need to do is unregister the fbdev instance, but we
can't just go ahead and kfree it. That must wait until the hotplug and
polling work are stopped, since they can race with the with the
teardown. That means we need to split up the fbdev teardown into the
unregister part and the cleanup part.

I originally suspected that this was broken in one of the unload
shuffles, but on closer inspection the oldest sequence I've dug out
also gets this wrong. Just not quite so badly.

I've run drv_module_reload a few hundred times and it's rock solid
compared to insta-death beforehand. This bug seems to have been
uncovered by

commit 88be58be886f1215cc73dc8c273c985eecd7385c
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Jul 6 15:00:19 2017 +0200

    drm/i915/fbdev: Always forward hotplug events

But the effect of that seems to only be to increase the race window
enough to make it blow up easier. I'm not exactly clear on what's
going on there ...

v2: Fix whitespace and use fetch_and_zero (Chris).

Testcase: igt/drv_module_reload
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101791
Cc: martin.peres@free.fr
Cc: chris@chris-wilson.co.uk
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20170714224656.6431-2-daniel.vetter@ffwll.ch

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 799dd3890cd9071ce7c88814a72c3285a7f767bd..ffeb3a3bed5867676b65867576fbc39a548befb6 100644 (file)
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1241,6 +1241,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
  */
 static void i915_driver_unregister(struct drm_i915_private *dev_priv)
 {
+       intel_fbdev_unregister(dev_priv);
        intel_audio_deinit(dev_priv);
 
        intel_gpu_ips_teardown();
@@ -1374,8 +1375,6 @@ void i915_driver_unload(struct drm_device *dev)
        struct drm_i915_private *dev_priv = to_i915(dev);
        struct pci_dev *pdev = dev_priv->drm.pdev;
 
-       intel_fbdev_fini(dev);
-
        if (i915_gem_suspend(dev_priv))
                DRM_ERROR("failed to idle hardware; continuing to unload!\n");
 

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cbdebb022273795b1588d52130256a497da25d58..560be6e09040368da448ad9bd3baf86a3cafe9b5 100644 (file)
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15796,6 +15796,9 @@ void intel_modeset_cleanup(struct drm_device *dev)
         */
        drm_kms_helper_poll_fini(dev);
 
+       /* poll work can call into fbdev, hence clean that up afterwards */
+       intel_fbdev_fini(dev_priv);
+
        intel_unregister_dsm_handler();
 
        intel_fbc_global_disable(dev_priv);

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0902d7cb48d95a5debe327ac6fb8d7e52380e43c..4f9775a05df75a80f0c84f9243f4c1b488374dda 100644 (file)
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1597,7 +1597,8 @@ void intel_hpd_poll_init(struct drm_i915_private *dev_priv);
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 extern int intel_fbdev_init(struct drm_device *dev);
 extern void intel_fbdev_initial_config_async(struct drm_device *dev);
-extern void intel_fbdev_fini(struct drm_device *dev);
+extern void intel_fbdev_unregister(struct drm_i915_private *dev_priv);
+extern void intel_fbdev_fini(struct drm_i915_private *dev_priv);
 extern void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous);
 extern void intel_fbdev_output_poll_changed(struct drm_device *dev);
 extern void intel_fbdev_restore_mode(struct drm_device *dev);
@@ -1611,7 +1612,11 @@ static inline void intel_fbdev_initial_config_async(struct drm_device *dev)
 {
 }
 
-static inline void intel_fbdev_fini(struct drm_device *dev)
+static inline void intel_fbdev_unregister(struct drm_i915_private *dev_priv)
+{
+}
+
+static inline void intel_fbdev_fini(struct drm_i915_private *dev_priv)
 {
 }
 

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index ee1a5b937590696c1087644959ae476391a71484..2b7a47c9102a3010fdc29cbb0eb2dbcfe762dac3 100644 (file)
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -531,8 +531,6 @@ static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
         * trying to rectify all the possible error paths leading here.
         */
 
-       drm_fb_helper_unregister_fbi(&ifbdev->helper);
-
        drm_fb_helper_fini(&ifbdev->helper);
 
        if (ifbdev->vma) {
@@ -720,8 +718,10 @@ static void intel_fbdev_initial_config(void *data, async_cookie_t cookie)
 
        /* Due to peculiar init order wrt to hpd handling this is separate. */
        if (drm_fb_helper_initial_config(&ifbdev->helper,
-                                        ifbdev->preferred_bpp))
-               intel_fbdev_fini(ifbdev->helper.dev);
+                                        ifbdev->preferred_bpp)) {
+               intel_fbdev_unregister(to_i915(ifbdev->helper.dev));
+               intel_fbdev_fini(to_i915(ifbdev->helper.dev));
+       }
 }
 
 void intel_fbdev_initial_config_async(struct drm_device *dev)
@@ -744,9 +744,8 @@ static void intel_fbdev_sync(struct intel_fbdev *ifbdev)
        ifbdev->cookie = 0;
 }
 
-void intel_fbdev_fini(struct drm_device *dev)
+void intel_fbdev_unregister(struct drm_i915_private *dev_priv)
 {
-       struct drm_i915_private *dev_priv = to_i915(dev);
        struct intel_fbdev *ifbdev = dev_priv->fbdev;
 
        if (!ifbdev)
@@ -756,8 +755,17 @@ void intel_fbdev_fini(struct drm_device *dev)
        if (!current_is_async())
                intel_fbdev_sync(ifbdev);
 
+       drm_fb_helper_unregister_fbi(&ifbdev->helper);
+}
+
+void intel_fbdev_fini(struct drm_i915_private *dev_priv)
+{
+       struct intel_fbdev *ifbdev = fetch_and_zero(&dev_priv->fbdev);
+
+       if (!ifbdev)
+               return;
+
        intel_fbdev_destroy(ifbdev);
-       dev_priv->fbdev = NULL;
 }
 
 void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous)