All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/amdgpu: don't runtime suspend if there are displays attached (v2)
@ 2022-04-21  3:16 Alex Deucher
  2022-04-21  4:40 ` Alex Deucher
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Alex Deucher @ 2022-04-21  3:16 UTC (permalink / raw)
  To: amd-gfx; +Cc: Alex Deucher, Michele Ballabio

We normally runtime suspend when there are displays attached if they
are in the DPMS off state, however, if something wakes the GPU
we send a hotplug event on resume (in case any displays were connected
while the GPU was in suspend) which can cause userspace to light
up the displays again soon after they were turned off.

Prior to
commit 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's."),
the driver took a runtime pm reference when the fbdev emulation was
enabled because we didn't implement proper shadowing support for
vram access when the device was off so the device never runtime
suspended when there was a console bound.  Once that commit landed,
we now utilize the core fb helper implementation which properly
handles the emulation, so runtime pm now suspends in cases where it did
not before.  Ultimately, we need to sort out why runtime suspend in not
working in this case for some users, but this should restore similar
behavior to before.

v2: move check into runtime_suspend
v3: wake ups -> wakeups in comment, retain pm_runtime behavior in
    runtime_idle callback

Fixes: 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's.")
Tested-by: Michele Ballabio <ballabio.m@gmail.com>
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 105 ++++++++++++++++--------
 1 file changed, 70 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 4efaa183abcd..ebd37fb19cdb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2395,6 +2395,71 @@ static int amdgpu_pmops_restore(struct device *dev)
 	return amdgpu_device_resume(drm_dev, true);
 }
 
+static int amdgpu_runtime_idle_check_display(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct drm_device *drm_dev = pci_get_drvdata(pdev);
+	struct amdgpu_device *adev = drm_to_adev(drm_dev);
+
+	if (adev->mode_info.num_crtc) {
+		struct drm_connector *list_connector;
+		struct drm_connector_list_iter iter;
+		int ret = 0;
+
+		/* XXX: Return busy if any displays are connected to avoid
+		 * possible display wakeups after runtime resume due to
+		 * hotplug events in case any displays were connected while
+		 * the GPU was in suspend.  Remove this once that is fixed.
+		 */
+		mutex_lock(&drm_dev->mode_config.mutex);
+		drm_connector_list_iter_begin(drm_dev, &iter);
+		drm_for_each_connector_iter(list_connector, &iter) {
+			if (list_connector->status == connector_status_connected) {
+				ret = -EBUSY;
+				break;
+			}
+		}
+		drm_connector_list_iter_end(&iter);
+		mutex_unlock(&drm_dev->mode_config.mutex);
+
+		if (ret)
+			return ret;
+
+		if (amdgpu_device_has_dc_support(adev)) {
+			struct drm_crtc *crtc;
+
+			drm_for_each_crtc(crtc, drm_dev) {
+				drm_modeset_lock(&crtc->mutex, NULL);
+				if (crtc->state->active)
+					ret = -EBUSY;
+				drm_modeset_unlock(&crtc->mutex);
+				if (ret < 0)
+					break;
+			}
+		} else {
+			mutex_lock(&drm_dev->mode_config.mutex);
+			drm_modeset_lock(&drm_dev->mode_config.connection_mutex, NULL);
+
+			drm_connector_list_iter_begin(drm_dev, &iter);
+			drm_for_each_connector_iter(list_connector, &iter) {
+				if (list_connector->dpms ==  DRM_MODE_DPMS_ON) {
+					ret = -EBUSY;
+					break;
+				}
+			}
+
+			drm_connector_list_iter_end(&iter);
+
+			drm_modeset_unlock(&drm_dev->mode_config.connection_mutex);
+			mutex_unlock(&drm_dev->mode_config.mutex);
+		}
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int amdgpu_pmops_runtime_suspend(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
@@ -2407,6 +2472,10 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev)
 		return -EBUSY;
 	}
 
+	ret = amdgpu_runtime_idle_check_display(dev);
+	if (ret)
+		return ret;
+
 	/* wait for all rings to drain before suspending */
 	for (i = 0; i < AMDGPU_MAX_RINGS; i++) {
 		struct amdgpu_ring *ring = adev->rings[i];
@@ -2516,41 +2585,7 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
 		return -EBUSY;
 	}
 
-	if (amdgpu_device_has_dc_support(adev)) {
-		struct drm_crtc *crtc;
-
-		drm_for_each_crtc(crtc, drm_dev) {
-			drm_modeset_lock(&crtc->mutex, NULL);
-			if (crtc->state->active)
-				ret = -EBUSY;
-			drm_modeset_unlock(&crtc->mutex);
-			if (ret < 0)
-				break;
-		}
-
-	} else {
-		struct drm_connector *list_connector;
-		struct drm_connector_list_iter iter;
-
-		mutex_lock(&drm_dev->mode_config.mutex);
-		drm_modeset_lock(&drm_dev->mode_config.connection_mutex, NULL);
-
-		drm_connector_list_iter_begin(drm_dev, &iter);
-		drm_for_each_connector_iter(list_connector, &iter) {
-			if (list_connector->dpms ==  DRM_MODE_DPMS_ON) {
-				ret = -EBUSY;
-				break;
-			}
-		}
-
-		drm_connector_list_iter_end(&iter);
-
-		drm_modeset_unlock(&drm_dev->mode_config.connection_mutex);
-		mutex_unlock(&drm_dev->mode_config.mutex);
-	}
-
-	if (ret == -EBUSY)
-		DRM_DEBUG_DRIVER("failing to power off - crtc active\n");
+	ret = amdgpu_runtime_idle_check_display(dev);
 
 	pm_runtime_mark_last_busy(dev);
 	pm_runtime_autosuspend(dev);
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/amdgpu: don't runtime suspend if there are displays attached (v2)
  2022-04-21  3:16 [PATCH] drm/amdgpu: don't runtime suspend if there are displays attached (v2) Alex Deucher
@ 2022-04-21  4:40 ` Alex Deucher
  2022-04-21  5:23 ` Quan, Evan
  2022-04-21 11:11 ` Thorsten Leemhuis
  2 siblings, 0 replies; 13+ messages in thread
From: Alex Deucher @ 2022-04-21  4:40 UTC (permalink / raw)
  To: Alex Deucher; +Cc: Michele Ballabio, amd-gfx list

Should be v3, fixed locally.

Alex

On Wed, Apr 20, 2022 at 11:16 PM Alex Deucher <alexander.deucher@amd.com> wrote:
>
> We normally runtime suspend when there are displays attached if they
> are in the DPMS off state, however, if something wakes the GPU
> we send a hotplug event on resume (in case any displays were connected
> while the GPU was in suspend) which can cause userspace to light
> up the displays again soon after they were turned off.
>
> Prior to
> commit 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's."),
> the driver took a runtime pm reference when the fbdev emulation was
> enabled because we didn't implement proper shadowing support for
> vram access when the device was off so the device never runtime
> suspended when there was a console bound.  Once that commit landed,
> we now utilize the core fb helper implementation which properly
> handles the emulation, so runtime pm now suspends in cases where it did
> not before.  Ultimately, we need to sort out why runtime suspend in not
> working in this case for some users, but this should restore similar
> behavior to before.
>
> v2: move check into runtime_suspend
> v3: wake ups -> wakeups in comment, retain pm_runtime behavior in
>     runtime_idle callback
>
> Fixes: 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's.")
> Tested-by: Michele Ballabio <ballabio.m@gmail.com>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 105 ++++++++++++++++--------
>  1 file changed, 70 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 4efaa183abcd..ebd37fb19cdb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2395,6 +2395,71 @@ static int amdgpu_pmops_restore(struct device *dev)
>         return amdgpu_device_resume(drm_dev, true);
>  }
>
> +static int amdgpu_runtime_idle_check_display(struct device *dev)
> +{
> +       struct pci_dev *pdev = to_pci_dev(dev);
> +       struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +       struct amdgpu_device *adev = drm_to_adev(drm_dev);
> +
> +       if (adev->mode_info.num_crtc) {
> +               struct drm_connector *list_connector;
> +               struct drm_connector_list_iter iter;
> +               int ret = 0;
> +
> +               /* XXX: Return busy if any displays are connected to avoid
> +                * possible display wakeups after runtime resume due to
> +                * hotplug events in case any displays were connected while
> +                * the GPU was in suspend.  Remove this once that is fixed.
> +                */
> +               mutex_lock(&drm_dev->mode_config.mutex);
> +               drm_connector_list_iter_begin(drm_dev, &iter);
> +               drm_for_each_connector_iter(list_connector, &iter) {
> +                       if (list_connector->status == connector_status_connected) {
> +                               ret = -EBUSY;
> +                               break;
> +                       }
> +               }
> +               drm_connector_list_iter_end(&iter);
> +               mutex_unlock(&drm_dev->mode_config.mutex);
> +
> +               if (ret)
> +                       return ret;
> +
> +               if (amdgpu_device_has_dc_support(adev)) {
> +                       struct drm_crtc *crtc;
> +
> +                       drm_for_each_crtc(crtc, drm_dev) {
> +                               drm_modeset_lock(&crtc->mutex, NULL);
> +                               if (crtc->state->active)
> +                                       ret = -EBUSY;
> +                               drm_modeset_unlock(&crtc->mutex);
> +                               if (ret < 0)
> +                                       break;
> +                       }
> +               } else {
> +                       mutex_lock(&drm_dev->mode_config.mutex);
> +                       drm_modeset_lock(&drm_dev->mode_config.connection_mutex, NULL);
> +
> +                       drm_connector_list_iter_begin(drm_dev, &iter);
> +                       drm_for_each_connector_iter(list_connector, &iter) {
> +                               if (list_connector->dpms ==  DRM_MODE_DPMS_ON) {
> +                                       ret = -EBUSY;
> +                                       break;
> +                               }
> +                       }
> +
> +                       drm_connector_list_iter_end(&iter);
> +
> +                       drm_modeset_unlock(&drm_dev->mode_config.connection_mutex);
> +                       mutex_unlock(&drm_dev->mode_config.mutex);
> +               }
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       return 0;
> +}
> +
>  static int amdgpu_pmops_runtime_suspend(struct device *dev)
>  {
>         struct pci_dev *pdev = to_pci_dev(dev);
> @@ -2407,6 +2472,10 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev)
>                 return -EBUSY;
>         }
>
> +       ret = amdgpu_runtime_idle_check_display(dev);
> +       if (ret)
> +               return ret;
> +
>         /* wait for all rings to drain before suspending */
>         for (i = 0; i < AMDGPU_MAX_RINGS; i++) {
>                 struct amdgpu_ring *ring = adev->rings[i];
> @@ -2516,41 +2585,7 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
>                 return -EBUSY;
>         }
>
> -       if (amdgpu_device_has_dc_support(adev)) {
> -               struct drm_crtc *crtc;
> -
> -               drm_for_each_crtc(crtc, drm_dev) {
> -                       drm_modeset_lock(&crtc->mutex, NULL);
> -                       if (crtc->state->active)
> -                               ret = -EBUSY;
> -                       drm_modeset_unlock(&crtc->mutex);
> -                       if (ret < 0)
> -                               break;
> -               }
> -
> -       } else {
> -               struct drm_connector *list_connector;
> -               struct drm_connector_list_iter iter;
> -
> -               mutex_lock(&drm_dev->mode_config.mutex);
> -               drm_modeset_lock(&drm_dev->mode_config.connection_mutex, NULL);
> -
> -               drm_connector_list_iter_begin(drm_dev, &iter);
> -               drm_for_each_connector_iter(list_connector, &iter) {
> -                       if (list_connector->dpms ==  DRM_MODE_DPMS_ON) {
> -                               ret = -EBUSY;
> -                               break;
> -                       }
> -               }
> -
> -               drm_connector_list_iter_end(&iter);
> -
> -               drm_modeset_unlock(&drm_dev->mode_config.connection_mutex);
> -               mutex_unlock(&drm_dev->mode_config.mutex);
> -       }
> -
> -       if (ret == -EBUSY)
> -               DRM_DEBUG_DRIVER("failing to power off - crtc active\n");
> +       ret = amdgpu_runtime_idle_check_display(dev);
>
>         pm_runtime_mark_last_busy(dev);
>         pm_runtime_autosuspend(dev);
> --
> 2.35.1
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH] drm/amdgpu: don't runtime suspend if there are displays attached (v2)
  2022-04-21  3:16 [PATCH] drm/amdgpu: don't runtime suspend if there are displays attached (v2) Alex Deucher
  2022-04-21  4:40 ` Alex Deucher
@ 2022-04-21  5:23 ` Quan, Evan
  2022-04-21 11:11 ` Thorsten Leemhuis
  2 siblings, 0 replies; 13+ messages in thread
From: Quan, Evan @ 2022-04-21  5:23 UTC (permalink / raw)
  To: Deucher, Alexander, amd-gfx; +Cc: Deucher, Alexander, Michele Ballabio

[AMD Official Use Only]

Reviewed-by: Evan Quan <evan.quan@amd.com>

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Alex
> Deucher
> Sent: Thursday, April 21, 2022 11:16 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Michele Ballabio
> <ballabio.m@gmail.com>
> Subject: [PATCH] drm/amdgpu: don't runtime suspend if there are displays
> attached (v2)
> 
> We normally runtime suspend when there are displays attached if they
> are in the DPMS off state, however, if something wakes the GPU
> we send a hotplug event on resume (in case any displays were connected
> while the GPU was in suspend) which can cause userspace to light
> up the displays again soon after they were turned off.
> 
> Prior to
> commit 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of
> setting up AMD own's."),
> the driver took a runtime pm reference when the fbdev emulation was
> enabled because we didn't implement proper shadowing support for
> vram access when the device was off so the device never runtime
> suspended when there was a console bound.  Once that commit landed,
> we now utilize the core fb helper implementation which properly
> handles the emulation, so runtime pm now suspends in cases where it did
> not before.  Ultimately, we need to sort out why runtime suspend in not
> working in this case for some users, but this should restore similar
> behavior to before.
> 
> v2: move check into runtime_suspend
> v3: wake ups -> wakeups in comment, retain pm_runtime behavior in
>     runtime_idle callback
> 
> Fixes: 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of
> setting up AMD own's.")
> Tested-by: Michele Ballabio <ballabio.m@gmail.com>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 105 ++++++++++++++++---
> -----
>  1 file changed, 70 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 4efaa183abcd..ebd37fb19cdb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2395,6 +2395,71 @@ static int amdgpu_pmops_restore(struct device
> *dev)
>  	return amdgpu_device_resume(drm_dev, true);
>  }
> 
> +static int amdgpu_runtime_idle_check_display(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +	struct amdgpu_device *adev = drm_to_adev(drm_dev);
> +
> +	if (adev->mode_info.num_crtc) {
> +		struct drm_connector *list_connector;
> +		struct drm_connector_list_iter iter;
> +		int ret = 0;
> +
> +		/* XXX: Return busy if any displays are connected to avoid
> +		 * possible display wakeups after runtime resume due to
> +		 * hotplug events in case any displays were connected while
> +		 * the GPU was in suspend.  Remove this once that is fixed.
> +		 */
> +		mutex_lock(&drm_dev->mode_config.mutex);
> +		drm_connector_list_iter_begin(drm_dev, &iter);
> +		drm_for_each_connector_iter(list_connector, &iter) {
> +			if (list_connector->status ==
> connector_status_connected) {
> +				ret = -EBUSY;
> +				break;
> +			}
> +		}
> +		drm_connector_list_iter_end(&iter);
> +		mutex_unlock(&drm_dev->mode_config.mutex);
> +
> +		if (ret)
> +			return ret;
> +
> +		if (amdgpu_device_has_dc_support(adev)) {
> +			struct drm_crtc *crtc;
> +
> +			drm_for_each_crtc(crtc, drm_dev) {
> +				drm_modeset_lock(&crtc->mutex, NULL);
> +				if (crtc->state->active)
> +					ret = -EBUSY;
> +				drm_modeset_unlock(&crtc->mutex);
> +				if (ret < 0)
> +					break;
> +			}
> +		} else {
> +			mutex_lock(&drm_dev->mode_config.mutex);
> +			drm_modeset_lock(&drm_dev-
> >mode_config.connection_mutex, NULL);
> +
> +			drm_connector_list_iter_begin(drm_dev, &iter);
> +			drm_for_each_connector_iter(list_connector, &iter)
> {
> +				if (list_connector->dpms ==
> DRM_MODE_DPMS_ON) {
> +					ret = -EBUSY;
> +					break;
> +				}
> +			}
> +
> +			drm_connector_list_iter_end(&iter);
> +
> +			drm_modeset_unlock(&drm_dev-
> >mode_config.connection_mutex);
> +			mutex_unlock(&drm_dev->mode_config.mutex);
> +		}
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int amdgpu_pmops_runtime_suspend(struct device *dev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> @@ -2407,6 +2472,10 @@ static int
> amdgpu_pmops_runtime_suspend(struct device *dev)
>  		return -EBUSY;
>  	}
> 
> +	ret = amdgpu_runtime_idle_check_display(dev);
> +	if (ret)
> +		return ret;
> +
>  	/* wait for all rings to drain before suspending */
>  	for (i = 0; i < AMDGPU_MAX_RINGS; i++) {
>  		struct amdgpu_ring *ring = adev->rings[i];
> @@ -2516,41 +2585,7 @@ static int amdgpu_pmops_runtime_idle(struct
> device *dev)
>  		return -EBUSY;
>  	}
> 
> -	if (amdgpu_device_has_dc_support(adev)) {
> -		struct drm_crtc *crtc;
> -
> -		drm_for_each_crtc(crtc, drm_dev) {
> -			drm_modeset_lock(&crtc->mutex, NULL);
> -			if (crtc->state->active)
> -				ret = -EBUSY;
> -			drm_modeset_unlock(&crtc->mutex);
> -			if (ret < 0)
> -				break;
> -		}
> -
> -	} else {
> -		struct drm_connector *list_connector;
> -		struct drm_connector_list_iter iter;
> -
> -		mutex_lock(&drm_dev->mode_config.mutex);
> -		drm_modeset_lock(&drm_dev-
> >mode_config.connection_mutex, NULL);
> -
> -		drm_connector_list_iter_begin(drm_dev, &iter);
> -		drm_for_each_connector_iter(list_connector, &iter) {
> -			if (list_connector->dpms ==  DRM_MODE_DPMS_ON)
> {
> -				ret = -EBUSY;
> -				break;
> -			}
> -		}
> -
> -		drm_connector_list_iter_end(&iter);
> -
> -		drm_modeset_unlock(&drm_dev-
> >mode_config.connection_mutex);
> -		mutex_unlock(&drm_dev->mode_config.mutex);
> -	}
> -
> -	if (ret == -EBUSY)
> -		DRM_DEBUG_DRIVER("failing to power off - crtc active\n");
> +	ret = amdgpu_runtime_idle_check_display(dev);
> 
>  	pm_runtime_mark_last_busy(dev);
>  	pm_runtime_autosuspend(dev);
> --
> 2.35.1

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/amdgpu: don't runtime suspend if there are displays attached (v2)
  2022-04-21  3:16 [PATCH] drm/amdgpu: don't runtime suspend if there are displays attached (v2) Alex Deucher
  2022-04-21  4:40 ` Alex Deucher
  2022-04-21  5:23 ` Quan, Evan
@ 2022-04-21 11:11 ` Thorsten Leemhuis
  2022-04-21 14:19   ` Alex Deucher
  2 siblings, 1 reply; 13+ messages in thread
From: Thorsten Leemhuis @ 2022-04-21 11:11 UTC (permalink / raw)
  To: Alex Deucher, amd-gfx; +Cc: Michele Ballabio

On 21.04.22 05:16, Alex Deucher wrote:
> We normally runtime suspend when there are displays attached if they
> are in the DPMS off state, however, if something wakes the GPU
> we send a hotplug event on resume (in case any displays were connected
> while the GPU was in suspend) which can cause userspace to light
> up the displays again soon after they were turned off.
> 
> Prior to
> commit 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's."),
> the driver took a runtime pm reference when the fbdev emulation was
> enabled because we didn't implement proper shadowing support for
> vram access when the device was off so the device never runtime
> suspended when there was a console bound.  Once that commit landed,
> we now utilize the core fb helper implementation which properly
> handles the emulation, so runtime pm now suspends in cases where it did
> not before.  Ultimately, we need to sort out why runtime suspend in not
> working in this case for some users, but this should restore similar
> behavior to before.
> 
> v2: move check into runtime_suspend
> v3: wake ups -> wakeups in comment, retain pm_runtime behavior in
>     runtime_idle callback
> 
> Fixes: 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's.")
> Tested-by: Michele Ballabio <ballabio.m@gmail.com>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> [...]

Hi Alex, how can I bribe you to start placing "Link:" tags in
submissions that fix regressions (like this one), as explained in the
Linux kernels documentation (see
'Documentation/process/submitting-patches.rst' and
'Documentation/process/5.Posting.rst'). E.g. in this case like this:

"Link:
https://lore.kernel.org/r/20220403132322.51c90903@darkstar.example.org/"

This concept is not new (Linus and quite a few other developers use them
like this for a long time), I just recently improved those documents to
clarify things, as my regression tracking efforts rely on them -- that's
why it's making my work a lot harder if they are missing most of the
time. :-/

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)

P.S.: As the Linux kernel's regression tracker I'm getting a lot of
reports on my table. I can only look briefly into most of them and lack
knowledge about most of the areas they concern. I thus unfortunately
will sometimes get things wrong or miss something important. I hope
that's not the case here; if you think it is, don't hesitate to tell me
in a public reply, it's in everyone's interest to set the public record
straight.

#regzbot ^backmonitor:
https://lore.kernel.org/lkml/20220403132322.51c90903@darkstar.example.org/

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/amdgpu: don't runtime suspend if there are displays attached (v2)
  2022-04-21 11:11 ` Thorsten Leemhuis
