[alsa-devel] [PATCH] drm/i915: Add private api for power well usage
Wang, Xingchao
xingchao.wang at intel.com
Thu Apr 25 04:36:57 CEST 2013
Hi Daniel/Paulo,
Any comments on this?
Add Jesse and Rafael in loop.
thanks
--xingchao
> -----Original Message-----
> From: Wang, Xingchao
> Sent: Wednesday, April 24, 2013 3:29 PM
> To: daniel.vetter at ffwll.ch; Zanoni, Paulo R; 'Takashi Iwai'
> Cc: ville.syrjala at linux.intel.com; Lin, Mengdong; Girdwood, Liam R; Li, Jocelyn;
> intel-gfx at lists.freedesktop.org; alsa-devel at 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 at linux.intel.com]
> > Sent: Thursday, April 24, 2014 10:20 PM
> > To: daniel.vetter at ffwll.ch; Zanoni, Paulo R
> > Cc: ville.syrjala at linux.intel.com; Lin, Mengdong; Girdwood, Liam R;
> > Li, Jocelyn; intel-gfx at 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 at 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 at 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
More information about the Alsa-devel
mailing list