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@xxxxxx>
> ---
> 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:
signature.asc
Description: OpenPGP digital signature