@ 2022-04-21 14:19   ` Alex Deucher
  0 siblings, 0 replies; 13+ messages in thread
From: Alex Deucher @ 2022-04-21 14:19 UTC (permalink / raw)
  To: Thorsten Leemhuis; +Cc: Alex Deucher, Michele Ballabio, amd-gfx list

On Thu, Apr 21, 2022 at 9:06 AM Thorsten Leemhuis
<regressions@leemhuis.info> wrote:
>
> On 21.04.22 05:16, Alex Deucher wrote:
> > We normally runtime suspend when there are displays attached if they
> > are in the DPMS off state, however, if something wakes the GPU
> > we send a hotplug event on resume (in case any displays were connected
> > while the GPU was in suspend) which can cause userspace to light
> > up the displays again soon after they were turned off.
> >
> > Prior to
> > commit 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's."),
> > the driver took a runtime pm reference when the fbdev emulation was
> > enabled because we didn't implement proper shadowing support for
> > vram access when the device was off so the device never runtime
> > suspended when there was a console bound.  Once that commit landed,
> > we now utilize the core fb helper implementation which properly
> > handles the emulation, so runtime pm now suspends in cases where it did
> > not before.  Ultimately, we need to sort out why runtime suspend in not
> > working in this case for some users, but this should restore similar
> > behavior to before.
> >
> > v2: move check into runtime_suspend
> > v3: wake ups -> wakeups in comment, retain pm_runtime behavior in
> >     runtime_idle callback
> >
> > Fixes: 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's.")
> > Tested-by: Michele Ballabio <ballabio.m@gmail.com>
> > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> > [...]
>
> Hi Alex, how can I bribe you to start placing "Link:" tags in
> submissions that fix regressions (like this one), as explained in the
> Linux kernels documentation (see
> 'Documentation/process/submitting-patches.rst' and
> 'Documentation/process/5.Posting.rst'). E.g. in this case like this:
>
> "Link:
> https://lore.kernel.org/r/20220403132322.51c90903@darkstar.example.org/"
>

Done.  Thanks.

Alex

> This concept is not new (Linus and quite a few other developers use them
> like this for a long time), I just recently improved those documents to
> clarify things, as my regression tracking efforts rely on them -- that's
> why it's making my work a lot harder if they are missing most of the
> time. :-/
>
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
>
> P.S.: As the Linux kernel's regression tracker I'm getting a lot of
> reports on my table. I can only look briefly into most of them and lack
> knowledge about most of the areas they concern. I thus unfortunately
> will sometimes get things wrong or miss something important. I hope
> that's not the case here; if you think it is, don't hesitate to tell me
> in a public reply, it's in everyone's interest to set the public record
> straight.
>
> #regzbot ^backmonitor:
> https://lore.kernel.org/lkml/20220403132322.51c90903@darkstar.example.org/

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/amdgpu: don't runtime suspend if there are displays attached (v2)
  2022-04-21  2:53 ` Quan, Evan
