Hello Jerome, After several days of testing here the adapted version of your patch. With this version s2disk and s2mem is working flawlessly for me.
For the freeze()/thaw() case, it does the "unpin the front buffers" part and for consistency thaw() is aligned accordingly. Without it, resume from s2disk crashes (machine freezes up). Basically that's all I can technically contribute to the patch at this level. Cheers, Oliver On Sunday 19 July 2015 18:21:04 Oliver Winker wrote: > Just for info: > > I also tested the https://patchwork.kernel.org/patch/6641371 and 6641381 > patches together with 0001-drm-radeon-rework-device-power-management- for- > radeon.patch: > > It hangs on resume after image restore. > > -Oliver > > On Sunday 19 July 2015 17:03:14 Oliver Winker wrote: > > Hello Jerome, > > > > Just to let you know that I'm back and a small intermediate status on this > > story: > > > > On Thursday 18 June 2015 23:16:57 Jerome Glisse wrote: > > > Anyway as it is your patch would break runtime suspend. > > > > > > I recently posted patch to fix the resume side of hibernation, they > > > might > > > also help in the freeze()/thaw() dance. Could you please test them > > > without > > > your patch : > > > https://patchwork.kernel.org/patch/6641371/ > > > https://patchwork.kernel.org/patch/6641381/ > > > > I tested those two patches, but they (at least separately) didn't give an > > s2disk improvement. > > > > > If it is not enough than also try with attached patch (which i have not > > > tested myself but i think the logic is right). > > > > The reworked patch seemed to worked for the s2disk snapshot-taking > > freeze()->thaw(), but the resume() of the restored image didn't work then. > > > > Attached is a patch onto your > > 0001-drm-radeon-rework-device-power-management- for-radeon.patch, which > > basically realigns the logic to the previous flow of my initial patch. > > With > > this it works ok again. > > > > In the freeze() case it does again the "unpin the front buffers" part and > > in thaw() in again does "init dig PHYs, disp eng pll" plus what follows. > > > > I need to test a bit more which is the minimum delta to have it working > > with regards to your patch. > > > > The attached patch is just for info. It doesn't reindent the code to keep > > it more readable for the time being. > > > > So I'll continue "tuning" this is a bit. But I do this just on the > > high-level suspend/resume() sequence that I see in the from the existing > > code. Without deeper radeon-hw-reasoning behind. > > > > BR, Oliver
>From ae3de3c9ffc21b6f41bbd3b97ae1e808b03170e8 Mon Sep 17 00:00:00 2001 From: Oliver Winker <[email protected]> Date: Sun, 19 Jul 2015 15:32:31 +0200 Subject: [PATCH] drm radeon rework device power management for radeon v2 --- drivers/gpu/drm/radeon/radeon.h | 4 +- drivers/gpu/drm/radeon/radeon_device.c | 107 +++++++++++++++++++-------------- drivers/gpu/drm/radeon/radeon_drv.c | 25 +++++--- drivers/gpu/drm/radeon/radeon_family.h | 16 +++++ 4 files changed, 95 insertions(+), 57 deletions(-) diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 33d5a4f..b727ffa 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -2979,8 +2979,8 @@ extern bool radeon_ttm_tt_has_userptr(struct ttm_tt *ttm); extern bool radeon_ttm_tt_is_readonly(struct ttm_tt *ttm); extern void radeon_vram_location(struct radeon_device *rdev, struct radeon_mc *mc, u64 base); extern void radeon_gtt_location(struct radeon_device *rdev, struct radeon_mc *mc); -extern int radeon_resume_kms(struct drm_device *dev, bool resume, bool fbcon); -extern int radeon_suspend_kms(struct drm_device *dev, bool suspend, bool fbcon); +extern int radeon_resume_kms(struct drm_device *dev, enum radeon_pm_state state); +extern int radeon_suspend_kms(struct drm_device *dev, enum radeon_pm_state state); extern void radeon_ttm_set_active_vram_size(struct radeon_device *rdev, u64 size); extern void radeon_program_register_sequence(struct radeon_device *rdev, const u32 *registers, diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c index bd7519f..5ef5604 100644 --- a/drivers/gpu/drm/radeon/radeon_device.c +++ b/drivers/gpu/drm/radeon/radeon_device.c @@ -1214,7 +1214,7 @@ static void radeon_switcheroo_set_state(struct pci_dev *pdev, enum vga_switchero if (d3_delay < 20 && (rdev->px_quirk_flags & RADEON_PX_QUIRK_LONG_WAKEUP)) dev->pdev->d3_delay = 20; - radeon_resume_kms(dev, true, true); + radeon_resume_kms(dev, RADEON_PM_RESUME); dev->pdev->d3_delay = d3_delay; @@ -1224,7 +1224,7 @@ static void radeon_switcheroo_set_state(struct pci_dev *pdev, enum vga_switchero printk(KERN_INFO "radeon: switched off\n"); drm_kms_helper_poll_disable(dev); dev->switch_power_state = DRM_SWITCH_POWER_CHANGING; - radeon_suspend_kms(dev, true, true); + radeon_suspend_kms(dev, RADEON_PM_SUSPEND); dev->switch_power_state = DRM_SWITCH_POWER_OFF; } } @@ -1523,13 +1523,13 @@ void radeon_device_fini(struct radeon_device *rdev) * radeon_suspend_kms - initiate device suspend * * @pdev: drm dev pointer - * @state: suspend state + * @state: suspend to state * * Puts the hw in the suspend state (all asics). * Returns 0 for success or an error on failure. * Called at driver suspend. */ -int radeon_suspend_kms(struct drm_device *dev, bool suspend, bool fbcon) +int radeon_suspend_kms(struct drm_device *dev, enum radeon_pm_state state) { struct radeon_device *rdev; struct drm_crtc *crtc; @@ -1545,11 +1545,17 @@ int radeon_suspend_kms(struct drm_device *dev, bool suspend, bool fbcon) if (dev->switch_power_state == DRM_SWITCH_POWER_OFF) return 0; + /* + * This is not symetrical for freeze() and thaw() but its fine + * as during thaw() we do not care for screen hotplug really. + */ drm_kms_helper_poll_disable(dev); - /* turn off display hw */ - list_for_each_entry(connector, &dev->mode_config.connector_list, head) { - drm_helper_connector_dpms(connector, DRM_MODE_DPMS_OFF); + if (state != RADEON_PM_FREEZE) { + /* turn off display hw */ + list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + drm_helper_connector_dpms(connector, DRM_MODE_DPMS_OFF); + } } /* unpin the front buffers */ @@ -1582,23 +1588,23 @@ int radeon_suspend_kms(struct drm_device *dev, bool suspend, bool fbcon) } } - radeon_save_bios_scratch_regs(rdev); + if (state != RADEON_PM_FREEZE) { + radeon_save_bios_scratch_regs(rdev); - radeon_suspend(rdev); - radeon_hpd_fini(rdev); - /* evict remaining vram memory */ - radeon_bo_evict_vram(rdev); + radeon_suspend(rdev); + radeon_hpd_fini(rdev); + /* evict remaining vram memory */ + radeon_bo_evict_vram(rdev); - radeon_agp_suspend(rdev); + radeon_agp_suspend(rdev); + } - pci_save_state(dev->pdev); - if (suspend) { + if (state == RADEON_PM_SUSPEND) { + pci_save_state(dev->pdev); /* Shut down the device */ pci_disable_device(dev->pdev); pci_set_power_state(dev->pdev, PCI_D3hot); - } - if (fbcon) { console_lock(); radeon_fbdev_set_suspend(rdev, 1); console_unlock(); @@ -1610,12 +1616,13 @@ int radeon_suspend_kms(struct drm_device *dev, bool suspend, bool fbcon) * radeon_resume_kms - initiate device resume * * @pdev: drm dev pointer + * @state: resume from state * * Bring the hw back to operating state (all asics). * Returns 0 for success or an error on failure. * Called at driver resume. */ -int radeon_resume_kms(struct drm_device *dev, bool resume, bool fbcon) +int radeon_resume_kms(struct drm_device *dev, enum radeon_pm_state state) { struct drm_connector *connector; struct radeon_device *rdev = dev->dev_private; @@ -1624,39 +1631,40 @@ int radeon_resume_kms(struct drm_device *dev, bool resume, bool fbcon) if (dev->switch_power_state == DRM_SWITCH_POWER_OFF) return 0; - if (fbcon) { + if (state == RADEON_PM_RESUME) { console_lock(); - } - if (resume) { pci_set_power_state(dev->pdev, PCI_D0); pci_restore_state(dev->pdev); if (pci_enable_device(dev->pdev)) { - if (fbcon) - console_unlock(); + console_unlock(); return -1; } } - /* resume AGP if in use */ - radeon_agp_resume(rdev); - radeon_resume(rdev); - r = radeon_ib_ring_tests(rdev); - if (r) - DRM_ERROR("ib ring test failed (%d).\n", r); + if (state != RADEON_PM_THAW) { + /* resume AGP if in use */ + radeon_agp_resume(rdev); + radeon_resume(rdev); - if ((rdev->pm.pm_method == PM_METHOD_DPM) && rdev->pm.dpm_enabled) { - /* do dpm late init */ - r = radeon_pm_late_init(rdev); - if (r) { - rdev->pm.dpm_enabled = false; - DRM_ERROR("radeon_pm_late_init failed, disabling dpm\n"); + r = radeon_ib_ring_tests(rdev); + if (r) + DRM_ERROR("ib ring test failed (%d).\n", r); + + if ((rdev->pm.pm_method == PM_METHOD_DPM) && + rdev->pm.dpm_enabled) { + /* do dpm late init */ + r = radeon_pm_late_init(rdev); + if (r) { + rdev->pm.dpm_enabled = false; + DRM_ERROR("radeon_pm_late_init failed, disabling dpm\n"); + } + } else { + /* resume old pm late */ + radeon_pm_resume(rdev); } - } else { - /* resume old pm late */ - radeon_pm_resume(rdev); - } - radeon_restore_bios_scratch_regs(rdev); + radeon_restore_bios_scratch_regs(rdev); + } /* init dig PHYs, disp eng pll */ if (rdev->is_atom_bios) { @@ -1665,29 +1673,36 @@ int radeon_resume_kms(struct drm_device *dev, bool resume, bool fbcon) /* turn on the BL */ if (rdev->mode_info.bl_encoder) { u8 bl_level = radeon_get_backlight_level(rdev, - rdev->mode_info.bl_encoder); - radeon_set_backlight_level(rdev, rdev->mode_info.bl_encoder, - bl_level); + rdev->mode_info.bl_encoder); + radeon_set_backlight_level(rdev, + rdev->mode_info.bl_encoder, + bl_level); } } /* reset hpd state */ radeon_hpd_init(rdev); + /* blat the mode back in */ - if (fbcon) { + if (state != RADEON_PM_RUNTIME_RESUME) { drm_helper_resume_force_mode(dev); /* turn on display hw */ - list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + list_for_each_entry(connector, + &dev->mode_config.connector_list, head) { drm_helper_connector_dpms(connector, DRM_MODE_DPMS_ON); } } + /* + * This is not symetrical for freeze() and thaw() but its fine + * as during thaw() we do not care for screen hotplug really. + */ drm_kms_helper_poll_enable(dev); /* set the power state here in case we are a PX system or headless */ if ((rdev->pm.pm_method == PM_METHOD_DPM) && rdev->pm.dpm_enabled) radeon_pm_compute_clocks(rdev); - if (fbcon) { + if (state == RADEON_PM_RESUME) { radeon_fbdev_set_suspend(rdev, 0); console_unlock(); } diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c index 5d684be..2ed2f19 100644 --- a/drivers/gpu/drm/radeon/radeon_drv.c +++ b/drivers/gpu/drm/radeon/radeon_drv.c @@ -101,8 +101,8 @@ void radeon_driver_postclose_kms(struct drm_device *dev, struct drm_file *file_priv); void radeon_driver_preclose_kms(struct drm_device *dev, struct drm_file *file_priv); -int radeon_suspend_kms(struct drm_device *dev, bool suspend, bool fbcon); -int radeon_resume_kms(struct drm_device *dev, bool resume, bool fbcon); +int radeon_suspend_kms(struct drm_device *dev, enum radeon_pm_state state); +int radeon_resume_kms(struct drm_device *dev, enum radeon_pm_state state); u32 radeon_get_vblank_counter_kms(struct drm_device *dev, int crtc); int radeon_enable_vblank_kms(struct drm_device *dev, int crtc); void radeon_disable_vblank_kms(struct drm_device *dev, int crtc); @@ -411,28 +411,35 @@ static int radeon_pmops_suspend(struct device *dev) { struct pci_dev *pdev = to_pci_dev(dev); struct drm_device *drm_dev = pci_get_drvdata(pdev); - return radeon_suspend_kms(drm_dev, true, true); + return radeon_suspend_kms(drm_dev, RADEON_PM_SUSPEND); } static int radeon_pmops_resume(struct device *dev) { struct pci_dev *pdev = to_pci_dev(dev); struct drm_device *drm_dev = pci_get_drvdata(pdev); - return radeon_resume_kms(drm_dev, true, true); + return radeon_resume_kms(drm_dev, RADEON_PM_RESUME); } static int radeon_pmops_freeze(struct device *dev) { struct pci_dev *pdev = to_pci_dev(dev); struct drm_device *drm_dev = pci_get_drvdata(pdev); - return radeon_suspend_kms(drm_dev, false, true); + return radeon_suspend_kms(drm_dev, RADEON_PM_FREEZE); +} + +static int radeon_pmops_poweroff(struct device *dev) +{ + struct pci_dev *pdev = to_pci_dev(dev); + struct drm_device *drm_dev = pci_get_drvdata(pdev); + return radeon_suspend_kms(drm_dev, RADEON_PM_POWEROFF); } static int radeon_pmops_thaw(struct device *dev) { struct pci_dev *pdev = to_pci_dev(dev); struct drm_device *drm_dev = pci_get_drvdata(pdev); - return radeon_resume_kms(drm_dev, false, true); + return radeon_resume_kms(drm_dev, RADEON_PM_THAW); } static int radeon_pmops_runtime_suspend(struct device *dev) @@ -450,7 +457,7 @@ static int radeon_pmops_runtime_suspend(struct device *dev) drm_kms_helper_poll_disable(drm_dev); vga_switcheroo_set_dynamic_switch(pdev, VGA_SWITCHEROO_OFF); - ret = radeon_suspend_kms(drm_dev, false, false); + ret = radeon_suspend_kms(drm_dev, RADEON_PM_RUNTIME_SUSPEND); pci_save_state(pdev); pci_disable_device(pdev); pci_ignore_hotplug(pdev); @@ -478,7 +485,7 @@ static int radeon_pmops_runtime_resume(struct device *dev) return ret; pci_set_master(pdev); - ret = radeon_resume_kms(drm_dev, false, false); + ret = radeon_resume_kms(drm_dev, RADEON_PM_RUNTIME_RESUME); drm_kms_helper_poll_enable(drm_dev); vga_switcheroo_set_dynamic_switch(pdev, VGA_SWITCHEROO_ON); drm_dev->switch_power_state = DRM_SWITCH_POWER_ON; @@ -532,7 +539,7 @@ static const struct dev_pm_ops radeon_pm_ops = { .resume = radeon_pmops_resume, .freeze = radeon_pmops_freeze, .thaw = radeon_pmops_thaw, - .poweroff = radeon_pmops_freeze, + .poweroff = radeon_pmops_poweroff, .restore = radeon_pmops_resume, .runtime_suspend = radeon_pmops_runtime_suspend, .runtime_resume = radeon_pmops_runtime_resume, diff --git a/drivers/gpu/drm/radeon/radeon_family.h b/drivers/gpu/drm/radeon/radeon_family.h index 4b7b87f..f6078f5 100644 --- a/drivers/gpu/drm/radeon/radeon_family.h +++ b/drivers/gpu/drm/radeon/radeon_family.h @@ -119,4 +119,20 @@ enum radeon_chip_flags { RADEON_IS_PX = 0x02000000UL, }; +/* + * Sadly there is no easy common place share btw radeon_drv.c and rest of kms + * code to define enum. So for lack of better place, here are the enum for the + * power management states. + */ +enum radeon_pm_state { + RADEON_PM_FREEZE, + RADEON_PM_POWEROFF, + RADEON_PM_SUSPEND, + RADEON_PM_RUNTIME_SUSPEND, + + RADEON_PM_RESUME, + RADEON_PM_RUNTIME_RESUME, + RADEON_PM_THAW, +}; + #endif -- 2.1.4
_______________________________________________ xorg-driver-ati mailing list [email protected] http://lists.x.org/mailman/listinfo/xorg-driver-ati
