[PATCH RFC v4 3/8] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders

[PATCH RFC v4 3/8] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders

[Jyri Sarha]

The hdmi-codec is a platform device driver to be registered from
drivers of external HDMI encoders with I2S and/or spdif interface. The
driver in turn registers an ASoC codec for the HDMI encoder's audio
functionality.

The structures and definitions in the API header are mostly redundant
copies of similar structures in ASoC headers. This is on purpose to
avoid direct dependencies to ASoC structures in video side driver.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 include/sound/hdmi-codec.h    | 104 +++++++++++
 sound/soc/codecs/Kconfig      |   6 +
 sound/soc/codecs/Makefile     |   2 +
 sound/soc/codecs/hdmi-codec.c | 404 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 516 insertions(+)
 create mode 100644 include/sound/hdmi-codec.h
 create mode 100644 sound/soc/codecs/hdmi-codec.c

diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h
new file mode 100644
index 0000000..15fe70f
--- /dev/null
+++ b/include/sound/hdmi-codec.h
@@ -0,0 +1,104 @@
+/*
+ * hdmi-codec.h - HDMI Codec driver API
+ *
+ * Copyright (C) 2014 Texas Instruments Incorporated - http://www.ti.com
+ *
+ * Author: Jyri Sarha <jsarha@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#ifndef __HDMI_CODEC_H__
+#define __HDMI_CODEC_H__
+
+#include <linux/hdmi.h>
+#include <drm/drm_edid.h>
+#include <sound/asoundef.h>
+#include <uapi/sound/asound.h>
+
+/*
+ * Protocol between ASoC cpu-dai and HDMI-encoder
+ */
+struct hdmi_codec_daifmt {
+	enum {
+		HDMI_I2S,
+		HDMI_RIGHT_J,
+		HDMI_LEFT_J,
+		HDMI_DSP_A,
+		HDMI_DSP_B,
+		HDMI_AC97,
+		HDMI_SPDIF,
+	} fmt;
+	int bit_clk_inv:1;
+	int frame_clk_inv:1;
+	int bit_clk_master:1;
+	int frame_clk_master:1;
+};
+
+/*
+ * HDMI audio parameters
+ */
+struct hdmi_codec_params {
+	struct hdmi_audio_infoframe cea;
+	struct snd_aes_iec958 iec;
+	int sample_rate;
+	int sample_width;
+	int channels;
+};
+
+struct hdmi_codec_ops {
+	/*
+	 * Called when ASoC starts an audio stream setup. The call
+	 * provides an audio abort callback for stoping an ongoing
+	 * stream from video side driver if the HDMI audio becomes
+	 * unavailable.
+	 * Optional
+	 */
+	int (*audio_startup)(struct device *dev,
+			     void (*abort_cb)(struct device *dev));
+
+	/*
+	 * Configures HDMI-encoder for audio stream.
+	 * Mandatory
+	 */
+	int (*hw_params)(struct device *dev,
+			 struct hdmi_codec_daifmt *fmt,
+			 struct hdmi_codec_params *hparms);
+
+	/*
+	 * Shuts down the audio stream.
+	 * Mandatory
+	 */
+	void (*audio_shutdown)(struct device *dev);
+
+	/*
+	 * Mute/unmute HDMI audio stream.
+	 * Optional
+	 */
+	int (*digital_mute)(struct device *dev, bool enable);
+
+	/*
+	 * Provides EDID-Like-Data from connected HDMI device.
+	 * Optional
+	 */
+	int (*get_eld)(struct device *dev, uint8_t *buf, size_t len);
+};
+
+/* HDMI codec initalization data */
+struct hdmi_codec_pdata {
+	const struct hdmi_codec_ops *ops;
+	uint i2s:1;
+	uint spdif:1;
+	int max_i2s_channels;
+};
+
+#define HDMI_CODEC_DRV_NAME "hdmi-audio-codec"
+
+#endif /* __HDMI_CODEC_H__ */
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 0142396..1e6c7e7 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -79,6 +79,7 @@ config SND_SOC_ALL_CODECS
 	select SND_SOC_MAX9877 if I2C
 	select SND_SOC_MC13783 if MFD_MC13XXX
 	select SND_SOC_ML26124 if I2C
+	select SND_SOC_HDMI_CODEC
 	select SND_SOC_PCM1681 if I2C
 	select SND_SOC_PCM1792A if SPI_MASTER
 	select SND_SOC_PCM3008
@@ -441,6 +442,11 @@ config SND_SOC_BT_SCO
 config SND_SOC_DMIC
 	tristate
 
+config SND_SOC_HDMI_CODEC
+       tristate
+       select SND_PCM_ELD
+       select SND_PCM_IEC958
+
 config SND_SOC_ES8328
 	tristate "Everest Semi ES8328 CODEC"
 
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index 7d7cc1b..068d483 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -72,6 +72,7 @@ snd-soc-max98925-objs := max98925.o
 snd-soc-max9850-objs := max9850.o
 snd-soc-mc13783-objs := mc13783.o
 snd-soc-ml26124-objs := ml26124.o
+snd-soc-hdmi-codec-objs := hdmi-codec.o
 snd-soc-pcm1681-objs := pcm1681.o
 snd-soc-pcm1792a-codec-objs := pcm1792a.o
 snd-soc-pcm3008-objs := pcm3008.o
@@ -263,6 +264,7 @@ obj-$(CONFIG_SND_SOC_MAX98925)	+= snd-soc-max98925.o
 obj-$(CONFIG_SND_SOC_MAX9850)	+= snd-soc-max9850.o
 obj-$(CONFIG_SND_SOC_MC13783)	+= snd-soc-mc13783.o
 obj-$(CONFIG_SND_SOC_ML26124)	+= snd-soc-ml26124.o
+obj-$(CONFIG_SND_SOC_HDMI_CODEC)	+= snd-soc-hdmi-codec.o
 obj-$(CONFIG_SND_SOC_PCM1681)	+= snd-soc-pcm1681.o
 obj-$(CONFIG_SND_SOC_PCM1792A)	+= snd-soc-pcm1792a-codec.o
 obj-$(CONFIG_SND_SOC_PCM3008)	+= snd-soc-pcm3008.o
diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
new file mode 100644
index 0000000..687332d
--- /dev/null
+++ b/sound/soc/codecs/hdmi-codec.c
@@ -0,0 +1,404 @@
+/*
+ * ALSA SoC codec for HDMI encoder drivers
+ * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/
+ * Author: Jyri Sarha <jsarha@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.	 See the GNU
+ * General Public License for more details.
+ */
+#include <linux/module.h>
+#include <linux/string.h>
+#include <sound/core.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <sound/pcm_drm_eld.h>
+#include <sound/hdmi-codec.h>
+#include <sound/pcm_iec958.h>
+
+#include <drm/drm_crtc.h> /* This is only to get MAX_ELD_BYTES */
+
+struct hdmi_codec_priv {
+	struct hdmi_codec_pdata hcd;
+	struct snd_soc_dai_driver *daidrv;
+	struct hdmi_codec_daifmt daifmt[2];
+	struct mutex current_stream_lock;
+	struct snd_pcm_substream *current_stream;
+	struct snd_pcm_hw_constraint_list ratec;
+	uint8_t eld[MAX_ELD_BYTES];
+};
+
+static const struct snd_soc_dapm_widget hdmi_widgets[] = {
+	SND_SOC_DAPM_OUTPUT("TX"),
+};
+
+static const struct snd_soc_dapm_route hdmi_routes[] = {
+	{ "TX", NULL, "Playback" },
+};
+
+enum {
+	DAI_ID_I2S = 0,
+	DAI_ID_SPDIF,
+};
+
+static void hdmi_codec_abort(struct device *dev)
+{
+	struct hdmi_codec_priv *hcp = dev_get_drvdata(dev);
+
+	dev_dbg(dev, "%s()\n", __func__);
+
+	mutex_lock(&hcp->current_stream_lock);
+	if (hcp->current_stream && hcp->current_stream->runtime &&
+	    snd_pcm_running(hcp->current_stream)) {
+		dev_info(dev, "HDMI audio playback aborted\n");
+		snd_pcm_stream_lock_irq(hcp->current_stream);
+		snd_pcm_stop(hcp->current_stream, SNDRV_PCM_STATE_DISCONNECTED);
+		snd_pcm_stream_unlock_irq(hcp->current_stream);
+	}
+	mutex_unlock(&hcp->current_stream_lock);
+}
+
+static int hdmi_codec_new_stream(struct snd_pcm_substream *substream,
+				 struct snd_soc_dai *dai)
+{
+	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
+	int ret = 0;
+
+	mutex_lock(&hcp->current_stream_lock);
+	if (!hcp->current_stream) {
+		hcp->current_stream = substream;
+	} else if (hcp->current_stream != substream) {
+		dev_err(dai->dev, "Only one simultaneous stream supported!\n");
+		ret = -EINVAL;
+	}
+	mutex_unlock(&hcp->current_stream_lock);
+
+	return ret;
+}
+
+static int hdmi_codec_startup(struct snd_pcm_substream *substream,
+			      struct snd_soc_dai *dai)
+{
+	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
+	int ret = 0;
+
+	dev_dbg(dai->dev, "%s()\n", __func__);
+
+	ret = hdmi_codec_new_stream(substream, dai);
+	if (ret)
+		return ret;
+
+	if (hcp->hcd.ops->audio_startup) {
+		ret = hcp->hcd.ops->audio_startup(dai->dev->parent,
+						  hdmi_codec_abort);
+		if (ret) {
+			mutex_lock(&hcp->current_stream_lock);
+			hcp->current_stream = NULL;
+			mutex_unlock(&hcp->current_stream_lock);
+			return ret;
+		}
+	}
+
+	if (hcp->hcd.ops->get_eld) {
+		ret = hcp->hcd.ops->get_eld(dai->dev->parent, hcp->eld,
+					    sizeof(hcp->eld));
+
+		if (!ret) {
+			ret = snd_pcm_hw_constraint_eld(substream->runtime,
+							hcp->eld);
+			if (ret)
+				return ret;
+		}
+	}
+	return 0;
+}
+
+static void hdmi_codec_shutdown(struct snd_pcm_substream *substream,
+				struct snd_soc_dai *dai)
+{
+	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
+
+	dev_dbg(dai->dev, "%s()\n", __func__);
+
+	WARN_ON(hcp->current_stream != substream);
+
+	hcp->hcd.ops->audio_shutdown(dai->dev->parent);
+
+	mutex_lock(&hcp->current_stream_lock);
+	hcp->current_stream = NULL;
+	mutex_unlock(&hcp->current_stream_lock);
+}
+
+static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
+				struct snd_pcm_hw_params *params,
+				struct snd_soc_dai *dai)
+{
+	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
+	struct hdmi_codec_params hp = {
+		.iec = {
+			.status = { 0 },
+			.subcode = { 0 },
+			.pad = 0,
+			.dig_subframe = { 0 },
+		}
+	};
+	int ret;
+
+	dev_dbg(dai->dev, "%s() width %d rate %d channels %d\n", __func__,
+		params_width(params), params_rate(params),
+		params_channels(params));
+
+	ret = snd_pcm_create_iec958_consumer_hw_params(params, hp.iec.status,
+						       sizeof(hp.iec.status));
+	if (ret < 0) {
+		dev_err(dai->dev, "Creating IEC958 channel status failed %d\n",
+			ret);
+		return ret;
+	}
+
+	ret = hdmi_codec_new_stream(substream, dai);
+	if (ret)
+		return ret;
+
+	hdmi_audio_infoframe_init(&hp.cea);
+	hp.cea.channels = params_channels(params);
+	hp.cea.coding_type = HDMI_AUDIO_CODING_TYPE_STREAM;
+	hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_STREAM;
+	hp.cea.sample_frequency = HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM;
+
+	hp.sample_width = params_width(params);
+	hp.sample_rate = params_rate(params);
+	hp.channels = params_channels(params);
+
+	return hcp->hcd.ops->hw_params(dai->dev->parent, &hcp->daifmt[dai->id],
+				       &hp);
+}
+
+static int hdmi_codec_set_fmt(struct snd_soc_dai *dai,
+			      unsigned int fmt)
+{
+	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
+	struct hdmi_codec_daifmt cf = { 0 };
+	int ret = 0;
+
+	dev_dbg(dai->dev, "%s()\n", __func__);
+
+	if (dai->id == DAI_ID_SPDIF) {
+		cf.fmt = HDMI_SPDIF;
+	} else {
+		switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
+		case SND_SOC_DAIFMT_CBM_CFM:
+			cf.bit_clk_master = 1;
+			cf.frame_clk_master = 1;
+			break;
+		case SND_SOC_DAIFMT_CBS_CFM:
+			cf.frame_clk_master = 1;
+			break;
+		case SND_SOC_DAIFMT_CBM_CFS:
+			cf.bit_clk_master = 1;
+			break;
+		case SND_SOC_DAIFMT_CBS_CFS:
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
+		case SND_SOC_DAIFMT_NB_NF:
+			break;
+		case SND_SOC_DAIFMT_NB_IF:
+			cf.frame_clk_inv = 1;
+			break;
+		case SND_SOC_DAIFMT_IB_NF:
+			cf.bit_clk_inv = 1;
+			break;
+		case SND_SOC_DAIFMT_IB_IF:
+			cf.frame_clk_inv = 1;
+			cf.bit_clk_inv = 1;
+			break;
+		}
+
+		switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+		case SND_SOC_DAIFMT_I2S:
+			cf.fmt = HDMI_I2S;
+			break;
+		case SND_SOC_DAIFMT_DSP_A:
+			cf.fmt = HDMI_DSP_A;
+			break;
+		case SND_SOC_DAIFMT_DSP_B:
+			cf.fmt = HDMI_DSP_B;
+			break;
+		case SND_SOC_DAIFMT_RIGHT_J:
+			cf.fmt = HDMI_RIGHT_J;
+			break;
+		case SND_SOC_DAIFMT_LEFT_J:
+			cf.fmt = HDMI_LEFT_J;
+			break;
+		case SND_SOC_DAIFMT_AC97:
+			cf.fmt = HDMI_AC97;
+			break;
+		default:
+			dev_err(dai->dev, "Invalid DAI interface format\n");
+			return -EINVAL;
+		}
+	}
+
+	hcp->daifmt[dai->id] = cf;
+
+	return ret;
+}
+
+static int hdmi_codec_digital_mute(struct snd_soc_dai *dai, int mute)
+{
+	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
+
+	dev_dbg(dai->dev, "%s()\n", __func__);
+
+	if (hcp->hcd.ops->digital_mute)
+		return hcp->hcd.ops->digital_mute(dai->dev->parent, mute);
+
+	return 0;
+}
+
+static const struct snd_soc_dai_ops hdmi_dai_ops = {
+	.startup	= hdmi_codec_startup,
+	.shutdown	= hdmi_codec_shutdown,
+	.hw_params	= hdmi_codec_hw_params,
+	.set_fmt	= hdmi_codec_set_fmt,
+	.digital_mute	= hdmi_codec_digital_mute,
+};
+
+
+#define HDMI_RATES	(SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |\
+			 SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 |\
+			 SNDRV_PCM_RATE_96000 | SNDRV_PCM_RATE_176400 |\
+			 SNDRV_PCM_RATE_192000)
+
+#define SPDIF_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\
+			 SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE |\
+			 SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_3BE |\
+			 SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE)
+
+#define I2S_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\
+			 SNDRV_PCM_FMTBIT_S18_3LE | SNDRV_PCM_FMTBIT_S18_3BE |\
+			 SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE |\
+			 SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_3BE |\
+			 SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE |\
+			 SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S32_BE)
+
+static struct snd_soc_dai_driver hdmi_i2s_dai = {
+	.name = "i2s-hifi",
+	.id = DAI_ID_I2S,
+	.playback = {
+		.stream_name = "Playback",
+		.channels_min = 2,
+		.channels_max = 8,
+		.rates = HDMI_RATES,
+		.formats = I2S_FORMATS,
+		.sig_bits = 24,
+	},
+	.ops = &hdmi_dai_ops,
+};
+
+static const struct snd_soc_dai_driver hdmi_spdif_dai = {
+	.name = "spdif-hifi",
+	.id = DAI_ID_SPDIF,
+	.playback = {
+		.stream_name = "Playback",
+		.channels_min = 2,
+		.channels_max = 2,
+		.rates = HDMI_RATES,
+		.formats = SPDIF_FORMATS,
+	},
+	.ops = &hdmi_dai_ops,
+};
+
+static struct snd_soc_codec_driver hdmi_codec = {
+	.dapm_widgets = hdmi_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(hdmi_widgets),
+	.dapm_routes = hdmi_routes,
+	.num_dapm_routes = ARRAY_SIZE(hdmi_routes),
+};
+
+static int hdmi_codec_probe(struct platform_device *pdev)
+{
+	struct hdmi_codec_pdata *hcd = pdev->dev.platform_data;
+	struct device *dev = &pdev->dev;
+	struct hdmi_codec_priv *hcp;
+	int dai_count, i = 0;
+	int ret;
+
+	dev_dbg(dev, "%s()\n", __func__);
+
+	if (!hcd) {
+		dev_err(dev, "%s: No plalform data\n", __func__);
+		return -EINVAL;
+	}
+
+	dai_count = hcd->i2s + hcd->spdif;
+	if (dai_count < 1 || !hcd->ops || !hcd->ops->hw_params ||
+	    !hcd->ops->audio_shutdown) {
+		dev_err(dev, "%s: Invalid parameters\n", __func__);
+		return -EINVAL;
+	}
+
+	hcp = devm_kzalloc(dev, sizeof(*hcp), GFP_KERNEL);
+	if (!hcp)
+		return -ENOMEM;
+
+	hcp->hcd = *hcd;
+	mutex_init(&hcp->current_stream_lock);
+
+	hcp->daidrv = devm_kzalloc(dev, dai_count * sizeof(*hcp->daidrv),
+				   GFP_KERNEL);
+	if (!hcp->daidrv)
+		return -ENOMEM;
+
+	if (hcd->i2s) {
+		hcp->daidrv[i] = hdmi_i2s_dai;
+		hcp->daidrv[i].playback.channels_max =
+			hcd->max_i2s_channels;
+		i++;
+	}
+
+	if (hcd->spdif)
+		hcp->daidrv[i] = hdmi_spdif_dai;
+
+	ret = snd_soc_register_codec(dev, &hdmi_codec, hcp->daidrv,
+				     dai_count);
+	if (ret) {
+		dev_err(dev, "%s: snd_soc_register_codec() failed (%d)\n",
+			__func__, ret);
+		return ret;
+	}
+
+	dev_set_drvdata(dev, hcp);
+	return 0;
+}
+
+static int hdmi_codec_remove(struct platform_device *pdev)
+{
+	snd_soc_unregister_codec(&pdev->dev);
+	return 0;
+}
+
+static struct platform_driver hdmi_codec_driver = {
+	.driver = {
+		.name = HDMI_CODEC_DRV_NAME,
+	},
+	.probe = hdmi_codec_probe,
+	.remove = hdmi_codec_remove,
+};
+
+module_platform_driver(hdmi_codec_driver);
+
+MODULE_AUTHOR("Jyri Sarha <jsarha@ti.com>");
+MODULE_DESCRIPTION("HDMI Audio Codec Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" HDMI_CODEC_DRV_NAME);
-- 
1.9.1

