[alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
Yang, Libin
libin.yang at intel.com
Tue Mar 26 06:42:15 CET 2019
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 at 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.
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
>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.
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 at alsa-project.org
>> >> >> >> >> https://mailman.alsa-project.org/mailman/listinfo/alsa-dev
>> >> >> >> >> el
>> >> >> >> >>
>> >> >> >>
>> >> >>
>> >>
>>
More information about the Alsa-devel
mailing list