@ 2022-04-21  3:00   ` Alex Deucher
  0 siblings, 0 replies; 13+ messages in thread
From: Alex Deucher @ 2022-04-21  3:00 UTC (permalink / raw)
  To: Quan, Evan; +Cc: Deucher, Alexander, Michele Ballabio, amd-gfx

On Wed, Apr 20, 2022 at 10:54 PM Quan, Evan <Evan.Quan@amd.com> wrote:
>
> [AMD Official Use Only]
>
>
>
> > -----Original Message-----
> > From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Alex
> > Deucher
> > Sent: Thursday, April 14, 2022 4:15 AM
> > To: amd-gfx@lists.freedesktop.org
> > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Michele Ballabio
> > <ballabio.m@gmail.com>
> > Subject: [PATCH] drm/amdgpu: don't runtime suspend if there are displays
> > attached (v2)
> >
> > We normally runtime suspend when there are displays attached if they
> > are in the DPMS off state, however, if something wakes the GPU
> > we send a hotplug event on resume (in case any displays were connected
> > while the GPU was in suspend) which can cause userspace to light
> > up the displays again soon after they were turned off.
> >
> > Prior to
> > commit 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of
> > setting up AMD own's."),
> > the driver took a runtime pm reference when the fbdev emulation was
> > enabled because we didn't implement proper shadowing support for
> > vram access when the device was off so the device never runtime
> > suspended when there was a console bound.  Once that commit landed,
> > we now utilize the core fb helper implementation which properly
> > handles the emulation, so runtime pm now suspends in cases where it did
> > not before.  Ultimately, we need to sort out why runtime suspend in not
> > working in this case for some users, but this should restore similar
> > behavior to before.
> >
> > v2: move check into runtime_suspend
> >
> > Fixes: 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of
> > setting up AMD own's.")
> > Tested-by: Michele Ballabio <ballabio.m@gmail.com>
> > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 107 ++++++++++++++++---
> > -----
> >  1 file changed, 72 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > index 4efaa183abcd..97a1aa02d76e 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > @@ -2395,6 +2395,71 @@ static int amdgpu_pmops_restore(struct device
> > *dev)
> >       return amdgpu_device_resume(drm_dev, true);
> >  }
> >
> > +static int amdgpu_runtime_idle_check_display(struct device *dev)
> > +{
> > +     struct pci_dev *pdev = to_pci_dev(dev);
> > +     struct drm_device *drm_dev = pci_get_drvdata(pdev);
> > +     struct amdgpu_device *adev = drm_to_adev(drm_dev);
> > +
> > +     if (adev->mode_info.num_crtc) {
> > +             struct drm_connector *list_connector;
> > +             struct drm_connector_list_iter iter;
> > +             int ret = 0;
> > +
> > +             /* XXX: Return busy if any displays are connected to avoid
> > +              * possible display wake ups after runtime resume due to
> > +              * hotplug events in case any displays were connected while
> > +              * the GPU was in suspend.  Remove this once that is fixed.
> > +              */
> > +             mutex_lock(&drm_dev->mode_config.mutex);
> > +             drm_connector_list_iter_begin(drm_dev, &iter);
> > +             drm_for_each_connector_iter(list_connector, &iter) {
> > +                     if (list_connector->status ==
> > connector_status_connected) {
> > +                             ret = -EBUSY;
> > +                             break;
> > +                     }
> > +             }
> > +             drm_connector_list_iter_end(&iter);
> > +             mutex_unlock(&drm_dev->mode_config.mutex);
> > +
> > +             if (ret)
> > +                     return ret;
> > +
> > +             if (amdgpu_device_has_dc_support(adev)) {
> > +                     struct drm_crtc *crtc;
> > +
> > +                     drm_for_each_crtc(crtc, drm_dev) {
> > +                             drm_modeset_lock(&crtc->mutex, NULL);
> > +                             if (crtc->state->active)
> > +                                     ret = -EBUSY;
> > +                             drm_modeset_unlock(&crtc->mutex);
> > +                             if (ret < 0)
> > +                                     break;
> > +                     }
> > +             } else {
> > +                     mutex_lock(&drm_dev->mode_config.mutex);
> > +                     drm_modeset_lock(&drm_dev-
> > >mode_config.connection_mutex, NULL);
> > +
> > +                     drm_connector_list_iter_begin(drm_dev, &iter);
> > +                     drm_for_each_connector_iter(list_connector, &iter)
> > {
> > +                             if (list_connector->dpms ==
> > DRM_MODE_DPMS_ON) {
> > +                                     ret = -EBUSY;
> > +                                     break;
> > +                             }
> > +                     }
> > +
> > +                     drm_connector_list_iter_end(&iter);
> > +
> > +                     drm_modeset_unlock(&drm_dev-
> > >mode_config.connection_mutex);
> > +                     mutex_unlock(&drm_dev->mode_config.mutex);
> > +             }
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >  static int amdgpu_pmops_runtime_suspend(struct device *dev)
> >  {
> >       struct pci_dev *pdev = to_pci_dev(dev);
> > @@ -2407,6 +2472,10 @@ static int
> > amdgpu_pmops_runtime_suspend(struct device *dev)
> >               return -EBUSY;
> >       }
> >
> > +     ret = amdgpu_runtime_idle_check_display(dev);
> > +     if (ret)
> > +             return ret;
> > +
> >       /* wait for all rings to drain before suspending */
> >       for (i = 0; i < AMDGPU_MAX_RINGS; i++) {
> >               struct amdgpu_ring *ring = adev->rings[i];
> > @@ -2516,41 +2585,9 @@ static int amdgpu_pmops_runtime_idle(struct
> > device *dev)
> >               return -EBUSY;
> >       }
> >
> > -     if (amdgpu_device_has_dc_support(adev)) {
> > -             struct drm_crtc *crtc;
> > -
> > -             drm_for_each_crtc(crtc, drm_dev) {
> > -                     drm_modeset_lock(&crtc->mutex, NULL);
> > -                     if (crtc->state->active)
> > -                             ret = -EBUSY;
> > -                     drm_modeset_unlock(&crtc->mutex);
> > -                     if (ret < 0)
> > -                             break;
> > -             }
> > -
> > -     } else {
> > -             struct drm_connector *list_connector;
> > -             struct drm_connector_list_iter iter;
> > -
> > -             mutex_lock(&drm_dev->mode_config.mutex);
> > -             drm_modeset_lock(&drm_dev-
> > >mode_config.connection_mutex, NULL);
> > -
> > -             drm_connector_list_iter_begin(drm_dev, &iter);
> > -             drm_for_each_connector_iter(list_connector, &iter) {
> > -                     if (list_connector->dpms ==  DRM_MODE_DPMS_ON)
> > {
> > -                             ret = -EBUSY;
> > -                             break;
> > -                     }
> > -             }
> > -
> > -             drm_connector_list_iter_end(&iter);
> > -
> > -             drm_modeset_unlock(&drm_dev-
> > >mode_config.connection_mutex);
> > -             mutex_unlock(&drm_dev->mode_config.mutex);
> > -     }
> > -
> > -     if (ret == -EBUSY)
> > -             DRM_DEBUG_DRIVER("failing to power off - crtc active\n");
> > +     ret = amdgpu_runtime_idle_check_display(dev);
> > +     if (ret)
> > +             return ret;
> [Quan, Evan] Previously, the following actions(pm_runtime_mark_last_busy/pm_runtime_autosuspend) will be still performed even if ret is set as EBUSY.
> But with the new changes, it will abort directly without the following actions performed.
> Is that expected? Will that make any logical change?

Good point.  I'll fix that up.

Alex

>
> BR
> Evan
> >
> >       pm_runtime_mark_last_busy(dev);
> >       pm_runtime_autosuspend(dev);
> > --
> > 2.35.1

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH] drm/amdgpu: don't runtime suspend if there are displays attached (v2)
  2022-04-13 20:15 Alex Deucher
  2022-04-19 13:47 ` Alex Deucher
  2022-04-19 14:04 ` Paul Menzel
@ 2022-04-21  2:53 ` Quan, Evan
  2022-04-21  3:00   ` Alex Deucher
  2 siblings, 1 reply; 13+ messages in thread
From: Quan, Evan @ 2022-04-21  2:53 UTC (permalink / raw)
  To: Deucher, Alexander, amd-gfx; +Cc: Deucher, Alexander, Michele Ballabio

[AMD Official Use Only]



> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Alex
> Deucher
> Sent: Thursday, April 14, 2022 4:15 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Michele Ballabio
> <ballabio.m@gmail.com>
> Subject: [PATCH] drm/amdgpu: don't runtime suspend if there are displays
> attached (v2)
> 
> We normally runtime suspend when there are displays attached if they
> are in the DPMS off state, however, if something wakes the GPU
> we send a hotplug event on resume (in case any displays were connected
> while the GPU was in suspend) which can cause userspace to light
> up the displays again soon after they were turned off.
> 
> Prior to
> commit 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of
> setting up AMD own's."),
> the driver took a runtime pm reference when the fbdev emulation was
> enabled because we didn't implement proper shadowing support for
> vram access when the device was off so the device never runtime
> suspended when there was a console bound.  Once that commit landed,
> we now utilize the core fb helper implementation which properly
> handles the emulation, so runtime pm now suspends in cases where it did
> not before.  Ultimately, we need to sort out why runtime suspend in not
> working in this case for some users, but this should restore similar
> behavior to before.
> 
> v2: move check into runtime_suspend
> 
> Fixes: 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of
> setting up AMD own's.")
> Tested-by: Michele Ballabio <ballabio.m@gmail.com>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 107 ++++++++++++++++---
> -----
>  1 file changed, 72 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 4efaa183abcd..97a1aa02d76e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2395,6 +2395,71 @@ static int amdgpu_pmops_restore(struct device
> *dev)
>  	return amdgpu_device_resume(drm_dev, true);
>  }
> 
> +static int amdgpu_runtime_idle_check_display(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +	struct amdgpu_device *adev = drm_to_adev(drm_dev);
> +
> +	if (adev->mode_info.num_crtc) {
> +		struct drm_connector *list_connector;
> +		struct drm_connector_list_iter iter;
> +		int ret = 0;
> +
> +		/* XXX: Return busy if any displays are connected to avoid
> +		 * possible display wake ups after runtime resume due to
> +		 * hotplug events in case any displays were connected while
> +		 * the GPU was in suspend.  Remove this once that is fixed.
> +		 */
> +		mutex_lock(&drm_dev->mode_config.mutex);
> +		drm_connector_list_iter_begin(drm_dev, &iter);
> +		drm_for_each_connector_iter(list_connector, &iter) {
> +			if (list_connector->status ==
> connector_status_connected) {
> +				ret = -EBUSY;
> +				break;
> +			}
> +		}
> +		drm_connector_list_iter_end(&iter);
> +		mutex_unlock(&drm_dev->mode_config.mutex);
> +
> +		if (ret)
> +			return ret;
> +
> +		if (amdgpu_device_has_dc_support(adev)) {
> +			struct drm_crtc *crtc;
> +
> +			drm_for_each_crtc(crtc, drm_dev) {
> +				drm_modeset_lock(&crtc->mutex, NULL);
> +				if (crtc->state->active)
> +					ret = -EBUSY;
> +				drm_modeset_unlock(&crtc->mutex);
> +				if (ret < 0)
> +					break;
> +			}
> +		} else {
> +			mutex_lock(&drm_dev->mode_config.mutex);
> +			drm_modeset_lock(&drm_dev-
> >mode_config.connection_mutex, NULL);
> +
> +			drm_connector_list_iter_begin(drm_dev, &iter);
> +			drm_for_each_connector_iter(list_connector, &iter)
> {
> +				if (list_connector->dpms ==
> DRM_MODE_DPMS_ON) {
> +					ret = -EBUSY;
> +					break;
> +				}
> +			}
> +
> +			drm_connector_list_iter_end(&iter);
> +
> +			drm_modeset_unlock(&drm_dev-
> >mode_config.connection_mutex);
> +			mutex_unlock(&drm_dev->mode_config.mutex);
> +		}
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int amdgpu_pmops_runtime_suspend(struct device *dev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> @@ -2407,6 +2472,10 @@ static int
> amdgpu_pmops_runtime_suspend(struct device *dev)
>  		return -EBUSY;
>  	}
> 
> +	ret = amdgpu_runtime_idle_check_display(dev);
> +	if (ret)
> +		return ret;
> +
>  	/* wait for all rings to drain before suspending */
>  	for (i = 0; i < AMDGPU_MAX_RINGS; i++) {
>  		struct amdgpu_ring *ring = adev->rings[i];
> @@ -2516,41 +2585,9 @@ static int amdgpu_pmops_runtime_idle(struct
> device *dev)
>  		return -EBUSY;
>  	}
> 
> -	if (amdgpu_device_has_dc_support(adev)) {
> -		struct drm_crtc *crtc;
> -
> -		drm_for_each_crtc(crtc, drm_dev) {
> -			drm_modeset_lock(&crtc->mutex, NULL);
> -			if (crtc->state->active)
> -				ret = -EBUSY;
> -			drm_modeset_unlock(&crtc->mutex);
> -			if (ret < 0)
> -				break;
> -		}
> -
> -	} else {
> -		struct drm_connector *list_connector;
> -		struct drm_connector_list_iter iter;
> -
> -		mutex_lock(&drm_dev->mode_config.mutex);
> -		drm_modeset_lock(&drm_dev-
> >mode_config.connection_mutex, NULL);
> -
> -		drm_connector_list_iter_begin(drm_dev, &iter);
> -		drm_for_each_connector_iter(list_connector, &iter) {
> -			if (list_connector->dpms ==  DRM_MODE_DPMS_ON)
> {
> -				ret = -EBUSY;
> -				break;
> -			}
> -		}
> -
> -		drm_connector_list_iter_end(&iter);
> -
> -		drm_modeset_unlock(&drm_dev-
> >mode_config.connection_mutex);
> -		mutex_unlock(&drm_dev->mode_config.mutex);
> -	}
> -
> -	if (ret == -EBUSY)
> -		DRM_DEBUG_DRIVER("failing to power off - crtc active\n");
> +	ret = amdgpu_runtime_idle_check_display(dev);
> +	if (ret)
> +		return ret;
[Quan, Evan] Previously, the following actions(pm_runtime_mark_last_busy/pm_runtime_autosuspend) will be still performed even if ret is set as EBUSY.
But with the new changes, it will abort directly without the following actions performed.
Is that expected? Will that make any logical change?

BR
Evan
> 
>  	pm_runtime_mark_last_busy(dev);
>  	pm_runtime_autosuspend(dev);
> --
> 2.35.1

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/amdgpu: don't runtime suspend if there are displays attached (v2)
  2022-04-20 13:57   ` Alex Deucher
@ 2022-04-20 14:05     ` Christian König
  0 siblings, 0 replies; 13+ messages in thread
From: Christian König @ 2022-04-20 14:05 UTC (permalink / raw)
  To: Alex Deucher, Alex Deucher; +Cc: amd-gfx list, Michele Ballabio

I could ack it, but I'm absolutely not an expert on that stuff.

Our DC team maybe? Or anybody working more on the PM code?

Christian.

Am 20.04.22 um 15:57 schrieb Alex Deucher:
> Ping?  Anyone care to review this?
>
> Alex
>
> On Tue, Apr 19, 2022 at 9:47 AM Alex Deucher <alexdeucher@gmail.com> wrote:
>> Ping?
>>
>> On Wed, Apr 13, 2022 at 4:15 PM Alex Deucher <alexander.deucher@amd.com> wrote:
>>> We normally runtime suspend when there are displays attached if they
>>> are in the DPMS off state, however, if something wakes the GPU
>>> we send a hotplug event on resume (in case any displays were connected
>>> while the GPU was in suspend) which can cause userspace to light
>>> up the displays again soon after they were turned off.
>>>
>>> Prior to
>>> commit 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's."),
>>> the driver took a runtime pm reference when the fbdev emulation was
>>> enabled because we didn't implement proper shadowing support for
>>> vram access when the device was off so the device never runtime
>>> suspended when there was a console bound.  Once that commit landed,
>>> we now utilize the core fb helper implementation which properly
>>> handles the emulation, so runtime pm now suspends in cases where it did
>>> not before.  Ultimately, we need to sort out why runtime suspend in not
>>> working in this case for some users, but this should restore similar
>>> behavior to before.
>>>
>>> v2: move check into runtime_suspend
>>>
>>> Fixes: 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's.")
>>> Tested-by: Michele Ballabio <ballabio.m@gmail.com>
>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 107 ++++++++++++++++--------
>>>   1 file changed, 72 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> index 4efaa183abcd..97a1aa02d76e 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> @@ -2395,6 +2395,71 @@ static int amdgpu_pmops_restore(struct device *dev)
>>>          return amdgpu_device_resume(drm_dev, true);
>>>   }
>>>
>>> +static int amdgpu_runtime_idle_check_display(struct device *dev)
>>> +{
>>> +       struct pci_dev *pdev = to_pci_dev(dev);
>>> +       struct drm_device *drm_dev = pci_get_drvdata(pdev);
>>> +       struct amdgpu_device *adev = drm_to_adev(drm_dev);
>>> +
>>> +       if (adev->mode_info.num_crtc) {
>>> +               struct drm_connector *list_connector;
>>> +               struct drm_connector_list_iter iter;
>>> +               int ret = 0;
>>> +
>>> +               /* XXX: Return busy if any displays are connected to avoid
>>> +                * possible display wake ups after runtime resume due to
>>> +                * hotplug events in case any displays were connected while
>>> +                * the GPU was in suspend.  Remove this once that is fixed.
>>> +                */
>>> +               mutex_lock(&drm_dev->mode_config.mutex);
>>> +               drm_connector_list_iter_begin(drm_dev, &iter);
>>> +               drm_for_each_connector_iter(list_connector, &iter) {
>>> +                       if (list_connector->status == connector_status_connected) {
>>> +                               ret = -EBUSY;
>>> +                               break;
>>> +                       }
>>> +               }
>>> +               drm_connector_list_iter_end(&iter);
>>> +               mutex_unlock(&drm_dev->mode_config.mutex);
>>> +
>>> +               if (ret)
>>> +                       return ret;
>>> +
>>> +               if (amdgpu_device_has_dc_support(adev)) {
>>> +                       struct drm_crtc *crtc;
>>> +
>>> +                       drm_for_each_crtc(crtc, drm_dev) {
>>> +                               drm_modeset_lock(&crtc->mutex, NULL);
>>> +                               if (crtc->state->active)
>>> +                                       ret = -EBUSY;
>>> +                               drm_modeset_unlock(&crtc->mutex);
>>> +                               if (ret < 0)
>>> +                                       break;
>>> +                       }
>>> +               } else {
>>> +                       mutex_lock(&drm_dev->mode_config.mutex);
>>> +                       drm_modeset_lock(&drm_dev->mode_config.connection_mutex, NULL);
>>> +
>>> +                       drm_connector_list_iter_begin(drm_dev, &iter);
>>> +                       drm_for_each_connector_iter(list_connector, &iter) {
>>> +                               if (list_connector->dpms ==  DRM_MODE_DPMS_ON) {
>>> +                                       ret = -EBUSY;
>>> +                                       break;
>>> +                               }
>>> +                       }
>>> +
>>> +                       drm_connector_list_iter_end(&iter);
>>> +
>>> +                       drm_modeset_unlock(&drm_dev->mode_config.connection_mutex);
>>> +                       mutex_unlock(&drm_dev->mode_config.mutex);
>>> +               }
>>> +               if (ret)
>>> +                       return ret;
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>>   static int amdgpu_pmops_runtime_suspend(struct device *dev)
>>>   {
>>>          struct pci_dev *pdev = to_pci_dev(dev);
>>> @@ -2407,6 +2472,10 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev)
>>>                  return -EBUSY;
>>>          }
>>>
>>> +       ret = amdgpu_runtime_idle_check_display(dev);
>>> +       if (ret)
>>> +               return ret;
>>> +
>>>          /* wait for all rings to drain before suspending */
>>>          for (i = 0; i < AMDGPU_MAX_RINGS; i++) {
>>>                  struct amdgpu_ring *ring = adev->rings[i];
>>> @@ -2516,41 +2585,9 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
>>>                  return -EBUSY;
>>>          }
>>>
>>> -       if (amdgpu_device_has_dc_support(adev)) {
>>> -               struct drm_crtc *crtc;
>>> -
>>> -               drm_for_each_crtc(crtc, drm_dev) {
>>> -                       drm_modeset_lock(&crtc->mutex, NULL);
>>> -                       if (crtc->state->active)
>>> -                               ret = -EBUSY;
>>> -                       drm_modeset_unlock(&crtc->mutex);
>>> -                       if (ret < 0)
>>> -                               break;
>>> -               }
>>> -
>>> -       } else {
>>> -               struct drm_connector *list_connector;
>>> -               struct drm_connector_list_iter iter;
>>> -
>>> -               mutex_lock(&drm_dev->mode_config.mutex);
>>> -               drm_modeset_lock(&drm_dev->mode_config.connection_mutex, NULL);
>>> -
>>> -               drm_connector_list_iter_begin(drm_dev, &iter);
>>> -               drm_for_each_connector_iter(list_connector, &iter) {
>>> -                       if (list_connector->dpms ==  DRM_MODE_DPMS_ON) {
>>> -                               ret = -EBUSY;
>>> -                               break;
>>> -                       }
>>> -               }
>>> -
>>> -               drm_connector_list_iter_end(&iter);
>>> -
>>> -               drm_modeset_unlock(&drm_dev->mode_config.connection_mutex);
>>> -               mutex_unlock(&drm_dev->mode_config.mutex);
>>> -       }
>>> -
>>> -       if (ret == -EBUSY)
>>> -               DRM_DEBUG_DRIVER("failing to power off - crtc active\n");
>>> +       ret = amdgpu_runtime_idle_check_display(dev);
>>> +       if (ret)
>>> +               return ret;
>>>
>>>          pm_runtime_mark_last_busy(dev);
>>>          pm_runtime_autosuspend(dev);
>>> --
>>> 2.35.1
>>>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/amdgpu: don't runtime suspend if there are displays attached (v2)
  2022-04-19 13:47 ` Alex Deucher
@ 2022-04-20 13:57   ` Alex Deucher
  2022-04-20 14:05     ` Christian König
  0 siblings, 1 reply; 13+ messages in thread
From: Alex Deucher @ 2022-04-20 13:57 UTC (permalink / raw)
  To: Alex Deucher; +Cc: Michele Ballabio, amd-gfx list

Ping?  Anyone care to review this?

Alex

