Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Apr 26, 2013 at 05:45:15PM +0200, Takashi Iwai wrote:
> At Fri, 26 Apr 2013 17:42:07 +0200,
> Daniel Vetter wrote:
> > 
> > On Fri, Apr 26, 2013 at 05:12:34PM +0200, Takashi Iwai wrote:
> > > At Fri, 26 Apr 2013 16:57:08 +0200,
> > > Daniel Vetter wrote:
> > > > 
> > > > On Fri, Apr 26, 2013 at 07:53:41AM +0000, Li, Jocelyn wrote:
> > > > > -----Original Message-----
> > > > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] 
> > > > > Sent: Friday, April 26, 2013 3:25 PM
> > > > > To: Li, Jocelyn
> > > > > Cc: Wang, Xingchao; Zanoni, Paulo R; ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Wang Xingchao; Takashi Iwai; Barnes, Jesse; Wysocki, Rafael J; Hindman, Gavin
> > > > > Subject: Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team
> > > > > Once we've figured out what needs to be changed in the audio driver we
> > > > > can look at the entire patch series and the interface to i915.ko.
> > > > > That's also why I didn't comment on Xingchao's patch right away, but
> > > > > only once he specifically asked for feedback, since doing a real review
> > > > > of the interface doesn't make sense until we have all the pieces (and a
> > > > > working audio driver).
> > > > > 
> > > > > [Jocelyn] I think you have made constructive comments on Xingchao's
> > > > > patch yesterday. Next, shall we have Xingchao improve his patch? Or we
> > > > > just have Xingchao wait till you have completed your pieces. Sorry, I am
> > > > > a little confused :)
> > > > 
> > > > I think the next step is to use Xinchao's patch as-is and get the audio
> > > > side going. Once we have that fixed up, we can revisit the interface and
> > > > make it solid. But for now trying to polish this relatively simple part
> > > > seems like wasted time.
> > > > 
> > > > Also, reviewing an interface is much easier if we have both the i915
> > > > pieces (already here) and the audio pieces (which I haven't seen yet)
> > > > avaialble.
> > > 
> > > I haven't checked the patch properly yet, but the patch pasted in the
> > > post looks like i915 driver exports the functions to control power
> > > well, and let audio driver calling them.  If so, the big mess in such
> > > a case is the dependency between driver modules.
> > > 
> > > A simple workaround would be to split this function and the relevant
> > > instance out of i915 and snd-hda-intel and put into an individual
> > > module (e.g. i915-powerwell-ctl).
> > 
> > Yeah, the current patch doesn't work for loading i915/snd-hda-intel in any
> > order. But it should be good enough to fix the hw interactions.
> 
> Well, my current concern is that the call is built into snd-hda-intel
> module and this will lead to load i915 module no matter whether it's
> used or not.  snd-hda-intel is used for any HD-audio controllers
> (despite its name).  Loading i915 on a pure AMD system will annoy some
> non-Intel people :)
> 
> Even if we built the code via ifdef CONFIG_DRM_I915 or such, it's
> still hardcoded, thus the problem above will occur with distro
> kernels.

Sure, the current patch won't cut it for upstreaming. But it should be
good enough to figure out what exactly we need (which to still be a bit
unclear). Or at least where exactly we need it and what kind of other
changes in snd-hda-intel are required to get things going.

> > > Also, it would be feasible to implement some PM governor over both
> > > graphics and audio, that is, it gives the proper serialization of
> > > power up for audio controller, for example.
> > 
> > Yeah, I think once we have the hw interaction/sequence and stuff all
> > figured out we need to figure out what kind of interface would suit best
> > here. One of the options we've talked about a bit is a full runtime PM
> > governor on a special device. Then only the device needs to be
> > instantiated first, the audio driver could just grab runtime pm references
> > and i915 could implement the PM backend.
> > 
> > But like I've said, those considerations should be a second step once we
> > have something working with a quickly hacked-together interface.
> 
> Yep, I agree that we'll need a quick fix at this stage.

power well stuff is disabled by default, 3.10 is kinda done already and we
still have plenty of time to hit anything for 3.11. So I don't think we
should rush the solution for upstream before it's clear what snd-hda-intel
precisely needs.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch