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 _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
