[PATCH 0/6] drm/drv: Remove drm_dev_unplug()

[PATCH 0/6] drm/drv: Remove drm_dev_unplug()

[Noralf Trønnes]

This series removes drm_dev_unplug() and moves the unplugged state
setting to drm_dev_unregister(). All drivers will now have access to the
unplugged state if they so desire.

The drm_device ref handling wrt to the last fd closed after unregister
have been simplified, which also fixed a double drm_dev_unregister()
situation.

Noralf.

Noralf Trønnes (6):
  drm: Fix drm_release() and device unplug
  drm/drv: Prepare to remove drm_dev_unplug()
  drm/amd: Use drm_dev_unregister()
  drm/udl: Use drm_dev_unregister()
  drm/xen: Use drm_dev_unregister()
  drm/drv: Remove drm_dev_unplug()

 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  3 +-
 drivers/gpu/drm/drm_drv.c               | 48 ++++++++-----------------
 drivers/gpu/drm/drm_file.c              |  6 ++--
 drivers/gpu/drm/udl/udl_drv.c           |  3 +-
 drivers/gpu/drm/xen/xen_drm_front.c     |  7 ++--
 include/drm/drm_drv.h                   | 11 +++---
 6 files changed, 27 insertions(+), 51 deletions(-)

-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/6] drm: Fix drm_release() and device unplug

[Noralf Trønnes]

If userspace has open fd(s) when drm_dev_unplug() is run, it will result
in drm_dev_unregister() being called twice. First in drm_dev_unplug() and
then later in drm_release() through the call to drm_put_dev().

Since userspace already holds a ref on drm_device through the drm_minor,
it's not necessary to add extra ref counting based on no open file
handles. Instead just drm_dev_put() unconditionally in drm_dev_unplug().

We now has this:
- Userpace holds a ref on drm_device as long as there's open fd(s)
- The driver holds a ref on drm_device as long as it's bound to the
  struct device

When both sides are done with drm_device, it is released.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_drv.c  | 6 +-----
 drivers/gpu/drm/drm_file.c | 6 ++----
 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 381581b01d48..05bbc2b622fc 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -376,11 +376,7 @@ void drm_dev_unplug(struct drm_device *dev)
 	synchronize_srcu(&drm_unplug_srcu);
 
 	drm_dev_unregister(dev);
-
-	mutex_lock(&drm_global_mutex);
-	if (dev->open_count == 0)
-		drm_dev_put(dev);
-	mutex_unlock(&drm_global_mutex);
+	drm_dev_put(dev);
 }
 EXPORT_SYMBOL(drm_dev_unplug);
 
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 46f48f245eb5..3f20f598cd7c 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -479,11 +479,9 @@ int drm_release(struct inode *inode, struct file *filp)
 
 	drm_file_free(file_priv);
 
-	if (!--dev->open_count) {
+	if (!--dev->open_count)
 		drm_lastclose(dev);
-		if (drm_dev_is_unplugged(dev))
-			drm_put_dev(dev);
-	}
+
 	mutex_unlock(&drm_global_mutex);
 
 	drm_minor_release(minor);
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()

[Noralf Trønnes]

The only thing now that makes drm_dev_unplug() special is that it sets
drm_device->unplugged. Move this code to drm_dev_unregister() so that we
can remove drm_dev_unplug().

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---

Maybe s/unplugged/unregistered/ ?

I looked at drm_device->registered, but using that would mean that
drm_dev_is_unplugged() would return before drm_device is registered.
And given that its current purpose is to prevent race against connector
registration, I stayed away from it.

Noralf.


 drivers/gpu/drm/drm_drv.c | 27 +++++++++++++++------------
 include/drm/drm_drv.h     | 10 ++++------
 2 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 05bbc2b622fc..e0941200edc6 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
  */
 void drm_dev_unplug(struct drm_device *dev)
 {
-	/*
-	 * After synchronizing any critical read section is guaranteed to see
-	 * the new value of ->unplugged, and any critical section which might
-	 * still have seen the old value of ->unplugged is guaranteed to have
-	 * finished.
-	 */
-	dev->unplugged = true;
-	synchronize_srcu(&drm_unplug_srcu);
-
 	drm_dev_unregister(dev);
 	drm_dev_put(dev);
 }
@@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
  * drm_dev_register() but does not deallocate the device. The caller must call
  * drm_dev_put() to drop their final reference.
  *
- * A special form of unregistering for hotpluggable devices is drm_dev_unplug(),
- * which can be called while there are still open users of @dev.
+ * This function can be called while there are still open users of @dev as long
+ * as the driver protects its device resources using drm_dev_enter() and
+ * drm_dev_exit().
  *
  * This should be called first in the device teardown code to make sure
- * userspace can't access the device instance any more.
+ * userspace can't access the device instance any more. Drivers that support
+ * device unplug will probably want to call drm_atomic_helper_shutdown() first
+ * in order to disable the hardware on regular driver module unload.
  */
 void drm_dev_unregister(struct drm_device *dev)
 {
@@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
 	if (drm_core_check_feature(dev, DRIVER_LEGACY))
 		drm_lastclose(dev);
 
+	/*
+	 * After synchronizing any critical read section is guaranteed to see
+	 * the new value of ->unplugged, and any critical section which might
+	 * still have seen the old value of ->unplugged is guaranteed to have
+	 * finished.
+	 */
+	dev->unplugged = true;
+	synchronize_srcu(&drm_unplug_srcu);
+
 	dev->registered = false;
 
 	drm_client_dev_unregister(dev);
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index ca46a45a9cce..c50696c82a42 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
  * drm_dev_is_unplugged - is a DRM device unplugged
  * @dev: DRM device
  *
- * This function can be called to check whether a hotpluggable is unplugged.
- * Unplugging itself is singalled through drm_dev_unplug(). If a device is
- * unplugged, these two functions guarantee that any store before calling
- * drm_dev_unplug() is visible to callers of this function after it completes
+ * This function can be called to check whether @dev is unregistered. This can
+ * be used to detect that the underlying parent device is gone.
  *
- * WARNING: This function fundamentally races against drm_dev_unplug(). It is
- * recommended that drivers instead use the underlying drm_dev_enter() and
+ * WARNING: This function fundamentally races against drm_dev_unregister(). It
+ * is recommended that drivers instead use the underlying drm_dev_enter() and
  * drm_dev_exit() function pairs.
  */
 static inline bool drm_dev_is_unplugged(struct drm_device *dev)
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/6] drm/amd: Use drm_dev_unregister()

[Noralf Trønnes]

drm_dev_unplug() has been stripped down and is going away. Open code its
2 remaining function calls.

Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: David (ChunMing) Zhou <David1.Zhou@amd.com>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index a1bb3773087b..1265fc07120a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -970,7 +970,8 @@ amdgpu_pci_remove(struct pci_dev *pdev)
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
 	DRM_ERROR("Device removal is currently not supported outside of fbcon\n");
-	drm_dev_unplug(dev);
+	drm_dev_unregister(dev);
+	drm_dev_put(dev);
 	pci_disable_device(pdev);
 	pci_set_drvdata(pdev, NULL);
 }
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/6] drm/udl: Use drm_dev_unregister()

[Noralf Trønnes]

drm_dev_unplug() has been stripped down and is going away. Open code its
2 remaining function calls.

Cc: Dave Airlie <airlied@redhat.com>
Cc: Sean Paul <sean@poorly.run>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/udl/udl_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 22cd2d13e272..063e8e1e2641 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -106,7 +106,8 @@ static void udl_usb_disconnect(struct usb_interface *interface)
 	drm_kms_helper_poll_disable(dev);
 	udl_fbdev_unplug(dev);
 	udl_drop_usb(dev);
-	drm_dev_unplug(dev);
+	drm_dev_unregister(dev);
+	drm_dev_put(dev);
 }
 
 /*
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/6] drm/xen: Use drm_dev_unregister()

[Noralf Trønnes]

drm_dev_unplug() has been stripped down and is going away. Open code its
2 remaining function calls.

Also remove the drm_dev_is_unplugged() check since this can't be true
before drm_dev_unregister() is called which happens after the check.

Cc: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/xen/xen_drm_front.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front.c b/drivers/gpu/drm/xen/xen_drm_front.c
index 3e78a832d7f9..5c5eb24c6342 100644
--- a/drivers/gpu/drm/xen/xen_drm_front.c
+++ b/drivers/gpu/drm/xen/xen_drm_front.c
@@ -576,12 +576,9 @@ static void xen_drm_drv_fini(struct xen_drm_front_info *front_info)
 	if (!dev)
 		return;
 
-	/* Nothing to do if device is already unplugged */
-	if (drm_dev_is_unplugged(dev))
-		return;
-
 	drm_kms_helper_poll_fini(dev);
-	drm_dev_unplug(dev);
+	drm_dev_unregister(dev);
+	drm_dev_put(dev);
 
 	front_info->drm_info = NULL;
 
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/6] drm/drv: Remove drm_dev_unplug()

[Noralf Trønnes]

There are no users left.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_drv.c | 17 -----------------
 include/drm/drm_drv.h     |  1 -
 2 files changed, 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index e0941200edc6..87210d4a9e53 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -354,23 +354,6 @@ void drm_dev_exit(int idx)
 }
 EXPORT_SYMBOL(drm_dev_exit);
 
-/**
- * drm_dev_unplug - unplug a DRM device
- * @dev: DRM device
- *
- * This unplugs a hotpluggable DRM device, which makes it inaccessible to
- * userspace operations. Entry-points can use drm_dev_enter() and
- * drm_dev_exit() to protect device resources in a race free manner. This
- * essentially unregisters the device like drm_dev_unregister(), but can be
- * called while there are still open users of @dev.
- */
-void drm_dev_unplug(struct drm_device *dev)
-{
-	drm_dev_unregister(dev);
-	drm_dev_put(dev);
-}
-EXPORT_SYMBOL(drm_dev_unplug);
-
 /*
  * DRM internal mount
  * We want to be able to allocate our own "struct address_space" to control
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index c50696c82a42..b8765a6fc092 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -730,7 +730,6 @@ void drm_dev_put(struct drm_device *dev);
 void drm_put_dev(struct drm_device *dev);
 bool drm_dev_enter(struct drm_device *dev, int *idx);
 void drm_dev_exit(int idx);
-void drm_dev_unplug(struct drm_device *dev);
 
 /**
  * drm_dev_is_unplugged - is a DRM device unplugged
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/drv: Remove drm_dev_unplug()

[Patchwork]

== Series Details ==

Series: drm/drv: Remove drm_dev_unplug()
URL   : https://patchwork.freedesktop.org/series/56153/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5530 -> Patchwork_12122
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/56153/revisions/1/mbox/

Known issues
------------

  Here are the changes found in Patchwork_12122 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live_execlists:
    - fi-apl-guc:         PASS -> INCOMPLETE [fdo#103927]

  * igt@kms_busy@basic-flip-b:
    - fi-gdg-551:         PASS -> FAIL [fdo#103182]

  
#### Possible fixes ####

  * igt@i915_selftest@live_hangcheck:
    - {fi-icl-u3}:        INCOMPLETE [fdo#108569] -> PASS

  * igt@kms_busy@basic-flip-a:
    - fi-gdg-551:         FAIL [fdo#103182] -> PASS

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - fi-byt-clapper:     FAIL [fdo#103191] / [fdo#107362] -> PASS +1

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#105998]: https://bugs.freedesktop.org/show_bug.cgi?id=105998
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#108622]: https://bugs.freedesktop.org/show_bug.cgi?id=108622
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109294]: https://bugs.freedesktop.org/show_bug.cgi?id=109294
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#109527]: https://bugs.freedesktop.org/show_bug.cgi?id=109527
  [fdo#109528]: https://bugs.freedesktop.org/show_bug.cgi?id=109528
  [fdo#109530]: https://bugs.freedesktop.org/show_bug.cgi?id=109530


Participating hosts (46 -> 44)
------------------------------

  Additional (1): fi-icl-y 
  Missing    (3): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan 


Build changes
-------------

    * Linux: CI_DRM_5530 -> Patchwork_12122

  CI_DRM_5530: 172243f0dd47614186ed35cba3a001ab9c2ad00a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4804: 0b9ac934a6aad9ed5c1fdfd48d2b0faa10bfbbc4 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12122: ae4d00bc6397b304dd464f5d3067674c02e029e9 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

ae4d00bc6397 drm/drv: Remove drm_dev_unplug()
cb65ca53743b drm/xen: Use drm_dev_unregister()
35b3891a748a drm/udl: Use drm_dev_unregister()
e21d21016df4 drm/amd: Use drm_dev_unregister()
f7989771f688 drm/drv: Prepare to remove drm_dev_unplug()
31c4690158a7 drm: Fix drm_release() and device unplug

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12122/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/drv: Remove drm_dev_unplug()

[Patchwork]

== Series Details ==

Series: drm/drv: Remove drm_dev_unplug()
URL   : https://patchwork.freedesktop.org/series/56153/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5530_full -> Patchwork_12122_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Known issues
------------

  Here are the changes found in Patchwork_12122_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_suspend@shrink:
    - shard-apl:          NOTRUN -> DMESG-WARN [fdo#107886] / [fdo#109244]

  * igt@kms_available_modes_crc@available_mode_test_crc:
    - shard-glk:          PASS -> FAIL [fdo#106641]

  * igt@kms_busy@extended-pageflip-hang-newfb-render-b:
    - shard-apl:          NOTRUN -> DMESG-WARN [fdo#107956]

  * igt@kms_cursor_crc@cursor-256x256-dpms:
    - shard-apl:          PASS -> FAIL [fdo#103232]

  * igt@kms_cursor_crc@cursor-64x64-sliding:
    - shard-glk:          PASS -> FAIL [fdo#103232] +1

  * igt@kms_flip@wf_vblank-ts-check:
    - shard-apl:          PASS -> INCOMPLETE [fdo#103927]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-mmap-gtt:
    - shard-apl:          PASS -> FAIL [fdo#103167]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-wc:
    - shard-glk:          PASS -> FAIL [fdo#103167] +2

  * igt@kms_plane_alpha_blend@pipe-c-alpha-7efc:
    - shard-kbl:          NOTRUN -> FAIL [fdo#108145] / [fdo#108590]
    - shard-apl:          NOTRUN -> FAIL [fdo#108145] +2

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
    - shard-glk:          PASS -> FAIL [fdo#103166]

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
    - shard-apl:          PASS -> FAIL [fdo#103166]

  
#### Possible fixes ####

  * igt@kms_busy@extended-modeset-hang-newfb-render-a:
    - shard-hsw:          DMESG-WARN [fdo#107956] -> PASS

  * igt@kms_busy@extended-pageflip-hang-oldfb-render-b:
    - shard-apl:          DMESG-WARN [fdo#103558] / [fdo#105602] -> PASS

  * igt@kms_color@pipe-b-ctm-max:
    - shard-apl:          FAIL [fdo#108147] -> PASS

  * igt@kms_cursor_crc@cursor-128x42-random:
    - shard-glk:          INCOMPLETE [fdo#103359] / [k.org#198133] -> PASS

  * igt@kms_cursor_crc@cursor-256x85-onscreen:
    - shard-apl:          FAIL [fdo#103232] -> PASS +7
    - shard-glk:          FAIL [fdo#103232] -> PASS +2

  * igt@kms_cursor_crc@cursor-64x64-suspend:
    - shard-apl:          FAIL [fdo#103191] / [fdo#103232] -> PASS +1

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-glk:          FAIL [fdo#102887] / [fdo#105363] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
    - shard-apl:          FAIL [fdo#103167] -> PASS +2

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-wc:
    - shard-glk:          FAIL [fdo#103167] -> PASS +3

  * igt@kms_frontbuffer_tracking@fbc-2p-rte:
    - shard-glk:          FAIL [fdo#103167] / [fdo#105682] -> PASS

  * igt@kms_plane@pixel-format-pipe-a-planes-source-clamping:
    - shard-glk:          FAIL [fdo#108948] -> PASS

  * igt@kms_plane@pixel-format-pipe-b-planes:
    - shard-apl:          FAIL [fdo#103166] -> PASS

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-none:
    - shard-glk:          FAIL [fdo#103166] -> PASS +3

  * igt@kms_setmode@basic:
    - shard-kbl:          FAIL [fdo#99912] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#102887]: https://bugs.freedesktop.org/show_bug.cgi?id=102887
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682
  [fdo#106641]: https://bugs.freedesktop.org/show_bug.cgi?id=106641
  [fdo#107886]: https://bugs.freedesktop.org/show_bug.cgi?id=107886
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108147]: https://bugs.freedesktop.org/show_bug.cgi?id=108147
  [fdo#108590]: https://bugs.freedesktop.org/show_bug.cgi?id=108590
  [fdo#108948]: https://bugs.freedesktop.org/show_bug.cgi?id=108948
  [fdo#109244]: https://bugs.freedesktop.org/show_bug.cgi?id=109244
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


Participating hosts (7 -> 5)
------------------------------

  Missing    (2): shard-skl shard-iclb 


Build changes
-------------

    * Linux: CI_DRM_5530 -> Patchwork_12122

  CI_DRM_5530: 172243f0dd47614186ed35cba3a001ab9c2ad00a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4804: 0b9ac934a6aad9ed5c1fdfd48d2b0faa10bfbbc4 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12122: ae4d00bc6397b304dd464f5d3067674c02e029e9 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12122/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/6] drm/drv: Remove drm_dev_unplug()

[Daniel Vetter]

On Sun, Feb 03, 2019 at 04:41:54PM +0100, Noralf Trønnes wrote:
> This series removes drm_dev_unplug() and moves the unplugged state
> setting to drm_dev_unregister(). All drivers will now have access to the
> unplugged state if they so desire.
> 
> The drm_device ref handling wrt to the last fd closed after unregister
> have been simplified, which also fixed a double drm_dev_unregister()
> situation.

Nice. On the series:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

But definitely wait 1-2 weeks for some more driver acks I'd say before
applying this all.
-Daniel

> 
> Noralf.
> 
> Noralf Trønnes (6):
>   drm: Fix drm_release() and device unplug
>   drm/drv: Prepare to remove drm_dev_unplug()
>   drm/amd: Use drm_dev_unregister()
>   drm/udl: Use drm_dev_unregister()
>   drm/xen: Use drm_dev_unregister()
>   drm/drv: Remove drm_dev_unplug()
> 
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  3 +-
>  drivers/gpu/drm/drm_drv.c               | 48 ++++++++-----------------
>  drivers/gpu/drm/drm_file.c              |  6 ++--
>  drivers/gpu/drm/udl/udl_drv.c           |  3 +-
>  drivers/gpu/drm/xen/xen_drm_front.c     |  7 ++--
>  include/drm/drm_drv.h                   | 11 +++---
>  6 files changed, 27 insertions(+), 51 deletions(-)
> 
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm: Fix drm_release() and device unplug

[Oleksandr Andrushchenko]

On 2/3/19 5:41 PM, Noralf Trønnes wrote:
> If userspace has open fd(s) when drm_dev_unplug() is run, it will result
> in drm_dev_unregister() being called twice. First in drm_dev_unplug() and
> then later in drm_release() through the call to drm_put_dev().
>
> Since userspace already holds a ref on drm_device through the drm_minor,
> it's not necessary to add extra ref counting based on no open file
> handles. Instead just drm_dev_put() unconditionally in drm_dev_unplug().
>
> We now has this:
s/has/have ?
> - Userpace holds a ref on drm_device as long as there's open fd(s)
> - The driver holds a ref on drm_device as long as it's bound to the
>    struct device
>
> When both sides are done with drm_device, it is released.
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> ---
>   drivers/gpu/drm/drm_drv.c  | 6 +-----
>   drivers/gpu/drm/drm_file.c | 6 ++----
>   2 files changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 381581b01d48..05bbc2b622fc 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -376,11 +376,7 @@ void drm_dev_unplug(struct drm_device *dev)
>   	synchronize_srcu(&drm_unplug_srcu);
>   
>   	drm_dev_unregister(dev);
> -
> -	mutex_lock(&drm_global_mutex);
> -	if (dev->open_count == 0)
> -		drm_dev_put(dev);
> -	mutex_unlock(&drm_global_mutex);
> +	drm_dev_put(dev);
>   }
>   EXPORT_SYMBOL(drm_dev_unplug);
>   
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 46f48f245eb5..3f20f598cd7c 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -479,11 +479,9 @@ int drm_release(struct inode *inode, struct file *filp)
>   
>   	drm_file_free(file_priv);
>   
> -	if (!--dev->open_count) {
> +	if (!--dev->open_count)
>   		drm_lastclose(dev);
> -		if (drm_dev_is_unplugged(dev))
> -			drm_put_dev(dev);
> -	}
> +
>   	mutex_unlock(&drm_global_mutex);
>   
>   	drm_minor_release(minor);
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()

[Oleksandr Andrushchenko]

On 2/3/19 5:41 PM, Noralf Trønnes wrote:
> The only thing now that makes drm_dev_unplug() special is that it sets
> drm_device->unplugged. Move this code to drm_dev_unregister() so that we
> can remove drm_dev_unplug().
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> ---
>
> Maybe s/unplugged/unregistered/ ?
>
> I looked at drm_device->registered, but using that would mean that
> drm_dev_is_unplugged() would return before drm_device is registered.
> And given that its current purpose is to prevent race against connector
> registration, I stayed away from it.
>
> Noralf.
>
>
>   drivers/gpu/drm/drm_drv.c | 27 +++++++++++++++------------
>   include/drm/drm_drv.h     | 10 ++++------
>   2 files changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 05bbc2b622fc..e0941200edc6 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
>    */
>   void drm_dev_unplug(struct drm_device *dev)
>   {
> -	/*
> -	 * After synchronizing any critical read section is guaranteed to see
> -	 * the new value of ->unplugged, and any critical section which might
> -	 * still have seen the old value of ->unplugged is guaranteed to have
> -	 * finished.
> -	 */
> -	dev->unplugged = true;
> -	synchronize_srcu(&drm_unplug_srcu);
> -
>   	drm_dev_unregister(dev);
>   	drm_dev_put(dev);
>   }
> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
>    * drm_dev_register() but does not deallocate the device. The caller must call
>    * drm_dev_put() to drop their final reference.
>    *
> - * A special form of unregistering for hotpluggable devices is drm_dev_unplug(),
> - * which can be called while there are still open users of @dev.
> + * This function can be called while there are still open users of @dev as long
> + * as the driver protects its device resources using drm_dev_enter() and
> + * drm_dev_exit().
>    *
>    * This should be called first in the device teardown code to make sure
> - * userspace can't access the device instance any more.
> + * userspace can't access the device instance any more. Drivers that support
> + * device unplug will probably want to call drm_atomic_helper_shutdown() first
> + * in order to disable the hardware on regular driver module unload.
>    */
>   void drm_dev_unregister(struct drm_device *dev)
>   {
> @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
>   	if (drm_core_check_feature(dev, DRIVER_LEGACY))
>   		drm_lastclose(dev);
>   
> +	/*
> +	 * After synchronizing any critical read section is guaranteed to see
> +	 * the new value of ->unplugged, and any critical section which might
> +	 * still have seen the old value of ->unplugged is guaranteed to have
> +	 * finished.
> +	 */
> +	dev->unplugged = true;
> +	synchronize_srcu(&drm_unplug_srcu);
> +
>   	dev->registered = false;
>   
>   	drm_client_dev_unregister(dev);
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index ca46a45a9cce..c50696c82a42 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
>    * drm_dev_is_unplugged - is a DRM device unplugged
>    * @dev: DRM device
>    *
> - * This function can be called to check whether a hotpluggable is unplugged.
> - * Unplugging itself is singalled through drm_dev_unplug(). If a device is
> - * unplugged, these two functions guarantee that any store before calling
> - * drm_dev_unplug() is visible to callers of this function after it completes
> + * This function can be called to check whether @dev is unregistered. This can
> + * be used to detect that the underlying parent device is gone.
>    *
> - * WARNING: This function fundamentally races against drm_dev_unplug(). It is
> - * recommended that drivers instead use the underlying drm_dev_enter() and
> + * WARNING: This function fundamentally races against drm_dev_unregister(). It
> + * is recommended that drivers instead use the underlying drm_dev_enter() and
>    * drm_dev_exit() function pairs.
>    */
>   static inline bool drm_dev_is_unplugged(struct drm_device *dev)
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/6] drm/xen: Use drm_dev_unregister()

[Oleksandr Andrushchenko]

On 2/3/19 5:41 PM, Noralf Trønnes wrote:
> drm_dev_unplug() has been stripped down and is going away. Open code its
> 2 remaining function calls.
>
> Also remove the drm_dev_is_unplugged() check since this can't be true
> before drm_dev_unregister() is called which happens after the check.
>
> Cc: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>   drivers/gpu/drm/xen/xen_drm_front.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/xen/xen_drm_front.c b/drivers/gpu/drm/xen/xen_drm_front.c
> index 3e78a832d7f9..5c5eb24c6342 100644
> --- a/drivers/gpu/drm/xen/xen_drm_front.c
> +++ b/drivers/gpu/drm/xen/xen_drm_front.c
> @@ -576,12 +576,9 @@ static void xen_drm_drv_fini(struct xen_drm_front_info *front_info)
>   	if (!dev)
>   		return;
>   
> -	/* Nothing to do if device is already unplugged */
> -	if (drm_dev_is_unplugged(dev))
> -		return;
xen_drm_drv_fini is called when the backend changes its state [1],
so I just use the check above to prevent possible race conditions here,
e.g. do not allow to run unregister code if it is already in progress
So, I think we should keep this and probably just add a comment why it is
here
> -
>   	drm_kms_helper_poll_fini(dev);
> -	drm_dev_unplug(dev);
> +	drm_dev_unregister(dev);
> +	drm_dev_put(dev);
>   
>   	front_info->drm_info = NULL;
>   
[1] https://elixir.bootlin.com/linux/v5.0-rc5/ident/displback_disconnect
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 6/6] drm/drv: Remove drm_dev_unplug()

[Oleksandr Andrushchenko]

On 2/3/19 5:42 PM, Noralf Trønnes wrote:
> There are no users left.
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> ---
>   drivers/gpu/drm/drm_drv.c | 17 -----------------
>   include/drm/drm_drv.h     |  1 -
>   2 files changed, 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index e0941200edc6..87210d4a9e53 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -354,23 +354,6 @@ void drm_dev_exit(int idx)
>   }
>   EXPORT_SYMBOL(drm_dev_exit);
>   
> -/**
> - * drm_dev_unplug - unplug a DRM device
> - * @dev: DRM device
> - *
> - * This unplugs a hotpluggable DRM device, which makes it inaccessible to
> - * userspace operations. Entry-points can use drm_dev_enter() and
> - * drm_dev_exit() to protect device resources in a race free manner. This
> - * essentially unregisters the device like drm_dev_unregister(), but can be
> - * called while there are still open users of @dev.
> - */
> -void drm_dev_unplug(struct drm_device *dev)
> -{
> -	drm_dev_unregister(dev);
> -	drm_dev_put(dev);
> -}
> -EXPORT_SYMBOL(drm_dev_unplug);
> -
>   /*
>    * DRM internal mount
>    * We want to be able to allocate our own "struct address_space" to control
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index c50696c82a42..b8765a6fc092 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -730,7 +730,6 @@ void drm_dev_put(struct drm_device *dev);
>   void drm_put_dev(struct drm_device *dev);
>   bool drm_dev_enter(struct drm_device *dev, int *idx);
>   void drm_dev_exit(int idx);
> -void drm_dev_unplug(struct drm_device *dev);
>   
>   /**
>    * drm_dev_is_unplugged - is a DRM device unplugged
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 0/6] drm/drv: Remove drm_dev_unplug()

[Koenig, Christian]

Adding Andrey who looked into cleaning this up a while ago as well.

Christian.

Am 03.02.19 um 16:41 schrieb Noralf Trønnes:
> This series removes drm_dev_unplug() and moves the unplugged state
> setting to drm_dev_unregister(). All drivers will now have access to the
> unplugged state if they so desire.
>
> The drm_device ref handling wrt to the last fd closed after unregister
> have been simplified, which also fixed a double drm_dev_unregister()
> situation.
>
> Noralf.
>
> Noralf Trønnes (6):
>    drm: Fix drm_release() and device unplug
>    drm/drv: Prepare to remove drm_dev_unplug()
>    drm/amd: Use drm_dev_unregister()
>    drm/udl: Use drm_dev_unregister()
>    drm/xen: Use drm_dev_unregister()
>    drm/drv: Remove drm_dev_unplug()
>
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  3 +-
>   drivers/gpu/drm/drm_drv.c               | 48 ++++++++-----------------
>   drivers/gpu/drm/drm_file.c              |  6 ++--
>   drivers/gpu/drm/udl/udl_drv.c           |  3 +-
>   drivers/gpu/drm/xen/xen_drm_front.c     |  7 ++--
>   include/drm/drm_drv.h                   | 11 +++---
>   6 files changed, 27 insertions(+), 51 deletions(-)
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/6] drm/xen: Use drm_dev_unregister()

[Noralf Trønnes]

Den 04.02.2019 11.42, skrev Oleksandr Andrushchenko:
> On 2/3/19 5:41 PM, Noralf Trønnes wrote:
>> drm_dev_unplug() has been stripped down and is going away. Open code its
>> 2 remaining function calls.
>>
>> Also remove the drm_dev_is_unplugged() check since this can't be true
>> before drm_dev_unregister() is called which happens after the check.
>>
>> Cc: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>   drivers/gpu/drm/xen/xen_drm_front.c | 7 ++-----
>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xen/xen_drm_front.c b/drivers/gpu/drm/xen/xen_drm_front.c
>> index 3e78a832d7f9..5c5eb24c6342 100644
>> --- a/drivers/gpu/drm/xen/xen_drm_front.c
>> +++ b/drivers/gpu/drm/xen/xen_drm_front.c
>> @@ -576,12 +576,9 @@ static void xen_drm_drv_fini(struct xen_drm_front_info *front_info)
>>   	if (!dev)
>>   		return;
>>   
>> -	/* Nothing to do if device is already unplugged */
>> -	if (drm_dev_is_unplugged(dev))
>> -		return;
> xen_drm_drv_fini is called when the backend changes its state [1],
> so I just use the check above to prevent possible race conditions here,
> e.g. do not allow to run unregister code if it is already in progress
> So, I think we should keep this and probably just add a comment why it is
> here

Ok, it's just me not reading the code closely enough. I'll put it back
in the next version.

Noralf.

>> -
>>   	drm_kms_helper_poll_fini(dev);
>> -	drm_dev_unplug(dev);
>> +	drm_dev_unregister(dev);
>> +	drm_dev_put(dev);
>>   
>>   	front_info->drm_info = NULL;
>>   
> [1] https://elixir.bootlin.com/linux/v5.0-rc5/ident/displback_disconnect
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()

[Daniel Vetter]

On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
> The only thing now that makes drm_dev_unplug() special is that it sets
> drm_device->unplugged. Move this code to drm_dev_unregister() so that we
> can remove drm_dev_unplug().
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
> 
> Maybe s/unplugged/unregistered/ ?
> 
> I looked at drm_device->registered, but using that would mean that
> drm_dev_is_unplugged() would return before drm_device is registered.
> And given that its current purpose is to prevent race against connector
> registration, I stayed away from it.

Yeah I think we need to keep the registered state separate from unplugged.
Iirc this exact scenario is what we discussed when you revamped the
unplug infrastructure.

> 
> Noralf.
> 
> 
>  drivers/gpu/drm/drm_drv.c | 27 +++++++++++++++------------
>  include/drm/drm_drv.h     | 10 ++++------
>  2 files changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 05bbc2b622fc..e0941200edc6 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
>   */
>  void drm_dev_unplug(struct drm_device *dev)
>  {
> -	/*
> -	 * After synchronizing any critical read section is guaranteed to see
> -	 * the new value of ->unplugged, and any critical section which might
> -	 * still have seen the old value of ->unplugged is guaranteed to have
> -	 * finished.
> -	 */
> -	dev->unplugged = true;
> -	synchronize_srcu(&drm_unplug_srcu);
> -
>  	drm_dev_unregister(dev);
>  	drm_dev_put(dev);
>  }
> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
>   * drm_dev_register() but does not deallocate the device. The caller must call
>   * drm_dev_put() to drop their final reference.
>   *
> - * A special form of unregistering for hotpluggable devices is drm_dev_unplug(),
> - * which can be called while there are still open users of @dev.
> + * This function can be called while there are still open users of @dev as long
> + * as the driver protects its device resources using drm_dev_enter() and
> + * drm_dev_exit().
>   *
>   * This should be called first in the device teardown code to make sure
> - * userspace can't access the device instance any more.
> + * userspace can't access the device instance any more. Drivers that support
> + * device unplug will probably want to call drm_atomic_helper_shutdown() first

Read once more with a bit more coffee, spotted this:

s/first/afterwards/ - shutting down the hw before we've taken it away from
userspace is kinda the wrong way round. It should be the inverse of driver
load, which is 1) allocate structures 2) prep hw 3) register driver with
the world (simplified ofc).

> + * in order to disable the hardware on regular driver module unload.
>   */
>  void drm_dev_unregister(struct drm_device *dev)
>  {
> @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
>  	if (drm_core_check_feature(dev, DRIVER_LEGACY))
>  		drm_lastclose(dev);
>  
> +	/*
> +	 * After synchronizing any critical read section is guaranteed to see
> +	 * the new value of ->unplugged, and any critical section which might
> +	 * still have seen the old value of ->unplugged is guaranteed to have
> +	 * finished.
> +	 */
> +	dev->unplugged = true;
> +	synchronize_srcu(&drm_unplug_srcu);
> +
>  	dev->registered = false;
>  
>  	drm_client_dev_unregister(dev);
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index ca46a45a9cce..c50696c82a42 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
>   * drm_dev_is_unplugged - is a DRM device unplugged
>   * @dev: DRM device
>   *
> - * This function can be called to check whether a hotpluggable is unplugged.
> - * Unplugging itself is singalled through drm_dev_unplug(). If a device is
> - * unplugged, these two functions guarantee that any store before calling
> - * drm_dev_unplug() is visible to callers of this function after it completes
> + * This function can be called to check whether @dev is unregistered. This can
> + * be used to detect that the underlying parent device is gone.

I think it'd be good to keep the first part, and just update the reference
to drm_dev_unregister. So:

 * This function can be called to check whether a hotpluggable is unplugged.
 * Unplugging itself is singalled through drm_dev_unregister(). If a device is
 * unplugged, these two functions guarantee that any store before calling
 * drm_dev_unregister() is visible to callers of this function after it
 * completes.

I think your version shrugs a few important details under the rug. With
those nits addressed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Cheers, Daniel

>   *
> - * WARNING: This function fundamentally races against drm_dev_unplug(). It is
> - * recommended that drivers instead use the underlying drm_dev_enter() and
> + * WARNING: This function fundamentally races against drm_dev_unregister(). It
> + * is recommended that drivers instead use the underlying drm_dev_enter() and
>   * drm_dev_exit() function pairs.
>   */
>  static inline bool drm_dev_is_unplugged(struct drm_device *dev)
> -- 
> 2.20.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()

[Noralf Trønnes]

Den 04.02.2019 16.41, skrev Daniel Vetter:
> On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
>> The only thing now that makes drm_dev_unplug() special is that it sets
>> drm_device->unplugged. Move this code to drm_dev_unregister() so that we
>> can remove drm_dev_unplug().
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---

[...]

>>  drivers/gpu/drm/drm_drv.c | 27 +++++++++++++++------------
>>  include/drm/drm_drv.h     | 10 ++++------
>>  2 files changed, 19 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 05bbc2b622fc..e0941200edc6 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
>>   */
>>  void drm_dev_unplug(struct drm_device *dev)
>>  {
>> -	/*
>> -	 * After synchronizing any critical read section is guaranteed to see
>> -	 * the new value of ->unplugged, and any critical section which might
>> -	 * still have seen the old value of ->unplugged is guaranteed to have
>> -	 * finished.
>> -	 */
>> -	dev->unplugged = true;
>> -	synchronize_srcu(&drm_unplug_srcu);
>> -
>>  	drm_dev_unregister(dev);
>>  	drm_dev_put(dev);
>>  }
>> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
>>   * drm_dev_register() but does not deallocate the device. The caller must call
>>   * drm_dev_put() to drop their final reference.
>>   *
>> - * A special form of unregistering for hotpluggable devices is drm_dev_unplug(),
>> - * which can be called while there are still open users of @dev.
>> + * This function can be called while there are still open users of @dev as long
>> + * as the driver protects its device resources using drm_dev_enter() and
>> + * drm_dev_exit().
>>   *
>>   * This should be called first in the device teardown code to make sure
>> - * userspace can't access the device instance any more.
>> + * userspace can't access the device instance any more. Drivers that support
>> + * device unplug will probably want to call drm_atomic_helper_shutdown() first
> 
> Read once more with a bit more coffee, spotted this:
> 
> s/first/afterwards/ - shutting down the hw before we've taken it away from
> userspace is kinda the wrong way round. It should be the inverse of driver
> load, which is 1) allocate structures 2) prep hw 3) register driver with
> the world (simplified ofc).
> 

The problem is that drm_dev_unregister() sets the device as unplugged
and if drm_atomic_helper_shutdown() is called afterwards it's not
allowed to touch hardware.

I know it's the wrong order, but the only way to do it in the right
order is to have a separate function that sets unplugged:

	drm_dev_unregister();
	drm_atomic_helper_shutdown();
	drm_dev_set_unplugged();

Noralf.

>> + * in order to disable the hardware on regular driver module unload.
>>   */
>>  void drm_dev_unregister(struct drm_device *dev)
>>  {
>> @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
>>  	if (drm_core_check_feature(dev, DRIVER_LEGACY))
>>  		drm_lastclose(dev);
>>  
>> +	/*
>> +	 * After synchronizing any critical read section is guaranteed to see
>> +	 * the new value of ->unplugged, and any critical section which might
>> +	 * still have seen the old value of ->unplugged is guaranteed to have
>> +	 * finished.
>> +	 */
>> +	dev->unplugged = true;
>> +	synchronize_srcu(&drm_unplug_srcu);
>> +
>>  	dev->registered = false;
>>  
>>  	drm_client_dev_unregister(dev);
>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>> index ca46a45a9cce..c50696c82a42 100644
>> --- a/include/drm/drm_drv.h
>> +++ b/include/drm/drm_drv.h
>> @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
>>   * drm_dev_is_unplugged - is a DRM device unplugged
>>   * @dev: DRM device
>>   *
>> - * This function can be called to check whether a hotpluggable is unplugged.
>> - * Unplugging itself is singalled through drm_dev_unplug(). If a device is
>> - * unplugged, these two functions guarantee that any store before calling
>> - * drm_dev_unplug() is visible to callers of this function after it completes
>> + * This function can be called to check whether @dev is unregistered. This can
>> + * be used to detect that the underlying parent device is gone.
> 
> I think it'd be good to keep the first part, and just update the reference
> to drm_dev_unregister. So:
> 
>  * This function can be called to check whether a hotpluggable is unplugged.
>  * Unplugging itself is singalled through drm_dev_unregister(). If a device is
>  * unplugged, these two functions guarantee that any store before calling
>  * drm_dev_unregister() is visible to callers of this function after it
>  * completes.
> 
> I think your version shrugs a few important details under the rug. With
> those nits addressed:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Cheers, Daniel
> 
>>   *
>> - * WARNING: This function fundamentally races against drm_dev_unplug(). It is
>> - * recommended that drivers instead use the underlying drm_dev_enter() and
>> + * WARNING: This function fundamentally races against drm_dev_unregister(). It
>> + * is recommended that drivers instead use the underlying drm_dev_enter() and
>>   * drm_dev_exit() function pairs.
>>   */
>>  static inline bool drm_dev_is_unplugged(struct drm_device *dev)
>> -- 
>> 2.20.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()

[Daniel Vetter]

On Mon, Feb 04, 2019 at 06:35:28PM +0100, Noralf Trønnes wrote:
> 
> 
> Den 04.02.2019 16.41, skrev Daniel Vetter:
> > On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
> >> The only thing now that makes drm_dev_unplug() special is that it sets
> >> drm_device->unplugged. Move this code to drm_dev_unregister() so that we
> >> can remove drm_dev_unplug().
> >>
> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >> ---
> 
> [...]
> 
> >>  drivers/gpu/drm/drm_drv.c | 27 +++++++++++++++------------
> >>  include/drm/drm_drv.h     | 10 ++++------
> >>  2 files changed, 19 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >> index 05bbc2b622fc..e0941200edc6 100644
> >> --- a/drivers/gpu/drm/drm_drv.c
> >> +++ b/drivers/gpu/drm/drm_drv.c
> >> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
> >>   */
> >>  void drm_dev_unplug(struct drm_device *dev)
> >>  {
> >> -	/*
> >> -	 * After synchronizing any critical read section is guaranteed to see
> >> -	 * the new value of ->unplugged, and any critical section which might
> >> -	 * still have seen the old value of ->unplugged is guaranteed to have
> >> -	 * finished.
> >> -	 */
> >> -	dev->unplugged = true;
> >> -	synchronize_srcu(&drm_unplug_srcu);
> >> -
> >>  	drm_dev_unregister(dev);
> >>  	drm_dev_put(dev);
> >>  }
> >> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
> >>   * drm_dev_register() but does not deallocate the device. The caller must call
> >>   * drm_dev_put() to drop their final reference.
> >>   *
> >> - * A special form of unregistering for hotpluggable devices is drm_dev_unplug(),
> >> - * which can be called while there are still open users of @dev.
> >> + * This function can be called while there are still open users of @dev as long
> >> + * as the driver protects its device resources using drm_dev_enter() and
> >> + * drm_dev_exit().
> >>   *
> >>   * This should be called first in the device teardown code to make sure
> >> - * userspace can't access the device instance any more.
> >> + * userspace can't access the device instance any more. Drivers that support
> >> + * device unplug will probably want to call drm_atomic_helper_shutdown() first
> > 
> > Read once more with a bit more coffee, spotted this:
> > 
> > s/first/afterwards/ - shutting down the hw before we've taken it away from
> > userspace is kinda the wrong way round. It should be the inverse of driver
> > load, which is 1) allocate structures 2) prep hw 3) register driver with
> > the world (simplified ofc).
> > 
> 
> The problem is that drm_dev_unregister() sets the device as unplugged
> and if drm_atomic_helper_shutdown() is called afterwards it's not
> allowed to touch hardware.
> 
> I know it's the wrong order, but the only way to do it in the right
> order is to have a separate function that sets unplugged:
> 
> 	drm_dev_unregister();
> 	drm_atomic_helper_shutdown();
> 	drm_dev_set_unplugged();

Annoying ... but yeah calling _shutdown() before we stopped userspace is
also not going to work. Because userspace could quickly re-enable
something, and then the refcounts would be all wrong again and leaking
objects.

I get a bit the feeling we're over-optimizing here with trying to devm-ize
drm_dev_register. Just getting drm_device correctly devm-ized is a big
step forward already, and will open up a lot of TODO items across a lot of
drivers. E.g. we could add a drm_dev_kzalloc, for allocating all the drm_*
structs, which gets released together with drm_device. I think that's a
much clearer path forward, I think we all agree that getting the kfree out
of the driver codes is a good thing, and it would allow us to do this
correctly.

Then once we have that and rolled out to a few drivers we can reconsider
the entire unregister/shutdown gordian knot here. Atm I just have no idea
how to do this properly :-/

Thoughts, other ideas?

Cheers, Daniel

> Noralf.
> 
> >> + * in order to disable the hardware on regular driver module unload.
> >>   */
> >>  void drm_dev_unregister(struct drm_device *dev)
> >>  {
> >> @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
> >>  	if (drm_core_check_feature(dev, DRIVER_LEGACY))
> >>  		drm_lastclose(dev);
> >>  
> >> +	/*
> >> +	 * After synchronizing any critical read section is guaranteed to see
> >> +	 * the new value of ->unplugged, and any critical section which might
> >> +	 * still have seen the old value of ->unplugged is guaranteed to have
> >> +	 * finished.
> >> +	 */
> >> +	dev->unplugged = true;
> >> +	synchronize_srcu(&drm_unplug_srcu);
> >> +
> >>  	dev->registered = false;
> >>  
> >>  	drm_client_dev_unregister(dev);
> >> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> >> index ca46a45a9cce..c50696c82a42 100644
> >> --- a/include/drm/drm_drv.h
> >> +++ b/include/drm/drm_drv.h
> >> @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
> >>   * drm_dev_is_unplugged - is a DRM device unplugged
> >>   * @dev: DRM device
> >>   *
> >> - * This function can be called to check whether a hotpluggable is unplugged.
> >> - * Unplugging itself is singalled through drm_dev_unplug(). If a device is
> >> - * unplugged, these two functions guarantee that any store before calling
> >> - * drm_dev_unplug() is visible to callers of this function after it completes
> >> + * This function can be called to check whether @dev is unregistered. This can
> >> + * be used to detect that the underlying parent device is gone.
> > 
> > I think it'd be good to keep the first part, and just update the reference
> > to drm_dev_unregister. So:
> > 
> >  * This function can be called to check whether a hotpluggable is unplugged.
> >  * Unplugging itself is singalled through drm_dev_unregister(). If a device is
> >  * unplugged, these two functions guarantee that any store before calling
> >  * drm_dev_unregister() is visible to callers of this function after it
> >  * completes.
> > 
> > I think your version shrugs a few important details under the rug. With
> > those nits addressed:
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > Cheers, Daniel
> > 
> >>   *
> >> - * WARNING: This function fundamentally races against drm_dev_unplug(). It is
> >> - * recommended that drivers instead use the underlying drm_dev_enter() and
> >> + * WARNING: This function fundamentally races against drm_dev_unregister(). It
> >> + * is recommended that drivers instead use the underlying drm_dev_enter() and
> >>   * drm_dev_exit() function pairs.
> >>   */
> >>  static inline bool drm_dev_is_unplugged(struct drm_device *dev)
> >> -- 
> >> 2.20.1
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()

[Noralf Trønnes]

Den 05.02.2019 10.11, skrev Daniel Vetter:
> On Mon, Feb 04, 2019 at 06:35:28PM +0100, Noralf Trønnes wrote:
>>
>>
>> Den 04.02.2019 16.41, skrev Daniel Vetter:
>>> On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
>>>> The only thing now that makes drm_dev_unplug() special is that it sets
>>>> drm_device->unplugged. Move this code to drm_dev_unregister() so that we
>>>> can remove drm_dev_unplug().
>>>>
>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>> ---
>>
>> [...]
>>
>>>>  drivers/gpu/drm/drm_drv.c | 27 +++++++++++++++------------
>>>>  include/drm/drm_drv.h     | 10 ++++------
>>>>  2 files changed, 19 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>>> index 05bbc2b622fc..e0941200edc6 100644
>>>> --- a/drivers/gpu/drm/drm_drv.c
>>>> +++ b/drivers/gpu/drm/drm_drv.c
>>>> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
>>>>   */
>>>>  void drm_dev_unplug(struct drm_device *dev)
>>>>  {
>>>> -	/*
>>>> -	 * After synchronizing any critical read section is guaranteed to see
>>>> -	 * the new value of ->unplugged, and any critical section which might
>>>> -	 * still have seen the old value of ->unplugged is guaranteed to have
>>>> -	 * finished.
>>>> -	 */
>>>> -	dev->unplugged = true;
>>>> -	synchronize_srcu(&drm_unplug_srcu);
>>>> -
>>>>  	drm_dev_unregister(dev);
>>>>  	drm_dev_put(dev);
>>>>  }
>>>> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
>>>>   * drm_dev_register() but does not deallocate the device. The caller must call
>>>>   * drm_dev_put() to drop their final reference.
>>>>   *
>>>> - * A special form of unregistering for hotpluggable devices is drm_dev_unplug(),
>>>> - * which can be called while there are still open users of @dev.
>>>> + * This function can be called while there are still open users of @dev as long
>>>> + * as the driver protects its device resources using drm_dev_enter() and
>>>> + * drm_dev_exit().
>>>>   *
>>>>   * This should be called first in the device teardown code to make sure
>>>> - * userspace can't access the device instance any more.
>>>> + * userspace can't access the device instance any more. Drivers that support
>>>> + * device unplug will probably want to call drm_atomic_helper_shutdown() first
>>>
>>> Read once more with a bit more coffee, spotted this:
>>>
>>> s/first/afterwards/ - shutting down the hw before we've taken it away from
>>> userspace is kinda the wrong way round. It should be the inverse of driver
>>> load, which is 1) allocate structures 2) prep hw 3) register driver with
>>> the world (simplified ofc).
>>>
>>
>> The problem is that drm_dev_unregister() sets the device as unplugged
>> and if drm_atomic_helper_shutdown() is called afterwards it's not
>> allowed to touch hardware.
>>
>> I know it's the wrong order, but the only way to do it in the right
>> order is to have a separate function that sets unplugged:
>>
>> 	drm_dev_unregister();
>> 	drm_atomic_helper_shutdown();
>> 	drm_dev_set_unplugged();
> 
> Annoying ... but yeah calling _shutdown() before we stopped userspace is
> also not going to work. Because userspace could quickly re-enable
> something, and then the refcounts would be all wrong again and leaking
> objects.
> 

What happens with a USB device that is unplugged with open userspace,
will that leak objects?

> I get a bit the feeling we're over-optimizing here with trying to devm-ize
> drm_dev_register. Just getting drm_device correctly devm-ized is a big
> step forward already, and will open up a lot of TODO items across a lot of
> drivers. E.g. we could add a drm_dev_kzalloc, for allocating all the drm_*
> structs, which gets released together with drm_device. I think that's a
> much clearer path forward, I think we all agree that getting the kfree out
> of the driver codes is a good thing, and it would allow us to do this
> correctly.
> 
> Then once we have that and rolled out to a few drivers we can reconsider
> the entire unregister/shutdown gordian knot here. Atm I just have no idea
> how to do this properly :-/
> 
> Thoughts, other ideas?
> 

Yeah, I've come to the conclusion that devm_drm_dev_register() doesn't
make much sense if we need a driver remove callback anyways.

I think devm_drm_dev_init() makes sense because it yields a cleaner
probe() function. An additional benefit is that it requires a
drm_driver->release function which is a step in the right direction to
get the drm_device lifetime right.

Do we agree that a drm_dev_set_unplugged() function is necessary to get
the remove/disconnect order right?

What about drm_dev_unplug() maybe I should just leave it be?

- amd uses drm_driver->unload, so that one takes some work to get right
  to support unplug. It doesn't check the unplugged state, so really
  doesn't need drm_dev_unplug() I guess. Do they have cards that can be
  hotplugged?
- udl uses drm_driver->unload, doesn't use drm_atomic_helper_shutdown().
  It has only one drm_dev_is_unplugged() check and that is in its
  fbdev->open callback.
- xen calls drm_atomic_helper_shutdown() in its drm_driver->release
  callback which I guess is not correct.

This is what it will look like with a set unplugged function:

void drm_dev_unplug(struct drm_device *dev)
{
	drm_dev_set_unplugged(dev);
	drm_dev_unregister(dev);
	drm_dev_put(dev);
}

Hm, I should probably remove it to avoid further use of it since it's
not correct for a modern driver.

Noralf.

> Cheers, Daniel
> 
>> Noralf.
>>
>>>> + * in order to disable the hardware on regular driver module unload.
>>>>   */
>>>>  void drm_dev_unregister(struct drm_device *dev)
>>>>  {
>>>> @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
>>>>  	if (drm_core_check_feature(dev, DRIVER_LEGACY))
>>>>  		drm_lastclose(dev);
>>>>  
>>>> +	/*
>>>> +	 * After synchronizing any critical read section is guaranteed to see
>>>> +	 * the new value of ->unplugged, and any critical section which might
>>>> +	 * still have seen the old value of ->unplugged is guaranteed to have
>>>> +	 * finished.
>>>> +	 */
>>>> +	dev->unplugged = true;
>>>> +	synchronize_srcu(&drm_unplug_srcu);
>>>> +
>>>>  	dev->registered = false;
>>>>  
>>>>  	drm_client_dev_unregister(dev);
>>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>>>> index ca46a45a9cce..c50696c82a42 100644
>>>> --- a/include/drm/drm_drv.h
>>>> +++ b/include/drm/drm_drv.h
>>>> @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
>>>>   * drm_dev_is_unplugged - is a DRM device unplugged
>>>>   * @dev: DRM device
>>>>   *
>>>> - * This function can be called to check whether a hotpluggable is unplugged.
>>>> - * Unplugging itself is singalled through drm_dev_unplug(). If a device is
>>>> - * unplugged, these two functions guarantee that any store before calling
>>>> - * drm_dev_unplug() is visible to callers of this function after it completes
>>>> + * This function can be called to check whether @dev is unregistered. This can
>>>> + * be used to detect that the underlying parent device is gone.
>>>
>>> I think it'd be good to keep the first part, and just update the reference
>>> to drm_dev_unregister. So:
>>>
>>>  * This function can be called to check whether a hotpluggable is unplugged.
>>>  * Unplugging itself is singalled through drm_dev_unregister(). If a device is
>>>  * unplugged, these two functions guarantee that any store before calling
>>>  * drm_dev_unregister() is visible to callers of this function after it
>>>  * completes.
>>>
>>> I think your version shrugs a few important details under the rug. With
>>> those nits addressed:
>>>
>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>
>>> Cheers, Daniel
>>>
>>>>   *
>>>> - * WARNING: This function fundamentally races against drm_dev_unplug(). It is
>>>> - * recommended that drivers instead use the underlying drm_dev_enter() and
>>>> + * WARNING: This function fundamentally races against drm_dev_unregister(). It
>>>> + * is recommended that drivers instead use the underlying drm_dev_enter() and
>>>>   * drm_dev_exit() function pairs.
>>>>   */
>>>>  static inline bool drm_dev_is_unplugged(struct drm_device *dev)
>>>> -- 
>>>> 2.20.1
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()

[Daniel Vetter]

On Tue, Feb 05, 2019 at 11:20:55AM +0100, Noralf Trønnes wrote:
> 
> 
> Den 05.02.2019 10.11, skrev Daniel Vetter:
> > On Mon, Feb 04, 2019 at 06:35:28PM +0100, Noralf Trønnes wrote:
> >>
> >>
> >> Den 04.02.2019 16.41, skrev Daniel Vetter:
> >>> On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
> >>>> The only thing now that makes drm_dev_unplug() special is that it sets
> >>>> drm_device->unplugged. Move this code to drm_dev_unregister() so that we
> >>>> can remove drm_dev_unplug().
> >>>>
> >>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >>>> ---
> >>
> >> [...]
> >>
> >>>>  drivers/gpu/drm/drm_drv.c | 27 +++++++++++++++------------
> >>>>  include/drm/drm_drv.h     | 10 ++++------
> >>>>  2 files changed, 19 insertions(+), 18 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >>>> index 05bbc2b622fc..e0941200edc6 100644
> >>>> --- a/drivers/gpu/drm/drm_drv.c
> >>>> +++ b/drivers/gpu/drm/drm_drv.c
> >>>> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
> >>>>   */
> >>>>  void drm_dev_unplug(struct drm_device *dev)
> >>>>  {
> >>>> -	/*
> >>>> -	 * After synchronizing any critical read section is guaranteed to see
> >>>> -	 * the new value of ->unplugged, and any critical section which might
> >>>> -	 * still have seen the old value of ->unplugged is guaranteed to have
> >>>> -	 * finished.
> >>>> -	 */
> >>>> -	dev->unplugged = true;
> >>>> -	synchronize_srcu(&drm_unplug_srcu);
> >>>> -
> >>>>  	drm_dev_unregister(dev);
> >>>>  	drm_dev_put(dev);
> >>>>  }
> >>>> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
> >>>>   * drm_dev_register() but does not deallocate the device. The caller must call
> >>>>   * drm_dev_put() to drop their final reference.
> >>>>   *
> >>>> - * A special form of unregistering for hotpluggable devices is drm_dev_unplug(),
> >>>> - * which can be called while there are still open users of @dev.
> >>>> + * This function can be called while there are still open users of @dev as long
> >>>> + * as the driver protects its device resources using drm_dev_enter() and
> >>>> + * drm_dev_exit().
> >>>>   *
> >>>>   * This should be called first in the device teardown code to make sure
> >>>> - * userspace can't access the device instance any more.
> >>>> + * userspace can't access the device instance any more. Drivers that support
> >>>> + * device unplug will probably want to call drm_atomic_helper_shutdown() first
> >>>
> >>> Read once more with a bit more coffee, spotted this:
> >>>
> >>> s/first/afterwards/ - shutting down the hw before we've taken it away from
> >>> userspace is kinda the wrong way round. It should be the inverse of driver
> >>> load, which is 1) allocate structures 2) prep hw 3) register driver with
> >>> the world (simplified ofc).
> >>>
> >>
> >> The problem is that drm_dev_unregister() sets the device as unplugged
> >> and if drm_atomic_helper_shutdown() is called afterwards it's not
> >> allowed to touch hardware.
> >>
> >> I know it's the wrong order, but the only way to do it in the right
> >> order is to have a separate function that sets unplugged:
> >>
> >> 	drm_dev_unregister();
> >> 	drm_atomic_helper_shutdown();
> >> 	drm_dev_set_unplugged();
> > 
> > Annoying ... but yeah calling _shutdown() before we stopped userspace is
> > also not going to work. Because userspace could quickly re-enable
> > something, and then the refcounts would be all wrong again and leaking
> > objects.
> > 
> 
> What happens with a USB device that is unplugged with open userspace,
> will that leak objects?

Maybe we've jumped to conclusions. drm_atomic_helper_shutdown() will run
as normal, the only thing that should be skipped is actually touching the
hw (as long as the driver doesn't protect too much with
drm_dev_enter/exit). So all the software updates (including refcounting
updates) will still be done. Ofc current udl is not yet atomic, so in
reality something else happens.

And we ofc still have the same issue that if you just unload the driver,
then the hw will stay on (which might really confuse the driver on next
load, when it assumes that it only gets loaded from cold boot where
everything is off - which usually is the case on an arm soc at least).

> > I get a bit the feeling we're over-optimizing here with trying to devm-ize
> > drm_dev_register. Just getting drm_device correctly devm-ized is a big
> > step forward already, and will open up a lot of TODO items across a lot of
> > drivers. E.g. we could add a drm_dev_kzalloc, for allocating all the drm_*
> > structs, which gets released together with drm_device. I think that's a
> > much clearer path forward, I think we all agree that getting the kfree out
> > of the driver codes is a good thing, and it would allow us to do this
> > correctly.
> > 
> > Then once we have that and rolled out to a few drivers we can reconsider
> > the entire unregister/shutdown gordian knot here. Atm I just have no idea
> > how to do this properly :-/
> > 
> > Thoughts, other ideas?
> > 
> 
> Yeah, I've come to the conclusion that devm_drm_dev_register() doesn't
> make much sense if we need a driver remove callback anyways.

Yup, that's what I meant with the above:
- merge devm_drm_dev_register, use it a lot. This is definitely a good
  idea.
- create a drm_dev_kzalloc, which automatically releases memory on the
  final drm_dev_put. Use it every in drivers for drm structures,
  especially in those drivers that currently use devm (which releases
  those allocations potentialy too early on unplug).
- figure out the next steps after that

> I think devm_drm_dev_init() makes sense because it yields a cleaner
> probe() function. An additional benefit is that it requires a
> drm_driver->release function which is a step in the right direction to
> get the drm_device lifetime right.
> 
> Do we agree that a drm_dev_set_unplugged() function is necessary to get
> the remove/disconnect order right?
> 
> What about drm_dev_unplug() maybe I should just leave it be?
> 
> - amd uses drm_driver->unload, so that one takes some work to get right
>   to support unplug. It doesn't check the unplugged state, so really
>   doesn't need drm_dev_unplug() I guess. Do they have cards that can be
>   hotplugged?

Yeah amd still uses ->load and ->unload, which is not great unfortunately.
I just stumbled over that when I tried to make a series to disable the
global drm_global_mutex for most drivers ...

> - udl uses drm_driver->unload, doesn't use drm_atomic_helper_shutdown().
>   It has only one drm_dev_is_unplugged() check and that is in its
>   fbdev->open callback.

udl isn't atomic, so can't use the atomic helpers. pre-atomic doesn't have
refcounting issues when something is left on iirc. I think udl is written
under the assumption that ->unload is always called for an unplug, never
for an actual unload.

> - xen calls drm_atomic_helper_shutdown() in its drm_driver->release
>   callback which I guess is not correct.

Yeah this smells fishy. ->release is supposed to be for cleaning up kernel
structures, not for cleaning up the hw. So maybe drm_mode_config_cleanup
could be put there, not sure honestly. But definitely not _shutdown().

> This is what it will look like with a set unplugged function:
> 
> void drm_dev_unplug(struct drm_device *dev)
> {
> 	drm_dev_set_unplugged(dev);
> 	drm_dev_unregister(dev);
> 	drm_dev_put(dev);
> }
> 
> Hm, I should probably remove it to avoid further use of it since it's
> not correct for a modern driver.

I think something flew over my head ... what's the drm_dev_set_unplugged
for? I think I'm not following you ...

Thanks, Daniel

> 
> Noralf.
> 
> > Cheers, Daniel
> > 
> >> Noralf.
> >>
> >>>> + * in order to disable the hardware on regular driver module unload.
> >>>>   */
> >>>>  void drm_dev_unregister(struct drm_device *dev)
> >>>>  {
> >>>> @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
> >>>>  	if (drm_core_check_feature(dev, DRIVER_LEGACY))
> >>>>  		drm_lastclose(dev);
> >>>>  
> >>>> +	/*
> >>>> +	 * After synchronizing any critical read section is guaranteed to see
> >>>> +	 * the new value of ->unplugged, and any critical section which might
> >>>> +	 * still have seen the old value of ->unplugged is guaranteed to have
> >>>> +	 * finished.
> >>>> +	 */
> >>>> +	dev->unplugged = true;
> >>>> +	synchronize_srcu(&drm_unplug_srcu);
> >>>> +
> >>>>  	dev->registered = false;
> >>>>  
> >>>>  	drm_client_dev_unregister(dev);
> >>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> >>>> index ca46a45a9cce..c50696c82a42 100644
> >>>> --- a/include/drm/drm_drv.h
> >>>> +++ b/include/drm/drm_drv.h
> >>>> @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
> >>>>   * drm_dev_is_unplugged - is a DRM device unplugged
> >>>>   * @dev: DRM device
> >>>>   *
> >>>> - * This function can be called to check whether a hotpluggable is unplugged.
> >>>> - * Unplugging itself is singalled through drm_dev_unplug(). If a device is
> >>>> - * unplugged, these two functions guarantee that any store before calling
> >>>> - * drm_dev_unplug() is visible to callers of this function after it completes
> >>>> + * This function can be called to check whether @dev is unregistered. This can
> >>>> + * be used to detect that the underlying parent device is gone.
> >>>
> >>> I think it'd be good to keep the first part, and just update the reference
> >>> to drm_dev_unregister. So:
> >>>
> >>>  * This function can be called to check whether a hotpluggable is unplugged.
> >>>  * Unplugging itself is singalled through drm_dev_unregister(). If a device is
> >>>  * unplugged, these two functions guarantee that any store before calling
> >>>  * drm_dev_unregister() is visible to callers of this function after it
> >>>  * completes.
> >>>
> >>> I think your version shrugs a few important details under the rug. With
> >>> those nits addressed:
> >>>
> >>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>
> >>> Cheers, Daniel
> >>>
> >>>>   *
> >>>> - * WARNING: This function fundamentally races against drm_dev_unplug(). It is
> >>>> - * recommended that drivers instead use the underlying drm_dev_enter() and
> >>>> + * WARNING: This function fundamentally races against drm_dev_unregister(). It
> >>>> + * is recommended that drivers instead use the underlying drm_dev_enter() and
> >>>>   * drm_dev_exit() function pairs.
> >>>>   */
> >>>>  static inline bool drm_dev_is_unplugged(struct drm_device *dev)
> >>>> -- 
> >>>> 2.20.1
> >>>>
> >>>> _______________________________________________
> >>>> Intel-gfx mailing list
> >>>> Intel-gfx@lists.freedesktop.org
> >>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>>
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()

[Noralf Trønnes]

Den 05.02.2019 17.31, skrev Daniel Vetter:
> On Tue, Feb 05, 2019 at 11:20:55AM +0100, Noralf Trønnes wrote:
>>
>>
>> Den 05.02.2019 10.11, skrev Daniel Vetter:
>>> On Mon, Feb 04, 2019 at 06:35:28PM +0100, Noralf Trønnes wrote:
>>>>
>>>>
>>>> Den 04.02.2019 16.41, skrev Daniel Vetter:
>>>>> On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
>>>>>> The only thing now that makes drm_dev_unplug() special is that it sets
>>>>>> drm_device->unplugged. Move this code to drm_dev_unregister() so that we
>>>>>> can remove drm_dev_unplug().
>>>>>>
>>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>>>> ---
>>>>
>>>> [...]
>>>>
>>>>>>  drivers/gpu/drm/drm_drv.c | 27 +++++++++++++++------------
>>>>>>  include/drm/drm_drv.h     | 10 ++++------
>>>>>>  2 files changed, 19 insertions(+), 18 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>>>>> index 05bbc2b622fc..e0941200edc6 100644
>>>>>> --- a/drivers/gpu/drm/drm_drv.c
>>>>>> +++ b/drivers/gpu/drm/drm_drv.c
>>>>>> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
>>>>>>   */
>>>>>>  void drm_dev_unplug(struct drm_device *dev)
>>>>>>  {
>>>>>> -	/*
>>>>>> -	 * After synchronizing any critical read section is guaranteed to see
>>>>>> -	 * the new value of ->unplugged, and any critical section which might
>>>>>> -	 * still have seen the old value of ->unplugged is guaranteed to have
>>>>>> -	 * finished.
>>>>>> -	 */
>>>>>> -	dev->unplugged = true;
>>>>>> -	synchronize_srcu(&drm_unplug_srcu);
>>>>>> -
>>>>>>  	drm_dev_unregister(dev);
>>>>>>  	drm_dev_put(dev);
>>>>>>  }
>>>>>> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
>>>>>>   * drm_dev_register() but does not deallocate the device. The caller must call
>>>>>>   * drm_dev_put() to drop their final reference.
>>>>>>   *
>>>>>> - * A special form of unregistering for hotpluggable devices is drm_dev_unplug(),
>>>>>> - * which can be called while there are still open users of @dev.
>>>>>> + * This function can be called while there are still open users of @dev as long
>>>>>> + * as the driver protects its device resources using drm_dev_enter() and
>>>>>> + * drm_dev_exit().
>>>>>>   *
>>>>>>   * This should be called first in the device teardown code to make sure
>>>>>> - * userspace can't access the device instance any more.
>>>>>> + * userspace can't access the device instance any more. Drivers that support
>>>>>> + * device unplug will probably want to call drm_atomic_helper_shutdown() first
>>>>>
>>>>> Read once more with a bit more coffee, spotted this:
>>>>>
>>>>> s/first/afterwards/ - shutting down the hw before we've taken it away from
>>>>> userspace is kinda the wrong way round. It should be the inverse of driver
>>>>> load, which is 1) allocate structures 2) prep hw 3) register driver with
>>>>> the world (simplified ofc).
>>>>>
>>>>
>>>> The problem is that drm_dev_unregister() sets the device as unplugged
>>>> and if drm_atomic_helper_shutdown() is called afterwards it's not
>>>> allowed to touch hardware.
>>>>
>>>> I know it's the wrong order, but the only way to do it in the right
>>>> order is to have a separate function that sets unplugged:
>>>>
>>>> 	drm_dev_unregister();
>>>> 	drm_atomic_helper_shutdown();
>>>> 	drm_dev_set_unplugged();
>>>
>>> Annoying ... but yeah calling _shutdown() before we stopped userspace is
>>> also not going to work. Because userspace could quickly re-enable
>>> something, and then the refcounts would be all wrong again and leaking
>>> objects.
>>>
>>
>> What happens with a USB device that is unplugged with open userspace,
>> will that leak objects?
> 
> Maybe we've jumped to conclusions. drm_atomic_helper_shutdown() will run
> as normal, the only thing that should be skipped is actually touching the
> hw (as long as the driver doesn't protect too much with
> drm_dev_enter/exit). So all the software updates (including refcounting
> updates) will still be done. Ofc current udl is not yet atomic, so in
> reality something else happens.
> 
> And we ofc still have the same issue that if you just unload the driver,
> then the hw will stay on (which might really confuse the driver on next
> load, when it assumes that it only gets loaded from cold boot where
> everything is off - which usually is the case on an arm soc at least).
> 
>>> I get a bit the feeling we're over-optimizing here with trying to devm-ize
>>> drm_dev_register. Just getting drm_device correctly devm-ized is a big
>>> step forward already, and will open up a lot of TODO items across a lot of
>>> drivers. E.g. we could add a drm_dev_kzalloc, for allocating all the drm_*
>>> structs, which gets released together with drm_device. I think that's a
>>> much clearer path forward, I think we all agree that getting the kfree out
>>> of the driver codes is a good thing, and it would allow us to do this
>>> correctly.
>>>
>>> Then once we have that and rolled out to a few drivers we can reconsider
>>> the entire unregister/shutdown gordian knot here. Atm I just have no idea
>>> how to do this properly :-/
>>>
>>> Thoughts, other ideas?
>>>
>>
>> Yeah, I've come to the conclusion that devm_drm_dev_register() doesn't
>> make much sense if we need a driver remove callback anyways.
> 
> Yup, that's what I meant with the above:
> - merge devm_drm_dev_register, use it a lot. This is definitely a good
>   idea.
> - create a drm_dev_kzalloc, which automatically releases memory on the
>   final drm_dev_put. Use it every in drivers for drm structures,
>   especially in those drivers that currently use devm (which releases
>   those allocations potentialy too early on unplug).
> - figure out the next steps after that
> 
>> I think devm_drm_dev_init() makes sense because it yields a cleaner
>> probe() function. An additional benefit is that it requires a
>> drm_driver->release function which is a step in the right direction to
>> get the drm_device lifetime right.
>>
>> Do we agree that a drm_dev_set_unplugged() function is necessary to get
>> the remove/disconnect order right?
>>
>> What about drm_dev_unplug() maybe I should just leave it be?
>>
>> - amd uses drm_driver->unload, so that one takes some work to get right
>>   to support unplug. It doesn't check the unplugged state, so really
>>   doesn't need drm_dev_unplug() I guess. Do they have cards that can be
>>   hotplugged?
> 
> Yeah amd still uses ->load and ->unload, which is not great unfortunately.
> I just stumbled over that when I tried to make a series to disable the
> global drm_global_mutex for most drivers ...
> 
>> - udl uses drm_driver->unload, doesn't use drm_atomic_helper_shutdown().
>>   It has only one drm_dev_is_unplugged() check and that is in its
>>   fbdev->open callback.
> 
> udl isn't atomic, so can't use the atomic helpers. pre-atomic doesn't have
> refcounting issues when something is left on iirc. I think udl is written
> under the assumption that ->unload is always called for an unplug, never
> for an actual unload.
> 
>> - xen calls drm_atomic_helper_shutdown() in its drm_driver->release
>>   callback which I guess is not correct.
> 
> Yeah this smells fishy. ->release is supposed to be for cleaning up kernel
> structures, not for cleaning up the hw. So maybe drm_mode_config_cleanup
> could be put there, not sure honestly. But definitely not _shutdown().
> 
>> This is what it will look like with a set unplugged function:
>>
>> void drm_dev_unplug(struct drm_device *dev)
>> {
>> 	drm_dev_set_unplugged(dev);
>> 	drm_dev_unregister(dev);
>> 	drm_dev_put(dev);
>> }
>>
>> Hm, I should probably remove it to avoid further use of it since it's
>> not correct for a modern driver.
> 
> I think something flew over my head ... what's the drm_dev_set_unplugged
> for? I think I'm not following you ...
> 

Taking it a few steps back:

This patch proposes to move 'dev->unplugged = true' from
drm_dev_unplug() to drm_dev_unregister().

Additionally I proposed this change to the drm_dev_unregister() docs:

  * This should be called first in the device teardown code to make sure
- * userspace can't access the device instance any more.
+ * userspace can't access the device instance any more. Drivers that
support
+ * device unplug will probably want to call
drm_atomic_helper_shutdown() first
+ * in order to disable the hardware on regular driver module unload.

Which would give a driver remove callback like this:

static int driver_remove()
{
	drm_atomic_helper_shutdown();
	drm_dev_unregister();
}

Your reaction was that drm_atomic_helper_shutdown() needs to be called
after drm_dev_unregister() to avoid a race resulting in leaked objects.
However if we call it afterwards, ->unplugged will be true and the
driver can't touch hardware.

Then I proposed moving the unplugged state setting to:

void drm_dev_set_unplugged(struct drm_device *dev)
{
	dev->unplugged = true;
	synchronize_srcu(&drm_unplug_srcu);
}

Now it is possible to avoid the race and still touch hardware:

static int driver_remove()
{
	drm_dev_unregister();
	drm_atomic_helper_shutdown();
	drm_dev_set_unplugged();
}

But now I'm back to the question: Is it the driver or the device that is
going away?

If it's the driver we are fine touching hardware, if it's the device it
depends on how we access the device resource and whether the subsystem
has protection in place to handle access after the device is gone. I
think USB can handle and block device access up until the disconnect
callback has finished (no point in doing so though, since the normal
operation is that the device is gone, not the driver unloading).

Is there a way to determine who's going away without changes to the
device core?

Maybe. The driver can only be unregistered if there are no open file
handles because a ref is taken on the driver module.

So maybe something along these lines:

int drm_dev_open_count(struct drm_device *dev)
{
	int open_count;

	mutex_lock(&drm_global_mutex);
	open_count = dev->open_count;
	mutex_unlock(&drm_global_mutex);

	return open_count;
}

static int driver_remove()
{
	drm_dev_unregister();

	open_count = drm_dev_open_count();

	/* Open fd's, device is going away, block access */
	if (open_count)
		drm_dev_set_unplugged();

	drm_atomic_helper_shutdown();

	/* No open fd's, driver is going away */
	if (!open_count)
		drm_dev_set_unplugged();
}


Personally I have 2 use cases:
- tinydrm SPI drivers
  The only hotpluggable SPI controllers I know of are USB which should
  handle device access while unregistering.
  SPI devices can be removed, but the controller driver is still in
  place so it's safe.
- A future USB driver (that I hope to work on when all this core stuff
  is in place).
  There's no point in touching the hw, so DRM can be set uplugged right
  away in the disconnect() callback.

This means that I don't need to know who's going away for my use cases.

This turned out to be quite long winding, but the bottom line is that
having a separate function to set the unplugged state seems to give a
lot of flexibility for various use cases.

I hope I didn't muddy the waters even more :-)

Noralf.

> Thanks, Daniel
> 
>>
>> Noralf.
>>
>>> Cheers, Daniel
>>>
>>>> Noralf.
>>>>
>>>>>> + * in order to disable the hardware on regular driver module unload.
>>>>>>   */
>>>>>>  void drm_dev_unregister(struct drm_device *dev)
>>>>>>  {
>>>>>> @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
>>>>>>  	if (drm_core_check_feature(dev, DRIVER_LEGACY))
>>>>>>  		drm_lastclose(dev);
>>>>>>  
>>>>>> +	/*
>>>>>> +	 * After synchronizing any critical read section is guaranteed to see
>>>>>> +	 * the new value of ->unplugged, and any critical section which might
>>>>>> +	 * still have seen the old value of ->unplugged is guaranteed to have
>>>>>> +	 * finished.
>>>>>> +	 */
>>>>>> +	dev->unplugged = true;
>>>>>> +	synchronize_srcu(&drm_unplug_srcu);
>>>>>> +
>>>>>>  	dev->registered = false;
>>>>>>  
>>>>>>  	drm_client_dev_unregister(dev);
>>>>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>>>>>> index ca46a45a9cce..c50696c82a42 100644
>>>>>> --- a/include/drm/drm_drv.h
>>>>>> +++ b/include/drm/drm_drv.h
>>>>>> @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
>>>>>>   * drm_dev_is_unplugged - is a DRM device unplugged
>>>>>>   * @dev: DRM device
>>>>>>   *
>>>>>> - * This function can be called to check whether a hotpluggable is unplugged.
>>>>>> - * Unplugging itself is singalled through drm_dev_unplug(). If a device is
>>>>>> - * unplugged, these two functions guarantee that any store before calling
>>>>>> - * drm_dev_unplug() is visible to callers of this function after it completes
>>>>>> + * This function can be called to check whether @dev is unregistered. This can
>>>>>> + * be used to detect that the underlying parent device is gone.
>>>>>
>>>>> I think it'd be good to keep the first part, and just update the reference
>>>>> to drm_dev_unregister. So:
>>>>>
>>>>>  * This function can be called to check whether a hotpluggable is unplugged.
>>>>>  * Unplugging itself is singalled through drm_dev_unregister(). If a device is
>>>>>  * unplugged, these two functions guarantee that any store before calling
>>>>>  * drm_dev_unregister() is visible to callers of this function after it
>>>>>  * completes.
>>>>>
>>>>> I think your version shrugs a few important details under the rug. With
>>>>> those nits addressed:
>>>>>
>>>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>>
>>>>> Cheers, Daniel
>>>>>
>>>>>>   *
>>>>>> - * WARNING: This function fundamentally races against drm_dev_unplug(). It is
>>>>>> - * recommended that drivers instead use the underlying drm_dev_enter() and
>>>>>> + * WARNING: This function fundamentally races against drm_dev_unregister(). It
>>>>>> + * is recommended that drivers instead use the underlying drm_dev_enter() and
>>>>>>   * drm_dev_exit() function pairs.
>>>>>>   */
>>>>>>  static inline bool drm_dev_is_unplugged(struct drm_device *dev)
>>>>>> -- 
>>>>>> 2.20.1
>>>>>>
>>>>>> _______________________________________________
>>>>>> Intel-gfx mailing list
>>>>>> Intel-gfx@lists.freedesktop.org
>>>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>>>
>>>
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()

[Daniel Vetter]

On Tue, Feb 05, 2019 at 06:57:50PM +0100, Noralf Trønnes wrote:
> 
> 
> Den 05.02.2019 17.31, skrev Daniel Vetter:
> > On Tue, Feb 05, 2019 at 11:20:55AM +0100, Noralf Trønnes wrote:
> >>
> >>
> >> Den 05.02.2019 10.11, skrev Daniel Vetter:
> >>> On Mon, Feb 04, 2019 at 06:35:28PM +0100, Noralf Trønnes wrote:
> >>>>
> >>>>
> >>>> Den 04.02.2019 16.41, skrev Daniel Vetter:
> >>>>> On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
> >>>>>> The only thing now that makes drm_dev_unplug() special is that it sets
> >>>>>> drm_device->unplugged. Move this code to drm_dev_unregister() so that we
> >>>>>> can remove drm_dev_unplug().
> >>>>>>
> >>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >>>>>> ---
> >>>>
> >>>> [...]
> >>>>
> >>>>>>  drivers/gpu/drm/drm_drv.c | 27 +++++++++++++++------------
> >>>>>>  include/drm/drm_drv.h     | 10 ++++------
> >>>>>>  2 files changed, 19 insertions(+), 18 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >>>>>> index 05bbc2b622fc..e0941200edc6 100644
> >>>>>> --- a/drivers/gpu/drm/drm_drv.c
> >>>>>> +++ b/drivers/gpu/drm/drm_drv.c
> >>>>>> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
> >>>>>>   */
> >>>>>>  void drm_dev_unplug(struct drm_device *dev)
> >>>>>>  {
> >>>>>> -	/*
> >>>>>> -	 * After synchronizing any critical read section is guaranteed to see
> >>>>>> -	 * the new value of ->unplugged, and any critical section which might
> >>>>>> -	 * still have seen the old value of ->unplugged is guaranteed to have
> >>>>>> -	 * finished.
> >>>>>> -	 */
> >>>>>> -	dev->unplugged = true;
> >>>>>> -	synchronize_srcu(&drm_unplug_srcu);
> >>>>>> -
> >>>>>>  	drm_dev_unregister(dev);
> >>>>>>  	drm_dev_put(dev);
> >>>>>>  }
> >>>>>> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
> >>>>>>   * drm_dev_register() but does not deallocate the device. The caller must call
> >>>>>>   * drm_dev_put() to drop their final reference.
> >>>>>>   *
> >>>>>> - * A special form of unregistering for hotpluggable devices is drm_dev_unplug(),
> >>>>>> - * which can be called while there are still open users of @dev.
> >>>>>> + * This function can be called while there are still open users of @dev as long
> >>>>>> + * as the driver protects its device resources using drm_dev_enter() and
> >>>>>> + * drm_dev_exit().
> >>>>>>   *
> >>>>>>   * This should be called first in the device teardown code to make sure
> >>>>>> - * userspace can't access the device instance any more.
> >>>>>> + * userspace can't access the device instance any more. Drivers that support
> >>>>>> + * device unplug will probably want to call drm_atomic_helper_shutdown() first
> >>>>>
> >>>>> Read once more with a bit more coffee, spotted this:
> >>>>>
> >>>>> s/first/afterwards/ - shutting down the hw before we've taken it away from
> >>>>> userspace is kinda the wrong way round. It should be the inverse of driver
> >>>>> load, which is 1) allocate structures 2) prep hw 3) register driver with
> >>>>> the world (simplified ofc).
> >>>>>
> >>>>
> >>>> The problem is that drm_dev_unregister() sets the device as unplugged
> >>>> and if drm_atomic_helper_shutdown() is called afterwards it's not
> >>>> allowed to touch hardware.
> >>>>
> >>>> I know it's the wrong order, but the only way to do it in the right
> >>>> order is to have a separate function that sets unplugged:
> >>>>
> >>>> 	drm_dev_unregister();
> >>>> 	drm_atomic_helper_shutdown();
> >>>> 	drm_dev_set_unplugged();
> >>>
> >>> Annoying ... but yeah calling _shutdown() before we stopped userspace is
> >>> also not going to work. Because userspace could quickly re-enable
> >>> something, and then the refcounts would be all wrong again and leaking
> >>> objects.
> >>>
> >>
> >> What happens with a USB device that is unplugged with open userspace,
> >> will that leak objects?
> > 
> > Maybe we've jumped to conclusions. drm_atomic_helper_shutdown() will run
> > as normal, the only thing that should be skipped is actually touching the
> > hw (as long as the driver doesn't protect too much with
> > drm_dev_enter/exit). So all the software updates (including refcounting
> > updates) will still be done. Ofc current udl is not yet atomic, so in
> > reality something else happens.
> > 
> > And we ofc still have the same issue that if you just unload the driver,
> > then the hw will stay on (which might really confuse the driver on next
> > load, when it assumes that it only gets loaded from cold boot where
> > everything is off - which usually is the case on an arm soc at least).
> > 
> >>> I get a bit the feeling we're over-optimizing here with trying to devm-ize
> >>> drm_dev_register. Just getting drm_device correctly devm-ized is a big
> >>> step forward already, and will open up a lot of TODO items across a lot of
> >>> drivers. E.g. we could add a drm_dev_kzalloc, for allocating all the drm_*
> >>> structs, which gets released together with drm_device. I think that's a
> >>> much clearer path forward, I think we all agree that getting the kfree out
> >>> of the driver codes is a good thing, and it would allow us to do this
> >>> correctly.
> >>>
> >>> Then once we have that and rolled out to a few drivers we can reconsider
> >>> the entire unregister/shutdown gordian knot here. Atm I just have no idea
> >>> how to do this properly :-/
> >>>
> >>> Thoughts, other ideas?
> >>>
> >>
> >> Yeah, I've come to the conclusion that devm_drm_dev_register() doesn't
> >> make much sense if we need a driver remove callback anyways.
> > 
> > Yup, that's what I meant with the above:
> > - merge devm_drm_dev_register, use it a lot. This is definitely a good
> >   idea.
> > - create a drm_dev_kzalloc, which automatically releases memory on the
> >   final drm_dev_put. Use it every in drivers for drm structures,
> >   especially in those drivers that currently use devm (which releases
> >   those allocations potentialy too early on unplug).
> > - figure out the next steps after that
> > 
> >> I think devm_drm_dev_init() makes sense because it yields a cleaner
> >> probe() function. An additional benefit is that it requires a
> >> drm_driver->release function which is a step in the right direction to
> >> get the drm_device lifetime right.
> >>
> >> Do we agree that a drm_dev_set_unplugged() function is necessary to get
> >> the remove/disconnect order right?
> >>
> >> What about drm_dev_unplug() maybe I should just leave it be?
> >>
> >> - amd uses drm_driver->unload, so that one takes some work to get right
> >>   to support unplug. It doesn't check the unplugged state, so really
> >>   doesn't need drm_dev_unplug() I guess. Do they have cards that can be
> >>   hotplugged?
> > 
> > Yeah amd still uses ->load and ->unload, which is not great unfortunately.
> > I just stumbled over that when I tried to make a series to disable the
> > global drm_global_mutex for most drivers ...
> > 
> >> - udl uses drm_driver->unload, doesn't use drm_atomic_helper_shutdown().
> >>   It has only one drm_dev_is_unplugged() check and that is in its
> >>   fbdev->open callback.
> > 
> > udl isn't atomic, so can't use the atomic helpers. pre-atomic doesn't have
> > refcounting issues when something is left on iirc. I think udl is written
> > under the assumption that ->unload is always called for an unplug, never
> > for an actual unload.
> > 
> >> - xen calls drm_atomic_helper_shutdown() in its drm_driver->release
> >>   callback which I guess is not correct.
> > 
> > Yeah this smells fishy. ->release is supposed to be for cleaning up kernel
> > structures, not for cleaning up the hw. So maybe drm_mode_config_cleanup
> > could be put there, not sure honestly. But definitely not _shutdown().
> > 
> >> This is what it will look like with a set unplugged function:
> >>
> >> void drm_dev_unplug(struct drm_device *dev)
> >> {
> >> 	drm_dev_set_unplugged(dev);
> >> 	drm_dev_unregister(dev);
> >> 	drm_dev_put(dev);
> >> }
> >>
> >> Hm, I should probably remove it to avoid further use of it since it's
> >> not correct for a modern driver.
> > 
> > I think something flew over my head ... what's the drm_dev_set_unplugged
> > for? I think I'm not following you ...
> > 
> 
> Taking it a few steps back:
> 
> This patch proposes to move 'dev->unplugged = true' from
> drm_dev_unplug() to drm_dev_unregister().
> 
> Additionally I proposed this change to the drm_dev_unregister() docs:
> 
>   * This should be called first in the device teardown code to make sure
> - * userspace can't access the device instance any more.
> + * userspace can't access the device instance any more. Drivers that
> support
> + * device unplug will probably want to call
> drm_atomic_helper_shutdown() first
> + * in order to disable the hardware on regular driver module unload.
> 
> Which would give a driver remove callback like this:
> 
> static int driver_remove()
> {
> 	drm_atomic_helper_shutdown();
> 	drm_dev_unregister();
> }
> 
> Your reaction was that drm_atomic_helper_shutdown() needs to be called
> after drm_dev_unregister() to avoid a race resulting in leaked objects.
> However if we call it afterwards, ->unplugged will be true and the
> driver can't touch hardware.
> 
> Then I proposed moving the unplugged state setting to:
> 
> void drm_dev_set_unplugged(struct drm_device *dev)
> {
> 	dev->unplugged = true;
> 	synchronize_srcu(&drm_unplug_srcu);
> }
> 
> Now it is possible to avoid the race and still touch hardware:
> 
> static int driver_remove()
> {
> 	drm_dev_unregister();
> 	drm_atomic_helper_shutdown();
> 	drm_dev_set_unplugged();
> }
> 
> But now I'm back to the question: Is it the driver or the device that is
> going away?
> 
> If it's the driver we are fine touching hardware, if it's the device it
> depends on how we access the device resource and whether the subsystem
> has protection in place to handle access after the device is gone. I
> think USB can handle and block device access up until the disconnect
> callback has finished (no point in doing so though, since the normal
> operation is that the device is gone, not the driver unloading).
> 
> Is there a way to determine who's going away without changes to the
> device core?
> 
> Maybe. The driver can only be unregistered if there are no open file
> handles because a ref is taken on the driver module.

This isn't true. You can (not many people do, but it's possible) to unbind
through sysfs. The module reference only keeps the cpu instructions valid,
nothing else.

> So maybe something along these lines:
> 
> int drm_dev_open_count(struct drm_device *dev)
> {
> 	int open_count;
> 
> 	mutex_lock(&drm_global_mutex);
> 	open_count = dev->open_count;
> 	mutex_unlock(&drm_global_mutex);

Random style nit: READ_ONCE, no locking needed (the locks don't change
anything, except if you have really strange locking rules). Serves
double-duty as a huge warning sign that something tricky is happening.

> 	return open_count;
> }
> 
> static int driver_remove()
> {
> 	drm_dev_unregister();
> 
> 	open_count = drm_dev_open_count();
> 
> 	/* Open fd's, device is going away, block access */
> 	if (open_count)
> 		drm_dev_set_unplugged();
> 
> 	drm_atomic_helper_shutdown();
> 
> 	/* No open fd's, driver is going away */
> 	if (!open_count)
> 		drm_dev_set_unplugged();
> }
> 
> 
> Personally I have 2 use cases:
> - tinydrm SPI drivers
>   The only hotpluggable SPI controllers I know of are USB which should
>   handle device access while unregistering.
>   SPI devices can be removed, but the controller driver is still in
>   place so it's safe.
> - A future USB driver (that I hope to work on when all this core stuff
>   is in place).
>   There's no point in touching the hw, so DRM can be set uplugged right
>   away in the disconnect() callback.
> 
> This means that I don't need to know who's going away for my use cases.
> 
> This turned out to be quite long winding, but the bottom line is that
> having a separate function to set the unplugged state seems to give a
> lot of flexibility for various use cases.
> 
> I hope I didn't muddy the waters even more :-)

Hm, I think I get your idea. Since I'm still not sure what the best option
is I'm leaning towards just leaving stuff as-is. I.e. drivers which want
to shut down hw can do the 1. drm_dev_unregister() 2.
drm_atomic_helper_shutdown() sequence. Drivers which care about hotunplug
more can do just the drm_dev_unplug().

Yes it's messy and unsatisfactory, but in case of serious doubt I like to
wait until maybe in the future we have a good idea. Meanwhile leaving a
bit of a mess around is better than charging ahead in a possibly totally
wrong direction.

There's cases where clue still hasn't hit me, even years later (or anyone
else). That's just how it is sometimes.

Zooming out more looking at the big picture I'd say all your work in the
past few years has enormously simplified drm for simple drivers already.
If we can't resolve this one here right now that just means you "only"
made drm 98% simpler instead of maybe 99%. It's still an epic win :-)

Cheers, Daniel


> Noralf.
> 
> > Thanks, Daniel
> > 
> >>
> >> Noralf.
> >>
> >>> Cheers, Daniel
> >>>
> >>>> Noralf.
> >>>>
> >>>>>> + * in order to disable the hardware on regular driver module unload.
> >>>>>>   */
> >>>>>>  void drm_dev_unregister(struct drm_device *dev)
> >>>>>>  {
> >>>>>> @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
> >>>>>>  	if (drm_core_check_feature(dev, DRIVER_LEGACY))
> >>>>>>  		drm_lastclose(dev);
> >>>>>>  
> >>>>>> +	/*
> >>>>>> +	 * After synchronizing any critical read section is guaranteed to see
> >>>>>> +	 * the new value of ->unplugged, and any critical section which might
> >>>>>> +	 * still have seen the old value of ->unplugged is guaranteed to have
> >>>>>> +	 * finished.
> >>>>>> +	 */
> >>>>>> +	dev->unplugged = true;
> >>>>>> +	synchronize_srcu(&drm_unplug_srcu);
> >>>>>> +
> >>>>>>  	dev->registered = false;
> >>>>>>  
> >>>>>>  	drm_client_dev_unregister(dev);
> >>>>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> >>>>>> index ca46a45a9cce..c50696c82a42 100644
> >>>>>> --- a/include/drm/drm_drv.h
> >>>>>> +++ b/include/drm/drm_drv.h
> >>>>>> @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
> >>>>>>   * drm_dev_is_unplugged - is a DRM device unplugged
> >>>>>>   * @dev: DRM device
> >>>>>>   *
> >>>>>> - * This function can be called to check whether a hotpluggable is unplugged.
> >>>>>> - * Unplugging itself is singalled through drm_dev_unplug(). If a device is
> >>>>>> - * unplugged, these two functions guarantee that any store before calling
> >>>>>> - * drm_dev_unplug() is visible to callers of this function after it completes
> >>>>>> + * This function can be called to check whether @dev is unregistered. This can
> >>>>>> + * be used to detect that the underlying parent device is gone.
> >>>>>
> >>>>> I think it'd be good to keep the first part, and just update the reference
> >>>>> to drm_dev_unregister. So:
> >>>>>
> >>>>>  * This function can be called to check whether a hotpluggable is unplugged.
> >>>>>  * Unplugging itself is singalled through drm_dev_unregister(). If a device is
> >>>>>  * unplugged, these two functions guarantee that any store before calling
> >>>>>  * drm_dev_unregister() is visible to callers of this function after it
> >>>>>  * completes.
> >>>>>
> >>>>> I think your version shrugs a few important details under the rug. With
> >>>>> those nits addressed:
> >>>>>
> >>>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>>>
> >>>>> Cheers, Daniel
> >>>>>
> >>>>>>   *
> >>>>>> - * WARNING: This function fundamentally races against drm_dev_unplug(). It is
> >>>>>> - * recommended that drivers instead use the underlying drm_dev_enter() and
> >>>>>> + * WARNING: This function fundamentally races against drm_dev_unregister(). It
> >>>>>> + * is recommended that drivers instead use the underlying drm_dev_enter() and
> >>>>>>   * drm_dev_exit() function pairs.
> >>>>>>   */
> >>>>>>  static inline bool drm_dev_is_unplugged(struct drm_device *dev)
> >>>>>> -- 
> >>>>>> 2.20.1
> >>>>>>
> >>>>>> _______________________________________________
> >>>>>> Intel-gfx mailing list
> >>>>>> Intel-gfx@lists.freedesktop.org
> >>>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>>>>
> >>>
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()

[Noralf Trønnes]

Den 06.02.2019 16.26, skrev Daniel Vetter:
> On Tue, Feb 05, 2019 at 06:57:50PM +0100, Noralf Trønnes wrote:
>>
>>
>> Den 05.02.2019 17.31, skrev Daniel Vetter:
>>> On Tue, Feb 05, 2019 at 11:20:55AM +0100, Noralf Trønnes wrote:
>>>>
>>>>
>>>> Den 05.02.2019 10.11, skrev Daniel Vetter:
>>>>> On Mon, Feb 04, 2019 at 06:35:28PM +0100, Noralf Trønnes wrote:
>>>>>>
>>>>>>
>>>>>> Den 04.02.2019 16.41, skrev Daniel Vetter:
>>>>>>> On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
>>>>>>>> The only thing now that makes drm_dev_unplug() special is that it sets
>>>>>>>> drm_device->unplugged. Move this code to drm_dev_unregister() so that we
>>>>>>>> can remove drm_dev_unplug().
>>>>>>>>
>>>>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>>>>>> ---
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>>  drivers/gpu/drm/drm_drv.c | 27 +++++++++++++++------------
>>>>>>>>  include/drm/drm_drv.h     | 10 ++++------
>>>>>>>>  2 files changed, 19 insertions(+), 18 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>>>>>>> index 05bbc2b622fc..e0941200edc6 100644
>>>>>>>> --- a/drivers/gpu/drm/drm_drv.c
>>>>>>>> +++ b/drivers/gpu/drm/drm_drv.c
>>>>>>>> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
>>>>>>>>   */
>>>>>>>>  void drm_dev_unplug(struct drm_device *dev)
>>>>>>>>  {
>>>>>>>> -	/*
>>>>>>>> -	 * After synchronizing any critical read section is guaranteed to see
>>>>>>>> -	 * the new value of ->unplugged, and any critical section which might
>>>>>>>> -	 * still have seen the old value of ->unplugged is guaranteed to have
>>>>>>>> -	 * finished.
>>>>>>>> -	 */
>>>>>>>> -	dev->unplugged = true;
>>>>>>>> -	synchronize_srcu(&drm_unplug_srcu);
>>>>>>>> -
>>>>>>>>  	drm_dev_unregister(dev);
>>>>>>>>  	drm_dev_put(dev);
>>>>>>>>  }
>>>>>>>> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
>>>>>>>>   * drm_dev_register() but does not deallocate the device. The caller must call
>>>>>>>>   * drm_dev_put() to drop their final reference.
>>>>>>>>   *
>>>>>>>> - * A special form of unregistering for hotpluggable devices is drm_dev_unplug(),
>>>>>>>> - * which can be called while there are still open users of @dev.
>>>>>>>> + * This function can be called while there are still open users of @dev as long
>>>>>>>> + * as the driver protects its device resources using drm_dev_enter() and
>>>>>>>> + * drm_dev_exit().
>>>>>>>>   *
>>>>>>>>   * This should be called first in the device teardown code to make sure
>>>>>>>> - * userspace can't access the device instance any more.
>>>>>>>> + * userspace can't access the device instance any more. Drivers that support
>>>>>>>> + * device unplug will probably want to call drm_atomic_helper_shutdown() first
>>>>>>>
>>>>>>> Read once more with a bit more coffee, spotted this:
>>>>>>>
>>>>>>> s/first/afterwards/ - shutting down the hw before we've taken it away from
>>>>>>> userspace is kinda the wrong way round. It should be the inverse of driver
>>>>>>> load, which is 1) allocate structures 2) prep hw 3) register driver with
>>>>>>> the world (simplified ofc).
>>>>>>>
>>>>>>
>>>>>> The problem is that drm_dev_unregister() sets the device as unplugged
>>>>>> and if drm_atomic_helper_shutdown() is called afterwards it's not
>>>>>> allowed to touch hardware.
>>>>>>
>>>>>> I know it's the wrong order, but the only way to do it in the right
>>>>>> order is to have a separate function that sets unplugged:
>>>>>>
>>>>>> 	drm_dev_unregister();
>>>>>> 	drm_atomic_helper_shutdown();
>>>>>> 	drm_dev_set_unplugged();
>>>>>
>>>>> Annoying ... but yeah calling _shutdown() before we stopped userspace is
>>>>> also not going to work. Because userspace could quickly re-enable
>>>>> something, and then the refcounts would be all wrong again and leaking
>>>>> objects.
>>>>>
>>>>
>>>> What happens with a USB device that is unplugged with open userspace,
>>>> will that leak objects?
>>>
>>> Maybe we've jumped to conclusions. drm_atomic_helper_shutdown() will run
>>> as normal, the only thing that should be skipped is actually touching the
>>> hw (as long as the driver doesn't protect too much with
>>> drm_dev_enter/exit). So all the software updates (including refcounting
>>> updates) will still be done. Ofc current udl is not yet atomic, so in
>>> reality something else happens.
>>>
>>> And we ofc still have the same issue that if you just unload the driver,
>>> then the hw will stay on (which might really confuse the driver on next
>>> load, when it assumes that it only gets loaded from cold boot where
>>> everything is off - which usually is the case on an arm soc at least).
>>>
>>>>> I get a bit the feeling we're over-optimizing here with trying to devm-ize
>>>>> drm_dev_register. Just getting drm_device correctly devm-ized is a big
>>>>> step forward already, and will open up a lot of TODO items across a lot of
>>>>> drivers. E.g. we could add a drm_dev_kzalloc, for allocating all the drm_*
>>>>> structs, which gets released together with drm_device. I think that's a
>>>>> much clearer path forward, I think we all agree that getting the kfree out
>>>>> of the driver codes is a good thing, and it would allow us to do this
>>>>> correctly.
>>>>>
>>>>> Then once we have that and rolled out to a few drivers we can reconsider
>>>>> the entire unregister/shutdown gordian knot here. Atm I just have no idea
>>>>> how to do this properly :-/
>>>>>
>>>>> Thoughts, other ideas?
>>>>>
>>>>
>>>> Yeah, I've come to the conclusion that devm_drm_dev_register() doesn't
>>>> make much sense if we need a driver remove callback anyways.
>>>
>>> Yup, that's what I meant with the above:
>>> - merge devm_drm_dev_register, use it a lot. This is definitely a good
>>>   idea.
>>> - create a drm_dev_kzalloc, which automatically releases memory on the
>>>   final drm_dev_put. Use it every in drivers for drm structures,
>>>   especially in those drivers that currently use devm (which releases
>>>   those allocations potentialy too early on unplug).
>>> - figure out the next steps after that
>>>
>>>> I think devm_drm_dev_init() makes sense because it yields a cleaner
>>>> probe() function. An additional benefit is that it requires a
>>>> drm_driver->release function which is a step in the right direction to
>>>> get the drm_device lifetime right.
>>>>
>>>> Do we agree that a drm_dev_set_unplugged() function is necessary to get
>>>> the remove/disconnect order right?
>>>>
>>>> What about drm_dev_unplug() maybe I should just leave it be?
>>>>
>>>> - amd uses drm_driver->unload, so that one takes some work to get right
>>>>   to support unplug. It doesn't check the unplugged state, so really
>>>>   doesn't need drm_dev_unplug() I guess. Do they have cards that can be
>>>>   hotplugged?
>>>
>>> Yeah amd still uses ->load and ->unload, which is not great unfortunately.
>>> I just stumbled over that when I tried to make a series to disable the
>>> global drm_global_mutex for most drivers ...
>>>
>>>> - udl uses drm_driver->unload, doesn't use drm_atomic_helper_shutdown().
>>>>   It has only one drm_dev_is_unplugged() check and that is in its
>>>>   fbdev->open callback.
>>>
>>> udl isn't atomic, so can't use the atomic helpers. pre-atomic doesn't have
>>> refcounting issues when something is left on iirc. I think udl is written
>>> under the assumption that ->unload is always called for an unplug, never
>>> for an actual unload.
>>>
>>>> - xen calls drm_atomic_helper_shutdown() in its drm_driver->release
>>>>   callback which I guess is not correct.
>>>
>>> Yeah this smells fishy. ->release is supposed to be for cleaning up kernel
>>> structures, not for cleaning up the hw. So maybe drm_mode_config_cleanup
>>> could be put there, not sure honestly. But definitely not _shutdown().
>>>
>>>> This is what it will look like with a set unplugged function:
>>>>
>>>> void drm_dev_unplug(struct drm_device *dev)
>>>> {
>>>> 	drm_dev_set_unplugged(dev);
>>>> 	drm_dev_unregister(dev);
>>>> 	drm_dev_put(dev);
>>>> }
>>>>
>>>> Hm, I should probably remove it to avoid further use of it since it's
>>>> not correct for a modern driver.
>>>
>>> I think something flew over my head ... what's the drm_dev_set_unplugged
>>> for? I think I'm not following you ...
>>>
>>
>> Taking it a few steps back:
>>
>> This patch proposes to move 'dev->unplugged = true' from
>> drm_dev_unplug() to drm_dev_unregister().
>>
>> Additionally I proposed this change to the drm_dev_unregister() docs:
>>
>>   * This should be called first in the device teardown code to make sure
>> - * userspace can't access the device instance any more.
>> + * userspace can't access the device instance any more. Drivers that
>> support
>> + * device unplug will probably want to call
>> drm_atomic_helper_shutdown() first
>> + * in order to disable the hardware on regular driver module unload.
>>
>> Which would give a driver remove callback like this:
>>
>> static int driver_remove()
>> {
>> 	drm_atomic_helper_shutdown();
>> 	drm_dev_unregister();
>> }
>>
>> Your reaction was that drm_atomic_helper_shutdown() needs to be called
>> after drm_dev_unregister() to avoid a race resulting in leaked objects.
>> However if we call it afterwards, ->unplugged will be true and the
>> driver can't touch hardware.
>>
>> Then I proposed moving the unplugged state setting to:
>>
>> void drm_dev_set_unplugged(struct drm_device *dev)
>> {
>> 	dev->unplugged = true;
>> 	synchronize_srcu(&drm_unplug_srcu);
>> }
>>
>> Now it is possible to avoid the race and still touch hardware:
>>
>> static int driver_remove()
>> {
>> 	drm_dev_unregister();
>> 	drm_atomic_helper_shutdown();
>> 	drm_dev_set_unplugged();
>> }
>>
>> But now I'm back to the question: Is it the driver or the device that is
>> going away?
>>
>> If it's the driver we are fine touching hardware, if it's the device it
>> depends on how we access the device resource and whether the subsystem
>> has protection in place to handle access after the device is gone. I
>> think USB can handle and block device access up until the disconnect
>> callback has finished (no point in doing so though, since the normal
>> operation is that the device is gone, not the driver unloading).
>>
>> Is there a way to determine who's going away without changes to the
>> device core?
>>
>> Maybe. The driver can only be unregistered if there are no open file
>> handles because a ref is taken on the driver module.
> 
> This isn't true. You can (not many people do, but it's possible) to unbind
> through sysfs. The module reference only keeps the cpu instructions valid,
> nothing else.
> 
>> So maybe something along these lines:
>>
>> int drm_dev_open_count(struct drm_device *dev)
>> {
>> 	int open_count;
>>
>> 	mutex_lock(&drm_global_mutex);
>> 	open_count = dev->open_count;
>> 	mutex_unlock(&drm_global_mutex);
> 
> Random style nit: READ_ONCE, no locking needed (the locks don't change
> anything, except if you have really strange locking rules). Serves
> double-duty as a huge warning sign that something tricky is happening.
> 
>> 	return open_count;
>> }
>>
>> static int driver_remove()
>> {
>> 	drm_dev_unregister();
>>
>> 	open_count = drm_dev_open_count();
>>
>> 	/* Open fd's, device is going away, block access */
>> 	if (open_count)
>> 		drm_dev_set_unplugged();
>>
>> 	drm_atomic_helper_shutdown();
>>
>> 	/* No open fd's, driver is going away */
>> 	if (!open_count)
>> 		drm_dev_set_unplugged();
>> }
>>
>>
>> Personally I have 2 use cases:
>> - tinydrm SPI drivers
>>   The only hotpluggable SPI controllers I know of are USB which should
>>   handle device access while unregistering.
>>   SPI devices can be removed, but the controller driver is still in
>>   place so it's safe.
>> - A future USB driver (that I hope to work on when all this core stuff
>>   is in place).
>>   There's no point in touching the hw, so DRM can be set uplugged right
>>   away in the disconnect() callback.
>>
>> This means that I don't need to know who's going away for my use cases.
>>
>> This turned out to be quite long winding, but the bottom line is that
>> having a separate function to set the unplugged state seems to give a
>> lot of flexibility for various use cases.
>>
>> I hope I didn't muddy the waters even more :-)
> 
> Hm, I think I get your idea. Since I'm still not sure what the best option
> is I'm leaning towards just leaving stuff as-is. I.e. drivers which want
> to shut down hw can do the 1. drm_dev_unregister() 2.
> drm_atomic_helper_shutdown() sequence. Drivers which care about hotunplug
> more can do just the drm_dev_unplug().
> 
> Yes it's messy and unsatisfactory, but in case of serious doubt I like to
> wait until maybe in the future we have a good idea. Meanwhile leaving a
> bit of a mess around is better than charging ahead in a possibly totally
> wrong direction.
> 
> There's cases where clue still hasn't hit me, even years later (or anyone
> else). That's just how it is sometimes.
> 

Ok, I'll drop this.

This means that I'll drop devm_drm_dev_init() as well since it doesn't
play well with drm_dev_unplug() since both will do drm_dev_put(). Not a
big deal really, it just means that I need to add one error path in the
probe function so I can drm_dev_put() on error.

Are you still ok with the first bug fix patch in this series?

> Zooming out more looking at the big picture I'd say all your work in the
> past few years has enormously simplified drm for simple drivers already.
> If we can't resolve this one here right now that just means you "only"
> made drm 98% simpler instead of maybe 99%. It's still an epic win :-)
> 

Thanks, your mentoring and reviews helped turn my rough ideas into
something useful :-)

Noralf.

> Cheers, Daniel
> 
> 
>> Noralf.
>>
>>> Thanks, Daniel
>>>
>>>>
>>>> Noralf.
>>>>
>>>>> Cheers, Daniel
>>>>>
>>>>>> Noralf.
>>>>>>
>>>>>>>> + * in order to disable the hardware on regular driver module unload.
>>>>>>>>   */
>>>>>>>>  void drm_dev_unregister(struct drm_device *dev)
>>>>>>>>  {
>>>>>>>> @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
>>>>>>>>  	if (drm_core_check_feature(dev, DRIVER_LEGACY))
>>>>>>>>  		drm_lastclose(dev);
>>>>>>>>  
>>>>>>>> +	/*
>>>>>>>> +	 * After synchronizing any critical read section is guaranteed to see
>>>>>>>> +	 * the new value of ->unplugged, and any critical section which might
>>>>>>>> +	 * still have seen the old value of ->unplugged is guaranteed to have
>>>>>>>> +	 * finished.
>>>>>>>> +	 */
>>>>>>>> +	dev->unplugged = true;
>>>>>>>> +	synchronize_srcu(&drm_unplug_srcu);
>>>>>>>> +
>>>>>>>>  	dev->registered = false;
>>>>>>>>  
>>>>>>>>  	drm_client_dev_unregister(dev);
>>>>>>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>>>>>>>> index ca46a45a9cce..c50696c82a42 100644
>>>>>>>> --- a/include/drm/drm_drv.h
>>>>>>>> +++ b/include/drm/drm_drv.h
>>>>>>>> @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
>>>>>>>>   * drm_dev_is_unplugged - is a DRM device unplugged
>>>>>>>>   * @dev: DRM device
>>>>>>>>   *
>>>>>>>> - * This function can be called to check whether a hotpluggable is unplugged.
>>>>>>>> - * Unplugging itself is singalled through drm_dev_unplug(). If a device is
>>>>>>>> - * unplugged, these two functions guarantee that any store before calling
>>>>>>>> - * drm_dev_unplug() is visible to callers of this function after it completes
>>>>>>>> + * This function can be called to check whether @dev is unregistered. This can
>>>>>>>> + * be used to detect that the underlying parent device is gone.
>>>>>>>
>>>>>>> I think it'd be good to keep the first part, and just update the reference
>>>>>>> to drm_dev_unregister. So:
>>>>>>>
>>>>>>>  * This function can be called to check whether a hotpluggable is unplugged.
>>>>>>>  * Unplugging itself is singalled through drm_dev_unregister(). If a device is
>>>>>>>  * unplugged, these two functions guarantee that any store before calling
>>>>>>>  * drm_dev_unregister() is visible to callers of this function after it
>>>>>>>  * completes.
>>>>>>>
>>>>>>> I think your version shrugs a few important details under the rug. With
>>>>>>> those nits addressed:
>>>>>>>
>>>>>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>>>>
>>>>>>> Cheers, Daniel
>>>>>>>
>>>>>>>>   *
>>>>>>>> - * WARNING: This function fundamentally races against drm_dev_unplug(). It is
>>>>>>>> - * recommended that drivers instead use the underlying drm_dev_enter() and
>>>>>>>> + * WARNING: This function fundamentally races against drm_dev_unregister(). It
>>>>>>>> + * is recommended that drivers instead use the underlying drm_dev_enter() and
>>>>>>>>   * drm_dev_exit() function pairs.
>>>>>>>>   */
>>>>>>>>  static inline bool drm_dev_is_unplugged(struct drm_device *dev)
>>>>>>>> -- 
>>>>>>>> 2.20.1
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> Intel-gfx mailing list
>>>>>>>> Intel-gfx@lists.freedesktop.org
>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>>>>>
>>>>>
>>>
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 559 bytes --]

Daniel Vetter <daniel-/w4YWyX8dFk@public.gmane.org> writes:
>
> Zooming out more looking at the big picture I'd say all your work in the
> past few years has enormously simplified drm for simple drivers already.
> If we can't resolve this one here right now that just means you "only"
> made drm 98% simpler instead of maybe 99%. It's still an epic win :-)

I'd like to second this.  So many of Noralf's cleanups I think "oof,
that's a lot of work for a little cleanup here".  But we've benefited
immensely from it accumulating over the years.  Thanks again!

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()

[Daniel Vetter]

On Wed, Feb 06, 2019 at 05:46:51PM +0100, Noralf Trønnes wrote:
> 
> 
> Den 06.02.2019 16.26, skrev Daniel Vetter:
> > On Tue, Feb 05, 2019 at 06:57:50PM +0100, Noralf Trønnes wrote:
> >>
> >>
> >> Den 05.02.2019 17.31, skrev Daniel Vetter:
> >>> On Tue, Feb 05, 2019 at 11:20:55AM +0100, Noralf Trønnes wrote:
> >>>>
> >>>>
> >>>> Den 05.02.2019 10.11, skrev Daniel Vetter:
> >>>>> On Mon, Feb 04, 2019 at 06:35:28PM +0100, Noralf Trønnes wrote:
> >>>>>>
> >>>>>>
> >>>>>> Den 04.02.2019 16.41, skrev Daniel Vetter:
> >>>>>>> On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
> >>>>>>>> The only thing now that makes drm_dev_unplug() special is that it sets
> >>>>>>>> drm_device->unplugged. Move this code to drm_dev_unregister() so that we
> >>>>>>>> can remove drm_dev_unplug().
> >>>>>>>>
> >>>>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >>>>>>>> ---
> >>>>>>
> >>>>>> [...]
> >>>>>>
> >>>>>>>>  drivers/gpu/drm/drm_drv.c | 27 +++++++++++++++------------
> >>>>>>>>  include/drm/drm_drv.h     | 10 ++++------
> >>>>>>>>  2 files changed, 19 insertions(+), 18 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >>>>>>>> index 05bbc2b622fc..e0941200edc6 100644
> >>>>>>>> --- a/drivers/gpu/drm/drm_drv.c
> >>>>>>>> +++ b/drivers/gpu/drm/drm_drv.c
> >>>>>>>> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
> >>>>>>>>   */
> >>>>>>>>  void drm_dev_unplug(struct drm_device *dev)
> >>>>>>>>  {
> >>>>>>>> -	/*
> >>>>>>>> -	 * After synchronizing any critical read section is guaranteed to see
> >>>>>>>> -	 * the new value of ->unplugged, and any critical section which might
> >>>>>>>> -	 * still have seen the old value of ->unplugged is guaranteed to have
> >>>>>>>> -	 * finished.
> >>>>>>>> -	 */
> >>>>>>>> -	dev->unplugged = true;
> >>>>>>>> -	synchronize_srcu(&drm_unplug_srcu);
> >>>>>>>> -
> >>>>>>>>  	drm_dev_unregister(dev);
> >>>>>>>>  	drm_dev_put(dev);
> >>>>>>>>  }
> >>>>>>>> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
> >>>>>>>>   * drm_dev_register() but does not deallocate the device. The caller must call
> >>>>>>>>   * drm_dev_put() to drop their final reference.
> >>>>>>>>   *
> >>>>>>>> - * A special form of unregistering for hotpluggable devices is drm_dev_unplug(),
> >>>>>>>> - * which can be called while there are still open users of @dev.
> >>>>>>>> + * This function can be called while there are still open users of @dev as long
> >>>>>>>> + * as the driver protects its device resources using drm_dev_enter() and
> >>>>>>>> + * drm_dev_exit().
> >>>>>>>>   *
> >>>>>>>>   * This should be called first in the device teardown code to make sure
> >>>>>>>> - * userspace can't access the device instance any more.
> >>>>>>>> + * userspace can't access the device instance any more. Drivers that support
> >>>>>>>> + * device unplug will probably want to call drm_atomic_helper_shutdown() first
> >>>>>>>
> >>>>>>> Read once more with a bit more coffee, spotted this:
> >>>>>>>
> >>>>>>> s/first/afterwards/ - shutting down the hw before we've taken it away from
> >>>>>>> userspace is kinda the wrong way round. It should be the inverse of driver
> >>>>>>> load, which is 1) allocate structures 2) prep hw 3) register driver with
> >>>>>>> the world (simplified ofc).
> >>>>>>>
> >>>>>>
> >>>>>> The problem is that drm_dev_unregister() sets the device as unplugged
> >>>>>> and if drm_atomic_helper_shutdown() is called afterwards it's not
> >>>>>> allowed to touch hardware.
> >>>>>>
> >>>>>> I know it's the wrong order, but the only way to do it in the right
> >>>>>> order is to have a separate function that sets unplugged:
> >>>>>>
> >>>>>> 	drm_dev_unregister();
> >>>>>> 	drm_atomic_helper_shutdown();
> >>>>>> 	drm_dev_set_unplugged();
> >>>>>
> >>>>> Annoying ... but yeah calling _shutdown() before we stopped userspace is
> >>>>> also not going to work. Because userspace could quickly re-enable
> >>>>> something, and then the refcounts would be all wrong again and leaking
> >>>>> objects.
> >>>>>
> >>>>
> >>>> What happens with a USB device that is unplugged with open userspace,
> >>>> will that leak objects?
> >>>
> >>> Maybe we've jumped to conclusions. drm_atomic_helper_shutdown() will run
> >>> as normal, the only thing that should be skipped is actually touching the
> >>> hw (as long as the driver doesn't protect too much with
> >>> drm_dev_enter/exit). So all the software updates (including refcounting
> >>> updates) will still be done. Ofc current udl is not yet atomic, so in
> >>> reality something else happens.
> >>>
> >>> And we ofc still have the same issue that if you just unload the driver,
> >>> then the hw will stay on (which might really confuse the driver on next
> >>> load, when it assumes that it only gets loaded from cold boot where
> >>> everything is off - which usually is the case on an arm soc at least).
> >>>
> >>>>> I get a bit the feeling we're over-optimizing here with trying to devm-ize
> >>>>> drm_dev_register. Just getting drm_device correctly devm-ized is a big
> >>>>> step forward already, and will open up a lot of TODO items across a lot of
> >>>>> drivers. E.g. we could add a drm_dev_kzalloc, for allocating all the drm_*
> >>>>> structs, which gets released together with drm_device. I think that's a
> >>>>> much clearer path forward, I think we all agree that getting the kfree out
> >>>>> of the driver codes is a good thing, and it would allow us to do this
> >>>>> correctly.
> >>>>>
> >>>>> Then once we have that and rolled out to a few drivers we can reconsider
> >>>>> the entire unregister/shutdown gordian knot here. Atm I just have no idea
> >>>>> how to do this properly :-/
> >>>>>
> >>>>> Thoughts, other ideas?
> >>>>>
> >>>>
> >>>> Yeah, I've come to the conclusion that devm_drm_dev_register() doesn't
> >>>> make much sense if we need a driver remove callback anyways.
> >>>
> >>> Yup, that's what I meant with the above:
> >>> - merge devm_drm_dev_register, use it a lot. This is definitely a good
> >>>   idea.
> >>> - create a drm_dev_kzalloc, which automatically releases memory on the
> >>>   final drm_dev_put. Use it every in drivers for drm structures,
> >>>   especially in those drivers that currently use devm (which releases
> >>>   those allocations potentialy too early on unplug).
> >>> - figure out the next steps after that
> >>>
> >>>> I think devm_drm_dev_init() makes sense because it yields a cleaner
> >>>> probe() function. An additional benefit is that it requires a
> >>>> drm_driver->release function which is a step in the right direction to
> >>>> get the drm_device lifetime right.
> >>>>
> >>>> Do we agree that a drm_dev_set_unplugged() function is necessary to get
> >>>> the remove/disconnect order right?
> >>>>
> >>>> What about drm_dev_unplug() maybe I should just leave it be?
> >>>>
> >>>> - amd uses drm_driver->unload, so that one takes some work to get right
> >>>>   to support unplug. It doesn't check the unplugged state, so really
> >>>>   doesn't need drm_dev_unplug() I guess. Do they have cards that can be
> >>>>   hotplugged?
> >>>
> >>> Yeah amd still uses ->load and ->unload, which is not great unfortunately.
> >>> I just stumbled over that when I tried to make a series to disable the
> >>> global drm_global_mutex for most drivers ...
> >>>
> >>>> - udl uses drm_driver->unload, doesn't use drm_atomic_helper_shutdown().
> >>>>   It has only one drm_dev_is_unplugged() check and that is in its
> >>>>   fbdev->open callback.
> >>>
> >>> udl isn't atomic, so can't use the atomic helpers. pre-atomic doesn't have
> >>> refcounting issues when something is left on iirc. I think udl is written
> >>> under the assumption that ->unload is always called for an unplug, never
> >>> for an actual unload.
> >>>
> >>>> - xen calls drm_atomic_helper_shutdown() in its drm_driver->release
> >>>>   callback which I guess is not correct.
> >>>
> >>> Yeah this smells fishy. ->release is supposed to be for cleaning up kernel
> >>> structures, not for cleaning up the hw. So maybe drm_mode_config_cleanup
> >>> could be put there, not sure honestly. But definitely not _shutdown().
> >>>
> >>>> This is what it will look like with a set unplugged function:
> >>>>
> >>>> void drm_dev_unplug(struct drm_device *dev)
> >>>> {
> >>>> 	drm_dev_set_unplugged(dev);
> >>>> 	drm_dev_unregister(dev);
> >>>> 	drm_dev_put(dev);
> >>>> }
> >>>>
> >>>> Hm, I should probably remove it to avoid further use of it since it's
> >>>> not correct for a modern driver.
> >>>
> >>> I think something flew over my head ... what's the drm_dev_set_unplugged
> >>> for? I think I'm not following you ...
> >>>
> >>
> >> Taking it a few steps back:
> >>
> >> This patch proposes to move 'dev->unplugged = true' from
> >> drm_dev_unplug() to drm_dev_unregister().
> >>
> >> Additionally I proposed this change to the drm_dev_unregister() docs:
> >>
> >>   * This should be called first in the device teardown code to make sure
> >> - * userspace can't access the device instance any more.
> >> + * userspace can't access the device instance any more. Drivers that
> >> support
> >> + * device unplug will probably want to call
> >> drm_atomic_helper_shutdown() first
> >> + * in order to disable the hardware on regular driver module unload.
> >>
> >> Which would give a driver remove callback like this:
> >>
> >> static int driver_remove()
> >> {
> >> 	drm_atomic_helper_shutdown();
> >> 	drm_dev_unregister();
> >> }
> >>
> >> Your reaction was that drm_atomic_helper_shutdown() needs to be called
> >> after drm_dev_unregister() to avoid a race resulting in leaked objects.
> >> However if we call it afterwards, ->unplugged will be true and the
> >> driver can't touch hardware.
> >>
> >> Then I proposed moving the unplugged state setting to:
> >>
> >> void drm_dev_set_unplugged(struct drm_device *dev)
> >> {
> >> 	dev->unplugged = true;
> >> 	synchronize_srcu(&drm_unplug_srcu);
> >> }
> >>
> >> Now it is possible to avoid the race and still touch hardware:
> >>
> >> static int driver_remove()
> >> {
> >> 	drm_dev_unregister();
> >> 	drm_atomic_helper_shutdown();
> >> 	drm_dev_set_unplugged();
> >> }
> >>
> >> But now I'm back to the question: Is it the driver or the device that is
> >> going away?
> >>
> >> If it's the driver we are fine touching hardware, if it's the device it
> >> depends on how we access the device resource and whether the subsystem
> >> has protection in place to handle access after the device is gone. I
> >> think USB can handle and block device access up until the disconnect
> >> callback has finished (no point in doing so though, since the normal
> >> operation is that the device is gone, not the driver unloading).
> >>
> >> Is there a way to determine who's going away without changes to the
> >> device core?
> >>
> >> Maybe. The driver can only be unregistered if there are no open file
> >> handles because a ref is taken on the driver module.
> > 
> > This isn't true. You can (not many people do, but it's possible) to unbind
> > through sysfs. The module reference only keeps the cpu instructions valid,
> > nothing else.
> > 
> >> So maybe something along these lines:
> >>
> >> int drm_dev_open_count(struct drm_device *dev)
> >> {
> >> 	int open_count;
> >>
> >> 	mutex_lock(&drm_global_mutex);
> >> 	open_count = dev->open_count;
> >> 	mutex_unlock(&drm_global_mutex);
> > 
> > Random style nit: READ_ONCE, no locking needed (the locks don't change
> > anything, except if you have really strange locking rules). Serves
> > double-duty as a huge warning sign that something tricky is happening.
> > 
> >> 	return open_count;
> >> }
> >>
> >> static int driver_remove()
> >> {
> >> 	drm_dev_unregister();
> >>
> >> 	open_count = drm_dev_open_count();
> >>
> >> 	/* Open fd's, device is going away, block access */
> >> 	if (open_count)
> >> 		drm_dev_set_unplugged();
> >>
> >> 	drm_atomic_helper_shutdown();
> >>
> >> 	/* No open fd's, driver is going away */
> >> 	if (!open_count)
> >> 		drm_dev_set_unplugged();
> >> }
> >>
> >>
> >> Personally I have 2 use cases:
> >> - tinydrm SPI drivers
> >>   The only hotpluggable SPI controllers I know of are USB which should
> >>   handle device access while unregistering.
> >>   SPI devices can be removed, but the controller driver is still in
> >>   place so it's safe.
> >> - A future USB driver (that I hope to work on when all this core stuff
> >>   is in place).
> >>   There's no point in touching the hw, so DRM can be set uplugged right
> >>   away in the disconnect() callback.
> >>
> >> This means that I don't need to know who's going away for my use cases.
> >>
> >> This turned out to be quite long winding, but the bottom line is that
> >> having a separate function to set the unplugged state seems to give a
> >> lot of flexibility for various use cases.
> >>
> >> I hope I didn't muddy the waters even more :-)
> > 
> > Hm, I think I get your idea. Since I'm still not sure what the best option
> > is I'm leaning towards just leaving stuff as-is. I.e. drivers which want
> > to shut down hw can do the 1. drm_dev_unregister() 2.
> > drm_atomic_helper_shutdown() sequence. Drivers which care about hotunplug
> > more can do just the drm_dev_unplug().
> > 
> > Yes it's messy and unsatisfactory, but in case of serious doubt I like to
> > wait until maybe in the future we have a good idea. Meanwhile leaving a
> > bit of a mess around is better than charging ahead in a possibly totally
> > wrong direction.
> > 
> > There's cases where clue still hasn't hit me, even years later (or anyone
> > else). That's just how it is sometimes.
> > 
> 
> Ok, I'll drop this.
> 
> This means that I'll drop devm_drm_dev_init() as well since it doesn't
> play well with drm_dev_unplug() since both will do drm_dev_put(). Not a
> big deal really, it just means that I need to add one error path in the
> probe function so I can drm_dev_put() on error.

Hm, I think we could just move the drm_dev_put out from drm_dev_unplug.
And then encourage everyone to use devm_drm_dev_init() everywhere. I do
like to get started with auto-cleaned up in drm somehow, and
devm_drm_dev_init doing the drm_dev_put() looks like a really good idea.

> Are you still ok with the first bug fix patch in this series?

Yeah that one still looks good.

Cheers, Daniel

> 
> > Zooming out more looking at the big picture I'd say all your work in the
> > past few years has enormously simplified drm for simple drivers already.
> > If we can't resolve this one here right now that just means you "only"
> > made drm 98% simpler instead of maybe 99%. It's still an epic win :-)
> > 
> 
> Thanks, your mentoring and reviews helped turn my rough ideas into
> something useful :-)
> 
> Noralf.
> 
> > Cheers, Daniel
> > 
> > 
> >> Noralf.
> >>
> >>> Thanks, Daniel
> >>>
> >>>>
> >>>> Noralf.
> >>>>
> >>>>> Cheers, Daniel
> >>>>>
> >>>>>> Noralf.
> >>>>>>
> >>>>>>>> + * in order to disable the hardware on regular driver module unload.
> >>>>>>>>   */
> >>>>>>>>  void drm_dev_unregister(struct drm_device *dev)
> >>>>>>>>  {
> >>>>>>>> @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
> >>>>>>>>  	if (drm_core_check_feature(dev, DRIVER_LEGACY))
> >>>>>>>>  		drm_lastclose(dev);
> >>>>>>>>  
> >>>>>>>> +	/*
> >>>>>>>> +	 * After synchronizing any critical read section is guaranteed to see
> >>>>>>>> +	 * the new value of ->unplugged, and any critical section which might
> >>>>>>>> +	 * still have seen the old value of ->unplugged is guaranteed to have
> >>>>>>>> +	 * finished.
> >>>>>>>> +	 */
> >>>>>>>> +	dev->unplugged = true;
> >>>>>>>> +	synchronize_srcu(&drm_unplug_srcu);
> >>>>>>>> +
> >>>>>>>>  	dev->registered = false;
> >>>>>>>>  
> >>>>>>>>  	drm_client_dev_unregister(dev);
> >>>>>>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> >>>>>>>> index ca46a45a9cce..c50696c82a42 100644
> >>>>>>>> --- a/include/drm/drm_drv.h
> >>>>>>>> +++ b/include/drm/drm_drv.h
> >>>>>>>> @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
> >>>>>>>>   * drm_dev_is_unplugged - is a DRM device unplugged
> >>>>>>>>   * @dev: DRM device
> >>>>>>>>   *
> >>>>>>>> - * This function can be called to check whether a hotpluggable is unplugged.
> >>>>>>>> - * Unplugging itself is singalled through drm_dev_unplug(). If a device is
> >>>>>>>> - * unplugged, these two functions guarantee that any store before calling
> >>>>>>>> - * drm_dev_unplug() is visible to callers of this function after it completes
> >>>>>>>> + * This function can be called to check whether @dev is unregistered. This can
> >>>>>>>> + * be used to detect that the underlying parent device is gone.
> >>>>>>>
> >>>>>>> I think it'd be good to keep the first part, and just update the reference
> >>>>>>> to drm_dev_unregister. So:
> >>>>>>>
> >>>>>>>  * This function can be called to check whether a hotpluggable is unplugged.
> >>>>>>>  * Unplugging itself is singalled through drm_dev_unregister(). If a device is
> >>>>>>>  * unplugged, these two functions guarantee that any store before calling
> >>>>>>>  * drm_dev_unregister() is visible to callers of this function after it
> >>>>>>>  * completes.
> >>>>>>>
> >>>>>>> I think your version shrugs a few important details under the rug. With
> >>>>>>> those nits addressed:
> >>>>>>>
> >>>>>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>>>>>
> >>>>>>> Cheers, Daniel
> >>>>>>>
> >>>>>>>>   *
> >>>>>>>> - * WARNING: This function fundamentally races against drm_dev_unplug(). It is
> >>>>>>>> - * recommended that drivers instead use the underlying drm_dev_enter() and
> >>>>>>>> + * WARNING: This function fundamentally races against drm_dev_unregister(). It
> >>>>>>>> + * is recommended that drivers instead use the underlying drm_dev_enter() and
> >>>>>>>>   * drm_dev_exit() function pairs.
> >>>>>>>>   */
> >>>>>>>>  static inline bool drm_dev_is_unplugged(struct drm_device *dev)
> >>>>>>>> -- 
> >>>>>>>> 2.20.1
> >>>>>>>>
> >>>>>>>> _______________________________________________
> >>>>>>>> Intel-gfx mailing list
> >>>>>>>> Intel-gfx@lists.freedesktop.org
> >>>>>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>>>>>>
> >>>>>
> >>>
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm: Fix drm_release() and device unplug

[Sean Paul]

On Sun, Feb 03, 2019 at 04:41:55PM +0100, Noralf Trønnes wrote:
> If userspace has open fd(s) when drm_dev_unplug() is run, it will result
> in drm_dev_unregister() being called twice. First in drm_dev_unplug() and
> then later in drm_release() through the call to drm_put_dev().
> 
> Since userspace already holds a ref on drm_device through the drm_minor,
> it's not necessary to add extra ref counting based on no open file
> handles. Instead just drm_dev_put() unconditionally in drm_dev_unplug().
> 
> We now has this:
> - Userpace holds a ref on drm_device as long as there's open fd(s)
> - The driver holds a ref on drm_device as long as it's bound to the
>   struct device
> 
> When both sides are done with drm_device, it is released.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

Reviewed-by: Sean Paul <sean@poorly.run>

> ---
>  drivers/gpu/drm/drm_drv.c  | 6 +-----
>  drivers/gpu/drm/drm_file.c | 6 ++----
>  2 files changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 381581b01d48..05bbc2b622fc 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -376,11 +376,7 @@ void drm_dev_unplug(struct drm_device *dev)
>  	synchronize_srcu(&drm_unplug_srcu);
>  
>  	drm_dev_unregister(dev);
> -
> -	mutex_lock(&drm_global_mutex);
> -	if (dev->open_count == 0)
> -		drm_dev_put(dev);
> -	mutex_unlock(&drm_global_mutex);
> +	drm_dev_put(dev);
>  }
>  EXPORT_SYMBOL(drm_dev_unplug);
>  
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 46f48f245eb5..3f20f598cd7c 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -479,11 +479,9 @@ int drm_release(struct inode *inode, struct file *filp)
>  
>  	drm_file_free(file_priv);
>  
> -	if (!--dev->open_count) {
> +	if (!--dev->open_count)
>  		drm_lastclose(dev);
> -		if (drm_dev_is_unplugged(dev))
> -			drm_put_dev(dev);
> -	}
> +
>  	mutex_unlock(&drm_global_mutex);
>  
>  	drm_minor_release(minor);
> -- 
> 2.20.1
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()

[Sean Paul]

On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
> The only thing now that makes drm_dev_unplug() special is that it sets
> drm_device->unplugged. Move this code to drm_dev_unregister() so that we
> can remove drm_dev_unplug().
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
> 
> Maybe s/unplugged/unregistered/ ?
> 
> I looked at drm_device->registered, but using that would mean that
> drm_dev_is_unplugged() would return before drm_device is registered.
> And given that its current purpose is to prevent race against connector
> registration, I stayed away from it.

Makes sense. I think having unregistered along with registered might cause some
confusion unless it's really clearly documented. Perhaps
s/unplugged/unregistered/ and s/registered/initialized/ but init has its own
meaning...

I'd hate to hold up the patchset over naming bikeshed, so maybe this would be
better off as a follow-on series where everyone can pile on opinions. So,

Reviewed-by: Sean Paul <sean@poorly.run>


> 
> Noralf.
> 
> 
>  drivers/gpu/drm/drm_drv.c | 27 +++++++++++++++------------
>  include/drm/drm_drv.h     | 10 ++++------
>  2 files changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 05bbc2b622fc..e0941200edc6 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
>   */
>  void drm_dev_unplug(struct drm_device *dev)
>  {
> -	/*
> -	 * After synchronizing any critical read section is guaranteed to see
> -	 * the new value of ->unplugged, and any critical section which might
> -	 * still have seen the old value of ->unplugged is guaranteed to have
> -	 * finished.
> -	 */
> -	dev->unplugged = true;
> -	synchronize_srcu(&drm_unplug_srcu);
> -
>  	drm_dev_unregister(dev);
>  	drm_dev_put(dev);
>  }
> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
>   * drm_dev_register() but does not deallocate the device. The caller must call
>   * drm_dev_put() to drop their final reference.
>   *
> - * A special form of unregistering for hotpluggable devices is drm_dev_unplug(),
> - * which can be called while there are still open users of @dev.
> + * This function can be called while there are still open users of @dev as long
> + * as the driver protects its device resources using drm_dev_enter() and
> + * drm_dev_exit().
>   *
>   * This should be called first in the device teardown code to make sure
> - * userspace can't access the device instance any more.
> + * userspace can't access the device instance any more. Drivers that support
> + * device unplug will probably want to call drm_atomic_helper_shutdown() first
> + * in order to disable the hardware on regular driver module unload.
>   */
>  void drm_dev_unregister(struct drm_device *dev)
>  {
> @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
>  	if (drm_core_check_feature(dev, DRIVER_LEGACY))
>  		drm_lastclose(dev);
>  
> +	/*
> +	 * After synchronizing any critical read section is guaranteed to see
> +	 * the new value of ->unplugged, and any critical section which might
> +	 * still have seen the old value of ->unplugged is guaranteed to have
> +	 * finished.
> +	 */
> +	dev->unplugged = true;
> +	synchronize_srcu(&drm_unplug_srcu);
> +
>  	dev->registered = false;
>  
>  	drm_client_dev_unregister(dev);
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index ca46a45a9cce..c50696c82a42 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
>   * drm_dev_is_unplugged - is a DRM device unplugged
>   * @dev: DRM device
>   *
> - * This function can be called to check whether a hotpluggable is unplugged.
> - * Unplugging itself is singalled through drm_dev_unplug(). If a device is
> - * unplugged, these two functions guarantee that any store before calling
> - * drm_dev_unplug() is visible to callers of this function after it completes
> + * This function can be called to check whether @dev is unregistered. This can
> + * be used to detect that the underlying parent device is gone.
>   *
> - * WARNING: This function fundamentally races against drm_dev_unplug(). It is
> - * recommended that drivers instead use the underlying drm_dev_enter() and
> + * WARNING: This function fundamentally races against drm_dev_unregister(). It
> + * is recommended that drivers instead use the underlying drm_dev_enter() and
>   * drm_dev_exit() function pairs.
>   */
>  static inline bool drm_dev_is_unplugged(struct drm_device *dev)
> -- 
> 2.20.1
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/6] drm/udl: Use drm_dev_unregister()

[Sean Paul]

On Sun, Feb 03, 2019 at 04:41:58PM +0100, Noralf Trønnes wrote:
> drm_dev_unplug() has been stripped down and is going away. Open code its
> 2 remaining function calls.
> 
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Sean Paul <sean@poorly.run>

Reviewed-by: Sean Paul <sean@poorly.run>

> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  drivers/gpu/drm/udl/udl_drv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> index 22cd2d13e272..063e8e1e2641 100644
> --- a/drivers/gpu/drm/udl/udl_drv.c
> +++ b/drivers/gpu/drm/udl/udl_drv.c
> @@ -106,7 +106,8 @@ static void udl_usb_disconnect(struct usb_interface *interface)
>  	drm_kms_helper_poll_disable(dev);
>  	udl_fbdev_unplug(dev);
>  	udl_drop_usb(dev);
> -	drm_dev_unplug(dev);
> +	drm_dev_unregister(dev);
> +	drm_dev_put(dev);
>  }
>  
>  /*
> -- 
> 2.20.1
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/drv: Remove drm_dev_unplug()

[Sean Paul]

On Sun, Feb 03, 2019 at 04:42:00PM +0100, Noralf Trønnes wrote:
> There are no users left.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

Reviewed-by: Sean Paul <sean@poorly.run>

> ---
>  drivers/gpu/drm/drm_drv.c | 17 -----------------
>  include/drm/drm_drv.h     |  1 -
>  2 files changed, 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index e0941200edc6..87210d4a9e53 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -354,23 +354,6 @@ void drm_dev_exit(int idx)
>  }
>  EXPORT_SYMBOL(drm_dev_exit);
>  
> -/**
> - * drm_dev_unplug - unplug a DRM device
> - * @dev: DRM device
> - *
> - * This unplugs a hotpluggable DRM device, which makes it inaccessible to
> - * userspace operations. Entry-points can use drm_dev_enter() and
> - * drm_dev_exit() to protect device resources in a race free manner. This
> - * essentially unregisters the device like drm_dev_unregister(), but can be
> - * called while there are still open users of @dev.
> - */
> -void drm_dev_unplug(struct drm_device *dev)
> -{
> -	drm_dev_unregister(dev);
> -	drm_dev_put(dev);
> -}
> -EXPORT_SYMBOL(drm_dev_unplug);
> -
>  /*
>   * DRM internal mount
>   * We want to be able to allocate our own "struct address_space" to control
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index c50696c82a42..b8765a6fc092 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -730,7 +730,6 @@ void drm_dev_put(struct drm_device *dev);
>  void drm_put_dev(struct drm_device *dev);
>  bool drm_dev_enter(struct drm_device *dev, int *idx);
>  void drm_dev_exit(int idx);
> -void drm_dev_unplug(struct drm_device *dev);
>  
>  /**
>   * drm_dev_is_unplugged - is a DRM device unplugged
> -- 
> 2.20.1
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()

[Sean Paul]

On Thu, Feb 07, 2019 at 04:07:33PM -0500, Sean Paul wrote:
> On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
> > The only thing now that makes drm_dev_unplug() special is that it sets
> > drm_device->unplugged. Move this code to drm_dev_unregister() so that we
> > can remove drm_dev_unplug().
> > 
> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > ---
> > 
> > Maybe s/unplugged/unregistered/ ?
> > 
> > I looked at drm_device->registered, but using that would mean that
> > drm_dev_is_unplugged() would return before drm_device is registered.
> > And given that its current purpose is to prevent race against connector
> > registration, I stayed away from it.
> 
> Makes sense. I think having unregistered along with registered might cause some
> confusion unless it's really clearly documented. Perhaps
> s/unplugged/unregistered/ and s/registered/initialized/ but init has its own
> meaning...
> 
> I'd hate to hold up the patchset over naming bikeshed, so maybe this would be
> better off as a follow-on series where everyone can pile on opinions. So,
> 
> Reviewed-by: Sean Paul <sean@poorly.run>

Looks like I was dropped from Cc, so I didn't see the replies after Oleksandr's
review. So go ahead and disregard this :-)

Sean

> 
> 
> > 
> > Noralf.
> > 
> > 
> >  drivers/gpu/drm/drm_drv.c | 27 +++++++++++++++------------
> >  include/drm/drm_drv.h     | 10 ++++------
> >  2 files changed, 19 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 05bbc2b622fc..e0941200edc6 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
> >   */
> >  void drm_dev_unplug(struct drm_device *dev)
> >  {
> > -	/*
> > -	 * After synchronizing any critical read section is guaranteed to see
> > -	 * the new value of ->unplugged, and any critical section which might
> > -	 * still have seen the old value of ->unplugged is guaranteed to have
> > -	 * finished.
> > -	 */
> > -	dev->unplugged = true;
> > -	synchronize_srcu(&drm_unplug_srcu);
> > -
> >  	drm_dev_unregister(dev);
> >  	drm_dev_put(dev);
> >  }
> > @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
> >   * drm_dev_register() but does not deallocate the device. The caller must call
> >   * drm_dev_put() to drop their final reference.
> >   *
> > - * A special form of unregistering for hotpluggable devices is drm_dev_unplug(),
> > - * which can be called while there are still open users of @dev.
> > + * This function can be called while there are still open users of @dev as long
> > + * as the driver protects its device resources using drm_dev_enter() and
> > + * drm_dev_exit().
> >   *
> >   * This should be called first in the device teardown code to make sure
> > - * userspace can't access the device instance any more.
> > + * userspace can't access the device instance any more. Drivers that support
> > + * device unplug will probably want to call drm_atomic_helper_shutdown() first
> > + * in order to disable the hardware on regular driver module unload.
> >   */
> >  void drm_dev_unregister(struct drm_device *dev)
> >  {
> > @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
> >  	if (drm_core_check_feature(dev, DRIVER_LEGACY))
> >  		drm_lastclose(dev);
> >  
> > +	/*
> > +	 * After synchronizing any critical read section is guaranteed to see
> > +	 * the new value of ->unplugged, and any critical section which might
> > +	 * still have seen the old value of ->unplugged is guaranteed to have
> > +	 * finished.
> > +	 */
> > +	dev->unplugged = true;
> > +	synchronize_srcu(&drm_unplug_srcu);
> > +
> >  	dev->registered = false;
> >  
> >  	drm_client_dev_unregister(dev);
> > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > index ca46a45a9cce..c50696c82a42 100644
> > --- a/include/drm/drm_drv.h
> > +++ b/include/drm/drm_drv.h
> > @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
> >   * drm_dev_is_unplugged - is a DRM device unplugged
> >   * @dev: DRM device
> >   *
> > - * This function can be called to check whether a hotpluggable is unplugged.
> > - * Unplugging itself is singalled through drm_dev_unplug(). If a device is
> > - * unplugged, these two functions guarantee that any store before calling
> > - * drm_dev_unplug() is visible to callers of this function after it completes
> > + * This function can be called to check whether @dev is unregistered. This can
> > + * be used to detect that the underlying parent device is gone.
> >   *
> > - * WARNING: This function fundamentally races against drm_dev_unplug(). It is
> > - * recommended that drivers instead use the underlying drm_dev_enter() and
> > + * WARNING: This function fundamentally races against drm_dev_unregister(). It
> > + * is recommended that drivers instead use the underlying drm_dev_enter() and
> >   * drm_dev_exit() function pairs.
> >   */
> >  static inline bool drm_dev_is_unplugged(struct drm_device *dev)
> > -- 
> > 2.20.1
> > 
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx