[PATCH] drm/i915: move power domain init earlier during system resume

[PATCH] drm/i915: move power domain init earlier during system resume

[Imre Deak]

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.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76152
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 68 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 57 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index fa5d0ed..696e127 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -537,6 +537,15 @@ static void intel_resume_hotplug(struct drm_device *dev)
 	drm_helper_hpd_irq_event(dev);
 }
 
+static int i915_drm_thaw_early(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	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;
@@ -553,8 +562,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 +626,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 +636,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 +657,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 +796,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 +805,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 +822,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 +851,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 +929,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 +979,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

[PATCH v2] drm/i915: move power domain init earlier during system resume

[Imre Deak]

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@intel.com>
---
 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

Re: [PATCH v2] drm/i915: move power domain init earlier during system resume

[Daniel Vetter]

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@intel.com>

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2] drm/i915: move power domain init earlier during system resume

[Imre Deak]

On Tue, 2014-04-01 at 19:48 +0200, Daniel Vetter wrote:
> 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@intel.com>
> 
> 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.

Right, I'm not too happy about this solution either, so if anything it
could be considered only a stop-gap fix. What you suggest seems to be a
cleaner way but it'd require more time to investigate/implement at least
on my part (but I'm ok to put it on my TODO list).

--Imre

Re: [PATCH v2] drm/i915: move power domain init earlier during system resume

[Daniel Vetter]

On Tue, Apr 01, 2014 at 09:50:43PM +0300, Imre Deak wrote:
> On Tue, 2014-04-01 at 19:48 +0200, Daniel Vetter wrote:
> > 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@intel.com>
> >
> > 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.
>
> Right, I'm not too happy about this solution either, so if anything it
> could be considered only a stop-gap fix. What you suggest seems to be a
> cleaner way but it'd require more time to investigate/implement at least
> on my part (but I'm ok to put it on my TODO list).

We'd definitely need to discuss this with Takashi Iwai, since it would
only really be useful if it's good enough to solve the general pile of
sound/gfx coordination issues we have with hdmi. Adding him and alsa-dev.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH v2] drm/i915: move power domain init earlier during system resume

[Takashi Iwai]

At Tue, 1 Apr 2014 22:26:20 +0200,
Daniel Vetter wrote:
> 
> On Tue, Apr 01, 2014 at 09:50:43PM +0300, Imre Deak wrote:
> > On Tue, 2014-04-01 at 19:48 +0200, Daniel Vetter wrote:
> > > 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@intel.com>
> > >
> > > 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.
> >
> > Right, I'm not too happy about this solution either, so if anything it
> > could be considered only a stop-gap fix. What you suggest seems to be a
> > cleaner way but it'd require more time to investigate/implement at least
> > on my part (but I'm ok to put it on my TODO list).
> 
> We'd definitely need to discuss this with Takashi Iwai, since it would
> only really be useful if it's good enough to solve the general pile of
> sound/gfx coordination issues we have with hdmi. Adding him and alsa-dev.

Yep, having a dedicated channel (hdmi sink bus or whatever) would be
definitely a better way to go.  OTOH, I agree that Imre's patch is
needed for the upcoming kernel.  The former implementation can't be
finished so quickly for 3.15.

I thought Mengdong started looking at (or working on) this, but
haven't heard the progress yet.  Mengdong, any news?


thanks,

Takashi

Re: [PATCH v2] drm/i915: move power domain init earlier during system resume

[Daniel Vetter]

On Wed, Apr 02, 2014 at 02:59:54PM +0200, Takashi Iwai wrote:
> At Tue, 1 Apr 2014 22:26:20 +0200,
> Daniel Vetter wrote:
> > 
> > On Tue, Apr 01, 2014 at 09:50:43PM +0300, Imre Deak wrote:
> > > On Tue, 2014-04-01 at 19:48 +0200, Daniel Vetter wrote:
> > > > 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@intel.com>
> > > >
> > > > 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.
> > >
> > > Right, I'm not too happy about this solution either, so if anything it
> > > could be considered only a stop-gap fix. What you suggest seems to be a
> > > cleaner way but it'd require more time to investigate/implement at least
> > > on my part (but I'm ok to put it on my TODO list).
> > 
> > We'd definitely need to discuss this with Takashi Iwai, since it would
> > only really be useful if it's good enough to solve the general pile of
> > sound/gfx coordination issues we have with hdmi. Adding him and alsa-dev.
> 
> Yep, having a dedicated channel (hdmi sink bus or whatever) would be
> definitely a better way to go.  OTOH, I agree that Imre's patch is
> needed for the upcoming kernel.  The former implementation can't be
> finished so quickly for 3.15.
> 
> I thought Mengdong started looking at (or working on) this, but
> haven't heard the progress yet.  Mengdong, any news?

Ok, I'll splash a big honky FIXME over this so we don't forget ;-) Is the
patch otherwise ok with you Takashi?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2] drm/i915: move power domain init earlier during system resume

[Takashi Iwai]

At Thu, 3 Apr 2014 17:23:10 +0200,
Daniel Vetter wrote:
> 
> On Wed, Apr 02, 2014 at 02:59:54PM +0200, Takashi Iwai wrote:
> > At Tue, 1 Apr 2014 22:26:20 +0200,
> > Daniel Vetter wrote:
> > > 
> > > On Tue, Apr 01, 2014 at 09:50:43PM +0300, Imre Deak wrote:
> > > > On Tue, 2014-04-01 at 19:48 +0200, Daniel Vetter wrote:
> > > > > 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@intel.com>
> > > > >
> > > > > 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.
> > > >
> > > > Right, I'm not too happy about this solution either, so if anything it
> > > > could be considered only a stop-gap fix. What you suggest seems to be a
> > > > cleaner way but it'd require more time to investigate/implement at least
> > > > on my part (but I'm ok to put it on my TODO list).
> > > 
> > > We'd definitely need to discuss this with Takashi Iwai, since it would
> > > only really be useful if it's good enough to solve the general pile of
> > > sound/gfx coordination issues we have with hdmi. Adding him and alsa-dev.
> > 
> > Yep, having a dedicated channel (hdmi sink bus or whatever) would be
> > definitely a better way to go.  OTOH, I agree that Imre's patch is
> > needed for the upcoming kernel.  The former implementation can't be
> > finished so quickly for 3.15.
> > 
> > I thought Mengdong started looking at (or working on) this, but
> > haven't heard the progress yet.  Mengdong, any news?
> 
> Ok, I'll splash a big honky FIXME over this so we don't forget ;-) Is the
> patch otherwise ok with you Takashi?

Yes, it's touching only i915 stuff in anyway ;)
Feel free to take my ack:
	Reviewed-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

Re: [PATCH v2] drm/i915: move power domain init earlier during system resume

[Daniel Vetter]

On Thu, Apr 03, 2014 at 05:31:17PM +0200, Takashi Iwai wrote:
> At Thu, 3 Apr 2014 17:23:10 +0200,
> Daniel Vetter wrote:
> > 
> > On Wed, Apr 02, 2014 at 02:59:54PM +0200, Takashi Iwai wrote:
> > > At Tue, 1 Apr 2014 22:26:20 +0200,
> > > Daniel Vetter wrote:
> > > > 
> > > > On Tue, Apr 01, 2014 at 09:50:43PM +0300, Imre Deak wrote:
> > > > > On Tue, 2014-04-01 at 19:48 +0200, Daniel Vetter wrote:
> > > > > > 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@intel.com>
> > > > > >
> > > > > > 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.
> > > > >
> > > > > Right, I'm not too happy about this solution either, so if anything it
> > > > > could be considered only a stop-gap fix. What you suggest seems to be a
> > > > > cleaner way but it'd require more time to investigate/implement at least
> > > > > on my part (but I'm ok to put it on my TODO list).
> > > > 
> > > > We'd definitely need to discuss this with Takashi Iwai, since it would
> > > > only really be useful if it's good enough to solve the general pile of
> > > > sound/gfx coordination issues we have with hdmi. Adding him and alsa-dev.
> > > 
> > > Yep, having a dedicated channel (hdmi sink bus or whatever) would be
> > > definitely a better way to go.  OTOH, I agree that Imre's patch is
> > > needed for the upcoming kernel.  The former implementation can't be
> > > finished so quickly for 3.15.
> > > 
> > > I thought Mengdong started looking at (or working on) this, but
> > > haven't heard the progress yet.  Mengdong, any news?
> > 
> > Ok, I'll splash a big honky FIXME over this so we don't forget ;-) Is the
> > patch otherwise ok with you Takashi?
> 
> Yes, it's touching only i915 stuff in anyway ;)
> Feel free to take my ack:
> 	Reviewed-by: Takashi Iwai <tiwai@suse.de>

Ok, I've pulled this into 3.15-fixes and added some big comments to the
newly-added late/early hooks. I'll try to poke Mengmeng about the status
of the hdmi infrastructure work.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch