Thomas Zimmermann
2025-Oct-01 14:39 UTC
[PATCH] drm/client: Remove holds_console_lock parameter from suspend/resume
Hi Am 01.10.25 um 14:46 schrieb Nirmoy Das:> > On 01.10.25 14:15, Thomas Zimmermann wrote: >> No caller of the client resume/suspend helpers holds the console >> lock. The last such cases were removed from radeon in the patch >> series at [1]. Now remove the related parameter and the TODO items. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de> >> Link: https://patchwork.freedesktop.org/series/151624/ # [1] > Acked-by: Nirmoy Das <nirmoyd at nvidia.com>Thanks. I sent out an update that addresses the CI issue before seeing your a-b, but it will be included when the patch lands. Best regards Thomas>> --- >> This patch would preferably be merged through drm-misc. >> --- >> ? drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |? 8 ++++---- >> ? drivers/gpu/drm/clients/drm_fbdev_client.c | 14 ++++---------- >> ? drivers/gpu/drm/clients/drm_log.c????????? |? 4 ++-- >> ? drivers/gpu/drm/drm_client_event.c???????? | 16 ++++++++-------- >> ? drivers/gpu/drm/drm_modeset_helper.c?????? |? 6 +++--- >> ? drivers/gpu/drm/i915/i915_driver.c???????? |? 6 +++--- >> ? drivers/gpu/drm/nouveau/nouveau_display.c? |? 4 ++-- >> ? drivers/gpu/drm/radeon/radeon_device.c???? |? 4 ++-- >> ? drivers/gpu/drm/xe/display/xe_display.c??? |? 6 +++--- >> ? include/drm/drm_client.h?????????????????? | 14 ++------------ >> ? include/drm/drm_client_event.h???????????? |? 4 ++-- >> ? 11 files changed, 35 insertions(+), 51 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> index a77000c2e0bb..f068e26d5080 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> @@ -5212,7 +5212,7 @@ int amdgpu_device_suspend(struct drm_device >> *dev, bool notify_clients) >> ????????? dev_warn(adev->dev, "smart shift update failed\n"); >> ? ????? if (notify_clients) >> -??????? drm_client_dev_suspend(adev_to_drm(adev), false); >> +??????? drm_client_dev_suspend(adev_to_drm(adev)); >> cancel_delayed_work_sync(&adev->delayed_init_work); >> ? @@ -5346,7 +5346,7 @@ int amdgpu_device_resume(struct drm_device >> *dev, bool notify_clients) >> ????? flush_delayed_work(&adev->delayed_init_work); >> ? ????? if (notify_clients) >> -??????? drm_client_dev_resume(adev_to_drm(adev), false); >> +??????? drm_client_dev_resume(adev_to_drm(adev)); >> ? ????? amdgpu_ras_resume(adev); >> ? @@ -5951,7 +5951,7 @@ int amdgpu_device_reinit_after_reset(struct >> amdgpu_reset_context *reset_context) >> ????????????????? if (r) >> ????????????????????? goto out; >> ? -??????????????? drm_client_dev_resume(adev_to_drm(tmp_adev), false); >> +??????????????? drm_client_dev_resume(adev_to_drm(tmp_adev)); >> ? ????????????????? /* >> ?????????????????? * The GPU enters bad state once faulty pages >> @@ -6286,7 +6286,7 @@ static void >> amdgpu_device_halt_activities(struct amdgpu_device *adev, >> ?????????? */ >> ????????? amdgpu_unregister_gpu_instance(tmp_adev); >> ? -??????? drm_client_dev_suspend(adev_to_drm(tmp_adev), false); >> +??????? drm_client_dev_suspend(adev_to_drm(tmp_adev)); >> ? ????????? /* disable ras on ALL IPs */ >> ????????? if (!need_emergency_restart && !amdgpu_reset_in_dpc(adev) && >> diff --git a/drivers/gpu/drm/clients/drm_fbdev_client.c >> b/drivers/gpu/drm/clients/drm_fbdev_client.c >> index f894ba52bdb5..ec5ab9f30547 100644 >> --- a/drivers/gpu/drm/clients/drm_fbdev_client.c >> +++ b/drivers/gpu/drm/clients/drm_fbdev_client.c >> @@ -62,26 +62,20 @@ static int drm_fbdev_client_hotplug(struct >> drm_client_dev *client) >> ????? return ret; >> ? } >> ? -static int drm_fbdev_client_suspend(struct drm_client_dev *client, >> bool holds_console_lock) >> +static int drm_fbdev_client_suspend(struct drm_client_dev *client) >> ? { >> ????? struct drm_fb_helper *fb_helper = >> drm_fb_helper_from_client(client); >> ? -??? if (holds_console_lock) >> -??????? drm_fb_helper_set_suspend(fb_helper, true); >> -??? else >> -??????? drm_fb_helper_set_suspend_unlocked(fb_helper, true); >> +??? drm_fb_helper_set_suspend_unlocked(fb_helper, true); >> ? ????? return 0; >> ? } >> ? -static int drm_fbdev_client_resume(struct drm_client_dev *client, >> bool holds_console_lock) >> +static int drm_fbdev_client_resume(struct drm_client_dev *client) >> ? { >> ????? struct drm_fb_helper *fb_helper = >> drm_fb_helper_from_client(client); >> ? -??? if (holds_console_lock) >> -??????? drm_fb_helper_set_suspend(fb_helper, false); >> -??? else >> -??????? drm_fb_helper_set_suspend_unlocked(fb_helper, false); >> +??? drm_fb_helper_set_suspend_unlocked(fb_helper, false); >> ? ????? return 0; >> ? } >> diff --git a/drivers/gpu/drm/clients/drm_log.c >> b/drivers/gpu/drm/clients/drm_log.c >> index d239f1e3c456..fd8556dd58ed 100644 >> --- a/drivers/gpu/drm/clients/drm_log.c >> +++ b/drivers/gpu/drm/clients/drm_log.c >> @@ -319,7 +319,7 @@ static int drm_log_client_hotplug(struct >> drm_client_dev *client) >> ????? return 0; >> ? } >> ? -static int drm_log_client_suspend(struct drm_client_dev *client, >> bool _console_lock) >> +static int drm_log_client_suspend(struct drm_client_dev *client) >> ? { >> ????? struct drm_log *dlog = client_to_drm_log(client); >> ? @@ -328,7 +328,7 @@ static int drm_log_client_suspend(struct >> drm_client_dev *client, bool _console_l >> ????? return 0; >> ? } >> ? -static int drm_log_client_resume(struct drm_client_dev *client, >> bool _console_lock) >> +static int drm_log_client_resume(struct drm_client_dev *client) >> ? { >> ????? struct drm_log *dlog = client_to_drm_log(client); >> ? diff --git a/drivers/gpu/drm/drm_client_event.c >> b/drivers/gpu/drm/drm_client_event.c >> index c83196ad8b59..c3baeb4d4e6b 100644 >> --- a/drivers/gpu/drm/drm_client_event.c >> +++ b/drivers/gpu/drm/drm_client_event.c >> @@ -122,7 +122,7 @@ void drm_client_dev_restore(struct drm_device *dev) >> ????? mutex_unlock(&dev->clientlist_mutex); >> ? } >> ? -static int drm_client_suspend(struct drm_client_dev *client, bool >> holds_console_lock) >> +static int drm_client_suspend(struct drm_client_dev *client) >> ? { >> ????? struct drm_device *dev = client->dev; >> ????? int ret = 0; >> @@ -131,7 +131,7 @@ static int drm_client_suspend(struct >> drm_client_dev *client, bool holds_console_ >> ????????? return 0; >> ? ????? if (client->funcs && client->funcs->suspend) >> -??????? ret = client->funcs->suspend(client, holds_console_lock); >> +??????? ret = client->funcs->suspend(client); >> ????? drm_dbg_kms(dev, "%s: ret=%d\n", client->name, ret); >> ? ????? client->suspended = true; >> @@ -139,20 +139,20 @@ static int drm_client_suspend(struct >> drm_client_dev *client, bool holds_console_ >> ????? return ret; >> ? } >> ? -void drm_client_dev_suspend(struct drm_device *dev, bool >> holds_console_lock) >> +void drm_client_dev_suspend(struct drm_device *dev) >> ? { >> ????? struct drm_client_dev *client; >> ? ????? mutex_lock(&dev->clientlist_mutex); >> ????? list_for_each_entry(client, &dev->clientlist, list) { >> ????????? if (!client->suspended) >> -??????????? drm_client_suspend(client, holds_console_lock); >> +??????????? drm_client_suspend(client); >> ????? } >> ????? mutex_unlock(&dev->clientlist_mutex); >> ? } >> ? EXPORT_SYMBOL(drm_client_dev_suspend); >> ? -static int drm_client_resume(struct drm_client_dev *client, bool >> holds_console_lock) >> +static int drm_client_resume(struct drm_client_dev *client) >> ? { >> ????? struct drm_device *dev = client->dev; >> ????? int ret = 0; >> @@ -161,7 +161,7 @@ static int drm_client_resume(struct >> drm_client_dev *client, bool holds_console_l >> ????????? return 0; >> ? ????? if (client->funcs && client->funcs->resume) >> -??????? ret = client->funcs->resume(client, holds_console_lock); >> +??????? ret = client->funcs->resume(client); >> ????? drm_dbg_kms(dev, "%s: ret=%d\n", client->name, ret); >> ? ????? client->suspended = false; >> @@ -172,14 +172,14 @@ static int drm_client_resume(struct >> drm_client_dev *client, bool holds_console_l >> ????? return ret; >> ? } >> ? -void drm_client_dev_resume(struct drm_device *dev, bool >> holds_console_lock) >> +void drm_client_dev_resume(struct drm_device *dev) >> ? { >> ????? struct drm_client_dev *client; >> ? ????? mutex_lock(&dev->clientlist_mutex); >> ????? list_for_each_entry(client, &dev->clientlist, list) { >> ????????? if? (client->suspended) >> -??????????? drm_client_resume(client, holds_console_lock); >> +??????????? drm_client_resume(client); >> ????? } >> ????? mutex_unlock(&dev->clientlist_mutex); >> ? } >> diff --git a/drivers/gpu/drm/drm_modeset_helper.c >> b/drivers/gpu/drm/drm_modeset_helper.c >> index 988735560570..a57f6a10ada4 100644 >> --- a/drivers/gpu/drm/drm_modeset_helper.c >> +++ b/drivers/gpu/drm/drm_modeset_helper.c >> @@ -203,10 +203,10 @@ int drm_mode_config_helper_suspend(struct >> drm_device *dev) >> ????? if (dev->mode_config.poll_enabled) >> ????????? drm_kms_helper_poll_disable(dev); >> ? -??? drm_client_dev_suspend(dev, false); >> +??? drm_client_dev_suspend(dev); >> ????? state = drm_atomic_helper_suspend(dev); >> ????? if (IS_ERR(state)) { >> -??????? drm_client_dev_resume(dev, false); >> +??????? drm_client_dev_resume(dev); >> ? ????????? /* >> ?????????? * Don't enable polling if it was never initialized >> @@ -252,7 +252,7 @@ int drm_mode_config_helper_resume(struct >> drm_device *dev) >> ????????? DRM_ERROR("Failed to resume (%d)\n", ret); >> ????? dev->mode_config.suspend_state = NULL; >> ? -??? drm_client_dev_resume(dev, false); >> +??? drm_client_dev_resume(dev); >> ? ????? /* >> ?????? * Don't enable polling if it is not initialized >> diff --git a/drivers/gpu/drm/i915/i915_driver.c >> b/drivers/gpu/drm/i915/i915_driver.c >> index 95165e45de74..162e50315beb 100644 >> --- a/drivers/gpu/drm/i915/i915_driver.c >> +++ b/drivers/gpu/drm/i915/i915_driver.c >> @@ -978,7 +978,7 @@ void i915_driver_shutdown(struct drm_i915_private >> *i915) >> ????? intel_runtime_pm_disable(&i915->runtime_pm); >> ????? intel_power_domains_disable(display); >> ? -??? drm_client_dev_suspend(&i915->drm, false); >> +??? drm_client_dev_suspend(&i915->drm); >> ????? if (intel_display_device_present(display)) { >> ????????? drm_kms_helper_poll_disable(&i915->drm); >> ????????? intel_display_driver_disable_user_access(display); >> @@ -1060,7 +1060,7 @@ static int i915_drm_suspend(struct drm_device >> *dev) >> ????? /* We do a lot of poking in a lot of registers, make sure they >> work >> ?????? * properly. */ >> ????? intel_power_domains_disable(display); >> -??? drm_client_dev_suspend(dev, false); >> +??? drm_client_dev_suspend(dev); >> ????? if (intel_display_device_present(display)) { >> ????????? drm_kms_helper_poll_disable(dev); >> ????????? intel_display_driver_disable_user_access(display); >> @@ -1257,7 +1257,7 @@ static int i915_drm_resume(struct drm_device *dev) >> ? ????? intel_opregion_resume(display); >> ? -??? drm_client_dev_resume(dev, false); >> +??? drm_client_dev_resume(dev); >> ? ????? intel_power_domains_enable(display); >> ? diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c >> b/drivers/gpu/drm/nouveau/nouveau_display.c >> index 54aed3656a4c..00515623a2cc 100644 >> --- a/drivers/gpu/drm/nouveau/nouveau_display.c >> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c >> @@ -765,7 +765,7 @@ nouveau_display_suspend(struct drm_device *dev, >> bool runtime) >> ? { >> ????? struct nouveau_display *disp = nouveau_display(dev); >> ? -??? drm_client_dev_suspend(dev, false); >> +??? drm_client_dev_suspend(dev); >> ? ????? if (drm_drv_uses_atomic_modeset(dev)) { >> ????????? if (!runtime) { >> @@ -796,7 +796,7 @@ nouveau_display_resume(struct drm_device *dev, >> bool runtime) >> ????????? } >> ????? } >> ? -??? drm_client_dev_resume(dev, false); >> +??? drm_client_dev_resume(dev); >> ? } >> ? ? int >> diff --git a/drivers/gpu/drm/radeon/radeon_device.c >> b/drivers/gpu/drm/radeon/radeon_device.c >> index 9e35b14e2bf0..60afaa8e56b4 100644 >> --- a/drivers/gpu/drm/radeon/radeon_device.c >> +++ b/drivers/gpu/drm/radeon/radeon_device.c >> @@ -1635,7 +1635,7 @@ int radeon_suspend_kms(struct drm_device *dev, >> bool suspend, >> ????? } >> ? ????? if (notify_clients) >> -??????? drm_client_dev_suspend(dev, false); >> +??????? drm_client_dev_suspend(dev); >> ? ????? return 0; >> ? } >> @@ -1739,7 +1739,7 @@ int radeon_resume_kms(struct drm_device *dev, >> bool resume, bool notify_clients) >> ????????? radeon_pm_compute_clocks(rdev); >> ? ????? if (notify_clients) >> -??????? drm_client_dev_resume(dev, false); >> +??????? drm_client_dev_resume(dev); >> ? ????? return 0; >> ? } >> diff --git a/drivers/gpu/drm/xe/display/xe_display.c >> b/drivers/gpu/drm/xe/display/xe_display.c >> index 19e691fccf8c..d3cc6181842c 100644 >> --- a/drivers/gpu/drm/xe/display/xe_display.c >> +++ b/drivers/gpu/drm/xe/display/xe_display.c >> @@ -324,7 +324,7 @@ void xe_display_pm_suspend(struct xe_device *xe) >> ?????? * properly. >> ?????? */ >> ????? intel_power_domains_disable(display); >> -??? drm_client_dev_suspend(&xe->drm, false); >> +??? drm_client_dev_suspend(&xe->drm); >> ? ????? if (intel_display_device_present(display)) { >> ????????? drm_kms_helper_poll_disable(&xe->drm); >> @@ -356,7 +356,7 @@ void xe_display_pm_shutdown(struct xe_device *xe) >> ????????? return; >> ? ????? intel_power_domains_disable(display); >> -??? drm_client_dev_suspend(&xe->drm, false); >> +??? drm_client_dev_suspend(&xe->drm); >> ? ????? if (intel_display_device_present(display)) { >> ????????? drm_kms_helper_poll_disable(&xe->drm); >> @@ -481,7 +481,7 @@ void xe_display_pm_resume(struct xe_device *xe) >> ? ????? intel_opregion_resume(display); >> ? -??? drm_client_dev_resume(&xe->drm, false); >> +??? drm_client_dev_resume(&xe->drm); >> ? ????? intel_power_domains_enable(display); >> ? } >> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h >> index bdd845e383ef..3556928d3938 100644 >> --- a/include/drm/drm_client.h >> +++ b/include/drm/drm_client.h >> @@ -70,13 +70,8 @@ struct drm_client_funcs { >> ?????? * Called when suspending the device. >> ?????? * >> ?????? * This callback is optional. >> -???? * >> -???? * FIXME: Some callers hold the console lock when invoking this >> -???? *??????? function. This interferes with fbdev emulation, which >> -???? *??????? also tries to acquire the lock. Push the console lock >> -???? *??????? into the callback and remove 'holds_console_lock'. >> ?????? */ >> -??? int (*suspend)(struct drm_client_dev *client, bool >> holds_console_lock); >> +??? int (*suspend)(struct drm_client_dev *client); >> ? ????? /** >> ?????? * @resume: >> @@ -84,13 +79,8 @@ struct drm_client_funcs { >> ?????? * Called when resuming the device from suspend. >> ?????? * >> ?????? * This callback is optional. >> -???? * >> -???? * FIXME: Some callers hold the console lock when invoking this >> -???? *??????? function. This interferes with fbdev emulation, which >> -???? *??????? also tries to acquire the lock. Push the console lock >> -???? *??????? into the callback and remove 'holds_console_lock'. >> ?????? */ >> -??? int (*resume)(struct drm_client_dev *client, bool >> holds_console_lock); >> +??? int (*resume)(struct drm_client_dev *client); >> ? }; >> ? ? /** >> diff --git a/include/drm/drm_client_event.h >> b/include/drm/drm_client_event.h >> index 1d544d3a3228..c3f318788b71 100644 >> --- a/include/drm/drm_client_event.h >> +++ b/include/drm/drm_client_event.h >> @@ -11,8 +11,8 @@ struct drm_device; >> ? void drm_client_dev_unregister(struct drm_device *dev); >> ? void drm_client_dev_hotplug(struct drm_device *dev); >> ? void drm_client_dev_restore(struct drm_device *dev); >> -void drm_client_dev_suspend(struct drm_device *dev, bool >> holds_console_lock); >> -void drm_client_dev_resume(struct drm_device *dev, bool >> holds_console_lock); >> +void drm_client_dev_suspend(struct drm_device *dev); >> +void drm_client_dev_resume(struct drm_device *dev); >> ? #else >> ? static inline void drm_client_dev_unregister(struct drm_device *dev) >> ? { }-- -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg)