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 at 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 at lists.freedesktop.org] On Behalf > Of Jim Qu > Sent: Wednesday, March 01, 2017 4:36 AM > To: amd-gfx at 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 at 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 at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
