[PULL] drm-xe-next
Hi Dave & Simona First drm-xe-next pull request for 6.13 Thanks, Thomas drm-xe-next-2024-10-10: Cross-subsystem Changes: - Add drm_line_printer (Michal) Driver Changes: - Fix an UAF (Matt Auld) - Sanity check compression and coherency mode (Matt Auld) - Some PIC-ID work (Jani) - Use IS_ENABLED() instead of defined() on config options. - gt powergating work (Riana) - Suppress missing out ter rpm protection warning (Rodrigo) - Fix a vm leak (Dafna) - Clean up and update 'has_flat_ccs' handling (Lucas) - Fix arg to pci_iomap (Lucas) - Mark reserved engines in shapshot (Lucas) - Don't keep stale pointer (Michal) - Fix build warning with CONFIG_PM=n (Arnd) - Add a xe_bo subtest for shrinking / swapping (Thomas) - Add a warkaround (Tejas) - Some display PM work (Maarten) - Enable Xe2 + PES disaggregation (Ashutosh) - Large xe_mmio rework / cleanup (Matt Roper) - A couple of fixes / cleanups in the xe client code (Matt Auld) - Fix page-fault handling on closed VMs (Matt Brost) - Fix overflow in OA batch buffer (José) - Style fixes (Lucas, Jiapeng, Nitin) - Fixes and new development around SRIOV (Michal) - Use devm_add_action_or_reset() in gt code (He) - Fix CCS offset calculation (Matt Auld) - Remove i915_drv.h include (Rodrigo) - Restore PCI state on resume (Rodrigo) - Fix DSB buffer coherency / Revert DSB disabling (Maarten / Animesh) - Convert USM lock to rwsem (Matt Brost) - Defer gt-mmio intialization (Matt Roper) - meemirq changes (Ilia) - Move some PVC related code out of xe-for-CI and to the driver (Rodrigo / Jani) - Use a helper for ASID->VM lookup (Matt Brost) - Add new PCI id for ARL (Dnyaneshwar) - Use Xe2_LPM steering tables for Xe2_HPM (Gustavo) - Performance tuning work for media GT and L3 cache flushing (Gustavo) - Clean up VM- and exec queue file lock usage (Matt Brost) - GuC locking fix (Matt Auld) - Fix UAF around queue destruction (Matt Auld) - Move IRQ-related registers to dedicated header (Matt Roper) - Resume TDR after GT reset (Matt Brost) - Move xa_alloc to prevent UAF (Matt Auld) - Fix OA stream close (José) - Remove unused i915_gpu_error.h (Jani) - Prevent null pointer access in xe_migrate_copy (Zhanjun) - Fix memory leak when aborting binds (Matt Brost) - Prevent UAF in send_recv() (Matt Auld) - Fix xa_store() error checking (Matt Auld) - drop irq disabling around xa_erase in guc code (Matt Auld) - Use fault injection infrastructure to find issues as probe time (Francois) - Fix a workaround implementation. (Vinay) - Mark wedged_mode debugfs writable (Matt Roper) - Fix for prviewous memirq work (Michal) - More SRIOV work (Michal) - Devcoredump work (John) - GuC logging + devcoredump support (John) - Don't report L3 bank availability on PTL (Shekhar) - Replicate Xe2 PAT settings on Xe2 (Matt Roper) - Define Xe3 feature flags (Haridhar) - Reuse Xe2 MOCS table on on PTL (Haridhar) - Add PTL platform definition (Haridhar) - Add MCR steering for Xe3 (Matt) - More work around GuC capture for devcoredump (Zhanjun) - Improve cache flushing behaviour on bmg (Matt Auld) - Fix shrinker test compiler warnings on 32-bit (Thomas) - Initial set of workarounds for Xe3 (Gustavo) - Extend workaround for xe2lpg (Aradhya) - Fix unbalanced rpm put x 2 (Matt Auld) The following changes since commit 9852d85ec9d492ebef56dc5f229416c925758edc: Linux 6.12-rc1 (2024-09-29 15:06:19 -0700) are available in the Git repository at: https://gitlab.freedesktop.org/drm/xe/kernel.git tags/drm-xe-next-2024-10-10 for you to fetch changes up to a187c1b0a800565a4db6372268692aff99df7f53: drm/xe: fix unbalanced rpm put() with declare_wedged() (2024-10-10 09:15:59 +0100) Cross-subsystem Changes: - Add drm_line_printer (Michal) Driver Changes: - Fix an UAF (Matt Auld) - Sanity check compression and coherency mode (Matt Auld) - Some PIC-ID work (Jani) - Use IS_ENABLED() instead of defined() on config options. - gt powergating work (Riana) - Suppress missing out ter rpm protection warning (Rodrigo) - Fix a vm leak (Dafna) - Clean up and update 'has_flat_ccs' handling (Lucas) - Fix arg to pci_iomap (Lucas) - Mark reserved engines in shapshot (Lucas) - Don't keep stale pointer (Michal) - Fix build warning with CONFIG_PM=n (Arnd) - Add a xe_bo subtest for shrinking / swapping (Thomas) - Add a warkaround (Tejas) - Some display PM work (Maarten) - Enable Xe2 + PES disaggregation (Ashutosh) - Large xe_mmio rework / cleanup (Matt Roper) - A couple of fixes / cleanups in the xe client code (Matt Auld) - Fix page-fault handling on closed VMs (Matt Brost) - Fix overflow in OA batch buffer (José) - Style fixes (Lucas, Jiapeng, Nitin) - Fixes and new development around SRIOV (Michal) - Use devm_add_action_or_reset() in gt code (He) - Fix CCS offset calculation (Matt Auld) - Remove i915_drv.h include (Rodrigo) - Restore PCI state on resume (Rodrigo) - Fix DSB buffer coherency / Revert DSB disabling (Maarten / Animesh) - Convert USM lock to rwsem (Matt Brost)
[PULL] drm-xe-fixes
Hi Dave and Sima Two small fixes this week. Thanks, Thomas drm-xe-fixes-2024-07-04: Driver Changes: - One copy/paste mistake fix. - One error path fix causing an error pointer dereference. The following changes since commit 22a40d14b572deb80c0648557f4bd502d7e83826: Linux 6.10-rc6 (2024-06-30 14:40:44 -0700) are available in the Git repository at: https://gitlab.freedesktop.org/drm/xe/kernel.git tags/drm-xe-fixes-2024-07-04 for you to fetch changes up to 1f006470284598060ca1307355352934400b37ca: drm/xe/mcr: Avoid clobbering DSS steering (2024-07-04 10:36:30 +0200) Driver Changes: - One copy/paste mistake fix. - One error path fix causing an error pointer dereference. Matt Roper (1): drm/xe/mcr: Avoid clobbering DSS steering Matthew Auld (1): drm/xe: fix error handling in xe_migrate_update_pgtables drivers/gpu/drm/xe/xe_gt_mcr.c | 6 +++--- drivers/gpu/drm/xe/xe_migrate.c | 8 2 files changed, 7 insertions(+), 7 deletions(-)
[PULL] drm-xe-fixes
Hi, Dave, Sima A single fix this week. Thanks, Thomas drm-xe-fixes-2024-06-20: Driver Changes: - Fix for invalid register access The following changes since commit 6ba59ff4227927d3a8530fc2973b80e94b54d58f: Linux 6.10-rc4 (2024-06-16 13:40:16 -0700) are available in the Git repository at: https://gitlab.freedesktop.org/drm/xe/kernel.git tags/drm-xe-fixes-2024-06-20 for you to fetch changes up to d21d44dbdde83c4a8553c95de1853e63e88d7954: drm/xe/vf: Don't touch GuC irq registers if using memory irqs (2024-06-20 09:22:37 +0200) Driver Changes: - Fix for invalid register access Michal Wajdeczko (1): drm/xe/vf: Don't touch GuC irq registers if using memory irqs drivers/gpu/drm/xe/xe_guc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
[PULL] drm-xe-fixes
Hi Dave & Sima, This week's drm-xe-fixes PR. Except from the maintainer update, nothing major, really. The "drm/xe: move disable_c6 call" required some conflict resolution (both visible and silent) which was also reflected in a conflict with drm-next when building drm-tip. drm-xe-fixes-2024-06-13: Core Changes: - Xe Maintainers update to MAINTAINERS file. Driver Changes: - Use correct forcewake assertions. - Assert that VRAM provisioning is only done on DGFX. - Flush render caches before user-fence signalling on all engines. - Move the disable_c6 call since it was sometimes never called. Thanks, Thomas The following changes since commit 0698ff57bf327d9a5735a898f78161b8dada160b: drm/xe/pf: Update the LMTT when freeing VF GT config (2024-06-04 16:31:24 +0200) are available in the Git repository at: https://gitlab.freedesktop.org/drm/xe/kernel.git tags/drm-xe-fixes-2024-06-13 for you to fetch changes up to 2470b141bfae2b9695b5b6823e3b978b22d33dde: drm/xe: move disable_c6 call (2024-06-13 12:35:13 +0200) Core Changes: - Xe Maintainers update to MAINTAINERS file. Driver Changes: - Use correct forcewake assertions. - Assert that VRAM provisioning is only done on DGFX. - Flush render caches before user-fence signalling on all engines. - Move the disable_c6 call since it was sometimes never called. Andrzej Hajda (1): drm/xe: flush engine buffers before signalling user fence on all engines Michal Wajdeczko (1): drm/xe/pf: Assert LMEM provisioning is done only on DGFX Oded Gabbay (1): MAINTAINERS: update Xe driver maintainers Riana Tauro (2): drm/xe/xe_gt_idle: use GT forcewake domain assertion drm/xe: move disable_c6 call Thomas Hellström (1): MAINTAINERS: Update Xe driver maintainers MAINTAINERS| 2 +- drivers/gpu/drm/xe/xe_gt_idle.c| 9 - drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c | 15 +-- drivers/gpu/drm/xe/xe_guc_pc.c | 6 -- drivers/gpu/drm/xe/xe_ring_ops.c | 18 -- 5 files changed, 38 insertions(+), 12 deletions(-)
[PULL] drm-xe-fixes
Dave and Sima, A single fix for a missing Local Memory Translation Table update for -rc3. Thanks, Thomas drm-xe-fixes-2024-06-04: Driver Changes: - drm/xe/pf: Update the LMTT when freeing VF GT config The following changes since commit 6c5cd0807c79eb4c0cda70b48f6be668a241d584: drm/xe: Properly handle alloc_guc_id() failure (2024-05-28 08:53:45 +0200) are available in the Git repository at: https://gitlab.freedesktop.org/drm/xe/kernel.git tags/drm-xe-fixes-2024-06-04 for you to fetch changes up to 0698ff57bf327d9a5735a898f78161b8dada160b: drm/xe/pf: Update the LMTT when freeing VF GT config (2024-06-04 16:31:24 +0200) Driver Changes: - drm/xe/pf: Update the LMTT when freeing VF GT config Michal Wajdeczko (1): drm/xe/pf: Update the LMTT when freeing VF GT config drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c | 1 + 1 file changed, 1 insertion(+)
[PULL] drm-xe-fixes
Hi Dave, Sima The drm-xe-fixes for -rc2 Only three fixes so far. I'm holding back one additional fix to be able to sort out whether it's correct or need more work. drm-xe-fixes-2024-05-30: Driver Changes: - One pcode polling timeout change - One fix for deadlocks for faulting VMs - One error-path lock imbalance fix The following changes since commit 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0: Linux 6.10-rc1 (2024-05-26 15:20:12 -0700) are available in the Git repository at: https://gitlab.freedesktop.org/drm/xe/kernel.git tags/drm-xe-fixes-2024-05-30 for you to fetch changes up to 6c5cd0807c79eb4c0cda70b48f6be668a241d584: drm/xe: Properly handle alloc_guc_id() failure (2024-05-28 08:53:45 +0200) Driver Changes: - One pcode polling timeout change - One fix for deadlocks for faulting VMs - One error-path lock imbalance fix Himal Prasad Ghimiray (1): drm/xe: Change pcode timeout to 50msec while polling again Matthew Brost (1): drm/xe: Only use reserved BCS instances for usm migrate exec queue Niranjana Vishwanathapura (1): drm/xe: Properly handle alloc_guc_id() failure drivers/gpu/drm/xe/xe_guc_submit.c | 1 + drivers/gpu/drm/xe/xe_migrate.c| 12 +--- drivers/gpu/drm/xe/xe_pcode.c | 2 +- 3 files changed, 7 insertions(+), 8 deletions(-)
[PULL] drm-xe-next-fixes
Hi, Dave & Sima This weeks -next-fixes. Two fixes breifly described below. Driver Changes: - Use ordered WQ for G2H handler. (Matthew Brost) - Use flexible-array rather than zero-sized (Lucas De Marchi) Thanks, Thomas The following changes since commit 3bc8848bb7f7478e6806e4522b06b63f40a53e1e: drm/xe: Merge 16021540221 and 18034896535 WAs (2024-05-02 11:29:42 +0200) are available in the Git repository at: https://gitlab.freedesktop.org/drm/xe/kernel.git tags/drm-xe-next-fixes-2024-05-09-1 for you to fetch changes up to d69c3d4b53829097b8948d6791ea32c07de3faab: drm/xe/ads: Use flexible-array (2024-05-09 17:51:46 +0200) Driver Changes: - Use ordered WQ for G2H handler. (Matthew Brost) - Use flexible-array rather than zero-sized (Lucas De Marchi) Lucas De Marchi (1): drm/xe/ads: Use flexible-array Matthew Brost (1): drm/xe: Use ordered WQ for G2H handler drivers/gpu/drm/xe/xe_guc_ads.c | 2 +- drivers/gpu/drm/xe/xe_guc_ct.c | 5 + drivers/gpu/drm/xe/xe_guc_ct.h | 2 +- drivers/gpu/drm/xe/xe_guc_ct_types.h | 2 ++ 4 files changed, 9 insertions(+), 2 deletions(-)
[PULL] drm-xe-next-fixes
Dave, Sima This week's small set of fixes for drm-next. drm-xe-next-fixes-2024-05-02: Driver Changes: - Fix for a backmerge going slightly wrong. - An UAF fix - Avoid a WA error on LNL. Thanks, Thomas The following changes since commit 4a56c0ed5aa0bcbe1f5f7d755fb1fe1ebf48ae9c: Merge tag 'amd-drm-next-6.10-2024-04-26' of https://gitlab.freedesktop.org/agd5f/linux into drm-next (2024-04-30 14:43:00 +1000) are available in the Git repository at: https://gitlab.freedesktop.org/drm/xe/kernel.git tags/drm-xe-next-fixes-2024-05-02 for you to fetch changes up to 3bc8848bb7f7478e6806e4522b06b63f40a53e1e: drm/xe: Merge 16021540221 and 18034896535 WAs (2024-05-02 11:29:42 +0200) Driver Changes: - Fix for a backmerge going slightly wrong. - An UAF fix - Avoid a WA error on LNL. Lucas De Marchi (1): drm/xe: Merge 16021540221 and 18034896535 WAs Matthew Auld (1): drm/xe/vm: prevent UAF in rebind_work_func() Thomas Hellström (1): drm/xe: Fix unexpected backmerge results drivers/gpu/drm/xe/xe_vm.c | 16 ++-- drivers/gpu/drm/xe/xe_vm_types.h | 4 drivers/gpu/drm/xe/xe_wa.c | 7 +-- 3 files changed, 15 insertions(+), 12 deletions(-)
[PULL] drm-xe-next
Hi, Dave, Sima The main -next 6.10 pull request for the xe driver. I scanned through the patches and tried to provide a somewhat condensed log below. Nothing spectacular in the uAPI changes. Among other things there are some flags that are reinstated now that we have published UMD for them. Unfortunately some of the underlying implementation got somehow lost in a backmerge but there is a patch pending to reinstate that. Will send another pull-request this week, or if you want I can resend this one when the patch passes review and CI with the patch included. Some hickups unfortunately in that we carry a couple of i915 patches. One that got mistakenly commited to drm-xe-next, but was later acked-by Rodrigo for carrying in drm-xe-next to simplify handling. There is also one that was part of the PM rework, and a fix for that patch. Thanks, Thomas drm-xe-next-2024-04-23: UAPI Changes: - Remove unused flags (Francois Dugast) - Extend uAPI to query HuC micro-controler firmware version (Francois Dugast) - drm/xe/uapi: Define topology types as indexes rather than masks (Francois Dugast) - drm/xe/uapi: Restore flags VM_BIND_FLAG_READONLY and VM_BIND_FLAG_IMMEDIATE (Francois Dugast) - devcoredump updates. Some touching the output format. (José Roberto de Souza, Matthew Brost) - drm/xe/hwmon: Add infra to support card power and energy attributes - Improve LRC, HWSP and HWCTX error capture. (Maarten Lankhorst) - drm/xe/uapi: Add IP version and stepping to GT list query (Matt roper) - Invalidate userptr VMA on page pin fault (Matthew Brost) - Improve xe_bo_move tracepoint (Priyanka Danamudi) - Align fence output format in ftrace log Cross-driver Changes: - drm/i915/hwmon: Get rid of devm (Ashutosh Dixit) (Acked-by: Rodrigo Vivi ) - drm/i915/display: convert inner wakeref get towards get_if_in_use (SOB Rodrigo Vivi) - drm/i915: Convert intel_runtime_pm_get_noresume towards raw wakeref (Committer, SOB Jani Nikula) Driver Changes: - Fix for unneeded CCS metadata allocation (Akshata Jahagirdar) - Fix for fix multicast support for Xe_LP platforms (Andrzej Hajda) - A couple of build fixes (Arnd Bergmann) - Fix register definition (Ashutosh Dixit) - Add BMG mocs table (Balasubramani Vivekanandan) - Replace sprintf() across driver (Bommu Krishnaiah) - Add an xe2 workaround (Bommu Krishnaiah) - Makefile fix (Dafna Hirschfeld) - force_wake_get error value check (Daniele Ceraolo Spurio) - Handle GSCCS ER interrupt (Daniele Ceraolo Spurio) - GSC Workaround (Daniele Ceraolo Spurio) - Build error fix (Dawei Li) - drm/xe/gt: Add L3 bank mask to GT topology (Francois Dugast) - Implement xe2- and GuC workarounds (Gustavo Sousa, Haridhar Kalvala, Himal rasad Ghimiray, John Harrison, Matt Roper, Radhakrishna Sripada, Vinay Belgaumkar, Badal Nilawar) - xe2hpg compression (Himal Ghimiray Prasad) - Error code cleanups and fixes (Himal Prasad Ghimiray) - struct xe_device cleanup (Jani Nikula) - Avoid validating bos when only requesting an exec dma-fence (José Roberto de Souza) - Remove debug message from migrate_clear (José Roberto de Souza) - Nuke EXEC_QUEUE_FLAG_PERSISTENT leftover internal flag (José Roberto de Souza) - Mark dpt and related vma as uncached (Juha-Pekka Heikkila) - Hwmon updates (Karthik Poosa) - KConfig fix when ACPI_WMI selcted (Lu Yao) - Update intel_uncore_read*() return types (Luca Coelho) - Mocs updates (Lucas De Marchi, Matt Roper) - Drop dynamic load-balancing workaround (Lucas De Marchi) - Fix a PVC workaround (Lucas De Marchi) - Group live kunit tests into a single module (Lucas De Marchi) - Various code cleanups (Lucas De Marchi) - Fix a ggtt init error patch and move ggtt invalidate out of ggtt lock (Maarten Lankhorst) - Fix a bo leak (Marten Lankhorst) - Add LRC parsing for more GPU instructions (Matt Roper) - Add various definitions for hardware and IP (Matt Roper) - Define all possible engines in media IP descriptors (Matt Roper) - Various cleanups, asserts and code fixes (Matthew Auld) - Various cleanups and code fixes (Matthew Brost) - Increase VM_BIND number of per-ioctl Ops (Matthew Brost, Paulo Zanoni) - Don't support execlists in xe_gt_tlb_invalidation layer (Matthew Brost) - Handle timing out of already signaled jobs gracefully (Matthew Brost) - Pipeline evict / restore of pinned BOs during suspend / resume (Matthew Brost) - Do not grab forcewakes when issuing GGTT TLB invalidation via GuC (Matthew Brost) - Drop ggtt invalidate from display code (Matthew Brost) - drm/xe: Add XE_BO_GGTT_INVALIDATE flag (Matthew Brost) - Add debug messages for MMU notifier and VMA invalidate (Matthew Brost) - Use ordered wq for preempt fence waiting (Matthew Brost) - Initial development for SR-IOV support including some refactoring (Michal Wajdeczko) - Various GuC- and GT- related cleanups and fixes (Michal Wajdeczko) - Move userptr over to start using hmm_range_fault (Oak Zeng) - Add new PCI IDs to DG2 platform (Ravi Kumar Vodapalli) - Pcode - and VRAM initialization check update (Riana Ta
[PULL] drm-xe-fixes
Hi Dave, Sima A single error path fix for 6.8 final (-rc8). Thanks, Thomas drm-xe-fixes-2024-03-07: Driver Changes: - An error path fix. The following changes since commit 90d35da658da8cff0d4ecbb5113f5fac9d00eb72: Linux 6.8-rc7 (2024-03-03 13:02:52 -0800) are available in the Git repository at: https://gitlab.freedesktop.org/drm/xe/kernel.git tags/drm-xe-fixes-2024-03-07 for you to fetch changes up to a4e7596e209783a7be2727d6b947cbd863c2bbcb: drm/xe: Return immediately on tile_init failure (2024-03-07 09:13:38 +0100) Driver Changes: - An error path fix. Rodrigo Vivi (1): drm/xe: Return immediately on tile_init failure drivers/gpu/drm/xe/xe_tile.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
[PULL] drm-xe-fixes
Dave, Sima The xe fixes for -rc7. It's mostly uapi sanitizing and future-proofing, and a couple of driver fixes. drm-xe-fixes-2024-02-29: UAPI Changes: - A couple of tracepoint updates from Priyanka and Lucas. - Make sure BINDs are completed before accepting UNBINDs on LR vms. - Don't arbitrarily restrict max number of batched binds. - Add uapi for dumpable bos (agreed on IRC). - Remove unused uapi flags and a leftover comment. Driver Changes: - A couple of fixes related to the execlist backend. - A 32-bit fix. /Thomas The following changes since commit 6650d23f3e20ca00482a71a4ef900f0ea776fb15: drm/xe: Fix modpost warning on xe_mocs kunit module (2024-02-21 11:06:52 +0100) are available in the Git repository at: https://gitlab.freedesktop.org/drm/xe/kernel.git tags/drm-xe-fixes-2024-02-29 for you to fetch changes up to 8188cae3cc3d8018ec97ca9ab8caa3acc69a056d: drm/xe/xe_trace: Add move_lacks_source detail to xe_bo_move trace (2024-02-29 12:32:15 +0100) UAPI Changes: - A couple of tracepoint updates from Priyanka and Lucas. - Make sure BINDs are completed before accepting UNBINDs on LR vms. - Don't arbitrarily restrict max number of batched binds. - Add uapi for dumpable bos (agreed on IRC). - Remove unused uapi flags and a leftover comment. Driver Changes: - A couple of fixes related to the execlist backend. - A 32-bit fix. Arnd Bergmann (1): drm/xe/mmio: fix build warning for BAR resize on 32-bit Francois Dugast (1): drm/xe/uapi: Remove unused flags José Roberto de Souza (1): drm/xe/uapi: Remove DRM_XE_VM_BIND_FLAG_ASYNC comment left over Lucas De Marchi (1): drm/xe: Use pointers in trace events Maarten Lankhorst (1): drm/xe: Add uapi for dumpable bos Matthew Brost (3): drm/xe: Fix execlist splat drm/xe: Don't support execlists in xe_gt_tlb_invalidation layer drm/xe: Use vmalloc for array of bind allocation in bind IOCTL Mika Kuoppala (2): drm/xe: Expose user fence from xe_sync_entry drm/xe: Deny unbinds if uapi ufence pending Paulo Zanoni (1): drm/xe: get rid of MAX_BINDS Priyanka Dandamudi (2): drm/xe/xe_bo_move: Enhance xe_bo_move trace drm/xe/xe_trace: Add move_lacks_source detail to xe_bo_move trace drivers/gpu/drm/xe/xe_bo.c | 11 +++- drivers/gpu/drm/xe/xe_bo.h | 1 + drivers/gpu/drm/xe/xe_drm_client.c | 12 +--- drivers/gpu/drm/xe/xe_exec_queue.c | 88 + drivers/gpu/drm/xe/xe_exec_queue_types.h| 10 drivers/gpu/drm/xe/xe_execlist.c| 2 +- drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c | 12 drivers/gpu/drm/xe/xe_lrc.c | 10 +--- drivers/gpu/drm/xe/xe_mmio.c| 2 +- drivers/gpu/drm/xe/xe_sync.c| 58 +++ drivers/gpu/drm/xe/xe_sync.h| 4 ++ drivers/gpu/drm/xe/xe_sync_types.h | 2 +- drivers/gpu/drm/xe/xe_trace.h | 59 +-- drivers/gpu/drm/xe/xe_vm.c | 80 ++ drivers/gpu/drm/xe/xe_vm_types.h| 11 ++-- include/uapi/drm/xe_drm.h | 21 +-- 16 files changed, 187 insertions(+), 196 deletions(-)
[PULL] drm-xe-fixes
Hi, Dave Sima The Xe pull request for 6.8-rc6 The uAPI fixes / adjustments we've been discussing are starting to appear, and I will hopefully have the rest for next week's PR. In addition two driver fixes. drm-xe-fixes-2024-02-22: UAPI Changes: - Remove support for persistent exec_queues - Drop a reduntant sysfs newline printout Cross-subsystem Changes: Core Changes: Driver Changes: - A three-patch fix for a VM_BIND rebind optimization path - Fix a modpost warning on an xe KUNIT module /Thomas The following changes since commit b401b621758e46812da61fa58a67c3fd8d91de0d: Linux 6.8-rc5 (2024-02-18 12:56:25 -0800) are available in the Git repository at: https://gitlab.freedesktop.org/drm/xe/kernel.git tags/drm-xe-fixes-2024-02-22 for you to fetch changes up to 6650d23f3e20ca00482a71a4ef900f0ea776fb15: drm/xe: Fix modpost warning on xe_mocs kunit module (2024-02-21 11:06:52 +0100) UAPI Changes: - Remove support for persistent exec_queues - Drop a reduntant sysfs newline printout Cross-subsystem Changes: Core Changes: Driver Changes: - A three-patch fix for a VM_BIND rebind optimization path - Fix a modpost warning on an xe KUNIT module Ashutosh Dixit (2): drm/xe/xe_gt_idle: Drop redundant newline in name drm/xe: Fix modpost warning on xe_mocs kunit module Matthew Brost (3): drm/xe: Fix xe_vma_set_pte_size drm/xe: Add XE_VMA_PTE_64K VMA flag drm/xe: Return 2MB page size for compact 64k PTEs Thomas Hellström (1): drm/xe/uapi: Remove support for persistent exec_queues drivers/gpu/drm/xe/tests/xe_mocs_test.c | 1 + drivers/gpu/drm/xe/xe_device.c | 39 drivers/gpu/drm/xe/xe_device.h | 4 drivers/gpu/drm/xe/xe_device_types.h | 8 --- drivers/gpu/drm/xe/xe_exec_queue.c | 33 --- drivers/gpu/drm/xe/xe_exec_queue_types.h | 10 drivers/gpu/drm/xe/xe_execlist.c | 2 -- drivers/gpu/drm/xe/xe_gt_idle.c | 4 ++-- drivers/gpu/drm/xe/xe_guc_submit.c | 2 -- drivers/gpu/drm/xe/xe_pt.c | 11 ++--- drivers/gpu/drm/xe/xe_vm.c | 14 drivers/gpu/drm/xe/xe_vm_types.h | 2 ++ include/uapi/drm/xe_drm.h| 1 - 13 files changed, 28 insertions(+), 103 deletions(-)
[PULL] drm-xe-fixes
Hi Dave, Sima! The xe fixes pull request for -rc5. drm-xe-fixes-2024-02-15: Driver Changes: - Fix an out-of-bounds shift. - Fix the display code thinking xe uses shmem - Fix a warning about index out-of-bound - Fix a clang-16 compilation warning Thanks, Thomas The following changes since commit bf4c27b8267d7848bb81fd41e6aa07aa662f07fb: drm/xe: Remove TEST_VM_ASYNC_OPS_ERROR (2024-02-08 09:51:19 +0100) are available in the Git repository at: https://gitlab.freedesktop.org/drm/xe/kernel.git tags/drm-xe-fixes-2024-02-15 for you to fetch changes up to 455dae7549aed709707feda5d6b3e085b37d33f7: drm/xe: avoid function cast warnings (2024-02-15 09:53:38 +0100) Driver Changes: - Fix an out-of-bounds shift. - Fix the display code thinking xe uses shmem - Fix a warning about index out-of-bound - Fix a clang-16 compilation warning Arnd Bergmann (1): drm/xe: avoid function cast warnings Matthew Auld (1): drm/xe/display: fix i915_gem_object_is_shmem() wrapper Thomas Hellström (2): drm/xe/vm: Avoid reserving zero fences drm/xe/pt: Allow for stricter type- and range checking .../xe/compat-i915-headers/gem/i915_gem_object.h | 2 +- drivers/gpu/drm/xe/xe_pt.c | 39 ++ drivers/gpu/drm/xe/xe_pt_walk.c| 2 +- drivers/gpu/drm/xe/xe_pt_walk.h| 19 ++- drivers/gpu/drm/xe/xe_range_fence.c| 7 +++- drivers/gpu/drm/xe/xe_vm.c | 13 ++-- 6 files changed, 46 insertions(+), 36 deletions(-)
[PULL] drm-xe-fixes
Dave, Sima The drm-xe-fixes pull for -rc4. Thanks, Thomas drm-xe-fixes-2024-02-08: Driver Changes: - Fix a loop in an error path - Fix a missing dma-fence reference - Fix a retry path on userptr REMAP - Workaround for a false gcc warning - Fix missing map of the usm batch buffer in the migrate vm. - Fix a memory leak. - Fix a bad assumption of used page size - Fix hitting a BUG() due to zero pages to map. - Remove some leftover async bind queue relics The following changes since commit 54be6c6c5ae8e0d93a6c4641cb7528eb0b6ba478: Linux 6.8-rc3 (2024-02-04 12:20:36 +) are available in the Git repository at: https://gitlab.freedesktop.org/drm/xe/kernel.git tags/drm-xe-fixes-2024-02-08 for you to fetch changes up to bf4c27b8267d7848bb81fd41e6aa07aa662f07fb: drm/xe: Remove TEST_VM_ASYNC_OPS_ERROR (2024-02-08 09:51:19 +0100) Driver Changes: - Fix a loop in an error path - Fix a missing dma-fence reference - Fix a retry path on userptr REMAP - Workaround for a false gcc warning - Fix missing map of the usm batch buffer in the migrate vm. - Fix a memory leak. - Fix a bad assumption of used page size - Fix hitting a BUG() due to zero pages to map. - Remove some leftover async bind queue relics Arnd Bergmann (1): drm/xe: circumvent bogus stringop-overflow warning Matthew Auld (1): drm/xe/vm: don't ignore error when in_kthread Matthew Brost (6): drm/xe: Fix loop in vm_bind_ioctl_ops_unwind drm/xe: Take a reference in xe_exec_queue_last_fence_get() drm/xe: Pick correct userptr VMA to repin on REMAP op failure drm/xe: Map both mem.kernel_bb_pool and usm.bb_pool drm/xe: Assume large page size if VMA not yet bound drm/xe: Remove TEST_VM_ASYNC_OPS_ERROR Xiaoming Wang (1): drm/xe/display: Fix memleak in display initialization drivers/gpu/drm/xe/xe_display.c | 6 drivers/gpu/drm/xe/xe_exec_queue.c | 8 +++-- drivers/gpu/drm/xe/xe_gt.c | 5 ++- drivers/gpu/drm/xe/xe_gt_pagefault.c | 2 +- drivers/gpu/drm/xe/xe_migrate.c | 28 drivers/gpu/drm/xe/xe_sched_job.c| 1 - drivers/gpu/drm/xe/xe_sync.c | 2 -- drivers/gpu/drm/xe/xe_vm.c | 62 ++-- drivers/gpu/drm/xe/xe_vm_types.h | 8 - 9 files changed, 57 insertions(+), 65 deletions(-)
[PULL] drm-xe-fixes
Hi Dave and Sima, The xe fixes for 6.8-rc2. drm-xe-fixes-2024-02-01: UAPI Changes: - Only allow a single user-fence per exec / bind. The reason for this clarification fix is a limitation in the implementation which can be lifted moving forward, if needed. Driver Changes: - A crash fix - A fix for an assert due to missing mem_acces ref - Some sparse warning fixes - Two fixes for compilation failures on various odd combinations of gcc / arch pointed out on LKML. - Fix a fragile partial allocation pointed out on LKML. Cross-driver Change: - A sysfs ABI documentation warning fix This also touches i915 and is acked by i915 maintainers. Thanks, Thomas The following changes since commit 9e3a13f3eef6b14a26cc2660ca2f43f0e46b4318: drm/xe: Remove PVC from xe_wa kunit tests (2024-01-24 11:13:55 +0100) are available in the Git repository at: https://gitlab.freedesktop.org/drm/xe/kernel.git tags/drm-xe-fixes-2024-02-01 for you to fetch changes up to 5f16ee27cd5abd5166e28b2311ac693c204063ff: drm/hwmon: Fix abi doc warnings (2024-02-01 12:04:52 +0100) UAPI Changes: - Only allow a single user-fence per exec / bind. The reason for this clarification fix is a limitation in the implementation which can be lifted moving forward, if needed. Driver Changes: - A crash fix - A fix for an assert due to missing mem_acces ref - Only allow a single user-fence per exec / bind. - Some sparse warning fixes - Two fixes for compilation failures on various odd combinations of gcc / arch pointed out on LKML. - Fix a fragile partial allocation pointed out on LKML. Cross-driver Change: - A sysfs ABI documentation warning fix This also touches i915 and is acked by i915 maintainers. Badal Nilawar (1): drm/hwmon: Fix abi doc warnings José Roberto de Souza (1): drm/xe: Fix crash in trace_dma_fence_init() Matt Roper (1): drm/xe: Grab mem_access when disabling C6 on skip_guc_pc platforms Matthew Brost (3): drm/xe: Only allow 1 ufence per exec / bind IOCTL drm/xe: Use LRC prefix rather than CTX prefix in lrc desc defines drm/xe: Make all GuC ABI shift values unsigned Thomas Hellström (3): drm/xe: Annotate mcr_[un]lock() drm/xe: Don't use __user error pointers drm/xe/vm: Subclass userptr vmas .../ABI/testing/sysfs-driver-intel-i915-hwmon | 14 +- .../ABI/testing/sysfs-driver-intel-xe-hwmon| 14 +- drivers/gpu/drm/xe/abi/guc_actions_abi.h | 4 +- drivers/gpu/drm/xe/abi/guc_actions_slpc_abi.h | 4 +- drivers/gpu/drm/xe/abi/guc_communication_ctb_abi.h | 8 +- drivers/gpu/drm/xe/abi/guc_klvs_abi.h | 6 +- drivers/gpu/drm/xe/abi/guc_messages_abi.h | 20 +-- drivers/gpu/drm/xe/xe_exec.c | 10 +- drivers/gpu/drm/xe/xe_gt_mcr.c | 4 +- drivers/gpu/drm/xe/xe_gt_pagefault.c | 11 +- drivers/gpu/drm/xe/xe_guc_pc.c | 2 + drivers/gpu/drm/xe/xe_hw_fence.c | 6 +- drivers/gpu/drm/xe/xe_lrc.c| 14 +- drivers/gpu/drm/xe/xe_pt.c | 32 ++-- drivers/gpu/drm/xe/xe_query.c | 50 +++ drivers/gpu/drm/xe/xe_sync.h | 5 + drivers/gpu/drm/xe/xe_vm.c | 165 - drivers/gpu/drm/xe/xe_vm.h | 16 +- drivers/gpu/drm/xe/xe_vm_types.h | 16 +- 19 files changed, 234 insertions(+), 167 deletions(-)
[PULL] drm-xe-fixes
Hi, Dave, Sima The Xe fixes PR for 6.8-rc2. Thanks, Thomas. The following changes since commit 6613476e225e090cc9aad49be7fa504e290dd33d: Linux 6.8-rc1 (2024-01-21 14:11:32 -0800) are available in the Git repository at: https://gitlab.freedesktop.org/drm/xe/kernel.git tags/drm-xe-fixes-2024-01-25 for you to fetch changes up to 9e3a13f3eef6b14a26cc2660ca2f43f0e46b4318: drm/xe: Remove PVC from xe_wa kunit tests (2024-01-24 11:13:55 +0100) Driver Changes: - Make an ops struct static - Fix an implicit 0 to NULL conversion - A couple of 32-bit fixes - A migration coherency fix for Lunar Lake. - An error path vm id leak fix - Remove PVC references in kunit tests Himal Prasad Ghimiray (1): drm/xe/xe2: Use XE_CACHE_WB pat index Lucas De Marchi (4): drm/xe: Use _ULL for u64 division drm/xe/mmio: Cast to u64 when printing drm/xe/display: Avoid calling readq() drm/xe: Remove PVC from xe_wa kunit tests Moti Haimovski (1): drm/xe/vm: bugfix in xe_vm_create_ioctl Thomas Hellström (2): drm/xe/dmabuf: Make xe_dmabuf_ops static drm/xe: Use a NULL pointer instead of 0. .../xe/compat-i915-headers/gem/i915_gem_object.h | 11 +-- drivers/gpu/drm/xe/tests/xe_wa_test.c | 3 --- drivers/gpu/drm/xe/xe_device.c | 2 +- drivers/gpu/drm/xe/xe_dma_buf.c| 2 +- drivers/gpu/drm/xe/xe_hwmon.c | 2 +- drivers/gpu/drm/xe/xe_migrate.c| 14 ++--- drivers/gpu/drm/xe/xe_mmio.c | 4 ++-- drivers/gpu/drm/xe/xe_vm.c | 23 +- 8 files changed, 31 insertions(+), 30 deletions(-)
Re: [Intel-gfx] [PATCH v3 20/22] drm/vmwgfx: Convert to CRTC VBLANK callbacks
On 1/20/20 9:23 AM, Thomas Zimmermann wrote: > VBLANK callbacks in struct drm_driver are deprecated in favor of > their equivalents in struct drm_crtc_funcs. Convert vmwgfx over. > > v2: > * remove accidental whitespace fixes > > Signed-off-by: Thomas Zimmermann > --- > drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 3 --- > drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 6 +++--- > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 6 +++--- > drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 3 +++ > drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 3 +++ > drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 3 +++ > 6 files changed, 15 insertions(+), 9 deletions(-) > Acked-by: Thomas Hellstrom ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 10/15] drm/vmwgfx: Delete mmaping functions
On Mon, 2019-11-18 at 11:35 +0100, Daniel Vetter wrote: > No need for stubs, dma-buf.c takes care of that. > > Aside, not having a ->release callback smelled like refcounting leak > somewhere. It will also score you a WARN_ON backtrace in dma-buf.c on > every export. But then I found that ttm_device_object_init overwrites > it. Overwriting const memory is not going to go down well in recent > kernels. It's actually taking a non-const copy and updating it. Not that that's much better, but at least it won't crash due to writing to wp memory. I'll add a backlog item to revisit this. > > One more aside: The (un)map_dma_buf can't ever be called because > ->attach rejects everything. Might want to drop a BUG_ON(1) in there. > Same for ->detach. And this. > > Signed-off-by: Daniel Vetter > Cc: VMware Graphics > Cc: Thomas Hellstrom > --- > Reviewed-by: Thomas Hellstrom Will you be taking this through drm-misc? Thanks, Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] dma_resv: prime lockdep annotations
On 10/21/19 4:50 PM, Daniel Vetter wrote: > Full audit of everyone: > > - i915, radeon, amdgpu should be clean per their maintainers. > > - vram helpers should be fine, they don't do command submission, so > really no business holding struct_mutex while doing copy_*_user. But > I haven't checked them all. > > - panfrost seems to dma_resv_lock only in panfrost_job_push, which > looks clean. > > - v3d holds dma_resv locks in the tail of its v3d_submit_cl_ioctl(), > copying from/to userspace happens all in v3d_lookup_bos which is > outside of the critical section. > > - vmwgfx has a bunch of ioctls that do their own copy_*_user: > - vmw_execbuf_process: First this does some copies in > vmw_execbuf_cmdbuf() and also in the vmw_execbuf_process() itself. > Then comes the usual ttm reserve/validate sequence, then actual > submission/fencing, then unreserving, and finally some more > copy_to_user in vmw_execbuf_copy_fence_user. Glossing over tons of > details, but looks all safe. > - vmw_fence_event_ioctl: No ttm_reserve/dma_resv_lock anywhere to be > seen, seems to only create a fence and copy it out. > - a pile of smaller ioctl in vmwgfx_ioctl.c, no reservations to be > found there. > Summary: vmwgfx seems to be fine too. > > - virtio: There's virtio_gpu_execbuffer_ioctl, which does all the > copying from userspace before even looking up objects through their > handles, so safe. Plus the getparam/getcaps ioctl, also both safe. > > - qxl only has qxl_execbuffer_ioctl, which calls into > qxl_process_single_command. There's a lovely comment before the > __copy_from_user_inatomic that the slowpath should be copied from > i915, but I guess that never happened. Try not to be unlucky and get > your CS data evicted between when it's written and the kernel tries > to read it. The only other copy_from_user is for relocs, but those > are done before qxl_release_reserve_list(), which seems to be the > only thing reserving buffers (in the ttm/dma_resv sense) in that > code. So looks safe. > > - A debugfs file in nouveau_debugfs_pstate_set() and the usif ioctl in > usif_ioctl() look safe. nouveau_gem_ioctl_pushbuf() otoh breaks this > everywhere and needs to be fixed up. > > v2: Thomas pointed at that vmwgfx calls dma_resv_init while it holds a > dma_resv lock of a different object already. Christian mentioned that > ttm core does this too for ghost objects. intel-gfx-ci highlighted > that i915 has similar issues. > > Unfortunately we can't do this in the usual module init functions, > because kernel threads don't have an ->mm - we have to wait around for > some user thread to do this. > > Solution is to spawn a worker (but only once). It's horrible, but it > works. > > v3: We can allocate mm! (Chris). Horrible worker hack out, clean > initcall solution in. > > v4: Annotate with __init (Rob Herring) > > Cc: Rob Herring > Cc: Alex Deucher > Cc: Christian König > Cc: Chris Wilson > Cc: Thomas Zimmermann > Cc: Rob Herring > Cc: Tomeu Vizoso > Cc: Eric Anholt > Cc: Dave Airlie > Cc: Gerd Hoffmann > Cc: Ben Skeggs > Cc: "VMware Graphics" > Cc: Thomas Hellstrom > Reviewed-by: Christian König > Reviewed-by: Chris Wilson > Tested-by: Chris Wilson > Signed-off-by: Daniel Vetter Including the vmwgfx audit, Reviewed-by: Thomas Hellstrom Thanks, Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 13/13] drm: allow render capable master with DRM_AUTH ioctls
On 5/27/19 3:16 PM, Daniel Vetter wrote:
On Mon, May 27, 2019 at 02:39:18PM +0200, Thomas Hellstrom wrote:
On 5/27/19 10:17 AM, Emil Velikov wrote:
From: Emil Velikov
There are cases (in mesa and applications) where one would open the
primary node without properly authenticating the client.
Sometimes we don't check if the authentication succeeds, but there's
also cases we simply forget to do it.
The former was a case for Mesa where it did not not check the return
value of drmGetMagic() [1]. That was fixed recently although, there's
the question of older drivers or other apps that exbibit this behaviour.
While omitting the call results in issues as seen in [2] and [3].
In the libva case, libva itself doesn't authenticate the DRM client and
the vaGetDisplayDRM documentation doesn't mention if the app should
either.
As of today, the official vainfo utility doesn't authenticate.
To workaround issues like these, some users resort to running their apps
under sudo. Which admittedly isn't always a good idea.
Since any DRIVER_RENDER driver has sufficient isolation between clients,
we can use that, for unauthenticated [primary node] ioctls that require
DRM_AUTH. But only if the respective ioctl is tagged as DRM_RENDER_ALLOW.
v2:
- Rework/simplify if check (Daniel V)
- Add examples to commit messages, elaborate. (Daniel V)
v3:
- Use single unlikely (Daniel V)
v4:
- Patch was reverted because it broke AMDGPU, apply again. The AMDGPU
issue is fixed with earlier patch.
[1]
https://gitlab.freedesktop.org/mesa/mesa/blob/2bc1f5c2e70fe3b4d41f060af9859bc2a94c5b62/src/egl/drivers/dri2/platform_wayland.c#L1136
[2] https://lists.freedesktop.org/archives/libva/2016-July/004185.html
[3] https://gitlab.freedesktop.org/mesa/kmscube/issues/1
Testcase: igt/core_unauth_vs_render
Cc: intel-gfx@lists.freedesktop.org
Signed-off-by: Emil Velikov
Reviewed-by: Daniel Vetter
Link:
https://patchwork.freedesktop.org/patch/msgid/20190114085408.15933-2-emil.l.veli...@gmail.com
---
drivers/gpu/drm/drm_ioctl.c | 20
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 9841c0076f02..b64b022a2b29 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -511,6 +511,13 @@ int drm_version(struct drm_device *dev, void *data,
return err;
}
+static inline bool
+drm_render_driver_and_ioctl(const struct drm_device *dev, u32 flags)
+{
+ return drm_core_check_feature(dev, DRIVER_RENDER) &&
+ (flags & DRM_RENDER_ALLOW);
+}
+
/**
* drm_ioctl_permit - Check ioctl permissions against caller
*
@@ -525,14 +532,19 @@ int drm_version(struct drm_device *dev, void *data,
*/
int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
{
+ const struct drm_device *dev = file_priv->minor->dev;
+
/* ROOT_ONLY is only for CAP_SYS_ADMIN */
if (unlikely((flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)))
return -EACCES;
- /* AUTH is only for authenticated or render client */
- if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) &&
-!file_priv->authenticated))
- return -EACCES;
+ /* AUTH is only for master ... */
+ if (unlikely((flags & DRM_AUTH) && drm_is_primary_client(file_priv))) {
+ /* authenticated ones, or render capable on DRM_RENDER_ALLOW. */
+ if (!file_priv->authenticated &&
+ !drm_render_driver_and_ioctl(dev, flags))
+ return -EACCES;
+ }
This breaks vmwgfx primary client authentication in the surface_reference
ioctl, which takes different paths in case of render clients and primary
clients, but adding an auth check in the primary path in the vmwgfx code
should fix this.
Hm yeah we need to adjust that ... otoh kinda not sure why this is gated
on authentication status, and not on "am I master or not" status. At least
from a very cursory read ...
-Daniel
The code snippet in question is:
if (drm_is_primary_client(file_priv) &&
user_srf->master != file_priv->master) {
DRM_ERROR("Trying to reference surface outside of"
" master domain.\n");
ret = -EACCES;
goto out_bad_resource;
}
In gem term's this means a client can't open a surface that hasn't been
flinked by a client in the same master realm: You can't read from
resources belonging to another X server's clients
/Thomas
/Thomas
/* MASTER is only for master or control clients */
if (unlikely((flags & DRM_MASTER) &&
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
https://lists.fre
Re: [Intel-gfx] [PATCH 13/13] drm: allow render capable master with DRM_AUTH ioctls
On 5/27/19 10:17 AM, Emil Velikov wrote:
From: Emil Velikov
There are cases (in mesa and applications) where one would open the
primary node without properly authenticating the client.
Sometimes we don't check if the authentication succeeds, but there's
also cases we simply forget to do it.
The former was a case for Mesa where it did not not check the return
value of drmGetMagic() [1]. That was fixed recently although, there's
the question of older drivers or other apps that exbibit this behaviour.
While omitting the call results in issues as seen in [2] and [3].
In the libva case, libva itself doesn't authenticate the DRM client and
the vaGetDisplayDRM documentation doesn't mention if the app should
either.
As of today, the official vainfo utility doesn't authenticate.
To workaround issues like these, some users resort to running their apps
under sudo. Which admittedly isn't always a good idea.
Since any DRIVER_RENDER driver has sufficient isolation between clients,
we can use that, for unauthenticated [primary node] ioctls that require
DRM_AUTH. But only if the respective ioctl is tagged as DRM_RENDER_ALLOW.
v2:
- Rework/simplify if check (Daniel V)
- Add examples to commit messages, elaborate. (Daniel V)
v3:
- Use single unlikely (Daniel V)
v4:
- Patch was reverted because it broke AMDGPU, apply again. The AMDGPU
issue is fixed with earlier patch.
[1]
https://gitlab.freedesktop.org/mesa/mesa/blob/2bc1f5c2e70fe3b4d41f060af9859bc2a94c5b62/src/egl/drivers/dri2/platform_wayland.c#L1136
[2] https://lists.freedesktop.org/archives/libva/2016-July/004185.html
[3] https://gitlab.freedesktop.org/mesa/kmscube/issues/1
Testcase: igt/core_unauth_vs_render
Cc: intel-gfx@lists.freedesktop.org
Signed-off-by: Emil Velikov
Reviewed-by: Daniel Vetter
Link:
https://patchwork.freedesktop.org/patch/msgid/20190114085408.15933-2-emil.l.veli...@gmail.com
---
drivers/gpu/drm/drm_ioctl.c | 20
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 9841c0076f02..b64b022a2b29 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -511,6 +511,13 @@ int drm_version(struct drm_device *dev, void *data,
return err;
}
+static inline bool
+drm_render_driver_and_ioctl(const struct drm_device *dev, u32 flags)
+{
+ return drm_core_check_feature(dev, DRIVER_RENDER) &&
+ (flags & DRM_RENDER_ALLOW);
+}
+
/**
* drm_ioctl_permit - Check ioctl permissions against caller
*
@@ -525,14 +532,19 @@ int drm_version(struct drm_device *dev, void *data,
*/
int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
{
+ const struct drm_device *dev = file_priv->minor->dev;
+
/* ROOT_ONLY is only for CAP_SYS_ADMIN */
if (unlikely((flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)))
return -EACCES;
- /* AUTH is only for authenticated or render client */
- if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) &&
-!file_priv->authenticated))
- return -EACCES;
+ /* AUTH is only for master ... */
+ if (unlikely((flags & DRM_AUTH) && drm_is_primary_client(file_priv))) {
+ /* authenticated ones, or render capable on DRM_RENDER_ALLOW. */
+ if (!file_priv->authenticated &&
+ !drm_render_driver_and_ioctl(dev, flags))
+ return -EACCES;
+ }
This breaks vmwgfx primary client authentication in the
surface_reference ioctl, which takes different paths in case of render
clients and primary clients, but adding an auth check in the primary
path in the vmwgfx code should fix this.
/Thomas
/* MASTER is only for master or control clients */
if (unlikely((flags & DRM_MASTER) &&
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/18] drm/vmwgfx: Add FIXME comments for customer page_flip handlers
Hi, Daniel, On 10/02/2018 03:35 PM, Daniel Vetter wrote: > The idea behind allowing drivers to override legacy ioctls (instead of > using the generic implementations unconditionally) is to handle bugs > in old driver-specific userspace. Like e.g. vmw_kms_set_config does, > to work around some vmwgfx userspace not clearing its ioctl structs > properly. > > But you can't use it to augment semantics and put in additional > checks, since from a correctly working userspace's pov there should > not be any difference in behaviour between the legacy and the atomic > paths. > > vmwgfx seems to be doing some strange things in its page_flip > handlers. Since I'm not an expert of this codebase just wrap some > FIXME comments around the potentially problematic code. > We've got proper patches for these. Apparently they got lost in my -next pull request, though. /Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/18] drm/vmwgfx: Remove confused comment from vmw_du_connector_atomic_set_property
On 10/02/2018 05:15 PM, Ville Syrjälä wrote:
> On Tue, Oct 02, 2018 at 03:35:12PM +0200, Daniel Vetter wrote:
>> The core _does_ the call to drm_atomic_commit for you. That's pretty
>> much the entire point of having the fancy new atomic_set/get_prop
>> callbacks.
>>
>> Signed-off-by: Daniel Vetter
>> Cc: VMware Graphics
>> Cc: Sinclair Yeh
>> Cc: Thomas Hellstrom
>> ---
>> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 6 --
>> 1 file changed, 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
>> b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
>> index 292e48feba83..049bd50eea87 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
>> @@ -2311,12 +2311,6 @@ vmw_du_connector_atomic_set_property(struct
>> drm_connector *connector,
>>
>> if (property == dev_priv->implicit_placement_property) {
>> vcs->is_implicit = val;
>> -
>> -/*
>> - * We should really be doing a drm_atomic_commit() to
>> - * commit the new state, but since this doesn't cause
>> - * an immedate state change, this is probably ok
>> - */
>> du->is_implicit = vcs->is_implicit;
> Maybe the comment is referring to delaying the du->is_implicit
> assignment to commit time? Otherwise a TEST_ONLY/failed commit
> will clobber this.
The is_implicit property is made read-only in a vmwgfx recent commit.
Not sure exactly where it ended up, though. (-fixes, -next or -limbo).
Need to take a look.
>
> Hmm. There's both .set_property() and .atomic_set_property()
> in there. I wonder what that's about.
Probably a leftover. I take it .set_property() is not needed when we
have .atomic_set_property()?
/Thomas
>
>> } else {
>> return -EINVAL;
>> --
>> 2.19.0.rc2
>>
>> ___
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fintel-gfx&data=02%7C01%7Cthellstrom%40vmware.com%7C8376824afaaa4e7ebd6808d6287a0a88%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636740901969428557&sdata=JDQsTWKhvZAyUnW76dNMFGm0nzJIJjNrSSJYtDuqDlg%3D&reserved=0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 14/14] drm/vmwgfx: Add FIXME comments for customer page_flip handlers
On 09/03/2018 06:54 PM, Daniel Vetter wrote: The idea behind allowing drivers to override legacy ioctls (instead of using the generic implementations unconditionally) is to handle bugs in old driver-specific userspace. Like e.g. vmw_kms_set_config does, to work around some vmwgfx userspace not clearing its ioctl structs properly. But you can't use it to augment semantics and put in additional checks, since from a correctly working userspace's pov there should not be any difference in behaviour between the legacy and the atomic paths. vmwgfx seems to be doing some strange things in its page_flip handlers. Since I'm not an expert of this codebase just wrap some FIXME comments around the potentially problematic code. Thanks for the patch, Daniel Your comments seem valid. We'll try to fix this internally before the next merge window rather than to add the FIXMEs /Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/9] drm: Begin an API for in-kernel clients
On 05/24/2018 12:14 PM, Daniel Vetter wrote: On Thu, May 24, 2018 at 11:25:04AM +0200, Thomas Hellstrom wrote: On 05/24/2018 10:32 AM, Daniel Vetter wrote: On Wed, May 23, 2018 at 11:45:00PM +0200, Thomas Hellstrom wrote: Hi, Noralf. A couple of issues below: On 05/23/2018 04:34 PM, Noralf Trønnes wrote: This the beginning of an API for in-kernel clients. First out is a way to get a framebuffer backed by a dumb buffer. Only GEM drivers are supported. The original idea of using an exported dma-buf was dropped because it also creates an anonomous file descriptor which doesn't work when the buffer is created from a kernel thread. The easy way out is to use drm_driver.gem_prime_vmap to get the virtual address, which requires a GEM object. This excludes the vmwgfx driver which is the only non-GEM driver apart from the legacy ones. A solution for vmwgfx will have to be worked out later if it wants to support the client API which it probably will when we have a bootsplash client. Couldn't you add vmap() and vunmap() to the dumb buffer API for in-kernel use rather than using GEM directly? But the main issue is pinning. It looks like the buffers are going to be vmapped() for a long time, which requires pinning, and that doesn't work for some drivers when they bind the framebuffer to a plane, since that might require pinning in another memory region and the vmap would have to be torn down. Besides, buffer pinning should really be avoided if possible: Since we can't page-fault vmaps, and setting up / tearing down vmaps is potentially an expensive operation, could we perhaps have a mapping api that allows the driver to cache vmaps? vmap() // Indicates that we want to map a bo begin_access() // Returns a virtual address which may vary between calls. Allows access. A fast operation. Behind the lines pins / reserves the bo and returns a cached vmap if the bo didn't move since last begin_access(), which is the typical case. end_access() // Disable access. Unpins / unreserves the bo. vunmap_cached() //Indicates that the map is no longer needed. The driver can release the cached map. The idea is that the API client would wrap all bo map accesses with begin_access() end_access(), allowing for the bo to be moved in between. So originally my ideas for the cpu side dma-buf interfaces where all meant to handle this. But then the first implementations bothered with none of this, but instead expected that stuff is pinned, and vmap Just Works. Which yeah doesn't work for vmwgfx and is a pain in a few other cases. I agree it'd be nice to fix all this, but it's also not a problem that this patch set here started. And since it's all optional (and vmwgfx isn't even using the current fb helper code) I think it's reasonable to address this post-merge (if someone gets around to it ever). What we'd need is is a fallback for when vmap doesn't exist (for fbdev that probably means a vmalloc'ed buffer + manual uploads, because fbdev), plus making sure dma-buf implementations actually implement it. My argument here is that, If I understand Noralf, this is intended to be an API exported outside of drm. In that case we shouldn't replicate the assumed behaviour of incomplete dma-buf implementations in a new API. Also the fact that vmwgfx currently isn't using the fbdev helpers isn't a good argument to design an API so that vmwgfx can _never_ use the fbdev helpers. The reason we aren't using them is that the kms implementation was so old that we didn't implement the necessary helper callbacks... Also, I might be misunderstanding the code a bit, but I doubt that vmwgfx is the only hardware with pinning restrictions on the framebuffer? I was under the assumption that most discrete hardware required the framebuffer to be pinned in VRAM? So the important question is, Is this a set of helpers for shared-memory GEM drivers to implement fbdev? Then I wouldn't bother, If it's intended to become an API for clients outside of DRM, then I would have to insist on the API being changed to reflect that. This is definitely not an api for anything outside of drm. Just an attempt to consolidate kernel-internal drm access like fbdev, a simple bootsplash or an emergency console would need to do. Having some limitations in the initial versions, as long as we have some idea how to handle them, seems perfectly fine to me. This isn't meant to be a mandatory replacement for anything - no intentions of exporting this to userspace. OK, yeah my concern is really for generic code and that there in the end would be too much code to change if we wanted to support this, but at least the generic code would be somewhat contained. But it seems like we're at least in agreement on the problematic areas, and as long as they are open for change I guess we can live with that. /Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/9] drm: Begin an API for in-kernel clients
On 05/24/2018 10:32 AM, Daniel Vetter wrote: On Wed, May 23, 2018 at 11:45:00PM +0200, Thomas Hellstrom wrote: Hi, Noralf. A couple of issues below: On 05/23/2018 04:34 PM, Noralf Trønnes wrote: This the beginning of an API for in-kernel clients. First out is a way to get a framebuffer backed by a dumb buffer. Only GEM drivers are supported. The original idea of using an exported dma-buf was dropped because it also creates an anonomous file descriptor which doesn't work when the buffer is created from a kernel thread. The easy way out is to use drm_driver.gem_prime_vmap to get the virtual address, which requires a GEM object. This excludes the vmwgfx driver which is the only non-GEM driver apart from the legacy ones. A solution for vmwgfx will have to be worked out later if it wants to support the client API which it probably will when we have a bootsplash client. Couldn't you add vmap() and vunmap() to the dumb buffer API for in-kernel use rather than using GEM directly? But the main issue is pinning. It looks like the buffers are going to be vmapped() for a long time, which requires pinning, and that doesn't work for some drivers when they bind the framebuffer to a plane, since that might require pinning in another memory region and the vmap would have to be torn down. Besides, buffer pinning should really be avoided if possible: Since we can't page-fault vmaps, and setting up / tearing down vmaps is potentially an expensive operation, could we perhaps have a mapping api that allows the driver to cache vmaps? vmap() // Indicates that we want to map a bo begin_access() // Returns a virtual address which may vary between calls. Allows access. A fast operation. Behind the lines pins / reserves the bo and returns a cached vmap if the bo didn't move since last begin_access(), which is the typical case. end_access() // Disable access. Unpins / unreserves the bo. vunmap_cached() //Indicates that the map is no longer needed. The driver can release the cached map. The idea is that the API client would wrap all bo map accesses with begin_access() end_access(), allowing for the bo to be moved in between. So originally my ideas for the cpu side dma-buf interfaces where all meant to handle this. But then the first implementations bothered with none of this, but instead expected that stuff is pinned, and vmap Just Works. Which yeah doesn't work for vmwgfx and is a pain in a few other cases. I agree it'd be nice to fix all this, but it's also not a problem that this patch set here started. And since it's all optional (and vmwgfx isn't even using the current fb helper code) I think it's reasonable to address this post-merge (if someone gets around to it ever). What we'd need is is a fallback for when vmap doesn't exist (for fbdev that probably means a vmalloc'ed buffer + manual uploads, because fbdev), plus making sure dma-buf implementations actually implement it. My argument here is that, If I understand Noralf, this is intended to be an API exported outside of drm. In that case we shouldn't replicate the assumed behaviour of incomplete dma-buf implementations in a new API. Also the fact that vmwgfx currently isn't using the fbdev helpers isn't a good argument to design an API so that vmwgfx can _never_ use the fbdev helpers. The reason we aren't using them is that the kms implementation was so old that we didn't implement the necessary helper callbacks... Also, I might be misunderstanding the code a bit, but I doubt that vmwgfx is the only hardware with pinning restrictions on the framebuffer? I was under the assumption that most discrete hardware required the framebuffer to be pinned in VRAM? So the important question is, Is this a set of helpers for shared-memory GEM drivers to implement fbdev? Then I wouldn't bother, If it's intended to become an API for clients outside of DRM, then I would have to insist on the API being changed to reflect that. /Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/9] drm: Begin an API for in-kernel clients
Hi, Noralf. A couple of issues below: On 05/23/2018 04:34 PM, Noralf Trønnes wrote: This the beginning of an API for in-kernel clients. First out is a way to get a framebuffer backed by a dumb buffer. Only GEM drivers are supported. The original idea of using an exported dma-buf was dropped because it also creates an anonomous file descriptor which doesn't work when the buffer is created from a kernel thread. The easy way out is to use drm_driver.gem_prime_vmap to get the virtual address, which requires a GEM object. This excludes the vmwgfx driver which is the only non-GEM driver apart from the legacy ones. A solution for vmwgfx will have to be worked out later if it wants to support the client API which it probably will when we have a bootsplash client. Couldn't you add vmap() and vunmap() to the dumb buffer API for in-kernel use rather than using GEM directly? But the main issue is pinning. It looks like the buffers are going to be vmapped() for a long time, which requires pinning, and that doesn't work for some drivers when they bind the framebuffer to a plane, since that might require pinning in another memory region and the vmap would have to be torn down. Besides, buffer pinning should really be avoided if possible: Since we can't page-fault vmaps, and setting up / tearing down vmaps is potentially an expensive operation, could we perhaps have a mapping api that allows the driver to cache vmaps? vmap() // Indicates that we want to map a bo begin_access() // Returns a virtual address which may vary between calls. Allows access. A fast operation. Behind the lines pins / reserves the bo and returns a cached vmap if the bo didn't move since last begin_access(), which is the typical case. end_access() // Disable access. Unpins / unreserves the bo. vunmap_cached() //Indicates that the map is no longer needed. The driver can release the cached map. The idea is that the API client would wrap all bo map accesses with begin_access() end_access(), allowing for the bo to be moved in between. /Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: More debug info for fb leaks in mode_config_cleanup
On 04/25/2018 08:49 AM, Daniel Vetter wrote:
On Wed, Apr 25, 2018 at 8:42 AM, Thomas Hellstrom wrote:
Hi,
On 12/07/2017 03:49 PM, Daniel Vetter wrote:
We're spotting this very rarely in CI, but have no idea. Let's add
more debug info about what's going on here.
References:
https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.freedesktop.org_show-5Fbug.cgi-3Fid-3D102707&d=DwIFaQ&c=uilaK90D4TOVoH58JNXRgQ&r=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ&m=HsUZYQaNMPVelJaUQrEYq1PTFDeRDixr0sJ9o5o5NNA&s=Fy9UT786mIt80SvkAKRdBU4Dy-_cr4ita_RmTA4Lqyk&e=
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/drm_mode_config.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/drm_mode_config.c
b/drivers/gpu/drm/drm_mode_config.c
index cc78b3d9e5e4..6ffe952142e6 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -469,6 +469,9 @@ void drm_mode_config_cleanup(struct drm_device *dev)
*/
WARN_ON(!list_empty(&dev->mode_config.fb_list));
list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head)
{
+ struct drm_printer p = drm_debug_printer("[leaked fb]");
+ drm_printf(&p, "framebuffer[%u]:\n", fb->base.id);
+ drm_framebuffer_print_info(&p, 1, fb);
drm_framebuffer_free(&fb->base.refcount);
}
Did we ever get to the bottom of what's causing those framebuffer leaks?
We've seen sporadic framebuffer leaks when transitioning between one and two
screens, but can't seem to repro with the latest drm-fixes branch?
claims the fixes, but that was incomplete. Ville's recent work to
restructure the legacy fb refcounting is meant to plug this leak once
and for all. Given that vmwgfx also has some custom code (for the
fbdev stuff) could very well be that you have such issues too. See:
Yes, hmm, the above doesn't fix our particular issue. I'll keep digging.
Thanks,
Thomas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: More debug info for fb leaks in mode_config_cleanup
Hi!
On 12/07/2017 03:49 PM, Daniel Vetter wrote:
We're spotting this very rarely in CI, but have no idea. Let's add
more debug info about what's going on here.
References:https://bugs.freedesktop.org/show_bug.cgi?id=102707
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/drm_mode_config.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/drm_mode_config.c
b/drivers/gpu/drm/drm_mode_config.c
index cc78b3d9e5e4..6ffe952142e6 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -469,6 +469,9 @@ void drm_mode_config_cleanup(struct drm_device *dev)
*/
WARN_ON(!list_empty(&dev->mode_config.fb_list));
list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) {
+ struct drm_printer p = drm_debug_printer("[leaked fb]");
+ drm_printf(&p, "framebuffer[%u]:\n", fb->base.id);
+ drm_framebuffer_print_info(&p, 1, fb);
drm_framebuffer_free(&fb->base.refcount);
}
Did we ever get to the bottom of what's causing this? We've seen
occasional framebuffer leaks when transitioning from one to two screens
and back again with vmwgfx on older drms, but I can't seem to reproduce
that with the latest upstream code?
Thanks,
Thomas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: More debug info for fb leaks in mode_config_cleanup
Hi,
On 12/07/2017 03:49 PM, Daniel Vetter wrote:
We're spotting this very rarely in CI, but have no idea. Let's add
more debug info about what's going on here.
References: https://bugs.freedesktop.org/show_bug.cgi?id=102707
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/drm_mode_config.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/drm_mode_config.c
b/drivers/gpu/drm/drm_mode_config.c
index cc78b3d9e5e4..6ffe952142e6 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -469,6 +469,9 @@ void drm_mode_config_cleanup(struct drm_device *dev)
*/
WARN_ON(!list_empty(&dev->mode_config.fb_list));
list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) {
+ struct drm_printer p = drm_debug_printer("[leaked fb]");
+ drm_printf(&p, "framebuffer[%u]:\n", fb->base.id);
+ drm_framebuffer_print_info(&p, 1, fb);
drm_framebuffer_free(&fb->base.refcount);
}
Did we ever get to the bottom of what's causing those framebuffer leaks?
We've seen sporadic framebuffer leaks when transitioning between one and
two screens, but can't seem to repro with the latest drm-fixes branch?
Thanks,
Thomas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/vmwgfx: Drop DRM_CONTROL_ALLOW
On 04/24/2018 03:01 PM, Daniel Vetter wrote:
On Fri, Apr 20, 2018 at 07:56:34PM +0200, Thomas Hellstrom wrote:
On 04/20/2018 08:51 AM, Daniel Vetter wrote:
Control nodes are no more!
Signed-off-by: Daniel Vetter
Cc: VMware Graphics
Cc: Sinclair Yeh
Cc: Thomas Hellstrom
---
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 70e1a8820a7c..97f37c3c16f2 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -159,14 +159,14 @@ static const struct drm_ioctl_desc vmw_ioctls[] = {
DRM_RENDER_ALLOW),
VMW_IOCTL_DEF(VMW_CURSOR_BYPASS,
vmw_kms_cursor_bypass_ioctl,
- DRM_MASTER | DRM_CONTROL_ALLOW),
+ DRM_MASTER),
VMW_IOCTL_DEF(VMW_CONTROL_STREAM, vmw_overlay_ioctl,
- DRM_MASTER | DRM_CONTROL_ALLOW),
+ DRM_MASTER),
VMW_IOCTL_DEF(VMW_CLAIM_STREAM, vmw_stream_claim_ioctl,
- DRM_MASTER | DRM_CONTROL_ALLOW),
+ DRM_MASTER),
VMW_IOCTL_DEF(VMW_UNREF_STREAM, vmw_stream_unref_ioctl,
- DRM_MASTER | DRM_CONTROL_ALLOW),
+ DRM_MASTER),
VMW_IOCTL_DEF(VMW_CREATE_CONTEXT, vmw_context_define_ioctl,
DRM_AUTH | DRM_RENDER_ALLOW),
Reviewed-by: Thomas Hellstrom
I can queue this on the next -fixes pull.
Through drm-misc-next would be simpler, assuming I get some review on
patches 1&4.
-Daniel
OK. I'll leave it out.
Thanks,
Thomas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/vmwgfx: Drop DRM_CONTROL_ALLOW
On 04/20/2018 08:51 AM, Daniel Vetter wrote:
Control nodes are no more!
Signed-off-by: Daniel Vetter
Cc: VMware Graphics
Cc: Sinclair Yeh
Cc: Thomas Hellstrom
---
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 70e1a8820a7c..97f37c3c16f2 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -159,14 +159,14 @@ static const struct drm_ioctl_desc vmw_ioctls[] = {
DRM_RENDER_ALLOW),
VMW_IOCTL_DEF(VMW_CURSOR_BYPASS,
vmw_kms_cursor_bypass_ioctl,
- DRM_MASTER | DRM_CONTROL_ALLOW),
+ DRM_MASTER),
VMW_IOCTL_DEF(VMW_CONTROL_STREAM, vmw_overlay_ioctl,
- DRM_MASTER | DRM_CONTROL_ALLOW),
+ DRM_MASTER),
VMW_IOCTL_DEF(VMW_CLAIM_STREAM, vmw_stream_claim_ioctl,
- DRM_MASTER | DRM_CONTROL_ALLOW),
+ DRM_MASTER),
VMW_IOCTL_DEF(VMW_UNREF_STREAM, vmw_stream_unref_ioctl,
- DRM_MASTER | DRM_CONTROL_ALLOW),
+ DRM_MASTER),
VMW_IOCTL_DEF(VMW_CREATE_CONTEXT, vmw_context_define_ioctl,
DRM_AUTH | DRM_RENDER_ALLOW),
Reviewed-by: Thomas Hellstrom
I can queue this on the next -fixes pull.
/Thomas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/7] drm/vmwgfx: Stop using plane->fb in vmw_kms_helper_dirty()
On 04/05/2018 09:50 PM, Ville Syrjala wrote:
From: Ville Syrjälä
Instead of plane->fb (which we're going to deprecate for atomic drivers)
we need to look at plane->state->fb. The maze of code leading to
vmw_kms_helper_dirty() wasn't particularly clear, but my analysis
concluded that the calls originating from vmw_*_primary_plane_atomic_update()
all pass in the crtc which means we'll never end up in this branch
of the function. All other callers use drm_modeset_lock_all() somewhere
higher up, which means accessing plane->state is safe. We'll toss in
a lockdep assert to catch wrongdoers.
Cc: Thomas Hellstrom
Cc: Sinclair Yeh
Cc: VMware Graphics
Cc: Daniel Vetter
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index a2a796b4cc23..5a824125c231 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -2326,9 +2326,18 @@ int vmw_kms_helper_dirty(struct vmw_private *dev_priv,
} else {
list_for_each_entry(crtc, &dev_priv->dev->mode_config.crtc_list,
head) {
- if (crtc->primary->fb != &framebuffer->base)
- continue;
- units[num_units++] = vmw_crtc_to_du(crtc);
+ struct drm_plane *plane = crtc->primary;
+
+ /*
+* vmw_*_primary_plane_atomic_update() pass in the crtc,
+* and so don't end up here. All other callers use
+* drm_modeset_lock_all(), hence we can access the
+* plane state safely.
+*/
+ lockdep_assert_held(&plane->mutex);
+
I think we can remove the comment (it's a helper, so current users may
not be future users),
but the lockdep assert should be OK.
+ if (plane->state->fb != &framebuffer->base)
+ units[num_units++] = vmw_crtc_to_du(crtc);
This doesn't seem to do what the original code did...
}
}
/Thomas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 12/23] drm/vmwgfx: Stop consulting plane->fb
On 03/22/2018 04:23 PM, Ville Syrjala wrote:
From: Ville Syrjälä
We want to get rid of plane->fb on atomic drivers. Stop looking at it.
Cc: VMware Graphics
Cc: Sinclair Yeh
Cc: Thomas Hellstrom
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 34ecc27fc30a..9fdb3ec9b4c4 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -385,9 +385,9 @@ vmw_du_cursor_plane_atomic_update(struct drm_plane *plane,
hotspot_x = du->hotspot_x;
hotspot_y = du->hotspot_y;
- if (plane->fb) {
- hotspot_x += plane->fb->hot_x;
- hotspot_y += plane->fb->hot_y;
+ if (plane->state->fb) {
+ hotspot_x += plane->state->fb->hot_x;
+ hotspot_y += plane->state->fb->hot_y;
}
du->cursor_surface = vps->surf;
LGTM.
Reviewed-by: Thomas Hellstrom
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Fix uabi regression by allowing garbage mode->type from userspace
On 03/21/2018 10:12 PM, Ville Syrjala wrote:
From: Ville Syrjälä
Apparently xf86-video-vmware leaves the mode->type uninitialized
when feeding the mode to the kernel. Thus we have no choice but
to accept the garbage in. We'll just ignore any of the bits we
don't want. The mode type is just a hint anyway, and more
useful for the kernel->userspace direction.
Reported-by: Thomas Hellstrom
CC: Thomas Hellstrom
Cc: Adam Jackson
Cc: Alex Deucher
Fixes: c6ed6dad5cfb ("drm/uapi: Validate the mode flags/type")
References:
https://lists.freedesktop.org/archives/dri-devel/2018-March/170213.html
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/drm_modes.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index f6b7c0e36a1a..e82b61e08f8c 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1611,7 +1611,13 @@ int drm_mode_convert_umode(struct drm_device *dev,
out->vscan = in->vscan;
out->vrefresh = in->vrefresh;
out->flags = in->flags;
- out->type = in->type;
+ /*
+* Old xf86-video-vmware (possibly others too) used to
+* leave 'type' unititialized. Just ignore any bits we
+* don't like. It's a just hint after all, and more
+* useful for the kernel->userspace direction anyway.
+*/
+ out->type = in->type & DRM_MODE_TYPE_ALL;
strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
Tested-by: Thomas Hellstrom
Thanks,
Thomas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] drm-next xf86-video-vmware breakage Was [PATCH 02/10] drm/uapi: Validate the mode flags/type
On 03/21/2018 09:51 PM, Ville Syrjälä wrote: On Wed, Mar 21, 2018 at 09:45:09PM +0100, Thomas Hellstrom wrote: Hi, Ville, On 11/14/2017 07:32 PM, Ville Syrjala wrote: From: Ville Syrjälä Currently userspace is allowed to feed in any king of garbage in the high bits of the mode flags/type, as are drivers when probing modes. Reject any mode with bogus flags/type. Hopefully this won't break any current userspace... Unfortunately this breaks xf86-video-vmware which currently leaves the mode->type uninitialized :(. That code is inherited from the old gallium xorg state tracker. I don't know whether it has spread elsewhere but the symptoms are that the X server frequently dies failing to set screen resources, a very uninformative error. I'll fix the xf86-video-vmware driver, but I guess we need to back out the mode->type check. Dang. So the flags check is fine but type check is not? Yes, we copy the flags field untranslated from xorg's DisplayModePtr::Flags field which seems to be what xf86-video-modesetting does as well, so I guess we should be OK there. /Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] drm-next xf86-video-vmware breakage Was [PATCH 02/10] drm/uapi: Validate the mode flags/type
Hi, Ville, On 11/14/2017 07:32 PM, Ville Syrjala wrote: From: Ville Syrjälä Currently userspace is allowed to feed in any king of garbage in the high bits of the mode flags/type, as are drivers when probing modes. Reject any mode with bogus flags/type. Hopefully this won't break any current userspace... Unfortunately this breaks xf86-video-vmware which currently leaves the mode->type uninitialized :(. That code is inherited from the old gallium xorg state tracker. I don't know whether it has spread elsewhere but the symptoms are that the X server frequently dies failing to set screen resources, a very uninformative error. I'll fix the xf86-video-vmware driver, but I guess we need to back out the mode->type check. /Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] linux-next: Signed-off-by missing for commit in the drm-misc-fixes tree
Hi, Stephen,
That would be me.
I've historically only added my signed-off-by:s on commits I've (co-)
authored myself, as the committer info is already registered,
But I take it that's not the correct approach?
/Thomas
On 01/18/2018 09:23 PM, Stephen Rothwell wrote:
Hi all,
Commit
8a510a5c7526 ("drm/vmwgfx: fix memory corruption with legacy/sou connectors")
is missing a Signed-off-by from its committer.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/8] drm: Nuke drm_atomic_helper_crtc_set_property
Acked-by: Thomas Hellstrom
On 07/25/2017 10:01 AM, Daniel Vetter wrote:
It's dead code because this is now handled in the core.
Signed-off-by: Daniel Vetter
Cc: Boris Brezillon
Cc: Daniel Vetter
Cc: Jani Nikula
Cc: Sean Paul
Cc: David Airlie
Cc: Ben Skeggs
Cc: Tomi Valkeinen
Cc: Laurent Pinchart
Cc: Alexey Brodkin
Cc: Shawn Guo
Cc: Eric Engestrom
Cc: Chris Wilson
Cc: "Ville Syrjälä"
Cc: Rob Clark
Cc: Philippe Cornu
Cc: Masahiro Yamada
Cc: Sushmita Susheelendra
Cc: Archit Taneja
Cc: intel-gfx@lists.freedesktop.org
Cc: nouv...@lists.freedesktop.org
Cc: Philipp Zabel
Cc: Maxime Ripard
Cc: Thomas Hellstrom
---
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 1 -
drivers/gpu/drm/drm_atomic_helper.c | 55 -
drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 1 -
drivers/gpu/drm/i915/intel_display.c| 1 -
drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c| 1 -
drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c| 2 -
drivers/gpu/drm/nouveau/nv50_display.c | 1 -
drivers/gpu/drm/omapdrm/omap_crtc.c | 1 -
include/drm/drm_atomic_helper.h | 3 --
9 files changed, 66 deletions(-)
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
index 4fbbeab5c5d4..d73281095fac 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
@@ -431,7 +431,6 @@ static const struct drm_crtc_funcs atmel_hlcdc_crtc_funcs =
{
.atomic_destroy_state = atmel_hlcdc_crtc_destroy_state,
.enable_vblank = atmel_hlcdc_crtc_enable_vblank,
.disable_vblank = atmel_hlcdc_crtc_disable_vblank,
- .set_property = drm_atomic_helper_crtc_set_property,
.gamma_set = drm_atomic_helper_legacy_gamma_set,
};
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 4a960c741e35..22245aa8b1aa 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2968,61 +2968,6 @@ int drm_atomic_helper_resume(struct drm_device *dev,
EXPORT_SYMBOL(drm_atomic_helper_resume);
/**
- * drm_atomic_helper_crtc_set_property - helper for crtc properties
- * @crtc: DRM crtc
- * @property: DRM property
- * @val: value of property
- *
- * Provides a default crtc set_property handler using the atomic driver
- * interface.
- *
- * RETURNS:
- * Zero on success, error code on failure
- */
-int
-drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
- struct drm_property *property,
- uint64_t val)
-{
- struct drm_atomic_state *state;
- struct drm_crtc_state *crtc_state;
- int ret = 0;
-
- state = drm_atomic_state_alloc(crtc->dev);
- if (!state)
- return -ENOMEM;
-
- /* ->set_property is always called with all locks held. */
- state->acquire_ctx = crtc->dev->mode_config.acquire_ctx;
-retry:
- crtc_state = drm_atomic_get_crtc_state(state, crtc);
- if (IS_ERR(crtc_state)) {
- ret = PTR_ERR(crtc_state);
- goto fail;
- }
-
- ret = drm_atomic_crtc_set_property(crtc, crtc_state,
- property, val);
- if (ret)
- goto fail;
-
- ret = drm_atomic_commit(state);
-fail:
- if (ret == -EDEADLK)
- goto backoff;
-
- drm_atomic_state_put(state);
- return ret;
-
-backoff:
- drm_atomic_state_clear(state);
- drm_atomic_legacy_backoff(state);
-
- goto retry;
-}
-EXPORT_SYMBOL(drm_atomic_helper_crtc_set_property);
-
-/**
* drm_atomic_helper_plane_set_property - helper for plane properties
* @plane: DRM plane
* @property: DRM property
diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
index 706efd0c4190..961551135a39 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
@@ -567,7 +567,6 @@ static const struct drm_crtc_funcs ade_crtc_funcs = {
.set_config = drm_atomic_helper_set_config,
.page_flip = drm_atomic_helper_page_flip,
.reset = drm_atomic_helper_crtc_reset,
- .set_property = drm_atomic_helper_crtc_set_property,
.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
.enable_vblank = ade_crtc_enable_vblank,
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index f7b128c33aa1..b4d0c5298a53 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12373,7 +12373,6 @@ static int intel_atomic_commit(struct drm_device *dev,
static const struct drm_crtc_funcs intel_crtc_funcs = {
.gamma_set = drm_
Re: [Intel-gfx] [PATCH 11/13] drm/vmwgfx: Drop drm_vblank_cleanup
With the below fixed,
Reviewed-by: Thomas Hellstrom
On 06/23/2017 04:10 PM, Sean Paul wrote:
On Wed, Jun 21, 2017 at 10:28:48AM +0200, Daniel Vetter wrote:
Again stopping the vblank before uninstalling the irq handler is kinda
the wrong way round, but the fb_off stuff should take care of
disabling the dsiplay at least in most cases. So drop the
drm_vblank_cleanup code since it's not really doing anything, it looks
all cargo-culted.
v2: Appease gcc better.
Cc: Sinclair Yeh
Cc: Thomas Hellstrom
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 9 +++--
drivers/gpu/drm/vmwgfx/vmwgfx_kms.h | 2 --
drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 4
drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 9 -
drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 27 +--
5 files changed, 4 insertions(+), 47 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 3d94ea67a825..a93c0bb73452 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -1658,7 +1658,7 @@ int vmw_kms_init(struct vmw_private *dev_priv)
int vmw_kms_close(struct vmw_private *dev_priv)
{
- int ret;
+ int ret = 0;
/*
* Docs says we should take the lock before calling this function
@@ -1666,11 +1666,8 @@ int vmw_kms_close(struct vmw_private *dev_priv)
* drm_encoder_cleanup which takes the lock we deadlock.
*/
drm_mode_config_cleanup(dev_priv->dev);
- if (dev_priv->active_display_unit == vmw_du_screen_object)
- ret = vmw_kms_sou_close_display(dev_priv);
- else if (dev_priv->active_display_unit == vmw_du_screen_target)
- ret = vmw_kms_stdu_close_display(dev_priv);
- else
+ if (dev_priv->active_display_unit != vmw_du_screen_object &&
+ dev_priv->active_display_unit != vmw_du_screen_target)
I think this is equivalent to:
if (dev_priv->active_display_unit == vmw_du_legacy)
ret = vmw_kms_ldu_close_display(dev_priv);
return ret;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
index 5f8d678ae675..ff9c8389ff21 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
@@ -390,7 +390,6 @@ int vmw_kms_update_proxy(struct vmw_resource *res,
* Screen Objects display functions - vmwgfx_scrn.c
*/
int vmw_kms_sou_init_display(struct vmw_private *dev_priv);
-int vmw_kms_sou_close_display(struct vmw_private *dev_priv);
int vmw_kms_sou_do_surface_dirty(struct vmw_private *dev_priv,
struct vmw_framebuffer *framebuffer,
struct drm_clip_rect *clips,
@@ -418,7 +417,6 @@ int vmw_kms_sou_readback(struct vmw_private *dev_priv,
* Screen Target Display Unit functions - vmwgfx_stdu.c
*/
int vmw_kms_stdu_init_display(struct vmw_private *dev_priv);
-int vmw_kms_stdu_close_display(struct vmw_private *dev_priv);
int vmw_kms_stdu_surface_dirty(struct vmw_private *dev_priv,
struct vmw_framebuffer *framebuffer,
struct drm_clip_rect *clips,
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
index d3987bcf53f8..449ed4fba0f2 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
@@ -582,13 +582,9 @@ int vmw_kms_ldu_init_display(struct vmw_private *dev_priv)
int vmw_kms_ldu_close_display(struct vmw_private *dev_priv)
{
- struct drm_device *dev = dev_priv->dev;
-
if (!dev_priv->ldu_priv)
return -ENOSYS;
- drm_vblank_cleanup(dev);
-
BUG_ON(!list_empty(&dev_priv->ldu_priv->active));
kfree(dev_priv->ldu_priv);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
index 8d7dc9def7c2..3b917c9b0c21 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
@@ -746,15 +746,6 @@ int vmw_kms_sou_init_display(struct vmw_private *dev_priv)
return 0;
}
-int vmw_kms_sou_close_display(struct vmw_private *dev_priv)
-{
- struct drm_device *dev = dev_priv->dev;
-
- drm_vblank_cleanup(dev);
-
- return 0;
-}
-
static int do_dmabuf_define_gmrfb(struct vmw_private *dev_priv,
struct vmw_framebuffer *framebuffer)
{
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
index 50be1f034f9e..6aecba6cd5e2 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
@@ -1651,36 +1651,11 @@ int vmw_kms_stdu_init_display(struct vmw_private
*dev_priv)
if (unlikely(ret != 0)) {
DRM_ERROR("Failed to initialize STDU %d", i);
- goto err_vblank_cleanup;
+
Re: [Intel-gfx] [PATCH 06/19] drm/vmwgfx: Drop the cursor locking hack
On 03/29/2017 10:00 AM, Daniel Vetter wrote: > On Mon, Mar 27, 2017 at 10:31:51AM +0200, Thomas Hellstrom wrote: >> On 03/27/2017 08:28 AM, Daniel Vetter wrote: >>> We discussed this quickly on irc, transcribing. >>> >>> On Mon, Mar 27, 2017 at 5:01 AM, Michel Dänzer wrote: >>>> Strictly speaking, the (virtual) hardware is too limited to support the >>>> legacy KMS cursor API. AFAIR e.g. weston at least used to make use of HW >>>> cursors for other surfaces, not sure that's currently the case though. >>> That was disabled again because of lack of atomic (together with all >>> overlay support if your driver isn't atomic). But atomic/universal >>> planes allows us to at least model vmwgfx correctly. For each crtc >>> we'd have one primary plane, but only one global cursor plane that we >>> attach to the cursor slot of each crtc. Then universal/atomic aware >>> userspace could realize that there's only 1 cursor plane and make sure >>> it's not over-used. >> That sounds encouraging. In practice we haven't really seen any problems >> because most users use vmware tools, >> which places the outputs in such a way that the cursor location visually >> coincides for all crtcs. >> The problem starts if someone would override tools and try to clone the >> contents across crtcs. >> The vmware xorg driver has some logic to try to detect such situations >> and fall back to software cursors, and possibly we might have to, at >> some point, implement software cursor composition in the kernel, but for >> now we live with the potential possibilty that users will see the cursor >> jumping across the screens.. > Ok, I've pulled in the series, except this patch plus the few cleanups > that depend upon it. I'll respin this as soon as vmwgfx atomic has landed, > with either a local mutex (if you still have more sw cursor planes than > real ones) or no changes (if your universal cursor code is fixed to only > have one cursor for the entire device instance). > > Thanks, Daniel Thanks, In the patch series we have added a local spinlock (cursor_lock) to protect from concurrent register access. /Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/19] drm/vmwgfx: Drop the cursor locking hack
On 03/27/2017 08:28 AM, Daniel Vetter wrote: > We discussed this quickly on irc, transcribing. > > On Mon, Mar 27, 2017 at 5:01 AM, Michel Dänzer wrote: >> Strictly speaking, the (virtual) hardware is too limited to support the >> legacy KMS cursor API. AFAIR e.g. weston at least used to make use of HW >> cursors for other surfaces, not sure that's currently the case though. > That was disabled again because of lack of atomic (together with all > overlay support if your driver isn't atomic). But atomic/universal > planes allows us to at least model vmwgfx correctly. For each crtc > we'd have one primary plane, but only one global cursor plane that we > attach to the cursor slot of each crtc. Then universal/atomic aware > userspace could realize that there's only 1 cursor plane and make sure > it's not over-used. That sounds encouraging. In practice we haven't really seen any problems because most users use vmware tools, which places the outputs in such a way that the cursor location visually coincides for all crtcs. The problem starts if someone would override tools and try to clone the contents across crtcs. The vmware xorg driver has some logic to try to detect such situations and fall back to software cursors, and possibly we might have to, at some point, implement software cursor composition in the kernel, but for now we live with the potential possibilty that users will see the cursor jumping across the screens.. /Thomas > -Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/19] drm/vmwgfx: Drop the cursor locking hack
On 03/23/2017 11:10 AM, Daniel Vetter wrote: > On Thu, Mar 23, 2017 at 09:35:25AM +0100, Thomas Hellstrom wrote: >> Hi, Daniel, >> >> On 03/23/2017 08:31 AM, Daniel Vetter wrote: >>> On Thu, Mar 23, 2017 at 08:28:32AM +0100, Daniel Vetter wrote: >>>> On Thu, Mar 23, 2017 at 07:22:31AM +0100, Thomas Hellstrom wrote: >>>>> On 03/22/2017 10:50 PM, Daniel Vetter wrote: >>>>>> It's been around forever, no one bothered to address the FIXME, so I >>>>>> presume it's all fine. >>>>>> >>>>>> Cc: Sinclair Yeh >>>>>> Cc: Thomas Hellstrom >>>>>> Signed-off-by: Daniel Vetter >>>>> NAK. We need to properly address this. Probably as part of the atomic >>>>> update. >>>> So could someone with vmwgfx understanding explain this? Note that the >>>> FIXME was originally added by me years ago, because I wasn't sure (only >>>> about 90%) that this is safe, and was essentially pleading for a vmwgfx >>>> expert to review this? >>>> >>>> Since it didn't happen I presume it's not that terribly and probably safe >>>> ... >>>> >>>> I'm still 90% sure that this is correct, but I'd love for a vmwgfx to >>>> audit it. Replying with a NAK is kinda not the response I was hoping for >>>> (and yes I guess I should have explained what's going on here better, but >>>> it's just a git blame of the FIXME comment away). >> So the code has been left in place because it works. Altering it now >> will create unnecessary merge conflicts with the atomic code, and the >> change isn't tested and audited which means we need to drop focus from >> what we're doing and audit and test code that isn't going to be used >> anyway for not apparent reason? But otoh put in the below context there >> indeed is a reason. >> >> From a quick audit of the existing code it seems like at least >> vmw_cursor_update_position is touching global device state so I think at >> a minimum we need to take a spinlock in that function. Otherwise it >> seems to be safe. > Note that you're holding the crtc lock already, which gives you exclusion > against concurrent page_flips, mode_sets and property changes. Note also > that page_flips themselves also only hold the crtc lock, so you can run > multiple page_flips in parallel on different crtc (iirc vmwgfx has > multiple crtc, if not this discussion is entirely moot). > > tbh I'd be surprised if my patch really breaks something that hasn't been > a pre-existing issue for a long time. The original commit which added this > FIXME comment is from 2012. Note also that because it's a hack, you > already have a pretty a real race with the core drm state keeping, and no > one seems to have hit that either. > > I mean I can dig through vmwgfx code and do the audit, but it'll take a > few hours and vmwgfx is it's own world, so much harder to understand (for > me). > I'm thinking of the situation when someone would call a cursor_set ioctl in parallell for two crtcs at the same time and race writing the position registers? Note that the device has only a single global cursor. Admittedly the effects of a race would probably be small, but I'd rather see it being properly protected. >> But I prefer if we can do that as part of the atomic update? > When does that vmwgfx atomic happen? > -Daniel We're targeting 4.12, which means the code that is currently under testing will need to be sent out for review pretty soon. It's already in our standalone testing repo at git://git.freedesktop.org/git/mesa/vmwgfx but the cursor code hasn't been fixed in that repo yet. BTW is this blocking some other core drm work you're doing? Thanks, /Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/19] drm/vmwgfx: Drop the cursor locking hack
Hi, Daniel, On 03/23/2017 08:31 AM, Daniel Vetter wrote: > On Thu, Mar 23, 2017 at 08:28:32AM +0100, Daniel Vetter wrote: >> On Thu, Mar 23, 2017 at 07:22:31AM +0100, Thomas Hellstrom wrote: >>> On 03/22/2017 10:50 PM, Daniel Vetter wrote: >>>> It's been around forever, no one bothered to address the FIXME, so I >>>> presume it's all fine. >>>> >>>> Cc: Sinclair Yeh >>>> Cc: Thomas Hellstrom >>>> Signed-off-by: Daniel Vetter >>> NAK. We need to properly address this. Probably as part of the atomic >>> update. >> So could someone with vmwgfx understanding explain this? Note that the >> FIXME was originally added by me years ago, because I wasn't sure (only >> about 90%) that this is safe, and was essentially pleading for a vmwgfx >> expert to review this? >> >> Since it didn't happen I presume it's not that terribly and probably safe >> ... >> >> I'm still 90% sure that this is correct, but I'd love for a vmwgfx to >> audit it. Replying with a NAK is kinda not the response I was hoping for >> (and yes I guess I should have explained what's going on here better, but >> it's just a git blame of the FIXME comment away). So the code has been left in place because it works. Altering it now will create unnecessary merge conflicts with the atomic code, and the change isn't tested and audited which means we need to drop focus from what we're doing and audit and test code that isn't going to be used anyway for not apparent reason? But otoh put in the below context there indeed is a reason. From a quick audit of the existing code it seems like at least vmw_cursor_update_position is touching global device state so I think at a minimum we need to take a spinlock in that function. Otherwise it seems to be safe. But I prefer if we can do that as part of the atomic update? Thanks, Thomas > Bit more context even: This lock dropping dance is _not_ safe from a drm > core perspective. But when I've done the original kms locking rework the > tradeoff between upsetting core state a bit and totally breaking vmwgfx > leaned towards not breaking vmwgfx. And iirc you or Syeh promised to look > at this and then either remove the FIXME, maybe with a vmwgfx lock/unlock > added if there's a gap (I looked, didn't find one, but I don't understand > vmwgfx in details really). > > Thanks, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/19] drm/vmwgfx: Drop the cursor locking hack
On 03/22/2017 10:50 PM, Daniel Vetter wrote: > It's been around forever, no one bothered to address the FIXME, so I > presume it's all fine. > > Cc: Sinclair Yeh > Cc: Thomas Hellstrom > Signed-off-by: Daniel Vetter NAK. We need to properly address this. Probably as part of the atomic update. /Thomas > --- > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 25 - > 1 file changed, 25 deletions(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > index d492d57d5309..424b3fc57203 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > @@ -148,15 +148,6 @@ int vmw_du_crtc_cursor_set2(struct drm_crtc *crtc, > struct drm_file *file_priv, > s32 hotspot_x, hotspot_y; > int ret; > > - /* > - * FIXME: Unclear whether there's any global state touched by the > - * cursor_set function, especially vmw_cursor_update_position looks > - * suspicious. For now take the easy route and reacquire all locks. We > - * can do this since the caller in the drm core doesn't check anything > - * which is protected by any looks. > - */ > - drm_modeset_unlock_crtc(crtc); > - drm_modeset_lock_all(dev_priv->dev); > hotspot_x = hot_x + du->hotspot_x; > hotspot_y = hot_y + du->hotspot_y; > > @@ -224,9 +215,6 @@ int vmw_du_crtc_cursor_set2(struct drm_crtc *crtc, struct > drm_file *file_priv, > } > > out: > - drm_modeset_unlock_all(dev_priv->dev); > - drm_modeset_lock_crtc(crtc, crtc->cursor); > - > return ret; > } > > @@ -239,25 +227,12 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int > x, int y) > du->cursor_x = x + du->set_gui_x; > du->cursor_y = y + du->set_gui_y; > > - /* > - * FIXME: Unclear whether there's any global state touched by the > - * cursor_set function, especially vmw_cursor_update_position looks > - * suspicious. For now take the easy route and reacquire all locks. We > - * can do this since the caller in the drm core doesn't check anything > - * which is protected by any looks. > - */ > - drm_modeset_unlock_crtc(crtc); > - drm_modeset_lock_all(dev_priv->dev); > - > vmw_cursor_update_position(dev_priv, shown, > du->cursor_x + du->hotspot_x + > du->core_hotspot_x, > du->cursor_y + du->hotspot_y + > du->core_hotspot_y); > > - drm_modeset_unlock_all(dev_priv->dev); > - drm_modeset_lock_crtc(crtc, crtc->cursor); > - > return 0; > } > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] [RFC] drm: Nerf DRM_CONTROL nodes
On 02/20/2017 11:22 PM, Daniel Vetter wrote: > On Sun, Feb 19, 2017 at 4:21 PM, Thomas Hellstrom wrote: >> So I think we need a quick revert of this commit or a quick stable >> follow-up to unbreak things on our side. > I'd much prefer we just register control nodes for vmwgfx only, with a > commit message explaining in detail what exactly your control tool is > using (i.e. which ioctl), plus links to the source code for future > references. Also not sold on the immediate revert, this stuff has been > merged since months. > -Daniel > https://cgit.freedesktop.org/~thomash/open-vm-tools/commit/?h=feature/thellstrom/resolutionKMS&id=9bf65a22d5a06d3a706bc14578619a56e06f8a24 /Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] DRM_CONTROL node breakage (Re: [PATCH] [RFC] drm: Nerf DRM_CONTROL nodes)
On 02/21/2017 06:34 AM, David Airlie wrote: >> No. >> >> IMO Not fixing this immediately through stable is out of the question. >> The deal is that we don't break userspace. >> Having said that, I'm not against a long term vmwgfx-only solution. But >> let's fix this now. >> >> Admittedly we missed testing this but you got to understand that not all >> developer teams have a multitude of >> developers (we have on average one for the whole linux graphics driver >> stack except GL), and the bug >> doesn't show up for QE on regression testing unless they run >> gnome-sheel/Wayland which they currently don't, and I guess they've been >> focused on the fb2 regression. >> >> It's no secret that we've been using the control nodes for some time. >> The CONTROL_ALLOW is present in the >> driver private ioctls and the commit has been there since 2016. >> >> The user-space code has been present in vmware-tools also since that >> commit and due to the long release cycles of >> open-vm-tools the open-vm-tools version was just about to be released. >> It's necessary for non-xorg > can you send a revert against drm-next? I'm not sure how clean it will be. > > there might be an intermediate step. > > Then can we port vmtools of this behaviour, not even sure what it is doing. > > Dave. So after a quick investigation of the impact it looks like the daemon patch was pulled out of the Fedora open-vm-tools update in time. This limits the impact to within VMware where we can update the daemon code and rerun the test cycle. I've posted a patch that makes it possible for us to use render-nodes instead of control nodes. /Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] DRM_CONTROL node breakage (Re: [PATCH] [RFC] drm: Nerf DRM_CONTROL nodes)
No.
IMO Not fixing this immediately through stable is out of the question.
The deal is that we don't break userspace.
Having said that, I'm not against a long term vmwgfx-only solution. But
let's fix this now.
Admittedly we missed testing this but you got to understand that not all
developer teams have a multitude of
developers (we have on average one for the whole linux graphics driver
stack except GL), and the bug
doesn't show up for QE on regression testing unless they run
gnome-sheel/Wayland which they currently don't, and I guess they've been
focused on the fb2 regression.
It's no secret that we've been using the control nodes for some time.
The CONTROL_ALLOW is present in the
driver private ioctls and the commit has been there since 2016.
The user-space code has been present in vmware-tools also since that
commit and due to the long release cycles of
open-vm-tools the open-vm-tools version was just about to be released.
It's necessary for non-xorg
/Thomas
On 02/20/2017 11:22 PM, Daniel Vetter wrote:
> On Sun, Feb 19, 2017 at 4:21 PM, Thomas Hellstrom wrote:
>> So I think we need a quick revert of this commit or a quick stable
>> follow-up to unbreak things on our side.
> I'd much prefer we just register control nodes for vmwgfx only, with a
> commit message explaining in detail what exactly your control tool is
> using (i.e. which ioctl), plus links to the source code for future
> references. Also not sold on the immediate revert, this stuff has been
> merged since months.
> -Daniel
>
>> /Thomas
>>
>>
>> On 02/19/2017 03:54 PM, Thomas Hellstrom wrote:
>>> Hi!
>>>
>>> This patch breaks the vmwgfx resolutionKMS daemon which opens a control
>>> node to tell DRM about the monitor layout...
>>>
>>> /Thomas
>>>
>>>
>>> On 10/28/2016 10:10 AM, Daniel Vetter wrote:
>>>> Looking at the ioctl permission checks I noticed that it's impossible
>>>> to import gem buffers into a control nodes, and fd2handle/handle2fd
>>>> also don't work, so no joy with dma-bufs.
>>>>
>>>> The only way to do anything with a control node is by drawing stuff
>>>> into a dumb buffer and displaying that. I suspect control nodes are an
>>>> entirely unused thing, and a cursory check shows that there does not
>>>> seem to be any callers of drmOpenControl nor of the other drmOpen
>>>> functions using DRM_MODE_CONTROL.
>>>>
>>>> Since I don't like dead uabi, let's remove it. But since this would be
>>>> a really big change I think it's better to start out small by simply
>>>> not registering anything. We can garbage-collect the dead code later
>>>> on, once we're sure it's really not used anywhere.
>>>>
>>>> Signed-off-by: Daniel Vetter
>>>> ---
>>>> drivers/gpu/drm/drm_drv.c | 6 --
>>>> 1 file changed, 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>>> index 6efdba4993fc..f085e28ffc6f 100644
>>>> --- a/drivers/gpu/drm/drm_drv.c
>>>> +++ b/drivers/gpu/drm/drm_drv.c
>>>> @@ -517,12 +517,6 @@ int drm_dev_init(struct drm_device *dev,
>>>> goto err_free;
>>>> }
>>>>
>>>> -if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>>>> -ret = drm_minor_alloc(dev, DRM_MINOR_CONTROL);
>>>> -if (ret)
>>>> -goto err_minors;
>>>> -}
>>>> -
>>>> if (drm_core_check_feature(dev, DRIVER_RENDER)) {
>>>> ret = drm_minor_alloc(dev, DRM_MINOR_RENDER);
>>>> if (ret)
>>> ___
>>> dri-devel mailing list
>>> dri-de...@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] [RFC] drm: Nerf DRM_CONTROL nodes
So I think we need a quick revert of this commit or a quick stable
follow-up to unbreak things on our side.
/Thomas
On 02/19/2017 03:54 PM, Thomas Hellstrom wrote:
> Hi!
>
> This patch breaks the vmwgfx resolutionKMS daemon which opens a control
> node to tell DRM about the monitor layout...
>
> /Thomas
>
>
> On 10/28/2016 10:10 AM, Daniel Vetter wrote:
>> Looking at the ioctl permission checks I noticed that it's impossible
>> to import gem buffers into a control nodes, and fd2handle/handle2fd
>> also don't work, so no joy with dma-bufs.
>>
>> The only way to do anything with a control node is by drawing stuff
>> into a dumb buffer and displaying that. I suspect control nodes are an
>> entirely unused thing, and a cursory check shows that there does not
>> seem to be any callers of drmOpenControl nor of the other drmOpen
>> functions using DRM_MODE_CONTROL.
>>
>> Since I don't like dead uabi, let's remove it. But since this would be
>> a really big change I think it's better to start out small by simply
>> not registering anything. We can garbage-collect the dead code later
>> on, once we're sure it's really not used anywhere.
>>
>> Signed-off-by: Daniel Vetter
>> ---
>> drivers/gpu/drm/drm_drv.c | 6 --
>> 1 file changed, 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 6efdba4993fc..f085e28ffc6f 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -517,12 +517,6 @@ int drm_dev_init(struct drm_device *dev,
>> goto err_free;
>> }
>>
>> -if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>> -ret = drm_minor_alloc(dev, DRM_MINOR_CONTROL);
>> -if (ret)
>> -goto err_minors;
>> -}
>> -
>> if (drm_core_check_feature(dev, DRIVER_RENDER)) {
>> ret = drm_minor_alloc(dev, DRM_MINOR_RENDER);
>> if (ret)
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] [RFC] drm: Nerf DRM_CONTROL nodes
Hi!
This patch breaks the vmwgfx resolutionKMS daemon which opens a control
node to tell DRM about the monitor layout...
/Thomas
On 10/28/2016 10:10 AM, Daniel Vetter wrote:
> Looking at the ioctl permission checks I noticed that it's impossible
> to import gem buffers into a control nodes, and fd2handle/handle2fd
> also don't work, so no joy with dma-bufs.
>
> The only way to do anything with a control node is by drawing stuff
> into a dumb buffer and displaying that. I suspect control nodes are an
> entirely unused thing, and a cursory check shows that there does not
> seem to be any callers of drmOpenControl nor of the other drmOpen
> functions using DRM_MODE_CONTROL.
>
> Since I don't like dead uabi, let's remove it. But since this would be
> a really big change I think it's better to start out small by simply
> not registering anything. We can garbage-collect the dead code later
> on, once we're sure it's really not used anywhere.
>
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_drv.c | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 6efdba4993fc..f085e28ffc6f 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -517,12 +517,6 @@ int drm_dev_init(struct drm_device *dev,
> goto err_free;
> }
>
> - if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> - ret = drm_minor_alloc(dev, DRM_MINOR_CONTROL);
> - if (ret)
> - goto err_minors;
> - }
> -
> if (drm_core_check_feature(dev, DRIVER_RENDER)) {
> ret = drm_minor_alloc(dev, DRM_MINOR_RENDER);
> if (ret)
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm: Resurrect atomic rmfb code, v2
On 01/25/2017 05:54 AM, Daniel Vetter wrote:
> On Tue, Jan 24, 2017 at 01:44:54PM -0800, Matt Roper wrote:
>> On Wed, Jan 11, 2017 at 05:15:47PM +0100, Daniel Vetter wrote:
>>> On Thu, Dec 15, 2016 at 03:29:45PM +0100, Maarten Lankhorst wrote:
From: Daniel Vetter
This was somehow lost between v3 and the merged version in Maarten's
patch merged as:
commit f2d580b9a8149735cbc4b59c4a8df60173658140
Author: Maarten Lankhorst
Date: Wed May 4 14:38:26 2016 +0200
drm/core: Do not preserve framebuffer on rmfb, v4.
Actual code copied from Maarten's patch, but with the slight change to
just use dev->mode_config.funcs->atomic_commit to decide whether to
use the atomic path or not.
v2:
- Remove plane->fb assignment, done by drm_atomic_clean_old_fb.
- Add WARN_ON when atomic_remove_fb fails.
- Always call drm_atomic_state_put.
Signed-off-by: Daniel Vetter
Signed-off-by: Daniel Vetter
Signed-off-by: Maarten Lankhorst
>>> Would be great if someone else could r-b this, I've proven pretty well
>>> that I don't understand the complexity here :(
>>> -Daniel
>> It looks like this will change the behavior slightly in that rmfb will
>> cause primary planes to be disabled, but no longer cause the entire CRTC
>> to be turned off. You'll probably want to note that in the commit
>> message, along with the justification on why this is okay ABI-wise.
>>
>> I know that 13803132818c ("drm/core: Preserve the framebuffer after
>> removing it.") was initially trying to not only leave the CRTC on, but
>> also preserve the framebuffer and leave the planes on; that wound up
>> causing some kind of regression for vmwgfx, but I'm unclear on the
>> details there. I'd suggest getting an Ack from one of the vmware guys
>> to ensure that the less drastic change in behavior here won't cause them
>> any problems.
The vmware Xorg driver is currently relying on rmfb to turn all attached
crtcs off. Even if we were to fix that in the Xorg driver now, older
Xorgs with newer kernels still would break.
/Thomas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: move allocation out of drm_get_format_name()
For the vmwgfx part:
Acked-by: Thomas Hellstrom
On 11/05/2016 08:33 AM, Eric Engestrom wrote:
> Fixes: 90844f00049e9f42573fd31d7c32e8fd31d3fd07
>
> drm: make drm_get_format_name thread-safe
>
> Signed-off-by: Eric Engestrom
> [danvet: Clarify that the returned pointer must be freed with
> kfree().]
> Signed-off-by: Daniel Vetter
>
> Suggested-by: Ville Syrjälä
> Signed-off-by: Eric Engestrom
> ---
> drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 7 ++---
> drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 7 ++---
> drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 3 +-
> drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 7 ++---
> drivers/gpu/drm/drm_atomic.c| 7 +++--
> drivers/gpu/drm/drm_crtc.c | 7 +++--
> drivers/gpu/drm/drm_fourcc.c| 12 +++-
> drivers/gpu/drm/drm_framebuffer.c | 7 +++--
> drivers/gpu/drm/drm_modeset_helper.c| 7 +++--
> drivers/gpu/drm/drm_plane.c | 7 +++--
> drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 7 ++---
> drivers/gpu/drm/i915/i915_debugfs.c | 8 ++---
> drivers/gpu/drm/i915/intel_atomic_plane.c | 8 ++---
> drivers/gpu/drm/i915/intel_display.c| 41
> ++---
> drivers/gpu/drm/radeon/atombios_crtc.c | 14 -
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 3 +-
> include/drm/drm_fourcc.h| 3 +-
> 17 files changed, 71 insertions(+), 84 deletions(-)
>
> diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> index dc0aafa..5a8cb4b 100644
> --- a/include/drm/drm_fourcc.h
> +++ b/include/drm/drm_fourcc.h
> @@ -54,6 +54,7 @@ int drm_format_horz_chroma_subsampling(uint32_t format);
> int drm_format_vert_chroma_subsampling(uint32_t format);
> int drm_format_plane_width(int width, uint32_t format, int plane);
> int drm_format_plane_height(int height, uint32_t format, int plane);
> -char *drm_get_format_name(uint32_t format) __malloc;
> +typedef char drm_format_name_buf[32];
> +char *drm_get_format_name(uint32_t format, drm_format_name_buf buf);
>
> #endif /* __DRM_FOURCC_H__ */
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index cbb8b77..34ed520 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -79,17 +79,13 @@ uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t
> depth)
> EXPORT_SYMBOL(drm_mode_legacy_fb_format);
>
> /**
> - * drm_get_format_name - return a string for drm fourcc format
> + * drm_get_format_name - fill a string with a drm fourcc format's name
> * @format: format to compute name of
> + * @buf: caller-supplied buffer
> - *
> - * Note that the buffer returned by this function is owned by the caller
> - * and will need to be freed using kfree().
> */
> -char *drm_get_format_name(uint32_t format)
> +char *drm_get_format_name(uint32_t format, drm_format_name_buf buf)
> {
> - char *buf = kmalloc(32, GFP_KERNEL);
> -
> - snprintf(buf, 32,
> + snprintf(buf, sizeof(drm_format_name_buf),
>"%c%c%c%c %s-endian (0x%08x)",
>printable_char(format & 0xff),
>printable_char((format >> 8) & 0xff),
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> index 199d3f7..cefa3d8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> @@ -2032,7 +2032,7 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc
> *crtc,
> u32 tmp, viewport_w, viewport_h;
> int r;
> bool bypass_lut = false;
> - char *format_name;
> + drm_format_name_buf format_name;
>
> /* no fb bound */
> if (!atomic && !crtc->primary->fb) {
> @@ -2144,9 +2144,8 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc
> *crtc,
> bypass_lut = true;
> break;
> default:
> - format_name = drm_get_format_name(target_fb->pixel_format);
> - DRM_ERROR("Unsupported screen format %s\n", format_name);
> - kfree(format_name);
> + DRM_ERROR("Unsupported screen format %s\n",
> + drm_get_format_name(target_fb->pixel_format,
> format_name));
> return -EINVAL;
> }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> index ecd000e..462abb8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> @@ -2
Re: [Intel-gfx] [PATCH 09/20] drm/kms: Nuke dirty_info property
On 08/09/2016 04:08 PM, Daniel Vetter wrote:
> On Tue, Aug 9, 2016 at 3:59 PM, Thomas Hellstrom
> wrote:
>> Hmm.
>>
>> I don't have a strong opinion on this, but IMO the original idea of
>> allowing a user-space driver to optimize away the dirty calls isn't a
>> bad one.
>> Since the xf86-video-vmware always assumed that all drms it has to deal
>> with has this property set it's not using it.
>> The modesetting driver could, otoh benefit from this if someone cared to
>> implement it.
>>
>> What about newer compositors? All using pageflips?
> Fully agree that it's a nice idea, but given that most of the drivers
> with a non-nop ->dirty callback don't set generic userspace can't rely
> on it at all. And specific userspace (vmware is the only one I've
> found) also ignores it. ABI without users doesn't server a purpose,
> hence I'd like to remove it.
So if DRM drivers with a non-nop dirty callback doesn't set this
property then I agree it should be removed and
replaced with something else once someone cares to implement user-space
code for it.
Acked-by: Thomas Hellstrom
>
> Of course totally not against merging this again, if someone bothers
> to fix up the bugs on the kernel side (we need consistency with
> advertising it on drivers that do have a dirty callback) and
> implements the corresponding userspace.
>
>> Also at this point I'm not aware of any user of this property but the
>> assumption that any user of DRM must be open-source to not be broken by
>> a kernel update seems dangerous to me. I though we must remain backwards
>> compatible always, not only for open-source user-space?
> I routinely break the blobs running on top of i915.ko and get away
> with it ;-) And imo this is just a consequence of requiring
> open-source userspace for any uabi - if it breaks that open-souce
> userspace then we'll fix it, no questions asked. If it breaks no
> open-source user at all, you've cheated when upstreaming the interface
> originally, and you get to keep all the pieces.
> -Daniel
>
>
>> Thomas
>>
>>
>> On 08/09/2016 03:41 PM, Daniel Vetter wrote:
>>> It was added way back together with the dirty_fb ioctl, but neither
>>> generic xfree86-modesetting nor the vmware driver use it. Everyone is
>>> supposed to just unconditionally call the dirtyfb when they do
>>> frontbuffer rendering.
>>>
>>> And since unused uabi is bad uabi (there's reasons we require open
>>> source userspace for everything) let's nuke this.
>>>
>>> For reference see
>>>
>>> commit 884840aa3ce3214259e69557be5b4ce0d781ffa4
>>> Author: Jakob Bornecrantz
>>> Date: Thu Dec 3 23:25:47 2009 +
>>>
>>> drm: Add dirty ioctl and property
>>>
>>> Cc: Jakob Bornecrantz
>>> Cc: Dave Airlie
>>> Cc: Sinclair Yeh
>>> Cc: Thomas Hellstrom
>>> Signed-off-by: Daniel Vetter
>>> ---
>>> drivers/gpu/drm/drm_crtc.c | 31 ---
>>> drivers/gpu/drm/udl/udl_connector.c | 3 ---
>>> drivers/gpu/drm/udl/udl_modeset.c| 2 --
>>> drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 9 -
>>> drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 11 ---
>>> drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 7 ---
>>> include/drm/drm_crtc.h | 7 ---
>>> 7 files changed, 70 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>>> index ad38a8a31898..deed2e2c8cf1 100644
>>> --- a/drivers/gpu/drm/drm_crtc.c
>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>> @@ -136,12 +136,6 @@ static const struct drm_prop_enum_list
>>> drm_tv_subconnector_enum_list[] = {
>>> DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
>>>drm_tv_subconnector_enum_list)
>>>
>>> -static const struct drm_prop_enum_list drm_dirty_info_enum_list[] = {
>>> - { DRM_MODE_DIRTY_OFF, "Off" },
>>> - { DRM_MODE_DIRTY_ON, "On" },
>>> - { DRM_MODE_DIRTY_ANNOTATE, "Annotate" },
>>> -};
>>> -
>>> struct drm_conn_prop_enum_list {
>>> int type;
>>> const char *name;
>>> @@ -1894,31 +1888,6 @@ int drm_mode_create_aspect_ratio_property(struct
>>> drm_device *dev)
>>> EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property);
>>>
>>> /**
>>> - * drm_mode_create_dirty_propert
Re: [Intel-gfx] [PATCH 09/20] drm/kms: Nuke dirty_info property
Hmm.
I don't have a strong opinion on this, but IMO the original idea of
allowing a user-space driver to optimize away the dirty calls isn't a
bad one.
Since the xf86-video-vmware always assumed that all drms it has to deal
with has this property set it's not using it.
The modesetting driver could, otoh benefit from this if someone cared to
implement it.
What about newer compositors? All using pageflips?
Also at this point I'm not aware of any user of this property but the
assumption that any user of DRM must be open-source to not be broken by
a kernel update seems dangerous to me. I though we must remain backwards
compatible always, not only for open-source user-space?
Thomas
On 08/09/2016 03:41 PM, Daniel Vetter wrote:
> It was added way back together with the dirty_fb ioctl, but neither
> generic xfree86-modesetting nor the vmware driver use it. Everyone is
> supposed to just unconditionally call the dirtyfb when they do
> frontbuffer rendering.
>
> And since unused uabi is bad uabi (there's reasons we require open
> source userspace for everything) let's nuke this.
>
> For reference see
>
> commit 884840aa3ce3214259e69557be5b4ce0d781ffa4
> Author: Jakob Bornecrantz
> Date: Thu Dec 3 23:25:47 2009 +
>
> drm: Add dirty ioctl and property
>
> Cc: Jakob Bornecrantz
> Cc: Dave Airlie
> Cc: Sinclair Yeh
> Cc: Thomas Hellstrom
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_crtc.c | 31 ---
> drivers/gpu/drm/udl/udl_connector.c | 3 ---
> drivers/gpu/drm/udl/udl_modeset.c| 2 --
> drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 9 -
> drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 11 ---
> drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 7 ---
> include/drm/drm_crtc.h | 7 ---
> 7 files changed, 70 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index ad38a8a31898..deed2e2c8cf1 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -136,12 +136,6 @@ static const struct drm_prop_enum_list
> drm_tv_subconnector_enum_list[] = {
> DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
>drm_tv_subconnector_enum_list)
>
> -static const struct drm_prop_enum_list drm_dirty_info_enum_list[] = {
> - { DRM_MODE_DIRTY_OFF, "Off" },
> - { DRM_MODE_DIRTY_ON, "On" },
> - { DRM_MODE_DIRTY_ANNOTATE, "Annotate" },
> -};
> -
> struct drm_conn_prop_enum_list {
> int type;
> const char *name;
> @@ -1894,31 +1888,6 @@ int drm_mode_create_aspect_ratio_property(struct
> drm_device *dev)
> EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property);
>
> /**
> - * drm_mode_create_dirty_property - create dirty property
> - * @dev: DRM device
> - *
> - * Called by a driver the first time it's needed, must be attached to desired
> - * connectors.
> - */
> -int drm_mode_create_dirty_info_property(struct drm_device *dev)
> -{
> - struct drm_property *dirty_info;
> -
> - if (dev->mode_config.dirty_info_property)
> - return 0;
> -
> - dirty_info =
> - drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE,
> - "dirty",
> - drm_dirty_info_enum_list,
> - ARRAY_SIZE(drm_dirty_info_enum_list));
> - dev->mode_config.dirty_info_property = dirty_info;
> -
> - return 0;
> -}
> -EXPORT_SYMBOL(drm_mode_create_dirty_info_property);
> -
> -/**
> * drm_mode_create_suggested_offset_properties - create suggests offset
> properties
> * @dev: DRM device
> *
> diff --git a/drivers/gpu/drm/udl/udl_connector.c
> b/drivers/gpu/drm/udl/udl_connector.c
> index 4709b54c204c..d2f57c52f7db 100644
> --- a/drivers/gpu/drm/udl/udl_connector.c
> +++ b/drivers/gpu/drm/udl/udl_connector.c
> @@ -150,8 +150,5 @@ int udl_connector_init(struct drm_device *dev, struct
> drm_encoder *encoder)
> drm_connector_register(connector);
> drm_mode_connector_attach_encoder(connector, encoder);
>
> - drm_object_attach_property(&connector->base,
> - dev->mode_config.dirty_info_property,
> - 1);
> return 0;
> }
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c
> b/drivers/gpu/drm/udl/udl_modeset.c
> index f92ea9579674..73695127c573 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -441,8 +441,6 @@ int udl_modeset_init(struct drm_device *dev)
>
> dev->mode_config.funcs = &udl_m
Re: [Intel-gfx] [PATCH] drm/core: Do not preserve framebuffer on rmfb, v3.
Hi,
Sorry for the late response, been very busy with other stuff lately.
I've tested this version against drm-fixes and it indeed fixes the
problem, as far as I can tell.
Tested-by: Thomas Hellstrom
On 03/31/2016 01:26 PM, Maarten Lankhorst wrote:
> It turns out that preserving framebuffers after the rmfb call breaks
> vmwgfx userspace. This was originally introduced because it was thought
> nobody relied on the behavior, but unfortunately it seems there are
> exceptions.
>
> drm_framebuffer_remove may fail with -EINTR now, so a straight revert
> is impossible. There is no way to remove the framebuffer from the lists
> and active planes without introducing a race because of the different
> locking requirements. Instead call drm_framebuffer_remove from a
> workqueue, which is unaffected by signals.
>
> Changes since v1:
> - Add comment.
> Changes since v2:
> - Add fastpath for refcount = 1. (danvet)
>
> Cc: sta...@vger.kernel.org #v4.4+
> Fixes: 13803132818c ("drm/core: Preserve the framebuffer after removing it.")
> Testcase: kms_flip.flip-vs-rmfb-interruptible
> References:
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_archives_dri-2Ddevel_2016-2DMarch_102876.html&d=BQIBAg&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=vpukPkBtpoNQp2IUKuFviOmPNYWVKmen3Jeeu55zmEA&m=_2qOX1NGnSnJOTgqvu1Ud574i5T3fLDlX91oUS3WXXI&s=9D34PFYdb1PT2vzX_M_7lNVoSebfM9-KsAqe5AXAQbQ&e=
>
> Cc: Thomas Hellstrom
> Cc: David Herrmann
> ---
> drivers/gpu/drm/drm_crtc.c | 29 -
> 1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 55ffde5a3a4a..743bece1f579 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3434,6 +3434,18 @@ int drm_mode_addfb2(struct drm_device *dev,
> return 0;
> }
>
> +struct drm_mode_rmfb_work {
> + struct work_struct work;
> + struct drm_framebuffer *fb;
> +};
> +
> +static void drm_mode_rmfb_work_fn(struct work_struct *w)
> +{
> + struct drm_mode_rmfb_work *arg = container_of(w, typeof(*arg), work);
> +
> + drm_framebuffer_remove(arg->fb);
> +}
> +
> /**
> * drm_mode_rmfb - remove an FB from the configuration
> * @dev: drm device for the ioctl
> @@ -3474,7 +3486,22 @@ int drm_mode_rmfb(struct drm_device *dev,
> mutex_unlock(&dev->mode_config.fb_lock);
> mutex_unlock(&file_priv->fbs_lock);
>
> - drm_framebuffer_unreference(fb);
> + /*
> + * drm_framebuffer_remove may fail with -EINTR on pending signals,
> + * so run this in a separate stack as there's no way to correctly
> + * handle this after the fb is already removed from the lookup table.
> + */
> + if (atomic_read(&fb->refcount.refcount) > 1) {
> + struct drm_mode_rmfb_work arg;
> +
> + INIT_WORK_ONSTACK(&arg.work, drm_mode_rmfb_work_fn);
> + arg.fb = fb;
> +
> + schedule_work(&arg.work);
> + flush_work(&arg.work);
> + destroy_work_on_stack(&arg.work);
> + } else
> + drm_framebuffer_unreference(fb);
>
> return 0;
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/5] drm/vmwgfx: Nuke preclose hook
LGTM.
Reviewed-by: Thomas Hellstrom
On 01/10/2016 11:26 PM, Daniel Vetter wrote:
> Again since the drm core takes care of event unlinking/disarming this
> is now just needless code.
>
> v2: I've completely missed eaction->fpriv_head and all the related
> code. We need to nuke that too to avoid accidentally deferencing the
> freed-up vmwgfx-private fpriv.
>
> v3: Also remove vmw_fpriv->fence_events and unused variables I missed.
>
> Cc: Thomas Hellström
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 11
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 1 -
> drivers/gpu/drm/vmwgfx/vmwgfx_fence.c | 52
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_fence.h | 2 --
> 4 files changed, 66 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index c49812b80dd0..c96a2d2d5107 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -971,15 +971,6 @@ static int vmw_driver_unload(struct drm_device *dev)
> return 0;
> }
>
> -static void vmw_preclose(struct drm_device *dev,
> - struct drm_file *file_priv)
> -{
> - struct vmw_fpriv *vmw_fp = vmw_fpriv(file_priv);
> - struct vmw_private *dev_priv = vmw_priv(dev);
> -
> - vmw_event_fence_fpriv_gone(dev_priv->fman, &vmw_fp->fence_events);
> -}
> -
> static void vmw_postclose(struct drm_device *dev,
>struct drm_file *file_priv)
> {
> @@ -1010,7 +1001,6 @@ static int vmw_driver_open(struct drm_device *dev,
> struct drm_file *file_priv)
> if (unlikely(vmw_fp == NULL))
> return ret;
>
> - INIT_LIST_HEAD(&vmw_fp->fence_events);
> vmw_fp->tfile = ttm_object_file_init(dev_priv->tdev, 10);
> if (unlikely(vmw_fp->tfile == NULL))
> goto out_no_tfile;
> @@ -1500,7 +1490,6 @@ static struct drm_driver driver = {
> .master_set = vmw_master_set,
> .master_drop = vmw_master_drop,
> .open = vmw_driver_open,
> - .preclose = vmw_preclose,
> .postclose = vmw_postclose,
> .set_busid = drm_pci_set_busid,
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> index 469cdd520615..5cb1b1687cd4 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> @@ -80,7 +80,6 @@
> struct vmw_fpriv {
> struct drm_master *locked_master;
> struct ttm_object_file *tfile;
> - struct list_head fence_events;
> bool gb_aware;
> };
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
> index e0edf149d9d5..ac477863fc07 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
> @@ -71,7 +71,6 @@ struct vmw_user_fence {
> */
> struct vmw_event_fence_action {
> struct vmw_fence_action action;
> - struct list_head fpriv_head;
>
> struct drm_pending_event *event;
> struct vmw_fence_obj *fence;
> @@ -808,44 +807,6 @@ int vmw_fence_obj_unref_ioctl(struct drm_device *dev,
> void *data,
> }
>
> /**
> - * vmw_event_fence_fpriv_gone - Remove references to struct drm_file objects
> - *
> - * @fman: Pointer to a struct vmw_fence_manager
> - * @event_list: Pointer to linked list of struct vmw_event_fence_action
> objects
> - * with pointers to a struct drm_file object about to be closed.
> - *
> - * This function removes all pending fence events with references to a
> - * specific struct drm_file object about to be closed. The caller is required
> - * to pass a list of all struct vmw_event_fence_action objects with such
> - * events attached. This function is typically called before the
> - * struct drm_file object's event management is taken down.
> - */
> -void vmw_event_fence_fpriv_gone(struct vmw_fence_manager *fman,
> - struct list_head *event_list)
> -{
> - struct vmw_event_fence_action *eaction;
> - struct drm_pending_event *event;
> - unsigned long irq_flags;
> -
> - while (1) {
> - spin_lock_irqsave(&fman->lock, irq_flags);
> - if (list_empty(event_list))
> - goto out_unlock;
> - eaction = list_first_entry(event_list,
> -struct vmw_event_fence_action,
> -fpriv_head);
> - list_del_init(&eaction->fpriv_head);
> - event = eaction->event;
> - eaction->event = NULL;
Re: [Intel-gfx] [PATCH] drm/vmwgfx: Nuke preclose hook
In general, looks good. Two things though.
1) vc4_drv.h looks like it ended up in the wrong patch.
2) Should be able to nuke also struct vmw_fpriv::fence_events and struct
vmw_event_fence_action::fpriv_head?
/Thomas
On 01/10/2016 11:02 PM, Daniel Vetter wrote:
> Again since the drm core takes care of event unlinking/disarming this
> is now just needless code.
>
> v2: I've completely missed eaction->fpriv_head and all the related
> code. We need to nuke that too to avoid accidentally deferencing the
> freed-up vmwgfx-private fpriv.
>
> Cc: Thomas Hellström
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/vc4/vc4_drv.h | 1 -
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 10
> drivers/gpu/drm/vmwgfx/vmwgfx_fence.c | 48
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_fence.h | 2 --
> 4 files changed, 61 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 080865ec2bae..4c734d087d7f 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -376,7 +376,6 @@ int vc4_bo_stats_debugfs(struct seq_file *m, void *arg);
> extern struct platform_driver vc4_crtc_driver;
> int vc4_enable_vblank(struct drm_device *dev, unsigned int crtc_id);
> void vc4_disable_vblank(struct drm_device *dev, unsigned int crtc_id);
> -void vc4_cancel_page_flip(struct drm_crtc *crtc, struct drm_file *file);
> int vc4_crtc_debugfs_regs(struct seq_file *m, void *arg);
>
> /* vc4_debugfs.c */
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index c49812b80dd0..c04c4d1e2f3e 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -971,15 +971,6 @@ static int vmw_driver_unload(struct drm_device *dev)
> return 0;
> }
>
> -static void vmw_preclose(struct drm_device *dev,
> - struct drm_file *file_priv)
> -{
> - struct vmw_fpriv *vmw_fp = vmw_fpriv(file_priv);
> - struct vmw_private *dev_priv = vmw_priv(dev);
> -
> - vmw_event_fence_fpriv_gone(dev_priv->fman, &vmw_fp->fence_events);
> -}
> -
> static void vmw_postclose(struct drm_device *dev,
>struct drm_file *file_priv)
> {
> @@ -1500,7 +1491,6 @@ static struct drm_driver driver = {
> .master_set = vmw_master_set,
> .master_drop = vmw_master_drop,
> .open = vmw_driver_open,
> - .preclose = vmw_preclose,
> .postclose = vmw_postclose,
> .set_busid = drm_pci_set_busid,
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
> index e0edf149d9d5..bb95bd20d415 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
> @@ -71,7 +71,6 @@ struct vmw_user_fence {
> */
> struct vmw_event_fence_action {
> struct vmw_fence_action action;
> - struct list_head fpriv_head;
>
> struct drm_pending_event *event;
> struct vmw_fence_obj *fence;
> @@ -808,44 +807,6 @@ int vmw_fence_obj_unref_ioctl(struct drm_device *dev,
> void *data,
> }
>
> /**
> - * vmw_event_fence_fpriv_gone - Remove references to struct drm_file objects
> - *
> - * @fman: Pointer to a struct vmw_fence_manager
> - * @event_list: Pointer to linked list of struct vmw_event_fence_action
> objects
> - * with pointers to a struct drm_file object about to be closed.
> - *
> - * This function removes all pending fence events with references to a
> - * specific struct drm_file object about to be closed. The caller is required
> - * to pass a list of all struct vmw_event_fence_action objects with such
> - * events attached. This function is typically called before the
> - * struct drm_file object's event management is taken down.
> - */
> -void vmw_event_fence_fpriv_gone(struct vmw_fence_manager *fman,
> - struct list_head *event_list)
> -{
> - struct vmw_event_fence_action *eaction;
> - struct drm_pending_event *event;
> - unsigned long irq_flags;
> -
> - while (1) {
> - spin_lock_irqsave(&fman->lock, irq_flags);
> - if (list_empty(event_list))
> - goto out_unlock;
> - eaction = list_first_entry(event_list,
> -struct vmw_event_fence_action,
> -fpriv_head);
> - list_del_init(&eaction->fpriv_head);
> - event = eaction->event;
> - eaction->event = NULL;
> - spin_unlock_irqrestore(&fman->lock, irq_flags);
> - event->destroy(event);
> - }
> -out_unlock:
> - spin_unlock_irqrestore(&fman->lock, irq_flags);
> -}
> -
> -
> -/**
> * vmw_event_fence_action_seq_passed
> *
> * @action: The struct vmw_fence_action embedded in a struct
> @@ -879,7 +840,6 @@ static void vmw_event_fence_action_seq_passed(struct
> vmw_fence_action *action)
> *eaction->tv_usec = tv.tv_usec;
Re: [Intel-gfx] [PATCH 21/21] drm/vmwgfx: Nuke preclose hook
On 01/09/2016 11:43 AM, Daniel Vetter wrote: > On Fri, Jan 8, 2016 at 9:53 PM, Thomas Hellstrom > wrote: >> On 01/08/2016 09:36 PM, Daniel Vetter wrote: >>> Again since the drm core takes care of event unlinking/disarming this >>> is now just needless code. >>> >>> Cc: Thomas Hellström >>> Signed-off-by: Daniel Vetter >> Hmm, >> >> IIRC this is actually a list of events that core drm is not aware of >> yet. They sit on this list waiting for a fence to pass and are then >> transferred to core drm > Yes I know. Earlier patches in the series extract new core functions > to setup/tear down such events and send them out, which is what's > needed to make this trick possible. Exynos similarly uses events > similarly, and is also converted. Same for nouveau it seems, but there > the code doesn't use the reserve/send split, so I'm unclear > how/whether at all it correctly handles this race. > -Daniel Ah. Hmm I should've looked more closely at the rest of the series. In any case, this particular patch leaves, from what I can tell, the eaction fpriv list intact when it is later freed in postclose, which is bad. Also each eaction is left with a dangling pointer to a freed pending event which is also very bad since that pointer will be dereferenced as soon as the fence's seqno has passed. So as far as i can tell, this function needs to remain except for the event destruction. Thinking of it, this must be a problem that is more general problem than for vmwgfx only, I mean, unless the driver traverses all core pending event list to find relevant pending events to process, something in the driver must actually point to the pending event (be it a pointer in the fence object or, as in the vmwgfx case, a pointer in the fence action object) and that pointer must somehow be invalidated when the pending event is freed... Which also brings up a question, where are the pending events actually destroyed? I can see they are unlinked in drm_fops.c. /Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 21/21] drm/vmwgfx: Nuke preclose hook
On 01/08/2016 09:36 PM, Daniel Vetter wrote:
> Again since the drm core takes care of event unlinking/disarming this
> is now just needless code.
>
> Cc: Thomas Hellström
> Signed-off-by: Daniel Vetter
Hmm,
IIRC this is actually a list of events that core drm is not aware of
yet. They sit on this list waiting for a fence to pass and are then
transferred to core drm
/Thomas
> ---
> drivers/gpu/drm/vc4/vc4_drv.h | 1 -
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 10 -
> drivers/gpu/drm/vmwgfx/vmwgfx_fence.c | 38
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_fence.h | 2 --
> 4 files changed, 51 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 080865ec2bae..4c734d087d7f 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -376,7 +376,6 @@ int vc4_bo_stats_debugfs(struct seq_file *m, void *arg);
> extern struct platform_driver vc4_crtc_driver;
> int vc4_enable_vblank(struct drm_device *dev, unsigned int crtc_id);
> void vc4_disable_vblank(struct drm_device *dev, unsigned int crtc_id);
> -void vc4_cancel_page_flip(struct drm_crtc *crtc, struct drm_file *file);
> int vc4_crtc_debugfs_regs(struct seq_file *m, void *arg);
>
> /* vc4_debugfs.c */
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index c49812b80dd0..c04c4d1e2f3e 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -971,15 +971,6 @@ static int vmw_driver_unload(struct drm_device *dev)
> return 0;
> }
>
> -static void vmw_preclose(struct drm_device *dev,
> - struct drm_file *file_priv)
> -{
> - struct vmw_fpriv *vmw_fp = vmw_fpriv(file_priv);
> - struct vmw_private *dev_priv = vmw_priv(dev);
> -
> - vmw_event_fence_fpriv_gone(dev_priv->fman, &vmw_fp->fence_events);
> -}
> -
> static void vmw_postclose(struct drm_device *dev,
>struct drm_file *file_priv)
> {
> @@ -1500,7 +1491,6 @@ static struct drm_driver driver = {
> .master_set = vmw_master_set,
> .master_drop = vmw_master_drop,
> .open = vmw_driver_open,
> - .preclose = vmw_preclose,
> .postclose = vmw_postclose,
> .set_busid = drm_pci_set_busid,
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
> index e0edf149d9d5..55dc3e4754df 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
> @@ -808,44 +808,6 @@ int vmw_fence_obj_unref_ioctl(struct drm_device *dev,
> void *data,
> }
>
> /**
> - * vmw_event_fence_fpriv_gone - Remove references to struct drm_file objects
> - *
> - * @fman: Pointer to a struct vmw_fence_manager
> - * @event_list: Pointer to linked list of struct vmw_event_fence_action
> objects
> - * with pointers to a struct drm_file object about to be closed.
> - *
> - * This function removes all pending fence events with references to a
> - * specific struct drm_file object about to be closed. The caller is required
> - * to pass a list of all struct vmw_event_fence_action objects with such
> - * events attached. This function is typically called before the
> - * struct drm_file object's event management is taken down.
> - */
> -void vmw_event_fence_fpriv_gone(struct vmw_fence_manager *fman,
> - struct list_head *event_list)
> -{
> - struct vmw_event_fence_action *eaction;
> - struct drm_pending_event *event;
> - unsigned long irq_flags;
> -
> - while (1) {
> - spin_lock_irqsave(&fman->lock, irq_flags);
> - if (list_empty(event_list))
> - goto out_unlock;
> - eaction = list_first_entry(event_list,
> -struct vmw_event_fence_action,
> -fpriv_head);
> - list_del_init(&eaction->fpriv_head);
> - event = eaction->event;
> - eaction->event = NULL;
> - spin_unlock_irqrestore(&fman->lock, irq_flags);
> - event->destroy(event);
> - }
> -out_unlock:
> - spin_unlock_irqrestore(&fman->lock, irq_flags);
> -}
> -
> -
> -/**
> * vmw_event_fence_action_seq_passed
> *
> * @action: The struct vmw_fence_action embedded in a struct
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.h
> b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.h
> index 8be6c29f5eb5..83ae301ee141 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.h
> @@ -116,8 +116,6 @@ extern int vmw_fence_obj_unref_ioctl(struct drm_device
> *dev, void *data,
>struct drm_file *file_priv);
> extern int vmw_fence_event_ioctl(struct drm_device *dev, void *data,
>struct drm_file *file_priv);
> -extern void vmw_event_fence_fpriv_gone(struct vmw_fence_manager *fman,
> -
Re: [Intel-gfx] linux-next: manual merge of the drm-misc tree with Linus' tree
On 12/14/2015 02:12 AM, Stephen Rothwell wrote:
> Hi all,
>
> Today's linux-next merge of the drm-misc tree got conflicts in:
>
> drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
>
> between commit:
>
> 8fbf9d92a7bc ("drm/vmwgfx: Implement the cursor_set2 callback v2")
>
> from Linus' tree and commit:
>
> f80de66eca65 ("drm/vmwgfx: Drop dummy save/restore hooks")
>
> from the drm-misc tree.
>
> I fixed it up (see below) and can carry the fix as necessary (no action
> is required).
>
FWIW, the fix looks correct to me.
/Thomas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/28] drm/vmwgfx: Drop dummy save/restore hooks
Reviewed-by: Thomas Hellstrom
On 12/04/2015 09:45 AM, Daniel Vetter wrote:
> These hooks will be gone soon.
>
> Cc: Thomas Hellstrom
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 16
> drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 4
> drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 4
> drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 4
> 4 files changed, 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index e38db35132ed..d50596153679 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -1331,14 +1331,6 @@ static int vmw_du_update_layout(struct vmw_private
> *dev_priv, unsigned num,
> return 0;
> }
>
> -void vmw_du_crtc_save(struct drm_crtc *crtc)
> -{
> -}
> -
> -void vmw_du_crtc_restore(struct drm_crtc *crtc)
> -{
> -}
> -
> void vmw_du_crtc_gamma_set(struct drm_crtc *crtc,
> u16 *r, u16 *g, u16 *b,
> uint32_t start, uint32_t size)
> @@ -1360,14 +1352,6 @@ int vmw_du_connector_dpms(struct drm_connector
> *connector, int mode)
> return 0;
> }
>
> -void vmw_du_connector_save(struct drm_connector *connector)
> -{
> -}
> -
> -void vmw_du_connector_restore(struct drm_connector *connector)
> -{
> -}
> -
> enum drm_connector_status
> vmw_du_connector_detect(struct drm_connector *connector, bool force)
> {
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> index bb63e4d795fa..c1c09b338cc1 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> @@ -295,8 +295,6 @@ static int vmw_ldu_crtc_set_config(struct drm_mode_set
> *set)
> }
>
> static struct drm_crtc_funcs vmw_legacy_crtc_funcs = {
> - .save = vmw_du_crtc_save,
> - .restore = vmw_du_crtc_restore,
> .cursor_set = vmw_du_crtc_cursor_set,
> .cursor_move = vmw_du_crtc_cursor_move,
> .gamma_set = vmw_du_crtc_gamma_set,
> @@ -329,8 +327,6 @@ static void vmw_ldu_connector_destroy(struct
> drm_connector *connector)
>
> static struct drm_connector_funcs vmw_legacy_connector_funcs = {
> .dpms = vmw_du_connector_dpms,
> - .save = vmw_du_connector_save,
> - .restore = vmw_du_connector_restore,
> .detect = vmw_du_connector_detect,
> .fill_modes = vmw_du_connector_fill_modes,
> .set_property = vmw_du_connector_set_property,
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> index b96d1ab610c5..26dfed6b0c48 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> @@ -531,8 +531,6 @@ out_no_fence:
> }
>
> static struct drm_crtc_funcs vmw_screen_object_crtc_funcs = {
> - .save = vmw_du_crtc_save,
> - .restore = vmw_du_crtc_restore,
> .cursor_set = vmw_du_crtc_cursor_set,
> .cursor_move = vmw_du_crtc_cursor_move,
> .gamma_set = vmw_du_crtc_gamma_set,
> @@ -565,8 +563,6 @@ static void vmw_sou_connector_destroy(struct
> drm_connector *connector)
>
> static struct drm_connector_funcs vmw_sou_connector_funcs = {
> .dpms = vmw_du_connector_dpms,
> - .save = vmw_du_connector_save,
> - .restore = vmw_du_connector_restore,
> .detect = vmw_du_connector_detect,
> .fill_modes = vmw_du_connector_fill_modes,
> .set_property = vmw_du_connector_set_property,
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> index b1fc1c02792d..05375a8cc129 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> @@ -1041,8 +1041,6 @@ out_finish:
> * Screen Target CRTC dispatch table
> */
> static struct drm_crtc_funcs vmw_stdu_crtc_funcs = {
> - .save = vmw_du_crtc_save,
> - .restore = vmw_du_crtc_restore,
> .cursor_set = vmw_du_crtc_cursor_set,
> .cursor_move = vmw_du_crtc_cursor_move,
> .gamma_set = vmw_du_crtc_gamma_set,
> @@ -1101,8 +1099,6 @@ static void vmw_stdu_connector_destroy(struct
> drm_connector *connector)
>
> static struct drm_connector_funcs vmw_stdu_connector_funcs = {
> .dpms = vmw_du_connector_dpms,
> - .save = vmw_du_connector_save,
> - .restore = vmw_du_connector_restore,
> .detect = vmw_du_connector_detect,
> .fill_modes = vmw_du_connector_fill_modes,
> .set_property = vmw_du_connector_set_property,
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t] tests: add core_setmaster_vs_auth
Daniel,
LGTM, except one comment below:
On 12/01/2015 08:45 AM, Daniel Vetter wrote:
> Tests that master state isn't leaked to new masters by checking
> that auth magics for the old master don't work any more.
>
> Based upon a simple test program provided by Thomas.
>
> v2: Use correct test stanza ... and I need coffee.
>
> Cc: Thomas Hellstrom
> Signed-off-by: Daniel Vetter
> ---
> tests/.gitignore | 1 +
> tests/Makefile.sources | 1 +
> tests/core_setmaster_vs_auth.c | 73
> ++
> 3 files changed, 75 insertions(+)
> create mode 100644 tests/core_setmaster_vs_auth.c
>
> diff --git a/tests/.gitignore b/tests/.gitignore
> index 85936ea45c9f..43d63d3abac9 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -4,6 +4,7 @@ core_getclient
> core_getstats
> core_getversion
> core_prop_blob
> +core_setmaster_vs_auth
> drm_auth
> drm_import_export
> drm_read
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index ff178f7a2df4..b70bca060253 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -102,6 +102,7 @@ TESTS_progs = \
> core_getstats \
> core_getversion \
> core_prop_blob \
> + core_setmaster_vs_auth \
> drm_auth \
> drm_import_export \
> drm_read \
> diff --git a/tests/core_setmaster_vs_auth.c b/tests/core_setmaster_vs_auth.c
> new file mode 100644
> index ..1d66044f7fe1
> --- /dev/null
> +++ b/tests/core_setmaster_vs_auth.c
> @@ -0,0 +1,73 @@
> +/*
> + * Copyright © 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *Daniel Vetter
> + *
> + * Based upon a test program provided by Thomas Hellstrom
>
> + */
> +
> +/*
> + * Testcase: Check that drop/setMaster correctly transfer master state
> + *
> + * Test approach is only checking auth state (which is part of master state)
> by
> + * trying to authenticate a client against the wrong master.
> + */
> +
> +#define _GNU_SOURCE
> +#include "igt.h"
> +#include
> +#include
> +#include
> +#include
> +#ifdef __linux__
> +# include
> +#else
> +# include
> +#endif
> +
> +igt_simple_main
> +{
> + int master1, master2, client;
> + drm_magic_t magic;
> +
> + master1 = drm_open_driver(DRIVER_ANY);
> + do_or_die(drmSetMaster(master1));
> +
> + /* Get an authentication magic from the first master */
> + client = drm_open_driver(DRIVER_ANY);
> + do_or_die(drmGetMagic(client, &magic));
> +
> + /* Open an fd an make it master */
> + master2 = drm_open_driver(DRIVER_ANY);
> + do_or_die(drmDropMaster(master1));
> + do_or_die(drmSetMaster(master2));
> +
> + /* auth shouldn't work any more since the master we have the magic from
> + * isn't the current master any more. */
Auth shouldn't work since we're authenticating with a different master than
the one we got the magic from.
> + igt_assert_neq(drmAuthMagic(master2, magic), 0);
> + igt_assert_eq(errno, EINVAL);
> +
> + close(client);
> + close(master2);
> + close(master1);
> +}
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4] dma-buf: Add ioctls to allow userspace to flush
> 26 aug 2015 kl. 16:58 skrev Tiago Vignatti : > >> On 08/26/2015 11:51 AM, Daniel Vetter wrote: >>> On Wed, Aug 26, 2015 at 11:32:30AM -0300, Tiago Vignatti wrote: On 08/26/2015 09:58 AM, Daniel Vetter wrote: The other is that right now there's no user nor implementation in sight which actually does range-based flush optimizations, so I'm pretty much expecting we'll get it wrong. Maybe instead we should go one step further and remove the range from the internal dma-buf interface and also drop it >>> >from the ioctl? With the flags we can always add something later on once we have a real user with a clear need for it. But afaik cros only wants to shuffle around entire tiles and has a buffer-per-tile approach. >>> >>> Thomas, I think Daniel has a point here and also, I wouldn't mind removing >>> all range control from the dma-buf ioctl either. >> >> if we go with nuking it from the ioctl I'd suggest to also nuke it from >> the dma-buf internal inferface first too. > > yep, I can do it. > > Thomas, so we leave 2d sync out now? > > Tiago > Sure! Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4] dma-buf: Add ioctls to allow userspace to flush
On 08/26/2015 04:32 PM, Tiago Vignatti wrote: > On 08/26/2015 09:58 AM, Daniel Vetter wrote: >> The other is that right now there's no user nor implementation in sight >> which actually does range-based flush optimizations, so I'm pretty much >> expecting we'll get it wrong. Maybe instead we should go one step >> further >> and remove the range from the internal dma-buf interface and also >> drop it >> from the ioctl? With the flags we can always add something later on once >> we have a real user with a clear need for it. But afaik cros only >> wants to >> shuffle around entire tiles and has a buffer-per-tile approach. > > Thomas, I think Daniel has a point here and also, I wouldn't mind > removing all range control from the dma-buf ioctl either. > > In this last patch we can see that it's not complicated to add the > 2d-sync if we eventually decides to want it. But right now there's no > way we can test it. Therefore in that case I'm all in favour of doing > this work gradually, adding the basics first. > Sure. Unfortunately I wasn't completely clear about the use-case here. IMO adding a user-space sync functionality should be purely for performance. /Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4] dma-buf: Add ioctls to allow userspace to flush
On 08/26/2015 02:10 PM, Daniel Vetter wrote: > On Wed, Aug 26, 2015 at 08:49:00AM +0200, Thomas Hellstrom wrote: >> Hi, Tiago. >> >> On 08/26/2015 02:02 AM, Tiago Vignatti wrote: >>> From: Daniel Vetter >>> >>> The userspace might need some sort of cache coherency management e.g. when >>> CPU >>> and GPU domains are being accessed through dma-buf at the same time. To >>> circumvent this problem there are begin/end coherency markers, that forward >>> directly to existing dma-buf device drivers vfunc hooks. Userspace can make >>> use >>> of those markers through the DMA_BUF_IOCTL_SYNC ioctl. The sequence would be >>> used like following: >>> >>> - mmap dma-buf fd >>> - for each drawing/upload cycle in CPU >>> 1. SYNC_START ioctl >>> 2. read/write to mmap area or a 2d sub-region of it >>> 3. SYNC_END ioctl. >>> - munamp once you don't need the buffer any more >>> >>> v2 (Tiago): Fix header file type names (u64 -> __u64) >>> v3 (Tiago): Add documentation. Use enum dma_buf_sync_flags to the begin/end >>> dma-buf functions. Check for overflows in start/length. >>> v4 (Tiago): use 2d regions for sync. >> Daniel V had issues with the sync argument proposed by Daniel S. I'm >> fine with that argument or an argument containing only a single sync >> rect. I'm not sure whether Daniel V will find it easier to accept only a >> single sync rect... > I'm kinda against all the 2d rect sync proposals ;-) At least for the > current stuff it's all about linear subranges afaik, and even there we > don't bother with flushing them precisely right now. > > My expectation would be that if you _really_ want to etch out that last > bit of performance with a list of 2d sync ranges then probably you want to > do the cpu cache flushing in userspace anyway, with 100% machine-specific > trickery. Daniel, I might be misunderstanding things, but isn't this about finally accepting a dma-buf mmap() generic interface for people who want to use it for zero-copy applications (like people have been trying to do for years but never bothered to specify an interface that actually performed on incoherent hardware)? If it's only about exposing the kernel 1D sync interface to user-space for correctness, then why isn't that done transparently to the user? Thanks, Thomas > -Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4] dma-buf: Add ioctls to allow userspace to flush
Hi, Tiago.
On 08/26/2015 02:02 AM, Tiago Vignatti wrote:
> From: Daniel Vetter
>
> The userspace might need some sort of cache coherency management e.g. when CPU
> and GPU domains are being accessed through dma-buf at the same time. To
> circumvent this problem there are begin/end coherency markers, that forward
> directly to existing dma-buf device drivers vfunc hooks. Userspace can make
> use
> of those markers through the DMA_BUF_IOCTL_SYNC ioctl. The sequence would be
> used like following:
>
> - mmap dma-buf fd
> - for each drawing/upload cycle in CPU
> 1. SYNC_START ioctl
> 2. read/write to mmap area or a 2d sub-region of it
> 3. SYNC_END ioctl.
> - munamp once you don't need the buffer any more
>
> v2 (Tiago): Fix header file type names (u64 -> __u64)
> v3 (Tiago): Add documentation. Use enum dma_buf_sync_flags to the begin/end
> dma-buf functions. Check for overflows in start/length.
> v4 (Tiago): use 2d regions for sync.
Daniel V had issues with the sync argument proposed by Daniel S. I'm
fine with that argument or an argument containing only a single sync
rect. I'm not sure whether Daniel V will find it easier to accept only a
single sync rect...
Also please see below.
>
> Cc: Sumit Semwal
> Signed-off-by: Daniel Vetter
> Signed-off-by: Tiago Vignatti
> ---
>
> I'm unable to test the 2d sync properly, because begin/end access in i915
> don't track mapped range for nothing.
>
> Documentation/dma-buf-sharing.txt | 13 ++
> drivers/dma-buf/dma-buf.c | 77
> --
> drivers/gpu/drm/i915/i915_gem_dmabuf.c | 6 ++-
> include/linux/dma-buf.h| 20 +
> include/uapi/linux/dma-buf.h | 57 +
> 5 files changed, 150 insertions(+), 23 deletions(-)
> create mode 100644 include/uapi/linux/dma-buf.h
>
> diff --git a/Documentation/dma-buf-sharing.txt
> b/Documentation/dma-buf-sharing.txt
> index 480c8de..8061ac0 100644
> --- a/Documentation/dma-buf-sharing.txt
> +++ b/Documentation/dma-buf-sharing.txt
> @@ -355,6 +355,19 @@ Being able to mmap an export dma-buf buffer object has 2
> main use-cases:
>
> No special interfaces, userspace simply calls mmap on the dma-buf fd.
>
> + Also, the userspace might need some sort of cache coherency management
> e.g.
> + when CPU and GPU domains are being accessed through dma-buf at the same
> + time. To circumvent this problem there are begin/end coherency markers,
> that
> + forward directly to existing dma-buf device drivers vfunc hooks. Userspace
> + can make use of those markers through the DMA_BUF_IOCTL_SYNC ioctl. The
> + sequence would be used like following:
> + - mmap dma-buf fd
> + - for each drawing/upload cycle in CPU
> + 1. SYNC_START ioctl
> + 2. read/write to mmap area or a 2d sub-region of it
> + 3. SYNC_END ioctl.
> + - munamp once you don't need the buffer any more
> +
> 2. Supporting existing mmap interfaces in importers
>
> Similar to the motivation for kernel cpu access it is again important that
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 155c146..b6a4a06 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -251,11 +251,55 @@ out:
> return events;
> }
>
> +static long dma_buf_ioctl(struct file *file,
> + unsigned int cmd, unsigned long arg)
> +{
> + struct dma_buf *dmabuf;
> + struct dma_buf_sync sync;
> + enum dma_data_direction direction;
> +
> + dmabuf = file->private_data;
> +
> + if (!is_dma_buf_file(file))
> + return -EINVAL;
> +
> + if (cmd != DMA_BUF_IOCTL_SYNC)
> + return -ENOTTY;
> +
> + if (copy_from_user(&sync, (void __user *) arg, sizeof(sync)))
> + return -EFAULT;
Do we actually copy all sync regions here?
> +
> + if (sync.flags & DMA_BUF_SYNC_RW)
> + direction = DMA_BIDIRECTIONAL;
> + else if (sync.flags & DMA_BUF_SYNC_READ)
> + direction = DMA_FROM_DEVICE;
> + else if (sync.flags & DMA_BUF_SYNC_WRITE)
> + direction = DMA_TO_DEVICE;
> + else
> + return -EINVAL;
> +
> + if (sync.flags & ~DMA_BUF_SYNC_VALID_FLAGS_MASK)
> + return -EINVAL;
> +
> + /* TODO: check for overflowing the buffer's size - how so, checking
> region by
> + * region here? Maybe need to check for the other parameters as well. */
> +
> + if (sync.flags & DMA_BUF_SYNC_END)
> + dma_buf_end_cpu_access(dmabuf, sync.stride_bytes,
> sync.bytes_per_pixel,
> + sync.num_regions, sync.regions, direction);
> + else
> + dma_buf_begin_cpu_access(dmabuf, sync.stride_bytes,
> sync.bytes_per_pixel,
> + sync.num_regions, sync.regions, direction);
> +
> + return 0;
> +}
> +
> static const struct file_operations dma_buf_fops = {
> .release= dma_bu
Re: [Intel-gfx] [PATCH v4] mmap on the dma-buf directly
On 08/13/2015 01:29 AM, Tiago Vignatti wrote:
> Hi,
>
> The idea is to create a GEM bo in one process and pass the prime handle of the
> it to another process, which in turn uses the handle only to map and write.
> This could be useful for Chrome OS architecture, where the Web content
> ("unpriviledged process") maps and CPU-draws a buffer, which was previously
> allocated in the GPU process ("priviledged process").
>
> In v2, I've added a patch that Daniel kindly drafted to allow the
> unpriviledged process flush through a prime fd. In v3, I've fixed a few
> concerns and then added end_cpu_access to i915. In v4, I fixed Sumit Semwal's
> concerns about dma-duf documentation and the FIXME missing in that same patch,
> and also removed WARN in i915 dma-buf mmap (pointed by Chris). PTAL.
>
> Best Regards,
>
> Tiago
Tiago,
I take it, this is intended to be a generic interface used mostly for 2D
rendering.
In that case, using SYNC is crucial for performance of incoherent
architectures and I'd rather see it mandatory than an option. It could
perhaps be made mandatory preferrably using an error or a one-time
kernel warning. If nobody uses the SYNC interface, it is of little use.
Also I think the syncing needs to be extended to two dimensions. A long
time ago when this was brought up people argued why we should limit it
to two dimensions, but I believe two dimensions addresses most
performance-problematic use-cases. A default implementation of
twodimensional sync can easily be made using the one-dimensional API.
Thanks,
Thomas
>
>
> Daniel Thompson (1):
> drm: prime: Honour O_RDWR during prime-handle-to-fd
>
> Daniel Vetter (1):
> dma-buf: Add ioctls to allow userspace to flush
>
> Tiago Vignatti (2):
> drm/i915: Implement end_cpu_access
> drm/i915: Use CPU mapping for userspace dma-buf mmap()
>
> Documentation/dma-buf-sharing.txt | 12
> drivers/dma-buf/dma-buf.c | 50
> ++
> drivers/gpu/drm/drm_prime.c| 10 ++-
> drivers/gpu/drm/i915/i915_gem_dmabuf.c | 28 ++-
> include/uapi/drm/drm.h | 1 +
> include/uapi/linux/dma-buf.h | 43 +
> 6 files changed, 136 insertions(+), 8 deletions(-)
> create mode 100644 include/uapi/linux/dma-buf.h
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/mm: Support 4 GiB and larger ranges
On 01/27/2015 09:15 AM, Thierry Reding wrote: > On Tue, Jan 27, 2015 at 07:07:44AM +0100, Thomas Hellstrom wrote: >> On 01/26/2015 11:51 PM, Dave Airlie wrote: >>> On 23 January 2015 at 18:05, Thierry Reding wrote: >>>> From: Thierry Reding >>>> >>>> The current implementation is limited by the number of addresses that >>>> fit into an unsigned long. This causes problems on 32-bit Tegra where >>>> unsigned long is 32-bit but drm_mm is used to manage an IOVA space of >>>> 4 GiB. Given the 32-bit limitation, the range is limited to 4 GiB - 1 >>>> (or 4 GiB - 4 KiB for page granularity). >>>> >>>> This commit changes the start and size of the range to be an unsigned >>>> 64-bit integer, thus allowing much larger ranges to be supported. >>> This seems fine to me, Chris, Daniel or Thomas, any objections? >>> >>> Dave. >> >> This is perfectly fine with me, although I'm a bit curious why the >> allocation granularity of the IOVA space needs to be 1 byte? > > Are you referring to the 4 GiB - 1 comment? The point I was trying to > make is not that the granularity of the IOVA space needs to be 1 byte > but rather that using an unsigned long for a size on a 32-bit machine > will give you 4 GiB - 1 addresses. The IOMMU page size is still 4 KiB > for Tegra. I was rather referring to that if the range manager (drm_mm) is set up to manage pages instead of bytes (like, for example, the TTM VM address space), you'd get 4G - 1 pages, which, I believe, is sufficient on most 32 bit systems? Thanks, /Thomas > > Thierry ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/mm: Support 4 GiB and larger ranges
On 01/26/2015 11:51 PM, Dave Airlie wrote: > On 23 January 2015 at 18:05, Thierry Reding wrote: >> From: Thierry Reding >> >> The current implementation is limited by the number of addresses that >> fit into an unsigned long. This causes problems on 32-bit Tegra where >> unsigned long is 32-bit but drm_mm is used to manage an IOVA space of >> 4 GiB. Given the 32-bit limitation, the range is limited to 4 GiB - 1 >> (or 4 GiB - 4 KiB for page granularity). >> >> This commit changes the start and size of the range to be an unsigned >> 64-bit integer, thus allowing much larger ranges to be supported. > This seems fine to me, Chris, Daniel or Thomas, any objections? > > Dave. This is perfectly fine with me, although I'm a bit curious why the allocation granularity of the IOVA space needs to be 1 byte? Thanks, Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH] drm/radeon: fixup locking inversion between mmap_sem and reservations
On 10/08/2013 06:47 PM, Jerome Glisse wrote: On Tue, Oct 08, 2013 at 06:29:35PM +0200, Thomas Hellstrom wrote: On 10/08/2013 04:55 PM, Jerome Glisse wrote: On Tue, Oct 08, 2013 at 04:45:18PM +0200, Christian König wrote: Am 08.10.2013 16:33, schrieb Jerome Glisse: On Tue, Oct 08, 2013 at 04:14:40PM +0200, Maarten Lankhorst wrote: Allocate and copy all kernel memory before doing reservations. This prevents a locking inversion between mmap_sem and reservation_class, and allows us to drop the trylocking in ttm_bo_vm_fault without upsetting lockdep. Signed-off-by: Maarten Lankhorst I would say NAK. Current code only allocate temporary page in AGP case. So AGP case is userspace -> temp page -> cs checker -> radeon ib. Non AGP is directly memcpy to radeon IB. Your patch allocate memory memcpy userspace to it and it will then be memcpy to IB. Which means you introduce an extra memcpy in the process not something we want. Totally agree. Additional to that there is no good reason to provide anything else than anonymous system memory to the CS ioctl, so the dependency between the mmap_sem and reservations are not really clear to me. Christian. I think is that in other code path you take mmap_sem first then reserve bo. But here we reserve bo and then we take mmap_sem because of copy >from user. Cheers, Jerome Actually the log message is a little confusing. I think the mmap_sem locking inversion problem is orthogonal to what's being fixed here. This patch fixes the possible recursive bo::reserve caused by malicious user-space handing a pointer to ttm memory so that the ttm fault handler is called when bos are already reserved. That may cause a (possibly interruptible) livelock. Once that is fixed, we are free to choose the mmap_sem -> bo::reserve locking order. Currently it's bo::reserve->mmap_sem(), but the hack required in the ttm fault handler is admittedly a bit ugly. The plan is to change the locking order to mmap_sem->bo::reserve I'm not sure if it applies to this particular case, but it should be possible to make sure that copy_from_user_inatomic() will always succeed, by making sure the pages are present using get_user_pages(), and release the pages after copy_from_user_inatomic() is done. That way there's no need for a double memcpy slowpath, but if the copied data is very fragmented I guess the resulting code may look ugly. The get_user_pages() function will return an error if it hits TTM pages. /Thomas get_user_pages + copy_from_user_inatomic is overkill. We should just do get_user_pages which fails with ttm memory and then use copy_highpage helper. Cheers, Jerome Yeah, it may well be that that's the preferred solution. /Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH] drm/radeon: fixup locking inversion between mmap_sem and reservations
On 10/08/2013 04:55 PM, Jerome Glisse wrote: On Tue, Oct 08, 2013 at 04:45:18PM +0200, Christian König wrote: Am 08.10.2013 16:33, schrieb Jerome Glisse: On Tue, Oct 08, 2013 at 04:14:40PM +0200, Maarten Lankhorst wrote: Allocate and copy all kernel memory before doing reservations. This prevents a locking inversion between mmap_sem and reservation_class, and allows us to drop the trylocking in ttm_bo_vm_fault without upsetting lockdep. Signed-off-by: Maarten Lankhorst I would say NAK. Current code only allocate temporary page in AGP case. So AGP case is userspace -> temp page -> cs checker -> radeon ib. Non AGP is directly memcpy to radeon IB. Your patch allocate memory memcpy userspace to it and it will then be memcpy to IB. Which means you introduce an extra memcpy in the process not something we want. Totally agree. Additional to that there is no good reason to provide anything else than anonymous system memory to the CS ioctl, so the dependency between the mmap_sem and reservations are not really clear to me. Christian. I think is that in other code path you take mmap_sem first then reserve bo. But here we reserve bo and then we take mmap_sem because of copy from user. Cheers, Jerome Actually the log message is a little confusing. I think the mmap_sem locking inversion problem is orthogonal to what's being fixed here. This patch fixes the possible recursive bo::reserve caused by malicious user-space handing a pointer to ttm memory so that the ttm fault handler is called when bos are already reserved. That may cause a (possibly interruptible) livelock. Once that is fixed, we are free to choose the mmap_sem -> bo::reserve locking order. Currently it's bo::reserve->mmap_sem(), but the hack required in the ttm fault handler is admittedly a bit ugly. The plan is to change the locking order to mmap_sem->bo::reserve I'm not sure if it applies to this particular case, but it should be possible to make sure that copy_from_user_inatomic() will always succeed, by making sure the pages are present using get_user_pages(), and release the pages after copy_from_user_inatomic() is done. That way there's no need for a double memcpy slowpath, but if the copied data is very fragmented I guess the resulting code may look ugly. The get_user_pages() function will return an error if it hits TTM pages. /Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH] drm/nouveau: fix nested locking in mmap handler
On 09/24/2013 11:43 AM, Maarten Lankhorst wrote: Op 24-09-13 11:03, Thomas Hellstrom schreef: On 09/24/2013 09:34 AM, Maarten Lankhorst wrote: Op 24-09-13 09:22, Thomas Hellstrom schreef: Actually, it's not the locking order bo:reserve -> mmap_sem that triggers the lockdep warning, right? but the fact that copy_from_user could recurse into the fault handler? Grabbing bo:reseves recursively? which should leave us free to choose locking order at this point. Same thing. When PROVE_LOCKING is enabled the might_fault calls in copy_to/from_user do a fake locking of mmap_sem, which means all locking errors, provided that the reservation lock is taken at least once with mmap_sem held (eg the ttm fault handler is called at least once, it can be done separately, it doesn't need to be done in the same syscall). So any bugs will be found. The only thing to worry about is that lockdep turns off after finding 1 error, so you have to make sure it doesn't bomb out before completing tests, which is sometimes a problem on early rc kernels. ;) My point was that when we only have copy_[to|from]_user_inatomic inside any bo:reservations, the might_fault would never be called inside any reservations and we should, in principle, be free to choose locking order, provided of course it could be done cleanly in fault()? I think it makes sense to keep mmap_sem the outer lock here, and not do scary things in fault. Especially if the mm locking is going to be changed in the future. But you're correct, if holding reservations only inatomic should be used. ~Maarten Now that we after all were forced to enter the dark realm of copy-user slowpaths, I don't really have a strong opinion anymore, other than that we should try to avoid building too much code in that depends on either locking order, so that we could change it if _really_ needed. /Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH] drm/nouveau: fix nested locking in mmap handler
On 09/24/2013 01:32 PM, Maarten Lankhorst wrote: Op 24-09-13 12:33, Thomas Hellstrom schreef: On 09/24/2013 12:11 PM, Maarten Lankhorst wrote: It seems userspace only updates offset and domain in nouveau. If it fails to update it would result in the same affect as when the buffer gets moved around by TTM. But hey maybe I'll have some fun, I'll lie to userspace, hardcode userspace offset to 0x4000, always force domain to be different and see what breaks. My guess is absolutely nothing, except it might expose some bugs where we forgot annotation.. I think that would certainly break if your return an -ERESTARTSYS after applying relocations but before submitting the command stream to hardware The relocations are updated before submitting the command stream, but it's copied back to userspace after submitting the command stream. I'm not sure what -ERESTARTSYS would change, the syscall is not in an interruptible state. Hmm, I was assuming without looking at the code that there was an interruptible wait for pipe/ring space after relocation application. /Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH] drm/nouveau: fix nested locking in mmap handler
On 09/24/2013 12:11 PM, Maarten Lankhorst wrote:
Op 24-09-13 11:36, Daniel Vetter schreef:
On Tue, Sep 24, 2013 at 11:03:37AM +0200, Thomas Hellstrom wrote:
On 09/24/2013 09:34 AM, Maarten Lankhorst wrote:
Op 24-09-13 09:22, Thomas Hellstrom schreef:
On 09/23/2013 05:33 PM, Maarten Lankhorst wrote:
Hey,
Op 13-09-13 11:00, Peter Zijlstra schreef:
On Fri, Sep 13, 2013 at 10:41:54AM +0200, Daniel Vetter wrote:
On Fri, Sep 13, 2013 at 10:29 AM, Peter Zijlstra wrote:
On Fri, Sep 13, 2013 at 09:46:03AM +0200, Thomas Hellstrom wrote:
if (!bo_tryreserve()) {
up_read mmap_sem(); // Release the mmap_sem to avoid deadlocks.
bo_reserve(); // Wait for the BO to become available
(interruptible)
bo_unreserve(); // Where is bo_wait_unreserved() when we need
it, Maarten :P
return VM_FAULT_RETRY; // Go ahead and retry the VMA walk, after
regrabbing
}
Anyway, could you describe what is wrong, with the above solution, because
it seems perfectly legal to me.
Luckily the rule of law doesn't have anything to do with this stuff --
at least I sincerely hope so.
The thing that's wrong with that pattern is that its still not
deterministic - although its a lot better than the pure trylock. Because
you have to release and re-acquire with the trylock another user might
have gotten in again. Its utterly prone to starvation.
The acquire+release does remove the dead/life-lock scenario from the
FIFO case, since blocking on the acquire will allow the other task to
run (or even get boosted on -rt).
Aside from that there's nothing particularly wrong with it and lockdep
should be happy afaict (but I haven't had my morning juice yet).
bo_reserve internally maps to a ww-mutex and task can already hold
ww-mutex (potentially even the same for especially nasty userspace).
OK, yes I wasn't aware of that. Yes in that case you're quite right.
I added a RFC patch below. I only tested with PROVE_LOCKING, and always forced
the slowpath for debugging.
This fixes nouveau and core ttm to always use blocking acquisition in fastpath.
Nouveau was a bit of a headache, but afaict it should work.
In almost all cases relocs are not updated, so I kept intact the fastpath
of not copying relocs from userspace. The slowpath tries to copy it atomically,
and if that fails it will unreserve all bo's and copy everything.
One thing to note is that the command submission ioctl may fail now with -EFAULT
if presumed cannot be updated, while the commands are submitted succesfully.
I think the Nouveau guys need to comment further on this, but returning -EFAULT might
break existing user-space, and that's not allowed, but IIRC the return value of
"presumed" is only a hint, and if it's incorrect will only trigger future
command stream patching.
Otherwise reviewing mostly the TTM stuff. FWIW, from wat I can tell the vmwgfx
driver doesn't need any fixups.
Well because we read the list of buffer objects the presumed offsets are at
least read-mapped. Although I guess in the worst case the mapping might
disappear before the syscall copies back the data.
So if -EFAULT happens here then userspace messed up in some way, either by
forgetting to map the offsets read-write, which cannot happen with libdrm or
free'ing the bo list before the syscall returns,
which would probably result in libdrm crashing shortly afterwards anyway.
Hmm, is the list of buffer objects (and the "presumed" members)
really in DRM memory? Because if it resides or may reside in
anonymous system memory, it may well be paged out between reading
and writing, in which case the -EFAULT return is incorrect.
In fact failures of pushbuf / execbuf *after* commands are
successfully submitted are generally very hard to recover from.
That's why the kernel should do whatever it takes to recover from
such failures, and user-space should do whatever it takes to recover
from copy-to-user failures of needed info from the kernel, and it
really depends on the user-space usage pattern of "presumed". IIRC
the original reason for copying it back to user-space was, that if a
relocation offsets were patched up by the kernel, and then the
process was sent a signal causing it to retry execbuf, then
"presumed" had to be updated, otherwise it would be inconsistent
with what's currently in the command stream, which is very bad. If
"presumed" is, however, only used by user-space to guess an offset,
the correct action would be to swallow the -EFAULT.
In i915 we've had tons of fun with a regression in 3.7 where exactly this
blows up: Some of our userspace (UXA ddx specifically) retains
relocations-trees partially accross an execbuf. Which means if the kernel
updates the relocations it _must_ update the presumed offset for
otherwise things will go haywire on the next execbuf. So we can't return
-EFAULT if the userspace memory needs
Re: [Intel-gfx] [RFC PATCH] drm/nouveau: fix nested locking in mmap handler
On 09/24/2013 09:34 AM, Maarten Lankhorst wrote:
Op 24-09-13 09:22, Thomas Hellstrom schreef:
On 09/23/2013 05:33 PM, Maarten Lankhorst wrote:
Hey,
Op 13-09-13 11:00, Peter Zijlstra schreef:
On Fri, Sep 13, 2013 at 10:41:54AM +0200, Daniel Vetter wrote:
On Fri, Sep 13, 2013 at 10:29 AM, Peter Zijlstra wrote:
On Fri, Sep 13, 2013 at 09:46:03AM +0200, Thomas Hellstrom wrote:
if (!bo_tryreserve()) {
up_read mmap_sem(); // Release the mmap_sem to avoid deadlocks.
bo_reserve(); // Wait for the BO to become available
(interruptible)
bo_unreserve(); // Where is bo_wait_unreserved() when we need
it, Maarten :P
return VM_FAULT_RETRY; // Go ahead and retry the VMA walk, after
regrabbing
}
Anyway, could you describe what is wrong, with the above solution, because
it seems perfectly legal to me.
Luckily the rule of law doesn't have anything to do with this stuff --
at least I sincerely hope so.
The thing that's wrong with that pattern is that its still not
deterministic - although its a lot better than the pure trylock. Because
you have to release and re-acquire with the trylock another user might
have gotten in again. Its utterly prone to starvation.
The acquire+release does remove the dead/life-lock scenario from the
FIFO case, since blocking on the acquire will allow the other task to
run (or even get boosted on -rt).
Aside from that there's nothing particularly wrong with it and lockdep
should be happy afaict (but I haven't had my morning juice yet).
bo_reserve internally maps to a ww-mutex and task can already hold
ww-mutex (potentially even the same for especially nasty userspace).
OK, yes I wasn't aware of that. Yes in that case you're quite right.
I added a RFC patch below. I only tested with PROVE_LOCKING, and always forced
the slowpath for debugging.
This fixes nouveau and core ttm to always use blocking acquisition in fastpath.
Nouveau was a bit of a headache, but afaict it should work.
In almost all cases relocs are not updated, so I kept intact the fastpath
of not copying relocs from userspace. The slowpath tries to copy it atomically,
and if that fails it will unreserve all bo's and copy everything.
One thing to note is that the command submission ioctl may fail now with -EFAULT
if presumed cannot be updated, while the commands are submitted succesfully.
I think the Nouveau guys need to comment further on this, but returning -EFAULT might
break existing user-space, and that's not allowed, but IIRC the return value of
"presumed" is only a hint, and if it's incorrect will only trigger future
command stream patching.
Otherwise reviewing mostly the TTM stuff. FWIW, from wat I can tell the vmwgfx
driver doesn't need any fixups.
Well because we read the list of buffer objects the presumed offsets are at
least read-mapped. Although I guess in the worst case the mapping might
disappear before the syscall copies back the data.
So if -EFAULT happens here then userspace messed up in some way, either by
forgetting to map the offsets read-write, which cannot happen with libdrm or
free'ing the bo list before the syscall returns,
which would probably result in libdrm crashing shortly afterwards anyway.
Hmm, is the list of buffer objects (and the "presumed" members) really
in DRM memory? Because if it resides or may reside in anonymous system
memory, it may well be paged out between reading and writing, in which
case the -EFAULT return is incorrect.
In fact failures of pushbuf / execbuf *after* commands are successfully
submitted are generally very hard to recover from. That's why the kernel
should do whatever it takes to recover from such failures, and
user-space should do whatever it takes to recover from copy-to-user
failures of needed info from the kernel, and it really depends on the
user-space usage pattern of "presumed". IIRC the original reason for
copying it back to user-space was, that if a relocation offsets were
patched up by the kernel, and then the process was sent a signal causing
it to retry execbuf, then "presumed" had to be updated, otherwise it
would be inconsistent with what's currently in the command stream, which
is very bad. If "presumed" is, however, only used by user-space to guess
an offset, the correct action would be to swallow the -EFAULT.
So I don't know whether to swallow that -EFAULT or not, which is what I asked.
And for vmwgfx just run it through PROVE_LOCKING and make sure all the
copy_to/from_user call sites are called at least once, and then you can be
certain it doesn't need fixups. ;)
Lockdep ftw..
Will do that.
Actually, it's not the locking order bo:reserve -> mmap_sem that triggers the
lockdep warning, right? but the fact that copy_from_user could recurse into the
fault handler? Grabbing bo:reseves recursively? which should leave u
Re: [Intel-gfx] [RFC PATCH] drm/nouveau: fix nested locking in mmap handler
On 09/23/2013 05:33 PM, Maarten Lankhorst wrote:
Hey,
Op 13-09-13 11:00, Peter Zijlstra schreef:
On Fri, Sep 13, 2013 at 10:41:54AM +0200, Daniel Vetter wrote:
On Fri, Sep 13, 2013 at 10:29 AM, Peter Zijlstra wrote:
On Fri, Sep 13, 2013 at 09:46:03AM +0200, Thomas Hellstrom wrote:
if (!bo_tryreserve()) {
up_read mmap_sem(); // Release the mmap_sem to avoid deadlocks.
bo_reserve(); // Wait for the BO to become available
(interruptible)
bo_unreserve(); // Where is bo_wait_unreserved() when we need
it, Maarten :P
return VM_FAULT_RETRY; // Go ahead and retry the VMA walk, after regrabbing
}
Anyway, could you describe what is wrong, with the above solution, because
it seems perfectly legal to me.
Luckily the rule of law doesn't have anything to do with this stuff --
at least I sincerely hope so.
The thing that's wrong with that pattern is that its still not
deterministic - although its a lot better than the pure trylock. Because
you have to release and re-acquire with the trylock another user might
have gotten in again. Its utterly prone to starvation.
The acquire+release does remove the dead/life-lock scenario from the
FIFO case, since blocking on the acquire will allow the other task to
run (or even get boosted on -rt).
Aside from that there's nothing particularly wrong with it and lockdep
should be happy afaict (but I haven't had my morning juice yet).
bo_reserve internally maps to a ww-mutex and task can already hold
ww-mutex (potentially even the same for especially nasty userspace).
OK, yes I wasn't aware of that. Yes in that case you're quite right.
I added a RFC patch below. I only tested with PROVE_LOCKING, and always forced
the slowpath for debugging.
This fixes nouveau and core ttm to always use blocking acquisition in fastpath.
Nouveau was a bit of a headache, but afaict it should work.
In almost all cases relocs are not updated, so I kept intact the fastpath
of not copying relocs from userspace. The slowpath tries to copy it atomically,
and if that fails it will unreserve all bo's and copy everything.
One thing to note is that the command submission ioctl may fail now with -EFAULT
if presumed cannot be updated, while the commands are submitted succesfully.
I think the Nouveau guys need to comment further on this, but returning
-EFAULT might break existing user-space, and that's not allowed, but
IIRC the return value of "presumed" is only a hint, and if it's
incorrect will only trigger future command stream patching.
Otherwise reviewing mostly the TTM stuff. FWIW, from wat I can tell the
vmwgfx driver doesn't need any fixups.
I'm not sure what the right behavior was here, and this can only happen if you
touch the memory during the ioctl or use a read-only page. Both of them are not
done
in the common case.
Reviews welcome. :P
8<---
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c
b/drivers/gpu/drm/nouveau/nouveau_gem.c
index e4d60e7..2964bb7 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -445,8 +445,6 @@ validate_list(struct nouveau_channel *chan, struct
nouveau_cli *cli,
uint64_t user_pbbo_ptr)
{
struct nouveau_drm *drm = chan->drm;
- struct drm_nouveau_gem_pushbuf_bo __user *upbbo =
- (void __force __user *)(uintptr_t)user_pbbo_ptr;
struct nouveau_bo *nvbo;
int ret, relocs = 0;
@@ -475,7 +473,7 @@ validate_list(struct nouveau_channel *chan, struct nouveau_cli *cli,
return ret;
}
- if (nv_device(drm->device)->card_type < NV_50) {
+ if (nv_device(drm->device)->card_type < NV_50 && !relocs) {
if (nvbo->bo.offset == b->presumed.offset &&
((nvbo->bo.mem.mem_type == TTM_PL_VRAM &&
b->presumed.domain & NOUVEAU_GEM_DOMAIN_VRAM) ||
@@ -483,53 +481,86 @@ validate_list(struct nouveau_channel *chan, struct
nouveau_cli *cli,
b->presumed.domain & NOUVEAU_GEM_DOMAIN_GART)))
continue;
- if (nvbo->bo.mem.mem_type == TTM_PL_TT)
- b->presumed.domain = NOUVEAU_GEM_DOMAIN_GART;
- else
- b->presumed.domain = NOUVEAU_GEM_DOMAIN_VRAM;
- b->presumed.offset = nvbo->bo.offset;
- b->presumed.valid = 0;
- relocs++;
-
- if (DRM_COPY_TO_USER(&upbbo[nvbo->pbbo_index].presumed,
-&b->presumed, sizeof(b->presumed)))
- return -EFAULT;
+ relocs = 1;
}
}
Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE
On 09/13/2013 10:58 AM, Maarten Lankhorst wrote:
Op 13-09-13 10:23, Thomas Hellstrom schreef:
On 09/13/2013 09:51 AM, Maarten Lankhorst wrote:
Op 13-09-13 09:46, Thomas Hellstrom schreef:
On 09/13/2013 09:16 AM, Maarten Lankhorst wrote:
Op 13-09-13 08:44, Thomas Hellstrom schreef:
On 09/12/2013 11:50 PM, Maarten Lankhorst wrote:
Op 12-09-13 18:44, Thomas Hellstrom schreef:
On 09/12/2013 05:45 PM, Maarten Lankhorst wrote:
Op 12-09-13 17:36, Daniel Vetter schreef:
On Thu, Sep 12, 2013 at 5:06 PM, Peter Zijlstra wrote:
So I'm poking around the preemption code and stumbled upon:
drivers/gpu/drm/i915/i915_gem.c:set_need_resched();
drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
drivers/gpu/drm/udl/udl_gem.c: set_need_resched();
All these sites basically do:
while (!trylock())
yield();
which is a horrible and broken locking pattern.
Firstly its deadlock prone, suppose the faulting process is a FIFOn+1
task that preempted the lock holder at FIFOn.
Secondly the implementation is worse than usual by abusing
VM_FAULT_NOPAGE, which is supposed to install a PTE so that the fault
doesn't retry, but you're using it as a get out of fault path. And
you're using set_need_resched() which is not something a driver should
_ever_ touch.
Now I'm going to take away set_need_resched() -- and while you can
'reimplement' it using set_thread_flag() you're not going to do that
because it will be broken due to changes to the preempt code.
So please as to fix ASAP and don't allow anybody to trick you into
merging silly things like that again ;-)
The set_need_resched in i915_gem.c:i915_gem_fault can actually be
removed. It was there to give the error handler a chance to sneak in
and reset the hw/sw tracking when the gpu is dead. That hack goes back
to the days when the locking around our error handler was somewhere
between nonexistent and totally broken, nowadays we keep things from
live-locking by a bit of magic in i915_mutex_lock_interruptible. I'll
whip up a patch to rip this out. I'll also check that our testsuite
properly exercises this path (needs a bit of work on a quick look for
better coverage).
The one in ttm is just bonghits to shut up lockdep: ttm can recurse
into it's own pagefault handler and then deadlock, the trylock just
keeps lockdep quiet. We've had that bug arise in drm/i915 due to some
fun userspace did and now have testcases for them. The right solution
to fix this is to use copy_to|from_user_atomic in ttm everywhere it
holds locks and have slowpaths which drops locks, copies stuff into a
temp allocation and then continues. At least that's how we've fixed
all those inversions in i915-gem. I'm not volunteering to fix this ;-)
Ah the case where a mmap'd address is passed to the execbuf ioctl? :P
Fine I'll look into it a bit, hopefully before tuesday. Else it might take a
bit longer since I'll be on my way to plumbers..
I think a possible fix would be if fault() were allowed to return an error and
drop the mmap_sem() before returning.
Otherwise we need to track down all copy_to_user / copy_from_user which happen
with bo::reserve held.
Actually, from looking at the mm code, it seems OK to do the following:
if (!bo_tryreserve()) {
up_read mmap_sem(); // Release the mmap_sem to avoid deadlocks.
bo_reserve(); // Wait for the BO to become available
(interruptible)
bo_unreserve(); // Where is bo_wait_unreserved() when we need
it, Maarten :P
return VM_FAULT_RETRY; // Go ahead and retry the VMA walk, after
regrabbing
}
Is this meant as a jab at me? You're doing locking wrong here! Again!
It's not meant as a jab at you. I'm sorry if it came out that way. It was
meant as a joke. I wasn't aware the topic was sensitive.
Anyway, could you describe what is wrong, with the above solution, because it
seems perfectly legal to me.
There is no substantial overhead, and there is no risc of deadlocks. Or do you
mean it's bad because it confuses lockdep?
Evil userspace can pass a bo as pointer to use for relocation lists, lockdep
will warn when that locks up, but still..
This is already a problem now, and your fixing will only cause lockdep to
explicitly warn on it.
As previously mentioned, copy_from_user should return -EFAULT, since the VMAs
are marked with VM_IO. It should not recurse into fault(), so evil user-space
looses.
You can make a complicated user program to test this, or simply use this
function for debugging:
void ttm_might_fault(void) { struct reservation_object obj; reservation_object_init(&obj);
ww_mutex_lock(&obj.lock, NULL); ww_mutex_unlock(&obj.lock);
reservation_object_fini(&obj); }
Put it near every instance of copy_to_user/copy_fr
Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE
On 09/13/2013 10:32 AM, Daniel Vetter wrote: On Fri, Sep 13, 2013 at 10:23 AM, Thomas Hellstrom wrote: As previously mentioned, copy_from_user should return -EFAULT, since the VMAs are marked with VM_IO. It should not recurse into fault(), so evil user-space looses. I haven't put a printk in the code to prove this, but gem mmap also sets VM_IO in drm_gem_mmap_obj. And we can very much hit our own fault handler and deadlock If this is indeed true, I guess I need to accept the fact that my solution is bad. (and worse I can't blame not having my morning coffee). I'll take a deeper look. /Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE
On 09/13/2013 09:51 AM, Maarten Lankhorst wrote:
Op 13-09-13 09:46, Thomas Hellstrom schreef:
On 09/13/2013 09:16 AM, Maarten Lankhorst wrote:
Op 13-09-13 08:44, Thomas Hellstrom schreef:
On 09/12/2013 11:50 PM, Maarten Lankhorst wrote:
Op 12-09-13 18:44, Thomas Hellstrom schreef:
On 09/12/2013 05:45 PM, Maarten Lankhorst wrote:
Op 12-09-13 17:36, Daniel Vetter schreef:
On Thu, Sep 12, 2013 at 5:06 PM, Peter Zijlstra wrote:
So I'm poking around the preemption code and stumbled upon:
drivers/gpu/drm/i915/i915_gem.c:set_need_resched();
drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
drivers/gpu/drm/udl/udl_gem.c: set_need_resched();
All these sites basically do:
while (!trylock())
yield();
which is a horrible and broken locking pattern.
Firstly its deadlock prone, suppose the faulting process is a FIFOn+1
task that preempted the lock holder at FIFOn.
Secondly the implementation is worse than usual by abusing
VM_FAULT_NOPAGE, which is supposed to install a PTE so that the fault
doesn't retry, but you're using it as a get out of fault path. And
you're using set_need_resched() which is not something a driver should
_ever_ touch.
Now I'm going to take away set_need_resched() -- and while you can
'reimplement' it using set_thread_flag() you're not going to do that
because it will be broken due to changes to the preempt code.
So please as to fix ASAP and don't allow anybody to trick you into
merging silly things like that again ;-)
The set_need_resched in i915_gem.c:i915_gem_fault can actually be
removed. It was there to give the error handler a chance to sneak in
and reset the hw/sw tracking when the gpu is dead. That hack goes back
to the days when the locking around our error handler was somewhere
between nonexistent and totally broken, nowadays we keep things from
live-locking by a bit of magic in i915_mutex_lock_interruptible. I'll
whip up a patch to rip this out. I'll also check that our testsuite
properly exercises this path (needs a bit of work on a quick look for
better coverage).
The one in ttm is just bonghits to shut up lockdep: ttm can recurse
into it's own pagefault handler and then deadlock, the trylock just
keeps lockdep quiet. We've had that bug arise in drm/i915 due to some
fun userspace did and now have testcases for them. The right solution
to fix this is to use copy_to|from_user_atomic in ttm everywhere it
holds locks and have slowpaths which drops locks, copies stuff into a
temp allocation and then continues. At least that's how we've fixed
all those inversions in i915-gem. I'm not volunteering to fix this ;-)
Ah the case where a mmap'd address is passed to the execbuf ioctl? :P
Fine I'll look into it a bit, hopefully before tuesday. Else it might take a
bit longer since I'll be on my way to plumbers..
I think a possible fix would be if fault() were allowed to return an error and
drop the mmap_sem() before returning.
Otherwise we need to track down all copy_to_user / copy_from_user which happen
with bo::reserve held.
Actually, from looking at the mm code, it seems OK to do the following:
if (!bo_tryreserve()) {
up_read mmap_sem(); // Release the mmap_sem to avoid deadlocks.
bo_reserve(); // Wait for the BO to become available
(interruptible)
bo_unreserve(); // Where is bo_wait_unreserved() when we need
it, Maarten :P
return VM_FAULT_RETRY; // Go ahead and retry the VMA walk, after
regrabbing
}
Is this meant as a jab at me? You're doing locking wrong here! Again!
It's not meant as a jab at you. I'm sorry if it came out that way. It was
meant as a joke. I wasn't aware the topic was sensitive.
Anyway, could you describe what is wrong, with the above solution, because it
seems perfectly legal to me.
There is no substantial overhead, and there is no risc of deadlocks. Or do you
mean it's bad because it confuses lockdep?
Evil userspace can pass a bo as pointer to use for relocation lists, lockdep
will warn when that locks up, but still..
This is already a problem now, and your fixing will only cause lockdep to
explicitly warn on it.
As previously mentioned, copy_from_user should return -EFAULT, since the
VMAs are marked with VM_IO. It should not recurse into fault(), so evil
user-space looses.
You can make a complicated user program to test this, or simply use this
function for debugging:
void ttm_might_fault(void) { struct reservation_object obj; reservation_object_init(&obj);
ww_mutex_lock(&obj.lock, NULL); ww_mutex_unlock(&obj.lock);
reservation_object_fini(&obj); }
Put it near every instance of copy_to_user/copy_from_user and you'll find the
bugs. :)
I'm still not convinced that there are any pro
Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE
On 09/13/2013 09:16 AM, Maarten Lankhorst wrote:
Op 13-09-13 08:44, Thomas Hellstrom schreef:
On 09/12/2013 11:50 PM, Maarten Lankhorst wrote:
Op 12-09-13 18:44, Thomas Hellstrom schreef:
On 09/12/2013 05:45 PM, Maarten Lankhorst wrote:
Op 12-09-13 17:36, Daniel Vetter schreef:
On Thu, Sep 12, 2013 at 5:06 PM, Peter Zijlstra wrote:
So I'm poking around the preemption code and stumbled upon:
drivers/gpu/drm/i915/i915_gem.c:set_need_resched();
drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
drivers/gpu/drm/udl/udl_gem.c: set_need_resched();
All these sites basically do:
while (!trylock())
yield();
which is a horrible and broken locking pattern.
Firstly its deadlock prone, suppose the faulting process is a FIFOn+1
task that preempted the lock holder at FIFOn.
Secondly the implementation is worse than usual by abusing
VM_FAULT_NOPAGE, which is supposed to install a PTE so that the fault
doesn't retry, but you're using it as a get out of fault path. And
you're using set_need_resched() which is not something a driver should
_ever_ touch.
Now I'm going to take away set_need_resched() -- and while you can
'reimplement' it using set_thread_flag() you're not going to do that
because it will be broken due to changes to the preempt code.
So please as to fix ASAP and don't allow anybody to trick you into
merging silly things like that again ;-)
The set_need_resched in i915_gem.c:i915_gem_fault can actually be
removed. It was there to give the error handler a chance to sneak in
and reset the hw/sw tracking when the gpu is dead. That hack goes back
to the days when the locking around our error handler was somewhere
between nonexistent and totally broken, nowadays we keep things from
live-locking by a bit of magic in i915_mutex_lock_interruptible. I'll
whip up a patch to rip this out. I'll also check that our testsuite
properly exercises this path (needs a bit of work on a quick look for
better coverage).
The one in ttm is just bonghits to shut up lockdep: ttm can recurse
into it's own pagefault handler and then deadlock, the trylock just
keeps lockdep quiet. We've had that bug arise in drm/i915 due to some
fun userspace did and now have testcases for them. The right solution
to fix this is to use copy_to|from_user_atomic in ttm everywhere it
holds locks and have slowpaths which drops locks, copies stuff into a
temp allocation and then continues. At least that's how we've fixed
all those inversions in i915-gem. I'm not volunteering to fix this ;-)
Ah the case where a mmap'd address is passed to the execbuf ioctl? :P
Fine I'll look into it a bit, hopefully before tuesday. Else it might take a
bit longer since I'll be on my way to plumbers..
I think a possible fix would be if fault() were allowed to return an error and
drop the mmap_sem() before returning.
Otherwise we need to track down all copy_to_user / copy_from_user which happen
with bo::reserve held.
Actually, from looking at the mm code, it seems OK to do the following:
if (!bo_tryreserve()) {
up_read mmap_sem(); // Release the mmap_sem to avoid deadlocks.
bo_reserve(); // Wait for the BO to become available
(interruptible)
bo_unreserve(); // Where is bo_wait_unreserved() when we need
it, Maarten :P
return VM_FAULT_RETRY; // Go ahead and retry the VMA walk, after regrabbing
}
Is this meant as a jab at me? You're doing locking wrong here! Again!
It's not meant as a jab at you. I'm sorry if it came out that way. It
was meant as a joke. I wasn't aware the topic was sensitive.
Anyway, could you describe what is wrong, with the above solution,
because it seems perfectly legal to me.
There is no substantial overhead, and there is no risc of deadlocks. Or
do you mean it's bad because it confuses lockdep?
/Thomas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE
On 09/12/2013 11:50 PM, Maarten Lankhorst wrote:
Op 12-09-13 18:44, Thomas Hellstrom schreef:
On 09/12/2013 05:45 PM, Maarten Lankhorst wrote:
Op 12-09-13 17:36, Daniel Vetter schreef:
On Thu, Sep 12, 2013 at 5:06 PM, Peter Zijlstra wrote:
So I'm poking around the preemption code and stumbled upon:
drivers/gpu/drm/i915/i915_gem.c:set_need_resched();
drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
drivers/gpu/drm/udl/udl_gem.c: set_need_resched();
All these sites basically do:
while (!trylock())
yield();
which is a horrible and broken locking pattern.
Firstly its deadlock prone, suppose the faulting process is a FIFOn+1
task that preempted the lock holder at FIFOn.
Secondly the implementation is worse than usual by abusing
VM_FAULT_NOPAGE, which is supposed to install a PTE so that the fault
doesn't retry, but you're using it as a get out of fault path. And
you're using set_need_resched() which is not something a driver should
_ever_ touch.
Now I'm going to take away set_need_resched() -- and while you can
'reimplement' it using set_thread_flag() you're not going to do that
because it will be broken due to changes to the preempt code.
So please as to fix ASAP and don't allow anybody to trick you into
merging silly things like that again ;-)
The set_need_resched in i915_gem.c:i915_gem_fault can actually be
removed. It was there to give the error handler a chance to sneak in
and reset the hw/sw tracking when the gpu is dead. That hack goes back
to the days when the locking around our error handler was somewhere
between nonexistent and totally broken, nowadays we keep things from
live-locking by a bit of magic in i915_mutex_lock_interruptible. I'll
whip up a patch to rip this out. I'll also check that our testsuite
properly exercises this path (needs a bit of work on a quick look for
better coverage).
The one in ttm is just bonghits to shut up lockdep: ttm can recurse
into it's own pagefault handler and then deadlock, the trylock just
keeps lockdep quiet. We've had that bug arise in drm/i915 due to some
fun userspace did and now have testcases for them. The right solution
to fix this is to use copy_to|from_user_atomic in ttm everywhere it
holds locks and have slowpaths which drops locks, copies stuff into a
temp allocation and then continues. At least that's how we've fixed
all those inversions in i915-gem. I'm not volunteering to fix this ;-)
Ah the case where a mmap'd address is passed to the execbuf ioctl? :P
Fine I'll look into it a bit, hopefully before tuesday. Else it might take a
bit longer since I'll be on my way to plumbers..
I think a possible fix would be if fault() were allowed to return an error and
drop the mmap_sem() before returning.
Otherwise we need to track down all copy_to_user / copy_from_user which happen
with bo::reserve held.
Actually, from looking at the mm code, it seems OK to do the following:
if (!bo_tryreserve()) {
up_read mmap_sem(); // Release the mmap_sem to avoid deadlocks.
bo_reserve(); // Wait for the BO to become available
(interruptible)
bo_unreserve(); // Where is bo_wait_unreserved() when we
need it, Maarten :P
return VM_FAULT_RETRY; // Go ahead and retry the VMA walk, after
regrabbing
}
Somebody conveniently added a VM_FAULT_RETRY, but for a different purpose.
If possible, I suggest to take this route for now to avoid the mess of
changing locking order in all TTM drivers, with
all give-up-locking slowpaths that comes with it. IIRC it took some time
for i915 to get that right, and completely get rid of all lockdep warnings.
This will keep the official locking order
bo::reserve
mmap_sem
/Thomas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE
On 09/12/2013 11:50 PM, Maarten Lankhorst wrote: Op 12-09-13 18:44, Thomas Hellstrom schreef: On 09/12/2013 05:45 PM, Maarten Lankhorst wrote: Op 12-09-13 17:36, Daniel Vetter schreef: On Thu, Sep 12, 2013 at 5:06 PM, Peter Zijlstra wrote: So I'm poking around the preemption code and stumbled upon: drivers/gpu/drm/i915/i915_gem.c:set_need_resched(); drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched(); drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched(); drivers/gpu/drm/udl/udl_gem.c: set_need_resched(); All these sites basically do: while (!trylock()) yield(); which is a horrible and broken locking pattern. Firstly its deadlock prone, suppose the faulting process is a FIFOn+1 task that preempted the lock holder at FIFOn. Secondly the implementation is worse than usual by abusing VM_FAULT_NOPAGE, which is supposed to install a PTE so that the fault doesn't retry, but you're using it as a get out of fault path. And you're using set_need_resched() which is not something a driver should _ever_ touch. Now I'm going to take away set_need_resched() -- and while you can 'reimplement' it using set_thread_flag() you're not going to do that because it will be broken due to changes to the preempt code. So please as to fix ASAP and don't allow anybody to trick you into merging silly things like that again ;-) The set_need_resched in i915_gem.c:i915_gem_fault can actually be removed. It was there to give the error handler a chance to sneak in and reset the hw/sw tracking when the gpu is dead. That hack goes back to the days when the locking around our error handler was somewhere between nonexistent and totally broken, nowadays we keep things from live-locking by a bit of magic in i915_mutex_lock_interruptible. I'll whip up a patch to rip this out. I'll also check that our testsuite properly exercises this path (needs a bit of work on a quick look for better coverage). The one in ttm is just bonghits to shut up lockdep: ttm can recurse into it's own pagefault handler and then deadlock, the trylock just keeps lockdep quiet. We've had that bug arise in drm/i915 due to some fun userspace did and now have testcases for them. The right solution to fix this is to use copy_to|from_user_atomic in ttm everywhere it holds locks and have slowpaths which drops locks, copies stuff into a temp allocation and then continues. At least that's how we've fixed all those inversions in i915-gem. I'm not volunteering to fix this ;-) Ah the case where a mmap'd address is passed to the execbuf ioctl? :P Fine I'll look into it a bit, hopefully before tuesday. Else it might take a bit longer since I'll be on my way to plumbers.. I think a possible fix would be if fault() were allowed to return an error and drop the mmap_sem() before returning. Otherwise we need to track down all copy_to_user / copy_from_user which happen with bo::reserve held. CONFIG_PROVE_LOCKING=y and hard grab that reserve lock within the fault handler, done.. lockdep will spit it out for you :p ~Maarten Given that all copy_to_user / copy_from_user paths are actually hit during testing, right? /Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE
On 09/12/2013 10:39 PM, Thomas Gleixner wrote: On Thu, 12 Sep 2013, Daniel Vetter wrote: On Thu, Sep 12, 2013 at 10:20 PM, Thomas Gleixner wrote: I think for ttm drivers it's just execbuf being exploitable. But on drm/i915 we've had the same issue with the pwrite/pread ioctls, so a simple glBufferData(glMap) kind of recursion from gl clients blew the kernel to pieces ... And the only answer you folks came up with is set_need_resched() and yield()? Oh well The yield was for a different lifelock, and that one is also fixed by now. The fault handler deadlock was fixed in the usual "drop locks and jump into slowpath" fasion, at least in drm/i915. So we can remove that whole yield/set_need_resched() mess completely ? Thanks, tglx No. The while(trylock) is there to address a potential locking inversion deadlock. If the trylock fails, the code returns to user-space which retries the fault. This code needs to stay until we can come up with either a way to drop the mmap_sem and sleep before returning to user-space, or a bunch of code is fixed with a different locking order. The set_need_resched() can (and should according to Peter) go. /Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE
On 09/12/2013 05:45 PM, Maarten Lankhorst wrote: Op 12-09-13 17:36, Daniel Vetter schreef: On Thu, Sep 12, 2013 at 5:06 PM, Peter Zijlstra wrote: So I'm poking around the preemption code and stumbled upon: drivers/gpu/drm/i915/i915_gem.c:set_need_resched(); drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched(); drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched(); drivers/gpu/drm/udl/udl_gem.c: set_need_resched(); All these sites basically do: while (!trylock()) yield(); which is a horrible and broken locking pattern. Firstly its deadlock prone, suppose the faulting process is a FIFOn+1 task that preempted the lock holder at FIFOn. Secondly the implementation is worse than usual by abusing VM_FAULT_NOPAGE, which is supposed to install a PTE so that the fault doesn't retry, but you're using it as a get out of fault path. And you're using set_need_resched() which is not something a driver should _ever_ touch. Now I'm going to take away set_need_resched() -- and while you can 'reimplement' it using set_thread_flag() you're not going to do that because it will be broken due to changes to the preempt code. So please as to fix ASAP and don't allow anybody to trick you into merging silly things like that again ;-) The set_need_resched in i915_gem.c:i915_gem_fault can actually be removed. It was there to give the error handler a chance to sneak in and reset the hw/sw tracking when the gpu is dead. That hack goes back to the days when the locking around our error handler was somewhere between nonexistent and totally broken, nowadays we keep things from live-locking by a bit of magic in i915_mutex_lock_interruptible. I'll whip up a patch to rip this out. I'll also check that our testsuite properly exercises this path (needs a bit of work on a quick look for better coverage). The one in ttm is just bonghits to shut up lockdep: ttm can recurse into it's own pagefault handler and then deadlock, the trylock just keeps lockdep quiet. We've had that bug arise in drm/i915 due to some fun userspace did and now have testcases for them. The right solution to fix this is to use copy_to|from_user_atomic in ttm everywhere it holds locks and have slowpaths which drops locks, copies stuff into a temp allocation and then continues. At least that's how we've fixed all those inversions in i915-gem. I'm not volunteering to fix this ;-) Ah the case where a mmap'd address is passed to the execbuf ioctl? :P Fine I'll look into it a bit, hopefully before tuesday. Else it might take a bit longer since I'll be on my way to plumbers.. I think a possible fix would be if fault() were allowed to return an error and drop the mmap_sem() before returning. Otherwise we need to track down all copy_to_user / copy_from_user which happen with bo::reserve held. /Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE
On 09/12/2013 05:58 PM, Daniel Vetter wrote: On Thu, Sep 12, 2013 at 5:43 PM, Peter Zijlstra wrote: The one in ttm is just bonghits to shut up lockdep: ttm can recurse into it's own pagefault handler and then deadlock, the trylock just keeps lockdep quiet. Could you describe how it could recurse into it's own pagefault handler? IIRC the VM flags of the TTM VMAs makes get_user_pages() refrain from touching these VMAs, hence I don't think this code can deadlock, but admittedly it's far from the optimal solution. Never mind, more on the set_need_resched() below. We've had that bug arise in drm/i915 due to some fun userspace did and now have testcases for them. The right solution to fix this is to use copy_to|from_user_atomic in ttm everywhere it holds locks and have slowpaths which drops locks, copies stuff into a temp allocation and then continues. At least that's how we've fixed all those inversions in i915-gem. I'm not volunteering to fix this ;-) Yikes.. so how common is it? If I simply rip the set_need_resched() out it will 'spin' on the fault a little longer until a 'natural' preemption point -- if such a thing is every going to happen. A typical case is if a process is throwing out a buffer from the GPU or system memory while another process pagefaults while writing to it. It's not a common situation, and it's by no means a fastpath situation. For correctness purposes, I think set_need_resched() can be safely removed. It's a case of "our userspace doesn't do this", so as long as you're not evil and frob the drm device nodes of ttm drivers directly the deadlock will never happen. No idea how much contention actually happens on e.g. shared buffer objects - in i915 we have just one lock and so suffer quite a bit more from contention. So no idea how much removing the yield would hurt. -Daniel /Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
