Re: [PATCH v2] OMAPDSS: hdmi: Reconfigure and restart audio when display is enabled

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

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

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


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