[PATCH 0/2] OMAPDSS: Couple of HDMI audio fixes

[PATCH 0/2] OMAPDSS: Couple of HDMI audio fixes

[Jyri Sarha]

The first fix is a hairy one, but I think the locking is fool proof
now. It is needed because there is no telling in which order user
space starts an audio and video stream playback. If the audio is
started first and the video mode is changed when video playback starts
the audio setup needs to survive display turning off and back on
again. After this patch the audio streams should survive a
suspend-resume cycle too.
 
The second one is a trivial work-around to a problem in ALSA constraint
resolver code.

The two fixes are totally independent and the video and audio side
patches applied separately trough their own trees.

Jyri Sarha (2):
  OMAPDSS: hdmi: Reconfigure and restart audio when display is enabled
  ASoC: omap-hdmi-audio: Set buffer bytes step constraint to 128

 drivers/video/fbdev/omap2/dss/hdmi.h  | 10 +++-
 drivers/video/fbdev/omap2/dss/hdmi4.c | 65 ++++++++++++++++++++-----
 drivers/video/fbdev/omap2/dss/hdmi5.c | 89 +++++++++++++++++++++++++++++------
 sound/soc/omap/omap-hdmi-audio.c      | 10 +++-
 4 files changed, 146 insertions(+), 28 deletions(-)

-- 
1.9.1

[PATCH 1/2] OMAPDSS: hdmi: Reconfigure and restart audio when display is enabled

[Jyri Sarha]

Reconfigure and restart audio when display is enabled, if audio
playback was active before. The audio configuration is stored when is
is successfully applied and a boolean is set when playback has started
and unset when stopped. This data is used to reconfigure the audio
when display is re-enabled. Abort audio playback if reconfiguration
fails.

A new spin lock is introduced to in order protect state variables
related to audio playback status. The wp_idlemode protection relies on
PM not touching it after the original initialization. A power
management API for controlling the idlemode would be needed for proper
synchronization.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/video/fbdev/omap2/dss/hdmi.h  | 10 +++-
 drivers/video/fbdev/omap2/dss/hdmi4.c | 65 ++++++++++++++++++++-----
 drivers/video/fbdev/omap2/dss/hdmi5.c | 89 +++++++++++++++++++++++++++++------
 3 files changed, 137 insertions(+), 27 deletions(-)

diff --git a/drivers/video/fbdev/omap2/dss/hdmi.h b/drivers/video/fbdev/omap2/dss/hdmi.h
index e4a32fe..1e45b84 100644
--- a/drivers/video/fbdev/omap2/dss/hdmi.h
+++ b/drivers/video/fbdev/omap2/dss/hdmi.h
@@ -351,12 +351,20 @@ struct omap_hdmi {
 	struct regulator *vdda_reg;
 
 	bool core_enabled;
-	bool display_enabled;
 
 	struct omap_dss_device output;
 
 	struct platform_device *audio_pdev;
 	void (*audio_abort_cb)(struct device *dev);
+
+	bool audio_configured;
+	struct omap_dss_audio audio_config;
+
+	/* This lock should be taken when any one of the three
+	   state variables bellow are touched. */
+	spinlock_t audio_playing_lock;
+	bool audio_playing;
+	bool display_enabled;
 	int wp_idlemode;
 };
 
diff --git a/drivers/video/fbdev/omap2/dss/hdmi4.c b/drivers/video/fbdev/omap2/dss/hdmi4.c
index 6d3aa3f..ea1fa64 100644
--- a/drivers/video/fbdev/omap2/dss/hdmi4.c
+++ b/drivers/video/fbdev/omap2/dss/hdmi4.c
@@ -324,6 +324,7 @@ static int read_edid(u8 *buf, int len)
 static int hdmi_display_enable(struct omap_dss_device *dssdev)
 {
 	struct omap_dss_device *out = &hdmi.output;
+	unsigned long flags;
 	int r = 0;
 
 	DSSDBG("ENTER hdmi_display_enable\n");
@@ -342,7 +343,26 @@ static int hdmi_display_enable(struct omap_dss_device *dssdev)
 		goto err0;
 	}
 
+	if (hdmi.audio_configured) {
+		hdmi4_audio_stop(&hdmi.core, &hdmi.wp);
+		hdmi_wp_audio_enable(&hdmi.wp, false);
+
+		r = hdmi4_audio_config(&hdmi.core, &hdmi.wp, &hdmi.audio_config,
+				       hdmi.cfg.timings.pixelclock);
+		if (r) {
+			DSSERR("Error restoring audio configuration: %d", r);
+			hdmi.audio_abort_cb(&hdmi.pdev->dev);
+			hdmi.audio_configured = false;
+		}
+	}
+
+	spin_lock_irqsave(&hdmi.audio_playing_lock, flags);
+	if (hdmi.audio_configured && hdmi.audio_playing) {
+		hdmi_wp_audio_enable(&hdmi.wp, true);
+		hdmi4_audio_start(&hdmi.core, &hdmi.wp);
+	}
 	hdmi.display_enabled = true;
+	spin_unlock_irqrestore(&hdmi.audio_playing_lock, flags);
 
 	mutex_unlock(&hdmi.lock);
 	return 0;
@@ -354,17 +374,18 @@ err0:
 
 static void hdmi_display_disable(struct omap_dss_device *dssdev)
 {
+	unsigned long flags;
+
 	DSSDBG("Enter hdmi_display_disable\n");
 
 	mutex_lock(&hdmi.lock);
 
-	if (hdmi.audio_pdev && hdmi.audio_abort_cb)
-		hdmi.audio_abort_cb(&hdmi.audio_pdev->dev);
+	spin_lock_irqsave(&hdmi.audio_playing_lock, flags);
+	hdmi.display_enabled = false;
+	spin_unlock_irqrestore(&hdmi.audio_playing_lock, flags);
 
 	hdmi_power_off_full(dssdev);
 
-	hdmi.display_enabled = false;
-
 	mutex_unlock(&hdmi.lock);
 }
 
@@ -565,9 +586,14 @@ out:
 static int hdmi_audio_shutdown(struct device *dev)
 {
 	struct omap_hdmi *hd = dev_get_drvdata(dev);
+	unsigned long flags;
 
 	mutex_lock(&hd->lock);
 	hd->audio_abort_cb = NULL;
+	hd->audio_configured = false;
+	spin_lock_irqsave(&hd->audio_playing_lock, flags);
+	hd->audio_playing = false;
+	spin_unlock_irqrestore(&hd->audio_playing_lock, flags);
 	mutex_unlock(&hd->lock);
 
 	return 0;
@@ -576,25 +602,38 @@ static int hdmi_audio_shutdown(struct device *dev)
 static int hdmi_audio_start(struct device *dev)
 {
 	struct omap_hdmi *hd = dev_get_drvdata(dev);
+	unsigned long flags;
 
 	WARN_ON(!hdmi_mode_has_audio(&hd->cfg));
-	WARN_ON(!hd->display_enabled);
 
-	hdmi_wp_audio_enable(&hd->wp, true);
-	hdmi4_audio_start(&hd->core, &hd->wp);
+	spin_lock_irqsave(&hd->audio_playing_lock, flags);
+
+	if (hd->display_enabled) {
+		hdmi_wp_audio_enable(&hd->wp, true);
+		hdmi4_audio_start(&hd->core, &hd->wp);
+	}
+	hd->audio_playing = true;
 
+	spin_unlock_irqrestore(&hd->audio_playing_lock, flags);
 	return 0;
 }
 
 static void hdmi_audio_stop(struct device *dev)
 {
 	struct omap_hdmi *hd = dev_get_drvdata(dev);
+	unsigned long flags;
 
 	WARN_ON(!hdmi_mode_has_audio(&hd->cfg));
-	WARN_ON(!hd->display_enabled);
 
-	hdmi4_audio_stop(&hd->core, &hd->wp);
-	hdmi_wp_audio_enable(&hd->wp, false);
+	spin_lock_irqsave(&hd->audio_playing_lock, flags);
+
+	if (hd->display_enabled) {
+		hdmi4_audio_stop(&hd->core, &hd->wp);
+		hdmi_wp_audio_enable(&hd->wp, false);
+	}
+	hd->audio_playing = false;
+
+	spin_unlock_irqrestore(&hd->audio_playing_lock, flags);
 }
 
 static int hdmi_audio_config(struct device *dev,
@@ -612,7 +651,10 @@ static int hdmi_audio_config(struct device *dev,
 
 	ret = hdmi4_audio_config(&hd->core, &hd->wp, dss_audio,
 				 hd->cfg.timings.pixelclock);
-
+	if (!ret) {
+		hd->audio_configured = true;
+		hd->audio_config = *dss_audio;
+	}
 out:
 	mutex_unlock(&hd->lock);
 
@@ -657,6 +699,7 @@ static int hdmi4_bind(struct device *dev, struct device *master, void *data)
 	dev_set_drvdata(&pdev->dev, &hdmi);
 
 	mutex_init(&hdmi.lock);
+	spin_lock_init(&hdmi.audio_playing_lock);
 
 	if (pdev->dev.of_node) {
 		r = hdmi_probe_of(pdev);
diff --git a/drivers/video/fbdev/omap2/dss/hdmi5.c b/drivers/video/fbdev/omap2/dss/hdmi5.c
index 7f87578..f352c4b 100644
--- a/drivers/video/fbdev/omap2/dss/hdmi5.c
+++ b/drivers/video/fbdev/omap2/dss/hdmi5.c
@@ -352,6 +352,7 @@ static int read_edid(u8 *buf, int len)
 static int hdmi_display_enable(struct omap_dss_device *dssdev)
 {
 	struct omap_dss_device *out = &hdmi.output;
+	unsigned long flags;
 	int r = 0;
 
 	DSSDBG("ENTER hdmi_display_enable\n");
@@ -370,7 +371,37 @@ static int hdmi_display_enable(struct omap_dss_device *dssdev)
 		goto err0;
 	}
 
+	if (hdmi.audio_configured) {
+		spin_lock_irqsave(&hdmi.audio_playing_lock, flags);
+		hdmi_wp_audio_core_req_enable(&hdmi.wp, false);
+		hdmi_wp_audio_enable(&hdmi.wp, false);
+		if (hdmi.wp_idlemode > 0)
+			REG_FLD_MOD(hdmi.wp.base, HDMI_WP_SYSCONFIG,
+				    hdmi.wp_idlemode, 3, 2);
+		hdmi.wp_idlemode = -1;
+		spin_unlock_irqrestore(&hdmi.audio_playing_lock, flags);
+
+		r = hdmi5_audio_config(&hdmi.core, &hdmi.wp, &hdmi.audio_config,
+				       hdmi.cfg.timings.pixelclock);
+		if (r) {
+			DSSERR("Error restoring audio configuration: %d", r);
+			hdmi.audio_abort_cb(&hdmi.pdev->dev);
+			hdmi.audio_configured = false;
+		}
+	}
+
+	spin_lock_irqsave(&hdmi.audio_playing_lock, flags);
+	if (hdmi.audio_configured && hdmi.audio_playing) {
+		/* No-idle while playing audio, store the old value */
+		hdmi.wp_idlemode +			REG_GET(hdmi.wp.base, HDMI_WP_SYSCONFIG, 3, 2);
+		REG_FLD_MOD(hdmi.wp.base, HDMI_WP_SYSCONFIG, 1, 3, 2);
+
+		hdmi_wp_audio_enable(&hdmi.wp, true);
+		hdmi_wp_audio_core_req_enable(&hdmi.wp, true);
+	}
 	hdmi.display_enabled = true;
+	spin_unlock_irqrestore(&hdmi.audio_playing_lock, flags);
 
 	mutex_unlock(&hdmi.lock);
 	return 0;
@@ -382,17 +413,18 @@ err0:
 
 static void hdmi_display_disable(struct omap_dss_device *dssdev)
 {
+	unsigned long flags;
+
 	DSSDBG("Enter hdmi_display_disable\n");
 
 	mutex_lock(&hdmi.lock);
 
-	if (hdmi.audio_pdev && hdmi.audio_abort_cb)
-		hdmi.audio_abort_cb(&hdmi.audio_pdev->dev);
+	spin_lock_irqsave(&hdmi.audio_playing_lock, flags);
+	hdmi.display_enabled = false;
+	spin_unlock_irqrestore(&hdmi.audio_playing_lock, flags);
 
 	hdmi_power_off_full(dssdev);
 
-	hdmi.display_enabled = false;
-
 	mutex_unlock(&hdmi.lock);
 }
 
@@ -593,9 +625,14 @@ out:
 static int hdmi_audio_shutdown(struct device *dev)
 {
 	struct omap_hdmi *hd = dev_get_drvdata(dev);
+	unsigned long flags;
 
 	mutex_lock(&hd->lock);
 	hd->audio_abort_cb = NULL;
+	hd->audio_configured = false;
+	spin_lock_irqsave(&hd->audio_playing_lock, flags);
+	hd->audio_playing = false;
+	spin_unlock_irqrestore(&hd->audio_playing_lock, flags);
 	mutex_unlock(&hd->lock);
 
 	return 0;
@@ -604,32 +641,48 @@ static int hdmi_audio_shutdown(struct device *dev)
 static int hdmi_audio_start(struct device *dev)
 {
 	struct omap_hdmi *hd = dev_get_drvdata(dev);
+	unsigned long flags;
 
 	WARN_ON(!hdmi_mode_has_audio(&hd->cfg));
-	WARN_ON(!hd->display_enabled);
 
-	/* No-idle while playing audio, store the old value */
-	hd->wp_idlemode = REG_GET(hdmi.wp.base, HDMI_WP_SYSCONFIG, 3, 2);
-	REG_FLD_MOD(hdmi.wp.base, HDMI_WP_SYSCONFIG, 1, 3, 2);
+	spin_lock_irqsave(&hd->audio_playing_lock, flags);
 
-	hdmi_wp_audio_enable(&hd->wp, true);
-	hdmi_wp_audio_core_req_enable(&hd->wp, true);
+	if (hd->display_enabled) {
+		/* No-idle while playing audio, store the old value */
+		hd->wp_idlemode +			REG_GET(hdmi.wp.base, HDMI_WP_SYSCONFIG, 3, 2);
+		REG_FLD_MOD(hdmi.wp.base, HDMI_WP_SYSCONFIG, 1, 3, 2);
+
+		hdmi_wp_audio_enable(&hd->wp, true);
+		hdmi_wp_audio_core_req_enable(&hd->wp, true);
+	}
+	hd->audio_playing = true;
 
+	spin_unlock_irqrestore(&hd->audio_playing_lock, flags);
 	return 0;
 }
 
 static void hdmi_audio_stop(struct device *dev)
 {
 	struct omap_hdmi *hd = dev_get_drvdata(dev);
+	unsigned long flags;
 
 	WARN_ON(!hdmi_mode_has_audio(&hd->cfg));
-	WARN_ON(!hd->display_enabled);
 
-	hdmi_wp_audio_core_req_enable(&hd->wp, false);
-	hdmi_wp_audio_enable(&hd->wp, false);
+	spin_lock_irqsave(&hd->audio_playing_lock, flags);
 
-	/* Playback stopped, restore original idlemode */
-	REG_FLD_MOD(hdmi.wp.base, HDMI_WP_SYSCONFIG, hd->wp_idlemode, 3, 2);
+	if (hd->display_enabled) {
+		hdmi_wp_audio_core_req_enable(&hd->wp, false);
+		hdmi_wp_audio_enable(&hd->wp, false);
+
+		/* Playback stopped, restore original idlemode */
+		REG_FLD_MOD(hdmi.wp.base, HDMI_WP_SYSCONFIG, hd->wp_idlemode,
+			    3, 2);
+		hd->wp_idlemode = -1;
+	}
+	hd->audio_playing = false;
+
+	spin_unlock_irqrestore(&hd->audio_playing_lock, flags);
 }
 
 static int hdmi_audio_config(struct device *dev,
@@ -648,6 +701,10 @@ static int hdmi_audio_config(struct device *dev,
 	ret = hdmi5_audio_config(&hd->core, &hd->wp, dss_audio,
 				 hd->cfg.timings.pixelclock);
 
+	if (!ret) {
+		hd->audio_configured = true;
+		hd->audio_config = *dss_audio;
+	}
 out:
 	mutex_unlock(&hd->lock);
 
@@ -692,6 +749,8 @@ static int hdmi5_bind(struct device *dev, struct device *master, void *data)
 	dev_set_drvdata(&pdev->dev, &hdmi);
 
 	mutex_init(&hdmi.lock);
+	spin_lock_init(&hdmi.audio_playing_lock);
+	hdmi.wp_idlemode = -1;
 
 	if (pdev->dev.of_node) {
 		r = hdmi_probe_of(pdev);
-- 
1.9.1

[PATCH 2/2] ASoC: omap-hdmi-audio: Set buffer bytes step constraint to 128

[Jyri Sarha]

Set buffer bytes step constraint to 128. A matching constraint has
already been set to period size. This helps PCM setup to tolerate ALSA
clients that set the PCM hw params in unusual order.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 sound/soc/omap/omap-hdmi-audio.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/sound/soc/omap/omap-hdmi-audio.c b/sound/soc/omap/omap-hdmi-audio.c
index aeef25c..584b237 100644
--- a/sound/soc/omap/omap-hdmi-audio.c
+++ b/sound/soc/omap/omap-hdmi-audio.c
@@ -81,7 +81,15 @@ static int hdmi_dai_startup(struct snd_pcm_substream *substream,
 	ret = snd_pcm_hw_constraint_step(substream->runtime, 0,
 					 SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 128);
 	if (ret < 0) {
-		dev_err(dai->dev, "could not apply constraint\n");
+		dev_err(dai->dev, "Could not apply period constraint: %d\n",
+			ret);
+		return ret;
+	}
+	ret = snd_pcm_hw_constraint_step(substream->runtime, 0,
+					 SNDRV_PCM_HW_PARAM_BUFFER_BYTES, 128);
+	if (ret < 0) {
+		dev_err(dai->dev, "Could not apply buffer constraint: %d\n",
+			ret);
 		return ret;
 	}
 
-- 
1.9.1

Re: [PATCH 0/2] OMAPDSS: Couple of HDMI audio fixes

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 376 bytes --]

On Wed, Aug 26, 2015 at 04:11:38PM +0300, Jyri Sarha wrote:

> The two fixes are totally independent and the video and audio side
> patches applied separately trough their own trees.

In general if patches aren't related to each other either through code
overlap or through content it's better to just send them as individual
changes, that avoids any potential for confusion.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] OMAPDSS: hdmi: Reconfigure and restart audio when display is enabled

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3284 bytes --]

Hi,

On 26/08/15 16:11, Jyri Sarha wrote:

I few comments, for the parts I had time to review:

> diff --git a/drivers/video/fbdev/omap2/dss/hdmi5.c b/drivers/video/fbdev/omap2/dss/hdmi5.c
> index 7f87578..f352c4b 100644
> --- a/drivers/video/fbdev/omap2/dss/hdmi5.c
> +++ b/drivers/video/fbdev/omap2/dss/hdmi5.c
> @@ -352,6 +352,7 @@ static int read_edid(u8 *buf, int len)
>  static int hdmi_display_enable(struct omap_dss_device *dssdev)
>  {
>  	struct omap_dss_device *out = &hdmi.output;
> +	unsigned long flags;
>  	int r = 0;
>  
>  	DSSDBG("ENTER hdmi_display_enable\n");
> @@ -370,7 +371,37 @@ static int hdmi_display_enable(struct omap_dss_device *dssdev)
>  		goto err0;
>  	}
>  

I think you could refactor parts of the code below into small helper
functions:

> +	if (hdmi.audio_configured) {
> +		spin_lock_irqsave(&hdmi.audio_playing_lock, flags);
> +		hdmi_wp_audio_core_req_enable(&hdmi.wp, false);
> +		hdmi_wp_audio_enable(&hdmi.wp, false);
> +		if (hdmi.wp_idlemode > 0)
> +			REG_FLD_MOD(hdmi.wp.base, HDMI_WP_SYSCONFIG,
> +				    hdmi.wp_idlemode, 3, 2);
> +		hdmi.wp_idlemode = -1;

The above looks like it's disabling audio output, the same that's done
in hdmi_audio_stop(). Adding a helper func for that makes the code more
readable.

For the wp_idlemode, I think hdmi.wp_idlemode could be initialized to a
valid value, so that it can be set at any time without the if check above.

> +		spin_unlock_irqrestore(&hdmi.audio_playing_lock, flags);
> +
> +		r = hdmi5_audio_config(&hdmi.core, &hdmi.wp, &hdmi.audio_config,
> +				       hdmi.cfg.timings.pixelclock);
> +		if (r) {
> +			DSSERR("Error restoring audio configuration: %d", r);
> +			hdmi.audio_abort_cb(&hdmi.pdev->dev);
> +			hdmi.audio_configured = false;
> +		}
> +	}
> +
> +	spin_lock_irqsave(&hdmi.audio_playing_lock, flags);
> +	if (hdmi.audio_configured && hdmi.audio_playing) {
> +		/* No-idle while playing audio, store the old value */
> +		hdmi.wp_idlemode =
> +			REG_GET(hdmi.wp.base, HDMI_WP_SYSCONFIG, 3, 2);
> +		REG_FLD_MOD(hdmi.wp.base, HDMI_WP_SYSCONFIG, 1, 3, 2);
> +
> +		hdmi_wp_audio_enable(&hdmi.wp, true);
> +		hdmi_wp_audio_core_req_enable(&hdmi.wp, true);

And here maybe a helper func to enable the audio output.

> +	}

>  	hdmi.display_enabled = true;
> +	spin_unlock_irqrestore(&hdmi.audio_playing_lock, flags);
>  
>  	mutex_unlock(&hdmi.lock);
>  	return 0;
> @@ -382,17 +413,18 @@ err0:
>  
>  static void hdmi_display_disable(struct omap_dss_device *dssdev)
>  {
> +	unsigned long flags;
> +
>  	DSSDBG("Enter hdmi_display_disable\n");
>  
>  	mutex_lock(&hdmi.lock);
>  
> -	if (hdmi.audio_pdev && hdmi.audio_abort_cb)
> -		hdmi.audio_abort_cb(&hdmi.audio_pdev->dev);
> +	spin_lock_irqsave(&hdmi.audio_playing_lock, flags);
> +	hdmi.display_enabled = false;
> +	spin_unlock_irqrestore(&hdmi.audio_playing_lock, flags);

Shouldn't something be done here in hdmi_display_disable about audio?
You probably want to keep the state flag enabled, so that we know the
user is playing audio, but you could still disable the HDMI audio HW.
I'm sure the audio output dies when the video is disabled, but being
more explicit here makes the code logic easier to follow.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] OMAPDSS: hdmi: Reconfigure and restart audio when display is enabled

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2267 bytes --]


On 26/08/15 16:11, Jyri Sarha wrote:

> diff --git a/drivers/video/fbdev/omap2/dss/hdmi5.c b/drivers/video/fbdev/omap2/dss/hdmi5.c
> index 7f87578..f352c4b 100644
> --- a/drivers/video/fbdev/omap2/dss/hdmi5.c
> +++ b/drivers/video/fbdev/omap2/dss/hdmi5.c
> @@ -352,6 +352,7 @@ static int read_edid(u8 *buf, int len)
>  static int hdmi_display_enable(struct omap_dss_device *dssdev)
>  {
>  	struct omap_dss_device *out = &hdmi.output;
> +	unsigned long flags;
>  	int r = 0;
>  
>  	DSSDBG("ENTER hdmi_display_enable\n");
> @@ -370,7 +371,37 @@ static int hdmi_display_enable(struct omap_dss_device *dssdev)
>  		goto err0;
>  	}
>  
> +	if (hdmi.audio_configured) {
> +		spin_lock_irqsave(&hdmi.audio_playing_lock, flags);
> +		hdmi_wp_audio_core_req_enable(&hdmi.wp, false);
> +		hdmi_wp_audio_enable(&hdmi.wp, false);
> +		if (hdmi.wp_idlemode > 0)
> +			REG_FLD_MOD(hdmi.wp.base, HDMI_WP_SYSCONFIG,
> +				    hdmi.wp_idlemode, 3, 2);
> +		hdmi.wp_idlemode = -1;
> +		spin_unlock_irqrestore(&hdmi.audio_playing_lock, flags);

Here I think the audio HW is always disabled already. It has to be,
because the whole HDMI IP has been off. So the above should not be needed.

> +
> +		r = hdmi5_audio_config(&hdmi.core, &hdmi.wp, &hdmi.audio_config,
> +				       hdmi.cfg.timings.pixelclock);
> +		if (r) {
> +			DSSERR("Error restoring audio configuration: %d", r);
> +			hdmi.audio_abort_cb(&hdmi.pdev->dev);
> +			hdmi.audio_configured = false;
> +		}
> +	}
> +
> +	spin_lock_irqsave(&hdmi.audio_playing_lock, flags);
> +	if (hdmi.audio_configured && hdmi.audio_playing) {
> +		/* No-idle while playing audio, store the old value */
> +		hdmi.wp_idlemode =
> +			REG_GET(hdmi.wp.base, HDMI_WP_SYSCONFIG, 3, 2);
> +		REG_FLD_MOD(hdmi.wp.base, HDMI_WP_SYSCONFIG, 1, 3, 2);
> +
> +		hdmi_wp_audio_enable(&hdmi.wp, true);
> +		hdmi_wp_audio_core_req_enable(&hdmi.wp, true);
> +	}
>  	hdmi.display_enabled = true;
> +	spin_unlock_irqrestore(&hdmi.audio_playing_lock, flags);

Maybe you've looked at the locking carefully, but it's not obvious to
me. So is hdmi_audio_start and hdmi_audio_stop the only functions that
are called from atomic context? Every other function is protected with
the mutex?

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] OMAPDSS: hdmi: Reconfigure and restart audio when display is enabled

[Jyri Sarha]

On 08/28/15 13:37, Tomi Valkeinen wrote:
>
> On 26/08/15 16:11, Jyri Sarha wrote:
>
>> diff --git a/drivers/video/fbdev/omap2/dss/hdmi5.c b/drivers/video/fbdev/omap2/dss/hdmi5.c
>> index 7f87578..f352c4b 100644
>> --- a/drivers/video/fbdev/omap2/dss/hdmi5.c
>> +++ b/drivers/video/fbdev/omap2/dss/hdmi5.c
>> @@ -352,6 +352,7 @@ static int read_edid(u8 *buf, int len)
>>   static int hdmi_display_enable(struct omap_dss_device *dssdev)
>>   {
>>   	struct omap_dss_device *out = &hdmi.output;
>> +	unsigned long flags;
>>   	int r = 0;
>>
>>   	DSSDBG("ENTER hdmi_display_enable\n");
>> @@ -370,7 +371,37 @@ static int hdmi_display_enable(struct omap_dss_device *dssdev)
>>   		goto err0;
>>   	}
>>
>> +	if (hdmi.audio_configured) {
>> +		spin_lock_irqsave(&hdmi.audio_playing_lock, flags);
>> +		hdmi_wp_audio_core_req_enable(&hdmi.wp, false);
>> +		hdmi_wp_audio_enable(&hdmi.wp, false);
>> +		if (hdmi.wp_idlemode > 0)
>> +			REG_FLD_MOD(hdmi.wp.base, HDMI_WP_SYSCONFIG,
>> +				    hdmi.wp_idlemode, 3, 2);
>> +		hdmi.wp_idlemode = -1;
>> +		spin_unlock_irqrestore(&hdmi.audio_playing_lock, flags);
>
> Here I think the audio HW is always disabled already. It has to be,
> because the whole HDMI IP has been off. So the above should not be needed.
>
>> +
>> +		r = hdmi5_audio_config(&hdmi.core, &hdmi.wp, &hdmi.audio_config,
>> +				       hdmi.cfg.timings.pixelclock);
>> +		if (r) {
>> +			DSSERR("Error restoring audio configuration: %d", r);
>> +			hdmi.audio_abort_cb(&hdmi.pdev->dev);
>> +			hdmi.audio_configured = false;
>> +		}
>> +	}
>> +
>> +	spin_lock_irqsave(&hdmi.audio_playing_lock, flags);
>> +	if (hdmi.audio_configured && hdmi.audio_playing) {
>> +		/* No-idle while playing audio, store the old value */
>> +		hdmi.wp_idlemode >> +			REG_GET(hdmi.wp.base, HDMI_WP_SYSCONFIG, 3, 2);
>> +		REG_FLD_MOD(hdmi.wp.base, HDMI_WP_SYSCONFIG, 1, 3, 2);
>> +
>> +		hdmi_wp_audio_enable(&hdmi.wp, true);
>> +		hdmi_wp_audio_core_req_enable(&hdmi.wp, true);
>> +	}
>>   	hdmi.display_enabled = true;
>> +	spin_unlock_irqrestore(&hdmi.audio_playing_lock, flags);
>
> Maybe you've looked at the locking carefully, but it's not obvious to
> me. So is hdmi_audio_start and hdmi_audio_stop the only functions that
> are called from atomic context? Every other function is protected with
> the mutex?
>

The idea is for the spinlock to make audio start, audio stop, and 
updates to hdmi.display_enabled and hdmi.audio_playing variable atomic.

This is needed for the transitions from display enabled state (when 
audio start/stop commands can be written to HW) to display disabled 
state (when audio start/stop commands update only the hdmi.audio_playing 
variable) to always serialize correctly.

IOW, the idea is to make sure the hdmi.audio_playing variable is always 
in sync with what is in the HW when hdmi.display_enabled = true.

For example: when display is turned back on we take the spinlock and we 
can be sure that the audio start/stop status won't change while we 
update the HW according to current state and set hdmi.display_enabled to 
true. After releasing the lock hdmi.display_enabled is true and all 
audio_start and audio_stop commands write their stuff directly to HW.

In theory, just making hdmi.display_enabled and hdmi.audio_playing 
atomic-variables and touching them always in correct oreder should be 
enough, but explaining the mechanism would then be even trickier.

Cheers,
Jyri