[alsa-devel] [PATCH v2] OMAPDSS: hdmi: Reconfigure and restart audio when display is enabled
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 in order to protect state variables related to audio playback status. 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 with the start/stop audio commands.
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 hdmi.audio_playing 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.
Signed-off-by: Jyri Sarha jsarha@ti.com --- I dropped the ASoC maintainers from the recipient list as this patch hardly concerns them.
drivers/video/fbdev/omap2/dss/hdmi.h | 9 +++- drivers/video/fbdev/omap2/dss/hdmi4.c | 69 +++++++++++++++++++++++++----- drivers/video/fbdev/omap2/dss/hdmi5.c | 79 ++++++++++++++++++++++++++++------- 3 files changed, 130 insertions(+), 27 deletions(-)
diff --git a/drivers/video/fbdev/omap2/dss/hdmi.h b/drivers/video/fbdev/omap2/dss/hdmi.h index e4a32fe..e48aefd 100644 --- a/drivers/video/fbdev/omap2/dss/hdmi.h +++ b/drivers/video/fbdev/omap2/dss/hdmi.h @@ -351,13 +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); int wp_idlemode; + + bool audio_configured; + struct omap_dss_audio audio_config; + + /* This lock should be taken when booleas bellow is touched. */ + spinlock_t audio_playing_lock; + bool audio_playing; + bool display_enabled; };
#endif diff --git a/drivers/video/fbdev/omap2/dss/hdmi4.c b/drivers/video/fbdev/omap2/dss/hdmi4.c index 6d3aa3f..6b9817a 100644 --- a/drivers/video/fbdev/omap2/dss/hdmi4.c +++ b/drivers/video/fbdev/omap2/dss/hdmi4.c @@ -321,9 +321,22 @@ static int read_edid(u8 *buf, int len) return r; }
+static void hdmi_start_audio_stream(struct omap_hdmi *hd) +{ + hdmi_wp_audio_enable(&hd->wp, true); + hdmi4_audio_start(&hd->core, &hd->wp); +} + +static void hdmi_stop_audio_stream(struct omap_hdmi *hd) +{ + hdmi4_audio_stop(&hd->core, &hd->wp); + hdmi_wp_audio_enable(&hd->wp, false); +} + 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 +355,21 @@ static int hdmi_display_enable(struct omap_dss_device *dssdev) goto err0; }
+ if (hdmi.audio_configured) { + 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_start_audio_stream(&hdmi); hdmi.display_enabled = true; + spin_unlock_irqrestore(&hdmi.audio_playing_lock, flags);
mutex_unlock(&hdmi.lock); return 0; @@ -354,17 +381,19 @@ 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_stop_audio_stream(&hdmi); + 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 +594,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 +610,34 @@ 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_start_audio_stream(hd); + 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) + hdmi_stop_audio_stream(hd); + hd->audio_playing = false; + + spin_unlock_irqrestore(&hd->audio_playing_lock, flags); }
static int hdmi_audio_config(struct device *dev, @@ -612,7 +655,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 +703,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..85e70af 100644 --- a/drivers/video/fbdev/omap2/dss/hdmi5.c +++ b/drivers/video/fbdev/omap2/dss/hdmi5.c @@ -349,9 +349,24 @@ static int read_edid(u8 *buf, int len) return r; }
+static void hdmi_start_audio_stream(struct omap_hdmi *hd) +{ + 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); +} + +static void hdmi_stop_audio_stream(struct omap_hdmi *hd) +{ + hdmi_wp_audio_core_req_enable(&hd->wp, false); + hdmi_wp_audio_enable(&hd->wp, false); + REG_FLD_MOD(hd->wp.base, HDMI_WP_SYSCONFIG, hd->wp_idlemode, 3, 2); +} + 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 +385,21 @@ static int hdmi_display_enable(struct omap_dss_device *dssdev) goto err0; }
+ if (hdmi.audio_configured) { + 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) + hdmi_start_audio_stream(&hdmi); hdmi.display_enabled = true; + spin_unlock_irqrestore(&hdmi.audio_playing_lock, flags);
mutex_unlock(&hdmi.lock); return 0; @@ -382,17 +411,19 @@ 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_stop_audio_stream(&hdmi); + 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 +624,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 +640,35 @@ 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) { + hdmi_start_audio_stream(hd); + } + 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); + + if (hd->display_enabled) + hdmi_stop_audio_stream(hd); + hd->audio_playing = false;
- /* Playback stopped, restore original idlemode */ - REG_FLD_MOD(hdmi.wp.base, HDMI_WP_SYSCONFIG, hd->wp_idlemode, 3, 2); + spin_unlock_irqrestore(&hd->audio_playing_lock, flags); }
static int hdmi_audio_config(struct device *dev, @@ -648,6 +687,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);
@@ -678,6 +721,11 @@ static int hdmi_audio_register(struct device *dev) if (IS_ERR(hdmi.audio_pdev)) return PTR_ERR(hdmi.audio_pdev);
+ hdmi_runtime_get(); + hdmi.wp_idlemode = + REG_GET(hdmi.wp.base, HDMI_WP_SYSCONFIG, 3, 2); + hdmi_runtime_put(); + return 0; }
@@ -692,6 +740,7 @@ 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);
if (pdev->dev.of_node) { r = hdmi_probe_of(pdev);
On 08/28/15 15:24, Jyri Sarha wrote:
@@ -565,9 +594,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);
BTW, This extra locking (and the corresponding change in hdmi5.c) should not be needed. But it does not harm either. I'll fix that, but I'll wait first if there is anything else fix.
Best regards, Jyri
Hi,
On 28/08/15 15:24, Jyri Sarha wrote:
Reconfigure and restart audio when display is enabled, if audio playback was active before. The audio configuration is stored when is
'is' -> 'it' above
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.
It would be good to start the description by telling what the current problem is. And probably the subject could also be better... This fixes the audio playback when a video mode change happens (or such), right?
A new spin lock is introduced in order to protect state variables related to audio playback status. 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 with the start/stop audio commands.
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 hdmi.audio_playing 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.
The question is (which was my point in the earlier mail), we already have mutex, so why a new spinlock?
I think the answer is that audio start/stop (anything else?) are called in atomic context, so mutex cannot be used.
Also (not exactly related to this patch), if the audio callbacks must be atomic, could we use a workqueue to run the audio start/stop work in non-atomic context? Protecting the whole hdmi state with a single mutex would be much nicer.
Signed-off-by: Jyri Sarha jsarha@ti.com
I dropped the ASoC maintainers from the recipient list as this patch hardly concerns them.
drivers/video/fbdev/omap2/dss/hdmi.h | 9 +++- drivers/video/fbdev/omap2/dss/hdmi4.c | 69 +++++++++++++++++++++++++----- drivers/video/fbdev/omap2/dss/hdmi5.c | 79 ++++++++++++++++++++++++++++------- 3 files changed, 130 insertions(+), 27 deletions(-)
diff --git a/drivers/video/fbdev/omap2/dss/hdmi.h b/drivers/video/fbdev/omap2/dss/hdmi.h index e4a32fe..e48aefd 100644 --- a/drivers/video/fbdev/omap2/dss/hdmi.h +++ b/drivers/video/fbdev/omap2/dss/hdmi.h @@ -351,13 +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); int wp_idlemode;
- bool audio_configured;
- struct omap_dss_audio audio_config;
- /* This lock should be taken when booleas bellow is touched. */
typo above.
Otherwise, looks much cleaner than the previous one. I tested it and worked fine for me: I could play audio while turning on and off the video output, and the audio would resume, except when the video was off for long enough.
Tomi
On 08/28/15 16:04, Tomi Valkeinen wrote: ...
The question is (which was my point in the earlier mail), we already have mutex, so why a new spinlock?
I think the answer is that audio start/stop (anything else?) are called in atomic context, so mutex cannot be used.
Yes, that is correct.
Also (not exactly related to this patch), if the audio callbacks must be atomic, could we use a workqueue to run the audio start/stop work in non-atomic context? Protecting the whole hdmi state with a single mutex would be much nicer.
The reason why the audio start and stop are done in atomic context is for audio synchronization to other audio devices or video stream to be more accurate.
I guess in theory a work queue could be used to serialize the audio commands, but that would ruin the whole point of why the start and stop commands are atomic in the first place.
Cheers, Jyri
participants (2)
-
Jyri Sarha
-
Tomi Valkeinen