Re: [PATCH RFC v4 3/8] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders

[Mark Brown]

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

On Fri, Sep 18, 2015 at 02:06:40PM +0300, Jyri Sarha wrote:

> +#define SPDIF_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\
> +			 SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE |\
> +			 SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_3BE |\
> +			 SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE)
> +
> +#define I2S_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\
> +			 SNDRV_PCM_FMTBIT_S18_3LE | SNDRV_PCM_FMTBIT_S18_3BE |\
> +			 SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE |\
> +			 SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_3BE |\
> +			 SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE |\
> +			 SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S32_BE)

I'm not sure how abstracted this I2S and S/PDIF DAI business is - it
doesn't feel like something that's going to be a property of all HDMI
devices, and the specific sets of formats above cause me to raise a bit
of an eyebrow.  Should this be an interface where the HDMI chip
registers multiple interfaces if it has them instead of automatically
getting this split as is?

We can probably skip more specific constraint stuff I guess on the basis
that things tend to support the full range of HDMI functionality, or at
least that can be added later?

Otherwise this looks mostly OK on a first pass, thanks for working on
it.

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

Re: [PATCH RFC v4 3/8] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders

[Russell King - ARM Linux]

On Sat, Sep 19, 2015 at 10:54:51AM -0700, Mark Brown wrote:
> On Fri, Sep 18, 2015 at 02:06:40PM +0300, Jyri Sarha wrote:
> > +#define SPDIF_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\
> > +			 SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE |\
> > +			 SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_3BE |\
> > +			 SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE)
> > +
> > +#define I2S_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\
> > +			 SNDRV_PCM_FMTBIT_S18_3LE | SNDRV_PCM_FMTBIT_S18_3BE |\
> > +			 SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE |\
> > +			 SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_3BE |\
> > +			 SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE |\
> > +			 SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S32_BE)
> 
> I'm not sure how abstracted this I2S and S/PDIF DAI business is - it
> doesn't feel like something that's going to be a property of all HDMI
> devices, and the specific sets of formats above cause me to raise a bit
> of an eyebrow.  Should this be an interface where the HDMI chip
> registers multiple interfaces if it has them instead of automatically
> getting this split as is?

