[alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
Takashi Iwai
tiwai at suse.de
Fri Mar 22 21:25:36 CET 2019
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 at intel.com wrote:
> >> >>
> >> >> From: Libin Yang <libin.yang at 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 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.
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-devel
> >> >>
> >>
>
More information about the Alsa-devel
mailing list