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. 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
