From: Rodrigo Vivi <rodrigo.vivi@intel.com>
To: Thomas Zimmermann <tzimmermann@suse.de>
Cc: <alexander.deucher@amd.com>, <christian.koenig@amd.com>,
<jani.nikula@linux.intel.com>, <joonas.lahtinen@linux.intel.com>,
<tursulin@ursulin.net>, <lyude@redhat.com>, <dakr@kernel.org>,
<lucas.demarchi@intel.com>, <thomas.hellstrom@linux.intel.com>,
<jfalempe@redhat.com>, <javierm@redhat.com>,
<dri-devel@lists.freedesktop.org>,
<amd-gfx@lists.freedesktop.org>,
<intel-gfx@lists.freedesktop.org>,
<nouveau@lists.freedesktop.org>, <intel-xe@lists.freedesktop.org>
Subject: Re: [PATCH v2] drm/client: Remove holds_console_lock parameter from suspend/resume
Date: Wed, 1 Oct 2025 11:18:43 -0400 [thread overview]
Message-ID: <aN1GU86CMXmikbua@intel.com> (raw)
In-Reply-To: <20251001143709.419736-1-tzimmermann@suse.de>
On Wed, Oct 01, 2025 at 04:37:03PM +0200, 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.
>
> v2:
> - update placeholders for CONFIG_DRM_CLIENT=n
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Link: https://patchwork.freedesktop.org/series/151624/ # [1]
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
(for both xe and i915 changes)
> ---
> 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 | 8 ++++----
> 11 files changed, 37 insertions(+), 53 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..985d6f02a4c4 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)
> { }
> @@ -20,9 +20,9 @@ static inline void drm_client_dev_hotplug(struct drm_device *dev)
> { }
> static inline void drm_client_dev_restore(struct drm_device *dev)
> { }
> -static inline void drm_client_dev_suspend(struct drm_device *dev, bool holds_console_lock)
> +static inline void drm_client_dev_suspend(struct drm_device *dev)
> { }
> -static inline void drm_client_dev_resume(struct drm_device *dev, bool holds_console_lock)
> +static inline void drm_client_dev_resume(struct drm_device *dev)
> { }
> #endif
>
> --
> 2.51.0
>
next prev parent reply other threads:[~2025-10-01 15:18 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-01 14:37 [PATCH v2] drm/client: Remove holds_console_lock parameter from suspend/resume Thomas Zimmermann
2025-10-01 14:45 ` ✓ CI.KUnit: success for drm/client: Remove holds_console_lock parameter from suspend/resume (rev2) Patchwork
2025-10-01 15:18 ` Rodrigo Vivi [this message]
2025-10-01 15:35 ` ✓ Xe.CI.BAT: " Patchwork
2025-10-01 16:39 ` ✗ Xe.CI.Full: failure " Patchwork
2025-10-01 16:53 ` ✗ i915.CI.BAT: " Patchwork
2025-10-01 19:37 ` [PATCH v2] drm/client: Remove holds_console_lock parameter from suspend/resume Petr Vorel
2025-10-07 10:06 ` Andi Shyti
2025-10-08 9:57 ` Danilo Krummrich
2025-10-13 16:07 ` Thomas Zimmermann
2025-10-14 7:33 ` Jocelyn Falempe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aN1GU86CMXmikbua@intel.com \
--to=rodrigo.vivi@intel.com \
--cc=alexander.deucher@amd.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=christian.koenig@amd.com \
--cc=dakr@kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=intel-xe@lists.freedesktop.org \
--cc=jani.nikula@linux.intel.com \
--cc=javierm@redhat.com \
--cc=jfalempe@redhat.com \
--cc=joonas.lahtinen@linux.intel.com \
--cc=lucas.demarchi@intel.com \
--cc=lyude@redhat.com \
--cc=nouveau@lists.freedesktop.org \
--cc=thomas.hellstrom@linux.intel.com \
--cc=tursulin@ursulin.net \
--cc=tzimmermann@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.