[PATCH] drm/kms: Catch mode_object lifetime errors

[PATCH] drm/kms: Catch mode_object lifetime errors

[Daniel Vetter]

Only dynamic mode objects, i.e. those which are refcounted and have a free
callback, can be added while the overall drm_device is visible to
userspace. All others must be added before drm_dev_register and
removed after drm_dev_unregister.

Small issue around drivers still using the load/unload callbacks, we
need to make sure we set dev->registered so that load/unload code in
these callbacks doesn't trigger false warnings. Only a small
adjustement in drm_dev_register was needed.

Motivated by some irc discussions about object ids of dynamic objects
like blobs become invalid, and me going on a bit an audit spree.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_drv.c         | 4 ++--
 drivers/gpu/drm/drm_mode_object.c | 4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index cb6f0245de7c..48c84e3e1931 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -997,14 +997,14 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
 	if (ret)
 		goto err_minors;
 
-	dev->registered = true;
-
 	if (dev->driver->load) {
 		ret = dev->driver->load(dev, flags);
 		if (ret)
 			goto err_minors;
 	}
 
+	dev->registered = true;
+
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
 		drm_modeset_register_all(dev);
 
diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
index 1c6e51135962..c355ba8e6d5d 100644
--- a/drivers/gpu/drm/drm_mode_object.c
+++ b/drivers/gpu/drm/drm_mode_object.c
@@ -42,6 +42,8 @@ int __drm_mode_object_add(struct drm_device *dev, struct drm_mode_object *obj,
 {
 	int ret;
 
+	WARN_ON(dev->registered && !obj_free_cb);
+
 	mutex_lock(&dev->mode_config.idr_mutex);
 	ret = idr_alloc(&dev->mode_config.object_idr, register_obj ? obj : NULL,
 			1, 0, GFP_KERNEL);
@@ -102,6 +104,8 @@ void drm_mode_object_register(struct drm_device *dev,
 void drm_mode_object_unregister(struct drm_device *dev,
 				struct drm_mode_object *object)
 {
+	WARN_ON(dev->registered && !object->free_cb);
+
 	mutex_lock(&dev->mode_config.idr_mutex);
 	if (object->id) {
 		idr_remove(&dev->mode_config.object_idr, object->id);
-- 
2.20.1

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

✗ Fi.CI.CHECKPATCH: warning for drm/kms: Catch mode_object lifetime errors

[Patchwork]

== Series Details ==

Series: drm/kms: Catch mode_object lifetime errors
URL   : https://patchwork.freedesktop.org/series/62083/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
4800ebbe4df3 drm/kms: Catch mode_object lifetime errors
-:63: WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author 'Daniel Vetter <daniel.vetter@ffwll.ch>'

total: 0 errors, 1 warnings, 0 checks, 32 lines checked

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

✓ Fi.CI.BAT: success for drm/kms: Catch mode_object lifetime errors

[Patchwork]

== Series Details ==

Series: drm/kms: Catch mode_object lifetime errors
URL   : https://patchwork.freedesktop.org/series/62083/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6265 -> Patchwork_13282
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_create@basic-files:
    - fi-icl-u3:          [PASS][1] -> [INCOMPLETE][2] ([fdo#107713] / [fdo#109100])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/fi-icl-u3/igt@gem_ctx_create@basic-files.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/fi-icl-u3/igt@gem_ctx_create@basic-files.html

  * igt@gem_exec_basic@basic-all:
    - fi-icl-y:           [PASS][3] -> [INCOMPLETE][4] ([fdo#107713])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/fi-icl-y/igt@gem_exec_basic@basic-all.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/fi-icl-y/igt@gem_exec_basic@basic-all.html

  
#### Possible fixes ####

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [FAIL][5] ([fdo#109485]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485


Participating hosts (53 -> 45)
------------------------------

  Additional (1): fi-byt-j1900 
  Missing    (9): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-byt-clapper fi-kbl-8809g fi-icl-dsi fi-bdw-samus 


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

  * Linux: CI_DRM_6265 -> Patchwork_13282

  CI_DRM_6265: 657b9f601946cab518d8911ea92dc0f437a1f4b4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5055: 495287320225e7f180d384cad7b207b77154438f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13282: 4800ebbe4df3d318d95be75cf230f31f7026afc3 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

4800ebbe4df3 drm/kms: Catch mode_object lifetime errors

== Logs ==

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

✓ Fi.CI.IGT: success for drm/kms: Catch mode_object lifetime errors

[Patchwork]

== Series Details ==

Series: drm/kms: Catch mode_object lifetime errors
URL   : https://patchwork.freedesktop.org/series/62083/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6265_full -> Patchwork_13282_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_eio@in-flight-internal-immediate:
    - shard-apl:          [PASS][1] -> [DMESG-WARN][2] ([fdo#110913 ])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-apl7/igt@gem_eio@in-flight-internal-immediate.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-apl2/igt@gem_eio@in-flight-internal-immediate.html
    - shard-glk:          [PASS][3] -> [DMESG-WARN][4] ([fdo#110913 ])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-glk9/igt@gem_eio@in-flight-internal-immediate.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-glk2/igt@gem_eio@in-flight-internal-immediate.html

  * igt@gem_eio@wait-wedge-1us:
    - shard-iclb:         [PASS][5] -> [DMESG-WARN][6] ([fdo#110913 ])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-iclb1/igt@gem_eio@wait-wedge-1us.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-iclb1/igt@gem_eio@wait-wedge-1us.html

  * igt@gem_persistent_relocs@forked-interruptible-thrashing:
    - shard-skl:          ([PASS][7], [PASS][8]) -> [DMESG-WARN][9] ([fdo#110913 ]) +1 similar issue
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-skl8/igt@gem_persistent_relocs@forked-interruptible-thrashing.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-skl7/igt@gem_persistent_relocs@forked-interruptible-thrashing.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-skl3/igt@gem_persistent_relocs@forked-interruptible-thrashing.html
    - shard-hsw:          [PASS][10] -> [DMESG-WARN][11] ([fdo#110789] / [fdo#110913 ])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-hsw5/igt@gem_persistent_relocs@forked-interruptible-thrashing.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-hsw4/igt@gem_persistent_relocs@forked-interruptible-thrashing.html

  * igt@gem_persistent_relocs@forked-thrashing:
    - shard-hsw:          ([PASS][12], [PASS][13]) -> [DMESG-WARN][14] ([fdo#110789] / [fdo#110913 ]) +1 similar issue
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-hsw6/igt@gem_persistent_relocs@forked-thrashing.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-hsw4/igt@gem_persistent_relocs@forked-thrashing.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-hsw7/igt@gem_persistent_relocs@forked-thrashing.html

  * igt@gem_pipe_control_store_loop@fresh-buffer:
    - shard-apl:          ([PASS][15], [PASS][16]) -> [INCOMPLETE][17] ([fdo#103927])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-apl1/igt@gem_pipe_control_store_loop@fresh-buffer.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-apl4/igt@gem_pipe_control_store_loop@fresh-buffer.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-apl8/igt@gem_pipe_control_store_loop@fresh-buffer.html

  * igt@gem_tiled_swapping@non-threaded:
    - shard-iclb:         [PASS][18] -> [FAIL][19] ([fdo#108686])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-iclb2/igt@gem_tiled_swapping@non-threaded.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-iclb1/igt@gem_tiled_swapping@non-threaded.html

  * igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy:
    - shard-apl:          ([PASS][20], [PASS][21]) -> [DMESG-WARN][22] ([fdo#110913 ])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-apl3/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-apl1/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-apl3/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy.html

  * igt@i915_pm_rpm@fences-dpms:
    - shard-iclb:         [PASS][23] -> [INCOMPLETE][24] ([fdo#107713] / [fdo#108840])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-iclb5/igt@i915_pm_rpm@fences-dpms.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-iclb2/igt@i915_pm_rpm@fences-dpms.html

  * igt@i915_selftest@mock_requests:
    - shard-skl:          ([PASS][25], [PASS][26]) -> [INCOMPLETE][27] ([fdo#110550])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-skl5/igt@i915_selftest@mock_requests.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-skl7/igt@i915_selftest@mock_requests.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-skl10/igt@i915_selftest@mock_requests.html

  * igt@kms_frontbuffer_tracking@fbc-2p-pri-indfb-multidraw:
    - shard-hsw:          [PASS][28] -> [SKIP][29] ([fdo#109271]) +3 similar issues
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-hsw5/igt@kms_frontbuffer_tracking@fbc-2p-pri-indfb-multidraw.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-hsw1/igt@kms_frontbuffer_tracking@fbc-2p-pri-indfb-multidraw.html

  * igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-cur-indfb-move:
    - shard-hsw:          ([PASS][30], [PASS][31]) -> [SKIP][32] ([fdo#109271]) +12 similar issues
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-hsw4/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-cur-indfb-move.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-hsw8/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-cur-indfb-move.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-hsw1/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-cur-indfb-move.html

  * igt@kms_frontbuffer_tracking@fbc-suspend:
    - shard-kbl:          ([PASS][33], [PASS][34]) -> [DMESG-WARN][35] ([fdo#108566]) +2 similar issues
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-kbl3/igt@kms_frontbuffer_tracking@fbc-suspend.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-kbl6/igt@kms_frontbuffer_tracking@fbc-suspend.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-kbl7/igt@kms_frontbuffer_tracking@fbc-suspend.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-mmap-gtt:
    - shard-iclb:         [PASS][36] -> [FAIL][37] ([fdo#103167]) +4 similar issues
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-iclb2/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-mmap-gtt.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-iclb2/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-mmap-gtt.html

  * igt@kms_plane@plane-panning-bottom-right-pipe-b-planes:
    - shard-skl:          ([PASS][38], [PASS][39]) -> [FAIL][40] ([fdo#103166])
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-skl5/igt@kms_plane@plane-panning-bottom-right-pipe-b-planes.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-skl4/igt@kms_plane@plane-panning-bottom-right-pipe-b-planes.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-skl1/igt@kms_plane@plane-panning-bottom-right-pipe-b-planes.html

  * igt@kms_psr@psr2_cursor_blt:
    - shard-iclb:         [PASS][41] -> [SKIP][42] ([fdo#109441]) +2 similar issues
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-iclb2/igt@kms_psr@psr2_cursor_blt.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-iclb5/igt@kms_psr@psr2_cursor_blt.html

  * igt@perf@blocking:
    - shard-skl:          ([PASS][43], [PASS][44]) -> [FAIL][45] ([fdo#110728])
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-skl7/igt@perf@blocking.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-skl3/igt@perf@blocking.html
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-skl2/igt@perf@blocking.html

  
#### Possible fixes ####

  * igt@gem_eio@in-flight-10ms:
    - shard-skl:          ([PASS][46], [DMESG-WARN][47]) ([fdo#110913 ]) -> [PASS][48] +3 similar issues
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-skl9/igt@gem_eio@in-flight-10ms.html
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-skl7/igt@gem_eio@in-flight-10ms.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-skl8/igt@gem_eio@in-flight-10ms.html

  * igt@gem_eio@reset-stress:
    - shard-snb:          ([PASS][49], [FAIL][50]) ([fdo#109661]) -> [PASS][51]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-snb4/igt@gem_eio@reset-stress.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-snb7/igt@gem_eio@reset-stress.html
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-snb1/igt@gem_eio@reset-stress.html

  * igt@gem_exec_balancer@smoke:
    - shard-iclb:         [SKIP][52] ([fdo#110854]) -> [PASS][53]
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-iclb3/igt@gem_exec_balancer@smoke.html
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-iclb4/igt@gem_exec_balancer@smoke.html

  * igt@gem_persistent_relocs@forked-faulting-reloc-thrash-inactive:
    - shard-apl:          ([INCOMPLETE][54], [PASS][55]) ([fdo#103927]) -> [PASS][56]
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-apl3/igt@gem_persistent_relocs@forked-faulting-reloc-thrash-inactive.html
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-apl6/igt@gem_persistent_relocs@forked-faulting-reloc-thrash-inactive.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-apl3/igt@gem_persistent_relocs@forked-faulting-reloc-thrash-inactive.html

  * igt@gem_persistent_relocs@forked-thrashing:
    - shard-glk:          ([PASS][57], [DMESG-WARN][58]) ([fdo#110913 ]) -> [PASS][59] +3 similar issues
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-glk2/igt@gem_persistent_relocs@forked-thrashing.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-glk8/igt@gem_persistent_relocs@forked-thrashing.html
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-glk1/igt@gem_persistent_relocs@forked-thrashing.html
    - shard-skl:          ([DMESG-WARN][60], [DMESG-WARN][61]) ([fdo#110913 ]) -> [PASS][62]
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-skl5/igt@gem_persistent_relocs@forked-thrashing.html
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-skl8/igt@gem_persistent_relocs@forked-thrashing.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-skl3/igt@gem_persistent_relocs@forked-thrashing.html
    - shard-kbl:          ([DMESG-WARN][63], [PASS][64]) ([fdo#110913 ]) -> [PASS][65] +1 similar issue
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-kbl6/igt@gem_persistent_relocs@forked-thrashing.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-kbl2/igt@gem_persistent_relocs@forked-thrashing.html
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-kbl2/igt@gem_persistent_relocs@forked-thrashing.html
    - shard-snb:          ([PASS][66], [DMESG-WARN][67]) ([fdo#110789] / [fdo#110913 ]) -> [PASS][68]
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-snb4/igt@gem_persistent_relocs@forked-thrashing.html
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-snb5/igt@gem_persistent_relocs@forked-thrashing.html
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-snb1/igt@gem_persistent_relocs@forked-thrashing.html

  * igt@gem_ppgtt@blt-vs-render-ctx0:
    - shard-apl:          ([PASS][69], [DMESG-WARN][70]) ([fdo#110913 ]) -> [PASS][71] +2 similar issues
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-apl4/igt@gem_ppgtt@blt-vs-render-ctx0.html
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-apl5/igt@gem_ppgtt@blt-vs-render-ctx0.html
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-apl4/igt@gem_ppgtt@blt-vs-render-ctx0.html

  * igt@gem_tiled_swapping@non-threaded:
    - shard-apl:          ([DMESG-WARN][72], [PASS][73]) ([fdo#108686]) -> [PASS][74]
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-apl3/igt@gem_tiled_swapping@non-threaded.html
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-apl1/igt@gem_tiled_swapping@non-threaded.html
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-apl4/igt@gem_tiled_swapping@non-threaded.html
    - shard-hsw:          ([FAIL][75], [INCOMPLETE][76]) ([fdo#103540] / [fdo#108686]) -> [PASS][77]
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-hsw8/igt@gem_tiled_swapping@non-threaded.html
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-hsw2/igt@gem_tiled_swapping@non-threaded.html
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-hsw8/igt@gem_tiled_swapping@non-threaded.html

  * igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy-gup:
    - shard-snb:          ([DMESG-WARN][78], [PASS][79]) ([fdo#110913 ]) -> [PASS][80]
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-snb1/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy-gup.html
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-snb5/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy-gup.html
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-snb5/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy-gup.html

  * igt@gem_userptr_blits@sync-unmap-cycles:
    - shard-hsw:          ([DMESG-WARN][81], [PASS][82]) ([fdo#110913 ]) -> [PASS][83] +1 similar issue
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-hsw8/igt@gem_userptr_blits@sync-unmap-cycles.html
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-hsw7/igt@gem_userptr_blits@sync-unmap-cycles.html
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-hsw2/igt@gem_userptr_blits@sync-unmap-cycles.html
    - shard-apl:          ([DMESG-WARN][84], [DMESG-WARN][85]) ([fdo#110913 ]) -> [PASS][86]
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-apl4/igt@gem_userptr_blits@sync-unmap-cycles.html
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-apl2/igt@gem_userptr_blits@sync-unmap-cycles.html
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-apl2/igt@gem_userptr_blits@sync-unmap-cycles.html

  * igt@kms_big_fb@x-tiled-32bpp-rotate-180:
    - shard-snb:          ([PASS][87], [SKIP][88]) ([fdo#109271]) -> [PASS][89]
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-snb7/igt@kms_big_fb@x-tiled-32bpp-rotate-180.html
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-snb4/igt@kms_big_fb@x-tiled-32bpp-rotate-180.html
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-snb7/igt@kms_big_fb@x-tiled-32bpp-rotate-180.html

  * igt@kms_cursor_crc@pipe-a-cursor-suspend:
    - shard-kbl:          ([PASS][90], [DMESG-WARN][91]) ([fdo#108566]) -> [PASS][92] +2 similar issues
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-kbl2/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
   [91]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-kbl7/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
   [92]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-kbl1/igt@kms_cursor_crc@pipe-a-cursor-suspend.html

  * igt@kms_cursor_legacy@cursor-vs-flip-varying-size:
    - shard-hsw:          ([FAIL][93], [PASS][94]) ([fdo#103355]) -> [PASS][95]
   [93]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-hsw6/igt@kms_cursor_legacy@cursor-vs-flip-varying-size.html
   [94]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-hsw8/igt@kms_cursor_legacy@cursor-vs-flip-varying-size.html
   [95]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-hsw2/igt@kms_cursor_legacy@cursor-vs-flip-varying-size.html

  * igt@kms_flip@2x-flip-vs-suspend:
    - shard-hsw:          [SKIP][96] ([fdo#109271]) -> [PASS][97] +8 similar issues
   [96]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-hsw1/igt@kms_flip@2x-flip-vs-suspend.html
   [97]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-hsw8/igt@kms_flip@2x-flip-vs-suspend.html

  * igt@kms_flip@2x-plain-flip:
    - shard-hsw:          ([PASS][98], [SKIP][99]) ([fdo#109271]) -> [PASS][100] +33 similar issues
   [98]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-hsw7/igt@kms_flip@2x-plain-flip.html
   [99]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-hsw1/igt@kms_flip@2x-plain-flip.html
   [100]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-hsw5/igt@kms_flip@2x-plain-flip.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-msflip-blt:
    - shard-iclb:         [FAIL][101] ([fdo#103167]) -> [PASS][102] +9 similar issues
   [101]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-iclb6/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-msflip-blt.html
   [102]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-iclb1/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-msflip-blt.html

  * igt@kms_frontbuffer_tracking@fbc-suspend:
    - shard-skl:          ([INCOMPLETE][103], [PASS][104]) ([fdo#104108]) -> [PASS][105]
   [103]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-skl8/igt@kms_frontbuffer_tracking@fbc-suspend.html
   [104]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-skl2/igt@kms_frontbuffer_tracking@fbc-suspend.html
   [105]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-skl5/igt@kms_frontbuffer_tracking@fbc-suspend.html

  * igt@kms_psr@psr2_sprite_plane_onoff:
    - shard-iclb:         [SKIP][106] ([fdo#109441]) -> [PASS][107]
   [106]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-iclb5/igt@kms_psr@psr2_sprite_plane_onoff.html
   [107]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-iclb2/igt@kms_psr@psr2_sprite_plane_onoff.html

  * igt@kms_setmode@basic:
    - shard-apl:          ([FAIL][108], [FAIL][109]) ([fdo#99912]) -> [PASS][110]
   [108]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-apl4/igt@kms_setmode@basic.html
   [109]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-apl2/igt@kms_setmode@basic.html
   [110]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-apl7/igt@kms_setmode@basic.html

  * igt@perf_pmu@rc6-runtime-pm:
    - shard-hsw:          ([FAIL][111], [FAIL][112]) ([fdo#105010]) -> [PASS][113]
   [111]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-hsw2/igt@perf_pmu@rc6-runtime-pm.html
   [112]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-hsw8/igt@perf_pmu@rc6-runtime-pm.html
   [113]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-hsw8/igt@perf_pmu@rc6-runtime-pm.html

  * igt@prime_busy@wait-hang-default:
    - shard-hsw:          ([DMESG-WARN][114], [DMESG-WARN][115]) ([fdo#110789] / [fdo#110913 ]) -> [PASS][116]
   [114]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-hsw8/igt@prime_busy@wait-hang-default.html
   [115]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6265/shard-hsw6/igt@prime_busy@wait-hang-default.html
   [116]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13282/shard-hsw1/igt@prime_busy@wait-hang-default.html

  
#### Warnings ####

  * igt@gem_eio@in-flight-suspend:
    - shard-kbl:          ([INCOMPLETE][117], [PASS][118]) ([fdo#103

== Logs ==

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

Re: [PATCH] drm/kms: Catch mode_object lifetime errors

[Sean Paul]

On Fri, Jun 14, 2019 at 08:17:23AM +0200, Daniel Vetter wrote:
> Only dynamic mode objects, i.e. those which are refcounted and have a free
> callback, can be added while the overall drm_device is visible to
> userspace. All others must be added before drm_dev_register and
> removed after drm_dev_unregister.
> 
> Small issue around drivers still using the load/unload callbacks, we
> need to make sure we set dev->registered so that load/unload code in
> these callbacks doesn't trigger false warnings. Only a small
> adjustement in drm_dev_register was needed.
> 
> Motivated by some irc discussions about object ids of dynamic objects
> like blobs become invalid, and me going on a bit an audit spree.
> 

Seems like a very worthwhile change, any idea how many drivers are going
to be sad after this change?

> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_drv.c         | 4 ++--
>  drivers/gpu/drm/drm_mode_object.c | 4 ++++
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index cb6f0245de7c..48c84e3e1931 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -997,14 +997,14 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
>  	if (ret)
>  		goto err_minors;
>  
> -	dev->registered = true;
> -
>  	if (dev->driver->load) {
>  		ret = dev->driver->load(dev, flags);
>  		if (ret)
>  			goto err_minors;
>  	}
>  
> +	dev->registered = true;
> +
>  	if (drm_core_check_feature(dev, DRIVER_MODESET))
>  		drm_modeset_register_all(dev);
>  
> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> index 1c6e51135962..c355ba8e6d5d 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -42,6 +42,8 @@ int __drm_mode_object_add(struct drm_device *dev, struct drm_mode_object *obj,
>  {
>  	int ret;
>  
> +	WARN_ON(dev->registered && !obj_free_cb);

These should probably have a comment above them giving some guidance to the
driver developer.

With some comments, this is:

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


> +
>  	mutex_lock(&dev->mode_config.idr_mutex);
>  	ret = idr_alloc(&dev->mode_config.object_idr, register_obj ? obj : NULL,
>  			1, 0, GFP_KERNEL);
> @@ -102,6 +104,8 @@ void drm_mode_object_register(struct drm_device *dev,
>  void drm_mode_object_unregister(struct drm_device *dev,
>  				struct drm_mode_object *object)
>  {
> +	WARN_ON(dev->registered && !object->free_cb);
> +
>  	mutex_lock(&dev->mode_config.idr_mutex);
>  	if (object->id) {
>  		idr_remove(&dev->mode_config.object_idr, object->id);
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH] drm/kms: Catch mode_object lifetime errors

[Daniel Vetter]

On Fri, Jun 28, 2019 at 7:24 PM Sean Paul <sean@poorly.run> wrote:
>
> On Fri, Jun 14, 2019 at 08:17:23AM +0200, Daniel Vetter wrote:
> > Only dynamic mode objects, i.e. those which are refcounted and have a free
> > callback, can be added while the overall drm_device is visible to
> > userspace. All others must be added before drm_dev_register and
> > removed after drm_dev_unregister.
> >
> > Small issue around drivers still using the load/unload callbacks, we
> > need to make sure we set dev->registered so that load/unload code in
> > these callbacks doesn't trigger false warnings. Only a small
> > adjustement in drm_dev_register was needed.
> >
> > Motivated by some irc discussions about object ids of dynamic objects
> > like blobs become invalid, and me going on a bit an audit spree.
> >
>
> Seems like a very worthwhile change, any idea how many drivers are going
> to be sad after this change?

None I think/hope, really just defense WARN_ON just in case. The main
ones that would be sad are all the ones that have a ->load callback,
but I'm taking care of them. Everyone else does things correctly and
calls drm_dev_register last in their probe function (or around where
they set up fbdev, which is also register the driver at least to the
fbdev world, so really the same).

> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_drv.c         | 4 ++--
> >  drivers/gpu/drm/drm_mode_object.c | 4 ++++
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index cb6f0245de7c..48c84e3e1931 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -997,14 +997,14 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
> >       if (ret)
> >               goto err_minors;
> >
> > -     dev->registered = true;
> > -
> >       if (dev->driver->load) {
> >               ret = dev->driver->load(dev, flags);
> >               if (ret)
> >                       goto err_minors;
> >       }
> >
> > +     dev->registered = true;
> > +
> >       if (drm_core_check_feature(dev, DRIVER_MODESET))
> >               drm_modeset_register_all(dev);
> >
> > diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> > index 1c6e51135962..c355ba8e6d5d 100644
> > --- a/drivers/gpu/drm/drm_mode_object.c
> > +++ b/drivers/gpu/drm/drm_mode_object.c
> > @@ -42,6 +42,8 @@ int __drm_mode_object_add(struct drm_device *dev, struct drm_mode_object *obj,
> >  {
> >       int ret;
> >
> > +     WARN_ON(dev->registered && !obj_free_cb);
>
> These should probably have a comment above them giving some guidance to the
> driver developer.
>
> With some comments, this is:

What comment do you expect here? drm_dev_register explains what you
should do already, and I expect driver developers to find that one
pretty quickly. From there: "This should be done last in the device
initialization sequence to make sure userspace can't access an
inconsistent state."
-Daniel

> Reviewed-by: Sean Paul <sean@poorly.run>
>
>
> > +
> >       mutex_lock(&dev->mode_config.idr_mutex);
> >       ret = idr_alloc(&dev->mode_config.object_idr, register_obj ? obj : NULL,
> >                       1, 0, GFP_KERNEL);
> > @@ -102,6 +104,8 @@ void drm_mode_object_register(struct drm_device *dev,
> >  void drm_mode_object_unregister(struct drm_device *dev,
> >                               struct drm_mode_object *object)
> >  {
> > +     WARN_ON(dev->registered && !object->free_cb);
> > +
> >       mutex_lock(&dev->mode_config.idr_mutex);
> >       if (object->id) {
> >               idr_remove(&dev->mode_config.object_idr, object->id);
> > --
> > 2.20.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/kms: Catch mode_object lifetime errors

[Souza, Jose]

On Sat, 2019-06-29 at 17:39 +0200, Daniel Vetter wrote:
> On Fri, Jun 28, 2019 at 7:24 PM Sean Paul <sean@poorly.run> wrote:
> > On Fri, Jun 14, 2019 at 08:17:23AM +0200, Daniel Vetter wrote:
> > > Only dynamic mode objects, i.e. those which are refcounted and
> > > have a free
> > > callback, can be added while the overall drm_device is visible to
> > > userspace. All others must be added before drm_dev_register and
> > > removed after drm_dev_unregister.
> > > 
> > > Small issue around drivers still using the load/unload callbacks,
> > > we
> > > need to make sure we set dev->registered so that load/unload code
> > > in
> > > these callbacks doesn't trigger false warnings. Only a small
> > > adjustement in drm_dev_register was needed.
> > > 
> > > Motivated by some irc discussions about object ids of dynamic
> > > objects
> > > like blobs become invalid, and me going on a bit an audit spree.
> > > 
> > 
> > Seems like a very worthwhile change, any idea how many drivers are
> > going
> > to be sad after this change?
> 
> None I think/hope, really just defense WARN_ON just in case. The main
> ones that would be sad are all the ones that have a ->load callback,
> but I'm taking care of them. Everyone else does things correctly and
> calls drm_dev_register last in their probe function (or around where
> they set up fbdev, which is also register the driver at least to the
> fbdev world, so really the same).
> 
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_drv.c         | 4 ++--
> > >  drivers/gpu/drm/drm_mode_object.c | 4 ++++
> > >  2 files changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_drv.c
> > > b/drivers/gpu/drm/drm_drv.c
> > > index cb6f0245de7c..48c84e3e1931 100644
> > > --- a/drivers/gpu/drm/drm_drv.c
> > > +++ b/drivers/gpu/drm/drm_drv.c
> > > @@ -997,14 +997,14 @@ int drm_dev_register(struct drm_device
> > > *dev, unsigned long flags)
> > >       if (ret)
> > >               goto err_minors;
> > > 
> > > -     dev->registered = true;
> > > -
> > >       if (dev->driver->load) {
> > >               ret = dev->driver->load(dev, flags);
> > >               if (ret)
> > >                       goto err_minors;
> > >       }
> > > 
> > > +     dev->registered = true;
> > > +
> > >       if (drm_core_check_feature(dev, DRIVER_MODESET))
> > >               drm_modeset_register_all(dev);
> > > 
> > > diff --git a/drivers/gpu/drm/drm_mode_object.c
> > > b/drivers/gpu/drm/drm_mode_object.c
> > > index 1c6e51135962..c355ba8e6d5d 100644
> > > --- a/drivers/gpu/drm/drm_mode_object.c
> > > +++ b/drivers/gpu/drm/drm_mode_object.c
> > > @@ -42,6 +42,8 @@ int __drm_mode_object_add(struct drm_device
> > > *dev, struct drm_mode_object *obj,
> > >  {
> > >       int ret;
> > > 
> > > +     WARN_ON(dev->registered && !obj_free_cb);

Getting warnings on i915 with MST, we add fake MST connectors when a
sink with MST support is connected;
 
intel_dp_add_mst_connector()->drm_connector_attach_max_bpc_property()

Not sure how to fix that, add a global i915 device property like we do
for "audio" and "Broadcast RGB" don't look the right approach here.
Any tips?

We definitely need a platform with a MST sink on our CI.

> > 
> > These should probably have a comment above them giving some
> > guidance to the
> > driver developer.
> > 
> > With some comments, this is:
> 
> What comment do you expect here? drm_dev_register explains what you
> should do already, and I expect driver developers to find that one
> pretty quickly. From there: "This should be done last in the device
> initialization sequence to make sure userspace can't access an
> inconsistent state."
> -Daniel
> 
> > Reviewed-by: Sean Paul <sean@poorly.run>
> > 
> > 
> > > +
> > >       mutex_lock(&dev->mode_config.idr_mutex);
> > >       ret = idr_alloc(&dev->mode_config.object_idr, register_obj
> > > ? obj : NULL,
> > >                       1, 0, GFP_KERNEL);
> > > @@ -102,6 +104,8 @@ void drm_mode_object_register(struct
> > > drm_device *dev,
> > >  void drm_mode_object_unregister(struct drm_device *dev,
> > >                               struct drm_mode_object *object)
> > >  {
> > > +     WARN_ON(dev->registered && !object->free_cb);
> > > +
> > >       mutex_lock(&dev->mode_config.idr_mutex);
> > >       if (object->id) {
> > >               idr_remove(&dev->mode_config.object_idr, object-
> > > >id);
> > > --
> > > 2.20.1
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > --
> > Sean Paul, Software Engineer, Google / Chromium OS
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/kms: Catch mode_object lifetime errors

[Daniel Vetter]

On Sat, Aug 17, 2019 at 12:42 AM Souza, Jose <jose.souza@intel.com> wrote:
> On Sat, 2019-06-29 at 17:39 +0200, Daniel Vetter wrote:
> > On Fri, Jun 28, 2019 at 7:24 PM Sean Paul <sean@poorly.run> wrote:
> > > On Fri, Jun 14, 2019 at 08:17:23AM +0200, Daniel Vetter wrote:
> > > > Only dynamic mode objects, i.e. those which are refcounted and
> > > > have a free
> > > > callback, can be added while the overall drm_device is visible to
> > > > userspace. All others must be added before drm_dev_register and
> > > > removed after drm_dev_unregister.
> > > >
> > > > Small issue around drivers still using the load/unload callbacks,
> > > > we
> > > > need to make sure we set dev->registered so that load/unload code
> > > > in
> > > > these callbacks doesn't trigger false warnings. Only a small
> > > > adjustement in drm_dev_register was needed.
> > > >
> > > > Motivated by some irc discussions about object ids of dynamic
> > > > objects
> > > > like blobs become invalid, and me going on a bit an audit spree.
> > > >
> > >
> > > Seems like a very worthwhile change, any idea how many drivers are
> > > going
> > > to be sad after this change?
> >
> > None I think/hope, really just defense WARN_ON just in case. The main
> > ones that would be sad are all the ones that have a ->load callback,
> > but I'm taking care of them. Everyone else does things correctly and
> > calls drm_dev_register last in their probe function (or around where
> > they set up fbdev, which is also register the driver at least to the
> > fbdev world, so really the same).
> >
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_drv.c         | 4 ++--
> > > >  drivers/gpu/drm/drm_mode_object.c | 4 ++++
> > > >  2 files changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_drv.c
> > > > b/drivers/gpu/drm/drm_drv.c
> > > > index cb6f0245de7c..48c84e3e1931 100644
> > > > --- a/drivers/gpu/drm/drm_drv.c
> > > > +++ b/drivers/gpu/drm/drm_drv.c
> > > > @@ -997,14 +997,14 @@ int drm_dev_register(struct drm_device
> > > > *dev, unsigned long flags)
> > > >       if (ret)
> > > >               goto err_minors;
> > > >
> > > > -     dev->registered = true;
> > > > -
> > > >       if (dev->driver->load) {
> > > >               ret = dev->driver->load(dev, flags);
> > > >               if (ret)
> > > >                       goto err_minors;
> > > >       }
> > > >
> > > > +     dev->registered = true;
> > > > +
> > > >       if (drm_core_check_feature(dev, DRIVER_MODESET))
> > > >               drm_modeset_register_all(dev);
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_mode_object.c
> > > > b/drivers/gpu/drm/drm_mode_object.c
> > > > index 1c6e51135962..c355ba8e6d5d 100644
> > > > --- a/drivers/gpu/drm/drm_mode_object.c
> > > > +++ b/drivers/gpu/drm/drm_mode_object.c
> > > > @@ -42,6 +42,8 @@ int __drm_mode_object_add(struct drm_device
> > > > *dev, struct drm_mode_object *obj,
> > > >  {
> > > >       int ret;
> > > >
> > > > +     WARN_ON(dev->registered && !obj_free_cb);
>
> Getting warnings on i915 with MST, we add fake MST connectors when a
> sink with MST support is connected;
>
> intel_dp_add_mst_connector()->drm_connector_attach_max_bpc_property()
>
> Not sure how to fix that, add a global i915 device property like we do
> for "audio" and "Broadcast RGB" don't look the right approach here.
> Any tips?
>
> We definitely need a platform with a MST sink on our CI.

Uh yeah this is bad. I guess we need to preemptively create all
possible bpc properties, so that we can only do an attach. Those
really can't be hotplugged.

We might also want to revert

commit 5ca0ef8a56b8c4812ed78ef9ca53052191dab6e7
Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
Date:   Tue Mar 26 16:25:54 2019 +0200

    drm/i915: Add max_bpc property for DP MST

as an interim solution. Adding Ville and Jani.
-Daniel

> > >
> > > These should probably have a comment above them giving some
> > > guidance to the
> > > driver developer.
> > >
> > > With some comments, this is:
> >
> > What comment do you expect here? drm_dev_register explains what you
> > should do already, and I expect driver developers to find that one
> > pretty quickly. From there: "This should be done last in the device
> > initialization sequence to make sure userspace can't access an
> > inconsistent state."
> > -Daniel
> >
> > > Reviewed-by: Sean Paul <sean@poorly.run>
> > >
> > >
> > > > +
> > > >       mutex_lock(&dev->mode_config.idr_mutex);
> > > >       ret = idr_alloc(&dev->mode_config.object_idr, register_obj
> > > > ? obj : NULL,
> > > >                       1, 0, GFP_KERNEL);
> > > > @@ -102,6 +104,8 @@ void drm_mode_object_register(struct
> > > > drm_device *dev,
> > > >  void drm_mode_object_unregister(struct drm_device *dev,
> > > >                               struct drm_mode_object *object)
> > > >  {
> > > > +     WARN_ON(dev->registered && !object->free_cb);
> > > > +
> > > >       mutex_lock(&dev->mode_config.idr_mutex);
> > > >       if (object->id) {
> > > >               idr_remove(&dev->mode_config.object_idr, object-
> > > > >id);
> > > > --
> > > > 2.20.1
> > > >
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > > --
> > > Sean Paul, Software Engineer, Google / Chromium OS
> >
> >



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/kms: Catch mode_object lifetime errors

[Jani Nikula]

On Tue, 20 Aug 2019, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Sat, Aug 17, 2019 at 12:42 AM Souza, Jose <jose.souza@intel.com> wrote:
>> On Sat, 2019-06-29 at 17:39 +0200, Daniel Vetter wrote:
>> > On Fri, Jun 28, 2019 at 7:24 PM Sean Paul <sean@poorly.run> wrote:
>> > > On Fri, Jun 14, 2019 at 08:17:23AM +0200, Daniel Vetter wrote:
>> > > > Only dynamic mode objects, i.e. those which are refcounted and
>> > > > have a free
>> > > > callback, can be added while the overall drm_device is visible to
>> > > > userspace. All others must be added before drm_dev_register and
>> > > > removed after drm_dev_unregister.
>> > > >
>> > > > Small issue around drivers still using the load/unload callbacks,
>> > > > we
>> > > > need to make sure we set dev->registered so that load/unload code
>> > > > in
>> > > > these callbacks doesn't trigger false warnings. Only a small
>> > > > adjustement in drm_dev_register was needed.
>> > > >
>> > > > Motivated by some irc discussions about object ids of dynamic
>> > > > objects
>> > > > like blobs become invalid, and me going on a bit an audit spree.
>> > > >
>> > >
>> > > Seems like a very worthwhile change, any idea how many drivers are
>> > > going
>> > > to be sad after this change?
>> >
>> > None I think/hope, really just defense WARN_ON just in case. The main
>> > ones that would be sad are all the ones that have a ->load callback,
>> > but I'm taking care of them. Everyone else does things correctly and
>> > calls drm_dev_register last in their probe function (or around where
>> > they set up fbdev, which is also register the driver at least to the
>> > fbdev world, so really the same).
>> >
>> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>> > > > ---
>> > > >  drivers/gpu/drm/drm_drv.c         | 4 ++--
>> > > >  drivers/gpu/drm/drm_mode_object.c | 4 ++++
>> > > >  2 files changed, 6 insertions(+), 2 deletions(-)
>> > > >
>> > > > diff --git a/drivers/gpu/drm/drm_drv.c
>> > > > b/drivers/gpu/drm/drm_drv.c
>> > > > index cb6f0245de7c..48c84e3e1931 100644
>> > > > --- a/drivers/gpu/drm/drm_drv.c
>> > > > +++ b/drivers/gpu/drm/drm_drv.c
>> > > > @@ -997,14 +997,14 @@ int drm_dev_register(struct drm_device
>> > > > *dev, unsigned long flags)
>> > > >       if (ret)
>> > > >               goto err_minors;
>> > > >
>> > > > -     dev->registered = true;
>> > > > -
>> > > >       if (dev->driver->load) {
>> > > >               ret = dev->driver->load(dev, flags);
>> > > >               if (ret)
>> > > >                       goto err_minors;
>> > > >       }
>> > > >
>> > > > +     dev->registered = true;
>> > > > +
>> > > >       if (drm_core_check_feature(dev, DRIVER_MODESET))
>> > > >               drm_modeset_register_all(dev);
>> > > >
>> > > > diff --git a/drivers/gpu/drm/drm_mode_object.c
>> > > > b/drivers/gpu/drm/drm_mode_object.c
>> > > > index 1c6e51135962..c355ba8e6d5d 100644
>> > > > --- a/drivers/gpu/drm/drm_mode_object.c
>> > > > +++ b/drivers/gpu/drm/drm_mode_object.c
>> > > > @@ -42,6 +42,8 @@ int __drm_mode_object_add(struct drm_device
>> > > > *dev, struct drm_mode_object *obj,
>> > > >  {
>> > > >       int ret;
>> > > >
>> > > > +     WARN_ON(dev->registered && !obj_free_cb);
>>
>> Getting warnings on i915 with MST, we add fake MST connectors when a
>> sink with MST support is connected;
>>
>> intel_dp_add_mst_connector()->drm_connector_attach_max_bpc_property()
>>
>> Not sure how to fix that, add a global i915 device property like we do
>> for "audio" and "Broadcast RGB" don't look the right approach here.
>> Any tips?
>>
>> We definitely need a platform with a MST sink on our CI.
>
> Uh yeah this is bad. I guess we need to preemptively create all
> possible bpc properties, so that we can only do an attach. Those
> really can't be hotplugged.

Should we plan to make all objects dynamic and refcounted in the long
run? It's kind of silly that we've made connectors dynamic yet can't
dynamically allocate properties for them. (Except blob properties which
are special dynamic snowflakes.)

> We might also want to revert
>
> commit 5ca0ef8a56b8c4812ed78ef9ca53052191dab6e7
> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Date:   Tue Mar 26 16:25:54 2019 +0200
>
>     drm/i915: Add max_bpc property for DP MST
>
> as an interim solution. Adding Ville and Jani.

I think we'll want the fixed, pre-created DP MST max BPC property,
shoved in dev_priv, as an interim solution before we have everything
dynamic. This shouldn't really be more than 50-100 lines, especially
given that we'll only need the 6-12 BPC range for MST connectors, and
the rest can use the usual methods.

Whether we need the revert as an interim solution before the interim
solution, I'll leave it up to Ville. How quick do you think you'd get to
fixing this?

Other than that, ugh. Just ugh.

BR,
Jani.


> -Daniel
>
>> > >
>> > > These should probably have a comment above them giving some
>> > > guidance to the
>> > > driver developer.
>> > >
>> > > With some comments, this is:
>> >
>> > What comment do you expect here? drm_dev_register explains what you
>> > should do already, and I expect driver developers to find that one
>> > pretty quickly. From there: "This should be done last in the device
>> > initialization sequence to make sure userspace can't access an
>> > inconsistent state."
>> > -Daniel
>> >
>> > > Reviewed-by: Sean Paul <sean@poorly.run>
>> > >
>> > >
>> > > > +
>> > > >       mutex_lock(&dev->mode_config.idr_mutex);
>> > > >       ret = idr_alloc(&dev->mode_config.object_idr, register_obj
>> > > > ? obj : NULL,
>> > > >                       1, 0, GFP_KERNEL);
>> > > > @@ -102,6 +104,8 @@ void drm_mode_object_register(struct
>> > > > drm_device *dev,
>> > > >  void drm_mode_object_unregister(struct drm_device *dev,
>> > > >                               struct drm_mode_object *object)
>> > > >  {
>> > > > +     WARN_ON(dev->registered && !object->free_cb);
>> > > > +
>> > > >       mutex_lock(&dev->mode_config.idr_mutex);
>> > > >       if (object->id) {
>> > > >               idr_remove(&dev->mode_config.object_idr, object-
>> > > > >id);
>> > > > --
>> > > > 2.20.1
>> > > >
>> > > > _______________________________________________
>> > > > dri-devel mailing list
>> > > > dri-devel@lists.freedesktop.org
>> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> > >
>> > > --
>> > > Sean Paul, Software Engineer, Google / Chromium OS
>> >
>> >

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/kms: Catch mode_object lifetime errors

[Daniel Vetter]

On Tue, Aug 20, 2019 at 11:28 AM Jani Nikula <jani.nikula@intel.com> wrote:
>
> On Tue, 20 Aug 2019, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > On Sat, Aug 17, 2019 at 12:42 AM Souza, Jose <jose.souza@intel.com> wrote:
> >> On Sat, 2019-06-29 at 17:39 +0200, Daniel Vetter wrote:
> >> > On Fri, Jun 28, 2019 at 7:24 PM Sean Paul <sean@poorly.run> wrote:
> >> > > On Fri, Jun 14, 2019 at 08:17:23AM +0200, Daniel Vetter wrote:
> >> > > > Only dynamic mode objects, i.e. those which are refcounted and
> >> > > > have a free
> >> > > > callback, can be added while the overall drm_device is visible to
> >> > > > userspace. All others must be added before drm_dev_register and
> >> > > > removed after drm_dev_unregister.
> >> > > >
> >> > > > Small issue around drivers still using the load/unload callbacks,
> >> > > > we
> >> > > > need to make sure we set dev->registered so that load/unload code
> >> > > > in
> >> > > > these callbacks doesn't trigger false warnings. Only a small
> >> > > > adjustement in drm_dev_register was needed.
> >> > > >
> >> > > > Motivated by some irc discussions about object ids of dynamic
> >> > > > objects
> >> > > > like blobs become invalid, and me going on a bit an audit spree.
> >> > > >
> >> > >
> >> > > Seems like a very worthwhile change, any idea how many drivers are
> >> > > going
> >> > > to be sad after this change?
> >> >
> >> > None I think/hope, really just defense WARN_ON just in case. The main
> >> > ones that would be sad are all the ones that have a ->load callback,
> >> > but I'm taking care of them. Everyone else does things correctly and
> >> > calls drm_dev_register last in their probe function (or around where
> >> > they set up fbdev, which is also register the driver at least to the
> >> > fbdev world, so really the same).
> >> >
> >> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> >> > > > ---
> >> > > >  drivers/gpu/drm/drm_drv.c         | 4 ++--
> >> > > >  drivers/gpu/drm/drm_mode_object.c | 4 ++++
> >> > > >  2 files changed, 6 insertions(+), 2 deletions(-)
> >> > > >
> >> > > > diff --git a/drivers/gpu/drm/drm_drv.c
> >> > > > b/drivers/gpu/drm/drm_drv.c
> >> > > > index cb6f0245de7c..48c84e3e1931 100644
> >> > > > --- a/drivers/gpu/drm/drm_drv.c
> >> > > > +++ b/drivers/gpu/drm/drm_drv.c
> >> > > > @@ -997,14 +997,14 @@ int drm_dev_register(struct drm_device
> >> > > > *dev, unsigned long flags)
> >> > > >       if (ret)
> >> > > >               goto err_minors;
> >> > > >
> >> > > > -     dev->registered = true;
> >> > > > -
> >> > > >       if (dev->driver->load) {
> >> > > >               ret = dev->driver->load(dev, flags);
> >> > > >               if (ret)
> >> > > >                       goto err_minors;
> >> > > >       }
> >> > > >
> >> > > > +     dev->registered = true;
> >> > > > +
> >> > > >       if (drm_core_check_feature(dev, DRIVER_MODESET))
> >> > > >               drm_modeset_register_all(dev);
> >> > > >
> >> > > > diff --git a/drivers/gpu/drm/drm_mode_object.c
> >> > > > b/drivers/gpu/drm/drm_mode_object.c
> >> > > > index 1c6e51135962..c355ba8e6d5d 100644
> >> > > > --- a/drivers/gpu/drm/drm_mode_object.c
> >> > > > +++ b/drivers/gpu/drm/drm_mode_object.c
> >> > > > @@ -42,6 +42,8 @@ int __drm_mode_object_add(struct drm_device
> >> > > > *dev, struct drm_mode_object *obj,
> >> > > >  {
> >> > > >       int ret;
> >> > > >
> >> > > > +     WARN_ON(dev->registered && !obj_free_cb);
> >>
> >> Getting warnings on i915 with MST, we add fake MST connectors when a
> >> sink with MST support is connected;
> >>
> >> intel_dp_add_mst_connector()->drm_connector_attach_max_bpc_property()
> >>
> >> Not sure how to fix that, add a global i915 device property like we do
> >> for "audio" and "Broadcast RGB" don't look the right approach here.
> >> Any tips?
> >>
> >> We definitely need a platform with a MST sink on our CI.
> >
> > Uh yeah this is bad. I guess we need to preemptively create all
> > possible bpc properties, so that we can only do an attach. Those
> > really can't be hotplugged.
>
> Should we plan to make all objects dynamic and refcounted in the long
> run? It's kind of silly that we've made connectors dynamic yet can't
> dynamically allocate properties for them. (Except blob properties which
> are special dynamic snowflakes.)

Need to make sure userspace is ok with that. Plus given that it took
us years to make drm_connector refcounting not be a pure fireworks
show (and Lyude is still busy) I'm not too enthusiastic about this.

Also even with the objects which have been dynamic since forever (edid
blobs) userspace mixed things up since we reuse ids. It's a mess.

> > We might also want to revert
> >
> > commit 5ca0ef8a56b8c4812ed78ef9ca53052191dab6e7
> > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Date:   Tue Mar 26 16:25:54 2019 +0200
> >
> >     drm/i915: Add max_bpc property for DP MST
> >
> > as an interim solution. Adding Ville and Jani.
>
> I think we'll want the fixed, pre-created DP MST max BPC property,
> shoved in dev_priv, as an interim solution before we have everything
> dynamic. This shouldn't really be more than 50-100 lines, especially
> given that we'll only need the 6-12 BPC range for MST connectors, and
> the rest can use the usual methods.
>
> Whether we need the revert as an interim solution before the interim
> solution, I'll leave it up to Ville. How quick do you think you'd get to
> fixing this?
>
> Other than that, ugh. Just ugh.

Agreed on that :-/
-Daniel

>
> BR,
> Jani.
>
>
> > -Daniel
> >
> >> > >
> >> > > These should probably have a comment above them giving some
> >> > > guidance to the
> >> > > driver developer.
> >> > >
> >> > > With some comments, this is:
> >> >
> >> > What comment do you expect here? drm_dev_register explains what you
> >> > should do already, and I expect driver developers to find that one
> >> > pretty quickly. From there: "This should be done last in the device
> >> > initialization sequence to make sure userspace can't access an
> >> > inconsistent state."
> >> > -Daniel
> >> >
> >> > > Reviewed-by: Sean Paul <sean@poorly.run>
> >> > >
> >> > >
> >> > > > +
> >> > > >       mutex_lock(&dev->mode_config.idr_mutex);
> >> > > >       ret = idr_alloc(&dev->mode_config.object_idr, register_obj
> >> > > > ? obj : NULL,
> >> > > >                       1, 0, GFP_KERNEL);
> >> > > > @@ -102,6 +104,8 @@ void drm_mode_object_register(struct
> >> > > > drm_device *dev,
> >> > > >  void drm_mode_object_unregister(struct drm_device *dev,
> >> > > >                               struct drm_mode_object *object)
> >> > > >  {
> >> > > > +     WARN_ON(dev->registered && !object->free_cb);
> >> > > > +
> >> > > >       mutex_lock(&dev->mode_config.idr_mutex);
> >> > > >       if (object->id) {
> >> > > >               idr_remove(&dev->mode_config.object_idr, object-
> >> > > > >id);
> >> > > > --
> >> > > > 2.20.1
> >> > > >
> >> > > > _______________________________________________
> >> > > > dri-devel mailing list
> >> > > > dri-devel@lists.freedesktop.org
> >> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >> > >
> >> > > --
> >> > > Sean Paul, Software Engineer, Google / Chromium OS
> >> >
> >> >
>
> --
> Jani Nikula, Intel Open Source Graphics Center



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx