[Intel-xe] [PATCH] drm/xe: Use atomic instead of mutex for xe_device_mem_access_ongoing
Lucas De Marchi
lucas.demarchi at intel.com
Fri Mar 10 21:05:11 UTC 2023
On Thu, Mar 02, 2023 at 05:33:55PM -0500, Rodrigo Vivi wrote:
>On Thu, Mar 02, 2023 at 12:26:18PM +0100, Maarten Lankhorst wrote:
>>
>> On 2023-03-02 00:14, Rodrigo Vivi wrote:
>> > On Wed, Mar 01, 2023 at 08:36:29AM -0800, Lucas De Marchi wrote:
>> > > On Tue, Feb 28, 2023 at 11:17:30AM +0100, Maarten Lankhorst wrote:
>> > > > xe_guc_ct_fast_path() is called from an irq context, and cannot lock
>> > > > the mutex used by xe_device_mem_access_ongoing().
>> > > >
>> > > > Fortunately it is easy to fix, and the atomic guarantees are good enough
>> > > > to ensure xe->mem_access.hold_rpm is set before last ref is dropped.
>> > > >
>> > > > As far as I can tell, the runtime ref in device access should be
>> > > > killable, but don't dare to do it yet.
>> > > I don't follow this last paragraph. Could you point it in the code?
>> > I also didn't understand this... if we remove that we will end up in
>> > memory access with the sleeping device...
>> I may understand the code wrong, but without error checking by the callers,
>> and changing the prototype to return int is there any way this will be
>> guaranteed to work regardless?
>> > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> > > > ---
>> > > > drivers/gpu/drm/xe/xe_device.c | 17 ++++++++---------
>> > > > drivers/gpu/drm/xe/xe_device.h | 14 ++++----------
>> > > > drivers/gpu/drm/xe/xe_device_types.h | 4 +---
>> > > > 3 files changed, 13 insertions(+), 22 deletions(-)
>> > > >
>> > > > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>> > > > index 4eb6786b11f0..ab179b1e24c1 100644
>> > > > --- a/drivers/gpu/drm/xe/xe_device.c
>> > > > +++ b/drivers/gpu/drm/xe/xe_device.c
>> > > > @@ -237,7 +237,6 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
>> > > > if (err)
>> > > > goto err;
>> > > >
>> > > > - mutex_init(&xe->mem_access.lock);
>> > > > return xe;
>> > > >
>> > > > err_put:
>> > > > @@ -424,25 +423,25 @@ u32 xe_device_ccs_bytes(struct xe_device *xe, u64 size)
>> > > > void xe_device_mem_access_get(struct xe_device *xe)
>> > > > {
>> > > > bool resumed = xe_pm_runtime_resume_if_suspended(xe);
>> > > > + int ref = atomic_inc_return(&xe->mem_access.ref);
>> > >
>> > > +Matt Brost
>> > >
>> > > Any reason for not using kref?
>> > hmmm... my bad actually...
>> >
>> > I did considered the kref, but I can't remember why I haven't used it.
>> > I recently was asking myself the same question.
>>
>> I looked at it, I don't think you can kref from 0 to 1 by design.
>>
>> xe_device_mem_access_get() is usually called with force wake held explicitly
>> or implicitly, so we shouldn't need the runtime pm ref there.
>
>But this is one of the main problems in i915 right now on why we can't
>enable the D3Cold. If we have any possibility of memory transaction we
>need to first wake up the device, then we need to proceed with the memory
>accesses. Or the bridges might be sleeping and returning 0xfffff
>
>so, the first one to take the very first mem_access ref will also ensure
>that the device is awake before proceeding. This works now without any
>error handling by the caller.
>
>Another way to do would probably need to involve a queue for every
>memory access or like i915 was going where there's a list...
>
>I wonder if making this spin_lock isn't enough to fix the current issue...
From what I can see it would be ok. However, how is this reproduced?
The only way I can see a lockdep complain is on a system with a PVC and
another card using the ast module. In that system we have the following
splat:
1678477803.564451 ======================================================
1678477803.564641 WARNING: possible circular locking dependency detected
1678477803.564797 6.1.0-xe-demarchi+ #34 Not tainted
1678477803.564986 ------------------------------------------------------
1678477803.565220 kworker/28:1/316 is trying to acquire lock:
1678477803.565429 ffff8881ccf8ea48 (lock#7){+.+.}-{3:3}, at: xe_device_mem_access_get+0x2f/0xc0 [xe]
1678477803.565585
but task is already holding lock:
1678477803.565762 ffff8881ccf83ed8 (&ct->lock){+.+.}-{3:3}, at: xe_guc_ct_send+0x2c/0x80 [xe]
1678477803.565921
which lock already depends on the new lock.
1678477803.566149
the existing dependency chain (in reverse order) is:
1678477803.566347
-> #2 (&ct->lock){+.+.}-{3:3}:
1678477803.566505 lock_acquire+0x169/0x410
1678477804.447764 xe_guc_ct_init+0x19b/0x260 [xe]
1678477804.448706 xe_guc_init+0x88/0x570 [xe]
1678477804.449251 xe_uc_init+0x4c/0xc0 [xe]
1678477804.450514 xe_gt_init+0x1be/0x490 [xe]
1678477804.451031 xe_device_probe+0x2d8/0x350 [xe]
1678477804.451724 xe_pci_probe+0x770/0x8d0 [xe]
1678477804.452298 pci_device_probe+0xf9/0x200
1678477804.452974 really_probe+0x13b/0x530
1678477804.453751 __driver_probe_device+0xca/0x210
1678477804.454309 driver_probe_device+0x4a/0xf0
1678477804.455055 __driver_attach+0x135/0x290
1678477804.455744 bus_for_each_dev+0xf9/0x160
1678477804.456304 bus_add_driver+0x29c/0x2f0
1678477804.459887 driver_register+0x10d/0x1a0
1678477804.460599 do_one_initcall+0xce/0x3f0
1678477804.461137 do_init_module+0xe4/0x320
1678477804.465037 load_module+0x331e/0x3560
1678477804.465838 __do_sys_finit_module+0x10d/0x1b0
1678477804.466735 do_syscall_64+0x37/0x90
1678477804.467223 entry_SYSCALL_64_after_hwframe+0x63/0xcd
1678477804.468172
-> #1 (fs_reclaim){+.+.}-{0:0}:
1678477804.468719 lock_acquire+0x169/0x410
1678477804.469372 fs_reclaim_acquire+0xca/0x100
1678477804.470024 __kmem_cache_alloc_node+0x4a/0x3b0
1678477804.470682 kmalloc_trace+0x26/0x60
1678477804.471246 _drm_do_get_edid+0xd3/0x400 [drm]
1678477804.471953 drm_get_edid+0x8d/0x100 [drm]
1678477804.472642 ast_vga_connector_helper_get_modes+0x55/0xb0 [ast]
1678477804.473231 drm_helper_probe_single_connector_modes+0x34b/0x850 [drm_kms_helper]
1678477804.473854 drm_client_modeset_probe+0x31a/0x1390 [drm]
1678477804.474494 __drm_fb_helper_initial_config_and_unlock+0xce/0x980 [drm_kms_helper]
1678477804.474998 drm_fbdev_client_hotplug+0x142/0x1a0 [drm_kms_helper]
1678477804.475577 drm_fbdev_generic_setup+0xbe/0x1e0 [drm_kms_helper]
1678477804.475987 ast_pci_probe+0xe9/0x100 [ast]
1678477804.476588 pci_device_probe+0xf9/0x200
1678477804.477076 really_probe+0x13b/0x530
1678477804.477485 __driver_probe_device+0xca/0x210
1678477804.545950 driver_probe_device+0x4a/0xf0
1678477804.546166 __driver_attach+0x135/0x290
1678477804.546320 bus_for_each_dev+0xf9/0x160
1678477804.546570 bus_add_driver+0x29c/0x2f0
1678477804.546717 driver_register+0x10d/0x1a0
1678477804.546868 do_one_initcall+0xce/0x3f0
1678477804.547007 do_init_module+0xe4/0x320
1678477804.547155 load_module+0x331e/0x3560
1678477804.547305 __do_sys_finit_module+0x10d/0x1b0
1678477804.547490 do_syscall_64+0x37/0x90
1678477804.547636 entry_SYSCALL_64_after_hwframe+0x63/0xcd
1678477804.547777
-> #0 (lock#7){+.+.}-{3:3}:
1678477804.547932 check_prev_add+0xfc/0x1190
1678477804.548114 __lock_acquire+0x1e0a/0x2710
1678477804.548296 lock_acquire+0x169/0x410
1678477804.548484 __mutex_lock+0x164/0xe10
1678477804.548633 xe_device_mem_access_get+0x2f/0xc0 [xe]
1678477804.548787 guc_ct_send_locked+0x224/0xbb0 [xe]
1678477804.548935 xe_guc_ct_send+0x43/0x80 [xe]
1678477804.549112 __register_engine+0x13b/0x180 [xe]
1678477804.549267 guc_engine_run_job+0xf1c/0x1080 [xe]
1678477804.549471 drm_sched_main+0x3fc/0x9e0 [gpu_sched]
1678477804.549619 process_one_work+0x54a/0x9a0
1678477804.549830 worker_thread+0x8f/0x630
1678477804.549984 kthread+0x17b/0x1b0
1678477804.550135 ret_from_fork+0x1f/0x30
Note that #1 is actually coming from the probe of the ast driver.
Lucas De Marchi
>
>But well, Thomas had some bad feelings about the whole idea of the
>memory access handling anyway and was in favor of the i915 approach...
>
>>
>> > > Lucas De Marchi
>> > >
>> > > > - mutex_lock(&xe->mem_access.lock);
>> > > > - if (xe->mem_access.ref++ == 0)
>> > > > + if (ref == 1)
>> > > > xe->mem_access.hold_rpm = xe_pm_runtime_get_if_active(xe);
>> > hmmm... I'm afraid this can be tricky without locks...
>> >
>> > if we have 3 simultaneous threads calling this.
>> > get
>> > get
>> > put
>> > get
>> >
>> > and they happened in this order but the resume didn't finished yet
>> > on the first one, then you will:
>> > 1. end up the runtime pm twice.
>> > 2. the second will pass over thinking the gpu is already awake, but it might
>> > be still asleep.
>>
>>
>>
>>
>>
>>
>>
>> > > > - mutex_unlock(&xe->mem_access.lock);
>> > > >
>> > > > /* The usage counter increased if device was immediately resumed */
>> > > > if (resumed)
>> > > > xe_pm_runtime_put(xe);
>> > > >
>> > > > - XE_WARN_ON(xe->mem_access.ref == S32_MAX);
>> > > > + XE_WARN_ON(ref == S32_MAX);
>> > > > }
>> > > >
>> > > > void xe_device_mem_access_put(struct xe_device *xe)
>> > > > {
>> > > > - mutex_lock(&xe->mem_access.lock);
>> > > > - if (--xe->mem_access.ref == 0 && xe->mem_access.hold_rpm)
>> > > > + bool hold = xe->mem_access.hold_rpm;
>> > > > + int ref = atomic_dec_return(&xe->mem_access.ref);
>> > > > +
>> > > > + if (!ref && hold)
>> > > > xe_pm_runtime_put(xe);
>> > > > - mutex_unlock(&xe->mem_access.lock);
>> > > >
>> > > > - XE_WARN_ON(xe->mem_access.ref < 0);
>> > > > + XE_WARN_ON(ref < 0);
>> > > > }
>> > > > diff --git a/drivers/gpu/drm/xe/xe_device.h b/drivers/gpu/drm/xe/xe_device.h
>> > > > index 263620953c3b..96b4f3d7969e 100644
>> > > > --- a/drivers/gpu/drm/xe/xe_device.h
>> > > > +++ b/drivers/gpu/drm/xe/xe_device.h
>> > > > @@ -90,20 +90,14 @@ static inline struct xe_force_wake * gt_to_fw(struct xe_gt *gt)
>> > > > void xe_device_mem_access_get(struct xe_device *xe);
>> > > > void xe_device_mem_access_put(struct xe_device *xe);
>> > > >
>> > > > -static inline void xe_device_assert_mem_access(struct xe_device *xe)
>> > > > +static inline bool xe_device_mem_access_ongoing(struct xe_device *xe)
>> > > > {
>> > > > - XE_WARN_ON(!xe->mem_access.ref);
>> > > > + return atomic_read(&xe->mem_access.ref);
>> > > > }
>> > > >
>> > > > -static inline bool xe_device_mem_access_ongoing(struct xe_device *xe)
>> > > > +static inline void xe_device_assert_mem_access(struct xe_device *xe)
>> > > > {
>> > > > - bool ret;
>> > > > -
>> > > > - mutex_lock(&xe->mem_access.lock);
>> > > > - ret = xe->mem_access.ref;
>> > > > - mutex_unlock(&xe->mem_access.lock);
>> > > > -
>> > > > - return ret;
>> > > > + XE_WARN_ON(!xe_device_mem_access_ongoing(xe));
>> > > > }
>> > > >
>> > > > static inline bool xe_device_in_fault_mode(struct xe_device *xe)
>> > > > diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>> > > > index 9743987fc883..0b8c4ee0ad48 100644
>> > > > --- a/drivers/gpu/drm/xe/xe_device_types.h
>> > > > +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> > > > @@ -230,10 +230,8 @@ struct xe_device {
>> > > > * triggering additional actions when they occur.
>> > > > */
>> > > > struct {
>> > > > - /** @lock: protect the ref count */
>> > > > - struct mutex lock;
>> > > > /** @ref: ref count of memory accesses */
>> > > > - s32 ref;
>> > > > + atomic_t ref;
>> > > > /** @hold_rpm: need to put rpm ref back at the end */
>> > > > bool hold_rpm;
>> > > > } mem_access;
>> > > > --
>> > > > 2.34.1
>> > > >
More information about the Intel-xe
mailing list