[Intel-gfx] [RFC PATCH 1/3] drm/915: Add private api for power well usage
Wang, Xingchao
xingchao.wang at intel.com
Fri May 10 06:47:55 CEST 2013
Hi Takashi,
> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai at suse.de]
> Sent: Friday, May 10, 2013 1:18 AM
> To: Daniel Vetter
> Cc: Wang, Xingchao; david.henningsson at canonical.com; Girdwood, Liam R;
> Barnes, Jesse; Li, Jocelyn; Lin, Mengdong; Zanoni, Paulo R; Wang Xingchao;
> intel-gfx; alsa-devel at alsa-project.org
> Subject: Re: [RFC PATCH 1/3] drm/915: Add private api for power well usage
>
> 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.
The power well has impact on both HD-Audio controller and hdmi codec, they share the same power well.
In the initial version patch, both controller and codecs would request power-well, but for hdmi-codec is unnecessary
as HD-audio controller must have power already, otherwise it's crashed, so I removed that, only HD-audio controller request power-well on demand.
That's why there's some changes based on David's patch.
Thanks
--xingchao
>
>
> 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 Intel-gfx
mailing list