On Tue, Apr 19, 2022 at 9:47 AM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> Ping?
>
> On Wed, Apr 13, 2022 at 4:15 PM Alex Deucher <alexander.deucher@amd.com> wrote:
> >
> > We normally runtime suspend when there are displays attached if they
> > are in the DPMS off state, however, if something wakes the GPU
> > we send a hotplug event on resume (in case any displays were connected
> > while the GPU was in suspend) which can cause userspace to light
> > up the displays again soon after they were turned off.
> >
> > Prior to
> > commit 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's."),
> > the driver took a runtime pm reference when the fbdev emulation was
> > enabled because we didn't implement proper shadowing support for
> > vram access when the device was off so the device never runtime
> > suspended when there was a console bound.  Once that commit landed,
> > we now utilize the core fb helper implementation which properly
> > handles the emulation, so runtime pm now suspends in cases where it did
> > not before.  Ultimately, we need to sort out why runtime suspend in not
> > working in this case for some users, but this should restore similar
> > behavior to before.
> >
> > v2: move check into runtime_suspend
> >
> > Fixes: 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's.")
> > Tested-by: Michele Ballabio <ballabio.m@gmail.com>
> > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 107 ++++++++++++++++--------
> >  1 file changed, 72 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > index 4efaa183abcd..97a1aa02d76e 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > @@ -2395,6 +2395,71 @@ static int amdgpu_pmops_restore(struct device *dev)
> >         return amdgpu_device_resume(drm_dev, true);
> >  }
> >
> > +static int amdgpu_runtime_idle_check_display(struct device *dev)
> > +{
> > +       struct pci_dev *pdev = to_pci_dev(dev);
> > +       struct drm_device *drm_dev = pci_get_drvdata(pdev);
> > +       struct amdgpu_device *adev = drm_to_adev(drm_dev);
> > +
> > +       if (adev->mode_info.num_crtc) {
> > +               struct drm_connector *list_connector;
> > +               struct drm_connector_list_iter iter;
> > +               int ret = 0;
> > +
> > +               /* XXX: Return busy if any displays are connected to avoid
> > +                * possible display wake ups after runtime resume due to
> > +                * hotplug events in case any displays were connected while
> > +                * the GPU was in suspend.  Remove this once that is fixed.
> > +                */
> > +               mutex_lock(&drm_dev->mode_config.mutex);
> > +               drm_connector_list_iter_begin(drm_dev, &iter);
> > +               drm_for_each_connector_iter(list_connector, &iter) {
> > +                       if (list_connector->status == connector_status_connected) {
> > +                               ret = -EBUSY;
> > +                               break;
> > +                       }
> > +               }
> > +               drm_connector_list_iter_end(&iter);
> > +               mutex_unlock(&drm_dev->mode_config.mutex);
> > +
> > +               if (ret)
> > +                       return ret;
> > +
> > +               if (amdgpu_device_has_dc_support(adev)) {
> > +                       struct drm_crtc *crtc;
> > +
> > +                       drm_for_each_crtc(crtc, drm_dev) {
> > +                               drm_modeset_lock(&crtc->mutex, NULL);
> > +                               if (crtc->state->active)
> > +                                       ret = -EBUSY;
> > +                               drm_modeset_unlock(&crtc->mutex);
> > +                               if (ret < 0)
> > +                                       break;
> > +                       }
> > +               } else {
> > +                       mutex_lock(&drm_dev->mode_config.mutex);
> > +                       drm_modeset_lock(&drm_dev->mode_config.connection_mutex, NULL);
> > +
> > +                       drm_connector_list_iter_begin(drm_dev, &iter);
> > +                       drm_for_each_connector_iter(list_connector, &iter) {
> > +                               if (list_connector->dpms ==  DRM_MODE_DPMS_ON) {
> > +                                       ret = -EBUSY;
> > +                                       break;
> > +                               }
> > +                       }
> > +
> > +                       drm_connector_list_iter_end(&iter);
> > +
> > +                       drm_modeset_unlock(&drm_dev->mode_config.connection_mutex);
> > +                       mutex_unlock(&drm_dev->mode_config.mutex);
> > +               }
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  static int amdgpu_pmops_runtime_suspend(struct device *dev)
> >  {
> >         struct pci_dev *pdev = to_pci_dev(dev);
> > @@ -2407,6 +2472,10 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev)
> >                 return -EBUSY;
> >         }
> >
> > +       ret = amdgpu_runtime_idle_check_display(dev);
> > +       if (ret)
> > +               return ret;
> > +
> >         /* wait for all rings to drain before suspending */
> >         for (i = 0; i < AMDGPU_MAX_RINGS; i++) {
> >                 struct amdgpu_ring *ring = adev->rings[i];
> > @@ -2516,41 +2585,9 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
> >                 return -EBUSY;
> >         }
> >
> > -       if (amdgpu_device_has_dc_support(adev)) {
> > -               struct drm_crtc *crtc;
> > -
> > -               drm_for_each_crtc(crtc, drm_dev) {
> > -                       drm_modeset_lock(&crtc->mutex, NULL);
> > -                       if (crtc->state->active)
> > -                               ret = -EBUSY;
> > -                       drm_modeset_unlock(&crtc->mutex);
> > -                       if (ret < 0)
> > -                               break;
> > -               }
> > -
> > -       } else {
> > -               struct drm_connector *list_connector;
> > -               struct drm_connector_list_iter iter;
> > -
> > -               mutex_lock(&drm_dev->mode_config.mutex);
> > -               drm_modeset_lock(&drm_dev->mode_config.connection_mutex, NULL);
> > -
> > -               drm_connector_list_iter_begin(drm_dev, &iter);
> > -               drm_for_each_connector_iter(list_connector, &iter) {
> > -                       if (list_connector->dpms ==  DRM_MODE_DPMS_ON) {
> > -                               ret = -EBUSY;
> > -                               break;
> > -                       }
> > -               }
> > -
> > -               drm_connector_list_iter_end(&iter);
> > -
> > -               drm_modeset_unlock(&drm_dev->mode_config.connection_mutex);
> > -               mutex_unlock(&drm_dev->mode_config.mutex);
> > -       }
> > -
> > -       if (ret == -EBUSY)
> > -               DRM_DEBUG_DRIVER("failing to power off - crtc active\n");
> > +       ret = amdgpu_runtime_idle_check_display(dev);
> > +       if (ret)
> > +               return ret;
> >
> >         pm_runtime_mark_last_busy(dev);
> >         pm_runtime_autosuspend(dev);
> > --
> > 2.35.1
> >

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/amdgpu: don't runtime suspend if there are displays attached (v2)
  2022-04-19 14:04 ` Paul Menzel
@ 2022-04-19 14:44   ` Alex Deucher
  0 siblings, 0 replies; 13+ messages in thread
From: Alex Deucher @ 2022-04-19 14:44 UTC (permalink / raw)
  To: Paul Menzel; +Cc: Alex Deucher, amd-gfx list, Michele Ballabio

On Tue, Apr 19, 2022 at 10:04 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Alex,
>
>
> Thank you for the patch.
>
> Am 13.04.22 um 22:15 schrieb Alex Deucher:
> > We normally runtime suspend when there are displays attached if they
> > are in the DPMS off state, however, if something wakes the GPU
> > we send a hotplug event on resume (in case any displays were connected
> > while the GPU was in suspend) which can cause userspace to light
> > up the displays again soon after they were turned off.
> >
> > Prior to
> > commit 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's."),
> > the driver took a runtime pm reference when the fbdev emulation was
> > enabled because we didn't implement proper shadowing support for
> > vram access when the device was off so the device never runtime
> > suspended when there was a console bound.  Once that commit landed,
> > we now utilize the core fb helper implementation which properly
> > handles the emulation, so runtime pm now suspends in cases where it did
> > not before.  Ultimately, we need to sort out why runtime suspend in not
> > working in this case for some users, but this should restore similar
> > behavior to before.
> >
> > v2: move check into runtime_suspend
> >
> > Fixes: 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's.")
> > Tested-by: Michele Ballabio <ballabio.m@gmail.com>
>
> On what system and device?

It was a polaris dGPU, but it has been seen on other GPUs as well.
It's not device specific.  The issue is hard to reproduce at least in
our testing unfortunately.

>
> > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 107 ++++++++++++++++--------
> >   1 file changed, 72 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > index 4efaa183abcd..97a1aa02d76e 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > @@ -2395,6 +2395,71 @@ static int amdgpu_pmops_restore(struct device *dev)
> >       return amdgpu_device_resume(drm_dev, true);
> >   }
> >
> > +static int amdgpu_runtime_idle_check_display(struct device *dev)
> > +{
> > +     struct pci_dev *pdev = to_pci_dev(dev);
> > +     struct drm_device *drm_dev = pci_get_drvdata(pdev);
> > +     struct amdgpu_device *adev = drm_to_adev(drm_dev);
> > +
> > +     if (adev->mode_info.num_crtc) {
> > +             struct drm_connector *list_connector;
> > +             struct drm_connector_list_iter iter;
> > +             int ret = 0;
> > +
> > +             /* XXX: Return busy if any displays are connected to avoid
> > +              * possible display wake ups after runtime resume due to
>
> Nit: wakeups

Ack.

>
> > +              * hotplug events in case any displays were connected while
> > +              * the GPU was in suspend.  Remove this once that is fixed.
> > +              */
>
> Do you have an (internal) issue to track this?

Yes, we are tracking it.

Alex

>
> > +             mutex_lock(&drm_dev->mode_config.mutex);
> > +             drm_connector_list_iter_begin(drm_dev, &iter);
> > +             drm_for_each_connector_iter(list_connector, &iter) {
> > +                     if (list_connector->status == connector_status_connected) {
> > +                             ret = -EBUSY;
> > +                             break;
> > +                     }
> > +             }
> > +             drm_connector_list_iter_end(&iter);
> > +             mutex_unlock(&drm_dev->mode_config.mutex);
> > +
> > +             if (ret)
> > +                     return ret;
> > +
> > +             if (amdgpu_device_has_dc_support(adev)) {
> > +                     struct drm_crtc *crtc;
> > +
> > +                     drm_for_each_crtc(crtc, drm_dev) {
> > +                             drm_modeset_lock(&crtc->mutex, NULL);
> > +                             if (crtc->state->active)
> > +                                     ret = -EBUSY;
> > +                             drm_modeset_unlock(&crtc->mutex);
> > +                             if (ret < 0)
> > +                                     break;
> > +                     }
> > +             } else {
> > +                     mutex_lock(&drm_dev->mode_config.mutex);
> > +                     drm_modeset_lock(&drm_dev->mode_config.connection_mutex, NULL);
> > +
> > +                     drm_connector_list_iter_begin(drm_dev, &iter);
> > +                     drm_for_each_connector_iter(list_connector, &iter) {
> > +                             if (list_connector->dpms ==  DRM_MODE_DPMS_ON) {
> > +                                     ret = -EBUSY;
> > +                                     break;
> > +                             }
> > +                     }
> > +
> > +                     drm_connector_list_iter_end(&iter);
> > +
> > +                     drm_modeset_unlock(&drm_dev->mode_config.connection_mutex);
> > +                     mutex_unlock(&drm_dev->mode_config.mutex);
> > +             }
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >   static int amdgpu_pmops_runtime_suspend(struct device *dev)
> >   {
> >       struct pci_dev *pdev = to_pci_dev(dev);
> > @@ -2407,6 +2472,10 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev)
> >               return -EBUSY;
> >       }
> >
> > +     ret = amdgpu_runtime_idle_check_display(dev);
> > +     if (ret)
> > +             return ret;
> > +
> >       /* wait for all rings to drain before suspending */
> >       for (i = 0; i < AMDGPU_MAX_RINGS; i++) {
> >               struct amdgpu_ring *ring = adev->rings[i];
> > @@ -2516,41 +2585,9 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
> >               return -EBUSY;
> >       }
> >
> > -     if (amdgpu_device_has_dc_support(adev)) {
> > -             struct drm_crtc *crtc;
> > -
> > -             drm_for_each_crtc(crtc, drm_dev) {
> > -                     drm_modeset_lock(&crtc->mutex, NULL);
> > -                     if (crtc->state->active)
> > -                             ret = -EBUSY;
> > -                     drm_modeset_unlock(&crtc->mutex);
> > -                     if (ret < 0)
> > -                             break;
> > -             }
> > -
> > -     } else {
> > -             struct drm_connector *list_connector;
> > -             struct drm_connector_list_iter iter;
> > -
> > -             mutex_lock(&drm_dev->mode_config.mutex);
> > -             drm_modeset_lock(&drm_dev->mode_config.connection_mutex, NULL);
> > -
> > -             drm_connector_list_iter_begin(drm_dev, &iter);
> > -             drm_for_each_connector_iter(list_connector, &iter) {
> > -                     if (list_connector->dpms ==  DRM_MODE_DPMS_ON) {
> > -                             ret = -EBUSY;
> > -                             break;
> > -                     }
> > -             }
> > -
> > -             drm_connector_list_iter_end(&iter);
> > -
> > -             drm_modeset_unlock(&drm_dev->mode_config.connection_mutex);
> > -             mutex_unlock(&drm_dev->mode_config.mutex);
> > -     }
> > -
> > -     if (ret == -EBUSY)
> > -             DRM_DEBUG_DRIVER("failing to power off - crtc active\n");
> > +     ret = amdgpu_runtime_idle_check_display(dev);
> > +     if (ret)
> > +             return ret;
> >
> >       pm_runtime_mark_last_busy(dev);
> >       pm_runtime_autosuspend(dev);
>
> The overall change looks good.
>
>
> Kind regards,
>
> Paul

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/amdgpu: don't runtime suspend if there are displays attached (v2)
  2022-04-13 20:15 Alex Deucher
  2022-04-19 13:47 ` Alex Deucher
@ 2022-04-19 14:04 ` Paul Menzel
  2022-04-19 14:44   ` Alex Deucher
  2022-04-21  2:53 ` Quan, Evan
  2 siblings, 1 reply; 13+ messages in thread
From: Paul Menzel @ 2022-04-19 14:04 UTC (permalink / raw)
  To: Alex Deucher; +Cc: Michele Ballabio, amd-gfx

Dear Alex,


Thank you for the patch.

Am 13.04.22 um 22:15 schrieb Alex Deucher:
> We normally runtime suspend when there are displays attached if they
> are in the DPMS off state, however, if something wakes the GPU
> we send a hotplug event on resume (in case any displays were connected
> while the GPU was in suspend) which can cause userspace to light
> up the displays again soon after they were turned off.
> 
> Prior to
> commit 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's."),
> the driver took a runtime pm reference when the fbdev emulation was
> enabled because we didn't implement proper shadowing support for
> vram access when the device was off so the device never runtime
> suspended when there was a console bound.  Once that commit landed,
> we now utilize the core fb helper implementation which properly
> handles the emulation, so runtime pm now suspends in cases where it did
> not before.  Ultimately, we need to sort out why runtime suspend in not
> working in this case for some users, but this should restore similar
> behavior to before.
> 
> v2: move check into runtime_suspend
> 
> Fixes: 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's.")
> Tested-by: Michele Ballabio <ballabio.m@gmail.com>

On what system and device?

> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 107 ++++++++++++++++--------
>   1 file changed, 72 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 4efaa183abcd..97a1aa02d76e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2395,6 +2395,71 @@ static int amdgpu_pmops_restore(struct device *dev)
>   	return amdgpu_device_resume(drm_dev, true);
>   }
>   
> +static int amdgpu_runtime_idle_check_display(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +	struct amdgpu_device *adev = drm_to_adev(drm_dev);
> +
> +	if (adev->mode_info.num_crtc) {
> +		struct drm_connector *list_connector;
> +		struct drm_connector_list_iter iter;
> +		int ret = 0;
> +
> +		/* XXX: Return busy if any displays are connected to avoid
> +		 * possible display wake ups after runtime resume due to

Nit: wakeups

> +		 * hotplug events in case any displays were connected while
> +		 * the GPU was in suspend.  Remove this once that is fixed.
> +		 */

Do you have an (internal) issue to track this?

> +		mutex_lock(&drm_dev->mode_config.mutex);
> +		drm_connector_list_iter_begin(drm_dev, &iter);
> +		drm_for_each_connector_iter(list_connector, &iter) {
> +			if (list_connector->status == connector_status_connected) {
> +				ret = -EBUSY;
> +				break;
> +			}
> +		}
> +		drm_connector_list_iter_end(&iter);
> +		mutex_unlock(&drm_dev->mode_config.mutex);
> +
> +		if (ret)
> +			return ret;
> +
> +		if (amdgpu_device_has_dc_support(adev)) {
> +			struct drm_crtc *crtc;
> +
> +			drm_for_each_crtc(crtc, drm_dev) {
> +				drm_modeset_lock(&crtc->mutex, NULL);
> +				if (crtc->state->active)
> +					ret = -EBUSY;
> +				drm_modeset_unlock(&crtc->mutex);
> +				if (ret < 0)
> +					break;
> +			}
> +		} else {
> +			mutex_lock(&drm_dev->mode_config.mutex);
> +			drm_modeset_lock(&drm_dev->mode_config.connection_mutex, NULL);
> +
> +			drm_connector_list_iter_begin(drm_dev, &iter);
> +			drm_for_each_connector_iter(list_connector, &iter) {
> +				if (list_connector->dpms ==  DRM_MODE_DPMS_ON) {
> +					ret = -EBUSY;
> +					break;
> +				}
> +			}
> +
> +			drm_connector_list_iter_end(&iter);
> +
> +			drm_modeset_unlock(&drm_dev->mode_config.connection_mutex);
> +			mutex_unlock(&drm_dev->mode_config.mutex);
> +		}
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>   static int amdgpu_pmops_runtime_suspend(struct device *dev)
>   {
>   	struct pci_dev *pdev = to_pci_dev(dev);
> @@ -2407,6 +2472,10 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev)
>   		return -EBUSY;
>   	}
>   
> +	ret = amdgpu_runtime_idle_check_display(dev);
> +	if (ret)
> +		return ret;
> +
>   	/* wait for all rings to drain before suspending */
>   	for (i = 0; i < AMDGPU_MAX_RINGS; i++) {
>   		struct amdgpu_ring *ring = adev->rings[i];
> @@ -2516,41 +2585,9 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
>   		return -EBUSY;
>   	}
>   
> -	if (amdgpu_device_has_dc_support(adev)) {
> -		struct drm_crtc *crtc;
> -
> -		drm_for_each_crtc(crtc, drm_dev) {
> -			drm_modeset_lock(&crtc->mutex, NULL);
> -			if (crtc->state->active)
> -				ret = -EBUSY;
> -			drm_modeset_unlock(&crtc->mutex);
> -			if (ret < 0)
> -				break;
> -		}
> -
> -	} else {
> -		struct drm_connector *list_connector;
> -		struct drm_connector_list_iter iter;
> -
> -		mutex_lock(&drm_dev->mode_config.mutex);
> -		drm_modeset_lock(&drm_dev->mode_config.connection_mutex, NULL);
> -
> -		drm_connector_list_iter_begin(drm_dev, &iter);
> -		drm_for_each_connector_iter(list_connector, &iter) {
> -			if (list_connector->dpms ==  DRM_MODE_DPMS_ON) {
> -				ret = -EBUSY;
> -				break;
> -			}
> -		}
> -
> -		drm_connector_list_iter_end(&iter);
> -
> -		drm_modeset_unlock(&drm_dev->mode_config.connection_mutex);
> -		mutex_unlock(&drm_dev->mode_config.mutex);
> -	}
> -
> -	if (ret == -EBUSY)
> -		DRM_DEBUG_DRIVER("failing to power off - crtc active\n");
> +	ret = amdgpu_runtime_idle_check_display(dev);
> +	if (ret)
> +		return ret;
>   
>   	pm_runtime_mark_last_busy(dev);
>   	pm_runtime_autosuspend(dev);

The overall change looks good.


Kind regards,

Paul

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/amdgpu: don't runtime suspend if there are displays attached (v2)
  2022-04-13 20:15 Alex Deucher
@ 2022-04-19 13:47 ` Alex Deucher
  2022-04-20 13:57   ` Alex Deucher
  2022-04-19 14:04 ` Paul Menzel
  2022-04-21  2:53 ` Quan, Evan
  2 siblings, 1 reply; 13+ messages in thread
From: Alex Deucher @ 2022-04-19 13:47 UTC (permalink / raw)
  To: Alex Deucher; +Cc: Michele Ballabio, amd-gfx list

Ping?

On Wed, Apr 13, 2022 at 4:15 PM Alex Deucher <alexander.deucher@amd.com> wrote:
>
> We normally runtime suspend when there are displays attached if they
> are in the DPMS off state, however, if something wakes the GPU
> we send a hotplug event on resume (in case any displays were connected
> while the GPU was in suspend) which can cause userspace to light
> up the displays again soon after they were turned off.
>
> Prior to
> commit 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's."),
> the driver took a runtime pm reference when the fbdev emulation was
> enabled because we didn't implement proper shadowing support for
> vram access when the device was off so the device never runtime
> suspended when there was a console bound.  Once that commit landed,
> we now utilize the core fb helper implementation which properly
> handles the emulation, so runtime pm now suspends in cases where it did
> not before.  Ultimately, we need to sort out why runtime suspend in not
> working in this case for some users, but this should restore similar
> behavior to before.
>
> v2: move check into runtime_suspend
>
> Fixes: 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's.")
> Tested-by: Michele Ballabio <ballabio.m@gmail.com>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 107 ++++++++++++++++--------
>  1 file changed, 72 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 4efaa183abcd..97a1aa02d76e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2395,6 +2395,71 @@ static int amdgpu_pmops_restore(struct device *dev)
>         return amdgpu_device_resume(drm_dev, true);
>  }
>
> +static int amdgpu_runtime_idle_check_display(struct device *dev)
> +{
> +       struct pci_dev *pdev = to_pci_dev(dev);
> +       struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +       struct amdgpu_device *adev = drm_to_adev(drm_dev);
> +
> +       if (adev->mode_info.num_crtc) {
> +               struct drm_connector *list_connector;
> +               struct drm_connector_list_iter iter;
> +               int ret = 0;
> +
> +               /* XXX: Return busy if any displays are connected to avoid
> +                * possible display wake ups after runtime resume due to
> +                * hotplug events in case any displays were connected while
> +                * the GPU was in suspend.  Remove this once that is fixed.
> +                */
> +               mutex_lock(&drm_dev->mode_config.mutex);
> +               drm_connector_list_iter_begin(drm_dev, &iter);
> +               drm_for_each_connector_iter(list_connector, &iter) {
> +                       if (list_connector->status == connector_status_connected) {
> +                               ret = -EBUSY;
> +                               break;
> +                       }
> +               }
> +               drm_connector_list_iter_end(&iter);
> +               mutex_unlock(&drm_dev->mode_config.mutex);
> +
> +               if (ret)
> +                       return ret;
> +
> +               if (amdgpu_device_has_dc_support(adev)) {
> +                       struct drm_crtc *crtc;
> +
> +                       drm_for_each_crtc(crtc, drm_dev) {
> +                               drm_modeset_lock(&crtc->mutex, NULL);
> +                               if (crtc->state->active)
> +                                       ret = -EBUSY;
> +                               drm_modeset_unlock(&crtc->mutex);
> +                               if (ret < 0)
> +                                       break;
> +                       }
> +               } else {
> +                       mutex_lock(&drm_dev->mode_config.mutex);
> +                       drm_modeset_lock(&drm_dev->mode_config.connection_mutex, NULL);
> +
> +                       drm_connector_list_iter_begin(drm_dev, &iter);
> +                       drm_for_each_connector_iter(list_connector, &iter) {
> +                               if (list_connector->dpms ==  DRM_MODE_DPMS_ON) {
> +                                       ret = -EBUSY;
> +                                       break;
> +                               }
> +                       }
> +
> +                       drm_connector_list_iter_end(&iter);
> +
> +                       drm_modeset_unlock(&drm_dev->mode_config.connection_mutex);
> +                       mutex_unlock(&drm_dev->mode_config.mutex);
> +               }
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       return 0;
> +}
> +
>  static int amdgpu_pmops_runtime_suspend(struct device *dev)
>  {
>         struct pci_dev *pdev = to_pci_dev(dev);
> @@ -2407,6 +2472,10 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev)
>                 return -EBUSY;
>         }
>
> +       ret = amdgpu_runtime_idle_check_display(dev);
> +       if (ret)
> +               return ret;
> +
>         /* wait for all rings to drain before suspending */
>         for (i = 0; i < AMDGPU_MAX_RINGS; i++) {
>                 struct amdgpu_ring *ring = adev->rings[i];
> @@ -2516,41 +2585,9 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
>                 return -EBUSY;
>         }
>
> -       if (amdgpu_device_has_dc_support(adev)) {
> -               struct drm_crtc *crtc;
> -
> -               drm_for_each_crtc(crtc, drm_dev) {
> -                       drm_modeset_lock(&crtc->mutex, NULL);
> -                       if (crtc->state->active)
> -                               ret = -EBUSY;
> -                       drm_modeset_unlock(&crtc->mutex);
> -                       if (ret < 0)
> -                               break;
> -               }
> -
> -       } else {
> -               struct drm_connector *list_connector;
> -               struct drm_connector_list_iter iter;
> -
> -               mutex_lock(&drm_dev->mode_config.mutex);
> -               drm_modeset_lock(&drm_dev->mode_config.connection_mutex, NULL);
> -
> -               drm_connector_list_iter_begin(drm_dev, &iter);
> -               drm_for_each_connector_iter(list_connector, &iter) {
> -                       if (list_connector->dpms ==  DRM_MODE_DPMS_ON) {
> -                               ret = -EBUSY;
> -                               break;
> -                       }
> -               }
> -
> -               drm_connector_list_iter_end(&iter);
> -
> -               drm_modeset_unlock(&drm_dev->mode_config.connection_mutex);
> -               mutex_unlock(&drm_dev->mode_config.mutex);
> -       }
> -
> -       if (ret == -EBUSY)
> -               DRM_DEBUG_DRIVER("failing to power off - crtc active\n");
> +       ret = amdgpu_runtime_idle_check_display(dev);
> +       if (ret)
> +               return ret;
>
>         pm_runtime_mark_last_busy(dev);
>         pm_runtime_autosuspend(dev);
> --
> 2.35.1
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] drm/amdgpu: don't runtime suspend if there are displays attached (v2)
@ 2022-04-13 20:15 Alex Deucher
  2022-04-19 13:47 ` Alex Deucher
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Alex Deucher @ 2022-04-13 20:15 UTC (permalink / raw)
  To: amd-gfx; +Cc: Alex Deucher, Michele Ballabio

We normally runtime suspend when there are displays attached if they
are in the DPMS off state, however, if something wakes the GPU
we send a hotplug event on resume (in case any displays were connected
while the GPU was in suspend) which can cause userspace to light
up the displays again soon after they were turned off.

Prior to
commit 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's."),
the driver took a runtime pm reference when the fbdev emulation was
enabled because we didn't implement proper shadowing support for
vram access when the device was off so the device never runtime
suspended when there was a console bound.  Once that commit landed,
we now utilize the core fb helper implementation which properly
handles the emulation, so runtime pm now suspends in cases where it did
not before.  Ultimately, we need to sort out why runtime suspend in not
working in this case for some users, but this should restore similar
behavior to before.

v2: move check into runtime_suspend

Fixes: 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's.")
Tested-by: Michele Ballabio <ballabio.m@gmail.com>
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 107 ++++++++++++++++--------
 1 file changed, 72 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 4efaa183abcd..97a1aa02d76e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2395,6 +2395,71 @@ static int amdgpu_pmops_restore(struct device *dev)
 	return amdgpu_device_resume(drm_dev, true);
 }
 
+static int amdgpu_runtime_idle_check_display(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct drm_device *drm_dev = pci_get_drvdata(pdev);
+	struct amdgpu_device *adev = drm_to_adev(drm_dev);
+
+	if (adev->mode_info.num_crtc) {
+		struct drm_connector *list_connector;
+		struct drm_connector_list_iter iter;
+		int ret = 0;
+
+		/* XXX: Return busy if any displays are connected to avoid
+		 * possible display wake ups after runtime resume due to
+		 * hotplug events in case any displays were connected while
+		 * the GPU was in suspend.  Remove this once that is fixed.
+		 */
+		mutex_lock(&drm_dev->mode_config.mutex);
+		drm_connector_list_iter_begin(drm_dev, &iter);
+		drm_for_each_connector_iter(list_connector, &iter) {
+			if (list_connector->status == connector_status_connected) {
+				ret = -EBUSY;
+				break;
+			}
+		}
+		drm_connector_list_iter_end(&iter);
+		mutex_unlock(&drm_dev->mode_config.mutex);
+
+		if (ret)
+			return ret;
+
+		if (amdgpu_device_has_dc_support(adev)) {
+			struct drm_crtc *crtc;
+
+			drm_for_each_crtc(crtc, drm_dev) {
+				drm_modeset_lock(&crtc->mutex, NULL);
+				if (crtc->state->active)
+					ret = -EBUSY;
+				drm_modeset_unlock(&crtc->mutex);
+				if (ret < 0)
+					break;
+			}
+		} else {
+			mutex_lock(&drm_dev->mode_config.mutex);
+			drm_modeset_lock(&drm_dev->mode_config.connection_mutex, NULL);
+
+			drm_connector_list_iter_begin(drm_dev, &iter);
+			drm_for_each_connector_iter(list_connector, &iter) {
+				if (list_connector->dpms ==  DRM_MODE_DPMS_ON) {
+					ret = -EBUSY;
+					break;
+				}
+			}
+
+			drm_connector_list_iter_end(&iter);
+
+			drm_modeset_unlock(&drm_dev->mode_config.connection_mutex);
+			mutex_unlock(&drm_dev->mode_config.mutex);
+		}
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int amdgpu_pmops_runtime_suspend(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
@@ -2407,6 +2472,10 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev)
 		return -EBUSY;
 	}
 
+	ret = amdgpu_runtime_idle_check_display(dev);
+	if (ret)
+		return ret;
+
 	/* wait for all rings to drain before suspending */
 	for (i = 0; i < AMDGPU_MAX_RINGS; i++) {
 		struct amdgpu_ring *ring = adev->rings[i];
@@ -2516,41 +2585,9 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
 		return -EBUSY;
 	}
 
-	if (amdgpu_device_has_dc_support(adev)) {
-		struct drm_crtc *crtc;
-
-		drm_for_each_crtc(crtc, drm_dev) {
-			drm_modeset_lock(&crtc->mutex, NULL);
-			if (crtc->state->active)
-				ret = -EBUSY;
-			drm_modeset_unlock(&crtc->mutex);
-			if (ret < 0)
-				break;
-		}
-
-	} else {
-		struct drm_connector *list_connector;
-		struct drm_connector_list_iter iter;
-
-		mutex_lock(&drm_dev->mode_config.mutex);
-		drm_modeset_lock(&drm_dev->mode_config.connection_mutex, NULL);
-
-		drm_connector_list_iter_begin(drm_dev, &iter);
-		drm_for_each_connector_iter(list_connector, &iter) {
-			if (list_connector->dpms ==  DRM_MODE_DPMS_ON) {
-				ret = -EBUSY;
-				break;
-			}
-		}
-
-		drm_connector_list_iter_end(&iter);
-
-		drm_modeset_unlock(&drm_dev->mode_config.connection_mutex);
-		mutex_unlock(&drm_dev->mode_config.mutex);
-	}
-
-	if (ret == -EBUSY)
-		DRM_DEBUG_DRIVER("failing to power off - crtc active\n");
+	ret = amdgpu_runtime_idle_check_display(dev);
+	if (ret)
+		return ret;
 
 	pm_runtime_mark_last_busy(dev);
 	pm_runtime_autosuspend(dev);
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2022-04-21 14:19 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-21  3:16 [PATCH] drm/amdgpu: don't runtime suspend if there are displays attached (v2) Alex Deucher
2022-04-21  4:40 ` Alex Deucher
2022-04-21  5:23 ` Quan, Evan
2022-04-21 11:11 ` Thorsten Leemhuis
2022-04-21 14:19   ` Alex Deucher
  -- strict thread matches above, loose matches on Subject: below --
2022-04-13 20:15 Alex Deucher
2022-04-19 13:47 ` Alex Deucher
2022-04-20 13:57   ` Alex Deucher
2022-04-20 14:05     ` Christian König
2022-04-19 14:04 ` Paul Menzel
2022-04-19 14:44   ` Alex Deucher
2022-04-21  2:53 ` Quan, Evan
2022-04-21  3:00   ` Alex Deucher

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.