On Mon, 06 May 2019 11:25:16 +0200, Yang, Libin wrote: > > Hi Jaroslav, > > >-----Original Message----- > >From: Jaroslav Kysela [mailto:perex@xxxxxxxx] > >Sent: Monday, May 6, 2019 5:04 PM > >To: Yang, Libin <libin.yang@xxxxxxxxx>; alsa-devel@xxxxxxxxxxxxxxxx > >Cc: tiwai@xxxxxxx; pierre-louis.bossart@xxxxxxxxxxxxxxx; broonie@xxxxxxxxxx; > >Hui Wang <hui.wang@xxxxxxxxxxxxx>; Lin, Mengdong > ><mengdong.lin@xxxxxxxxx>; Wang, Rander <rander.wang@xxxxxxxxx> > >Subject: Re: [RFC PATCH] ASoC: codec: hdac_hdmi: no checking > >monitor in hw_params > > > >Dne 06. 05. 19 v 10:46 Yang, Libin napsal(a): > >> Add Mengdong, Hui, Rander > >> > >> Hi Jaroslav, > >> > >>> -----Original Message----- > >>> From: Jaroslav Kysela [mailto:perex@xxxxxxxx] > >>> Sent: Monday, May 6, 2019 4:20 PM > >>> To: Yang, Libin <libin.yang@xxxxxxxxx>; alsa-devel@xxxxxxxxxxxxxxxx > >>> Cc: tiwai@xxxxxxx; pierre-louis.bossart@xxxxxxxxxxxxxxx; > >>> broonie@xxxxxxxxxx; subhransu.s.prusty@xxxxxxxxx; > >>> samreen.nilofer@xxxxxxxxx > >>> Subject: Re: [RFC PATCH] ASoC: codec: hdac_hdmi: no > >>> checking monitor in hw_params > >>> > >>> Dne 06. 05. 19 v 8:59 libin.yang@xxxxxxxxx napsal(a): > >>>> From: Libin Yang <libin.yang@xxxxxxxxx> > >>>> > >>>> This patch move the check of monitor from hw_params to trigger callback. > >>>> > >>>> The original code will check the monitor presence in hw_params. If > >>>> the monitor doesn't exist, hw_params will return -ENODEV. Mostly this is > >OK. > >>>> > >>>> However, pulseaudio will check the pcm devices when kernel is booting > >up. > >>>> It will try to open, set hw_params, prepare such pcm devices. We > >>>> can't guarantee that the monitor will be connected when kernel is booting > >up. > >>>> Especially, hdac_hdmi will export 3 pcms at most. It's hard to say > >>>> users will connect 3 monitors to the HDMI/DP ports. This will cause > >>>> pulseaudio fail in parsing the pcm devices because the driver will > >>>> return -ENODEV in hw_params. > >>>> > >>>> This patch tries to move the check of monitor presence into trigger > >>>> callback. This can "trick" the pulseaudio the pcm is ready. > >>>> > >>>> This bug is found when we try to enable HDMI detection in > >>>> gnome-sound-setting for ASoC hdac_hdmi. After we enable the hdmi in > >>>> UCM, pulseaudio will try to parse the hdmi pcm devices. It will > >>>> cause failure if there are no monitors connected. > >>> > >>> I don't like this solution much. PA should use the Jack control to > >>> add the devices dynamically and avoid probing when the Jack control is > >false. > >> > >> Before we decided to use UCM, we did some investigation on Jack controls. > >> And we found we need do much more changes in driver to support Jack > >> Controls. > > > >How do you handle the dynamic monitor configuration (like when user > >disconnects the monitor on-the-fly) then? The control interface can notify the > >state change through the Jack controls. The PCM interface does not handle > >this. > > We may still need Jack control for this case. This is my second plan to enable > Jack control. But even we enable Jack control, it seems it is still easy to use UCM. > This is because ASoC control names don't follow legacy HDA rule. This means > we may need to change other controls name besides HDMI in the driver or we > may need to add more configures in pulseaudio/alsa-mixer/paths. Ah, I overlooked that hdac_hdmi chose the own jack control names. That's basically useless, yeah. > Hi Mengdong & Hui, > > Do you have more comments for Jack control or UCM? I guess a UCM profile would handle the jack detection as well, by specifying the right kcontrol names, right? thanks, Takashi > > Regards, > Libin > > > > > Jaroslav > > > >> > >> Regards, > >> Libin > >> > >>> > >>> Jaroslav > >>> > >>>> > >>>> Signed-off-by: Libin Yang <libin.yang@xxxxxxxxx> > >>>> --- > >>>> sound/soc/codecs/hdac_hdmi.c | 44 > >>>> +++++++++++++++++++++++++++++++------------- > >>>> 1 file changed, 31 insertions(+), 13 deletions(-) > >>>> > >>>> diff --git a/sound/soc/codecs/hdac_hdmi.c > >>>> b/sound/soc/codecs/hdac_hdmi.c index 4de1fbf..f482e09 100644 > >>>> --- a/sound/soc/codecs/hdac_hdmi.c > >>>> +++ b/sound/soc/codecs/hdac_hdmi.c > >>>> @@ -455,24 +455,11 @@ static int hdac_hdmi_set_hw_params(struct > >>> snd_pcm_substream *substream, > >>>> struct snd_pcm_hw_params *hparams, struct snd_soc_dai *dai) { > >>>> struct hdac_hdmi_priv *hdmi = snd_soc_dai_get_drvdata(dai); > >>>> - struct hdac_device *hdev = hdmi->hdev; > >>>> struct hdac_hdmi_dai_port_map *dai_map; > >>>> - struct hdac_hdmi_port *port; > >>>> struct hdac_hdmi_pcm *pcm; > >>>> int format; > >>>> > >>>> dai_map = &hdmi->dai_map[dai->id]; > >>>> - port = dai_map->port; > >>>> - > >>>> - if (!port) > >>>> - return -ENODEV; > >>>> - > >>>> - if ((!port->eld.monitor_present) || (!port->eld.eld_valid)) { > >>>> - dev_err(&hdev->dev, > >>>> - "device is not configured for this pin:port%d:%d\n", > >>>> - port->pin->nid, port->id); > >>>> - return -ENODEV; > >>>> - } > >>>> > >>>> format = snd_hdac_calc_stream_format(params_rate(hparams), > >>>> params_channels(hparams), > >>> params_format(hparams), @@ -630,6 > >>>> +617,36 @@ static void hdac_hdmi_pcm_close(struct > >snd_pcm_substream > >>> *substream, > >>>> dai_map->port = NULL; > >>>> } > >>>> > >>>> +static int hdac_hdmi_pcm_trigger(struct snd_pcm_substream > >>>> +*substream, > >>> int cmd, > >>>> + struct snd_soc_dai *dai) > >>>> +{ > >>>> + struct hdac_hdmi_port *port; > >>>> + struct hdac_hdmi_dai_port_map *dai_map; > >>>> + struct hdac_hdmi_priv *hdmi = snd_soc_dai_get_drvdata(dai); > >>>> + struct hdac_device *hdev = hdmi->hdev; > >>>> + > >>>> + /* > >>>> + * When start, if there is no monitor, > >>>> + * It should not start audio. > >>>> + */ > >>>> + if (cmd == SNDRV_PCM_TRIGGER_START) { > >>>> + dai_map = &hdmi->dai_map[dai->id]; > >>>> + port = dai_map->port; > >>>> + > >>>> + if (!port) > >>>> + return -ENODEV; > >>>> + > >>>> + if ((!port->eld.monitor_present) || (!port->eld.eld_valid)) { > >>>> + dev_err(&hdev->dev, > >>>> + "device is not configured for this > >>> pin:port%d:%d\n", > >>>> + port->pin->nid, port->id); > >>>> + return -ENODEV; > >>>> + } > >>>> + } > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> static int > >>>> hdac_hdmi_query_cvt_params(struct hdac_device *hdev, struct > >>>> hdac_hdmi_cvt *cvt) { @@ -1389,6 +1406,7 @@ static const struct > >>>> snd_soc_dai_ops hdmi_dai_ops = { > >>>> .startup = hdac_hdmi_pcm_open, > >>>> .shutdown = hdac_hdmi_pcm_close, > >>>> .hw_params = hdac_hdmi_set_hw_params, > >>>> + .trigger = hdac_hdmi_pcm_trigger, > >>>> .set_tdm_slot = hdac_hdmi_set_tdm_slot, }; > >>>> > >>>> > >>> > >>> > >>> -- > >>> Jaroslav Kysela <perex@xxxxxxxx> > >>> Linux Sound Maintainer; ALSA Project; Red Hat, Inc. > > > > > >-- > >Jaroslav Kysela <perex@xxxxxxxx> > >Linux Sound Maintainer; ALSA Project; Red Hat, Inc. _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