The inclusion of the 32-bit formats does raise an eyebrow here too.
Audio transmission across the HDMI link is S/PDIF, supporting up to
24-bit uncompressed audio (aka L-PCM).

The device may accept 32 bit I2S, but it would have to be truncated to
24 bit before transmitting it to the sink.  This should be mentioned
somewhere.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC v4 3/8] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders

[Jyri Sarha]

On 09/21/15 12:31, Russell King - ARM Linux wrote:
> On Sat, Sep 19, 2015 at 10:54:51AM -0700, Mark Brown wrote:
>> On Fri, Sep 18, 2015 at 02:06:40PM +0300, Jyri Sarha wrote:
>>> +#define SPDIF_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\
>>> +			 SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE |\
>>> +			 SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_3BE |\
>>> +			 SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE)
>>> +
>>> +#define I2S_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\
>>> +			 SNDRV_PCM_FMTBIT_S18_3LE | SNDRV_PCM_FMTBIT_S18_3BE |\
>>> +			 SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE |\
>>> +			 SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_3BE |\
>>> +			 SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE |\
>>> +			 SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S32_BE)
>>
>> I'm not sure how abstracted this I2S and S/PDIF DAI business is - it
>> doesn't feel like something that's going to be a property of all HDMI
>> devices, and the specific sets of formats above cause me to raise a bit
>> of an eyebrow.  Should this be an interface where the HDMI chip
>> registers multiple interfaces if it has them instead of automatically
>> getting this split as is?
>
> The inclusion of the 32-bit formats does raise an eyebrow here too.
> Audio transmission across the HDMI link is S/PDIF, supporting up to
> 24-bit uncompressed audio (aka L-PCM).
>
> The device may accept 32 bit I2S, but it would have to be truncated to
> 24 bit before transmitting it to the sink.  This should be mentioned
> somewhere.
>

There is ".sig_bits = 24" in hdmi_i2s_dai, but I can add an explicit 
comment about it.

We needed 32bit format in practice until Peter got 24_LE properly 
working with McASP. I just thought the may still be some platforms out 
there that can not produce 24bit i2s samples for some reason, but work 
just fine with 32bit samples. Those platforms would be limited to less 
than 24bit precision without 32bit formats. But as said, it is not 
critical to us any more and I can drop the 32bit formats as well.

BR,
Jyri



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC v4 3/8] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1098 bytes --]

On Mon, Sep 21, 2015 at 04:41:20PM +0300, Jyri Sarha wrote:
> On 09/21/15 12:31, Russell King - ARM Linux wrote:

> >The device may accept 32 bit I2S, but it would have to be truncated to
> >24 bit before transmitting it to the sink.  This should be mentioned
> >somewhere.

> There is ".sig_bits = 24" in hdmi_i2s_dai, but I can add an explicit comment
> about it.

> We needed 32bit format in practice until Peter got 24_LE properly working
> with McASP. I just thought the may still be some platforms out there that
> can not produce 24bit i2s samples for some reason, but work just fine with
> 32bit samples. Those platforms would be limited to less than 24bit precision
> without 32bit formats. But as said, it is not critical to us any more and I
> can drop the 32bit formats as well.

Yes, this is currently a bit messy - we should really be able to allow
S24_LE and S32 to interoperate better without drivers having to know
about it since physically they are essentially compatible in memory.
I'm pretty much OK with it for now since even with that framework
support it should be harmless.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH RFC v4 3/8] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders

[Arnaud Pouliquen]

few questions/remarks
BR,
Arnaud

> +static void hdmi_codec_abort(struct device *dev)
> +{
> +    struct hdmi_codec_priv *hcp = dev_get_drvdata(dev);
> +
> +    dev_dbg(dev, "%s()\n", __func__);
> +
> +    mutex_lock(&hcp->current_stream_lock);
> +    if (hcp->current_stream && hcp->current_stream->runtime &&
> +        snd_pcm_running(hcp->current_stream)) {
> +        dev_info(dev, "HDMI audio playback aborted\n");
> +        snd_pcm_stream_lock_irq(hcp->current_stream);
> +        snd_pcm_stop(hcp->current_stream, SNDRV_PCM_STATE_DISCONNECTED);
> +        snd_pcm_stream_unlock_irq(hcp->current_stream);
> +    }
> +    mutex_unlock(&hcp->current_stream_lock);
> +}
Does driver should stop the stream in case of unplug?
This could generate unexpected behavior at application level.
Perhaps should be better if this part is managed in DRM driver. if HDMI 
master, I2S bus should be maintained ON to consume the audio stream 
until application action.

> +
> +static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
> +                struct snd_pcm_hw_params *params,
> +                struct snd_soc_dai *dai)
> +{
> +    struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
> +    struct hdmi_codec_params hp = {
> +        .iec = {
> +            .status = { 0 },
> +            .subcode = { 0 },
> +            .pad = 0,
> +            .dig_subframe = { 0 },
> +        }
> +    };
> +    int ret;
> +
> +    dev_dbg(dai->dev, "%s() width %d rate %d channels %d\n", __func__,
> +        params_width(params), params_rate(params),
> +        params_channels(params));
> +
> +    ret = snd_pcm_create_iec958_consumer_hw_params(params, hp.iec.status,
> +                               sizeof(hp.iec.status));
Tell me if i am wrong, but in case of SPDIF format, IEC status is 
managed by cpu_dai not by the codec.
To manage IEC61937 a control should be needed to update IEC status bits...

> +    if (ret < 0) {
> +        dev_err(dai->dev, "Creating IEC958 channel status failed %d\n",
> +            ret);
> +        return ret;
> +    }
> +
> +    ret = hdmi_codec_new_stream(substream, dai);
> +    if (ret)
> +        return ret;
> +
> +    hdmi_audio_infoframe_init(&hp.cea);
> +    hp.cea.channels = params_channels(params);
> +    hp.cea.coding_type = HDMI_AUDIO_CODING_TYPE_STREAM;
> +    hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_STREAM;
> +    hp.cea.sample_frequency = HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM;
> +
> +    hp.sample_width = params_width(params);
> +    hp.sample_rate = params_rate(params);
> +    hp.channels = params_channels(params);
> +
> +    return hcp->hcd.ops->hw_params(dai->dev->parent,
> &hcp->daifmt[dai->id],
> +                       &hp);
> +}
> +

Re: [PATCH RFC v4 3/8] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders

[Russell King - ARM Linux]

On Mon, Sep 28, 2015 at 11:01:34AM +0200, Arnaud Pouliquen wrote:
> few questions/remarks
> BR,
> Arnaud
> 
> >+static void hdmi_codec_abort(struct device *dev)
> >+{
> >+    struct hdmi_codec_priv *hcp = dev_get_drvdata(dev);
> >+
> >+    dev_dbg(dev, "%s()\n", __func__);
> >+
> >+    mutex_lock(&hcp->current_stream_lock);
> >+    if (hcp->current_stream && hcp->current_stream->runtime &&
> >+        snd_pcm_running(hcp->current_stream)) {
> >+        dev_info(dev, "HDMI audio playback aborted\n");
> >+        snd_pcm_stream_lock_irq(hcp->current_stream);
> >+        snd_pcm_stop(hcp->current_stream, SNDRV_PCM_STATE_DISCONNECTED);
> >+        snd_pcm_stream_unlock_irq(hcp->current_stream);
> >+    }
> >+    mutex_unlock(&hcp->current_stream_lock);
> >+}
> Does driver should stop the stream in case of unplug?
> This could generate unexpected behavior at application level.
> Perhaps should be better if this part is managed in DRM driver. if HDMI
> master, I2S bus should be maintained ON to consume the audio stream until
> application action.

If it does, that's really horrid.

Firstly, do you expect your video playback to stop just because you've
unplugged the TV?  Maybe you've got a dumb HDMI switch, and you've
switched from one input to another to momentarily check something on
another input - should the video playback suddenly end up with its
audio path being dumped on the floor because of that?

Also, as I've said before, there's hardware out there where the SPDIF
audio output is routed to more than just the HDMI interface, and the
capabilities of HDMI really don't apply in that situation - you may
have an AV receiver connected to the SPDIF output which is able to
accept much more than the basic audio that most TVs accept.  Having
stuff restricted to the union of the abilities is far too restrictive.
Stopping the audio output because the TV went away in this case is also
plain idiotic.

SPDIF is something that can be routed to multiple devices simultaneously,
and there's no capability or connection reporting involved in it.
Imposing the status of one "SPDIF listener" on the entire audio system is
unreasonable.

> >+
> >+static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
> >+                struct snd_pcm_hw_params *params,
> >+                struct snd_soc_dai *dai)
> >+{
> >+    struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
> >+    struct hdmi_codec_params hp = {
> >+        .iec = {
> >+            .status = { 0 },
> >+            .subcode = { 0 },
> >+            .pad = 0,
> >+            .dig_subframe = { 0 },
> >+        }
> >+    };
> >+    int ret;
> >+
> >+    dev_dbg(dai->dev, "%s() width %d rate %d channels %d\n", __func__,
> >+        params_width(params), params_rate(params),
> >+        params_channels(params));
> >+
> >+    ret = snd_pcm_create_iec958_consumer_hw_params(params, hp.iec.status,
> >+                               sizeof(hp.iec.status));
> Tell me if i am wrong, but in case of SPDIF format, IEC status is managed by
> cpu_dai not by the codec.

Correct.  I2S needs the IEC958 status programmed into the HDMI interface
though, because HDMI is basically SPDIF.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC v4 3/8] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders

[Daniel Vetter]

On Mon, Sep 28, 2015 at 12:26:28PM +0100, Russell King - ARM Linux wrote:
> On Mon, Sep 28, 2015 at 11:01:34AM +0200, Arnaud Pouliquen wrote:
> > few questions/remarks
> > BR,
> > Arnaud
> > 
> > >+static void hdmi_codec_abort(struct device *dev)
> > >+{
> > >+    struct hdmi_codec_priv *hcp = dev_get_drvdata(dev);
> > >+
> > >+    dev_dbg(dev, "%s()\n", __func__);
> > >+
> > >+    mutex_lock(&hcp->current_stream_lock);
> > >+    if (hcp->current_stream && hcp->current_stream->runtime &&
> > >+        snd_pcm_running(hcp->current_stream)) {
> > >+        dev_info(dev, "HDMI audio playback aborted\n");
> > >+        snd_pcm_stream_lock_irq(hcp->current_stream);
> > >+        snd_pcm_stop(hcp->current_stream, SNDRV_PCM_STATE_DISCONNECTED);
> > >+        snd_pcm_stream_unlock_irq(hcp->current_stream);
> > >+    }
> > >+    mutex_unlock(&hcp->current_stream_lock);
> > >+}
> > Does driver should stop the stream in case of unplug?
> > This could generate unexpected behavior at application level.
> > Perhaps should be better if this part is managed in DRM driver. if HDMI
> > master, I2S bus should be maintained ON to consume the audio stream until
> > application action.
> 
> If it does, that's really horrid.

