Hi Takashi, >-----Original Message----- >From: Takashi Iwai [mailto:tiwai@xxxxxxx] >Sent: Monday, March 25, 2019 6:08 PM >To: Yang, Libin <libin.yang@xxxxxxxxx> >Cc: alsa-devel@xxxxxxxxxxxxxxxx; broonie@xxxxxxxxxx >Subject: Re: [PATCH V2] ASoC: fix hdmi codec driver contest in S3 > >On Mon, 25 Mar 2019 07:51:11 +0100, >Yang, Libin wrote: >> >> Hi Takashi, >> >> >-----Original Message----- >> >From: Takashi Iwai [mailto:tiwai@xxxxxxx] >> >Sent: Saturday, March 23, 2019 4:26 AM >> >To: Yang, Libin <libin.yang@xxxxxxxxx> >> >Cc: alsa-devel@xxxxxxxxxxxxxxxx; broonie@xxxxxxxxxx >> >Subject: Re: [PATCH V2] ASoC: fix hdmi codec driver >> >contest in S3 >> > >> >On Fri, 22 Mar 2019 12:46:25 +0100, >> >Yang, Libin wrote: >> >> >> >> Hi Takashi, >> >> >> >> >> > >> >> >> >On Fri, 22 Mar 2019 07:24:56 +0100, libin.yang@xxxxxxxxx wrote: >> >> >> >> >> >> >> >> From: Libin Yang <libin.yang@xxxxxxxxx> >> >> >> >> >> >> >> >> In S3, there is a contest between dapm event and getting >> >> >> >> display >> >power. >> >> >> >> >> >> >> >> When resume, the sequence is: >> >> >> >> >> >> >> >> hdac_hdmi_runtime_resume() >> >> >> >> => snd_hdac_display_power() >> >> >> >> hdac_hdmi_xxx_widget_event() >> >> >> >> >> >> >> >> The hdac_hdmi_xxx_widget_event() are based on the power being >on. >> >> >> >> Otherwise, the operation on the codec register will fail. >> >> >> >> >> >> >> >> However, in snd_hdac_display_power(), it may sleep because of >> >> >> >> the >> >> >> >> mutex_lock() in display driver. In this case, >> >> >> >> hdac_hdmi_xxx_widget_event() will be called before the power is >on. >> >> >> >> >> >> >> >> Let's not operate the registers and wait for the power in >> >> >> >> hdac_hdmi_xxx_widget_event() >> >> >> >> >> >> >> >> Signed-off-by: Libin Yang <libin.yang@xxxxxxxxx> >> >> >> > >> >> >> >IMO the workaround looks a bit too fragile. The substantial >> >> >> >problem is the race between codec and DAPM. Can this be >> >> >> >controlled better, e.g. by defining the proper PM dependency? >> >> >> >I thought we may rearrange the PM topology dynamically. >> >> >> >> >> >> It seems codec and DAPM is OK so far. Codec resume will be >> >> >> called firstly and then DAPM. But in HDMI codec runtime resume, >> >> >> it will call snd_hdac_display_power(). This function will try to get a >mutex_lock. >> >> >> If it fails to get the lock, it will sleep. This will cause dapm >> >> >> event handler to be called before audio power domain in display >> >> >> is on in the HDMI >> >> >driver. >> >> > >> >> >It implies that the calls are racy. If they have to be called in >> >> >some order, they have to be serialized in a more better way than >> >> >inserting >> >a random delay... >> >> >> >> Yes. The code does seem to be misleading. What do you think of >> >> using something like "wait_for_complete()"? This will be more readable. >> > >> >Well, maybe we should create a proper device link for assuring the PM >> >dependency instead? Then PM core will take care of everything like >> >that ugly stuff. >> >> Thanks for the suggestion. I did some investigation based on your >> suggestion today. Below is the result on my sof platform: >> There are 3 devices we concerned in this case: >> machine_device: this is the device of the machine, its parent is >> pci_device >> hdmi_device: this is the device of the hdmi codec, its parent is >> pci_device >> pci_device: this is the device of the pci >> >> When resume, pci_device will be resumed firstly, and then >> machine_device and hdmi_device. >> When machine_device is resumed, it will schedule a work to call the >> xxx_event(). So the xxx_event() will be run later. >> When hdmi_device is resumed, it will try to turn on display power. >> However, it may sleep if it can't get the mutex_lock. >> >> If hdmi_device sleeps, xxx_event() will be called firstly. However, >> this is wrong, because xxx_event() depends on the display power. >> >> So if we want to make sure machine_device resume is called always >> after hdmi_device resume, we must set hdmi_device being the parent >machine_device. >> This seems a little weird because codecs are different for different platforms. >> And what's more, one platform may have several codecs. It's hard to >> say which codec should be the parent. > >Ah, it's an async work. Then the device link won't work as expected. >(FWIW, you can define the PM dependency and call order via > device_link_add() calls even if they are no device parent/child relationship. I >thought it would work in this case, but it's effective only for the direct >invocation from the PM callbacks.) > >> There is another solution I can figure out. We can do power on display >> power in each xxx_event() manually. However, this may not be a very good >solution. >> There are several xxx_event() functions. So each time power on, the >> display power will be turned on several time. >> >> As we can make sure that display power is turned on already or will be >> turned on very soon in the future. What about we just wait for >> completion of the display power on? > >I don't think that's the overhead, rather it's the right solution. >You just need to assure the runtime PM via wrapping >pm_runtime_get_sync() and the counterpart in each place where you need >the display power in an async code path. OK. I will try to get display power in each xxx_event(). I tried to call pm_runtime_get_sync() in xxx_event() to turn on the display power before. As it's already in the resume process, the runtime PM will not be called again. So it doesn't help. It seems I have to call snd_hdac_display_power() manually. Another potential issue is if I called snd_hdac_display_power(on) in xxx_event(), I also need call snd_hdac_display_power(off) at the end of xxx_event()? If so, the display power may really be turned off as currently snd_hdac_display_power() doesn't use counter, but itis using " set_bit(idx, &bus->display_power_status)" and " clear_bit(idx, &bus->display_power_status)" to track the power. Let's assume such scenario: Runtime pm turns on display power Xxx_event() turns on the display power Xxx_event() turns off the display power Now display power may be really turned off. But actually we don't want to turn off display power off here. This will be wrong. Regards, Libin > > >thanks, > >Takashi > >> >> Regards, >> Libin >> >> > >> > >> >Takashi >> > >> >> >> >> Regards, >> >> Libin >> >> >> >> > >> >> > >> >> >thanks, >> >> > >> >> >Takashi >> >> > >> >> >> >> >> >> Regards, >> >> >> Libin >> >> >> >> >> >> > >> >> >> > >> >> >> >thanks, >> >> >> > >> >> >> >Takashi >> >> >> > >> >> >> >> --- >> >> >> >> sound/soc/codecs/hdac_hdmi.c | 36 >> >> >> >> ++++++++++++++++++++++++++++++++++++ >> >> >> >> 1 file changed, 36 insertions(+) >> >> >> >> >> >> >> >> diff --git a/sound/soc/codecs/hdac_hdmi.c >> >> >> >> b/sound/soc/codecs/hdac_hdmi.c index 5eeb0fe..3c5c122 100644 >> >> >> >> --- a/sound/soc/codecs/hdac_hdmi.c >> >> >> >> +++ b/sound/soc/codecs/hdac_hdmi.c >> >> >> >> @@ -144,6 +144,7 @@ struct hdac_hdmi_priv { >> >> >> >> int num_pin; >> >> >> >> int num_cvt; >> >> >> >> int num_ports; >> >> >> >> + int display_power; /* 0: power off; 1 power on */ >> >> >> >> struct mutex pin_mutex; >> >> >> >> struct hdac_chmap chmap; >> >> >> >> struct hdac_hdmi_drv_data *drv_data; @@ -742,12 +743,27 >> >@@ >> >> >> >static >> >> >> >> void hdac_hdmi_set_amp(struct hdac_device *hdev, >> >> >> >> >> > AC_VERB_SET_AMP_GAIN_MUTE, >> >> >> >val); } >> >> >> >> >> >> >> >> +static int wait_for_display_power(struct hdac_hdmi_priv *hdmi) { >> >> >> >> + int i = 0; >> >> >> >> + >> >> >> >> + /* sleep for totally 80ms ~ 200ms should be enough */ >> >> >> >> + while (i < 40) { >> >> >> >> + if (!hdmi->display_power) >> >> >> >> + usleep_range(2000, 5000); >> >> >> >> + else >> >> >> >> + return 0; >> >> >> >> + i++; >> >> >> >> + } >> >> >> >> + return -EAGAIN; >> >> >> >> +} >> >> >> >> >> >> >> >> static int hdac_hdmi_pin_output_widget_event(struct >> >> >> >snd_soc_dapm_widget *w, >> >> >> >> struct snd_kcontrol *kc, int >> >event) { >> >> >> >> struct hdac_hdmi_port *port = w->priv; >> >> >> >> struct hdac_device *hdev = dev_to_hdac_dev(w->dapm->dev); >> >> >> >> + struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); >> >> >> >> struct hdac_hdmi_pcm *pcm; >> >> >> >> >> >> >> >> dev_dbg(&hdev->dev, "%s: widget: %s event: %x\n", @@ - >> >757,6 >> >> >> >+773,11 >> >> >> >> @@ static int hdac_hdmi_pin_output_widget_event(struct >> >> >> >snd_soc_dapm_widget *w, >> >> >> >> if (!pcm) >> >> >> >> return -EIO; >> >> >> >> >> >> >> >> + if (wait_for_display_power(hdmi)) { >> >> >> >> + dev_err(&hdev->dev, "%s: hdmi power is not ready\n", >> >> >> >__func__); >> >> >> >> + return -EAGAIN; >> >> >> >> + } >> >> >> >> + >> >> >> >> /* set the device if pin is mst_capable */ >> >> >> >> if (hdac_hdmi_port_select_set(hdev, port) < 0) >> >> >> >> return -EIO; >> >> >> >> @@ -803,6 +824,11 @@ static int >> >> >> >hdac_hdmi_cvt_output_widget_event(struct snd_soc_dapm_widget >> >> >> >*w, >> >> >> >> if (!pcm) >> >> >> >> return -EIO; >> >> >> >> >> >> >> >> + if (wait_for_display_power(hdmi)) { >> >> >> >> + dev_err(&hdev->dev, "%s: hdmi power is not ready\n", >> >> >> >__func__); >> >> >> >> + return -EAGAIN; >> >> >> >> + } >> >> >> >> + >> >> >> >> switch (event) { >> >> >> >> case SND_SOC_DAPM_PRE_PMU: >> >> >> >> hdac_hdmi_set_power_state(hdev, cvt->nid, >> >AC_PWRST_D0); >> >> >> >@@ -840,6 >> >> >> >> +866,7 @@ static int hdac_hdmi_pin_mux_widget_event(struct >> >> >> >> snd_soc_dapm_widget *w, { >> >> >> >> struct hdac_hdmi_port *port = w->priv; >> >> >> >> struct hdac_device *hdev = dev_to_hdac_dev(w->dapm->dev); >> >> >> >> + struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); >> >> >> >> int mux_idx; >> >> >> >> >> >> >> >> dev_dbg(&hdev->dev, "%s: widget: %s event: %x\n", @@ - >> >850,6 >> >> >> >+877,11 >> >> >> >> @@ static int hdac_hdmi_pin_mux_widget_event(struct >> >> >> >> snd_soc_dapm_widget *w, >> >> >> >> >> >> >> >> mux_idx = dapm_kcontrol_get_value(kc); >> >> >> >> >> >> >> >> + if (wait_for_display_power(hdmi)) { >> >> >> >> + dev_err(&hdev->dev, "%s: hdmi power is not ready\n", >> >> >> >__func__); >> >> >> >> + return -EAGAIN; >> >> >> >> + } >> >> >> >> + >> >> >> >> /* set the device if pin is mst_capable */ >> >> >> >> if (hdac_hdmi_port_select_set(hdev, port) < 0) >> >> >> >> return -EIO; >> >> >> >> @@ -2068,6 +2100,7 @@ static int >> >> >> >> hdac_hdmi_runtime_suspend(struct device *dev) { >> >> >> >> struct hdac_device *hdev = dev_to_hdac_dev(dev); >> >> >> >> struct hdac_bus *bus = hdev->bus; >> >> >> >> + struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); >> >> >> >> struct hdac_ext_link *hlink = NULL; >> >> >> >> >> >> >> >> dev_dbg(dev, "Enter: %s\n", __func__); @@ -2095,6 +2128,7 >> >@@ >> >> >> >static >> >> >> >> int hdac_hdmi_runtime_suspend(struct device *dev) >> >> >> >> snd_hdac_ext_bus_link_put(bus, hlink); >> >> >> >> >> >> >> >> snd_hdac_display_power(bus, hdev->addr, false); >> >> >> >> + hdmi->display_power = 0; >> >> >> >> >> >> >> >> return 0; >> >> >> >> } >> >> >> >> @@ -2102,6 +2136,7 @@ static int >> >> >> >> hdac_hdmi_runtime_suspend(struct device *dev) static int >> >> >> >> hdac_hdmi_runtime_resume(struct device >> >> >> >> *dev) { >> >> >> >> struct hdac_device *hdev = dev_to_hdac_dev(dev); >> >> >> >> + struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); >> >> >> >> struct hdac_bus *bus = hdev->bus; >> >> >> >> struct hdac_ext_link *hlink = NULL; >> >> >> >> >> >> >> >> @@ -2128,6 +2163,7 @@ static int >> >hdac_hdmi_runtime_resume(struct >> >> >> >device *dev) >> >> >> >> snd_hdac_codec_read(hdev, hdev->afg, 0, >> >> >> > AC_VERB_SET_POWER_STATE, >> >> >> >> >> > AC_PWRST_D0); >> >> >> >> >> >> >> >> + hdmi->display_power = 1; >> >> >> >> return 0; >> >> >> >> } >> >> >> >> #else >> >> >> >> -- >> >> >> >> 2.7.4 >> >> >> >> >> >> >> >> _______________________________________________ >> >> >> >> Alsa-devel mailing list >> >> >> >> Alsa-devel@xxxxxxxxxxxxxxxx >> >> >> >> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel >> >> >> >> >> >> >> >> >> >> _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
