[alsa-devel] [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team
Hi Guys,
I apologize that I may have misunderstood on the agreement we have made on power well work. I assumed that audio team is expected to create private gfx driver API to control the power well between gfx and audio for internal releases. That's why I asked Xingchao work on that.
So the correct implementation approach between gfx and audio should be,
#1, Daniel will work on the existing private API in gfx driver for power well control that can be used by the audio driver to enable/disable the power well.
#2, Once #1 is done, Audio team will integrate Daniel's patch into audio driver and add/improve on anything that is missing
Please correct me if my understanding is not correct.
Thanks, Jocelyn
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] Sent: Thursday, April 25, 2013 3:52 PM To: Wang, Xingchao Cc: Zanoni, Paulo R; ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; Li, Jocelyn; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Wang Xingchao; Takashi Iwai; Barnes, Jesse; Wysocki, Rafael J Subject: Re: [PATCH] drm/i915: Add private api for power well usage
On Thu, Apr 25, 2013 at 4:36 AM, Wang, Xingchao xingchao.wang@intel.com wrote:
Hi Daniel/Paulo,
Any comments on this? Add Jesse and Rafael in loop.
Well I've figured we want to get the power well integration working before we waste time reviewing an interface which might or might not quite work. But if you want, I can drop two quick comments: - I think the functions needs some prefix like i915_ - Imo the power well should just be reference counted (the current interface name suggests it's generally useable, but actually can only cope with one request from the audio driver). - Is the spin_lock (irq-disabling even) really required for the audio driver to work? power well enabling can take up to 20ms, so this won't work as-is. Can't we just use a mutex? - I'd need to see the audio driver side of this to check how this works for coping with arbitrary module load ordering.
Cheers, Daniel
thanks --xingchao
-----Original Message----- From: Wang, Xingchao Sent: Wednesday, April 24, 2013 3:29 PM To: daniel.vetter@ffwll.ch; Zanoni, Paulo R; 'Takashi Iwai' Cc: ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; Li, Jocelyn; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; 'Wang Xingchao' Subject: RE: [PATCH] drm/i915: Add private api for power well usage
Hi Guys,
Sorry I should Add Takashi and alsa maillist in loop.
I tested this patch against Daniel's "drm-intel-next-queued" Branch, on Haswell Mobile C3 stepping machines.
Test steps:
- only connect one monitor through HDMI cable 2. echo 1 >
/sys/module/i915/parameters/disable_power_well ==> I think this could be set as 1 by default if applied my patch. 3. plug out HDMI cable 4. cat /proc/asound/card0/codec#0
At step 3, i915 will try to disable power well but rejected as display audio is using power well. Without the patch, display audio would crash at step 4.
I did not list suspend/resume test here, as during suspend sequence, i915 will _ENABLE_ power well, that's a BUG?
For me above test case is the only one to reproduce the issue, if you have more ideas and need more test, please let me know.
I'm afraid there's some risk because the dependency between alsa driver and drm driver. For example, if i915 module was not loaded, display audio Will call request_power_well() and meet symbol error. Even if i915 module loaded after snd-hda-intel module, there's such risk too. Any comment on this?
Another risk is the pointer "power_well_device", it should be set NULL when i915 module unloaded, I added the caller but have no idea where to call it, if anyone has suggestion, that would be fine.
thanks --xingchao
-----Original Message----- From: Wang Xingchao [mailto:xingchao.wang@linux.intel.com] Sent: Thursday, April 24, 2014 10:20 PM To: daniel.vetter@ffwll.ch; Zanoni, Paulo R Cc: ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; Li, Jocelyn; intel-gfx@lists.freedesktop.org; Wang, Xingchao; Wang Xingchao Subject: [PATCH] drm/i915: Add private api for power well usage
Display audio depends on power well in graphic side, it should request power well before use it and release power well after use. I915 will not shutdown power well if it detects audio is using. This patch protects display audio crash for Intel Haswell mobile C3 stepping board.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
drivers/gpu/drm/i915/intel_pm.c | 73 +++++++++++++++++++++++++++++++++++---- include/drm/audio_drm.h | 39 +++++++++++++++++++++ sound/pci/hda/hda_intel.c | 8 +++++ 3 files changed, 113 insertions(+), 7 deletions(-) create mode 100644 include/drm/audio_drm.h
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 2557926..9983f56 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4298,18 +4298,12 @@ bool intel_using_power_well(struct drm_device *dev) return true; }
-void intel_set_power_well(struct drm_device *dev, bool enable) +void set_power_well(struct drm_device *dev, bool enable) { struct drm_i915_private *dev_priv = dev->dev_private; bool is_enabled, enable_requested; uint32_t tmp;
- if (!HAS_POWER_WELL(dev))
return;- if (!i915_disable_power_well && !enable)
return;- tmp = I915_READ(HSW_PWR_WELL_DRIVER); is_enabled = tmp & HSW_PWR_WELL_STATE; enable_requested = tmp & HSW_PWR_WELL_ENABLE; @@ -4332,6
+4326,71 @@ void intel_set_power_well(struct drm_device *dev, bool +enable) } }
+/* Global drm_device for display audio drvier usage */ static +struct drm_device *power_well_device; static bool +audio_power_well_using, i915_power_well_using; +/* Lock protecting power well setting */ +DEFINE_SPINLOCK(powerwell_lock);
+void request_power_well(void) +{
- DRM_DEBUG_KMS("Display audio requesting power well\n");
- spin_lock_irq(&powerwell_lock);
- if (!power_well_device)
goto out;- if (i915_power_well_using == false)
set_power_well(power_well_device, true);- audio_power_well_using = true;
+out:
- spin_unlock_irq(&powerwell_lock);
- return;
+} +EXPORT_SYMBOL_GPL(request_power_well);
+void release_power_well(void) +{
- DRM_DEBUG_KMS("Display audio releasing power well\n");
- spin_lock_irq(&powerwell_lock);
- if (!power_well_device)
goto out;- if (i915_power_well_using == false)
set_power_well(power_well_device, false);- audio_power_well_using = false;
+out:
- spin_unlock_irq(&powerwell_lock);
- return;
+} +EXPORT_SYMBOL_GPL(release_power_well);
+/* TODO: Call this when i915 module unload */ void +remove_power_well(void) {
- spin_lock_irq(&powerwell_lock);
- audio_power_well_using = false;
- i915_power_well_using = false;
- power_well_device = NULL;
- spin_unlock_irq(&powerwell_lock); }
+void intel_set_power_well(struct drm_device *dev, bool enable) {
- if (!HAS_POWER_WELL(dev))
return;- spin_lock_irq(&powerwell_lock);
- power_well_device = dev;
- i915_power_well_using = enable;
- if (!enable && audio_power_well_using)
goto out;- if (!i915_disable_power_well && !enable)
goto out;- set_power_well(dev, enable);
+out:
- spin_unlock_irq(&powerwell_lock);
- return;
+}
/*
- Starting with Haswell, we have a "Power Down Well" that can be
turned
off
- when not needed anymore. We have 4 registers that can request
the power well diff --git a/include/drm/audio_drm.h b/include/drm/audio_drm.h new file mode 100644 index 0000000..4412b36 --- /dev/null +++ b/include/drm/audio_drm.h @@ -0,0 +1,39 @@
+/**************************************************************
+****
- Copyright 2013 Intel Inc.
- All Rights Reserved.
- Permission is hereby granted, free of charge, to any person
+obtaining a
- copy of this software and associated documentation files (the
- "Software"), to deal in the Software without restriction,
+including
- without limitation the rights to use, copy, modify, merge,
+publish,
- distribute, sub license, and/or sell copies of the Software,
+and to
- permit persons to whom the Software is furnished to do so,
+subject to
- the following conditions:
- The above copyright notice and this permission notice
+(including the
- next paragraph) shall be included in all copies or substantial
+portions
- of the Software.
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
KIND,
+EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO
EVENT +SHALL
- THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE
FOR +ANY CLAIM,
- DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
TORT +OR
- OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
SOFTWARE +OR THE
- USE OR OTHER DEALINGS IN THE SOFTWARE.
+***************************************************************
+***/ +/*
- Authors:
- Wang Xingchao xingchao.wang@intel.com */
+#ifndef _AUDIO_DRM_H_ +#define _AUDIO_DRM_H_
+extern void request_power_well(void); extern void +release_power_well(void);
+#endif /* _AUDIO_DRM_H_ */ diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 418bfc0..59a85de 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -50,6 +50,7 @@ #include <linux/clocksource.h> #include <linux/time.h> #include <linux/completion.h> +#include <drm/audio_drm.h>
#ifdef CONFIG_X86 /* for snoop control */ @@ -2869,6 +2870,7 @@ static int azx_suspend(struct device *dev) pci_disable_device(pci); pci_save_state(pci); pci_set_power_state(pci, PCI_D3hot);
- release_power_well(); return 0;
}
@@ -2881,6 +2883,7 @@ static int azx_resume(struct device *dev) if (chip->disabled) return 0;
- request_power_well(); pci_set_power_state(pci, PCI_D0); pci_restore_state(pci); if (pci_enable_device(pci) < 0) { @@ -2913,6 +2916,7 @@ static
int azx_runtime_suspend(struct device *dev)
azx_stop_chip(chip); azx_clear_irq_pending(chip);
- release_power_well(); return 0;
}
@@ -2921,6 +2925,7 @@ static int azx_runtime_resume(struct device *dev) struct snd_card *card = dev_get_drvdata(dev); struct azx *chip = card->private_data;
- request_power_well(); azx_init_pci(chip); azx_init_chip(chip, 1); return 0;
@@ -3671,6 +3676,8 @@ static int azx_probe(struct pci_dev *pci, return -ENOENT; }
- request_power_well();
- err = snd_card_create(index[dev], id[dev], THIS_MODULE, 0, &card); if (err < 0) { snd_printk(KERN_ERR "hda-intel: Error creating
card!\n"); @@ -3806,6 +3813,7 @@ static void azx_remove(struct pci_dev *pci) if (card) snd_card_free(card); pci_set_drvdata(pci, NULL);
- release_power_well();
}
/* PCI IDs */
1.7.9.5
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Fri, Apr 26, 2013 at 8:58 AM, Li, Jocelyn jocelyn.li@intel.com wrote:
Hi Guys,
I apologize that I may have misunderstood on the agreement we have made on power well work. I assumed that audio team is expected to create private gfx driver API to control the power well between gfx and audio for internal releases. That's why I asked Xingchao work on that.
Yes, that's my understanding, too.
So the correct implementation approach between gfx and audio should be,
#1, Daniel will work on the existing private API in gfx driver for power well control that can be used by the audio driver to enable/disable the power well.
#2, Once #1 is done, Audio team will integrate Daniel's patch into audio driver and add/improve on anything that is missing
That sounds backwards to me. Imo first priority should be to get the audio driver to work with power well functionality enabled. And Xingchao's patch looks like it should be good enough to start with that on the audio side.
Once we've figured out what needs to be changed in the audio driver we can look at the entire patch series and the interface to i915.ko. That's also why I didn't comment on Xingchao's patch right away, but only once he specifically asked for feedback, since doing a real review of the interface doesn't make sense until we have all the pieces (and a working audio driver).
Yours, Daniel
Please correct me if my understanding is not correct.
Thanks, Jocelyn
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] Sent: Thursday, April 25, 2013 3:52 PM To: Wang, Xingchao Cc: Zanoni, Paulo R; ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; Li, Jocelyn; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Wang Xingchao; Takashi Iwai; Barnes, Jesse; Wysocki, Rafael J Subject: Re: [PATCH] drm/i915: Add private api for power well usage
On Thu, Apr 25, 2013 at 4:36 AM, Wang, Xingchao xingchao.wang@intel.com wrote:
Hi Daniel/Paulo,
Any comments on this? Add Jesse and Rafael in loop.
Well I've figured we want to get the power well integration working before we waste time reviewing an interface which might or might not quite work. But if you want, I can drop two quick comments:
- I think the functions needs some prefix like i915_
- Imo the power well should just be reference counted (the current interface name suggests it's generally useable, but actually can only cope with one request from the audio driver).
- Is the spin_lock (irq-disabling even) really required for the audio driver to work? power well enabling can take up to 20ms, so this won't work as-is. Can't we just use a mutex?
- I'd need to see the audio driver side of this to check how this works for coping with arbitrary module load ordering.
Cheers, Daniel
thanks --xingchao
-----Original Message----- From: Wang, Xingchao Sent: Wednesday, April 24, 2013 3:29 PM To: daniel.vetter@ffwll.ch; Zanoni, Paulo R; 'Takashi Iwai' Cc: ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; Li, Jocelyn; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; 'Wang Xingchao' Subject: RE: [PATCH] drm/i915: Add private api for power well usage
Hi Guys,
Sorry I should Add Takashi and alsa maillist in loop.
I tested this patch against Daniel's "drm-intel-next-queued" Branch, on Haswell Mobile C3 stepping machines.
Test steps:
- only connect one monitor through HDMI cable 2. echo 1 >
/sys/module/i915/parameters/disable_power_well ==> I think this could be set as 1 by default if applied my patch. 3. plug out HDMI cable 4. cat /proc/asound/card0/codec#0
At step 3, i915 will try to disable power well but rejected as display audio is using power well. Without the patch, display audio would crash at step 4.
I did not list suspend/resume test here, as during suspend sequence, i915 will _ENABLE_ power well, that's a BUG?
For me above test case is the only one to reproduce the issue, if you have more ideas and need more test, please let me know.
I'm afraid there's some risk because the dependency between alsa driver and drm driver. For example, if i915 module was not loaded, display audio Will call request_power_well() and meet symbol error. Even if i915 module loaded after snd-hda-intel module, there's such risk too. Any comment on this?
Another risk is the pointer "power_well_device", it should be set NULL when i915 module unloaded, I added the caller but have no idea where to call it, if anyone has suggestion, that would be fine.
thanks --xingchao
-----Original Message----- From: Wang Xingchao [mailto:xingchao.wang@linux.intel.com] Sent: Thursday, April 24, 2014 10:20 PM To: daniel.vetter@ffwll.ch; Zanoni, Paulo R Cc: ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; Li, Jocelyn; intel-gfx@lists.freedesktop.org; Wang, Xingchao; Wang Xingchao Subject: [PATCH] drm/i915: Add private api for power well usage
Display audio depends on power well in graphic side, it should request power well before use it and release power well after use. I915 will not shutdown power well if it detects audio is using. This patch protects display audio crash for Intel Haswell mobile C3 stepping board.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
drivers/gpu/drm/i915/intel_pm.c | 73 +++++++++++++++++++++++++++++++++++---- include/drm/audio_drm.h | 39 +++++++++++++++++++++ sound/pci/hda/hda_intel.c | 8 +++++ 3 files changed, 113 insertions(+), 7 deletions(-) create mode 100644 include/drm/audio_drm.h
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 2557926..9983f56 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4298,18 +4298,12 @@ bool intel_using_power_well(struct drm_device *dev) return true; }
-void intel_set_power_well(struct drm_device *dev, bool enable) +void set_power_well(struct drm_device *dev, bool enable) { struct drm_i915_private *dev_priv = dev->dev_private; bool is_enabled, enable_requested; uint32_t tmp;
- if (!HAS_POWER_WELL(dev))
return;- if (!i915_disable_power_well && !enable)
return;- tmp = I915_READ(HSW_PWR_WELL_DRIVER); is_enabled = tmp & HSW_PWR_WELL_STATE; enable_requested = tmp & HSW_PWR_WELL_ENABLE; @@ -4332,6
+4326,71 @@ void intel_set_power_well(struct drm_device *dev, bool +enable) } }
+/* Global drm_device for display audio drvier usage */ static +struct drm_device *power_well_device; static bool +audio_power_well_using, i915_power_well_using; +/* Lock protecting power well setting */ +DEFINE_SPINLOCK(powerwell_lock);
+void request_power_well(void) +{
- DRM_DEBUG_KMS("Display audio requesting power well\n");
- spin_lock_irq(&powerwell_lock);
- if (!power_well_device)
goto out;- if (i915_power_well_using == false)
set_power_well(power_well_device, true);- audio_power_well_using = true;
+out:
- spin_unlock_irq(&powerwell_lock);
- return;
+} +EXPORT_SYMBOL_GPL(request_power_well);
+void release_power_well(void) +{
- DRM_DEBUG_KMS("Display audio releasing power well\n");
- spin_lock_irq(&powerwell_lock);
- if (!power_well_device)
goto out;- if (i915_power_well_using == false)
set_power_well(power_well_device, false);- audio_power_well_using = false;
+out:
- spin_unlock_irq(&powerwell_lock);
- return;
+} +EXPORT_SYMBOL_GPL(release_power_well);
+/* TODO: Call this when i915 module unload */ void +remove_power_well(void) {
- spin_lock_irq(&powerwell_lock);
- audio_power_well_using = false;
- i915_power_well_using = false;
- power_well_device = NULL;
- spin_unlock_irq(&powerwell_lock); }
+void intel_set_power_well(struct drm_device *dev, bool enable) {
- if (!HAS_POWER_WELL(dev))
return;- spin_lock_irq(&powerwell_lock);
- power_well_device = dev;
- i915_power_well_using = enable;
- if (!enable && audio_power_well_using)
goto out;- if (!i915_disable_power_well && !enable)
goto out;- set_power_well(dev, enable);
+out:
- spin_unlock_irq(&powerwell_lock);
- return;
+}
/*
- Starting with Haswell, we have a "Power Down Well" that can be
turned
off
- when not needed anymore. We have 4 registers that can request
the power well diff --git a/include/drm/audio_drm.h b/include/drm/audio_drm.h new file mode 100644 index 0000000..4412b36 --- /dev/null +++ b/include/drm/audio_drm.h @@ -0,0 +1,39 @@
+/**************************************************************
+****
- Copyright 2013 Intel Inc.
- All Rights Reserved.
- Permission is hereby granted, free of charge, to any person
+obtaining a
- copy of this software and associated documentation files (the
- "Software"), to deal in the Software without restriction,
+including
- without limitation the rights to use, copy, modify, merge,
+publish,
- distribute, sub license, and/or sell copies of the Software,
+and to
- permit persons to whom the Software is furnished to do so,
+subject to
- the following conditions:
- The above copyright notice and this permission notice
+(including the
- next paragraph) shall be included in all copies or substantial
+portions
- of the Software.
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
KIND,
+EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO
EVENT +SHALL
- THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE
FOR +ANY CLAIM,
- DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
TORT +OR
- OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
SOFTWARE +OR THE
- USE OR OTHER DEALINGS IN THE SOFTWARE.
+***************************************************************
+***/ +/*
- Authors:
- Wang Xingchao xingchao.wang@intel.com */
+#ifndef _AUDIO_DRM_H_ +#define _AUDIO_DRM_H_
+extern void request_power_well(void); extern void +release_power_well(void);
+#endif /* _AUDIO_DRM_H_ */ diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 418bfc0..59a85de 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -50,6 +50,7 @@ #include <linux/clocksource.h> #include <linux/time.h> #include <linux/completion.h> +#include <drm/audio_drm.h>
#ifdef CONFIG_X86 /* for snoop control */ @@ -2869,6 +2870,7 @@ static int azx_suspend(struct device *dev) pci_disable_device(pci); pci_save_state(pci); pci_set_power_state(pci, PCI_D3hot);
- release_power_well(); return 0;
}
@@ -2881,6 +2883,7 @@ static int azx_resume(struct device *dev) if (chip->disabled) return 0;
- request_power_well(); pci_set_power_state(pci, PCI_D0); pci_restore_state(pci); if (pci_enable_device(pci) < 0) { @@ -2913,6 +2916,7 @@ static
int azx_runtime_suspend(struct device *dev)
azx_stop_chip(chip); azx_clear_irq_pending(chip);
- release_power_well(); return 0;
}
@@ -2921,6 +2925,7 @@ static int azx_runtime_resume(struct device *dev) struct snd_card *card = dev_get_drvdata(dev); struct azx *chip = card->private_data;
- request_power_well(); azx_init_pci(chip); azx_init_chip(chip, 1); return 0;
@@ -3671,6 +3676,8 @@ static int azx_probe(struct pci_dev *pci, return -ENOENT; }
- request_power_well();
- err = snd_card_create(index[dev], id[dev], THIS_MODULE, 0, &card); if (err < 0) { snd_printk(KERN_ERR "hda-intel: Error creating
card!\n"); @@ -3806,6 +3813,7 @@ static void azx_remove(struct pci_dev *pci) if (card) snd_card_free(card); pci_set_drvdata(pci, NULL);
- release_power_well();
}
/* PCI IDs */
1.7.9.5
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Hi Daniel,
Please see my comments below.
Regards, Jocelyn
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] Sent: Friday, April 26, 2013 3:25 PM To: Li, Jocelyn Cc: Wang, Xingchao; Zanoni, Paulo R; ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Wang Xingchao; Takashi Iwai; Barnes, Jesse; Wysocki, Rafael J; Hindman, Gavin Subject: Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team
On Fri, Apr 26, 2013 at 8:58 AM, Li, Jocelyn jocelyn.li@intel.com wrote:
Hi Guys,
I apologize that I may have misunderstood on the agreement we have made on power well work. I assumed that audio team is expected to create private gfx driver API to control the power well between gfx and audio for internal releases. That's why I asked Xingchao work on that.
Yes, that's my understanding, too.
So the correct implementation approach between gfx and audio should be,
#1, Daniel will work on the existing private API in gfx driver for power well control that can be used by the audio driver to enable/disable the power well.
#2, Once #1 is done, Audio team will integrate Daniel's patch into audio driver and add/improve on anything that is missing
That sounds backwards to me. Imo first priority should be to get the audio driver to work with power well functionality enabled. And Xingchao's patch looks like it should be good enough to start with that on the audio side.
Once we've figured out what needs to be changed in the audio driver we can look at the entire patch series and the interface to i915.ko. That's also why I didn't comment on Xingchao's patch right away, but only once he specifically asked for feedback, since doing a real review of the interface doesn't make sense until we have all the pieces (and a working audio driver).
[Jocelyn] I think you have made constructive comments on Xingchao's patch yesterday. Next, shall we have Xingchao improve his patch? Or we just have Xingchao wait till you have completed your pieces. Sorry, I am a little confused :)
Yours, Daniel
Please correct me if my understanding is not correct.
Thanks, Jocelyn
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] Sent: Thursday, April 25, 2013 3:52 PM To: Wang, Xingchao Cc: Zanoni, Paulo R; ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; Li, Jocelyn; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Wang Xingchao; Takashi Iwai; Barnes, Jesse; Wysocki, Rafael J Subject: Re: [PATCH] drm/i915: Add private api for power well usage
On Thu, Apr 25, 2013 at 4:36 AM, Wang, Xingchao xingchao.wang@intel.com wrote:
Hi Daniel/Paulo,
Any comments on this? Add Jesse and Rafael in loop.
Well I've figured we want to get the power well integration working before we waste time reviewing an interface which might or might not quite work. But if you want, I can drop two quick comments:
- I think the functions needs some prefix like i915_
- Imo the power well should just be reference counted (the current interface name suggests it's generally useable, but actually can only cope with one request from the audio driver).
- Is the spin_lock (irq-disabling even) really required for the audio driver to work? power well enabling can take up to 20ms, so this won't work as-is. Can't we just use a mutex?
- I'd need to see the audio driver side of this to check how this works for coping with arbitrary module load ordering.
Cheers, Daniel
thanks --xingchao
-----Original Message----- From: Wang, Xingchao Sent: Wednesday, April 24, 2013 3:29 PM To: daniel.vetter@ffwll.ch; Zanoni, Paulo R; 'Takashi Iwai' Cc: ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; Li, Jocelyn; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; 'Wang Xingchao' Subject: RE: [PATCH] drm/i915: Add private api for power well usage
Hi Guys,
Sorry I should Add Takashi and alsa maillist in loop.
I tested this patch against Daniel's "drm-intel-next-queued" Branch, on Haswell Mobile C3 stepping machines.
Test steps:
- only connect one monitor through HDMI cable 2. echo 1 >
/sys/module/i915/parameters/disable_power_well ==> I think this could be set as 1 by default if applied my patch. 3. plug out HDMI cable 4. cat /proc/asound/card0/codec#0
At step 3, i915 will try to disable power well but rejected as display audio is using power well. Without the patch, display audio would crash at step 4.
I did not list suspend/resume test here, as during suspend sequence, i915 will _ENABLE_ power well, that's a BUG?
For me above test case is the only one to reproduce the issue, if you have more ideas and need more test, please let me know.
I'm afraid there's some risk because the dependency between alsa driver and drm driver. For example, if i915 module was not loaded, display audio Will call request_power_well() and meet symbol error. Even if i915 module loaded after snd-hda-intel module, there's such risk too. Any comment on this?
Another risk is the pointer "power_well_device", it should be set NULL when i915 module unloaded, I added the caller but have no idea where to call it, if anyone has suggestion, that would be fine.
thanks --xingchao
-----Original Message----- From: Wang Xingchao [mailto:xingchao.wang@linux.intel.com] Sent: Thursday, April 24, 2014 10:20 PM To: daniel.vetter@ffwll.ch; Zanoni, Paulo R Cc: ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; Li, Jocelyn; intel-gfx@lists.freedesktop.org; Wang, Xingchao; Wang Xingchao Subject: [PATCH] drm/i915: Add private api for power well usage
Display audio depends on power well in graphic side, it should request power well before use it and release power well after use. I915 will not shutdown power well if it detects audio is using. This patch protects display audio crash for Intel Haswell mobile C3 stepping board.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
drivers/gpu/drm/i915/intel_pm.c | 73 +++++++++++++++++++++++++++++++++++---- include/drm/audio_drm.h | 39 +++++++++++++++++++++ sound/pci/hda/hda_intel.c | 8 +++++ 3 files changed, 113 insertions(+), 7 deletions(-) create mode 100644 include/drm/audio_drm.h
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 2557926..9983f56 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4298,18 +4298,12 @@ bool intel_using_power_well(struct drm_device *dev) return true; }
-void intel_set_power_well(struct drm_device *dev, bool enable) +void set_power_well(struct drm_device *dev, bool enable) { struct drm_i915_private *dev_priv = dev->dev_private; bool is_enabled, enable_requested; uint32_t tmp;
- if (!HAS_POWER_WELL(dev))
return;- if (!i915_disable_power_well && !enable)
return;- tmp = I915_READ(HSW_PWR_WELL_DRIVER); is_enabled = tmp & HSW_PWR_WELL_STATE; enable_requested = tmp & HSW_PWR_WELL_ENABLE; @@ -4332,6
+4326,71 @@ void intel_set_power_well(struct drm_device *dev, bool +enable) } }
+/* Global drm_device for display audio drvier usage */ static +struct drm_device *power_well_device; static bool +audio_power_well_using, i915_power_well_using; +/* Lock protecting power well setting */ +DEFINE_SPINLOCK(powerwell_lock);
+void request_power_well(void) +{
- DRM_DEBUG_KMS("Display audio requesting power well\n");
- spin_lock_irq(&powerwell_lock);
- if (!power_well_device)
goto out;- if (i915_power_well_using == false)
set_power_well(power_well_device, true);- audio_power_well_using = true;
+out:
- spin_unlock_irq(&powerwell_lock);
- return;
+} +EXPORT_SYMBOL_GPL(request_power_well);
+void release_power_well(void) +{
- DRM_DEBUG_KMS("Display audio releasing power well\n");
- spin_lock_irq(&powerwell_lock);
- if (!power_well_device)
goto out;- if (i915_power_well_using == false)
set_power_well(power_well_device, false);- audio_power_well_using = false;
+out:
- spin_unlock_irq(&powerwell_lock);
- return;
+} +EXPORT_SYMBOL_GPL(release_power_well);
+/* TODO: Call this when i915 module unload */ void +remove_power_well(void) {
- spin_lock_irq(&powerwell_lock);
- audio_power_well_using = false;
- i915_power_well_using = false;
- power_well_device = NULL;
- spin_unlock_irq(&powerwell_lock); }
+void intel_set_power_well(struct drm_device *dev, bool enable) {
- if (!HAS_POWER_WELL(dev))
return;- spin_lock_irq(&powerwell_lock);
- power_well_device = dev;
- i915_power_well_using = enable;
- if (!enable && audio_power_well_using)
goto out;- if (!i915_disable_power_well && !enable)
goto out;- set_power_well(dev, enable);
+out:
- spin_unlock_irq(&powerwell_lock);
- return;
+}
/*
- Starting with Haswell, we have a "Power Down Well" that can be
turned
off
- when not needed anymore. We have 4 registers that can request
the power well diff --git a/include/drm/audio_drm.h b/include/drm/audio_drm.h new file mode 100644 index 0000000..4412b36 --- /dev/null +++ b/include/drm/audio_drm.h @@ -0,0 +1,39 @@
+/**************************************************************
+****
- Copyright 2013 Intel Inc.
- All Rights Reserved.
- Permission is hereby granted, free of charge, to any person
+obtaining a
- copy of this software and associated documentation files (the
- "Software"), to deal in the Software without restriction,
+including
- without limitation the rights to use, copy, modify, merge,
+publish,
- distribute, sub license, and/or sell copies of the Software,
+and to
- permit persons to whom the Software is furnished to do so,
+subject to
- the following conditions:
- The above copyright notice and this permission notice
+(including the
- next paragraph) shall be included in all copies or substantial
+portions
- of the Software.
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
KIND,
+EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO
EVENT +SHALL
- THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE
FOR +ANY CLAIM,
- DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
TORT +OR
- OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
SOFTWARE +OR THE
- USE OR OTHER DEALINGS IN THE SOFTWARE.
+***************************************************************
+***/ +/*
- Authors:
- Wang Xingchao xingchao.wang@intel.com */
+#ifndef _AUDIO_DRM_H_ +#define _AUDIO_DRM_H_
+extern void request_power_well(void); extern void +release_power_well(void);
+#endif /* _AUDIO_DRM_H_ */ diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 418bfc0..59a85de 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -50,6 +50,7 @@ #include <linux/clocksource.h> #include <linux/time.h> #include <linux/completion.h> +#include <drm/audio_drm.h>
#ifdef CONFIG_X86 /* for snoop control */ @@ -2869,6 +2870,7 @@ static int azx_suspend(struct device *dev) pci_disable_device(pci); pci_save_state(pci); pci_set_power_state(pci, PCI_D3hot);
- release_power_well(); return 0;
}
@@ -2881,6 +2883,7 @@ static int azx_resume(struct device *dev) if (chip->disabled) return 0;
- request_power_well(); pci_set_power_state(pci, PCI_D0); pci_restore_state(pci); if (pci_enable_device(pci) < 0) { @@ -2913,6 +2916,7 @@ static
int azx_runtime_suspend(struct device *dev)
azx_stop_chip(chip); azx_clear_irq_pending(chip);
- release_power_well(); return 0;
}
@@ -2921,6 +2925,7 @@ static int azx_runtime_resume(struct device *dev) struct snd_card *card = dev_get_drvdata(dev); struct azx *chip = card->private_data;
- request_power_well(); azx_init_pci(chip); azx_init_chip(chip, 1); return 0;
@@ -3671,6 +3676,8 @@ static int azx_probe(struct pci_dev *pci, return -ENOENT; }
- request_power_well();
- err = snd_card_create(index[dev], id[dev], THIS_MODULE, 0, &card); if (err < 0) { snd_printk(KERN_ERR "hda-intel: Error creating
card!\n"); @@ -3806,6 +3813,7 @@ static void azx_remove(struct pci_dev *pci) if (card) snd_card_free(card); pci_set_drvdata(pci, NULL);
- release_power_well();
}
/* PCI IDs */
1.7.9.5
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Fri, Apr 26, 2013 at 07:53:41AM +0000, Li, Jocelyn wrote:
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] Sent: Friday, April 26, 2013 3:25 PM To: Li, Jocelyn Cc: Wang, Xingchao; Zanoni, Paulo R; ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Wang Xingchao; Takashi Iwai; Barnes, Jesse; Wysocki, Rafael J; Hindman, Gavin Subject: Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team Once we've figured out what needs to be changed in the audio driver we can look at the entire patch series and the interface to i915.ko. That's also why I didn't comment on Xingchao's patch right away, but only once he specifically asked for feedback, since doing a real review of the interface doesn't make sense until we have all the pieces (and a working audio driver).
[Jocelyn] I think you have made constructive comments on Xingchao's patch yesterday. Next, shall we have Xingchao improve his patch? Or we just have Xingchao wait till you have completed your pieces. Sorry, I am a little confused :)
I think the next step is to use Xinchao's patch as-is and get the audio side going. Once we have that fixed up, we can revisit the interface and make it solid. But for now trying to polish this relatively simple part seems like wasted time.
Also, reviewing an interface is much easier if we have both the i915 pieces (already here) and the audio pieces (which I haven't seen yet) avaialble.
Cheers, Daniel
At Fri, 26 Apr 2013 16:57:08 +0200, Daniel Vetter wrote:
On Fri, Apr 26, 2013 at 07:53:41AM +0000, Li, Jocelyn wrote:
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] Sent: Friday, April 26, 2013 3:25 PM To: Li, Jocelyn Cc: Wang, Xingchao; Zanoni, Paulo R; ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Wang Xingchao; Takashi Iwai; Barnes, Jesse; Wysocki, Rafael J; Hindman, Gavin Subject: Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team Once we've figured out what needs to be changed in the audio driver we can look at the entire patch series and the interface to i915.ko. That's also why I didn't comment on Xingchao's patch right away, but only once he specifically asked for feedback, since doing a real review of the interface doesn't make sense until we have all the pieces (and a working audio driver).
[Jocelyn] I think you have made constructive comments on Xingchao's patch yesterday. Next, shall we have Xingchao improve his patch? Or we just have Xingchao wait till you have completed your pieces. Sorry, I am a little confused :)
I think the next step is to use Xinchao's patch as-is and get the audio side going. Once we have that fixed up, we can revisit the interface and make it solid. But for now trying to polish this relatively simple part seems like wasted time.
Also, reviewing an interface is much easier if we have both the i915 pieces (already here) and the audio pieces (which I haven't seen yet) avaialble.
I haven't checked the patch properly yet, but the patch pasted in the post looks like i915 driver exports the functions to control power well, and let audio driver calling them. If so, the big mess in such a case is the dependency between driver modules.
A simple workaround would be to split this function and the relevant instance out of i915 and snd-hda-intel and put into an individual module (e.g. i915-powerwell-ctl).
Also, it would be feasible to implement some PM governor over both graphics and audio, that is, it gives the proper serialization of power up for audio controller, for example.
thanks,
Takashi
On Fri, Apr 26, 2013 at 05:12:34PM +0200, Takashi Iwai wrote:
At Fri, 26 Apr 2013 16:57:08 +0200, Daniel Vetter wrote:
On Fri, Apr 26, 2013 at 07:53:41AM +0000, Li, Jocelyn wrote:
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] Sent: Friday, April 26, 2013 3:25 PM To: Li, Jocelyn Cc: Wang, Xingchao; Zanoni, Paulo R; ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Wang Xingchao; Takashi Iwai; Barnes, Jesse; Wysocki, Rafael J; Hindman, Gavin Subject: Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team Once we've figured out what needs to be changed in the audio driver we can look at the entire patch series and the interface to i915.ko. That's also why I didn't comment on Xingchao's patch right away, but only once he specifically asked for feedback, since doing a real review of the interface doesn't make sense until we have all the pieces (and a working audio driver).
[Jocelyn] I think you have made constructive comments on Xingchao's patch yesterday. Next, shall we have Xingchao improve his patch? Or we just have Xingchao wait till you have completed your pieces. Sorry, I am a little confused :)
I think the next step is to use Xinchao's patch as-is and get the audio side going. Once we have that fixed up, we can revisit the interface and make it solid. But for now trying to polish this relatively simple part seems like wasted time.
Also, reviewing an interface is much easier if we have both the i915 pieces (already here) and the audio pieces (which I haven't seen yet) avaialble.
I haven't checked the patch properly yet, but the patch pasted in the post looks like i915 driver exports the functions to control power well, and let audio driver calling them. If so, the big mess in such a case is the dependency between driver modules.
A simple workaround would be to split this function and the relevant instance out of i915 and snd-hda-intel and put into an individual module (e.g. i915-powerwell-ctl).
Yeah, the current patch doesn't work for loading i915/snd-hda-intel in any order. But it should be good enough to fix the hw interactions.
Also, it would be feasible to implement some PM governor over both graphics and audio, that is, it gives the proper serialization of power up for audio controller, for example.
Yeah, I think once we have the hw interaction/sequence and stuff all figured out we need to figure out what kind of interface would suit best here. One of the options we've talked about a bit is a full runtime PM governor on a special device. Then only the device needs to be instantiated first, the audio driver could just grab runtime pm references and i915 could implement the PM backend.
But like I've said, those considerations should be a second step once we have something working with a quickly hacked-together interface. -Daniel
At Fri, 26 Apr 2013 17:42:07 +0200, Daniel Vetter wrote:
On Fri, Apr 26, 2013 at 05:12:34PM +0200, Takashi Iwai wrote:
At Fri, 26 Apr 2013 16:57:08 +0200, Daniel Vetter wrote:
On Fri, Apr 26, 2013 at 07:53:41AM +0000, Li, Jocelyn wrote:
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] Sent: Friday, April 26, 2013 3:25 PM To: Li, Jocelyn Cc: Wang, Xingchao; Zanoni, Paulo R; ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Wang Xingchao; Takashi Iwai; Barnes, Jesse; Wysocki, Rafael J; Hindman, Gavin Subject: Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team Once we've figured out what needs to be changed in the audio driver we can look at the entire patch series and the interface to i915.ko. That's also why I didn't comment on Xingchao's patch right away, but only once he specifically asked for feedback, since doing a real review of the interface doesn't make sense until we have all the pieces (and a working audio driver).
[Jocelyn] I think you have made constructive comments on Xingchao's patch yesterday. Next, shall we have Xingchao improve his patch? Or we just have Xingchao wait till you have completed your pieces. Sorry, I am a little confused :)
I think the next step is to use Xinchao's patch as-is and get the audio side going. Once we have that fixed up, we can revisit the interface and make it solid. But for now trying to polish this relatively simple part seems like wasted time.
Also, reviewing an interface is much easier if we have both the i915 pieces (already here) and the audio pieces (which I haven't seen yet) avaialble.
I haven't checked the patch properly yet, but the patch pasted in the post looks like i915 driver exports the functions to control power well, and let audio driver calling them. If so, the big mess in such a case is the dependency between driver modules.
A simple workaround would be to split this function and the relevant instance out of i915 and snd-hda-intel and put into an individual module (e.g. i915-powerwell-ctl).
Yeah, the current patch doesn't work for loading i915/snd-hda-intel in any order. But it should be good enough to fix the hw interactions.
Well, my current concern is that the call is built into snd-hda-intel module and this will lead to load i915 module no matter whether it's used or not. snd-hda-intel is used for any HD-audio controllers (despite its name). Loading i915 on a pure AMD system will annoy some non-Intel people :)
Even if we built the code via ifdef CONFIG_DRM_I915 or such, it's still hardcoded, thus the problem above will occur with distro kernels.
Also, it would be feasible to implement some PM governor over both graphics and audio, that is, it gives the proper serialization of power up for audio controller, for example.
Yeah, I think once we have the hw interaction/sequence and stuff all figured out we need to figure out what kind of interface would suit best here. One of the options we've talked about a bit is a full runtime PM governor on a special device. Then only the device needs to be instantiated first, the audio driver could just grab runtime pm references and i915 could implement the PM backend.
But like I've said, those considerations should be a second step once we have something working with a quickly hacked-together interface.
Yep, I agree that we'll need a quick fix at this stage.
thanks,
Takashi
On Fri, Apr 26, 2013 at 05:45:15PM +0200, Takashi Iwai wrote:
At Fri, 26 Apr 2013 17:42:07 +0200, Daniel Vetter wrote:
On Fri, Apr 26, 2013 at 05:12:34PM +0200, Takashi Iwai wrote:
At Fri, 26 Apr 2013 16:57:08 +0200, Daniel Vetter wrote:
On Fri, Apr 26, 2013 at 07:53:41AM +0000, Li, Jocelyn wrote:
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] Sent: Friday, April 26, 2013 3:25 PM To: Li, Jocelyn Cc: Wang, Xingchao; Zanoni, Paulo R; ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Wang Xingchao; Takashi Iwai; Barnes, Jesse; Wysocki, Rafael J; Hindman, Gavin Subject: Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team Once we've figured out what needs to be changed in the audio driver we can look at the entire patch series and the interface to i915.ko. That's also why I didn't comment on Xingchao's patch right away, but only once he specifically asked for feedback, since doing a real review of the interface doesn't make sense until we have all the pieces (and a working audio driver).
[Jocelyn] I think you have made constructive comments on Xingchao's patch yesterday. Next, shall we have Xingchao improve his patch? Or we just have Xingchao wait till you have completed your pieces. Sorry, I am a little confused :)
I think the next step is to use Xinchao's patch as-is and get the audio side going. Once we have that fixed up, we can revisit the interface and make it solid. But for now trying to polish this relatively simple part seems like wasted time.
Also, reviewing an interface is much easier if we have both the i915 pieces (already here) and the audio pieces (which I haven't seen yet) avaialble.
I haven't checked the patch properly yet, but the patch pasted in the post looks like i915 driver exports the functions to control power well, and let audio driver calling them. If so, the big mess in such a case is the dependency between driver modules.
A simple workaround would be to split this function and the relevant instance out of i915 and snd-hda-intel and put into an individual module (e.g. i915-powerwell-ctl).
Yeah, the current patch doesn't work for loading i915/snd-hda-intel in any order. But it should be good enough to fix the hw interactions.
Well, my current concern is that the call is built into snd-hda-intel module and this will lead to load i915 module no matter whether it's used or not. snd-hda-intel is used for any HD-audio controllers (despite its name). Loading i915 on a pure AMD system will annoy some non-Intel people :)
Even if we built the code via ifdef CONFIG_DRM_I915 or such, it's still hardcoded, thus the problem above will occur with distro kernels.
Sure, the current patch won't cut it for upstreaming. But it should be good enough to figure out what exactly we need (which to still be a bit unclear). Or at least where exactly we need it and what kind of other changes in snd-hda-intel are required to get things going.
Also, it would be feasible to implement some PM governor over both graphics and audio, that is, it gives the proper serialization of power up for audio controller, for example.
Yeah, I think once we have the hw interaction/sequence and stuff all figured out we need to figure out what kind of interface would suit best here. One of the options we've talked about a bit is a full runtime PM governor on a special device. Then only the device needs to be instantiated first, the audio driver could just grab runtime pm references and i915 could implement the PM backend.
But like I've said, those considerations should be a second step once we have something working with a quickly hacked-together interface.
Yep, I agree that we'll need a quick fix at this stage.
power well stuff is disabled by default, 3.10 is kinda done already and we still have plenty of time to hit anything for 3.11. So I don't think we should rush the solution for upstream before it's clear what snd-hda-intel precisely needs. -Daniel
Hi Daniel/Takashi,
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Saturday, April 27, 2013 1:18 AM To: Takashi Iwai Cc: Daniel Vetter; Li, Jocelyn; Daniel Vetter; Wang, Xingchao; Zanoni, Paulo R; ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Wang Xingchao; Barnes, Jesse; Wysocki, Rafael J; Hindman, Gavin Subject: Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team
On Fri, Apr 26, 2013 at 05:45:15PM +0200, Takashi Iwai wrote:
At Fri, 26 Apr 2013 17:42:07 +0200, Daniel Vetter wrote:
On Fri, Apr 26, 2013 at 05:12:34PM +0200, Takashi Iwai wrote:
At Fri, 26 Apr 2013 16:57:08 +0200, Daniel Vetter wrote:
On Fri, Apr 26, 2013 at 07:53:41AM +0000, Li, Jocelyn wrote:
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] Sent: Friday, April 26, 2013 3:25 PM To: Li, Jocelyn Cc: Wang, Xingchao; Zanoni, Paulo R; ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Wang Xingchao; Takashi Iwai; Barnes, Jesse; Wysocki, Rafael J; Hindman, Gavin Subject: Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team Once we've figured out what needs to be changed in the audio driver we can
look at the entire patch series and the interface to i915.ko.
That's also why I didn't comment on Xingchao's patch right away, but only once he specifically asked for feedback, since doing a real review of the interface doesn't make sense until we have all the pieces (and a working audio driver).
[Jocelyn] I think you have made constructive comments on Xingchao's patch yesterday. Next, shall we have Xingchao improve his patch? Or we just have Xingchao wait till you have completed your pieces. Sorry, I am a little confused :)
I think the next step is to use Xinchao's patch as-is and get the audio side going. Once we have that fixed up, we can revisit the interface and make it solid. But for now trying to polish this relatively simple part seems like wasted time.
Also, reviewing an interface is much easier if we have both the i915 pieces (already here) and the audio pieces (which I haven't seen yet) avaialble.
I haven't checked the patch properly yet, but the patch pasted in the post looks like i915 driver exports the functions to control power well, and let audio driver calling them. If so, the big mess in such a case is the dependency between driver modules.
A simple workaround would be to split this function and the relevant instance out of i915 and snd-hda-intel and put into an individual module (e.g. i915-powerwell-ctl).
Yeah, the current patch doesn't work for loading i915/snd-hda-intel in any order. But it should be good enough to fix the hw interactions.
Well, my current concern is that the call is built into snd-hda-intel module and this will lead to load i915 module no matter whether it's used or not. snd-hda-intel is used for any HD-audio controllers (despite its name). Loading i915 on a pure AMD system will annoy some non-Intel people :)
Even if we built the code via ifdef CONFIG_DRM_I915 or such, it's still hardcoded, thus the problem above will occur with distro kernels.
Sure, the current patch won't cut it for upstreaming. But it should be good enough to figure out what exactly we need (which to still be a bit unclear). Or at least where exactly we need it and what kind of other changes in snd-hda-intel are required to get things going.
Let me throw a basic proposal on Audio driver side, please give your comments freely.
it contains the power well control usage points: #1: audio request power well at boot up. I915 may shut down power well after bootup initialization, as there's no monitor connected outside or only eDP on pipe A. #2: audio request power on resume After exit from D3 mode, audio driver quest power on. This may happen at normal resume or runtime resume. #3: audio release power well control at suspend Audio driver will let i915 know it doensot need power well anymore as it's going to suspend. This may happened at normal suspend or runtime suspend point. #4: audio release power well when module unload Audio release power well at remove callback to let i915 know.
1/2 should have API like i915_request_power_well(). 3/4 should be i915_release_power_well().
Also there're some dependencies: #1: I915 driver not loaded yet, it will be loaded soon. This may happen at bootup, the audio driver module was initialized at first. We may follow Takashi's suggestion to move things into single module. #2: i915 driver not loaded. It's a BUG obviourly, audio drvier will not work in this case. The API should do nothing and should be no error output. #3: i915 suspend early and resume later This brings another dependency. Audio driver maybe the first user to request power on or the last user to release power well, power well should be enabled/disabled by audio actively? Is it okay to gave audio driver an chance to open/close power well?
Thanks --xingchao
Also, it would be feasible to implement some PM governor over both graphics and audio, that is, it gives the proper serialization of power up for audio controller, for example.
Yeah, I think once we have the hw interaction/sequence and stuff all figured out we need to figure out what kind of interface would suit best here. One of the options we've talked about a bit is a full runtime PM governor on a special device. Then only the device needs to be instantiated first, the audio driver could just grab runtime pm references and i915 could implement the PM backend.
But like I've said, those considerations should be a second step once we have something working with a quickly hacked-together interface.
Yep, I agree that we'll need a quick fix at this stage.
power well stuff is disabled by default, 3.10 is kinda done already and we still have plenty of time to hit anything for 3.11. So I don't think we should rush the solution for upstream before it's clear what snd-hda-intel precisely needs.
-Daniel
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Sat, Apr 27, 2013 at 09:20:39AM +0000, Wang, Xingchao wrote:
Let me throw a basic proposal on Audio driver side, please give your comments freely.
it contains the power well control usage points: #1: audio request power well at boot up. I915 may shut down power well after bootup initialization, as there's no monitor connected outside or only eDP on pipe A. #2: audio request power on resume After exit from D3 mode, audio driver quest power on. This may happen at normal resume or runtime resume. #3: audio release power well control at suspend Audio driver will let i915 know it doensot need power well anymore as it's going to suspend. This may happened at normal suspend or runtime suspend point. #4: audio release power well when module unload Audio release power well at remove callback to let i915 know.
I miss the power well grab/dropping at runtime from the audio side. If the audio driver forces the power well to be on the entire time it's loaded, that's not good, since the power well stuff is very much for runtime PM. We _must_ be able to switch off the power well whenever possible. -Daniel
On Sat, 27 Apr 2013 13:35:29 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Sat, Apr 27, 2013 at 09:20:39AM +0000, Wang, Xingchao wrote:
Let me throw a basic proposal on Audio driver side, please give your comments freely.
it contains the power well control usage points: #1: audio request power well at boot up. I915 may shut down power well after bootup initialization, as there's no monitor connected outside or only eDP on pipe A. #2: audio request power on resume After exit from D3 mode, audio driver quest power on. This may happen at normal resume or runtime resume. #3: audio release power well control at suspend Audio driver will let i915 know it doensot need power well anymore as it's going to suspend. This may happened at normal suspend or runtime suspend point. #4: audio release power well when module unload Audio release power well at remove callback to let i915 know.
I miss the power well grab/dropping at runtime from the audio side. If the audio driver forces the power well to be on the entire time it's loaded, that's not good, since the power well stuff is very much for runtime PM. We _must_ be able to switch off the power well whenever possible.
Xingchao, I'm not an audio developer so I'm probably way off.
But what we really need is a very small and targeted set of calls into the i915 driver from say the HDMI driver in HDA. It looks like the prepare/cleanup pair in the pcm_ops structure might be the right place to put things? If that's too fine grained, you could do it at open/close time I guess, but the danger there is that some app will keep the device open even while it's not playing.
If that won't work, maybe calling i915 from hda_power_work in the higher level code would be better?
For detecting whether to call i915 at all, you can filter on the PCI IDs (just look for an Intel graphics device and if present, try to get the i915 symbols for the power functions).
--- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -3860,6 +3860,8 @@ static unsigned int hda_call_codec_suspend(struct hda_code codec->patch_ops.suspend(codec); hda_cleanup_all_streams(codec); state = hda_set_power_state(codec, AC_PWRST_D3); + if (i915_shared_power_well) + i915_put_power_well(codec->i915_data); /* Cancel delayed work if we aren't currently running from it. */ if (!in_wq) cancel_delayed_work_sync(&codec->power_work); @@ -4807,6 +4809,9 @@ static void __snd_hda_power_up(struct hda_codec *codec, bo return; spin_unlock(&codec->power_lock);
+ if (i915_shared_power_well) + i915_get_power_well(codec->i915_data); + cancel_delayed_work_sync(&codec->power_work);
spin_lock(&codec->power_lock);
With some code at init time to get the i915 symbols you need to call and whether or not the shared power well is present...
Takashi, any other ideas?
The high level goal here should be for the audio driver to call into i915 with get/put power well around the sequences where it needs the power to be up (reading/writing registers, playing audio), but not across the whole time the driver is loaded, just like you already do with the powersave work functions, e.g. hda_call_codec_suspend.
Jesse
On 04/29/2013 05:02 PM, Jesse Barnes wrote:
On Sat, 27 Apr 2013 13:35:29 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Sat, Apr 27, 2013 at 09:20:39AM +0000, Wang, Xingchao wrote:
Let me throw a basic proposal on Audio driver side, please give your comments freely.
it contains the power well control usage points: #1: audio request power well at boot up. I915 may shut down power well after bootup initialization, as there's no monitor connected outside or only eDP on pipe A. #2: audio request power on resume After exit from D3 mode, audio driver quest power on. This may happen at normal resume or runtime resume. #3: audio release power well control at suspend Audio driver will let i915 know it doensot need power well anymore as it's going to suspend. This may happened at normal suspend or runtime suspend point. #4: audio release power well when module unload Audio release power well at remove callback to let i915 know.
I miss the power well grab/dropping at runtime from the audio side. If the audio driver forces the power well to be on the entire time it's loaded, that's not good, since the power well stuff is very much for runtime PM. We _must_ be able to switch off the power well whenever possible.
Xingchao, I'm not an audio developer so I'm probably way off.
But what we really need is a very small and targeted set of calls into the i915 driver from say the HDMI driver in HDA. It looks like the prepare/cleanup pair in the pcm_ops structure might be the right place to put things? If that's too fine grained, you could do it at open/close time I guess, but the danger there is that some app will keep the device open even while it's not playing.
If that won't work, maybe calling i915 from hda_power_work in the higher level code would be better?
For detecting whether to call i915 at all, you can filter on the PCI IDs (just look for an Intel graphics device and if present, try to get the i915 symbols for the power functions).
--- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -3860,6 +3860,8 @@ static unsigned int hda_call_codec_suspend(struct hda_code codec->patch_ops.suspend(codec); hda_cleanup_all_streams(codec); state = hda_set_power_state(codec, AC_PWRST_D3);
if (i915_shared_power_well)i915_put_power_well(codec->i915_data); /* Cancel delayed work if we aren't currently running from it. */ if (!in_wq) cancel_delayed_work_sync(&codec->power_work);@@ -4807,6 +4809,9 @@ static void __snd_hda_power_up(struct hda_codec *codec, bo return; spin_unlock(&codec->power_lock);
if (i915_shared_power_well)i915_get_power_well(codec->i915_data);
Is it wise that a _get function actually has side effects? Perhaps _push and _pop or something else would be better semantics.
cancel_delayed_work_sync(&codec->power_work); spin_lock(&codec->power_lock);With some code at init time to get the i915 symbols you need to call and whether or not the shared power well is present...
Takashi, any other ideas?
The high level goal here should be for the audio driver to call into i915 with get/put power well around the sequences where it needs the power to be up (reading/writing registers, playing audio), but not across the whole time the driver is loaded, just like you already do with the powersave work functions, e.g. hda_call_codec_suspend.
I think this sounds about right. The question is how to avoid a dependency on the i915 driver when it's not necessary, such as when the HDMI codec is AMD or Nvidia.
The most obvious way to me seems to be to create a new snd-hda-codec-hdmi-haswell module (that depends on both i915 and snd-hda-codec-hdmi), and then let that call into i915 and codec-hdmi drivers as necessary, e g using the set_power_state callback for the i915 stuff.
But maybe there's something smarter to do here, as I'm not experienced in mending kernel pieces together :-)
On Tue, 30 Apr 2013 12:29:37 +0200 David Henningsson david.henningsson@canonical.com wrote:
On 04/29/2013 05:02 PM, Jesse Barnes wrote:
On Sat, 27 Apr 2013 13:35:29 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Sat, Apr 27, 2013 at 09:20:39AM +0000, Wang, Xingchao wrote:
Let me throw a basic proposal on Audio driver side, please give your comments freely.
it contains the power well control usage points: #1: audio request power well at boot up. I915 may shut down power well after bootup initialization, as there's no monitor connected outside or only eDP on pipe A. #2: audio request power on resume After exit from D3 mode, audio driver quest power on. This may happen at normal resume or runtime resume. #3: audio release power well control at suspend Audio driver will let i915 know it doensot need power well anymore as it's going to suspend. This may happened at normal suspend or runtime suspend point. #4: audio release power well when module unload Audio release power well at remove callback to let i915 know.
I miss the power well grab/dropping at runtime from the audio side. If the audio driver forces the power well to be on the entire time it's loaded, that's not good, since the power well stuff is very much for runtime PM. We _must_ be able to switch off the power well whenever possible.
Xingchao, I'm not an audio developer so I'm probably way off.
But what we really need is a very small and targeted set of calls into the i915 driver from say the HDMI driver in HDA. It looks like the prepare/cleanup pair in the pcm_ops structure might be the right place to put things? If that's too fine grained, you could do it at open/close time I guess, but the danger there is that some app will keep the device open even while it's not playing.
If that won't work, maybe calling i915 from hda_power_work in the higher level code would be better?
For detecting whether to call i915 at all, you can filter on the PCI IDs (just look for an Intel graphics device and if present, try to get the i915 symbols for the power functions).
--- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -3860,6 +3860,8 @@ static unsigned int hda_call_codec_suspend(struct hda_code codec->patch_ops.suspend(codec); hda_cleanup_all_streams(codec); state = hda_set_power_state(codec, AC_PWRST_D3);
if (i915_shared_power_well)i915_put_power_well(codec->i915_data); /* Cancel delayed work if we aren't currently running from it. */ if (!in_wq) cancel_delayed_work_sync(&codec->power_work);@@ -4807,6 +4809,9 @@ static void __snd_hda_power_up(struct hda_codec *codec, bo return; spin_unlock(&codec->power_lock);
if (i915_shared_power_well)i915_get_power_well(codec->i915_data);Is it wise that a _get function actually has side effects? Perhaps _push and _pop or something else would be better semantics.
We have other get/put functions like this, so I thought it fit (vblank irq, PCI runtime PM).
cancel_delayed_work_sync(&codec->power_work); spin_lock(&codec->power_lock);With some code at init time to get the i915 symbols you need to call and whether or not the shared power well is present...
Takashi, any other ideas?
The high level goal here should be for the audio driver to call into i915 with get/put power well around the sequences where it needs the power to be up (reading/writing registers, playing audio), but not across the whole time the driver is loaded, just like you already do with the powersave work functions, e.g. hda_call_codec_suspend.
I think this sounds about right. The question is how to avoid a dependency on the i915 driver when it's not necessary, such as when the HDMI codec is AMD or Nvidia.
The most obvious way to me seems to be to create a new snd-hda-codec-hdmi-haswell module (that depends on both i915 and snd-hda-codec-hdmi), and then let that call into i915 and codec-hdmi drivers as necessary, e g using the set_power_state callback for the i915 stuff.
But maybe there's something smarter to do here, as I'm not experienced in mending kernel pieces together :-)
Yeah a separate module would be ok too. But I was hoping the i915 dependencies could be pretty well contained so as not to affect readability in the HDA driver, while having all the conditionality needed for whether i915 is loaded, or loaded first, or needed at all on a given platform.
Thanks, Jesse
On 04/29/2013 05:02 PM, Jesse Barnes wrote:
On Sat, 27 Apr 2013 13:35:29 +0200 Daniel Vetter daniel@ffwll.ch wrote: The high level goal here should be for the audio driver to call into i915 with get/put power well around the sequences where it needs the power to be up (reading/writing registers, playing audio), but not across the whole time the driver is loaded, just like you already do with the powersave work functions, e.g. hda_call_codec_suspend.
Hi Daniel,
We can modify the patch so that the audio driver will not request the power well across the whole time the HD-A driver is loaded, but request/release the power well at runtime.
The HD-Audio driver can support runtime PM. When the codec is idle, it can suspend the codec and further suspend the controller, thus the power well is no longer needed. So we can add haswell-specific code to release the power well in HD-A runtime suspend callback azx_suspend(), and request the power well in audio runtime resume callback azx_resume().
Thanks Mengdong
On Tue, 2013-04-30 at 12:29 +0200, David Henningsson wrote:
On 04/29/2013 05:02 PM, Jesse Barnes wrote:
On Sat, 27 Apr 2013 13:35:29 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Sat, Apr 27, 2013 at 09:20:39AM +0000, Wang, Xingchao wrote:
Let me throw a basic proposal on Audio driver side, please give your comments freely.
it contains the power well control usage points: #1: audio request power well at boot up. I915 may shut down power well after bootup initialization, as there's no monitor connected outside or only eDP on pipe A. #2: audio request power on resume After exit from D3 mode, audio driver quest power on. This may happen at normal resume or runtime resume. #3: audio release power well control at suspend Audio driver will let i915 know it doensot need power well anymore as it's going to suspend. This may happened at normal suspend or runtime suspend point. #4: audio release power well when module unload Audio release power well at remove callback to let i915 know.
I miss the power well grab/dropping at runtime from the audio side. If the audio driver forces the power well to be on the entire time it's loaded, that's not good, since the power well stuff is very much for runtime PM. We _must_ be able to switch off the power well whenever possible.
Xingchao, I'm not an audio developer so I'm probably way off.
But what we really need is a very small and targeted set of calls into the i915 driver from say the HDMI driver in HDA. It looks like the prepare/cleanup pair in the pcm_ops structure might be the right place to put things? If that's too fine grained, you could do it at open/close time I guess, but the danger there is that some app will keep the device open even while it's not playing.
If that won't work, maybe calling i915 from hda_power_work in the higher level code would be better?
For detecting whether to call i915 at all, you can filter on the PCI IDs (just look for an Intel graphics device and if present, try to get the i915 symbols for the power functions).
--- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -3860,6 +3860,8 @@ static unsigned int hda_call_codec_suspend(struct hda_code codec->patch_ops.suspend(codec); hda_cleanup_all_streams(codec); state = hda_set_power_state(codec, AC_PWRST_D3);
if (i915_shared_power_well)i915_put_power_well(codec->i915_data); /* Cancel delayed work if we aren't currently running from it. */ if (!in_wq) cancel_delayed_work_sync(&codec->power_work);@@ -4807,6 +4809,9 @@ static void __snd_hda_power_up(struct hda_codec *codec, bo return; spin_unlock(&codec->power_lock);
if (i915_shared_power_well)i915_get_power_well(codec->i915_data);Is it wise that a _get function actually has side effects? Perhaps _push and _pop or something else would be better semantics.
I think the intention here is to model on the PM runtime subsystem where we can get/put the reference count on a PM resource. I'd expect with this API that i915_get_power_well() will increment the count and prevent the shared PM resource from being powered OFF.
cancel_delayed_work_sync(&codec->power_work); spin_lock(&codec->power_lock);With some code at init time to get the i915 symbols you need to call and whether or not the shared power well is present...
Takashi, any other ideas?
The high level goal here should be for the audio driver to call into i915 with get/put power well around the sequences where it needs the power to be up (reading/writing registers, playing audio), but not across the whole time the driver is loaded, just like you already do with the powersave work functions, e.g. hda_call_codec_suspend.
I think this sounds about right. The question is how to avoid a dependency on the i915 driver when it's not necessary, such as when the HDMI codec is AMD or Nvidia.
The most obvious way to me seems to be to create a new snd-hda-codec-hdmi-haswell module (that depends on both i915 and snd-hda-codec-hdmi), and then let that call into i915 and codec-hdmi drivers as necessary, e g using the set_power_state callback for the i915 stuff.
But maybe there's something smarter to do here, as I'm not experienced in mending kernel pieces together :-)
Interesting idea, we could have something similar to the AC97 ad-hoc device support where we could load other HW specific AC97 modules (like touchscreen controllers) without breaking the generic nature of the base driver. e.g. the WM9712 AC97 touchscreen driver could connect to the generic AC97 audio driver (get it's device struct ac97 *) and perform AC97 register IO, ac97 API calls etc.
Regards
Liam
On 04/30/2013 04:41 PM, Liam Girdwood wrote:
On Tue, 2013-04-30 at 12:29 +0200, David Henningsson wrote:
On 04/29/2013 05:02 PM, Jesse Barnes wrote:
On Sat, 27 Apr 2013 13:35:29 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Sat, Apr 27, 2013 at 09:20:39AM +0000, Wang, Xingchao wrote:
Let me throw a basic proposal on Audio driver side, please give your comments freely.
it contains the power well control usage points: #1: audio request power well at boot up. I915 may shut down power well after bootup initialization, as there's no monitor connected outside or only eDP on pipe A. #2: audio request power on resume After exit from D3 mode, audio driver quest power on. This may happen at normal resume or runtime resume. #3: audio release power well control at suspend Audio driver will let i915 know it doensot need power well anymore as it's going to suspend. This may happened at normal suspend or runtime suspend point. #4: audio release power well when module unload Audio release power well at remove callback to let i915 know.
I miss the power well grab/dropping at runtime from the audio side. If the audio driver forces the power well to be on the entire time it's loaded, that's not good, since the power well stuff is very much for runtime PM. We _must_ be able to switch off the power well whenever possible.
Xingchao, I'm not an audio developer so I'm probably way off.
But what we really need is a very small and targeted set of calls into the i915 driver from say the HDMI driver in HDA. It looks like the prepare/cleanup pair in the pcm_ops structure might be the right place to put things? If that's too fine grained, you could do it at open/close time I guess, but the danger there is that some app will keep the device open even while it's not playing.
If that won't work, maybe calling i915 from hda_power_work in the higher level code would be better?
For detecting whether to call i915 at all, you can filter on the PCI IDs (just look for an Intel graphics device and if present, try to get the i915 symbols for the power functions).
--- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -3860,6 +3860,8 @@ static unsigned int hda_call_codec_suspend(struct hda_code codec->patch_ops.suspend(codec); hda_cleanup_all_streams(codec); state = hda_set_power_state(codec, AC_PWRST_D3);
if (i915_shared_power_well)i915_put_power_well(codec->i915_data); /* Cancel delayed work if we aren't currently running from it. */ if (!in_wq) cancel_delayed_work_sync(&codec->power_work);@@ -4807,6 +4809,9 @@ static void __snd_hda_power_up(struct hda_codec *codec, bo return; spin_unlock(&codec->power_lock);
if (i915_shared_power_well)i915_get_power_well(codec->i915_data);Is it wise that a _get function actually has side effects? Perhaps _push and _pop or something else would be better semantics.
I think the intention here is to model on the PM runtime subsystem where we can get/put the reference count on a PM resource. I'd expect with this API that i915_get_power_well() will increment the count and prevent the shared PM resource from being powered OFF.
Ok, I stand corrected.
cancel_delayed_work_sync(&codec->power_work); spin_lock(&codec->power_lock);With some code at init time to get the i915 symbols you need to call and whether or not the shared power well is present...
Takashi, any other ideas?
The high level goal here should be for the audio driver to call into i915 with get/put power well around the sequences where it needs the power to be up (reading/writing registers, playing audio), but not across the whole time the driver is loaded, just like you already do with the powersave work functions, e.g. hda_call_codec_suspend.
I think this sounds about right. The question is how to avoid a dependency on the i915 driver when it's not necessary, such as when the HDMI codec is AMD or Nvidia.
The most obvious way to me seems to be to create a new snd-hda-codec-hdmi-haswell module (that depends on both i915 and snd-hda-codec-hdmi), and then let that call into i915 and codec-hdmi drivers as necessary, e g using the set_power_state callback for the i915 stuff.
But maybe there's something smarter to do here, as I'm not experienced in mending kernel pieces together :-)
Interesting idea, we could have something similar to the AC97 ad-hoc device support where we could load other HW specific AC97 modules (like touchscreen controllers) without breaking the generic nature of the base driver. e.g. the WM9712 AC97 touchscreen driver could connect to the generic AC97 audio driver (get it's device struct ac97 *) and perform AC97 register IO, ac97 API calls etc.
Nobody objected, so I wrote a very draft patch, which is attached to this email. It probably does not even compile, but should show how I envision things could be mended together. Do you think it could work?
Also, I tried to find the patch set for the i915 side, but couldn't find any while looking on the intel-gfx mailinglist (which I'm not subscribed to) - only comments on those patches. Where can the latest version of the i915 patches be found?
Hi David,
Thank you very much for your draft patch. I have some more work on a new patchset, some ideas are from your patch.
Here's a brief introduction of attached patchset:
1. a new bus type in /sound/had_bus.c, used to bind the single module and codec device It looks like ac97_bus.c
2. add a new device node in "struct hda_codec", it's used to register for new bus type.
3. a new single module hdmi_i915, which compiled in only when DRM_I915 and CODEC_HDMI enabled. It stores the private API for gfx part. There's no support to probe haswell hdmi codec only yet. Power well will be used only for haswell display audio.
4. power well API implementation in gfx side.
Please feel free to add your idea and I will help test your patch too.
Thanks --xingchao
-----Original Message----- From: David Henningsson [mailto:david.henningsson@canonical.com] Sent: Thursday, May 02, 2013 10:49 PM To: Liam Girdwood Cc: Barnes, Jesse; alsa-devel@alsa-project.org; Zanoni, Paulo R; Takashi Iwai; Daniel Vetter; intel-gfx@lists.freedesktop.org; Wysocki, Rafael J; Wang Xingchao; Wang, Xingchao; Li, Jocelyn; Hindman, Gavin; Daniel Vetter; Lin, Mengdong; ville.syrjala@linux.intel.com Subject: Re: [alsa-devel] [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team
On 04/30/2013 04:41 PM, Liam Girdwood wrote:
On Tue, 2013-04-30 at 12:29 +0200, David Henningsson wrote:
On 04/29/2013 05:02 PM, Jesse Barnes wrote:
On Sat, 27 Apr 2013 13:35:29 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Sat, Apr 27, 2013 at 09:20:39AM +0000, Wang, Xingchao wrote:
Let me throw a basic proposal on Audio driver side, please give your
comments freely.
it contains the power well control usage points: #1: audio request power well at boot up. I915 may shut down power well after bootup initialization, as there's no
monitor connected outside or only eDP on pipe A.
#2: audio request power on resume After exit from D3 mode, audio driver quest power on. This may happen
at normal resume or runtime resume.
#3: audio release power well control at suspend Audio driver will let i915 know it doensot need power well anymore as it's going to
suspend. This may happened at normal suspend or runtime suspend point.
#4: audio release power well when module unload Audio release power well at remove callback to let i915 know.
I miss the power well grab/dropping at runtime from the audio side. If the audio driver forces the power well to be on the entire time it's loaded, that's not good, since the power well stuff is very much for
runtime PM.
We _must_ be able to switch off the power well whenever possible.
Xingchao, I'm not an audio developer so I'm probably way off.
But what we really need is a very small and targeted set of calls into the i915 driver from say the HDMI driver in HDA. It looks like the prepare/cleanup pair in the pcm_ops structure might be the right place to put things? If that's too fine grained, you could do it at open/close time I guess, but the danger there is that some app will keep the device open even while it's not playing.
If that won't work, maybe calling i915 from hda_power_work in the higher level code would be better?
For detecting whether to call i915 at all, you can filter on the PCI IDs (just look for an Intel graphics device and if present, try to get the i915 symbols for the power functions).
--- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -3860,6 +3860,8 @@ static unsigned int
hda_call_codec_suspend(struct hda_code
codec->patch_ops.suspend(codec); hda_cleanup_all_streams(codec); state = hda_set_power_state(codec, AC_PWRST_D3);
if (i915_shared_power_well)i915_put_power_well(codec->i915_data); /* Cancel delayed work if we aren't currently running from it.*/
if (!in_wq) cancel_delayed_work_sync(&codec->power_work);@@ -4807,6 +4809,9 @@ static void __snd_hda_power_up(struct
hda_codec *codec, bo
return; spin_unlock(&codec->power_lock);
if (i915_shared_power_well)i915_get_power_well(codec->i915_data);Is it wise that a _get function actually has side effects? Perhaps _push and _pop or something else would be better semantics.
I think the intention here is to model on the PM runtime subsystem where we can get/put the reference count on a PM resource. I'd expect with this API that i915_get_power_well() will increment the count and prevent the shared PM resource from being powered OFF.
Ok, I stand corrected.
cancel_delayed_work_sync(&codec->power_work); spin_lock(&codec->power_lock);With some code at init time to get the i915 symbols you need to call and whether or not the shared power well is present...
Takashi, any other ideas?
The high level goal here should be for the audio driver to call into i915 with get/put power well around the sequences where it needs the power to be up (reading/writing registers, playing audio), but not across the whole time the driver is loaded, just like you already do with the powersave work functions, e.g. hda_call_codec_suspend.
I think this sounds about right. The question is how to avoid a dependency on the i915 driver when it's not necessary, such as when the HDMI codec is AMD or Nvidia.
The most obvious way to me seems to be to create a new snd-hda-codec-hdmi-haswell module (that depends on both i915 and snd-hda-codec-hdmi), and then let that call into i915 and codec-hdmi drivers as necessary, e g using the set_power_state callback for the i915 stuff.
But maybe there's something smarter to do here, as I'm not experienced in mending kernel pieces together :-)
Interesting idea, we could have something similar to the AC97 ad-hoc device support where we could load other HW specific AC97 modules (like touchscreen controllers) without breaking the generic nature of the base driver. e.g. the WM9712 AC97 touchscreen driver could connect to the generic AC97 audio driver (get it's device struct ac97 *) and perform AC97 register IO, ac97 API calls etc.
Nobody objected, so I wrote a very draft patch, which is attached to this email. It probably does not even compile, but should show how I envision things could be mended together. Do you think it could work?
Also, I tried to find the patch set for the i915 side, but couldn't find any while looking on the intel-gfx mailinglist (which I'm not subscribed to) - only comments on those patches. Where can the latest version of the i915 patches be found?
-- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
On 05/03/2013 10:28 AM, Wang, Xingchao wrote:
Hi David,
Thank you very much for your draft patch. I have some more work on a new patchset, some ideas are from your patch.
Thanks.
Here's a brief introduction of attached patchset:
- a new bus type in /sound/had_bus.c, used to bind the single module and codec device
It looks like ac97_bus.c
I don't understand why this is needed. It does not look like it's used from the gfx side either, or anything like that?
add a new device node in "struct hda_codec", it's used to register for new bus type.
a new single module hdmi_i915, which compiled in only when DRM_I915 and CODEC_HDMI enabled.
It stores the private API for gfx part. There's no support to probe haswell hdmi codec only yet. Power well will be used only for haswell display audio.
- power well API implementation in gfx side.
Please feel free to add your idea and I will help test your patch too.
Ok. So the patch I wrote would (if it works) be combined with your patch 3, which implements the gfx side. The gfx side is not my area of expertise.
The proposed way in my patch would be more elegant since it does not introduce any i915 related code in hda_codec* files.
Still, Takashi is the boss here so he has the final say :-)
Thanks --xingchao
-----Original Message----- From: David Henningsson [mailto:david.henningsson@canonical.com] Sent: Thursday, May 02, 2013 10:49 PM To: Liam Girdwood Cc: Barnes, Jesse; alsa-devel@alsa-project.org; Zanoni, Paulo R; Takashi Iwai; Daniel Vetter; intel-gfx@lists.freedesktop.org; Wysocki, Rafael J; Wang Xingchao; Wang, Xingchao; Li, Jocelyn; Hindman, Gavin; Daniel Vetter; Lin, Mengdong; ville.syrjala@linux.intel.com Subject: Re: [alsa-devel] [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team
On 04/30/2013 04:41 PM, Liam Girdwood wrote:
On Tue, 2013-04-30 at 12:29 +0200, David Henningsson wrote:
On 04/29/2013 05:02 PM, Jesse Barnes wrote:
On Sat, 27 Apr 2013 13:35:29 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Sat, Apr 27, 2013 at 09:20:39AM +0000, Wang, Xingchao wrote: > Let me throw a basic proposal on Audio driver side, please give your
comments freely.
> > it contains the power well control usage points: > #1: audio request power well at boot up. > I915 may shut down power well after bootup initialization, as there's no
monitor connected outside or only eDP on pipe A.
> #2: audio request power on resume > After exit from D3 mode, audio driver quest power on. This may happen
at normal resume or runtime resume.
> #3: audio release power well control at suspend Audio driver will > let i915 know it doensot need power well anymore as it's going to
suspend. This may happened at normal suspend or runtime suspend point.
> #4: audio release power well when module unload Audio release > power well at remove callback to let i915 know.
I miss the power well grab/dropping at runtime from the audio side. If the audio driver forces the power well to be on the entire time it's loaded, that's not good, since the power well stuff is very much for
runtime PM.
We _must_ be able to switch off the power well whenever possible.
Xingchao, I'm not an audio developer so I'm probably way off.
But what we really need is a very small and targeted set of calls into the i915 driver from say the HDMI driver in HDA. It looks like the prepare/cleanup pair in the pcm_ops structure might be the right place to put things? If that's too fine grained, you could do it at open/close time I guess, but the danger there is that some app will keep the device open even while it's not playing.
If that won't work, maybe calling i915 from hda_power_work in the higher level code would be better?
For detecting whether to call i915 at all, you can filter on the PCI IDs (just look for an Intel graphics device and if present, try to get the i915 symbols for the power functions).
--- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -3860,6 +3860,8 @@ static unsigned int
hda_call_codec_suspend(struct hda_code
codec->patch_ops.suspend(codec); hda_cleanup_all_streams(codec); state = hda_set_power_state(codec, AC_PWRST_D3);
if (i915_shared_power_well)i915_put_power_well(codec->i915_data); /* Cancel delayed work if we aren't currently running from it.*/
if (!in_wq) cancel_delayed_work_sync(&codec->power_work);@@ -4807,6 +4809,9 @@ static void __snd_hda_power_up(struct
hda_codec *codec, bo
return; spin_unlock(&codec->power_lock);
if (i915_shared_power_well)i915_get_power_well(codec->i915_data);Is it wise that a _get function actually has side effects? Perhaps _push and _pop or something else would be better semantics.
I think the intention here is to model on the PM runtime subsystem where we can get/put the reference count on a PM resource. I'd expect with this API that i915_get_power_well() will increment the count and prevent the shared PM resource from being powered OFF.
Ok, I stand corrected.
cancel_delayed_work_sync(&codec->power_work); spin_lock(&codec->power_lock);With some code at init time to get the i915 symbols you need to call and whether or not the shared power well is present...
Takashi, any other ideas?
The high level goal here should be for the audio driver to call into i915 with get/put power well around the sequences where it needs the power to be up (reading/writing registers, playing audio), but not across the whole time the driver is loaded, just like you already do with the powersave work functions, e.g. hda_call_codec_suspend.
I think this sounds about right. The question is how to avoid a dependency on the i915 driver when it's not necessary, such as when the HDMI codec is AMD or Nvidia.
The most obvious way to me seems to be to create a new snd-hda-codec-hdmi-haswell module (that depends on both i915 and snd-hda-codec-hdmi), and then let that call into i915 and codec-hdmi drivers as necessary, e g using the set_power_state callback for the i915 stuff.
But maybe there's something smarter to do here, as I'm not experienced in mending kernel pieces together :-)
Interesting idea, we could have something similar to the AC97 ad-hoc device support where we could load other HW specific AC97 modules (like touchscreen controllers) without breaking the generic nature of the base driver. e.g. the WM9712 AC97 touchscreen driver could connect to the generic AC97 audio driver (get it's device struct ac97 *) and perform AC97 register IO, ac97 API calls etc.
Nobody objected, so I wrote a very draft patch, which is attached to this email. It probably does not even compile, but should show how I envision things could be mended together. Do you think it could work?
Also, I tried to find the patch set for the i915 side, but couldn't find any while looking on the intel-gfx mailinglist (which I'm not subscribed to) - only comments on those patches. Where can the latest version of the i915 patches be found?
-- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
At Fri, 03 May 2013 14:02:04 +0200, David Henningsson wrote:
On 05/03/2013 10:28 AM, Wang, Xingchao wrote:
Hi David,
Thank you very much for your draft patch. I have some more work on a new patchset, some ideas are from your patch.
Thanks.
Here's a brief introduction of attached patchset:
- a new bus type in /sound/had_bus.c, used to bind the single module and codec device
It looks like ac97_bus.c
I don't understand why this is needed. It does not look like it's used from the gfx side either, or anything like that?
add a new device node in "struct hda_codec", it's used to register for new bus type.
a new single module hdmi_i915, which compiled in only when DRM_I915 and CODEC_HDMI enabled.
It stores the private API for gfx part. There's no support to probe haswell hdmi codec only yet. Power well will be used only for haswell display audio.
- power well API implementation in gfx side.
Please feel free to add your idea and I will help test your patch too.
Ok. So the patch I wrote would (if it works) be combined with your patch 3, which implements the gfx side. The gfx side is not my area of expertise.
The proposed way in my patch would be more elegant since it does not introduce any i915 related code in hda_codec* files.
Still, Takashi is the boss here so he has the final say :-)
Indeed. If the reference to power well API is limited in a newly split snd-hda-codec-hdmi-i915 driver, we don't have to create yet another driver instance. The snd-hda-codec-hdmi-i915 can simply depend on i915, by referring to a symbol exported from i915 driver.
If we now touch the whole PM sequence in both gfx and audio drivers (i.e. it influences on the HD-audio controller code, hda_intel.c), then we may need a different management. But I thought it's not yet discussed here, right?
Takashi
Hi Jesse,
-----Original Message----- From: Barnes, Jesse Sent: Monday, April 29, 2013 11:02 PM To: Daniel Vetter Cc: Wang, Xingchao; Takashi Iwai; Li, Jocelyn; Daniel Vetter; Zanoni, Paulo R; ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Wang Xingchao; Wysocki, Rafael J; Hindman, Gavin Subject: Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team
On Sat, 27 Apr 2013 13:35:29 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Sat, Apr 27, 2013 at 09:20:39AM +0000, Wang, Xingchao wrote:
Let me throw a basic proposal on Audio driver side, please give your
comments freely.
it contains the power well control usage points: #1: audio request power well at boot up. I915 may shut down power well after bootup initialization, as there's no
monitor connected outside or only eDP on pipe A.
#2: audio request power on resume After exit from D3 mode, audio driver quest power on. This may happen at
normal resume or runtime resume.
#3: audio release power well control at suspend Audio driver will let i915 know it doensot need power well anymore as it's going to suspend.
This may happened at normal suspend or runtime suspend point.
#4: audio release power well when module unload Audio release power well at remove callback to let i915 know.
I miss the power well grab/dropping at runtime from the audio side. If the audio driver forces the power well to be on the entire time it's loaded, that's not good, since the power well stuff is very much for runtime
PM.
We _must_ be able to switch off the power well whenever possible.
Xingchao, I'm not an audio developer so I'm probably way off.
But what we really need is a very small and targeted set of calls into the i915 driver from say the HDMI driver in HDA. It looks like the prepare/cleanup pair in the pcm_ops structure might be the right place to put things? If that's too fine grained, you could do it at open/close time I guess, but the danger there is that some app will keep the device open even while it's not playing.
If that won't work, maybe calling i915 from hda_power_work in the higher level code would be better?
For detecting whether to call i915 at all, you can filter on the PCI IDs (just look for an Intel graphics device and if present, try to get the i915 symbols for the power functions).
--- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -3860,6 +3860,8 @@ static unsigned int hda_call_codec_suspend(struct hda_code codec->patch_ops.suspend(codec); hda_cleanup_all_streams(codec); state = hda_set_power_state(codec, AC_PWRST_D3);
if (i915_shared_power_well)i915_put_power_well(codec->i915_data); /* Cancel delayed work if we aren't currently running from it. */ if (!in_wq) cancel_delayed_work_sync(&codec->power_work);@@ -4807,6 +4809,9 @@ static void __snd_hda_power_up(struct hda_codec *codec, bo return; spin_unlock(&codec->power_lock);
if (i915_shared_power_well)i915_get_power_well(codec->i915_data);cancel_delayed_work_sync(&codec->power_work); spin_lock(&codec->power_lock);With some code at init time to get the i915 symbols you need to call and whether or not the shared power well is present...
Takashi, any other ideas?
The high level goal here should be for the audio driver to call into i915 with get/put power well around the sequences where it needs the power to be up (reading/writing registers, playing audio), but not across the whole time the driver is loaded, just like you already do with the powersave work functions, e.g. hda_call_codec_suspend.
That's the point. I missed the background about power well it's part of runtime power feature. Audio driver should not always keep power well on even it's not active, I guess you were misunderstood by the power well request point at azx_probe(). azx_probe() is very initial point for power well request, and it will switch to runtime pm soon(please check Mengdong's reply in another email), so the power well will be release if the codec is inactive, that match the initial idea of power well.
Thanks --xingchao
At Mon, 29 Apr 2013 08:02:19 -0700, Jesse Barnes wrote:
On Sat, 27 Apr 2013 13:35:29 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Sat, Apr 27, 2013 at 09:20:39AM +0000, Wang, Xingchao wrote:
Let me throw a basic proposal on Audio driver side, please give your comments freely.
it contains the power well control usage points: #1: audio request power well at boot up. I915 may shut down power well after bootup initialization, as there's no monitor connected outside or only eDP on pipe A. #2: audio request power on resume After exit from D3 mode, audio driver quest power on. This may happen at normal resume or runtime resume. #3: audio release power well control at suspend Audio driver will let i915 know it doensot need power well anymore as it's going to suspend. This may happened at normal suspend or runtime suspend point. #4: audio release power well when module unload Audio release power well at remove callback to let i915 know.
I miss the power well grab/dropping at runtime from the audio side. If the audio driver forces the power well to be on the entire time it's loaded, that's not good, since the power well stuff is very much for runtime PM. We _must_ be able to switch off the power well whenever possible.
Xingchao, I'm not an audio developer so I'm probably way off.
But what we really need is a very small and targeted set of calls into the i915 driver from say the HDMI driver in HDA. It looks like the prepare/cleanup pair in the pcm_ops structure might be the right place to put things? If that's too fine grained, you could do it at open/close time I guess, but the danger there is that some app will keep the device open even while it's not playing.
Well, the question is what impact the power well on/off has against the audio. Do we need to resume the HD-audio controller / codec fully from the scratch? I guess not, but I have no certain idea.
If the impact of the change (i.e. the procedure needed to resume) is small, somehow limited to the targeted converter/pin, it can be implemented in the prepare/cleanup callback of the codec driver, yes.
Though, the easiest path would be to add i915_get/put_power_well() in the codec probe, suspend, resume, and free callbacks, as you pointed out below.
If that won't work, maybe calling i915 from hda_power_work in the higher level code would be better?
For detecting whether to call i915 at all, you can filter on the PCI IDs (just look for an Intel graphics device and if present, try to get the i915 symbols for the power functions).
--- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -3860,6 +3860,8 @@ static unsigned int hda_call_codec_suspend(struct hda_code codec->patch_ops.suspend(codec); hda_cleanup_all_streams(codec); state = hda_set_power_state(codec, AC_PWRST_D3);
if (i915_shared_power_well)i915_put_power_well(codec->i915_data); /* Cancel delayed work if we aren't currently running from it. */ if (!in_wq) cancel_delayed_work_sync(&codec->power_work);@@ -4807,6 +4809,9 @@ static void __snd_hda_power_up(struct hda_codec *codec, bo return; spin_unlock(&codec->power_lock);
if (i915_shared_power_well)i915_get_power_well(codec->i915_data);cancel_delayed_work_sync(&codec->power_work); spin_lock(&codec->power_lock);With some code at init time to get the i915 symbols you need to call and whether or not the shared power well is present...
Takashi, any other ideas?
The high level goal here should be for the audio driver to call into i915 with get/put power well around the sequences where it needs the power to be up (reading/writing registers, playing audio), but not across the whole time the driver is loaded, just like you already do with the powersave work functions, e.g. hda_call_codec_suspend.
I guess controlling the suspend/resume path would be practically working well. If a system is really power-conscious, it should use a sound backend like PulseAudio, which closes the unused PCM devices frequently enough, and the power_save option should be changed by the power management tool on the fly.
If such mechanisms aren't used, it means that user doesn't care about power, after all.
thanks,
Takashi
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Friday, May 03, 2013 10:27 PM To: Barnes, Jesse Cc: Daniel Vetter; Wang, Xingchao; Li, Jocelyn; Daniel Vetter; Zanoni, Paulo R; ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Wang Xingchao; Wysocki, Rafael J; Hindman, Gavin Subject: Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team
At Mon, 29 Apr 2013 08:02:19 -0700, Jesse Barnes wrote:
On Sat, 27 Apr 2013 13:35:29 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Sat, Apr 27, 2013 at 09:20:39AM +0000, Wang, Xingchao wrote:
Let me throw a basic proposal on Audio driver side, please give your
comments freely.
it contains the power well control usage points: #1: audio request power well at boot up. I915 may shut down power well after bootup initialization, as there's no
monitor connected outside or only eDP on pipe A.
#2: audio request power on resume After exit from D3 mode, audio driver quest power on. This may happen at
normal resume or runtime resume.
#3: audio release power well control at suspend Audio driver will let i915 know it doensot need power well anymore as it's going to suspend.
This may happened at normal suspend or runtime suspend point.
#4: audio release power well when module unload Audio release power well at remove callback to let i915 know.
I miss the power well grab/dropping at runtime from the audio side. If the audio driver forces the power well to be on the entire time it's loaded, that's not good, since the power well stuff is very much for
runtime PM.
We _must_ be able to switch off the power well whenever possible.
Xingchao, I'm not an audio developer so I'm probably way off.
But what we really need is a very small and targeted set of calls into the i915 driver from say the HDMI driver in HDA. It looks like the prepare/cleanup pair in the pcm_ops structure might be the right place to put things? If that's too fine grained, you could do it at open/close time I guess, but the danger there is that some app will keep the device open even while it's not playing.
Well, the question is what impact the power well on/off has against the audio. Do we need to resume the HD-audio controller / codec fully from the scratch? I guess not, but I have no certain idea.
Both the display H-DA controller and codec are under control by power well. When the power well is off, for H-DA controller, the MMIO space is off, the PCI Registers are in on well. The codec could not access anymore.
If the impact of the change (i.e. the procedure needed to resume) is small, somehow limited to the targeted converter/pin, it can be implemented in the prepare/cleanup callback of the codec driver, yes.
Though, the easiest path would be to add i915_get/put_power_well() in the codec probe, suspend, resume, and free callbacks, as you pointed out below.
Yes, and Jesse should get the background that, even power well is requested at probe, It will not take long time to waste power. The controller/codec will enter power save mode If runtime pm enabled.
Thanks --xingchao
If that won't work, maybe calling i915 from hda_power_work in the higher level code would be better?
For detecting whether to call i915 at all, you can filter on the PCI IDs (just look for an Intel graphics device and if present, try to get the i915 symbols for the power functions).
--- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -3860,6 +3860,8 @@ static unsigned int hda_call_codec_suspend(struct
hda_code
codec->patch_ops.suspend(codec); hda_cleanup_all_streams(codec); state = hda_set_power_state(codec, AC_PWRST_D3);
if (i915_shared_power_well)i915_put_power_well(codec->i915_data); /* Cancel delayed work if we aren't currently running from it. */ if (!in_wq) cancel_delayed_work_sync(&codec->power_work);@@ -4807,6 +4809,9 @@ static void __snd_hda_power_up(struct
hda_codec *codec, bo
return; spin_unlock(&codec->power_lock);
if (i915_shared_power_well)i915_get_power_well(codec->i915_data);cancel_delayed_work_sync(&codec->power_work); spin_lock(&codec->power_lock);With some code at init time to get the i915 symbols you need to call and whether or not the shared power well is present...
Takashi, any other ideas?
The high level goal here should be for the audio driver to call into i915 with get/put power well around the sequences where it needs the power to be up (reading/writing registers, playing audio), but not across the whole time the driver is loaded, just like you already do with the powersave work functions, e.g. hda_call_codec_suspend.
I guess controlling the suspend/resume path would be practically working well. If a system is really power-conscious, it should use a sound backend like PulseAudio, which closes the unused PCM devices frequently enough, and the power_save option should be changed by the power management tool on the fly.
If such mechanisms aren't used, it means that user doesn't care about power, after all.
thanks,
Takashi
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Friday, April 26, 2013 11:13 PM To: Daniel Vetter Cc: Li, Jocelyn; Daniel Vetter; Wang, Xingchao; Zanoni, Paulo R; ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Wang Xingchao; Barnes, Jesse; Wysocki, Rafael J; Hindman, Gavin Subject: Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team
At Fri, 26 Apr 2013 16:57:08 +0200, Daniel Vetter wrote:
On Fri, Apr 26, 2013 at 07:53:41AM +0000, Li, Jocelyn wrote:
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] Sent: Friday, April 26, 2013 3:25 PM To: Li, Jocelyn Cc: Wang, Xingchao; Zanoni, Paulo R; ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Wang Xingchao; Takashi Iwai; Barnes, Jesse; Wysocki, Rafael J; Hindman, Gavin Subject: Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team Once we've figured out what needs to be changed in the audio driver we can look at the entire
patch series and the interface to i915.ko.
That's also why I didn't comment on Xingchao's patch right away, but only once he specifically asked for feedback, since doing a real review of the interface doesn't make sense until we have all the pieces (and a working audio driver).
[Jocelyn] I think you have made constructive comments on Xingchao's patch yesterday. Next, shall we have Xingchao improve his patch? Or we just have Xingchao wait till you have completed your pieces. Sorry, I am a little confused :)
I think the next step is to use Xinchao's patch as-is and get the audio side going. Once we have that fixed up, we can revisit the interface and make it solid. But for now trying to polish this relatively simple part seems like wasted time.
Also, reviewing an interface is much easier if we have both the i915 pieces (already here) and the audio pieces (which I haven't seen yet) avaialble.
I haven't checked the patch properly yet, but the patch pasted in the post looks like i915 driver exports the functions to control power well, and let audio driver calling them. If so, the big mess in such a case is the dependency between driver modules.
A simple workaround would be to split this function and the relevant instance out of i915 and snd-hda-intel and put into an individual module (e.g. i915-powerwell-ctl).
I agree this a single independent module could solve the dependency issue. I tested the patch without i915 module loaded, there's symbol error in audio drvier side.
Also, it would be feasible to implement some PM governor over both graphics and audio, that is, it gives the proper serialization of power up for audio controller, for example.
This could avoid more private APIs between gfx and audio driver as there're really some dependencies on that now. Besides that could the governor help serialize the resume sequence for gfx and audio? We have some good cases to start for the implementations now. i.e. audio driver resume too early while gfx doesnot enable relative transcoders which cause codec pin issues. If you have more guide on this, that would be appreciated.
Thanks --xingchao
Hi Daniel,
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Friday, April 26, 2013 10:57 PM To: Li, Jocelyn Cc: Daniel Vetter; Wang, Xingchao; Zanoni, Paulo R; ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Wang Xingchao; Takashi Iwai; Barnes, Jesse; Wysocki, Rafael J; Hindman, Gavin Subject: Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team
On Fri, Apr 26, 2013 at 07:53:41AM +0000, Li, Jocelyn wrote:
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] Sent: Friday, April 26, 2013 3:25 PM To: Li, Jocelyn Cc: Wang, Xingchao; Zanoni, Paulo R; ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Wang Xingchao; Takashi Iwai; Barnes, Jesse; Wysocki, Rafael J; Hindman, Gavin Subject: Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team Once we've figured out what needs to be changed in the audio driver we can look at the entire patch
series and the interface to i915.ko.
That's also why I didn't comment on Xingchao's patch right away, but only once he specifically asked for feedback, since doing a real review of the interface doesn't make sense until we have all the pieces (and a working audio driver).
[Jocelyn] I think you have made constructive comments on Xingchao's patch yesterday. Next, shall we have Xingchao improve his patch? Or we just have Xingchao wait till you have completed your pieces. Sorry, I am a little confused :)
I think the next step is to use Xinchao's patch as-is and get the audio side going.
If you mean the audio working with this patch while power well feature enabled, I say yes. Please see my reply after sending this patch: http://lists.freedesktop.org/archives/intel-gfx/2013-April/027157.html
The patch could prevent power well losing, thus audio driver could keep its power on. Without this patch, audio codec info will become unavailable if the power well is shut down by graphic side.
Once we have that fixed up, we can revisit the interface and make it solid. But for now trying to polish this relatively simple part seems like wasted time.
Also, reviewing an interface is much easier if we have both the i915 pieces (already here) and the audio pieces (which I haven't seen yet) avaialble.
It's good idea to list audio driver's usage pieces, I will prepare for that in another email later.
Thanks --xingchao
participants (9)
-
Daniel Vetter -
Daniel Vetter -
David Henningsson -
Jesse Barnes -
Li, Jocelyn -
Liam Girdwood -
Lin, Mengdong -
Takashi Iwai -
Wang, Xingchao