[alsa-devel] [PATCH 2/7] ALSA: hda: Refactor display power management

[Takashi Iwai]

On Tue, 11 Dec 2018 14:58:37 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> On 12/11/18 12:54 AM, Takashi Iwai wrote:
> > On Mon, 10 Dec 2018 21:52:05 +0100,
> > Pierre-Louis Bossart wrote:
> >>
> >> On 12/9/18 3:33 AM, Takashi Iwai wrote:
> >>> The current HD-audio code manages the DRM audio power via too complex
> >>> redirections, and this seems even still unbalanced in a corner case as
> >>> Intel DRM CI has been intermittently reporting.  This patch is a big
> >>> surgery for addressing the complexity and the possible unbalance.
> >>>
> >>> Basically the patch changes the display PM in the following ways:
> >>>
> >>> - Both HD-audio controller and codec drivers call a single helper,
> >>>     snd_hdac_display_power().  (Formerly, the display power control from
> >>>     a codec was done indirectly via link_power bus ops.)
> >>>
> >>> - snd_hdac_display_power() receives the codec address index.  For
> >>>     turning on/off from the controller, pass HDA_CODEC_IDX_CONTROLLER.
> >> The need for this virtual index==16 isn't fully clear to me,
> >> especially if you use the bitfields instead of reference counts.
> >>
> >> Isn't there a risk of the controller setting the bit16 to zero, but
> >> you still have bit4 on (assuming the idx is 4). If you use this
> >> virtual index, it should override the actual physical bits when
> >> set/cleared.
> > This is the index for a controller, i.e. we'd need bits for the max
> > number of codecs + 1.
> >
> > Actually we do support only up to 8 codecs (HDA_MAX_CODECS), so it
> > should be 8, instead of 16, too.  I'll update to be HDA_MAX_CODECS.
> >
> >> Or is this meant to actually implement a preemption mechanism, where
> >> the display power remains on for as long as the controller wishes,
> >> regardless of what the patch_hdmi and hdac_hdmi code requests?
> > Right.  That's the mechanism at the initial phase, we need the display
> > power on while probing the codec, i.e. before identifying the codec
> > ID.
> >
> >> Also don't we already have the HDMI codec address already after the
> >> probe, so couldn't we provide the address directly?
> > The resume seemed requiring the controller to take the display power
> > at first, so the same mechanism is used.
> 
> ok, makes sense, thanks for the explanations.
> 
> So I guess for the SOF patches, the only change would be to add the
> second argument HDA_CODEC_IDX_CONTROLLER to snd_hdac_display_power()
> calls, the rest looks unchanged or hidden inside the hdac library or
> hdac_hdmi parts.

Yes, other than that, this change makes things easier.

Since we don't manage with refcount, the only important point is to
turn off/on properly at suspend/resume (also off at remove), no matter
how many times it gets called.


thanks,

Takashi