Atm the rule for display outputs is that nothing gets yanked until
userspace approves, since otherwise compositors get stuck (or fall over
with an unexpected -EINVAL from the kernel). The exception is DP MST
because the current implementation is a complete hack for DP MST sink
lifetimes and that's why we need to synchronously nuke them (which means
shutting down everything). I'm surprised not a hole lot more people
complain about this ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC v4 3/8] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders

[Jyri Sarha]

On 09/28/15 12:01, Arnaud Pouliquen wrote:
> few questions/remarks
> BR,
> Arnaud
>
>> +static void hdmi_codec_abort(struct device *dev)
>> +{
>> +    struct hdmi_codec_priv *hcp = dev_get_drvdata(dev);
>> +
>> +    dev_dbg(dev, "%s()\n", __func__);
>> +
>> +    mutex_lock(&hcp->current_stream_lock);
>> +    if (hcp->current_stream && hcp->current_stream->runtime &&
>> +        snd_pcm_running(hcp->current_stream)) {
>> +        dev_info(dev, "HDMI audio playback aborted\n");
>> +        snd_pcm_stream_lock_irq(hcp->current_stream);
>> +        snd_pcm_stop(hcp->current_stream, SNDRV_PCM_STATE_DISCONNECTED);
>> +        snd_pcm_stream_unlock_irq(hcp->current_stream);
>> +    }
>> +    mutex_unlock(&hcp->current_stream_lock);
>> +}
> Does driver should stop the stream in case of unplug?
> This could generate unexpected behavior at application level.
> Perhaps should be better if this part is managed in DRM driver. if HDMI
> master, I2S bus should be maintained ON to consume the audio stream
> until application action.
>

The whole point of this abort callback is to provide the means for the 
video side to stop the audio playback in a clean way.

The abort callback is given to the video side in startup() callback 
(ASoC side calls video side). If the video side sees that the playback 
can not go on it can call the abort callback and the audio playback will 
abort in standard ALSA way and a properly written applications should 
not get confused.

Nothing is forcing the video side to call the callback ever, if so 
decided. But if for instance power management stops the i2s clocks when 
connector is unplugged, then the audio can not go on any more and it 
should be aborted (actually it would abort in a moment when ALSA 
realizes that the DMA is not running).

>> +
>> +static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
>> +                struct snd_pcm_hw_params *params,
>> +                struct snd_soc_dai *dai)
>> +{
>> +    struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
>> +    struct hdmi_codec_params hp = {
>> +        .iec = {
>> +            .status = { 0 },
>> +            .subcode = { 0 },
>> +            .pad = 0,
>> +            .dig_subframe = { 0 },
>> +        }
>> +    };
>> +    int ret;
>> +
>> +    dev_dbg(dai->dev, "%s() width %d rate %d channels %d\n", __func__,
>> +        params_width(params), params_rate(params),
>> +        params_channels(params));
>> +
>> +    ret = snd_pcm_create_iec958_consumer_hw_params(params,
>> hp.iec.status,
>> +                               sizeof(hp.iec.status));
> Tell me if i am wrong, but in case of SPDIF format, IEC status is
> managed by cpu_dai not by the codec.
> To manage IEC61937 a control should be needed to update IEC status bits...
>

AFAIK yes. However, the video side implementation is free to ignore 
status bits in the struct hdmi_codec_params and it should normally do so 
if the format is SPDIF. Of course I could save couple CPU cycles in the 
codec code if would not even produce the bits when the format is SPDIF.

Best regards,
Jyri

>> +    if (ret < 0) {
>> +        dev_err(dai->dev, "Creating IEC958 channel status failed %d\n",
>> +            ret);
>> +        return ret;
>> +    }
>> +
>> +    ret = hdmi_codec_new_stream(substream, dai);
>> +    if (ret)
>> +        return ret;
>> +
>> +    hdmi_audio_infoframe_init(&hp.cea);
>> +    hp.cea.channels = params_channels(params);
>> +    hp.cea.coding_type = HDMI_AUDIO_CODING_TYPE_STREAM;
>> +    hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_STREAM;
>> +    hp.cea.sample_frequency = HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM;
>> +
>> +    hp.sample_width = params_width(params);
>> +    hp.sample_rate = params_rate(params);
>> +    hp.channels = params_channels(params);
>> +
>> +    return hcp->hcd.ops->hw_params(dai->dev->parent,
>> &hcp->daifmt[dai->id],
>> +                       &hp);
>> +}
>> +

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel