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

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

 



On Mon, 25 Mar 2019 07:51:11 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> >-----Original Message-----
> >From: Takashi Iwai [mailto:tiwai@xxxxxxx]
> >Sent: Saturday, March 23, 2019 4:26 AM
> >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 Fri, 22 Mar 2019 12:46:25 +0100,
> >Yang, Libin wrote:
> >>
> >> Hi Takashi,
> >>
> >> >> >
> >> >> >On Fri, 22 Mar 2019 07:24:56 +0100, libin.yang@xxxxxxxxx wrote:
> >> >> >>
> >> >> >> From: Libin Yang <libin.yang@xxxxxxxxx>
> >> >> >>
> >> >> >> 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@xxxxxxxxx>
> >> >> >
> >> >> >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.


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