On Tue, Apr 01, 2014 at 07:55:22PM +0300, Imre Deak wrote:
> During resume the intel hda audio driver depends on the i915 driver
> reinitializing the audio power domain. Since the order of calling the
> i915 resume handler wrt. that of the audio driver is not guaranteed,
> move the power domain reinitialization step to the resume_early
> handler. This is guaranteed to run before the resume handler of any
> other driver.
>
> The power domain initialization in turn requires us to enable the i915
> pci device first, so move that part earlier too.
>
> Accordingly disabling of the i915 pci device should happen after the
> audio suspend handler ran. So move the disabling later from the i915
> resume handler to the resume_late handler.
>
> v2:
> - move intel_uncore_sanitize/early_sanitize earlier too, so they don't
> get reordered wrt. intel_power_domains_init_hw()
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76152
> Signed-off-by: Imre Deak <[email protected]>
So this is kinda why we should have gone with something proper, like a new
hdmi sink platform device created by i915 and registered as a driver by
snd-hda. Then the power domains stuff in the device core should take care
of these kinds of ordering issues. Or at least snd-hda can tell it that it
needs to wait for the hdmi-sink power domain to go on first before it can
resume, I'm not really fluent on the details here.
And having a hdmi sink bus would allow us to throw all kinds of crap into
a clearly-defined interface, e.g. eld handling, hdcp synchronization, hpd
forwarding and all the other fun stuff.
So not sure what I should do with this here now.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_drv.c | 72
> +++++++++++++++++++++++++++++++++--------
> 1 file changed, 58 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 11f77a8..90c399d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -537,14 +537,21 @@ static void intel_resume_hotplug(struct drm_device *dev)
> drm_helper_hpd_irq_event(dev);
> }
>
> -static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
> +static int i915_drm_thaw_early(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - int error = 0;
>
> intel_uncore_early_sanitize(dev);
> -
> intel_uncore_sanitize(dev);
> + intel_power_domains_init_hw(dev_priv);
> +
> + return 0;
> +}
> +
> +static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + int error = 0;
>
> if (drm_core_check_feature(dev, DRIVER_MODESET) &&
> restore_gtt_mappings) {
> @@ -553,8 +560,6 @@ static int __i915_drm_thaw(struct drm_device *dev, bool
> restore_gtt_mappings)
> mutex_unlock(&dev->struct_mutex);
> }
>
> - intel_power_domains_init_hw(dev_priv);
> -
> i915_restore_state(dev);
> intel_opregion_setup(dev);
>
> @@ -619,11 +624,8 @@ static int i915_drm_thaw(struct drm_device *dev)
> return __i915_drm_thaw(dev, true);
> }
>
> -int i915_resume(struct drm_device *dev)
> +static int i915_resume_early(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - int ret;
> -
> if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> return 0;
>
> @@ -632,6 +634,14 @@ int i915_resume(struct drm_device *dev)
>
> pci_set_master(dev->pdev);
>
> + return i915_drm_thaw_early(dev);
> +}
> +
> +int i915_resume(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + int ret;
> +
> /*
> * Platforms with opregion should have sane BIOS, older ones (gen3 and
> * earlier) need to restore the GTT mappings since the BIOS might clear
> @@ -645,6 +655,14 @@ int i915_resume(struct drm_device *dev)
> return 0;
> }
>
> +int i915_resume_legacy(struct drm_device *dev)
> +{
> + i915_resume_early(dev);
> + i915_resume(dev);
> +
> + return 0;
> +}
> +
> /**
> * i915_reset - reset chip after a hang
> * @dev: drm device to reset
> @@ -776,7 +794,6 @@ static int i915_pm_suspend(struct device *dev)
> {
> struct pci_dev *pdev = to_pci_dev(dev);
> struct drm_device *drm_dev = pci_get_drvdata(pdev);
> - int error;
>
> if (!drm_dev || !drm_dev->dev_private) {
> dev_err(dev, "DRM not initialized, aborting suspend.\n");
> @@ -786,9 +803,16 @@ static int i915_pm_suspend(struct device *dev)
> if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> return 0;
>
> - error = i915_drm_freeze(drm_dev);
> - if (error)
> - return error;
> + return i915_drm_freeze(drm_dev);
> +}
> +
> +static int i915_pm_suspend_late(struct device *dev)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> + struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +
> + if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> + return 0;
>
> pci_disable_device(pdev);
> pci_set_power_state(pdev, PCI_D3hot);
> @@ -796,6 +820,14 @@ static int i915_pm_suspend(struct device *dev)
> return 0;
> }
>
> +static int i915_pm_resume_early(struct device *dev)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> + struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +
> + return i915_resume_early(drm_dev);
> +}
> +
> static int i915_pm_resume(struct device *dev)
> {
> struct pci_dev *pdev = to_pci_dev(dev);
> @@ -817,6 +849,14 @@ static int i915_pm_freeze(struct device *dev)
> return i915_drm_freeze(drm_dev);
> }
>
> +static int i915_pm_thaw_early(struct device *dev)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> + struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +
> + return i915_drm_thaw_early(drm_dev);
> +}
> +
> static int i915_pm_thaw(struct device *dev)
> {
> struct pci_dev *pdev = to_pci_dev(dev);
> @@ -887,10 +927,14 @@ static int i915_runtime_resume(struct device *device)
>
> static const struct dev_pm_ops i915_pm_ops = {
> .suspend = i915_pm_suspend,
> + .suspend_late = i915_pm_suspend_late,
> + .resume_early = i915_pm_resume_early,
> .resume = i915_pm_resume,
> .freeze = i915_pm_freeze,
> + .thaw_early = i915_pm_thaw_early,
> .thaw = i915_pm_thaw,
> .poweroff = i915_pm_poweroff,
> + .restore_early = i915_pm_resume_early,
> .restore = i915_pm_resume,
> .runtime_suspend = i915_runtime_suspend,
> .runtime_resume = i915_runtime_resume,
> @@ -933,7 +977,7 @@ static struct drm_driver driver = {
>
> /* Used in place of i915_pm_ops for non-DRIVER_MODESET */
> .suspend = i915_suspend,
> - .resume = i915_resume,
> + .resume = i915_resume_legacy,
>
> .device_is_agp = i915_driver_device_is_agp,
> .master_create = i915_master_create,
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> [email protected]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
