Re: [PATCH v3 07/12] drm/client: Move suspend/resume into DRM client callbacks

[Thomas Zimmermann <tzimmermann@suse.de>]

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
>>
>>

-- 
--
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)