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 <imre.deak@xxxxxxxxx> 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 > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > 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 Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
