[PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[Li, Jocelyn]

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:
>> 1. 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

Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[Daniel Vetter]

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:
>>> 1. 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

Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[Li, Jocelyn]

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:
>>> 1. 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

Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[Takashi Iwai]

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

Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[Takashi Iwai]

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

Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[Wang, 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 

Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[Wang, Xingchao]

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

Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[Wang, Xingchao]

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

Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[Jesse Barnes]

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

Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[David Henningsson]

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 :-)


-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[Jesse Barnes]

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

Re: [alsa-devel] [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[Liam Girdwood]

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

Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[Lin, Mengdong]

> > 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

Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[Wang, Xingchao]

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

Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[David Henningsson]

[-- Attachment #1: Type: text/plain, Size: 5706 bytes --]

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

[-- Attachment #2: 0001-ALSA-hda-implement-i915-power-well-callbacks.patch --]
[-- Type: text/x-patch, Size: 6181 bytes --]

>From 66d9a3fd80f1dc0471949680a076c7696a869adc Mon Sep 17 00:00:00 2001
From: David Henningsson <david.henningsson@canonical.com>
Date: Thu, 2 May 2013 16:38:54 +0200
Subject: [PATCH] ALSA: hda - implement i915 power well callbacks

Draft patch

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 sound/pci/hda/Kconfig           |   14 ++++++++++
 sound/pci/hda/Makefile          |    4 +++
 sound/pci/hda/patch_hdmi.c      |   19 ++++++++++---
 sound/pci/hda/patch_hdmi.h      |    6 ++++
 sound/pci/hda/patch_hdmi_i915.c |   58 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 97 insertions(+), 4 deletions(-)
 create mode 100644 sound/pci/hda/patch_hdmi.h
 create mode 100644 sound/pci/hda/patch_hdmi_i915.c

diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig
index 80a7d44..18226de 100644
--- a/sound/pci/hda/Kconfig
+++ b/sound/pci/hda/Kconfig
@@ -152,6 +152,20 @@ config SND_HDA_CODEC_HDMI
 	  snd-hda-codec-hdmi.
 	  This module is automatically loaded at probing.
 
+config SND_HDA_CODEC_HDMI_I915
+	bool "Build HDMI/DisplayPort HD-audio codec support for i915 cards"
+	depends on SND_HDA_CODEC_HDMI
+	depends on DRM_I915
+	default y
+	help
+	  Say Y here to include full HDMI and DisplayPort HD-audio codec
+	  support for Intel graphics cards based on the i915 driver.
+
+	  When the HD-audio driver is built as a module, the codec
+	  support code is also built as another module,
+	  snd-hda-codec-hdmi-i915.
+	  This module is automatically loaded at probing.
+
 config SND_HDA_CODEC_CIRRUS
 	bool "Build Cirrus Logic codec support"
 	default y
diff --git a/sound/pci/hda/Makefile b/sound/pci/hda/Makefile
index 24a2514..ed0b2de 100644
--- a/sound/pci/hda/Makefile
+++ b/sound/pci/hda/Makefile
@@ -21,6 +21,7 @@ snd-hda-codec-ca0132-objs :=	patch_ca0132.o
 snd-hda-codec-conexant-objs :=	patch_conexant.o
 snd-hda-codec-via-objs :=	patch_via.o
 snd-hda-codec-hdmi-objs :=	patch_hdmi.o hda_eld.o
+snd-hda-codec-hdmi-i915-objs :=	patch_hdmi_i915.o
 
 # common driver
 obj-$(CONFIG_SND_HDA_INTEL) := snd-hda-codec.o
@@ -59,6 +60,9 @@ endif
 ifdef CONFIG_SND_HDA_CODEC_HDMI
 obj-$(CONFIG_SND_HDA_INTEL) += snd-hda-codec-hdmi.o
 endif
+ifdef CONFIG_SND_HDA_CODEC_HDMI_I915
+obj-$(CONFIG_SND_HDA_INTEL) += snd-hda-codec-hdmi-i915.o
+endif
 
 # this must be the last entry after codec drivers;
 # otherwise the codec patches won't be hooked before the PCI probe
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 32930e6..4fbbc1e 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1892,8 +1892,7 @@ static const struct hda_fixup hdmi_fixups[] = {
 	},
 };
 
-
-static int patch_generic_hdmi(struct hda_codec *codec)
+int initialize_hdmi_patch_ops(struct hda_codec *codec, struct hda_codec_ops *ops)
 {
 	struct hdmi_spec *spec;
 
@@ -1916,12 +1915,26 @@ static int patch_generic_hdmi(struct hda_codec *codec)
 		return -EINVAL;
 	}
 	codec->patch_ops = generic_hdmi_patch_ops;
+#ifdef CONFIG_PM
+	if (ops != NULL) {
+		if (ops->suspend)
+			codec->patch_ops.suspend = ops->suspend;
+		if (ops->resume)
+			codec->patch_ops.resume = ops->resume;
+	}
+#endif
 	generic_hdmi_init_per_pins(codec);
 
 	init_channel_allocations();
 
 	return 0;
 }
+EXPORT_SYMBOL_HDA(initialize_i915_hdmi)
+
+static int patch_generic_hdmi(struct hda_codec *codec)
+{
+	return initialize_hdmi_patch_ops(codec, NULL);
+}
 
 /*
  * Shared non-generic implementations
@@ -2559,7 +2572,6 @@ static const struct hda_codec_preset snd_hda_preset_hdmi[] = {
 { .id = 0x80862804, .name = "IbexPeak HDMI",	.patch = patch_generic_hdmi },
 { .id = 0x80862805, .name = "CougarPoint HDMI",	.patch = patch_generic_hdmi },
 { .id = 0x80862806, .name = "PantherPoint HDMI", .patch = patch_generic_hdmi },
-{ .id = 0x80862807, .name = "Haswell HDMI",	.patch = patch_generic_hdmi },
 { .id = 0x80862880, .name = "CedarTrail HDMI",	.patch = patch_generic_hdmi },
 { .id = 0x808629fb, .name = "Crestline HDMI",	.patch = patch_generic_hdmi },
 {} /* terminator */
@@ -2612,7 +2624,6 @@ MODULE_ALIAS("snd-hda-codec-id:80862803");
 MODULE_ALIAS("snd-hda-codec-id:80862804");
 MODULE_ALIAS("snd-hda-codec-id:80862805");
 MODULE_ALIAS("snd-hda-codec-id:80862806");
-MODULE_ALIAS("snd-hda-codec-id:80862807");
 MODULE_ALIAS("snd-hda-codec-id:80862880");
 MODULE_ALIAS("snd-hda-codec-id:808629fb");
 
diff --git a/sound/pci/hda/patch_hdmi.h b/sound/pci/hda/patch_hdmi.h
new file mode 100644
index 0000000..ab60c8b
--- /dev/null
+++ b/sound/pci/hda/patch_hdmi.h
@@ -0,0 +1,6 @@
+#ifndef __SOUND_HDA_PATCH_HDMI_H
+#define __SOUND_HDA_PATCH_HDMI_H
+
+int initialize_hdmi_patch_ops(struct hda_codec *codec, struct hda_codec_ops *ops);
+
+#endif
diff --git a/sound/pci/hda/patch_hdmi_i915.c b/sound/pci/hda/patch_hdmi_i915.c
new file mode 100644
index 0000000..e82a3eb
--- /dev/null
+++ b/sound/pci/hda/patch_hdmi_i915.c
@@ -0,0 +1,58 @@
+#include <drm/audio_drm.h>
+#include "patch_hdmi.h"
+
+#ifdef CONFIG_PM
+static int i915_suspend(struct hda_codec *codec)
+{
+	release_power_well(); /* Or other preferred name */
+}
+
+static int i915_resume(struct hda_codec *codec)
+{
+	request_power_well(); /* Or other preferred name */
+
+	if (codec->patch_ops.init)
+		codec->patch_ops.init(codec);
+	snd_hda_codec_resume_amp(codec);
+	snd_hda_codec_resume_cache(codec);
+}
+#endif
+
+static const struct hda_codec_ops i915_patch_ops = {
+#ifdef CONFIG_PM
+	.suspend	= i915_suspend,
+	.resume		= i915_resume,
+#endif
+}
+
+
+static int patch_i915_hdmi(struct hda_codec *codec)
+{
+	return initialize_hdmi_ops(codec, i915_patch_ops);
+}
+
+
+static const struct hda_codec_preset snd_hda_preset_hdmi[] = {
+{ .id = 0x80862807, .name = "Haswell HDMI",	.patch = patch_i915_hdmi },
+{} /* terminator */
+};
+
+MODULE_ALIAS("snd-hda-codec-id:80862807");
+
+static struct hda_codec_preset_list intel_list = {
+	.preset = snd_hda_preset_hdmi,
+	.owner = THIS_MODULE,
+};
+
+static int __init patch_hdmi_i915_init(void)
+{
+	return snd_hda_add_codec_preset(&intel_list);
+}
+
+static void __exit patch_hdmi_i915_exit(void)
+{
+	snd_hda_delete_codec_preset(&intel_list);
+}
+
+module_init(patch_hdmi_i915_init)
+module_exit(patch_hdmi_i915_exit)
-- 
1.7.9.5


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [alsa-devel] [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[Wang, Xingchao]

[-- Attachment #1: Type: text/plain, Size: 7454 bytes --]

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

[-- Attachment #2: 0001-Alsa-hda-add-hdmi_i915-driver-to-fix-dependency.patch --]
[-- Type: application/octet-stream, Size: 9382 bytes --]

From 972098c939e0476ae6dd6bbb84a6ffb5d2293083 Mon Sep 17 00:00:00 2001
From: Wang Xingchao <xingchao.wang@linux.intel.com>
Date: Fri, 3 May 2013 13:57:53 +0800
Subject: [RFC PATCH 1/4] Alsa: hda - add hdmi_i915 driver to fix dependency

This driver used to avoid dependency between ALSA and GFX.

Power well is a good starting point to test this driver.

In alsa side:
- A new hda_bus type is created to track all codecs.
- A new hdmi_i915 driver is registered on the bus too.
- The driver contains private APIs for gfx i915 module, will
add put/get_power_well() in next version.
- In codec driver, codec get its private data, which contains callbacks
about power well request/release.
- the hdmi_i915 has dependency on both CODEC_HDMI and DRM_I915 now, it's
intended to fix the dependency for Haswell platform only now.

Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
---
 sound/Kconfig             |    7 +++++
 sound/Makefile            |    1 +
 sound/hda_bus.c           |   76 +++++++++++++++++++++++++++++++++++++++++++++
 sound/pci/hda/Kconfig     |    7 +++++
 sound/pci/hda/Makefile    |    4 +++
 sound/pci/hda/hda_codec.c |   16 +++++++++-
 sound/pci/hda/hda_codec.h |    8 +++++
 sound/pci/hda/hdmi_i915.c |   49 +++++++++++++++++++++++++++++
 8 files changed, 167 insertions(+), 1 deletion(-)
 create mode 100644 sound/hda_bus.c
 create mode 100644 sound/pci/hda/hdmi_i915.c

diff --git a/sound/Kconfig b/sound/Kconfig
index c710ce2..3aa927d 100644
--- a/sound/Kconfig
+++ b/sound/Kconfig
@@ -132,3 +132,10 @@ config AC97_BUS
 	  sound subsystem and other function drivers completely unrelated to
 	  sound although they're sharing the AC97 bus. Concerned drivers
 	  should "select" this.
+
+# HDA_BUS is used from both sound and drm_i915 
+config HDA_BUS
+	bool "Build HDMI/DisplayPort HD-audio codec support for i915 cards"
+	default y
+	help
+	  This is used to avoid dependency between drm i915 module and hdmi driver
diff --git a/sound/Makefile b/sound/Makefile
index ce9132b..79a5aa3 100644
--- a/sound/Makefile
+++ b/sound/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_SND_AOA) += aoa/
 
 # This one must be compilable even if sound is configured out
 obj-$(CONFIG_AC97_BUS) += ac97_bus.o
+obj-$(CONFIG_HDA_BUS) += hda_bus.o
 
 ifeq ($(CONFIG_SND),y)
   obj-y += last.o
diff --git a/sound/hda_bus.c b/sound/hda_bus.c
new file mode 100644
index 0000000..f8eb8bf
--- /dev/null
+++ b/sound/hda_bus.c
@@ -0,0 +1,76 @@
+/*
+ * Linux driver model hda bus interface
+ *
+ * Author: 	Wang xingchao	
+ * Created:	5/3, 2013	
+ * Copyright:	(C) Intel Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/string.h>
+
+/*
+ * Let drivers decide whether they want to support given codec from their
+ * probe method. Drivers have direct access to the struct snd_ac97
+ * structure and may  decide based on the id field amongst other things.
+ */
+static int hda_bus_match(struct device *dev, struct device_driver *drv)
+{
+	return 1;
+}
+
+#ifdef CONFIG_PM
+static int hda_bus_suspend(struct device *dev, pm_message_t state)
+{
+	int ret = 0;
+
+	if (dev->driver && dev->driver->suspend)
+		ret = dev->driver->suspend(dev, state);
+
+	return ret;
+}
+
+static int hda_bus_resume(struct device *dev)
+{
+	int ret = 0;
+
+	if (dev->driver && dev->driver->resume)
+		ret = dev->driver->resume(dev);
+
+	return ret;
+}
+#endif /* CONFIG_PM */
+
+struct bus_type hda_bus_type = {
+	.name		= "hda",
+	.match		= hda_bus_match,
+#ifdef CONFIG_PM
+	.suspend	= hda_bus_suspend,
+	.resume		= hda_bus_resume,
+#endif /* CONFIG_PM */
+};
+
+static int __init hda_bus_init(void)
+{
+	return bus_register(&hda_bus_type);
+}
+
+subsys_initcall(hda_bus_init);
+
+static void __exit hda_bus_exit(void)
+{
+	bus_unregister(&hda_bus_type);
+}
+
+module_exit(hda_bus_exit);
+
+EXPORT_SYMBOL(hda_bus_type);
+
+MODULE_LICENSE("GPL");
diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig
index 80a7d44..97cde17 100644
--- a/sound/pci/hda/Kconfig
+++ b/sound/pci/hda/Kconfig
@@ -254,4 +254,11 @@ config SND_HDA_POWER_SAVE_DEFAULT
 	  The default time-out value in seconds for HD-audio automatic
 	  power-save mode.  0 means to disable the power-save mode.
 
+config SND_HDA_CODEC_HDMI_I915
+	bool "avoid haswell hdmi and drm i915 dependency"
+	depends on SND_HDA_CODEC_HDMI && DRM_I915
+	default y
+	help 
+	  This driver avoid dependency between drm i915 and hda driver
+
 endif
diff --git a/sound/pci/hda/Makefile b/sound/pci/hda/Makefile
index 24a2514..90aba6d 100644
--- a/sound/pci/hda/Makefile
+++ b/sound/pci/hda/Makefile
@@ -21,6 +21,7 @@ snd-hda-codec-ca0132-objs :=	patch_ca0132.o
 snd-hda-codec-conexant-objs :=	patch_conexant.o
 snd-hda-codec-via-objs :=	patch_via.o
 snd-hda-codec-hdmi-objs :=	patch_hdmi.o hda_eld.o
+snd-hda-codec-hdmi-i915-objs :=	hdmi_i915.o
 
 # common driver
 obj-$(CONFIG_SND_HDA_INTEL) := snd-hda-codec.o
@@ -59,6 +60,9 @@ endif
 ifdef CONFIG_SND_HDA_CODEC_HDMI
 obj-$(CONFIG_SND_HDA_INTEL) += snd-hda-codec-hdmi.o
 endif
+ifdef CONFIG_SND_HDA_CODEC_HDMI_I915
+obj-$(CONFIG_SND_HDA_INTEL) += snd-hda-codec-hdmi-i915.o
+endif
 
 # this must be the last entry after codec drivers;
 # otherwise the codec patches won't be hooked before the PCI probe
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index ecdf30e..04a8ba4 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -787,8 +787,20 @@ static int snd_hda_bus_dev_register(struct snd_device *device)
 	list_for_each_entry(codec, &bus->codec_list, list) {
 		snd_hda_hwdep_add_sysfs(codec);
 		snd_hda_hwdep_add_power_sysfs(codec);
+
+        	codec->dev.bus = &hda_bus_type;
+        	codec->dev.parent = codec->bus->card->dev;
+        	codec->dev.release = codec_device_release;
+        	dev_set_name(&codec->dev, "%d-%d:%s",
+        		     codec->bus->card->number, codec->addr,
+        		     codec->chip_name);
+        	if ((err = device_register(&codec->dev)) < 0) {
+        		snd_printk(KERN_ERR "Can't register hda bus\n");
+        		codec->dev.bus = NULL;
+        		return err;
+        	}
 	}
-	return 0;
+    	return 0;
 }
 #else
 #define snd_hda_bus_dev_register	NULL
@@ -5503,6 +5515,7 @@ int snd_hda_suspend(struct hda_bus *bus)
 		cancel_delayed_work_sync(&codec->jackpoll_work);
 		if (hda_codec_is_power_on(codec))
 			hda_call_codec_suspend(codec, false);
+		//TODO: Get codec private i915 data and release power well
 	}
 	return 0;
 }
@@ -5519,6 +5532,7 @@ int snd_hda_resume(struct hda_bus *bus)
 	struct hda_codec *codec;
 
 	list_for_each_entry(codec, &bus->codec_list, list) {
+		//TODO: Get codec private i915 data and request power well
 		hda_call_codec_resume(codec);
 	}
 	return 0;
diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
index 23ca172..fc6088b 100644
--- a/sound/pci/hda/hda_codec.h
+++ b/sound/pci/hda/hda_codec.h
@@ -920,6 +920,11 @@ struct hda_codec {
 
 	/* additional init verbs */
 	struct snd_array verbs;
+
+	/* device node for hda bus */
+	//TODO: Add hdmi_i915_private pointer
+	struct hdmi_i915_private *i915_data; 
+	struct device dev;
 };
 
 /* direction */
@@ -1195,6 +1200,9 @@ snd_hda_codec_load_dsp_cleanup(struct hda_codec *codec,
 				struct snd_dma_buffer *dmab) {}
 #endif
 
+/* HDA display audio device driver access */
+extern struct bus_type hda_bus_type;
+
 /*
  * Codec modularization
  */
diff --git a/sound/pci/hda/hdmi_i915.c b/sound/pci/hda/hdmi_i915.c
new file mode 100644
index 0000000..ca0916e
--- /dev/null
+++ b/sound/pci/hda/hdmi_i915.c
@@ -0,0 +1,49 @@
+/*
+ * hdmi_i915.c  --  Haswell driver for Display audio and gfx i915 module
+ *
+ * Copyright Intel. Inc 
+ * Author: Wang xingchao <xingchao.wang@intel.com>
+ *
+ *  This program is free software; you can redistribute  it and/or modify it
+ *  under  the terms of  the GNU General  Public License as published by the
+ *  Free Software Foundation;  either version 2 of the  License, or (at your
+ *  option) any later version.
+ */
+
+//TODO:
+//1. Add i915 private APIs here
+//2. register the private data to device side(hda_codec)
+
+struct hdmi_i915_private {
+	//place callbacks here
+};
+
+static struct device_driver hdmi_i915_driver = {
+	.name =		"hdmi-i915",
+	.bus =		&hda_bus_type,
+	.owner =	THIS_MODULE,
+#if 0
+	.probe =	wm97xx_probe,
+	.remove =	wm97xx_remove,
+	.suspend =	wm97xx_suspend,
+	.resume =	wm97xx_resume,
+#endif
+};
+
+static int __init hdmi_i915_init(void)
+{
+	return driver_register(&hdmi_i915_driver);
+}
+
+static void __exit hdmi_i915_exit(void)
+{
+	driver_unregister(&hdmi_i915_driver);
+}
+
+module_init(hdmi_i915_init);
+module_exit(hdmi_i915_exit);
+
+/* Module information */
+MODULE_AUTHOR("Wang xingchao <xingchao.wang@intel.com>");
+MODULE_DESCRIPTION("Haswell Display audio dependency driver");
+MODULE_LICENSE("GPL");
-- 
1.7.9.5


[-- Attachment #3: 0002-Alsa-hda-Add-put-get-power-well-api-in-hdmi_i915-dri.patch --]
[-- Type: application/octet-stream, Size: 3585 bytes --]

From 5dad5e15293db9693b0c23db447062b786495d88 Mon Sep 17 00:00:00 2001
From: Wang Xingchao <xingchao.wang@linux.intel.com>
Date: Fri, 3 May 2013 15:00:32 +0800
Subject: [RFC PATCH 2/4] Alsa: hda - Add put/get power well api in hdmi_i915
 driver

Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
---
 sound/pci/hda/hdmi_i915.c |   54 +++++++++++++++++++++++++++++++++++++--------
 sound/pci/hda/hdmi_i915.h |   28 +++++++++++++++++++++++
 2 files changed, 73 insertions(+), 9 deletions(-)
 create mode 100644 sound/pci/hda/hdmi_i915.h

diff --git a/sound/pci/hda/hdmi_i915.c b/sound/pci/hda/hdmi_i915.c
index ca0916e..c3d8347 100644
--- a/sound/pci/hda/hdmi_i915.c
+++ b/sound/pci/hda/hdmi_i915.c
@@ -9,25 +9,61 @@
  *  Free Software Foundation;  either version 2 of the  License, or (at your
  *  option) any later version.
  */
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/pm.h>
+#include <drm/audio_drm.h>
+#include "hdmi_i915.h"
 
 //TODO:
 //1. Add i915 private APIs here
 //2. register the private data to device side(hda_codec)
 
-struct hdmi_i915_private {
-	//place callbacks here
-};
+static void hdmi_get_power_well(void)
+{
+	request_power_well();
+}
+
+static void hdmi_put_power_well(void)
+{
+	release_power_well();
+}
+
+static int hdmi_i915_probe(struct device *dev)
+{
+	struct hdmi_i915_private *i915_data;
+
+	i915_data = kzalloc(sizeof(*i915_data), GFP_KERNEL);
+	if (!i915_data)
+		return -ENOMEM;
+	i915_data->get_power_well = hdmi_get_power_well;
+	i915_data->put_power_well = hdmi_put_power_well;
+
+	dev_set_drvdata(dev, i915_data);
+
+	return 0;
+}
+
+static int hdmi_i915_remove(struct device *dev)
+{
+	struct hdmi_i915_private *i915_data;
+	
+	i915_data = dev_get_drvdata(dev);
+	kfree(i915_data);
+
+	return 0;
+}
 
 static struct device_driver hdmi_i915_driver = {
 	.name =		"hdmi-i915",
 	.bus =		&hda_bus_type,
 	.owner =	THIS_MODULE,
-#if 0
-	.probe =	wm97xx_probe,
-	.remove =	wm97xx_remove,
-	.suspend =	wm97xx_suspend,
-	.resume =	wm97xx_resume,
-#endif
+	.probe =	hdmi_i915_probe,
+	.remove =	hdmi_i915_remove,
+	//.suspend =	hdmi_i915_suspend,
+	//.resume =	hdmi_i915_resume,
 };
 
 static int __init hdmi_i915_init(void)
diff --git a/sound/pci/hda/hdmi_i915.h b/sound/pci/hda/hdmi_i915.h
new file mode 100644
index 0000000..c48cc49
--- /dev/null
+++ b/sound/pci/hda/hdmi_i915.h
@@ -0,0 +1,28 @@
+/*  hdmi_i915.h - header for hdmi_i915 driver
+ *
+ *  This program is free software; you can redistribute it and/or modify it
+ *  under the terms of the GNU General Public License as published by the Free
+ *  Software Foundation; either version 2 of the License, or (at your option)
+ *  any later version.
+ *
+ *  This program is distributed in the hope that it will be useful, but WITHOUT
+ *  ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ *  FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ *  more details.
+ *
+ *  You should have received a copy of the GNU General Public License along with
+ *  this program; if not, write to the Free Software Foundation, Inc., 59
+ *  Temple Place - Suite 330, Boston, MA  02111-1307, USA.
+ */
+
+#ifndef __SOUND_HDMI_I915_H
+#define __SOUND_HDMI_I915_H
+
+struct hdmi_i915_private {
+	//place callbacks here
+	void (*get_power_well)(void);
+	void (*put_power_well)(void);
+};
+
+#endif /* __SOUND_HDMI_I915_H */
+
-- 
1.7.9.5


[-- Attachment #4: 0003-drm-915-Add-private-api-for-power-well-usage.patch --]
[-- Type: application/octet-stream, Size: 5410 bytes --]

From a6a746b4d4ffb07a7b46458f858292e3f1ca0907 Mon Sep 17 00:00:00 2001
From: Wang Xingchao <xingchao.wang@linux.intel.com>
Date: Fri, 3 May 2013 15:08:19 +0800
Subject: [RFC PATCH 3/4] drm/915: 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 +++++++++++++++++++++
 2 files changed, 105 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_ */
-- 
1.7.9.5


[-- Attachment #5: 0004-Alsa-hda-Add-power-well-API-in-codec-driver.patch --]
[-- Type: application/octet-stream, Size: 1960 bytes --]

From a72d792a39a41bb8f65976b1f542c523b65997bf Mon Sep 17 00:00:00 2001
From: Wang Xingchao <xingchao.wang@linux.intel.com>
Date: Fri, 3 May 2013 15:28:13 +0800
Subject: [RFC PATCH 4/4] Alsa: hda - Add power well API in codec driver

Add get/put power well in codec's suspend/resume callback.

TODO:
As power well also has impact on HDA controller registers, azx_probe()
also need request power well.

Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
---
 sound/pci/hda/hda_codec.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 04a8ba4..7738440 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -39,6 +39,7 @@
 
 #define CREATE_TRACE_POINTS
 #include "hda_trace.h"
+#include "hdmi_i915.h"
 
 /*
  * vendor / preset table
@@ -5510,12 +5511,15 @@ EXPORT_SYMBOL_HDA(snd_hda_add_imux_item);
 int snd_hda_suspend(struct hda_bus *bus)
 {
 	struct hda_codec *codec;
+	struct hdmi_i915_private *i915_data;
 
 	list_for_each_entry(codec, &bus->codec_list, list) {
 		cancel_delayed_work_sync(&codec->jackpoll_work);
 		if (hda_codec_is_power_on(codec))
 			hda_call_codec_suspend(codec, false);
-		//TODO: Get codec private i915 data and release power well
+		i915_data = dev_get_drvdata(&codec->dev);
+		if (i915_data && i915_data->put_power_well)
+			i915_data->put_power_well();
 	}
 	return 0;
 }
@@ -5530,9 +5534,12 @@ EXPORT_SYMBOL_HDA(snd_hda_suspend);
 int snd_hda_resume(struct hda_bus *bus)
 {
 	struct hda_codec *codec;
+	struct hdmi_i915_private *i915_data;
 
 	list_for_each_entry(codec, &bus->codec_list, list) {
-		//TODO: Get codec private i915 data and request power well
+		i915_data = dev_get_drvdata(&codec->dev);
+		if (i915_data && i915_data->get_power_well)
+			i915_data->get_power_well();
 		hda_call_codec_resume(codec);
 	}
 	return 0;
-- 
1.7.9.5


[-- Attachment #6: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [alsa-devel] [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[David Henningsson]

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:
>
> 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

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?

> 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.

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



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[Takashi Iwai]

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

Re: [alsa-devel] [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[Takashi Iwai]

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:
> >
> > 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
> 
> 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?
>
> > 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.
> 
> 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

Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[Wang, Xingchao]

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