Re: [PATCH V2] ASoC: fix hdmi codec driver contest in S3

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux