| Message ID | 20241008120652.159190-8-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Introduce DRM client library | expand |
-----Original Message----- From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Thomas Zimmermann Sent: Tuesday, October 8, 2024 4:59 AM To: simona@ffwll.ch; airlied@gmail.com; javierm@redhat.com; jfalempe@redhat.com Cc: dri-devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org; Thomas Zimmermann <tzimmermann@suse.de> Subject: [PATCH v3 07/12] drm/client: Move suspend/resume into DRM client callbacks > > Suspend and resume is still tied to fbdev emulation. Modeset helpers > and several drivers call drm_fb_helper_set_suspend_unlocked() to inform > the fbdev client about suspend/resume events. > > To make it work with arbitrary clients, add per-client callback > functions for suspend and resume. Implement them for fbdev emulation > with the existing drm_fb_helper_set_suspend_unlocked(). Then update > DRM's modeset helpers to call the new interface. > > Clients that are not fbdev can now implement suspend/resume to their > requirements. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Some questions below, but nothing blocking. Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com> > --- > drivers/gpu/drm/drm_client_event.c | 60 ++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_fbdev_client.c | 30 +++++++++++++- > drivers/gpu/drm/drm_modeset_helper.c | 14 ++++--- > include/drm/drm_client.h | 35 ++++++++++++++++ > include/drm/drm_client_event.h | 2 + > 5 files changed, 133 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/drm_client_event.c b/drivers/gpu/drm/drm_client_event.c > index d13d44320c5c..c52e93643672 100644 > --- a/drivers/gpu/drm/drm_client_event.c > +++ b/drivers/gpu/drm/drm_client_event.c > @@ -107,6 +107,66 @@ 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) > +{ > + struct drm_device *dev = client->dev; > + int ret = 0; > + > + if (drm_WARN_ON_ONCE(dev, client->suspended)) > + return 0; > + > + if (client->funcs && client->funcs->suspend) > + ret = client->funcs->suspend(client, holds_console_lock); > + drm_dbg_kms(dev, "%s: ret=%d\n", client->name, ret); > + > + client->suspended = true; > + > + return ret; > +} > + > +void drm_client_dev_suspend(struct drm_device *dev, bool holds_console_lock) > +{ > + 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); > + } > + 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) > +{ > + struct drm_device *dev = client->dev; > + int ret = 0; > + > + if (drm_WARN_ON_ONCE(dev, !client->suspended)) > + return 0; > + > + if (client->funcs && client->funcs->resume) > + ret = client->funcs->resume(client, holds_console_lock); > + drm_dbg_kms(dev, "%s: ret=%d\n", client->name, ret); > + > + client->suspended = false; > + > + return ret; > +} > + > +void drm_client_dev_resume(struct drm_device *dev, bool holds_console_lock) > +{ > + 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); > + } > + mutex_unlock(&dev->clientlist_mutex); > +} > +EXPORT_SYMBOL(drm_client_dev_resume); I had to double check, as it seemed a bit weird to have a Boolean "holds_console_lock" if it was always going to be False in the use cases presented in this patch, but we do set it to True in the Radeon use case in patch 10, so that makes more sense. > + > #ifdef CONFIG_DEBUG_FS > static int drm_client_debugfs_internal_clients(struct seq_file *m, void *data) > { > diff --git a/drivers/gpu/drm/drm_fbdev_client.c b/drivers/gpu/drm/drm_fbdev_client.c > index a09382afe2fb..246fb63ab250 100644 > --- a/drivers/gpu/drm/drm_fbdev_client.c > +++ b/drivers/gpu/drm/drm_fbdev_client.c > @@ -61,11 +61,37 @@ 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) > +{ > + 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); > + > + return 0; > +} > + > +static int drm_fbdev_client_resume(struct drm_client_dev *client, bool holds_console_lock) > +{ > + 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); > + > + return 0; > +} > + > static const struct drm_client_funcs drm_fbdev_client_funcs = { > .owner = THIS_MODULE, > .unregister = drm_fbdev_client_unregister, > .restore = drm_fbdev_client_restore, > .hotplug = drm_fbdev_client_hotplug, > + .suspend = drm_fbdev_client_suspend, > + .resume = drm_fbdev_client_resume, > }; Just a question for my own understanding: The expected order of operations here is: drm_mode_config_helper_suspend calls drm_client_dev_suspend drm_client_dev_suspend calls drm_client_suspend drm_client_suspend calls client->funcs->suspend, which for fbdev is drm_fbdev_client_suspend drm_fbdev_client_suspend calls drm_fb_helper_set_suspend(_unlocked) And, circling back to the start, drm_mode_config_helper_suspend is called by several drivers in the suspend case. Is this correct? > > /** > @@ -76,8 +102,8 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = { > * > * This function sets up fbdev emulation. Restore, hotplug events and > * teardown are all taken care of. Drivers that do suspend/resume need > - * to call drm_fb_helper_set_suspend_unlocked() themselves. Simple > - * drivers might use drm_mode_config_helper_suspend(). > + * to call drm_client_dev_suspend() and drm_client_dev_resume() by > + * themselves. Simple drivers might use drm_mode_config_helper_suspend(). > * > * This function is safe to call even when there are no connectors present. > * Setup will be retried on the next hotplug event. > diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c > index 2c582020cb42..5565464c1734 100644 > --- a/drivers/gpu/drm/drm_modeset_helper.c > +++ b/drivers/gpu/drm/drm_modeset_helper.c > @@ -21,7 +21,7 @@ > */ > > #include <drm/drm_atomic_helper.h> > -#include <drm/drm_fb_helper.h> > +#include <drm/drm_client_event.h> > #include <drm/drm_fourcc.h> > #include <drm/drm_framebuffer.h> > #include <drm/drm_modeset_helper.h> > @@ -185,7 +185,7 @@ EXPORT_SYMBOL(drm_crtc_init); > * Zero on success, negative error code on error. > * > * See also: > - * drm_kms_helper_poll_disable() and drm_fb_helper_set_suspend_unlocked(). > + * drm_kms_helper_poll_disable() and drm_client_dev_suspend(). > */ > int drm_mode_config_helper_suspend(struct drm_device *dev) > { > @@ -199,10 +199,11 @@ int drm_mode_config_helper_suspend(struct drm_device *dev) > if (dev->mode_config.poll_enabled) > drm_kms_helper_poll_disable(dev); > > - drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 1); > + drm_client_dev_suspend(dev, false); > state = drm_atomic_helper_suspend(dev); > if (IS_ERR(state)) { > - drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0); > + drm_client_dev_resume(dev, false); > + > /* > * Don't enable polling if it was never initialized > */ > @@ -230,7 +231,7 @@ EXPORT_SYMBOL(drm_mode_config_helper_suspend); > * Zero on success, negative error code on error. > * > * See also: > - * drm_fb_helper_set_suspend_unlocked() and drm_kms_helper_poll_enable(). > + * drm_client_dev_resume() and drm_kms_helper_poll_enable(). > */ > int drm_mode_config_helper_resume(struct drm_device *dev) > { > @@ -247,7 +248,8 @@ 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_fb_helper_set_suspend_unlocked(dev->fb_helper, 0); > + drm_client_dev_resume(dev, false); > + > /* > * Don't enable polling if it is not initialized > */ > diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h > index dfd5afcc9463..c03c4b0f3e94 100644 > --- a/include/drm/drm_client.h > +++ b/include/drm/drm_client.h > @@ -63,6 +63,34 @@ struct drm_client_funcs { > * This callback is optional. > */ > int (*hotplug)(struct drm_client_dev *client); > + > + /** > + * @suspend: > + * > + * 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'. > + */ Is there an estimated time for the fix to this FIXME? It's out of scope for this series, so I won't insist on fixing it immediately, but if it's a "quick" fix (relatively speaking), then we should definitely try to get this FIXME resolved in short order. -Jonathan Cavitt > + int (*suspend)(struct drm_client_dev *client, bool holds_console_lock); > + > + /** > + * @resume: > + * > + * 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); > }; > > /** > @@ -107,6 +135,13 @@ struct drm_client_dev { > */ > struct drm_mode_set *modesets; > > + /** > + * @suspended: > + * > + * The client has been suspended. > + */ > + bool suspended; > + > /** > * @hotplug_failed: > * > diff --git a/include/drm/drm_client_event.h b/include/drm/drm_client_event.h > index 2c8915241120..72c97d111169 100644 > --- a/include/drm/drm_client_event.h > +++ b/include/drm/drm_client_event.h > @@ -8,5 +8,7 @@ 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); > > #endif > -- > 2.46.0 > >
Hi Am 08.10.24 um 23:21 schrieb Cavitt, Jonathan: > -----Original Message----- > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Thomas Zimmermann > Sent: Tuesday, October 8, 2024 4:59 AM > To: simona@ffwll.ch; airlied@gmail.com; javierm@redhat.com; jfalempe@redhat.com > Cc: dri-devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org; Thomas Zimmermann <tzimmermann@suse.de> > Subject: [PATCH v3 07/12] drm/client: Move suspend/resume into DRM client callbacks >> Suspend and resume is still tied to fbdev emulation. Modeset helpers >> and several drivers call drm_fb_helper_set_suspend_unlocked() to inform >> the fbdev client about suspend/resume events. >> >> To make it work with arbitrary clients, add per-client callback >> functions for suspend and resume. Implement them for fbdev emulation >> with the existing drm_fb_helper_set_suspend_unlocked(). Then update >> DRM's modeset helpers to call the new interface. >> >> Clients that are not fbdev can now implement suspend/resume to their >> requirements. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > Some questions below, but nothing blocking. > > Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com> Thanks for reviewing this series. > >> --- >> drivers/gpu/drm/drm_client_event.c | 60 ++++++++++++++++++++++++++++ >> drivers/gpu/drm/drm_fbdev_client.c | 30 +++++++++++++- >> drivers/gpu/drm/drm_modeset_helper.c | 14 ++++--- >> include/drm/drm_client.h | 35 ++++++++++++++++ >> include/drm/drm_client_event.h | 2 + >> 5 files changed, 133 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_client_event.c b/drivers/gpu/drm/drm_client_event.c >> index d13d44320c5c..c52e93643672 100644 >> --- a/drivers/gpu/drm/drm_client_event.c >> +++ b/drivers/gpu/drm/drm_client_event.c >> @@ -107,6 +107,66 @@ 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) >> +{ >> + struct drm_device *dev = client->dev; >> + int ret = 0; >> + >> + if (drm_WARN_ON_ONCE(dev, client->suspended)) >> + return 0; >> + >> + if (client->funcs && client->funcs->suspend) >> + ret = client->funcs->suspend(client, holds_console_lock); >> + drm_dbg_kms(dev, "%s: ret=%d\n", client->name, ret); >> + >> + client->suspended = true; >> + >> + return ret; >> +} >> + >> +void drm_client_dev_suspend(struct drm_device *dev, bool holds_console_lock) >> +{ >> + 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); >> + } >> + 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) >> +{ >> + struct drm_device *dev = client->dev; >> + int ret = 0; >> + >> + if (drm_WARN_ON_ONCE(dev, !client->suspended)) >> + return 0; >> + >> + if (client->funcs && client->funcs->resume) >> + ret = client->funcs->resume(client, holds_console_lock); >> + drm_dbg_kms(dev, "%s: ret=%d\n", client->name, ret); >> + >> + client->suspended = false; >> + >> + return ret; >> +} >> + >> +void drm_client_dev_resume(struct drm_device *dev, bool holds_console_lock) >> +{ >> + 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); >> + } >> + mutex_unlock(&dev->clientlist_mutex); >> +} >> +EXPORT_SYMBOL(drm_client_dev_resume); > I had to double check, as it seemed a bit weird to have a Boolean "holds_console_lock" > if it was always going to be False in the use cases presented in this patch, but we do set > it to True in the Radeon use case in patch 10, so that makes more sense. I can mention this in the commit message. > >> + >> #ifdef CONFIG_DEBUG_FS >> static int drm_client_debugfs_internal_clients(struct seq_file *m, void *data) >> { >> diff --git a/drivers/gpu/drm/drm_fbdev_client.c b/drivers/gpu/drm/drm_fbdev_client.c >> index a09382afe2fb..246fb63ab250 100644 >> --- a/drivers/gpu/drm/drm_fbdev_client.c >> +++ b/drivers/gpu/drm/drm_fbdev_client.c >> @@ -61,11 +61,37 @@ 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) >> +{ >> + 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); >> + >> + return 0; >> +} >> + >> +static int drm_fbdev_client_resume(struct drm_client_dev *client, bool holds_console_lock) >> +{ >> + 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); >> + >> + return 0; >> +} >> + >> static const struct drm_client_funcs drm_fbdev_client_funcs = { >> .owner = THIS_MODULE, >> .unregister = drm_fbdev_client_unregister, >> .restore = drm_fbdev_client_restore, >> .hotplug = drm_fbdev_client_hotplug, >> + .suspend = drm_fbdev_client_suspend, >> + .resume = drm_fbdev_client_resume, >> }; > Just a question for my own understanding: > > > The expected order of operations here is: > > drm_mode_config_helper_suspend calls drm_client_dev_suspend > > drm_client_dev_suspend calls drm_client_suspend > > drm_client_suspend calls client->funcs->suspend, which for fbdev is > drm_fbdev_client_suspend > > drm_fbdev_client_suspend calls drm_fb_helper_set_suspend(_unlocked) > > And, circling back to the start, drm_mode_config_helper_suspend is called by > several drivers in the suspend case. > > > Is this correct? Yes, that's what's happening. This patch pushes the driver's direct call to drm_fb_helper_() interfaces into a callback of the client. That releases the module dependency and other clients can be used as well. > >> >> /** >> @@ -76,8 +102,8 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = { >> * >> * This function sets up fbdev emulation. Restore, hotplug events and >> * teardown are all taken care of. Drivers that do suspend/resume need >> - * to call drm_fb_helper_set_suspend_unlocked() themselves. Simple >> - * drivers might use drm_mode_config_helper_suspend(). >> + * to call drm_client_dev_suspend() and drm_client_dev_resume() by >> + * themselves. Simple drivers might use drm_mode_config_helper_suspend(). >> * >> * This function is safe to call even when there are no connectors present. >> * Setup will be retried on the next hotplug event. >> diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c >> index 2c582020cb42..5565464c1734 100644 >> --- a/drivers/gpu/drm/drm_modeset_helper.c >> +++ b/drivers/gpu/drm/drm_modeset_helper.c >> @@ -21,7 +21,7 @@ >> */ >> >> #include <drm/drm_atomic_helper.h> >> -#include <drm/drm_fb_helper.h> >> +#include <drm/drm_client_event.h> >> #include <drm/drm_fourcc.h> >> #include <drm/drm_framebuffer.h> >> #include <drm/drm_modeset_helper.h> >> @@ -185,7 +185,7 @@ EXPORT_SYMBOL(drm_crtc_init); >> * Zero on success, negative error code on error. >> * >> * See also: >> - * drm_kms_helper_poll_disable() and drm_fb_helper_set_suspend_unlocked(). >> + * drm_kms_helper_poll_disable() and drm_client_dev_suspend(). >> */ >> int drm_mode_config_helper_suspend(struct drm_device *dev) >> { >> @@ -199,10 +199,11 @@ int drm_mode_config_helper_suspend(struct drm_device *dev) >> if (dev->mode_config.poll_enabled) >> drm_kms_helper_poll_disable(dev); >> >> - drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 1); >> + drm_client_dev_suspend(dev, false); >> state = drm_atomic_helper_suspend(dev); >> if (IS_ERR(state)) { >> - drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0); >> + drm_client_dev_resume(dev, false); >> + >> /* >> * Don't enable polling if it was never initialized >> */ >> @@ -230,7 +231,7 @@ EXPORT_SYMBOL(drm_mode_config_helper_suspend); >> * Zero on success, negative error code on error. >> * >> * See also: >> - * drm_fb_helper_set_suspend_unlocked() and drm_kms_helper_poll_enable(). >> + * drm_client_dev_resume() and drm_kms_helper_poll_enable(). >> */ >> int drm_mode_config_helper_resume(struct drm_device *dev) >> { >> @@ -247,7 +248,8 @@ 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_fb_helper_set_suspend_unlocked(dev->fb_helper, 0); >> + drm_client_dev_resume(dev, false); >> + >> /* >> * Don't enable polling if it is not initialized >> */ >> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h >> index dfd5afcc9463..c03c4b0f3e94 100644 >> --- a/include/drm/drm_client.h >> +++ b/include/drm/drm_client.h >> @@ -63,6 +63,34 @@ struct drm_client_funcs { >> * This callback is optional. >> */ >> int (*hotplug)(struct drm_client_dev *client); >> + >> + /** >> + * @suspend: >> + * >> + * 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'. >> + */ > Is there an estimated time for the fix to this FIXME? It's out of scope > for this series, so I won't insist on fixing it immediately, but if it's a > "quick" fix (relatively speaking), then we should definitely try to get > this FIXME resolved in short order. There's no quick fix AFAICT. Radeon, i915 and xe call drm_fb_helper_set_suspend() from various places. These drivers always had their own code, so they likely need some work to make the change to the _unlocked() helper. Best regards Thomas > > -Jonathan Cavitt > >> + int (*suspend)(struct drm_client_dev *client, bool holds_console_lock); >> + >> + /** >> + * @resume: >> + * >> + * 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); >> }; >> >> /** >> @@ -107,6 +135,13 @@ struct drm_client_dev { >> */ >> struct drm_mode_set *modesets; >> >> + /** >> + * @suspended: >> + * >> + * The client has been suspended. >> + */ >> + bool suspended; >> + >> /** >> * @hotplug_failed: >> * >> diff --git a/include/drm/drm_client_event.h b/include/drm/drm_client_event.h >> index 2c8915241120..72c97d111169 100644 >> --- a/include/drm/drm_client_event.h >> +++ b/include/drm/drm_client_event.h >> @@ -8,5 +8,7 @@ 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); >> >> #endif >> -- >> 2.46.0 >> >>
diff --git a/drivers/gpu/drm/drm_client_event.c b/drivers/gpu/drm/drm_client_event.c index d13d44320c5c..c52e93643672 100644 --- a/drivers/gpu/drm/drm_client_event.c +++ b/drivers/gpu/drm/drm_client_event.c @@ -107,6 +107,66 @@ 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) +{ + struct drm_device *dev = client->dev; + int ret = 0; + + if (drm_WARN_ON_ONCE(dev, client->suspended)) + return 0; + + if (client->funcs && client->funcs->suspend) + ret = client->funcs->suspend(client, holds_console_lock); + drm_dbg_kms(dev, "%s: ret=%d\n", client->name, ret); + + client->suspended = true; + + return ret; +} + +void drm_client_dev_suspend(struct drm_device *dev, bool holds_console_lock) +{ + 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); + } + 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) +{ + struct drm_device *dev = client->dev; + int ret = 0; + + if (drm_WARN_ON_ONCE(dev, !client->suspended)) + return 0; + + if (client->funcs && client->funcs->resume) + ret = client->funcs->resume(client, holds_console_lock); + drm_dbg_kms(dev, "%s: ret=%d\n", client->name, ret); + + client->suspended = false; + + return ret; +} + +void drm_client_dev_resume(struct drm_device *dev, bool holds_console_lock) +{ + 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); + } + mutex_unlock(&dev->clientlist_mutex); +} +EXPORT_SYMBOL(drm_client_dev_resume); + #ifdef CONFIG_DEBUG_FS static int drm_client_debugfs_internal_clients(struct seq_file *m, void *data) { diff --git a/drivers/gpu/drm/drm_fbdev_client.c b/drivers/gpu/drm/drm_fbdev_client.c index a09382afe2fb..246fb63ab250 100644 --- a/drivers/gpu/drm/drm_fbdev_client.c +++ b/drivers/gpu/drm/drm_fbdev_client.c @@ -61,11 +61,37 @@ 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) +{ + 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); + + return 0; +} + +static int drm_fbdev_client_resume(struct drm_client_dev *client, bool holds_console_lock) +{ + 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); + + return 0; +} + static const struct drm_client_funcs drm_fbdev_client_funcs = { .owner = THIS_MODULE, .unregister = drm_fbdev_client_unregister, .restore = drm_fbdev_client_restore, .hotplug = drm_fbdev_client_hotplug, + .suspend = drm_fbdev_client_suspend, + .resume = drm_fbdev_client_resume, }; /** @@ -76,8 +102,8 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = { * * This function sets up fbdev emulation. Restore, hotplug events and * teardown are all taken care of. Drivers that do suspend/resume need - * to call drm_fb_helper_set_suspend_unlocked() themselves. Simple - * drivers might use drm_mode_config_helper_suspend(). + * to call drm_client_dev_suspend() and drm_client_dev_resume() by + * themselves. Simple drivers might use drm_mode_config_helper_suspend(). * * This function is safe to call even when there are no connectors present. * Setup will be retried on the next hotplug event. diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c index 2c582020cb42..5565464c1734 100644 --- a/drivers/gpu/drm/drm_modeset_helper.c +++ b/drivers/gpu/drm/drm_modeset_helper.c @@ -21,7 +21,7 @@ */ #include <drm/drm_atomic_helper.h> -#include <drm/drm_fb_helper.h> +#include <drm/drm_client_event.h> #include <drm/drm_fourcc.h> #include <drm/drm_framebuffer.h> #include <drm/drm_modeset_helper.h> @@ -185,7 +185,7 @@ EXPORT_SYMBOL(drm_crtc_init); * Zero on success, negative error code on error. * * See also: - * drm_kms_helper_poll_disable() and drm_fb_helper_set_suspend_unlocked(). + * drm_kms_helper_poll_disable() and drm_client_dev_suspend(). */ int drm_mode_config_helper_suspend(struct drm_device *dev) { @@ -199,10 +199,11 @@ int drm_mode_config_helper_suspend(struct drm_device *dev) if (dev->mode_config.poll_enabled) drm_kms_helper_poll_disable(dev); - drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 1); + drm_client_dev_suspend(dev, false); state = drm_atomic_helper_suspend(dev); if (IS_ERR(state)) { - drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0); + drm_client_dev_resume(dev, false); + /* * Don't enable polling if it was never initialized */ @@ -230,7 +231,7 @@ EXPORT_SYMBOL(drm_mode_config_helper_suspend); * Zero on success, negative error code on error. * * See also: - * drm_fb_helper_set_suspend_unlocked() and drm_kms_helper_poll_enable(). + * drm_client_dev_resume() and drm_kms_helper_poll_enable(). */ int drm_mode_config_helper_resume(struct drm_device *dev) { @@ -247,7 +248,8 @@ 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_fb_helper_set_suspend_unlocked(dev->fb_helper, 0); + drm_client_dev_resume(dev, false); + /* * Don't enable polling if it is not initialized */ diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h index dfd5afcc9463..c03c4b0f3e94 100644 --- a/include/drm/drm_client.h +++ b/include/drm/drm_client.h @@ -63,6 +63,34 @@ struct drm_client_funcs { * This callback is optional. */ int (*hotplug)(struct drm_client_dev *client); + + /** + * @suspend: + * + * 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); + + /** + * @resume: + * + * 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); }; /** @@ -107,6 +135,13 @@ struct drm_client_dev { */ struct drm_mode_set *modesets; + /** + * @suspended: + * + * The client has been suspended. + */ + bool suspended; + /** * @hotplug_failed: * diff --git a/include/drm/drm_client_event.h b/include/drm/drm_client_event.h index 2c8915241120..72c97d111169 100644 --- a/include/drm/drm_client_event.h +++ b/include/drm/drm_client_event.h @@ -8,5 +8,7 @@ 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); #endif
Suspend and resume is still tied to fbdev emulation. Modeset helpers and several drivers call drm_fb_helper_set_suspend_unlocked() to inform the fbdev client about suspend/resume events. To make it work with arbitrary clients, add per-client callback functions for suspend and resume. Implement them for fbdev emulation with the existing drm_fb_helper_set_suspend_unlocked(). Then update DRM's modeset helpers to call the new interface. Clients that are not fbdev can now implement suspend/resume to their requirements. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/drm_client_event.c | 60 ++++++++++++++++++++++++++++ drivers/gpu/drm/drm_fbdev_client.c | 30 +++++++++++++- drivers/gpu/drm/drm_modeset_helper.c | 14 ++++--- include/drm/drm_client.h | 35 ++++++++++++++++ include/drm/drm_client_event.h | 2 + 5 files changed, 133 insertions(+), 8 deletions(-)