Hi Takashi,

-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Friday, March 22, 2019 5:04 PM To: Yang, Libin <libin.yang@intel.com> Cc: alsa-devel@alsa-project.org; broonie@kernel.org Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3

On Fri, 22 Mar 2019 07:24:56 +0100, libin.yang@intel.com wrote:

...

From: Libin Yang <libin.yang@intel.com>

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@intel.com>

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. 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@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Hi Takashi,

...

...

On Fri, 22 Mar 2019 07:24:56 +0100, libin.yang@intel.com wrote:

...

From: Libin Yang <libin.yang@intel.com>

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@intel.com>

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. 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@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Hi Takashi,

-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Saturday, March 23, 2019 4:26 AM To: Yang, Libin <libin.yang@intel.com> Cc: alsa-devel@alsa-project.org; broonie@kernel.org Subject: Re: [alsa-devel] [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@intel.com wrote:

...

From: Libin Yang <libin.yang@intel.com>

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@intel.com>

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. 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? 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@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Hi Takashi,

-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Monday, March 25, 2019 6:08 PM To: Yang, Libin <libin.yang@intel.com> Cc: alsa-devel@alsa-project.org; broonie@kernel.org Subject: Re: [alsa-devel] [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@suse.de] Sent: Saturday, March 23, 2019 4:26 AM To: Yang, Libin <libin.yang@intel.com> Cc: alsa-devel@alsa-project.org; broonie@kernel.org Subject: Re: [alsa-devel] [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@intel.com wrote: >> >> From: Libin Yang <libin.yang@intel.com> >> >> 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@intel.com> > >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@alsa-project.org >> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel >>

Hi Takashi,

...

...

...

...

...

>> >> >> >> 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@intel.com> >> > >> >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.

Isn't it called from an async work? If yes, it should wait until the runtime resume finishes, hence it must be already in the sanely powered up state. OTOH, if it's called from the same context as the runtime resume itself, we shoot our foot. Then it must be done differently.

It's async work.

...

It seems I have to call snd_hdac_display_power() manually.

Something goes really wrong, then.

Again, if it's the direct PM callback path, managing the PM device order via device_link_add() should be the way to go. If it's an async work context, an explicit sync with pm_runtime_get_sync() is needed.

As it's async, we can't use PM device order to manage this sequence. I tried to use pm_runtime_get_sync() and had a test, but it seems it doesn't help. xxx_event() setting register will still be called before display power is turned on.

...

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.

If the display power is solely managed by the codec driver, DAPM event callback needs to call pm_runtime_get*() / put*() wrt the codec driver, too. The usage is ref-counted, so it won't go to the power off until all users are gone.

Of course, the above assumes that xxx_event() gets called in an async context.

Please check it again.

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

Below is my draft patch, which doesn't work with pm_runtime_get_sync(). Is there anything wrong in my code? diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index 5eeb0fe..429a831 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -763,6 +763,8 @@ static int hdac_hdmi_pin_output_widget_event(struct snd_soc_dapm_widget *w, switch (event) { case SND_SOC_DAPM_PRE_PMU: + pm_runtime_get_sync(&hdev->dev); + dev_err(&hdev->dev, "in %s %d ylb\n", __func__, __LINE__); hdac_hdmi_set_power_state(hdev, port->pin->nid, AC_PWRST_D0); /* Enable out path for this pin widget */ @@ -781,6 +783,7 @@ static int hdac_hdmi_pin_output_widget_event(struct snd_soc_dapm_widget *w, AC_VERB_SET_PIN_WIDGET_CONTROL, 0); hdac_hdmi_set_power_state(hdev, port->pin->nid, AC_PWRST_D3); + pm_runtime_put_sync(&hdev->dev); break; } @@ -805,6 +808,8 @@ static int hdac_hdmi_cvt_output_widget_event(struct snd_soc_dapm_widget *w, switch (event) { case SND_SOC_DAPM_PRE_PMU: + pm_runtime_get_sync(&hdev->dev); + dev_err(&hdev->dev, "in %s %d ylb\n", __func__, __LINE__); hdac_hdmi_set_power_state(hdev, cvt->nid, AC_PWRST_D0); /* Enable transmission */ @@ -828,6 +833,7 @@ static int hdac_hdmi_cvt_output_widget_event(struct snd_soc_dapm_widget *w, AC_VERB_SET_STREAM_FORMAT, 0); hdac_hdmi_set_power_state(hdev, cvt->nid, AC_PWRST_D3); + pm_runtime_put_sync(&hdev->dev); break; } @@ -847,7 +853,8 @@ static int hdac_hdmi_pin_mux_widget_event(struct snd_soc_dapm_widget *w, if (!kc) kc = w->kcontrols[0]; - + pm_runtime_get_sync(&hdev->dev); + dev_err(&hdev->dev, "in %s %d ylb\n", __func__, __LINE__); mux_idx = dapm_kcontrol_get_value(kc); /* set the device if pin is mst_capable */ @@ -858,6 +865,7 @@ static int hdac_hdmi_pin_mux_widget_event(struct snd_soc_dapm_widget *w, snd_hdac_codec_write(hdev, port->pin->nid, 0, AC_VERB_SET_CONNECT_SEL, (mux_idx - 1)); } + pm_runtime_put_sync(&hdev->dev); return 0; } @@ -1879,7 +1887,7 @@ static int hdmi_codec_resume(struct device *dev) struct hdac_device *hdev = dev_to_hdac_dev(dev); struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); int ret; - + dev_err(&hdev->dev, "in %s %d ylb\n", __func__, __LINE__); ret = pm_runtime_force_resume(dev); if (ret < 0) return ret; @@ -2111,6 +2119,7 @@ static int hdac_hdmi_runtime_resume(struct device *dev) if (!bus) return 0; + dev_err(&hdev->dev, "in %s %d ylb 1\n", __func__, __LINE__); hlink = snd_hdac_ext_bus_get_link(bus, dev_name(dev)); if (!hlink) { dev_err(dev, "hdac link not found\n"); @@ -2123,7 +2132,7 @@ static int hdac_hdmi_runtime_resume(struct device *dev) hdac_hdmi_skl_enable_all_pins(hdev); hdac_hdmi_skl_enable_dp12(hdev); - + dev_err(&hdev->dev, "in %s %d ylb 2\n", __func__, __LINE__); /* Power up afg */ snd_hdac_codec_read(hdev, hdev->afg, 0, AC_VERB_SET_POWER_STATE, AC_PWRST_D0); -- 2.7.4 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 power is turned on. Regards, Libin

thanks,

Takashi

...

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@alsa-project.org >> >> https://mailman.alsa-project.org/mailman/listinfo/alsa-dev >> >> el >> >> >>

Hi Takashi,

-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, March 26, 2019 3:37 PM To: Yang, Libin <libin.yang@intel.com> Cc: alsa-devel@alsa-project.org; broonie@kernel.org Subject: Re: [alsa-devel] [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

On Thu, 28 Mar 2019 08:10:34 +0100, Yang, Libin wrote:

Hi Takashi,

...

...

...

...

...

>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 tried to move display power setting from runtime suspend/resume to component suspend/resume. I found there may be another issue: playback will NOT call component suspend/resume. This means we will never have chance to set the display power if we don't do the S3.

The runtime suspend/resume are fine and need to be kept, I suppose. The only problem is the system suspend/resume callbacks that call pm_runtime_force_*(). Just moving these two to component would work?

Let's see the flow of the suspend: 1. Component suspend is called. It will call snd_hdac_display_power() to turn off the display power.

No, what I meant was to make the component suspend calling pm_runtime_force_suspend() instead of the system suspend. Ditto for the component resume calling pm_runtime_force_resume().

It seems this component suspend/resume method works well. I will do more test and send out the patch later.

I did some stress test. And I found if I remove the debug message, the issue can still be reproduced with component suspend/resume added. If I add the debug message, it will be much better. I'm not sure whether my patch is wrong or not. I've set the bus_control = 1 in the dai driver.

I guess you forgot to remove the suspend/resume call from the codec device PM? Otherwise the codec device resume is still executed concurrently. Takashi

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index 5eeb0fe..505efd9 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -1900,9 +1900,31 @@ static int hdmi_codec_resume(struct device *dev) #define hdmi_codec_resume NULL #endif

+static int hdmi_component_suspend(struct snd_soc_component *component) +{ + struct hdac_hdmi_priv *hdmi = snd_soc_component_get_drvdata(component); + struct hdac_device *hdev = hdmi->hdev; + + pm_runtime_force_suspend(&hdev->dev); + + return 0; +} + +static int hdmi_component_resume(struct snd_soc_component *component) +{ + struct hdac_hdmi_priv *hdmi = snd_soc_component_get_drvdata(component); + struct hdac_device *hdev = hdmi->hdev; + + pm_runtime_force_resume(&hdev->dev); + + return 0; +} + static const struct snd_soc_component_driver hdmi_hda_codec = { .probe = hdmi_codec_probe, .remove = hdmi_codec_remove, + .suspend = hdmi_component_suspend, + .resume = hdmi_component_resume, .use_pmdown_time = 1, .endianness = 1, .non_legacy_dai_naming = 1,

...

Regards, Libin