[PATCH 0/2]

[PATCH 0/2]

[Jim Qu]

Jim Qu (2):
  drm/amd/amdgpu: fix console deadlock if late init failed
  drm/amd/amdgpu: add atomic helper to suspend/resume functions

 drivers/gpu/drm/amd/amdgpu/amdgpu.h        |   1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 182 +++++++++++++++++------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c     |   7 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h   |   2 +-
 4 files changed, 115 insertions(+), 77 deletions(-)

-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/2] drm/amd/amdgpu: fix console deadlock if late init failed

[Jim Qu]

Change-Id: Ia914fac083ebab39617dad046d4bb98ab1ddf0a8
Signed-off-by: Jim Qu <Jim.Qu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index c0679d3..6eeda24 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -2186,8 +2186,11 @@ int amdgpu_device_resume(struct drm_device *dev, bool resume, bool fbcon)
 	}
 
 	r = amdgpu_late_init(adev);
-	if (r)
+	if (r) {
+		if (fbcon)
+			console_unlock();
 		return r;
+	}
 
 	/* pin cursors */
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] drm/amd/amdgpu: add atomic helper to suspend/resume functions

[Jim Qu]

Change-Id: I7f5b4bddf1fe0538de81f2268fe80927bee09ec5
Signed-off-by: Jim Qu <Jim.Qu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        |   1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 179 +++++++++++++++++------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c     |   7 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h   |   2 +-
 4 files changed, 112 insertions(+), 77 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 8a5f8cb..07b99ab 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1504,6 +1504,7 @@ struct amdgpu_device {
 	/* record hw reset is performed */
 	bool has_hw_reset;
 
+	struct drm_atomic_state *atomic_state;
 };
 
 static inline struct amdgpu_device *amdgpu_ttm_adev(struct ttm_bo_device *bdev)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 6eeda24..0dfabc6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -2027,39 +2027,18 @@ void amdgpu_device_fini(struct amdgpu_device *adev)
 	amdgpu_debugfs_remove_files(adev);
 }
 
-
-/*
- * Suspend & resume.
- */
-/**
- * amdgpu_device_suspend - initiate device suspend
- *
- * @pdev: drm dev pointer
- * @state: suspend state
- *
- * Puts the hw in the suspend state (all asics).
- * Returns 0 for success or an error on failure.
- * Called at driver suspend.
- */
-int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
+int amdgpu_device_suspend_display(struct amdgpu_device *adev)
 {
-	struct amdgpu_device *adev;
 	struct drm_crtc *crtc;
 	struct drm_connector *connector;
+	struct drm_device *dev = adev->ddev;
 	int r;
 
-	if (dev == NULL || dev->dev_private == NULL) {
-		return -ENODEV;
-	}
-
-	adev = dev->dev_private;
-
-	if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
-		return 0;
-
-	drm_kms_helper_poll_disable(dev);
-
-	if (!amdgpu_device_has_dc_support(adev)) {
+	if (amdgpu_device_has_dc_support(adev)) {
+		adev->atomic_state = drm_atomic_helper_suspend(dev);
+		if (IS_ERR(adev->atomic_state))
+			return -PTR_ERR(adev->atomic_state);
+	} else {
 		/* turn off display hw */
 		drm_modeset_lock_all(dev);
 		list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
@@ -2096,6 +2075,97 @@ int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
 			}
 		}
 	}
+
+	return 0;
+}
+
+
+int amdgpu_device_resume_display(struct amdgpu_device *adev)
+{
+	struct drm_crtc *crtc;
+	struct drm_connector *connector;
+	struct drm_device *dev = adev->ddev;
+	int r;
+
+	/* pin cursors */
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
+
+		if (amdgpu_crtc->cursor_bo) {
+			struct amdgpu_bo *aobj = gem_to_amdgpu_bo(amdgpu_crtc->cursor_bo);
+			r = amdgpu_bo_reserve(aobj, false);
+			if (r == 0) {
+				r = amdgpu_bo_pin(aobj,
+						  AMDGPU_GEM_DOMAIN_VRAM,
+						  &amdgpu_crtc->cursor_addr);
+				if (r != 0)
+					DRM_ERROR("Failed to pin cursor BO (%d)\n", r);
+				amdgpu_bo_unreserve(aobj);
+			}
+		}
+	}
+
+	/* blat the mode back in */
+	if (amdgpu_device_has_dc_support(adev)) {
+		r = drm_atomic_helper_resume(dev, adev->atomic_state);
+		if (r)
+			return r;
+		r = amdgpu_dm_display_resume(adev);
+		if(r)
+			return r;
+	} else {
+		/* pre DCE11 */
+		drm_helper_resume_force_mode(dev);
+
+		/* turn on display hw */
+		drm_modeset_lock_all(dev);
+		list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
+			drm_helper_connector_dpms(connector, DRM_MODE_DPMS_ON);
+		}
+		drm_modeset_unlock_all(dev);
+	}
+
+	return 0;
+}
+
+/*
+ * Suspend & resume.
+ */
+/**
+ * amdgpu_device_suspend - initiate device suspend
+ *
+ * @pdev: drm dev pointer
+ * @state: suspend state
+ *
+ * Puts the hw in the suspend state (all asics).
+ * Returns 0 for success or an error on failure.
+ * Called at driver suspend.
+ */
+int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
+{
+	struct amdgpu_device *adev;
+	int r;
+
+	if (dev == NULL || dev->dev_private == NULL) {
+		return -ENODEV;
+	}
+
+	adev = dev->dev_private;
+
+	if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
+		return 0;
+
+	drm_kms_helper_poll_disable(dev);
+
+	amdgpu_fbdev_set_suspend(adev, 1, fbcon);
+
+	r = amdgpu_device_suspend_display(adev);
+	if(r) {
+		amdgpu_fbdev_set_suspend(adev, 0, fbcon);
+		drm_kms_helper_poll_enable(dev);
+		return r;
+	}
+
 	/* evict vram memory */
 	amdgpu_bo_evict_vram(adev);
 
@@ -2121,11 +2191,6 @@ int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
 			DRM_ERROR("amdgpu asic reset failed\n");
 	}
 
-	if (fbcon) {
-		console_lock();
-		amdgpu_fbdev_set_suspend(adev, 1);
-		console_unlock();
-	}
 	return 0;
 }
 
@@ -2140,9 +2205,7 @@ int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
  */
 int amdgpu_device_resume(struct drm_device *dev, bool resume, bool fbcon)
 {
-	struct drm_connector *connector;
 	struct amdgpu_device *adev = dev->dev_private;
-	struct drm_crtc *crtc;
 	int r;
 
 	if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
@@ -2192,45 +2255,11 @@ int amdgpu_device_resume(struct drm_device *dev, bool resume, bool fbcon)
 		return r;
 	}
 
-	/* pin cursors */
-	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
-		struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
-
-		if (amdgpu_crtc->cursor_bo) {
-			struct amdgpu_bo *aobj = gem_to_amdgpu_bo(amdgpu_crtc->cursor_bo);
-			r = amdgpu_bo_reserve(aobj, false);
-			if (r == 0) {
-				r = amdgpu_bo_pin(aobj,
-						  AMDGPU_GEM_DOMAIN_VRAM,
-						  &amdgpu_crtc->cursor_addr);
-				if (r != 0)
-					DRM_ERROR("Failed to pin cursor BO (%d)\n", r);
-				amdgpu_bo_unreserve(aobj);
-			}
-		}
-	}
-
-	/* blat the mode back in */
-	if (fbcon) {
-		if (!amdgpu_device_has_dc_support(adev)) {
-			/* pre DCE11 */
-			drm_helper_resume_force_mode(dev);
-
-			/* turn on display hw */
-			drm_modeset_lock_all(dev);
-			list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-				drm_helper_connector_dpms(connector, DRM_MODE_DPMS_ON);
-			}
-			drm_modeset_unlock_all(dev);
-		} else {
-			/*
-			 * There is no equivalent atomic helper to turn on
-			 * display, so we defined our own function for this,
-			 * once suspend resume is supported by the atomic
-			 * framework this will be reworked
-			 */
-			amdgpu_dm_display_resume(adev);
-		}
+	r = amdgpu_device_resume_display(adev);
+	if (r) {
+		if (fbcon)
+			console_unlock();
+		return r;
 	}
 
 	drm_kms_helper_poll_enable(dev);
@@ -2256,7 +2285,7 @@ int amdgpu_device_resume(struct drm_device *dev, bool resume, bool fbcon)
 #endif
 
 	if (fbcon) {
-		amdgpu_fbdev_set_suspend(adev, 0);
+		amdgpu_fbdev_set_suspend(adev, 0, false);
 		console_unlock();
 	}
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
index 2867f55..d638e4f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
@@ -37,6 +37,7 @@
 #include <drm/drm_fb_helper.h>
 
 #include <linux/vga_switcheroo.h>
+#include <linux/console.h>
 
 /* object hierarchy -
    this contains a helper + a amdgpu fb
@@ -400,11 +401,15 @@ void amdgpu_fbdev_fini(struct amdgpu_device *adev)
 	adev->mode_info.rfbdev = NULL;
 }
 
-void amdgpu_fbdev_set_suspend(struct amdgpu_device *adev, int state)
+void amdgpu_fbdev_set_suspend(struct amdgpu_device *adev, int state, bool fbcon)
 {
+	if (fbcon)
+		console_lock();
 	if (adev->mode_info.rfbdev)
 		drm_fb_helper_set_suspend(&adev->mode_info.rfbdev->helper,
 			state);
+	if (fbcon)
+		console_unlock();
 }
 
 int amdgpu_fbdev_total_size(struct amdgpu_device *adev)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
index 0740673..8f36f63 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
@@ -659,7 +659,7 @@ void amdgpu_panel_mode_fixup(struct drm_encoder *encoder,
 /* fbdev layer */
 int amdgpu_fbdev_init(struct amdgpu_device *adev);
 void amdgpu_fbdev_fini(struct amdgpu_device *adev);
-void amdgpu_fbdev_set_suspend(struct amdgpu_device *adev, int state);
+void amdgpu_fbdev_set_suspend(struct amdgpu_device *adev, int state, bool fbcon);
 int amdgpu_fbdev_total_size(struct amdgpu_device *adev);
 bool amdgpu_fbdev_robj_is_fb(struct amdgpu_device *adev, struct amdgpu_bo *robj);
 void amdgpu_fbdev_restore_mode(struct amdgpu_device *adev);
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 2/2] drm/amd/amdgpu: add atomic helper to suspend/resume functions

[Grodzovsky, Andrey]

Hi, We in DAL as part of upstream effort also implemented switch to using S3 atomic helpers - http://git.amd.com:8080/#/c/67108/ (currently AMD folks view only)  please take a look 
It seems to me a more appropriate would be to call atomic APIs from DAL, maybe merging this change with the DAL change would be a good idea.

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Jim Qu
> Sent: Wednesday, March 01, 2017 4:36 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Qu, Jim
> Subject: [PATCH 2/2] drm/amd/amdgpu: add atomic helper to
> suspend/resume functions
> 
> Change-Id: I7f5b4bddf1fe0538de81f2268fe80927bee09ec5
> Signed-off-by: Jim Qu <Jim.Qu@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h        |   1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 179
> +++++++++++++++++------------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c     |   7 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h   |   2 +-
>  4 files changed, 112 insertions(+), 77 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 8a5f8cb..07b99ab 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1504,6 +1504,7 @@ struct amdgpu_device {
>  	/* record hw reset is performed */
>  	bool has_hw_reset;
> 
> +	struct drm_atomic_state *atomic_state;
>  };
> 
>  static inline struct amdgpu_device *amdgpu_ttm_adev(struct
> ttm_bo_device *bdev) diff --git
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 6eeda24..0dfabc6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2027,39 +2027,18 @@ void amdgpu_device_fini(struct amdgpu_device
> *adev)
>  	amdgpu_debugfs_remove_files(adev);
>  }
> 
> -
> -/*
> - * Suspend & resume.
> - */
> -/**
> - * amdgpu_device_suspend - initiate device suspend
> - *
> - * @pdev: drm dev pointer
> - * @state: suspend state
> - *
> - * Puts the hw in the suspend state (all asics).
> - * Returns 0 for success or an error on failure.
> - * Called at driver suspend.
> - */
> -int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool
> fbcon)
> +int amdgpu_device_suspend_display(struct amdgpu_device *adev)
>  {
> -	struct amdgpu_device *adev;
>  	struct drm_crtc *crtc;
>  	struct drm_connector *connector;
> +	struct drm_device *dev = adev->ddev;
>  	int r;
> 
> -	if (dev == NULL || dev->dev_private == NULL) {
> -		return -ENODEV;
> -	}
> -
> -	adev = dev->dev_private;
> -
> -	if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> -		return 0;
> -
> -	drm_kms_helper_poll_disable(dev);
> -
> -	if (!amdgpu_device_has_dc_support(adev)) {
> +	if (amdgpu_device_has_dc_support(adev)) {
> +		adev->atomic_state = drm_atomic_helper_suspend(dev);
> +		if (IS_ERR(adev->atomic_state))
> +			return -PTR_ERR(adev->atomic_state);
> +	} else {
>  		/* turn off display hw */
>  		drm_modeset_lock_all(dev);
>  		list_for_each_entry(connector, &dev-
> >mode_config.connector_list, head) { @@ -2096,6 +2075,97 @@ int
> amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool
> fbcon)
>  			}
>  		}
>  	}
> +
> +	return 0;
> +}
> +
> +
> +int amdgpu_device_resume_display(struct amdgpu_device *adev) {
> +	struct drm_crtc *crtc;
> +	struct drm_connector *connector;
> +	struct drm_device *dev = adev->ddev;
> +	int r;
> +
> +	/* pin cursors */
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +		struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
> +
> +		if (amdgpu_crtc->cursor_bo) {
> +			struct amdgpu_bo *aobj =
> gem_to_amdgpu_bo(amdgpu_crtc->cursor_bo);
> +			r = amdgpu_bo_reserve(aobj, false);
> +			if (r == 0) {
> +				r = amdgpu_bo_pin(aobj,
> +
> AMDGPU_GEM_DOMAIN_VRAM,
> +						  &amdgpu_crtc-
> >cursor_addr);
> +				if (r != 0)
> +					DRM_ERROR("Failed to pin cursor BO
> (%d)\n", r);
> +				amdgpu_bo_unreserve(aobj);
> +			}
> +		}
> +	}
> +
> +	/* blat the mode back in */
> +	if (amdgpu_device_has_dc_support(adev)) {
> +		r = drm_atomic_helper_resume(dev, adev->atomic_state);
> +		if (r)
> +			return r;
> +		r = amdgpu_dm_display_resume(adev);

 [Grodzovsky, Andrey] Seems to me atomic_comit will be called twice, once from drm_atomic_helper_resume and the other from  amdgpu_dm_display_resume.

> +		if(r)
> +			return r;
> +	} else {
> +		/* pre DCE11 */
> +		drm_helper_resume_force_mode(dev);
> +
> +		/* turn on display hw */
> +		drm_modeset_lock_all(dev);
> +		list_for_each_entry(connector, &dev-
> >mode_config.connector_list, head) {
> +			drm_helper_connector_dpms(connector,
> DRM_MODE_DPMS_ON);
> +		}
> +		drm_modeset_unlock_all(dev);
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Suspend & resume.
> + */
> +/**
> + * amdgpu_device_suspend - initiate device suspend
> + *
> + * @pdev: drm dev pointer
> + * @state: suspend state
> + *
> + * Puts the hw in the suspend state (all asics).
> + * Returns 0 for success or an error on failure.
> + * Called at driver suspend.
> + */
> +int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool
> +fbcon) {
> +	struct amdgpu_device *adev;
> +	int r;
> +
> +	if (dev == NULL || dev->dev_private == NULL) {
> +		return -ENODEV;
> +	}
> +
> +	adev = dev->dev_private;
> +
> +	if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> +		return 0;
> +
> +	drm_kms_helper_poll_disable(dev);
> +
> +	amdgpu_fbdev_set_suspend(adev, 1, fbcon);
> +
> +	r = amdgpu_device_suspend_display(adev);
> +	if(r) {
> +		amdgpu_fbdev_set_suspend(adev, 0, fbcon);
> +		drm_kms_helper_poll_enable(dev);
> +		return r;
> +	}
> +
>  	/* evict vram memory */
>  	amdgpu_bo_evict_vram(adev);
> 
> @@ -2121,11 +2191,6 @@ int amdgpu_device_suspend(struct drm_device
> *dev, bool suspend, bool fbcon)
>  			DRM_ERROR("amdgpu asic reset failed\n");
>  	}
> 
> -	if (fbcon) {
> -		console_lock();
> -		amdgpu_fbdev_set_suspend(adev, 1);
> -		console_unlock();
> -	}
>  	return 0;
>  }
> 
> @@ -2140,9 +2205,7 @@ int amdgpu_device_suspend(struct drm_device
> *dev, bool suspend, bool fbcon)
>   */
>  int amdgpu_device_resume(struct drm_device *dev, bool resume, bool
> fbcon)  {
> -	struct drm_connector *connector;
>  	struct amdgpu_device *adev = dev->dev_private;
> -	struct drm_crtc *crtc;
>  	int r;
> 
>  	if (dev->switch_power_state == DRM_SWITCH_POWER_OFF) @@ -
> 2192,45 +2255,11 @@ int amdgpu_device_resume(struct drm_device *dev,
> bool resume, bool fbcon)
>  		return r;
>  	}
> 
> -	/* pin cursors */
> -	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> -		struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
> -
> -		if (amdgpu_crtc->cursor_bo) {
> -			struct amdgpu_bo *aobj =
> gem_to_amdgpu_bo(amdgpu_crtc->cursor_bo);
> -			r = amdgpu_bo_reserve(aobj, false);
> -			if (r == 0) {
> -				r = amdgpu_bo_pin(aobj,
> -
> AMDGPU_GEM_DOMAIN_VRAM,
> -						  &amdgpu_crtc-
> >cursor_addr);
> -				if (r != 0)
> -					DRM_ERROR("Failed to pin cursor BO
> (%d)\n", r);
> -				amdgpu_bo_unreserve(aobj);
> -			}
> -		}
> -	}
> -
> -	/* blat the mode back in */
> -	if (fbcon) {
> -		if (!amdgpu_device_has_dc_support(adev)) {
> -			/* pre DCE11 */
> -			drm_helper_resume_force_mode(dev);
> -
> -			/* turn on display hw */
> -			drm_modeset_lock_all(dev);
> -			list_for_each_entry(connector, &dev-
> >mode_config.connector_list, head) {
> -				drm_helper_connector_dpms(connector,
> DRM_MODE_DPMS_ON);
> -			}
> -			drm_modeset_unlock_all(dev);
> -		} else {
> -			/*
> -			 * There is no equivalent atomic helper to turn on
> -			 * display, so we defined our own function for this,
> -			 * once suspend resume is supported by the atomic
> -			 * framework this will be reworked
> -			 */
> -			amdgpu_dm_display_resume(adev);
> -		}
> +	r = amdgpu_device_resume_display(adev);
> +	if (r) {
> +		if (fbcon)
> +			console_unlock();
> +		return r;
>  	}
> 
>  	drm_kms_helper_poll_enable(dev);
> @@ -2256,7 +2285,7 @@ int amdgpu_device_resume(struct drm_device
> *dev, bool resume, bool fbcon)  #endif
> 
>  	if (fbcon) {
> -		amdgpu_fbdev_set_suspend(adev, 0);
> +		amdgpu_fbdev_set_suspend(adev, 0, false);
>  		console_unlock();
>  	}
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
> index 2867f55..d638e4f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
> @@ -37,6 +37,7 @@
>  #include <drm/drm_fb_helper.h>
> 
>  #include <linux/vga_switcheroo.h>
> +#include <linux/console.h>
> 
>  /* object hierarchy -
>     this contains a helper + a amdgpu fb @@ -400,11 +401,15 @@ void
> amdgpu_fbdev_fini(struct amdgpu_device *adev)
>  	adev->mode_info.rfbdev = NULL;
>  }
> 
> -void amdgpu_fbdev_set_suspend(struct amdgpu_device *adev, int state)
> +void amdgpu_fbdev_set_suspend(struct amdgpu_device *adev, int state,
> +bool fbcon)
>  {
> +	if (fbcon)
> +		console_lock();
>  	if (adev->mode_info.rfbdev)
>  		drm_fb_helper_set_suspend(&adev->mode_info.rfbdev-
> >helper,
>  			state);
> +	if (fbcon)
> +		console_unlock();
>  }
> 
>  int amdgpu_fbdev_total_size(struct amdgpu_device *adev) diff --git
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> index 0740673..8f36f63 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> @@ -659,7 +659,7 @@ void amdgpu_panel_mode_fixup(struct
> drm_encoder *encoder,
>  /* fbdev layer */
>  int amdgpu_fbdev_init(struct amdgpu_device *adev);  void
> amdgpu_fbdev_fini(struct amdgpu_device *adev); -void
> amdgpu_fbdev_set_suspend(struct amdgpu_device *adev, int state);
> +void amdgpu_fbdev_set_suspend(struct amdgpu_device *adev, int state,
> +bool fbcon);
>  int amdgpu_fbdev_total_size(struct amdgpu_device *adev);  bool
> amdgpu_fbdev_robj_is_fb(struct amdgpu_device *adev, struct amdgpu_bo
> *robj);  void amdgpu_fbdev_restore_mode(struct amdgpu_device *adev);
> --
> 1.9.1
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amd/amdgpu: fix console deadlock if late init failed

[Michel Dänzer]

On 01/03/17 06:35 PM, Jim Qu wrote:
> Change-Id: Ia914fac083ebab39617dad046d4bb98ab1ddf0a8
> Signed-off-by: Jim Qu <Jim.Qu@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index c0679d3..6eeda24 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2186,8 +2186,11 @@ int amdgpu_device_resume(struct drm_device *dev, bool resume, bool fbcon)
>  	}
>  
>  	r = amdgpu_late_init(adev);
> -	if (r)
> +	if (r) {
> +		if (fbcon)
> +			console_unlock();
>  		return r;
> +	}
>  
>  	/* pin cursors */
>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> 

Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

答复: [PATCH 2/2] drm/amd/amdgpu: add atomic helper to suspend/resume functions

[Qu, Jim]

Hi Andrey:

Yes, it is appropriate way to implement it in DC. it seems our base driver also need to be a bit change to adapt that. 

We encounter a custom P2 S4 issue on CI which need that fix. please go ahead , and push the change ASAP.

Thanks
JimQu

________________________________________
发件人: Grodzovsky, Andrey
发送时间: 2017年3月2日 0:29
收件人: Qu, Jim; amd-gfx@lists.freedesktop.org
抄送: Wentland, Harry
主题: RE: [PATCH 2/2] drm/amd/amdgpu: add atomic helper to suspend/resume functions

Hi, We in DAL as part of upstream effort also implemented switch to using S3 atomic helpers - http://git.amd.com:8080/#/c/67108/ (currently AMD folks view only)  please take a look
It seems to me a more appropriate would be to call atomic APIs from DAL, maybe merging this change with the DAL change would be a good idea.

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Jim Qu
> Sent: Wednesday, March 01, 2017 4:36 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Qu, Jim
> Subject: [PATCH 2/2] drm/amd/amdgpu: add atomic helper to
> suspend/resume functions
>
> Change-Id: I7f5b4bddf1fe0538de81f2268fe80927bee09ec5
> Signed-off-by: Jim Qu <Jim.Qu@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h        |   1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 179
> +++++++++++++++++------------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c     |   7 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h   |   2 +-
>  4 files changed, 112 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 8a5f8cb..07b99ab 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1504,6 +1504,7 @@ struct amdgpu_device {
>       /* record hw reset is performed */
>       bool has_hw_reset;
>
> +     struct drm_atomic_state *atomic_state;
>  };
>
>  static inline struct amdgpu_device *amdgpu_ttm_adev(struct
> ttm_bo_device *bdev) diff --git
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 6eeda24..0dfabc6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2027,39 +2027,18 @@ void amdgpu_device_fini(struct amdgpu_device
> *adev)
>       amdgpu_debugfs_remove_files(adev);
>  }
>
> -
> -/*
> - * Suspend & resume.
> - */
> -/**
> - * amdgpu_device_suspend - initiate device suspend
> - *
> - * @pdev: drm dev pointer
> - * @state: suspend state
> - *
> - * Puts the hw in the suspend state (all asics).
> - * Returns 0 for success or an error on failure.
> - * Called at driver suspend.
> - */
> -int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool
> fbcon)
> +int amdgpu_device_suspend_display(struct amdgpu_device *adev)
>  {
> -     struct amdgpu_device *adev;
>       struct drm_crtc *crtc;
>       struct drm_connector *connector;
> +     struct drm_device *dev = adev->ddev;
>       int r;
>
> -     if (dev == NULL || dev->dev_private == NULL) {
> -             return -ENODEV;
> -     }
> -
> -     adev = dev->dev_private;
> -
> -     if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> -             return 0;
> -
> -     drm_kms_helper_poll_disable(dev);
> -
> -     if (!amdgpu_device_has_dc_support(adev)) {
> +     if (amdgpu_device_has_dc_support(adev)) {
> +             adev->atomic_state = drm_atomic_helper_suspend(dev);
> +             if (IS_ERR(adev->atomic_state))
> +                     return -PTR_ERR(adev->atomic_state);
> +     } else {
>               /* turn off display hw */
>               drm_modeset_lock_all(dev);
>               list_for_each_entry(connector, &dev-
> >mode_config.connector_list, head) { @@ -2096,6 +2075,97 @@ int
> amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool
> fbcon)
>                       }
>               }
>       }
> +
> +     return 0;
> +}
> +
> +
> +int amdgpu_device_resume_display(struct amdgpu_device *adev) {
> +     struct drm_crtc *crtc;
> +     struct drm_connector *connector;
> +     struct drm_device *dev = adev->ddev;
> +     int r;
> +
> +     /* pin cursors */
> +     list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +             struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
> +
> +             if (amdgpu_crtc->cursor_bo) {
> +                     struct amdgpu_bo *aobj =
> gem_to_amdgpu_bo(amdgpu_crtc->cursor_bo);
> +                     r = amdgpu_bo_reserve(aobj, false);
> +                     if (r == 0) {
> +                             r = amdgpu_bo_pin(aobj,
> +
> AMDGPU_GEM_DOMAIN_VRAM,
> +                                               &amdgpu_crtc-
> >cursor_addr);
> +                             if (r != 0)
> +                                     DRM_ERROR("Failed to pin cursor BO
> (%d)\n", r);
> +                             amdgpu_bo_unreserve(aobj);
> +                     }
> +             }
> +     }
> +
> +     /* blat the mode back in */
> +     if (amdgpu_device_has_dc_support(adev)) {
> +             r = drm_atomic_helper_resume(dev, adev->atomic_state);
> +             if (r)
> +                     return r;
> +             r = amdgpu_dm_display_resume(adev);

 [Grodzovsky, Andrey] Seems to me atomic_comit will be called twice, once from drm_atomic_helper_resume and the other from  amdgpu_dm_display_resume.

> +             if(r)
> +                     return r;
> +     } else {
> +             /* pre DCE11 */
> +             drm_helper_resume_force_mode(dev);
> +
> +             /* turn on display hw */
> +             drm_modeset_lock_all(dev);
> +             list_for_each_entry(connector, &dev-
> >mode_config.connector_list, head) {
> +                     drm_helper_connector_dpms(connector,
> DRM_MODE_DPMS_ON);
> +             }
> +             drm_modeset_unlock_all(dev);
> +     }
> +
> +     return 0;
> +}
> +
> +/*
> + * Suspend & resume.
> + */
> +/**
> + * amdgpu_device_suspend - initiate device suspend
> + *
> + * @pdev: drm dev pointer
> + * @state: suspend state
> + *
> + * Puts the hw in the suspend state (all asics).
> + * Returns 0 for success or an error on failure.
> + * Called at driver suspend.
> + */
> +int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool
> +fbcon) {
> +     struct amdgpu_device *adev;
> +     int r;
> +
> +     if (dev == NULL || dev->dev_private == NULL) {
> +             return -ENODEV;
> +     }
> +
> +     adev = dev->dev_private;
> +
> +     if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> +             return 0;
> +
> +     drm_kms_helper_poll_disable(dev);
> +
> +     amdgpu_fbdev_set_suspend(adev, 1, fbcon);
> +
> +     r = amdgpu_device_suspend_display(adev);
> +     if(r) {
> +             amdgpu_fbdev_set_suspend(adev, 0, fbcon);
> +             drm_kms_helper_poll_enable(dev);
> +             return r;
> +     }
> +
>       /* evict vram memory */
>       amdgpu_bo_evict_vram(adev);
>
> @@ -2121,11 +2191,6 @@ int amdgpu_device_suspend(struct drm_device
> *dev, bool suspend, bool fbcon)
>                       DRM_ERROR("amdgpu asic reset failed\n");
>       }
>
> -     if (fbcon) {
> -             console_lock();
> -             amdgpu_fbdev_set_suspend(adev, 1);
> -             console_unlock();
> -     }
>       return 0;
>  }
>
> @@ -2140,9 +2205,7 @@ int amdgpu_device_suspend(struct drm_device
> *dev, bool suspend, bool fbcon)
>   */
>  int amdgpu_device_resume(struct drm_device *dev, bool resume, bool
> fbcon)  {
> -     struct drm_connector *connector;
>       struct amdgpu_device *adev = dev->dev_private;
> -     struct drm_crtc *crtc;
>       int r;
>
>       if (dev->switch_power_state == DRM_SWITCH_POWER_OFF) @@ -
> 2192,45 +2255,11 @@ int amdgpu_device_resume(struct drm_device *dev,
> bool resume, bool fbcon)
>               return r;
>       }
>
> -     /* pin cursors */
> -     list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> -             struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
> -
> -             if (amdgpu_crtc->cursor_bo) {
> -                     struct amdgpu_bo *aobj =
> gem_to_amdgpu_bo(amdgpu_crtc->cursor_bo);
> -                     r = amdgpu_bo_reserve(aobj, false);
> -                     if (r == 0) {
> -                             r = amdgpu_bo_pin(aobj,
> -
> AMDGPU_GEM_DOMAIN_VRAM,
> -                                               &amdgpu_crtc-
> >cursor_addr);
> -                             if (r != 0)
> -                                     DRM_ERROR("Failed to pin cursor BO
> (%d)\n", r);
> -                             amdgpu_bo_unreserve(aobj);
> -                     }
> -             }
> -     }
> -
> -     /* blat the mode back in */
> -     if (fbcon) {
> -             if (!amdgpu_device_has_dc_support(adev)) {
> -                     /* pre DCE11 */
> -                     drm_helper_resume_force_mode(dev);
> -
> -                     /* turn on display hw */
> -                     drm_modeset_lock_all(dev);
> -                     list_for_each_entry(connector, &dev-
> >mode_config.connector_list, head) {
> -                             drm_helper_connector_dpms(connector,
> DRM_MODE_DPMS_ON);
> -                     }
> -                     drm_modeset_unlock_all(dev);
> -             } else {
> -                     /*
> -                      * There is no equivalent atomic helper to turn on
> -                      * display, so we defined our own function for this,
> -                      * once suspend resume is supported by the atomic
> -                      * framework this will be reworked
> -                      */
> -                     amdgpu_dm_display_resume(adev);
> -             }
> +     r = amdgpu_device_resume_display(adev);
> +     if (r) {
> +             if (fbcon)
> +                     console_unlock();
> +             return r;
>       }
>
>       drm_kms_helper_poll_enable(dev);
> @@ -2256,7 +2285,7 @@ int amdgpu_device_resume(struct drm_device
> *dev, bool resume, bool fbcon)  #endif
>
>       if (fbcon) {
> -             amdgpu_fbdev_set_suspend(adev, 0);
> +             amdgpu_fbdev_set_suspend(adev, 0, false);
>               console_unlock();
>       }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
> index 2867f55..d638e4f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
> @@ -37,6 +37,7 @@
>  #include <drm/drm_fb_helper.h>
>
>  #include <linux/vga_switcheroo.h>
> +#include <linux/console.h>
>
>  /* object hierarchy -
>     this contains a helper + a amdgpu fb @@ -400,11 +401,15 @@ void
> amdgpu_fbdev_fini(struct amdgpu_device *adev)
>       adev->mode_info.rfbdev = NULL;
>  }
>
> -void amdgpu_fbdev_set_suspend(struct amdgpu_device *adev, int state)
> +void amdgpu_fbdev_set_suspend(struct amdgpu_device *adev, int state,
> +bool fbcon)
>  {
> +     if (fbcon)
> +             console_lock();
>       if (adev->mode_info.rfbdev)
>               drm_fb_helper_set_suspend(&adev->mode_info.rfbdev-
> >helper,
>                       state);
> +     if (fbcon)
> +             console_unlock();
>  }
>
>  int amdgpu_fbdev_total_size(struct amdgpu_device *adev) diff --git
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> index 0740673..8f36f63 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> @@ -659,7 +659,7 @@ void amdgpu_panel_mode_fixup(struct
> drm_encoder *encoder,
>  /* fbdev layer */
>  int amdgpu_fbdev_init(struct amdgpu_device *adev);  void
> amdgpu_fbdev_fini(struct amdgpu_device *adev); -void
> amdgpu_fbdev_set_suspend(struct amdgpu_device *adev, int state);
> +void amdgpu_fbdev_set_suspend(struct amdgpu_device *adev, int state,
> +bool fbcon);
>  int amdgpu_fbdev_total_size(struct amdgpu_device *adev);  bool
> amdgpu_fbdev_robj_is_fb(struct amdgpu_device *adev, struct amdgpu_bo
> *robj);  void amdgpu_fbdev_restore_mode(struct amdgpu_device *adev);
> --
> 1.9.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx