[alsa-devel] [RFC PATCH 1/3] drm/915: Add private api for power well usage
Takashi Iwai
tiwai at suse.de
Thu May 9 19:17:36 CEST 2013
At Thu, 9 May 2013 11:23:21 +0200,
Daniel Vetter wrote:
>
> 2. On a quick read of the hda driver stuff I don't think the power
> well enable/disable places are at the right spot: We force the power
> well on whenever the hda codec is used, but we must only do that when
> we want to output audio to external screens. And since that state
> transition can only really happen due to a modeset change on the gfx
> side I don't think we need any autosuspend delays either.
>
> The use-case I'm thinking of is media playback with just the panel
> enabled: In that case we can switch off the power well on the display
> side, and I don't think the power well is required for audio play back
> on the integrated speakerrs/headphone-jack either.
Well, in the case of Haswell, the audio controller/codec is dedicated
to only HDMI audio, and in the audio driver level, the power saving of
each codec/controller chip is managed individually. So, it's not that
bad to handle power well on/off at that point. A sane power-conscious
OS would open/close the audio device file in a fine grained way.
> 3. The moduel depencies seem to still not work out dynamically, at
> least I think we still have a hard chain between hda-intel and i915.ko
> (just with one thing in between now).
True.
The question is in which level we need power_well_*() controls.
If the initialization of HD-audio controller (e.g. PCI registers)
requires the power well on, hda_intel.c requires the calls, as this
patch does. OTOH, if it's only about the HD-audio verbs, basically we
can push the power well calls into the codec driver,
i.e. patch_hdmi.c. In the latter case, as David once suggested, we
can split the Haswell codec driver from patch_hdmi.c so that only the
new driver depends on i915.
thanks,
Takashi
>
> Cheers, Daniel
>
>
>
> On Thu, May 9, 2013 at 9:23 AM, Wang, Xingchao <xingchao.wang at intel.com> wrote:
> > Hi All,
> >
> > This is a newer version patchset for power well feature implementation.
> >
> > I will send out a document later to describe the design, especially for David and Takashi as they could not see Bspec
> > so it maybe a bit confuse. Meanwhile Liam will send out a meeting request for discussion, welcome Daniel, Paulo, Jesse to join us.
> >
> > Basically, here's the latest working status on my side:
> >
> > I tested the power well feature on Haswell ULT board, there's only one eDP port used.
> > Without this patch and we enabled power well feature, gfx driver will shutdown power well, audio driver will crash.
> > Applied this patch, display audio driver will request power well on before initialize the card. In gfx side, it will enable power well.
> >
> > Also power_save feature in audio side should be enabled, I set "5" second to let codec enter suspend when free for 5s long.
> > Audio controller driver detects all codecs are suspended it will release power well and suspend too. Gfx driver will receive the request and shut down power well.
> >
> > If user trigger some audio operations(cat codec# info), audio controller driver will request power well again...
> >
> > If gfx side decided to disable power well now, while audio is in use, power well would be kept on.
> >
> > Generally audio drvier will request power well on need and release it immediately after suspend. Gfx should make decision at his side.
> >
> > The new introduced module "hda-i915" has dependency on i915, only built in when i915 enabled. So it's safe for call.
> > This module was based on David's original patch, which added pm_suspend/resume callback for hdmi codec. As the codecs and controller are both depending on powerwell,
> > and codec *must* already on power so I moved the power well request/release to controller side and changed the module name to "hda-i915".
> > David, would you like me to add you as author for the second patch? :)
> >
> > For non-Haswell platform, power well is no need atm. If the module is built in and gfx will reject power well request at its side, so it's safe too.
> >
> > thanks
> > --xingchao
> >
> >
> >> -----Original Message-----
> >> From: Wang Xingchao [mailto:xingchao.wang at linux.intel.com]
> >> Sent: Thursday, May 09, 2013 3:01 PM
> >> To: david.henningsson at canonical.com; Girdwood, Liam R; tiwai at suse.de;
> >> daniel at ffwll.ch
> >> Cc: Barnes, Jesse; Li, Jocelyn; Lin, Mengdong; Wang, Xingchao; Zanoni, Paulo R;
> >> Wang Xingchao
> >> Subject: [RFC PATCH 1/3] drm/915: Add private api for power well usage
> >>
> >> Haswell 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 at linux.intel.com>
> >> ---
> >> drivers/gpu/drm/i915/intel_pm.c | 127
> >> ++++++++++++++++++++++++++++++++++++---
> >> include/drm/audio_drm.h | 39 ++++++++++++
> >> 2 files changed, 159 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..cba753e 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,125 @@ 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;
> >> +/* Lock protecting power well setting */
> >> +DEFINE_SPINLOCK(powerwell_lock); static bool i915_power_well_using;
> >> +
> >> +struct power_well {
> >> + int user_cnt;
> >> + struct list_head power_well_list;
> >> +};
> >> +
> >> +static struct power_well hsw_power = {
> >> + .user_cnt = 0,
> >> + .power_well_list = LIST_HEAD_INIT(hsw_power.power_well_list),
> >> +};
> >> +
> >> +struct power_well_user {
> >> + char name[16];
> >> + struct list_head list;
> >> +};
> >> +
> >> +void i915_request_power_well(const char *name) {
> >> + struct power_well_user *user;
> >> +
> >> + DRM_DEBUG_KMS("request power well for %s\n", name);
> >> + spin_lock_irq(&powerwell_lock);
> >> + if (!power_well_device)
> >> + goto out;
> >> +
> >> + if (!IS_HASWELL(power_well_device))
> >> + goto out;
> >> +
> >> + list_for_each_entry(user, &hsw_power.power_well_list, list) {
> >> + if (!strcmp(user->name, name)) {
> >> + goto out;
> >> + }
> >> + }
> >> +
> >> + user = kzalloc(sizeof(*user), GFP_KERNEL);
> >> + if (user == NULL) {
> >> + goto out;
> >> + }
> >> + strcpy(user->name, name);
> >> +
> >> + list_add(&user->list, &hsw_power.power_well_list);
> >> + hsw_power.user_cnt++;
> >> +
> >> + if (hsw_power.user_cnt == 1) {
> >> + set_power_well(power_well_device, true);
> >> + DRM_DEBUG_KMS("%s set power well on\n", user->name);
> >> + }
> >> +out:
> >> + spin_unlock_irq(&powerwell_lock);
> >> + return;
> >> +}
> >> +EXPORT_SYMBOL_GPL(i915_request_power_well);
> >> +
> >> +void i915_release_power_well(const char *name) {
> >> + struct power_well_user *user;
> >> +
> >> + DRM_DEBUG_KMS("release power well from %s\n", name);
> >> + spin_lock_irq(&powerwell_lock);
> >> + if (!power_well_device)
> >> + goto out;
> >> +
> >> + if (!IS_HASWELL(power_well_device))
> >> + goto out;
> >> +
> >> + list_for_each_entry(user, &hsw_power.power_well_list, list) {
> >> + if (!strcmp(user->name, name)) {
> >> + list_del(&user->list);
> >> + hsw_power.user_cnt--;
> >> + DRM_DEBUG_KMS("Releaseing power well:%s, user_cnt:%d,
> >> i915 using:%d\n",
> >> + user->name, hsw_power.user_cnt,
> >> i915_power_well_using);
> >> + if (!hsw_power.user_cnt && !i915_power_well_using)
> >> + set_power_well(power_well_device, false);
> >> + goto out;
> >> + }
> >> + }
> >> +
> >> + DRM_DEBUG_KMS("power well %s doesnot exist\n", name);
> >> +out:
> >> + spin_unlock_irq(&powerwell_lock);
> >> + return;
> >> +}
> >> +EXPORT_SYMBOL_GPL(i915_release_power_well);
> >> +
> >> +/* TODO: Call this when i915 module unload */ void
> >> +remove_power_well(void) {
> >> + spin_lock_irq(&powerwell_lock);
> >> + 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 && hsw_power.user_cnt) {
> >> + DRM_DEBUG_KMS("Display audio power well busy using now\n");
> >> + 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..215295f
> >> --- /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 at intel.com> */
> >> +
> >> +#ifndef _AUDIO_DRM_H_
> >> +#define _AUDIO_DRM_H_
> >> +
> >> +extern void i915_request_power_well(const char *name); extern void
> >> +i915_release_power_well(const char *name);
> >> +
> >> +#endif /* _AUDIO_DRM_H_ */
> >> --
> >> 1.7.9.5
> >
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
More information about the Alsa-devel
mailing list