Hi Takashi, >-----Original Message----- >From: Takashi Iwai [mailto:tiwai@xxxxxxx] >Sent: Tuesday, March 26, 2019 3:37 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 Tue, 26 Mar 2019 06:42:15 +0100, >Yang, Libin wrote: >(snip) >> Hi Takashi, >> Below is my draft patch, which doesn't work with >> pm_runtime_get_sync(). Is there anything wrong in my code? >(snip) >> And the dmesg is: >> [ 36.087224] HDMI HDA Codec ehdaudio0D2: in hdmi_codec_resume 1890 >ylb >> [ 36.087230] HDMI HDA Codec ehdaudio0D2: in >hdac_hdmi_runtime_resume 2122 ylb 1 >> [ 36.087335] HDMI HDA Codec ehdaudio0D2: in >hdac_hdmi_cvt_output_widget_event 812 ylb >> [ 40.097586] HDMI HDA Codec ehdaudio0D2: in >hdac_hdmi_runtime_resume 2135 ylb 2 >> [ 40.097766] HDMI HDA Codec ehdaudio0D2: in >hdac_hdmi_pin_output_widget_event 767 ylb >> [ 45.108632] HDMI HDA Codec ehdaudio0D2: in >hdac_hdmi_pin_mux_widget_event 857 ylb >> >> >From the dmesg, hdac_hdmi_cvt_output_widget_event() >> is called between "ylb 1" and "ylb 2" in runtime_resume(). >> This means xxx_event() registers setting runs before display power is >> turned on. > >OK, now I understood what went wrong. It's a similar issue as we've hit for >the legacy HD-audio and figured out recently. > >If my understanding is correct, the problem is the call of >pm_runtime_force_resume() in PM resume callback combined with an async >work. pm_runtime_force_resume() sets the runtime state immediately to >RPM_ACTIVE even before finishing the task. The code seems assuming blindly >that there is no other conflicting task while S3/S4 resume, but this is a >problem. That's why the next pm_runtime_get_sync() wasn't blocked but just >went through; since the RPM flag was already set to RPM_ACTIVE in >pm_runtime_force_resume(), it thought as if it were already active, while the >real runtime resume code was still processing the callback. Yes, exactly right. And I have checked dev->power.runtime_status, it's RPM_ACTIVE when xxx_event() calls pm_runtime_get_sync(). > >In the case of legacy HD-audio, we "fixed" the problem by avoiding the trigger >of async work at resume, but let it be triggered from runtime resume. In ASoC >case, however, it's no option. > >Maybe a possible solution in the sound driver side would be to move from >system suspend/resume to ASoC component suspend/resume. The runtime >suspend/resume can be kept as is, and call pm_runtime_force_suspend and >resume from the component suspend/resume callbacks. Since the >component callbacks are certainly processed before DAPM events, this should >assure the order. I have worked out another patch. This patch decouples the xxx_event() and runtime suspend/resume. This means in whichever case, xxx_event() can make sure display power is in the correct status. What do you think? On the same time, I will try the component suspend/resume. My understanding is that snd_hdac_display_power() should be called either in runtime_suspend/ resume or in component suspend/resume. diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index 5eeb0fe..2acb7f1 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -144,7 +144,9 @@ struct hdac_hdmi_priv { int num_pin; int num_cvt; int num_ports; + int power_cnt; struct mutex pin_mutex; + struct mutex power_mutex; struct hdac_chmap chmap; struct hdac_hdmi_drv_data *drv_data; struct snd_soc_dai_driver *dai_drv; @@ -286,6 +288,79 @@ static struct hdac_hdmi_pcm *get_hdmi_pcm_from_id(struct hdac_hdmi_priv *hdmi, return NULL; } +#define INTEL_VENDOR_NID_0x2 0x02 +#define INTEL_VENDOR_NID_0x8 0x08 +#define INTEL_VENDOR_NID_0xb 0x0b +#define INTEL_GET_VENDOR_VERB 0xf81 +#define INTEL_SET_VENDOR_VERB 0x781 +#define INTEL_EN_DP12 0x02 /* enable DP 1.2 features */ +#define INTEL_EN_ALL_PIN_CVTS 0x01 /* enable 2nd & 3rd pins and convertors */ + +static void hdac_hdmi_skl_enable_all_pins(struct hdac_device *hdev) +{ + unsigned int vendor_param; + struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); + unsigned int vendor_nid = hdmi->drv_data->vendor_nid; + + vendor_param = snd_hdac_codec_read(hdev, vendor_nid, 0, + INTEL_GET_VENDOR_VERB, 0); + if (vendor_param == -1 || vendor_param & INTEL_EN_ALL_PIN_CVTS) + return; + + vendor_param |= INTEL_EN_ALL_PIN_CVTS; + vendor_param = snd_hdac_codec_read(hdev, vendor_nid, 0, + INTEL_SET_VENDOR_VERB, vendor_param); + if (vendor_param == -1) + return; +} + +static void hdac_hdmi_skl_enable_dp12(struct hdac_device *hdev) +{ + unsigned int vendor_param; + struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); + unsigned int vendor_nid = hdmi->drv_data->vendor_nid; + + vendor_param = snd_hdac_codec_read(hdev, vendor_nid, 0, + INTEL_GET_VENDOR_VERB, 0); + if (vendor_param == -1 || vendor_param & INTEL_EN_DP12) + return; + + /* enable DP1.2 mode */ + vendor_param |= INTEL_EN_DP12; + vendor_param = snd_hdac_codec_read(hdev, vendor_nid, 0, + INTEL_SET_VENDOR_VERB, vendor_param); + if (vendor_param == -1) + return; + +} + +static void put_display_power(struct hdac_device *hdev) +{ + struct hdac_bus *bus = hdev->bus; + struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); + + mutex_lock(&hdmi->power_mutex); + hdmi->power_cnt--; + if (hdmi->power_cnt == 0) + snd_hdac_display_power(bus, hdev->addr, false); + mutex_unlock(&hdmi->power_mutex); +} + +static void get_display_power(struct hdac_device *hdev) +{ + struct hdac_bus *bus = hdev->bus; + struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); + + mutex_lock(&hdmi->power_mutex); + if (hdmi->power_cnt == 0) { + snd_hdac_display_power(bus, hdev->addr, true); + hdac_hdmi_skl_enable_all_pins(hdev); + hdac_hdmi_skl_enable_dp12(hdev); + } + hdmi->power_cnt++; + mutex_unlock(&hdmi->power_mutex); +} + static unsigned int sad_format(const u8 *sad) { return ((sad[0] >> 0x3) & 0x1f); @@ -749,17 +824,21 @@ static int hdac_hdmi_pin_output_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_pcm *pcm; + int ret = 0; dev_dbg(&hdev->dev, "%s: widget: %s event: %x\n", __func__, w->name, event); + get_display_power(hdev); pcm = hdac_hdmi_get_pcm(hdev, port); if (!pcm) return -EIO; /* set the device if pin is mst_capable */ - if (hdac_hdmi_port_select_set(hdev, port) < 0) + if (hdac_hdmi_port_select_set(hdev, port) < 0) { + put_display_power(hdev); return -EIO; + } switch (event) { case SND_SOC_DAPM_PRE_PMU: @@ -771,7 +850,8 @@ static int hdac_hdmi_pin_output_widget_event(struct snd_soc_dapm_widget *w, hdac_hdmi_set_amp(hdev, port->pin->nid, AMP_OUT_UNMUTE); - return hdac_hdmi_setup_audio_infoframe(hdev, pcm, port); + ret = hdac_hdmi_setup_audio_infoframe(hdev, pcm, port); + break; case SND_SOC_DAPM_POST_PMD: hdac_hdmi_set_amp(hdev, port->pin->nid, AMP_OUT_MUTE); @@ -784,8 +864,9 @@ static int hdac_hdmi_pin_output_widget_event(struct snd_soc_dapm_widget *w, break; } + put_display_power(hdev); - return 0; + return ret; } static int hdac_hdmi_cvt_output_widget_event(struct snd_soc_dapm_widget *w, @@ -803,6 +884,7 @@ static int hdac_hdmi_cvt_output_widget_event(struct snd_soc_dapm_widget *w, if (!pcm) return -EIO; + get_display_power(hdev); switch (event) { case SND_SOC_DAPM_PRE_PMU: hdac_hdmi_set_power_state(hdev, cvt->nid, AC_PWRST_D0); @@ -831,6 +913,7 @@ static int hdac_hdmi_cvt_output_widget_event(struct snd_soc_dapm_widget *w, break; } + put_display_power(hdev); return 0; } @@ -850,15 +933,20 @@ static int hdac_hdmi_pin_mux_widget_event(struct snd_soc_dapm_widget *w, mux_idx = dapm_kcontrol_get_value(kc); + get_display_power(hdev); /* set the device if pin is mst_capable */ - if (hdac_hdmi_port_select_set(hdev, port) < 0) + if (hdac_hdmi_port_select_set(hdev, port) < 0) { + put_display_power(hdev); return -EIO; + } if (mux_idx > 0) { snd_hdac_codec_write(hdev, port->pin->nid, 0, AC_VERB_SET_CONNECT_SEL, (mux_idx - 1)); } + put_display_power(hdev); + return 0; } @@ -1339,52 +1427,6 @@ static int hdac_hdmi_add_pin(struct hdac_device *hdev, hda_nid_t nid) return 0; } -#define INTEL_VENDOR_NID_0x2 0x02 -#define INTEL_VENDOR_NID_0x8 0x08 -#define INTEL_VENDOR_NID_0xb 0x0b -#define INTEL_GET_VENDOR_VERB 0xf81 -#define INTEL_SET_VENDOR_VERB 0x781 -#define INTEL_EN_DP12 0x02 /* enable DP 1.2 features */ -#define INTEL_EN_ALL_PIN_CVTS 0x01 /* enable 2nd & 3rd pins and convertors */ - -static void hdac_hdmi_skl_enable_all_pins(struct hdac_device *hdev) -{ - unsigned int vendor_param; - struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); - unsigned int vendor_nid = hdmi->drv_data->vendor_nid; - - vendor_param = snd_hdac_codec_read(hdev, vendor_nid, 0, - INTEL_GET_VENDOR_VERB, 0); - if (vendor_param == -1 || vendor_param & INTEL_EN_ALL_PIN_CVTS) - return; - - vendor_param |= INTEL_EN_ALL_PIN_CVTS; - vendor_param = snd_hdac_codec_read(hdev, vendor_nid, 0, - INTEL_SET_VENDOR_VERB, vendor_param); - if (vendor_param == -1) - return; -} - -static void hdac_hdmi_skl_enable_dp12(struct hdac_device *hdev) -{ - unsigned int vendor_param; - struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); - unsigned int vendor_nid = hdmi->drv_data->vendor_nid; - - vendor_param = snd_hdac_codec_read(hdev, vendor_nid, 0, - INTEL_GET_VENDOR_VERB, 0); - if (vendor_param == -1 || vendor_param & INTEL_EN_DP12) - return; - - /* enable DP1.2 mode */ - vendor_param |= INTEL_EN_DP12; - vendor_param = snd_hdac_codec_read(hdev, vendor_nid, 0, - INTEL_SET_VENDOR_VERB, vendor_param); - if (vendor_param == -1) - return; - -} - static const struct snd_soc_dai_ops hdmi_dai_ops = { .startup = hdac_hdmi_pcm_open, .shutdown = hdac_hdmi_pcm_close, @@ -1473,9 +1515,6 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_device *hdev, struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); int ret; - hdac_hdmi_skl_enable_all_pins(hdev); - hdac_hdmi_skl_enable_dp12(hdev); - num_nodes = snd_hdac_get_sub_nodes(hdev, hdev->afg, &nid); if (!nid || num_nodes <= 0) { dev_warn(&hdev->dev, "HDMI: failed to get afg sub nodes\n"); @@ -2032,12 +2071,13 @@ static int hdac_hdmi_dev_probe(struct hdac_device *hdev) INIT_LIST_HEAD(&hdmi_priv->cvt_list); INIT_LIST_HEAD(&hdmi_priv->pcm_list); mutex_init(&hdmi_priv->pin_mutex); + mutex_init(&hdmi_priv->power_mutex); /* * Turned off in the runtime_suspend during the first explicit * pm_runtime_suspend call. */ - snd_hdac_display_power(hdev->bus, hdev->addr, true); + get_display_power(hdev); ret = hdac_hdmi_parse_and_map_nid(hdev, &hdmi_dais, &num_dais); if (ret < 0) { @@ -2058,7 +2098,7 @@ static int hdac_hdmi_dev_probe(struct hdac_device *hdev) static int hdac_hdmi_dev_remove(struct hdac_device *hdev) { - snd_hdac_display_power(hdev->bus, hdev->addr, false); + put_display_power(hdev); return 0; } @@ -2094,7 +2134,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); + put_display_power(hdev); return 0; } @@ -2119,11 +2159,7 @@ static int hdac_hdmi_runtime_resume(struct device *dev) snd_hdac_ext_bus_link_get(bus, hlink); - snd_hdac_display_power(bus, hdev->addr, true); - - hdac_hdmi_skl_enable_all_pins(hdev); - hdac_hdmi_skl_enable_dp12(hdev); - + get_display_power(hdev); /* Power up afg */ snd_hdac_codec_read(hdev, hdev->afg, 0, AC_VERB_SET_POWER_STATE, AC_PWRST_D0); > > >thanks, > >Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
