Hi Takashi, >-----Original Message----- >From: Takashi Iwai [mailto:tiwai@xxxxxxx] >Sent: Monday, May 6, 2019 4:40 PM >To: Jaroslav Kysela <perex@xxxxxxxx> >Cc: Yang, Libin <libin.yang@xxxxxxxxx>; alsa-devel@xxxxxxxxxxxxxxxx; 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 > >On Mon, 06 May 2019 10:19:48 +0200, >Jaroslav Kysela wrote: >> >> 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. > >Ideally, yes, but this isn't going to happen soon, I'm afraid. And we're still >responsible for fixing for the existing platforms. So I find the proposed patch >OK although it's hackish. The added code in the trigger has almost no >overhead, and it won't break stuff. Yes, this is why we want to use UCM. We need more changes in the driver to use Jack control. I have another question: Is it possible that we remove the monitor detection? I mean just returning ok even there are no monitors? There is one case: playback -> disconnect monitor -> S3. As it will trigger start after S3, my patch will cause playback to exit as it will check the monitors. The original code will not break playback in this case. Regards, Libin > > >thanks, > >Takashi > >> >> 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. >> _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
