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
