Hi Daniel,
-----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.
Well it's good news you have plan to add power well API support in graphic side, then it's easy for audio driver to use. I donot know much background about your idea and current status of the power well integration status there so it's better to avoid duplicated 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).
Indeed it was my initial idea to use reference counter to track "power well" users. A second thinking is It become complex as i915 did not call intel_set_power_well(enable/disable) symmetrically. It may enable Power well several times but disable it only once. That's why I just use a basic logic to track the "power well" usage.
- 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.
Thanks your review, i will change it in next versioin patch.
Thanks --xingchao