[RFC PATCH 1/4] alsa: make hw_params negotiation infrastructure 'bclk_ratio aware'

[RFC PATCH 1/4] alsa: make hw_params negotiation infrastructure 'bclk_ratio aware'

[Sven Van Asbroeck]

Negotiation seems to work ok, but bclk_ratio is exposed to
userspace via snd_pcm_hw_params, which is not acceptable.

Constrain bclk_ratio by:
- cpu   dai capabilities && rules
- codec dai capabilities && rules
- minimum bclk_ratio is sample_width * channels

In hw_params_choose(), pick the smallest supported bclk_ratio,
which should correspond to the most efficient solution.

If cpu and codec dais do not specify or constrain supported
bclk_ratios, alsa will pick sample_width * channels.

Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
 include/sound/pcm.h         | 11 +++++++++++
 include/sound/soc.h         |  2 ++
 include/uapi/sound/asound.h |  5 +++--
 sound/core/pcm_native.c     | 34 +++++++++++++++++++++++++++++++++-
 sound/soc/soc-pcm.c         |  8 ++++++++
 5 files changed, 57 insertions(+), 3 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index d6bd3caf6878..71ac7e8de23d 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -56,6 +56,8 @@ struct snd_pcm_hardware {
 	unsigned int periods_min;	/* min # of periods */
 	unsigned int periods_max;	/* max # of periods */
 	size_t fifo_size;		/* fifo size in bytes */
+	unsigned int bclk_ratio_min;	/* min bclk ratio for wire format */
+	unsigned int bclk_ratio_max;	/* max bclk ratio for wire format */
 };
 
 struct snd_pcm_substream;
@@ -980,6 +982,15 @@ static inline unsigned int params_buffer_bytes(const struct snd_pcm_hw_params *p
 	return hw_param_interval_c(p, SNDRV_PCM_HW_PARAM_BUFFER_BYTES)->min;
 }
 
+/**
+ * params_bclk_ratio - Get the bclk_ratio from the hw params
+ * @p: hw params
+ */
+static inline unsigned int params_bclk_ratio(const struct snd_pcm_hw_params *p)
+{
+	return hw_param_interval_c(p, SNDRV_PCM_HW_PARAM_BCLK_RATIO)->min;
+}
+
 int snd_interval_refine(struct snd_interval *i, const struct snd_interval *v);
 int snd_interval_list(struct snd_interval *i, unsigned int count,
 		      const unsigned int *list, unsigned int mask);
diff --git a/include/sound/soc.h b/include/sound/soc.h
index e665f111b0d2..96d669423688 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -732,6 +732,8 @@ struct snd_soc_pcm_stream {
 	unsigned int channels_min;	/* min channels */
 	unsigned int channels_max;	/* max channels */
 	unsigned int sig_bits;		/* number of bits of content */
+	unsigned int bclk_ratio_min;	/* min bclk ratio for wire format */
+	unsigned int bclk_ratio_max;	/* max bclk ratio for wire format */
 };
 
 /* SoC audio ops */
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 404d4b9ffe76..c3ea94eaaa77 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -371,8 +371,9 @@ typedef int snd_pcm_hw_param_t;
 #define	SNDRV_PCM_HW_PARAM_BUFFER_SIZE	17	/* Size of buffer in frames */
 #define	SNDRV_PCM_HW_PARAM_BUFFER_BYTES	18	/* Size of buffer in bytes */
 #define	SNDRV_PCM_HW_PARAM_TICK_TIME	19	/* Approx tick duration in us */
+#define SNDRV_PCM_HW_PARAM_BCLK_RATIO	20	/* bclk_ratio for wire format */
 #define	SNDRV_PCM_HW_PARAM_FIRST_INTERVAL	SNDRV_PCM_HW_PARAM_SAMPLE_BITS
-#define	SNDRV_PCM_HW_PARAM_LAST_INTERVAL	SNDRV_PCM_HW_PARAM_TICK_TIME
+#define	SNDRV_PCM_HW_PARAM_LAST_INTERVAL	SNDRV_PCM_HW_PARAM_BCLK_RATIO
 
 #define SNDRV_PCM_HW_PARAMS_NORESAMPLE	(1<<0)	/* avoid rate resampling */
 #define SNDRV_PCM_HW_PARAMS_EXPORT_BUFFER	(1<<1)	/* export buffer */
@@ -399,7 +400,7 @@ struct snd_pcm_hw_params {
 	struct snd_mask mres[5];	/* reserved masks */
 	struct snd_interval intervals[SNDRV_PCM_HW_PARAM_LAST_INTERVAL -
 				        SNDRV_PCM_HW_PARAM_FIRST_INTERVAL + 1];
-	struct snd_interval ires[9];	/* reserved intervals */
+	struct snd_interval ires[8];	/* reserved intervals */
 	unsigned int rmask;		/* W: requested masks */
 	unsigned int cmask;		/* R: changed masks */
 	unsigned int info;		/* R: Info flags for returned setup */
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 818dff1de545..23dbe43a6691 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -516,6 +516,7 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
 		      struct snd_pcm_hw_params *params)
 {
 	int err;
+	struct snd_interval *r = hw_param_interval(params, SNDRV_PCM_HW_PARAM_BCLK_RATIO);
 
 	params->info = 0;
 	params->fifo_size = 0;
@@ -525,6 +526,12 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
 		params->rate_num = 0;
 		params->rate_den = 0;
 	}
+	/*
+	 * if left zero (not empty), assume userspace is oblivious, and
+	 * completely flexible
+	 */
+	if (snd_interval_single(r) && snd_interval_min(r) == 0)
+		snd_interval_any(r);
 
 	err = constrain_mask_params(substream, params);
 	if (err < 0)
@@ -610,7 +617,8 @@ static inline void snd_pcm_timer_notify(struct snd_pcm_substream *substream,
  * Choose one configuration from configuration space defined by @params.
  * The configuration chosen is that obtained fixing in this order:
  * first access, first format, first subformat, min channels,
- * min rate, min period time, max buffer size, min tick time
+ * min rate, min period time, max buffer size, min tick time,
+ * min bclk_ratio
  *
  * Return: Zero if successful, or a negative error code on failure.
  */
@@ -626,6 +634,7 @@ static int snd_pcm_hw_params_choose(struct snd_pcm_substream *pcm,
 		SNDRV_PCM_HW_PARAM_PERIOD_TIME,
 		SNDRV_PCM_HW_PARAM_BUFFER_SIZE,
 		SNDRV_PCM_HW_PARAM_TICK_TIME,
+		SNDRV_PCM_HW_PARAM_BCLK_RATIO,
 		-1
 	};
 	const int *v;
@@ -2100,6 +2109,18 @@ static int snd_pcm_hw_rule_format(struct snd_pcm_hw_params *params,
 	return snd_mask_refine(mask, &m);
 }
 
+static int snd_pcm_hw_rule_bclk_ratio(struct snd_pcm_hw_params *params,
+				  struct snd_pcm_hw_rule *rule)
+{
+	struct snd_interval i;
+	struct snd_interval *ratios = hw_param_interval(params, SNDRV_PCM_HW_PARAM_BCLK_RATIO);
+	snd_interval_any(&i);
+	i.openmax = 1;
+	i.min = params_channels(params) * params_width(params);
+	i.integer = 1;
+	return snd_interval_refine(ratios, &i);
+}
+
 static int snd_pcm_hw_rule_sample_bits(struct snd_pcm_hw_params *params,
 				       struct snd_pcm_hw_rule *rule)
 {
@@ -2180,12 +2201,18 @@ int snd_pcm_hw_constraints_init(struct snd_pcm_substream *substream)
 	snd_interval_setinteger(constrs_interval(constrs, SNDRV_PCM_HW_PARAM_BUFFER_BYTES));
 	snd_interval_setinteger(constrs_interval(constrs, SNDRV_PCM_HW_PARAM_SAMPLE_BITS));
 	snd_interval_setinteger(constrs_interval(constrs, SNDRV_PCM_HW_PARAM_FRAME_BITS));
+	snd_interval_setinteger(constrs_interval(constrs, SNDRV_PCM_HW_PARAM_BCLK_RATIO));
 
 	err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_FORMAT,
 				   snd_pcm_hw_rule_format, NULL,
 				   SNDRV_PCM_HW_PARAM_SAMPLE_BITS, -1);
 	if (err < 0)
 		return err;
+	err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_BCLK_RATIO,
+				   snd_pcm_hw_rule_bclk_ratio, NULL,
+				   SNDRV_PCM_HW_PARAM_SAMPLE_BITS, SNDRV_PCM_HW_PARAM_CHANNELS, -1);
+	if (err < 0)
+		return err;
 	err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_SAMPLE_BITS, 
 				  snd_pcm_hw_rule_sample_bits, NULL,
 				  SNDRV_PCM_HW_PARAM_FORMAT, 
@@ -2341,6 +2368,11 @@ int snd_pcm_hw_constraints_complete(struct snd_pcm_substream *substream)
 	if (err < 0)
 		return err;
 
+	err = snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_BCLK_RATIO,
+					   hw->bclk_ratio_min, hw->bclk_ratio_max);
+	if (err < 0)
+		return err;
+
 	err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_BUFFER_BYTES, 
 				  snd_pcm_hw_rule_buffer_bytes_max, substream,
 				  SNDRV_PCM_HW_PARAM_BUFFER_BYTES, -1);
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 03f36e534050..747026595634 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -381,6 +381,7 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream)
 	struct snd_soc_pcm_stream *cpu_stream;
 	unsigned int chan_min = 0, chan_max = UINT_MAX;
 	unsigned int rate_min = 0, rate_max = UINT_MAX;
+	unsigned int bclk_ratio_min = 0, bclk_ratio_max = UINT_MAX;
 	unsigned int rates = UINT_MAX;
 	u64 formats = ULLONG_MAX;
 	int i;
@@ -413,6 +414,8 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream)
 			codec_stream = &codec_dai_drv->capture;
 		chan_min = max(chan_min, codec_stream->channels_min);
 		chan_max = min(chan_max, codec_stream->channels_max);
+		bclk_ratio_min = max(bclk_ratio_min, codec_stream->bclk_ratio_min);
+		bclk_ratio_max = min_not_zero(bclk_ratio_max, codec_stream->bclk_ratio_max);
 		rate_min = max(rate_min, codec_stream->rate_min);
 		rate_max = min_not_zero(rate_max, codec_stream->rate_max);
 		formats &= codec_stream->formats;
@@ -443,6 +446,11 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream)
 	hw->rate_min = max(hw->rate_min, rate_min);
 	hw->rate_max = min_not_zero(hw->rate_max, cpu_stream->rate_max);
 	hw->rate_max = min_not_zero(hw->rate_max, rate_max);
+
+	hw->bclk_ratio_min = max(hw->bclk_ratio_min, cpu_stream->bclk_ratio_min);
+	hw->bclk_ratio_min = max(hw->bclk_ratio_min, bclk_ratio_min);
+	hw->bclk_ratio_max = min_not_zero(hw->bclk_ratio_max, cpu_stream->bclk_ratio_max);
+	hw->bclk_ratio_max = min_not_zero(hw->bclk_ratio_max, bclk_ratio_max);
 }
 
 static int soc_pcm_components_close(struct snd_pcm_substream *substream,
-- 
2.17.1

[RFC PATCH 2/4] ASoC: hdmi-codec: add support for bclk_ratio

[Sven Van Asbroeck]

Retrieve the bclk_ratio from the hw_params and provide it
to the HDMI bridge.

Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
 include/sound/hdmi-codec.h    | 1 +
 sound/soc/codecs/hdmi-codec.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h
index 9483c55f871b..50062c773a12 100644
--- a/include/sound/hdmi-codec.h
+++ b/include/sound/hdmi-codec.h
@@ -53,6 +53,7 @@ struct hdmi_codec_params {
 	int sample_rate;
 	int sample_width;
 	int channels;
+	int bclk_ratio;
 };
 
 struct hdmi_codec_pdata;
diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
index e5b6769b9797..6a457278fe6d 100644
--- a/sound/soc/codecs/hdmi-codec.c
+++ b/sound/soc/codecs/hdmi-codec.c
@@ -519,6 +519,7 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
 	hp.sample_width = params_width(params);
 	hp.sample_rate = params_rate(params);
 	hp.channels = params_channels(params);
+	hp.bclk_ratio = params_bclk_ratio(params);
 
 	return hcp->hcd.ops->hw_params(dai->dev->parent, hcp->hcd.data,
 				       &hcp->daifmt[dai->id], &hp);
-- 
2.17.1

[RFC PATCH 3/4] drm/i2c: tda998x: calculate CTS_N directly from the bclk_ratio

[Sven Van Asbroeck]

Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 20 ++++++++++++++------
 include/drm/i2c/tda998x.h         |  1 +
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index a7c39f39793f..caccfaf90dd2 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -893,19 +893,26 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 		reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S);
 		clksel_aip = AIP_CLKSEL_AIP_I2S;
 		clksel_fs = AIP_CLKSEL_FS_ACLK;
-		switch (params->sample_width) {
+		switch (params->bclk_ratio) {
 		case 16:
+			cts_n = CTS_N_M(3) | CTS_N_K(0);
+			break;
+		case 32:
 			cts_n = CTS_N_M(3) | CTS_N_K(1);
 			break;
-		case 18:
-		case 20:
-		case 24:
+		case 48:
 			cts_n = CTS_N_M(3) | CTS_N_K(2);
 			break;
-		default:
-		case 32:
+		case 64:
 			cts_n = CTS_N_M(3) | CTS_N_K(3);
 			break;
+		case 128:
+			cts_n = CTS_N_M(0) | CTS_N_K(0);
+			break;
+		default:
+			dev_err(&priv->hdmi->dev,
+				"unsupported I2S bclk ratio\n");
+			return -EINVAL;
 		}
 		break;
 
@@ -984,6 +991,7 @@ static int tda998x_audio_hw_params(struct device *dev, void *data,
 	struct tda998x_audio_params audio = {
 		.sample_width = params->sample_width,
 		.sample_rate = params->sample_rate,
+		.bclk_ratio = params->bclk_ratio,
 		.cea = params->cea,
 	};
 
diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
index 3cb25ccbe5e6..62d85c662aa1 100644
--- a/include/drm/i2c/tda998x.h
+++ b/include/drm/i2c/tda998x.h
@@ -16,6 +16,7 @@ struct tda998x_audio_params {
 	u8 format;
 	unsigned sample_width;
 	unsigned sample_rate;
+	unsigned int bclk_ratio;
 	struct hdmi_audio_infoframe cea;
 	u8 status[5];
 };
-- 
2.17.1

[RFC PATCH 4/4] ASoC: fsl_ssi: constrain bclk_ratio in i2s master mode

[Sven Van Asbroeck]

The fsl_ssi always has a bclk_ratio of 64 in ssi master
mode, no matter the configured word length.

Add a rule which constrains the bclk_ratio to 64 in
ssi master mode.

Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
 sound/soc/fsl/fsl_ssi.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 0a648229e643..1c5b674fe253 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -627,6 +627,29 @@ static void fsl_ssi_setup_ac97(struct fsl_ssi *ssi)
 	regmap_write(regs, REG_SSI_SOR, SSI_SOR_WAIT(3));
 }
 
+static int fsl_ssi_hw_rule_i2s_master(struct snd_pcm_hw_params *params,
+						struct snd_pcm_hw_rule *rule)
+{
+	struct snd_interval *it = hw_param_interval(params,
+					SNDRV_PCM_HW_PARAM_BCLK_RATIO);
+	struct fsl_ssi *ssi = rule->private;
+	struct snd_interval t;
+
+	if (!fsl_ssi_is_i2s_master(ssi))
+		return 0;
+
+	/*
+	 * In i2s master mode, the ssi always generates 32 physical
+	 * bits/channel. This mode always has 2 channels.
+	 * This results in a fixed bclk_ratio of 64.
+	 */
+	memset(&t, 0, sizeof(t));
+	t.min = t.max = 64;
+	t.integer = 1;
+
+	return snd_interval_refine(it, &t);
+}
+
 static int fsl_ssi_startup(struct snd_pcm_substream *substream,
 			   struct snd_soc_dai *dai)
 {
@@ -648,6 +671,12 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream,
 		snd_pcm_hw_constraint_step(substream->runtime, 0,
 					   SNDRV_PCM_HW_PARAM_PERIOD_SIZE, 2);
 
+	snd_pcm_hw_rule_add(substream->runtime, 0,
+		SNDRV_PCM_HW_PARAM_BCLK_RATIO,
+		fsl_ssi_hw_rule_i2s_master, ssi,
+		SNDRV_PCM_HW_PARAM_FORMAT, SNDRV_PCM_HW_PARAM_CHANNELS,
+		-1);
+
 	return 0;
 }
 
-- 
2.17.1

Re: [RFC PATCH 1/4] alsa: make hw_params negotiation infrastructure 'bclk_ratio aware'

[Takashi Sakamoto]

Hi,

On Mon, Mar 04, 2019 at 11:59:52AM -0500, Sven Van Asbroeck wrote:
> Negotiation seems to work ok, but bclk_ratio is exposed to
> userspace via snd_pcm_hw_params, which is not acceptable.
> 
> Constrain bclk_ratio by:
> - cpu   dai capabilities && rules
> - codec dai capabilities && rules
> - minimum bclk_ratio is sample_width * channels
> 
> In hw_params_choose(), pick the smallest supported bclk_ratio,
> which should correspond to the most efficient solution.
> 
> If cpu and codec dais do not specify or constrain supported
> bclk_ratios, alsa will pick sample_width * channels.
> 
> Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
> ---
>  include/sound/pcm.h         | 11 +++++++++++
>  include/sound/soc.h         |  2 ++
>  include/uapi/sound/asound.h |  5 +++--
>  sound/core/pcm_native.c     | 34 +++++++++++++++++++++++++++++++++-
>  sound/soc/soc-pcm.c         |  8 ++++++++
>  5 files changed, 57 insertions(+), 3 deletions(-)

In UAPI of Linux sound subsystem, sample formats are represented
in enumerators prefixed with 'SNDRV_PCM_FORMAT_'. In the enumerators,
sample bits and padding bits are represented:

              S8 S16 S20 S24 S32 S18_3 S20_3 S24_3
sample-bits:   8  16  20  24  32  18    20    24 (=width)
padding-bits:  0   0  12   8   0   6    12     0
total bits:    8  16  32  32  32  24    24    24 (=physical_width)
 
When indicating a certain bit ratio of BCLK/WS from userspace,
applications use correct combination of the above format (=physical_width)
and the number of samples per audio data frame (=channels).

All of drivers can add constraints and rules for runtime of PCM substream
in its .open callback. As a result, applications can see available
combination of the format and the channels and choose correct combination.

In my opinion, your issue is a lack of the constraints and rules in
relevant drivers; perhaps in tda998x driver. Or core functionality of
ALSA SoC part has a lack of consideration about the above design of
ALSA PCM core and PCM interface. If a certain combination of CPU-dai and
CODEC-dai requires to ignore the above somehow, it should be done in
interaction between drivers for CPU-dai/CODEC-dai. In short, your issue
should not be exported to userspace.

I note that for your 'hypothetical' cpu dai[1], 20x2/20x2 mode
(physical_width=20, channels=2) is not available from userspace application.
We have no sample format suitable to it.


Anyway, when posting to change UAPI of Linux sound subsystem, it's
better to describe the reason fot new stuffs; what they're designed for,
and the reason to require them. Wnen posting in a result of series of
discussion done previously, it's better to add any reference to it. The
change effects all of userspace stuffs, regardless of whether they exist
or doesn't.  Furthermore the change has forced application developers to
take care of codes newly added. Additionally, it's better to note actual
example of applications with the added code. Unless, no one can judge
that the added code is enough abstracted for an application to handle
various type of hardware by the same code.

[1] https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/146062.html


Regards

Takashi Sakamoto

Re: [RFC PATCH 1/4] alsa: make hw_params negotiation infrastructure 'bclk_ratio aware'

[Russell King - ARM Linux admin]

On Tue, Mar 05, 2019 at 01:42:32PM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> On Mon, Mar 04, 2019 at 11:59:52AM -0500, Sven Van Asbroeck wrote:
> > Negotiation seems to work ok, but bclk_ratio is exposed to
> > userspace via snd_pcm_hw_params, which is not acceptable.
> > 
> > Constrain bclk_ratio by:
> > - cpu   dai capabilities && rules
> > - codec dai capabilities && rules
> > - minimum bclk_ratio is sample_width * channels
> > 
> > In hw_params_choose(), pick the smallest supported bclk_ratio,
> > which should correspond to the most efficient solution.
> > 
> > If cpu and codec dais do not specify or constrain supported
> > bclk_ratios, alsa will pick sample_width * channels.
> > 
> > Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
> > ---
> >  include/sound/pcm.h         | 11 +++++++++++
> >  include/sound/soc.h         |  2 ++
> >  include/uapi/sound/asound.h |  5 +++--
> >  sound/core/pcm_native.c     | 34 +++++++++++++++++++++++++++++++++-
> >  sound/soc/soc-pcm.c         |  8 ++++++++
> >  5 files changed, 57 insertions(+), 3 deletions(-)
> 
> In UAPI of Linux sound subsystem, sample formats are represented
> in enumerators prefixed with 'SNDRV_PCM_FORMAT_'. In the enumerators,
> sample bits and padding bits are represented:
> 
>               S8 S16 S20 S24 S32 S18_3 S20_3 S24_3
> sample-bits:   8  16  20  24  32  18    20    24 (=width)
> padding-bits:  0   0  12   8   0   6    12     0
> total bits:    8  16  32  32  32  24    24    24 (=physical_width)
>  
> When indicating a certain bit ratio of BCLK/WS from userspace,
> applications use correct combination of the above format (=physical_width)
> and the number of samples per audio data frame (=channels).

You seem to be conflating the in-memory format with the on-wire format.
These are two entirely different things.  Your table above clearly shows
the in-memory format, not the on-wire format.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC PATCH 1/4] alsa: make hw_params negotiation infrastructure 'bclk_ratio aware'

[Sven Van Asbroeck]

Hi Takashi,

On Mon, Mar 4, 2019 at 11:42 PM Takashi Sakamoto
<o-takashi@sakamocchi.jp> wrote:
>
> Anyway, when posting to change UAPI of Linux sound subsystem, it's
> better to describe the reason fot new stuffs; what they're designed for,
> and the reason to require them. Wnen posting in a result of series of
> discussion done previously, it's better to add any reference to it.

My apologies for not linking in previous discussions, I'll certainly make
sure to link in the future.

The original problem was that the fsl_ssi did not appear to work with the
tda998x hdmi codec. This codec seems to be 'unique' in the sense that it
needs to know the number of clocks per frame on the wire, i.e. the bclk_ratio,
which no-one in alsa is providing or using at the moment.

I first made the error of conflating the physical width and the bclk_ratio,
and posted this patch - Russell King quickly pointed out my error:
https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/145916.html

This then led to the following discussions:
https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/thread.html#145985
https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/thread.html#145947

Most of the discussion is about a mechanism to convey bclk_ratio from
the frame master
to the tda998x. We don't yet seem to have a consensus on how to do this best.

My rfc patch was only intended to provoke discussion, and to allow
people to experiment
with a flawed solution that would allow the alsa core to negotiate
bclk_ratio between
components. The uapi change is a serious flaw. bclk_ratio negotiation should be
invisible to userspace. But I cannot see a way to accomplish this.

Sven

Re: [RFC PATCH 1/4] alsa: make hw_params negotiation infrastructure 'bclk_ratio aware'

[Jaroslav Kysela]

Dne 05. 03. 19 v 15:23 Sven Van Asbroeck napsal(a):
> Hi Takashi,
> 
> On Mon, Mar 4, 2019 at 11:42 PM Takashi Sakamoto
> <o-takashi@sakamocchi.jp> wrote:
>>
>> Anyway, when posting to change UAPI of Linux sound subsystem, it's
>> better to describe the reason fot new stuffs; what they're designed for,
>> and the reason to require them. Wnen posting in a result of series of
>> discussion done previously, it's better to add any reference to it.
> 
> My apologies for not linking in previous discussions, I'll certainly make
> sure to link in the future.
> 
> The original problem was that the fsl_ssi did not appear to work with the
> tda998x hdmi codec. This codec seems to be 'unique' in the sense that it
> needs to know the number of clocks per frame on the wire, i.e. the bclk_ratio,
> which no-one in alsa is providing or using at the moment.
> 
> I first made the error of conflating the physical width and the bclk_ratio,
> and posted this patch - Russell King quickly pointed out my error:
> https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/145916.html
> 
> This then led to the following discussions:
> https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/thread.html#145985
> https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/thread.html#145947
> 
> Most of the discussion is about a mechanism to convey bclk_ratio from
> the frame master
> to the tda998x. We don't yet seem to have a consensus on how to do this best.
> 
> My rfc patch was only intended to provoke discussion, and to allow
> people to experiment
> with a flawed solution that would allow the alsa core to negotiate
> bclk_ratio between
> components. The uapi change is a serious flaw. bclk_ratio negotiation should be
> invisible to userspace. But I cannot see a way to accomplish this.

I think that this might be resolved using double constraint rules in the
affected driver:

    snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_FORMAT,
                        bclk_format_constraint, substream,
                                  SNDRV_PCM_HW_PARAM_CHANNELS, -1);
    snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS,
                        bclk_channels_constraint, substream,
                                  SNDRV_PCM_HW_PARAM_FORMAT, -1);

And refine the format bitmask / channel interval according your bclk
constraint. The physical sample width can be obtained in both
bclk_format_constraint / bclk_channels_constraint function so all input
values are known. The final bclk value can be obtained like in your
proposed code:

    params_channels(params) * params_width(params)

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [RFC PATCH 1/4] alsa: make hw_params negotiation infrastructure 'bclk_ratio aware'

[Takashi Sakamoto]

On Tue, Mar 05, 2019 at 09:23:46AM -0500, Sven Van Asbroeck wrote:
> The original problem was that the fsl_ssi did not appear to work with the
> tda998x hdmi codec. This codec seems to be 'unique' in the sense that it
> needs to know the number of clocks per frame on the wire, i.e. the bclk_ratio,
> which no-one in alsa is providing or using at the moment.
> 
> I first made the error of conflating the physical width and the bclk_ratio,
> and posted this patch - Russell King quickly pointed out my error:
> https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/145916.html
> 
> This then led to the following discussions:
> https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/thread.html#145985
> https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/thread.html#145947
> 
> Most of the discussion is about a mechanism to convey bclk_ratio from
> the frame master
> to the tda998x. We don't yet seem to have a consensus on how to do this best.
> 
> My rfc patch was only intended to provoke discussion, and to allow
> people to experiment
> with a flawed solution that would allow the alsa core to negotiate
> bclk_ratio between
> components. The uapi change is a serious flaw. bclk_ratio negotiation should be
> invisible to userspace. But I cannot see a way to accomplish this.

In your case:

+---+            +-----+
|CPU| <- wire -> |CODEC|
|DAI|            | DAI |
+---+            +-----+

So that:

CPU-DAI = fsl_ssi
CODEC-DAI = tda998x
wire = I2S

In I2S:
 - SCK-line = serial clock
 - WS-line = word select
 - SD-line = serial data 

In general I2S communication:
 - 2 samples are transferred in a phase of WS

In my opinion:
 - 'the number of clocks per frame on the wire' (=you need)
   = the number of phases of SCK per phase of WS

In expectation of ALSA PCM interface for hardware for usual device:
 - half number of phases of SCK per phase of WC
   = physical_width of sample
   = bytes per sample
 - the number of phases of SCK per phase of WC
   = physical_width * channels (=2)
   = bytes per frame
 - the ratio between phases of SCK and phases of WC per second
   = rate

Here, usual device is expected to give FIFO for DMA and I2S bus,
synchronized to SCK without any conversion. CODED-DAI also has
FIFO to process signals.

In my opinion, if both of fsl_ssi and tda988x have enough constraints
and rules for runtime of PCM substream about the physical_width, channels
and rate, things look to work well. Perhaps, tda998x driver should have a
condition statement for its role of 'frame master' to add the constraints
and rules, to describe its quirk.

It's helpful to describe the 'unique'ness of tda998x hdmi codec for
the detail.


Regards

Takashi Sakamoto (not maintainer of this subsystem)

Re: [RFC PATCH 1/4] alsa: make hw_params negotiation infrastructure 'bclk_ratio aware'

[Russell King - ARM Linux admin]

On Fri, Mar 08, 2019 at 01:10:57PM +0900, Takashi Sakamoto wrote:
> On Tue, Mar 05, 2019 at 09:23:46AM -0500, Sven Van Asbroeck wrote:
> > The original problem was that the fsl_ssi did not appear to work with the
> > tda998x hdmi codec. This codec seems to be 'unique' in the sense that it
> > needs to know the number of clocks per frame on the wire, i.e. the bclk_ratio,
> > which no-one in alsa is providing or using at the moment.
> > 
> > I first made the error of conflating the physical width and the bclk_ratio,
> > and posted this patch - Russell King quickly pointed out my error:
> > https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/145916.html
> > 
> > This then led to the following discussions:
> > https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/thread.html#145985
> > https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/thread.html#145947
> > 
> > Most of the discussion is about a mechanism to convey bclk_ratio from
> > the frame master
> > to the tda998x. We don't yet seem to have a consensus on how to do this best.
> > 
> > My rfc patch was only intended to provoke discussion, and to allow
> > people to experiment
> > with a flawed solution that would allow the alsa core to negotiate
> > bclk_ratio between
> > components. The uapi change is a serious flaw. bclk_ratio negotiation should be
> > invisible to userspace. But I cannot see a way to accomplish this.
> 
> In your case:
> 
> +---+            +-----+
> |CPU| <- wire -> |CODEC|
> |DAI|            | DAI |
> +---+            +-----+
> 
> So that:
> 
> CPU-DAI = fsl_ssi
> CODEC-DAI = tda998x
> wire = I2S
> 
> In I2S:
>  - SCK-line = serial clock
>  - WS-line = word select
>  - SD-line = serial data 
> 
> In general I2S communication:
>  - 2 samples are transferred in a phase of WS
> 
> In my opinion:
>  - 'the number of clocks per frame on the wire' (=you need)
>    = the number of phases of SCK per phase of WS
> 
> In expectation of ALSA PCM interface for hardware for usual device:
>  - half number of phases of SCK per phase of WC
>    = physical_width of sample
>    = bytes per sample

They are not the same thing.

Let's take SNDRV_PCM_FORMAT_S16_LE.  The in-memory layout of this format
is two 16-bit samples next to each other in a single 32-bit word.  Their
width is 16, their physical_width is 16, and bytes per sample is 2.

A CPU DAI can do one of two things:

1) it can generate exactly 16 SCK clock cycles per sample before WS
   changes state, leading to a total of 32 SCK clock cycles per
   frame.

2) it can generate more than 16 SCK clock cycles per sample.

Both are entirely legal and permissable under the I2S specification.
Both look the same in memory.

The ALSA format specification (SNDRV_PCM_FORMAT_*) does not specify
which of these two is used on the wire - it only specifies the in-
memory format.

If it were to specify the on-wire format, then we'd have to multiply
the number of in-memory formats by the number of on-wire formats.
These are (at least): AC'97, SPDIF, I2S(Philips), I2S(Left justified),
I2S(Right justified), two different DSP formats, and PDM.  Then for
at least the tree I2S modes, there are the number of SCK clocks per
sample which can be anything from the number of bits in the sample
up to an undefined limit.

What this means is that multiplying the number of in-memory formats
by the number of unboundable bus-specific formats leads to an
unboundable quantity of formats.

This is why ASoC has the ability to set the bclk (bit clock, SCK)
to sample rate ratio - in other words, the number of clocks to
completely transmit the samples for the number of channels on the
link - bit clock rate = sample rate * bclk_ratio.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC PATCH 1/4] alsa: make hw_params negotiation infrastructure 'bclk_ratio aware'

[Russell King - ARM Linux admin]

On Fri, Mar 08, 2019 at 12:59:16PM +0000, Russell King - ARM Linux admin wrote:
> On Fri, Mar 08, 2019 at 01:10:57PM +0900, Takashi Sakamoto wrote:
> > On Tue, Mar 05, 2019 at 09:23:46AM -0500, Sven Van Asbroeck wrote:
> > > The original problem was that the fsl_ssi did not appear to work with the
> > > tda998x hdmi codec. This codec seems to be 'unique' in the sense that it
> > > needs to know the number of clocks per frame on the wire, i.e. the bclk_ratio,
> > > which no-one in alsa is providing or using at the moment.
> > > 
> > > I first made the error of conflating the physical width and the bclk_ratio,
> > > and posted this patch - Russell King quickly pointed out my error:
> > > https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/145916.html
> > > 
> > > This then led to the following discussions:
> > > https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/thread.html#145985
> > > https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/thread.html#145947
> > > 
> > > Most of the discussion is about a mechanism to convey bclk_ratio from
> > > the frame master
> > > to the tda998x. We don't yet seem to have a consensus on how to do this best.
> > > 
> > > My rfc patch was only intended to provoke discussion, and to allow
> > > people to experiment
> > > with a flawed solution that would allow the alsa core to negotiate
> > > bclk_ratio between
> > > components. The uapi change is a serious flaw. bclk_ratio negotiation should be
> > > invisible to userspace. But I cannot see a way to accomplish this.
> > 
> > In your case:
> > 
> > +---+            +-----+
> > |CPU| <- wire -> |CODEC|
> > |DAI|            | DAI |
> > +---+            +-----+
> > 
> > So that:
> > 
> > CPU-DAI = fsl_ssi
> > CODEC-DAI = tda998x
> > wire = I2S
> > 
> > In I2S:
> >  - SCK-line = serial clock
> >  - WS-line = word select
> >  - SD-line = serial data 
> > 
> > In general I2S communication:
> >  - 2 samples are transferred in a phase of WS
> > 
> > In my opinion:
> >  - 'the number of clocks per frame on the wire' (=you need)
> >    = the number of phases of SCK per phase of WS
> > 
> > In expectation of ALSA PCM interface for hardware for usual device:
> >  - half number of phases of SCK per phase of WC
> >    = physical_width of sample
> >    = bytes per sample
> 
> They are not the same thing.
> 
> Let's take SNDRV_PCM_FORMAT_S16_LE.  The in-memory layout of this format
> is two 16-bit samples next to each other in a single 32-bit word.  Their
> width is 16, their physical_width is 16, and bytes per sample is 2.
> 
> A CPU DAI can do one of two things:
> 
> 1) it can generate exactly 16 SCK clock cycles per sample before WS
>    changes state, leading to a total of 32 SCK clock cycles per
>    frame.
> 
> 2) it can generate more than 16 SCK clock cycles per sample.
> 
> Both are entirely legal and permissable under the I2S specification.
> Both look the same in memory.
> 
> The ALSA format specification (SNDRV_PCM_FORMAT_*) does not specify
> which of these two is used on the wire - it only specifies the in-
> memory format.
> 
> If it were to specify the on-wire format, then we'd have to multiply
> the number of in-memory formats by the number of on-wire formats.
> These are (at least): AC'97, SPDIF, I2S(Philips), I2S(Left justified),
> I2S(Right justified), two different DSP formats, and PDM.  Then for
> at least the tree I2S modes, there are the number of SCK clocks per
> sample which can be anything from the number of bits in the sample
> up to an undefined limit.
> 
> What this means is that multiplying the number of in-memory formats
> by the number of unboundable bus-specific formats leads to an
> unboundable quantity of formats.

I missed stating another point in that.  Let's say that we were to have
definitions such as:

SNDRV_PCM_FORMAT_S16_LE_I2S16
SNDRV_PCM_FORMAT_S16_LE_I2S32
SNDRV_PCM_FORMAT_S16_LE_SPDIF

We have a CPU interface that can simultaneously produce both
SNDRV_PCM_FORMAT_S16_LE_I2S32 and SNDRV_PCM_FORMAT_S16_LE_SPDIF.
However, the ALSA format negotiation is such that only _one_ format
can be negotiated between user space and kernel space.  So, one of
those has to be selected.  So, despite the hardware being perfectly
capable of producing both streams, combining the on-wire formats
with the in-memory formats means that we can't support simultaneous
operation, despite the hardware being perfectly capable.

Another point to make there is that in the general case, if a codec
supports reception of SNDRV_PCM_FORMAT_S16_LE_I2S16, then it supports
reception of SNDRV_PCM_FORMAT_S16_LE_I2S32 - the I2S bus specification
explicitly allows for more clocks than there are sample bits, and
states what happens to the extra bits.

The exception to that is when the receiving device uses the bit clock
(aka SCK) for some other purpose, such as TDA998x devices, where a
driver for such a device needs to know the ratio between the bus bit
clock and the sample rate.  As previously illustrated, ASoC has partial
support for this.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC PATCH 1/4] alsa: make hw_params negotiation infrastructure 'bclk_ratio aware'

[Takashi Sakamoto]

Hi,

On Fri, Mar 08, 2019 at 12:59:16PM +0000, Russell King - ARM Linux admin wrote:
> > In your case:
> > 
> > +---+            +-----+
> > |CPU| <- wire -> |CODEC|
> > |DAI|            | DAI |
> > +---+            +-----+
> > 
> > So that:
> > 
> > CPU-DAI = fsl_ssi
> > CODEC-DAI = tda998x
> > wire = I2S
> > 
> > In I2S:
> >  - SCK-line = serial clock
> >  - WS-line = word select
> >  - SD-line = serial data 
> > 
> > In general I2S communication:
> >  - 2 samples are transferred in a phase of WS
> > 
> > In my opinion:
> >  - 'the number of clocks per frame on the wire' (=you need)
> >    = the number of phases of SCK per phase of WS
> > 
> > In expectation of ALSA PCM interface for hardware for usual device:
> >  - half number of phases of SCK per phase of WC
> >    = physical_width of sample
> >    = bytes per sample
> 
> They are not the same thing.
> 
> Let's take SNDRV_PCM_FORMAT_S16_LE.  The in-memory layout of this format
> is two 16-bit samples next to each other in a single 32-bit word.  Their
> width is 16, their physical_width is 16, and bytes per sample is 2.
> 
> A CPU DAI can do one of two things:
> 
> 1) it can generate exactly 16 SCK clock cycles per sample before WS
>    changes state, leading to a total of 32 SCK clock cycles per
>    frame.
> 
> 2) it can generate more than 16 SCK clock cycles per sample.
>
> Both are entirely legal and permissable under the I2S specification.
> Both look the same in memory.
> 
> The ALSA format specification (SNDRV_PCM_FORMAT_*) does not specify
> which of these two is used on the wire - it only specifies the in-
> memory format.
> 
> If it were to specify the on-wire format, then we'd have to multiply
> the number of in-memory formats by the number of on-wire formats.
> These are (at least): AC'97, SPDIF, I2S(Philips), I2S(Left justified),
> I2S(Right justified), two different DSP formats, and PDM.  Then for
> at least the tree I2S modes, there are the number of SCK clocks per
> sample which can be anything from the number of bits in the sample
> up to an undefined limit.
> 
> What this means is that multiplying the number of in-memory formats
> by the number of unboundable bus-specific formats leads to an
> unboundable quantity of formats.
> 
> This is why ASoC has the ability to set the bclk (bit clock, SCK)
> to sample rate ratio - in other words, the number of clocks to
> completely transmit the samples for the number of channels on the
> link - bit clock rate = sample rate * bclk_ratio.

Hm. In ALSA PCM core, the combination of 'rate_num' and 'rate_den' is
another representation of sampling rate[1], and drivers can have
constraints and rules for these two parameters of runtime of PCM
substream. Once core functionalities and drivers in ALSA SoC part have
common specifications about actual value of these two parameters, the
issue could be solved, in my opinion.
(But I need time to consider it further in this weekend...)


However, I don't have clear view of your case 2) yet. In that case,
how drivers calculate return value in 'struct snd_pcm_ops.pointer'
callback? It should be frame count, but WS seems not to be available
for 16 bit sample because the number of SCK clock cycles per sample is
larger than 16.
(If WS clock works as I expected, SCK clock cycles per sample include
expression of padding bits added to 16 bit sample.)


[1] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/tree/include/sound/pcm.h#n380

Thanks

Takashi Sakamoto

Re: [RFC PATCH 1/4] alsa: make hw_params negotiation infrastructure 'bclk_ratio aware'

[Russell King - ARM Linux admin]

On Fri, Mar 08, 2019 at 11:29:46PM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> On Fri, Mar 08, 2019 at 12:59:16PM +0000, Russell King - ARM Linux admin wrote:
> > > In your case:
> > > 
> > > +---+            +-----+
> > > |CPU| <- wire -> |CODEC|
> > > |DAI|            | DAI |
> > > +---+            +-----+
> > > 
> > > So that:
> > > 
> > > CPU-DAI = fsl_ssi
> > > CODEC-DAI = tda998x
> > > wire = I2S
> > > 
> > > In I2S:
> > >  - SCK-line = serial clock
> > >  - WS-line = word select
> > >  - SD-line = serial data 
> > > 
> > > In general I2S communication:
> > >  - 2 samples are transferred in a phase of WS
> > > 
> > > In my opinion:
> > >  - 'the number of clocks per frame on the wire' (=you need)
> > >    = the number of phases of SCK per phase of WS
> > > 
> > > In expectation of ALSA PCM interface for hardware for usual device:
> > >  - half number of phases of SCK per phase of WC
> > >    = physical_width of sample
> > >    = bytes per sample
> > 
> > They are not the same thing.
> > 
> > Let's take SNDRV_PCM_FORMAT_S16_LE.  The in-memory layout of this format
> > is two 16-bit samples next to each other in a single 32-bit word.  Their
> > width is 16, their physical_width is 16, and bytes per sample is 2.
> > 
> > A CPU DAI can do one of two things:
> > 
> > 1) it can generate exactly 16 SCK clock cycles per sample before WS
> >    changes state, leading to a total of 32 SCK clock cycles per
> >    frame.
> > 
> > 2) it can generate more than 16 SCK clock cycles per sample.
> >
> > Both are entirely legal and permissable under the I2S specification.
> > Both look the same in memory.
> > 
> > The ALSA format specification (SNDRV_PCM_FORMAT_*) does not specify
> > which of these two is used on the wire - it only specifies the in-
> > memory format.
> > 
> > If it were to specify the on-wire format, then we'd have to multiply
> > the number of in-memory formats by the number of on-wire formats.
> > These are (at least): AC'97, SPDIF, I2S(Philips), I2S(Left justified),
> > I2S(Right justified), two different DSP formats, and PDM.  Then for
> > at least the tree I2S modes, there are the number of SCK clocks per
> > sample which can be anything from the number of bits in the sample
> > up to an undefined limit.
> > 
> > What this means is that multiplying the number of in-memory formats
> > by the number of unboundable bus-specific formats leads to an
> > unboundable quantity of formats.
> > 
> > This is why ASoC has the ability to set the bclk (bit clock, SCK)
> > to sample rate ratio - in other words, the number of clocks to
> > completely transmit the samples for the number of channels on the
> > link - bit clock rate = sample rate * bclk_ratio.
> 
> Hm. In ALSA PCM core, the combination of 'rate_num' and 'rate_den' is
> another representation of sampling rate[1], and drivers can have
> constraints and rules for these two parameters of runtime of PCM
> substream. Once core functionalities and drivers in ALSA SoC part have
> common specifications about actual value of these two parameters, the
> issue could be solved, in my opinion.
> (But I need time to consider it further in this weekend...)

rate_num and rate_den describe the sampling rate in fractional
notation, as described by the ALSA tracepoint documentation:

``rate_num``
        Read-only. This value represents numerator of sampling rate in fraction
        notation. Basically, when a parameter of SNDRV_PCM_HW_PARAM_RATE was
        decided as a single value, this value is also calculated according to
        it. Else, zero. But this behaviour depends on implementations in driver
        side.
``rate_den``
        Read-only. This value represents denominator of sampling rate in
        fraction notation. Basically, when a parameter of
        SNDRV_PCM_HW_PARAM_RATE was decided as a single value, this value is
        also calculated according to it. Else, zero. But this behaviour depends
        on implementations in driver side.

and is also described in the header files as:

        unsigned int rate_num;          /* R: rate numerator */
        unsigned int rate_den;          /* R: rate denominator */

params_rate() returns the rate from the interval negotiated with
userspace, and is described in the ALSA header files as:

#define SNDRV_PCM_HW_PARAM_RATE         11      /* Approx rate */

This only describes the sample rate, it does not describe anything to do
with bit clocks or their ratio to the sample rate.

> However, I don't have clear view of your case 2) yet. In that case,
> how drivers calculate return value in 'struct snd_pcm_ops.pointer'
> callback?  It should be frame count, but WS seems not to be available
> for 16 bit sample because the number of SCK clock cycles per sample is
> larger than 16.
> (If WS clock works as I expected, SCK clock cycles per sample include
> expression of padding bits added to 16 bit sample.)

snd_pcm_ops.pointer has nothing at all to do with what is happening on
the I2S bus.  It has nothing to do with the WS nor SCK.  Please see the
documentation in Documentation/sound/kernel-api, which states:

  pointer callback
  ~~~~~~~~~~~~~~~

  ::

    static snd_pcm_uframes_t snd_xxx_pointer(struct snd_pcm_substream *substream)

  This callback is called when the PCM middle layer inquires the current
  hardware position on the buffer. The position must be returned in
  frames, ranging from 0 to ``buffer_size - 1``.

  This is called usually from the buffer-update routine in the pcm
  middle layer, which is invoked when :c:func:`snd_pcm_period_elapsed()`
  is called in the interrupt routine. Then the pcm middle layer updates
  the position and calculates the available space, and wakes up the
  sleeping poll threads, etc.

This is the position within the memory buffer in frames (hence the
return type) where the hardware has read or written.  It does *not*
take account of any FIFO that may be between the DMA accessing the
memory buffer and the samples appearing on the wire to the codec.
ALSA models that effective delay using the "fifo_size" member of
struct snd_pcm_hardware.

ALSA defines frames as (also from the documentation):

  In the ALSA world, ``1 frame = channels \* samples-size``.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC PATCH 1/4] alsa: make hw_params negotiation infrastructure 'bclk_ratio aware'

[Mark Brown]

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

On Fri, Mar 08, 2019 at 12:59:16PM +0000, Russell King - ARM Linux admin wrote:
> On Fri, Mar 08, 2019 at 01:10:57PM +0900, Takashi Sakamoto wrote:

> > In expectation of ALSA PCM interface for hardware for usual device:
> >  - half number of phases of SCK per phase of WC
> >    = physical_width of sample
> >    = bytes per sample

> They are not the same thing.

> Let's take SNDRV_PCM_FORMAT_S16_LE.  The in-memory layout of this format
> is two 16-bit samples next to each other in a single 32-bit word.  Their
> width is 16, their physical_width is 16, and bytes per sample is 2.

> A CPU DAI can do one of two things:

> 1) it can generate exactly 16 SCK clock cycles per sample before WS
>    changes state, leading to a total of 32 SCK clock cycles per
>    frame.

> 2) it can generate more than 16 SCK clock cycles per sample.

> Both are entirely legal and permissable under the I2S specification.
> Both look the same in memory.

> The ALSA format specification (SNDRV_PCM_FORMAT_*) does not specify
> which of these two is used on the wire - it only specifies the in-
> memory format.

Everything Russell is saying here is correct.  The actual
ABI only affects the in memory format, userspace really shouldn't care
what's going on on the wire.  However we don't have separate
infrastructure for what goes on the wire and 90% of the time you can
just translate the memory layout into a wire layout which works so we're
conflating the two in a lot of places internally which is confusing and
fragile.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

Re: [RFC PATCH 1/4] alsa: make hw_params negotiation infrastructure 'bclk_ratio aware'

[Jaroslav Kysela]

Dne 08. 03. 19 v 18:22 Mark Brown napsal(a):
> On Fri, Mar 08, 2019 at 12:59:16PM +0000, Russell King - ARM Linux admin wrote:
>> On Fri, Mar 08, 2019 at 01:10:57PM +0900, Takashi Sakamoto wrote:
> 
>>> In expectation of ALSA PCM interface for hardware for usual device:
>>>  - half number of phases of SCK per phase of WC
>>>    = physical_width of sample
>>>    = bytes per sample
> 
>> They are not the same thing.
> 
>> Let's take SNDRV_PCM_FORMAT_S16_LE.  The in-memory layout of this format
>> is two 16-bit samples next to each other in a single 32-bit word.  Their
>> width is 16, their physical_width is 16, and bytes per sample is 2.
> 
>> A CPU DAI can do one of two things:
> 
>> 1) it can generate exactly 16 SCK clock cycles per sample before WS
>>    changes state, leading to a total of 32 SCK clock cycles per
>>    frame.
> 
>> 2) it can generate more than 16 SCK clock cycles per sample.
> 
>> Both are entirely legal and permissable under the I2S specification.
>> Both look the same in memory.
> 
>> The ALSA format specification (SNDRV_PCM_FORMAT_*) does not specify
>> which of these two is used on the wire - it only specifies the in-
>> memory format.
> 
> Everything Russell is saying here is correct.  The actual
> ABI only affects the in memory format, userspace really shouldn't care
> what's going on on the wire.  However we don't have separate
> infrastructure for what goes on the wire and 90% of the time you can
> just translate the memory layout into a wire layout which works so we're
> conflating the two in a lot of places internally which is confusing and
> fragile.

I agree. We just need a library which will:

1) gather the information from hardware drivers
  - a simple description of the bclk constrains
2) create right constraints (hw_params rules) for the ALSA PCM API
3) return the selected bclk when hw_params are installed

The description of the bclk constraints from the hardware driver might
be min/max or a list of allowed wire format bit width * channels,
eventually define the wire formats (bitmask) and use them in this
library. I can imagine that all of those bclk contraints descriptions
(or any future, if there are such requirement) can be implemented in
this library.

The library should not extend hw_params (new interval), but it should
work as a separate layer - use new structures / functions etc.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [RFC PATCH 1/4] alsa: make hw_params negotiation infrastructure 'bclk_ratio aware'

[Sven Van Asbroeck]

On Fri, Mar 8, 2019 at 2:54 PM Jaroslav Kysela <perex@perex.cz> wrote:
>
> I agree. We just need a library which will:
>
> 1) gather the information from hardware drivers
>   - a simple description of the bclk constrains
> 2) create right constraints (hw_params rules) for the ALSA PCM API
> 3) return the selected bclk when hw_params are installed
>

Yes, that's what the RFC patch I posted attempts to do.
But it extends hw_params, which is clearly wrong.

>
> The library should not extend hw_params (new interval), but it should
> work as a separate layer - use new structures / functions etc.
>

This, I could not work out how to approach :)

Re: [RFC PATCH 1/4] alsa: make hw_params negotiation infrastructure 'bclk_ratio aware'

[Pierre-Louis Bossart]

>> I agree. We just need a library which will:
>>
>> 1) gather the information from hardware drivers
>>    - a simple description of the bclk constrains
>> 2) create right constraints (hw_params rules) for the ALSA PCM API
>> 3) return the selected bclk when hw_params are installed
>>
> Yes, that's what the RFC patch I posted attempts to do.
> But it extends hw_params, which is clearly wrong.
>
>> The library should not extend hw_params (new interval), but it should
>> work as a separate layer - use new structures / functions etc.
>>
> This, I could not work out how to approach :)

I am not sure I fully understand the ask but wanted to point out that 
for ASoC topology-based solutions the bclk rate is typically passed as a 
parameter from userspace (w/ a request_firmware and topology parsing) 
and might be forwarded over IPC to a DSP. On some Intel platforms which 
can't support 32x fs that is typically how we represent a bclk ratio 
multiple of 25. the kernel has no idea of the relationship between the 
representation of the stream in memory and the final bit clock, only the 
DSP which programs the hardware registers knows about the latter.

It's really quite typical that the DAI is programmed for a fixed 
configuration and the DSP takes care of the conversions. The kernel only 
deals with stream triggers and power management without know all the 
internal details of the audio graph.

Re: [RFC PATCH 1/4] alsa: make hw_params negotiation infrastructure 'bclk_ratio aware'

[Mark Brown]

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

On Fri, Mar 08, 2019 at 02:49:48PM -0600, Pierre-Louis Bossart wrote:

> I am not sure I fully understand the ask but wanted to point out that for
> ASoC topology-based solutions the bclk rate is typically passed as a
> parameter from userspace (w/ a request_firmware and topology parsing) and

You mean for x86 systems :)  Well, big DSP really.  It's not really
topology related.

> might be forwarded over IPC to a DSP. On some Intel platforms which can't
> support 32x fs that is typically how we represent a bclk ratio multiple of
> 25. the kernel has no idea of the relationship between the representation of
> the stream in memory and the final bit clock, only the DSP which programs
> the hardware registers knows about the latter.

> It's really quite typical that the DAI is programmed for a fixed
> configuration and the DSP takes care of the conversions. The kernel only
> deals with stream triggers and power management without know all the
> internal details of the audio graph.

I think this is more the issue with not having transitioned fully to
components which we've talked about before I think - it's related but
not quite the same thing.  In the big DSP case there's really two audio
links that aren't really connected but we're currently trying to treat
them as one since the code was designed for much smaller DSPs.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

Re: [RFC PATCH 1/4] alsa: make hw_params negotiation infrastructure 'bclk_ratio aware'

[Pierre-Louis Bossart]

On 3/8/19 3:13 PM, Mark Brown wrote:
> On Fri, Mar 08, 2019 at 02:49:48PM -0600, Pierre-Louis Bossart wrote:
>
>> I am not sure I fully understand the ask but wanted to point out that for
>> ASoC topology-based solutions the bclk rate is typically passed as a
>> parameter from userspace (w/ a request_firmware and topology parsing) and
> You mean for x86 systems :)  Well, big DSP really.  It's not really
> topology related.

I was referring to asoc.h and the following structure. For once it's not 
an Intel-specific hack, though the topology does need a lot of love to 
be hardened and extended.

struct snd_soc_tplg_hw_config {
     __le32 size;            /* in bytes of this structure */
     __le32 id;        /* unique ID - - used to match */
     __le32 fmt;        /* SND_SOC_DAI_FORMAT_ format value */
     __u8 clock_gated;    /* SND_SOC_TPLG_DAI_CLK_GATE_ value */
     __u8 invert_bclk;    /* 1 for inverted BCLK, 0 for normal */
     __u8 invert_fsync;    /* 1 for inverted frame clock, 0 for normal */
     __u8 bclk_master;    /* SND_SOC_TPLG_BCLK_ value */
     __u8 fsync_master;    /* SND_SOC_TPLG_FSYNC_ value */
     __u8 mclk_direction;    /* SND_SOC_TPLG_MCLK_ value */
     __le16 reserved;    /* for 32bit alignment */
     __le32 mclk_rate;    /* MCLK or SYSCLK freqency in Hz */
     __le32 bclk_rate;    /* BCLK frequency in Hz */
     __le32 fsync_rate;    /* frame clock in Hz */
     __le32 tdm_slots;    /* number of TDM slots in use */
     __le32 tdm_slot_width;    /* width in bits for each slot */
     __le32 tx_slots;    /* bit mask for active Tx slots */
     __le32 rx_slots;    /* bit mask for active Rx slots */
     __le32 tx_channels;    /* number of Tx channels */
     __le32 tx_chanmap[SND_SOC_TPLG_MAX_CHAN]; /* array of slot number */
     __le32 rx_channels;    /* number of Rx channels */
     __le32 rx_chanmap[SND_SOC_TPLG_MAX_CHAN]; /* array of slot number */
} __attribute__((packed));


>
>> might be forwarded over IPC to a DSP. On some Intel platforms which can't
>> support 32x fs that is typically how we represent a bclk ratio multiple of
>> 25. the kernel has no idea of the relationship between the representation of
>> the stream in memory and the final bit clock, only the DSP which programs
>> the hardware registers knows about the latter.
>> It's really quite typical that the DAI is programmed for a fixed
>> configuration and the DSP takes care of the conversions. The kernel only
>> deals with stream triggers and power management without know all the
>> internal details of the audio graph.
> I think this is more the issue with not having transitioned fully to
> components which we've talked about before I think - it's related but
> not quite the same thing.  In the big DSP case there's really two audio
> links that aren't really connected but we're currently trying to treat
> them as one since the code was designed for much smaller DSPs.

Yes, this notion of hw_params negotiation made me think about the 
constraints propagation that Lars talked about ~2 years ago, it's not as 
simple as a helper library I am afraid.

This discussion on bclk ratio makes a lot of sense. Some codecs have 
undocumented restrictions, e.g PCM512x or some Maxim amps, and it's not 
uncommon to come up with a configuration mismatch that take time to 
debug. If at least we could have an error thrown it'd be a good thing.

Re: [RFC PATCH 1/4] alsa: make hw_params negotiation infrastructure 'bclk_ratio aware'

[Takashi Sakamoto]

Hi all,

On Fri, Mar 08, 2019 at 08:54:03PM +0100, Jaroslav Kysela wrote:
> Dne 08. 03. 19 v 18:22 Mark Brown napsal(a):
> > On Fri, Mar 08, 2019 at 12:59:16PM +0000, Russell King - ARM Linux admin wrote:
> >> On Fri, Mar 08, 2019 at 01:10:57PM +0900, Takashi Sakamoto wrote:
> > 
> >>> In expectation of ALSA PCM interface for hardware for usual device:
> >>>  - half number of phases of SCK per phase of WC
> >>>    = physical_width of sample
> >>>    = bytes per sample
> > 
> >> They are not the same thing.
> > 
> >> Let's take SNDRV_PCM_FORMAT_S16_LE.  The in-memory layout of this format
> >> is two 16-bit samples next to each other in a single 32-bit word.  Their
> >> width is 16, their physical_width is 16, and bytes per sample is 2.
> > 
> >> A CPU DAI can do one of two things:
> > 
> >> 1) it can generate exactly 16 SCK clock cycles per sample before WS
> >>    changes state, leading to a total of 32 SCK clock cycles per
> >>    frame.
> > 
> >> 2) it can generate more than 16 SCK clock cycles per sample.
> > 
> >> Both are entirely legal and permissable under the I2S specification.
> >> Both look the same in memory.
> > 
> >> The ALSA format specification (SNDRV_PCM_FORMAT_*) does not specify
> >> which of these two is used on the wire - it only specifies the in-
> >> memory format.
> > 
> > Everything Russell is saying here is correct.  The actual
> > ABI only affects the in memory format, userspace really shouldn't care
> > what's going on on the wire.  However we don't have separate
> > infrastructure for what goes on the wire and 90% of the time you can
> > just translate the memory layout into a wire layout which works so we're
> > conflating the two in a lot of places internally which is confusing and
> > fragile.
> 
> I agree. We just need a library which will:
> 
> 1) gather the information from hardware drivers
>   - a simple description of the bclk constrains
> 2) create right constraints (hw_params rules) for the ALSA PCM API
> 3) return the selected bclk when hw_params are installed
> 
> The description of the bclk constraints from the hardware driver might
> be min/max or a list of allowed wire format bit width * channels,
> eventually define the wire formats (bitmask) and use them in this
> library. I can imagine that all of those bclk contraints descriptions
> (or any future, if there are such requirement) can be implemented in
> this library.
> 
> The library should not extend hw_params (new interval), but it should
> work as a separate layer - use new structures / functions etc.

As another option I can suggest; usage of 'subformat' field of 'struct
snd_pcm_hw_params'.

At present, ALSA PCM interface has one entry to this field:
 - SNDRV_PCM_SUBFORMAT_STD

And no drivers use this entry.

Let's assume to add new entries to represent bclk/ws for the issued case:
 - SNDRV_PCM_SUBFORMAT_I2S_16BCLK_PER_WS
 - SNDRV_PCM_SUBFORMAT_I2S_48BCLK_PER_WS
 - SNDRV_PCM_SUBFORMAT_I2S_64BCLK_PER_WS
 - SNDRV_PCM_SUBFORMAT_I2S_128BCLK_PER_WS

Drivers can have constraints and rules for the parameter. At least, ALSA
PCM core should have an rule (if I understand correctly):
 - BCLK count of these subformats > physical_width * channel

One week point is it's not 'interval' type of parameter but 'mask'
parameter, thus we can't represent min/max/integer and so on. However,
less changes for ALSA PCM interface.

This is just an idea so it's not enough for me to consider about
relevant implementation. Any indication is welcome.


Regards

Takashi Sakamoto

Re: [RFC PATCH 1/4] alsa: make hw_params negotiation infrastructure 'bclk_ratio aware'

[Jaroslav Kysela]

Dne 11. 03. 19 v 9:15 Takashi Sakamoto napsal(a):
> Hi all,
> 
> On Fri, Mar 08, 2019 at 08:54:03PM +0100, Jaroslav Kysela wrote:
>> Dne 08. 03. 19 v 18:22 Mark Brown napsal(a):
>>> On Fri, Mar 08, 2019 at 12:59:16PM +0000, Russell King - ARM Linux admin wrote:
>>>> On Fri, Mar 08, 2019 at 01:10:57PM +0900, Takashi Sakamoto wrote:
>>>
>>>>> In expectation of ALSA PCM interface for hardware for usual device:
>>>>>  - half number of phases of SCK per phase of WC
>>>>>    = physical_width of sample
>>>>>    = bytes per sample
>>>
>>>> They are not the same thing.
>>>
>>>> Let's take SNDRV_PCM_FORMAT_S16_LE.  The in-memory layout of this format
>>>> is two 16-bit samples next to each other in a single 32-bit word.  Their
>>>> width is 16, their physical_width is 16, and bytes per sample is 2.
>>>
>>>> A CPU DAI can do one of two things:
>>>
>>>> 1) it can generate exactly 16 SCK clock cycles per sample before WS
>>>>    changes state, leading to a total of 32 SCK clock cycles per
>>>>    frame.
>>>
>>>> 2) it can generate more than 16 SCK clock cycles per sample.
>>>
>>>> Both are entirely legal and permissable under the I2S specification.
>>>> Both look the same in memory.
>>>
>>>> The ALSA format specification (SNDRV_PCM_FORMAT_*) does not specify
>>>> which of these two is used on the wire - it only specifies the in-
>>>> memory format.
>>>
>>> Everything Russell is saying here is correct.  The actual
>>> ABI only affects the in memory format, userspace really shouldn't care
>>> what's going on on the wire.  However we don't have separate
>>> infrastructure for what goes on the wire and 90% of the time you can
>>> just translate the memory layout into a wire layout which works so we're
>>> conflating the two in a lot of places internally which is confusing and
>>> fragile.
>>
>> I agree. We just need a library which will:
>>
>> 1) gather the information from hardware drivers
>>   - a simple description of the bclk constrains
>> 2) create right constraints (hw_params rules) for the ALSA PCM API
>> 3) return the selected bclk when hw_params are installed
>>
>> The description of the bclk constraints from the hardware driver might
>> be min/max or a list of allowed wire format bit width * channels,
>> eventually define the wire formats (bitmask) and use them in this
>> library. I can imagine that all of those bclk contraints descriptions
>> (or any future, if there are such requirement) can be implemented in
>> this library.
>>
>> The library should not extend hw_params (new interval), but it should
>> work as a separate layer - use new structures / functions etc.
> 
> As another option I can suggest; usage of 'subformat' field of 'struct
> snd_pcm_hw_params'.
> 
> At present, ALSA PCM interface has one entry to this field:
>  - SNDRV_PCM_SUBFORMAT_STD
> 
> And no drivers use this entry.
> 
> Let's assume to add new entries to represent bclk/ws for the issued case:
>  - SNDRV_PCM_SUBFORMAT_I2S_16BCLK_PER_WS
>  - SNDRV_PCM_SUBFORMAT_I2S_48BCLK_PER_WS
>  - SNDRV_PCM_SUBFORMAT_I2S_64BCLK_PER_WS
>  - SNDRV_PCM_SUBFORMAT_I2S_128BCLK_PER_WS
> 
> Drivers can have constraints and rules for the parameter. At least, ALSA
> PCM core should have an rule (if I understand correctly):
>  - BCLK count of these subformats > physical_width * channel
> 
> One week point is it's not 'interval' type of parameter but 'mask'
> parameter, thus we can't represent min/max/integer and so on. However,
> less changes for ALSA PCM interface.
> 
> This is just an idea so it's not enough for me to consider about
> relevant implementation. Any indication is welcome.

I would not use any of the "user space" ioctl API to represent the
hardware bclk requirements. The applications should know just the DMA
memory layout.

Also, think about the multiple simultaneous paths for the audio output
in the sound controller (so one DMA from the user space to the
controller, but the controller can do multiple simultaneous outputs
using different clocks combining different wire buses or so). Yes, it's
the corner case, but it's another reason to have the bclk code totally
separated from the user space ALSA's PCM API.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [RFC PATCH 1/4] alsa: make hw_params negotiation infrastructure 'bclk_ratio aware'

[Mark Brown]

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

On Mon, Mar 11, 2019 at 04:43:39PM +0100, Jaroslav Kysela wrote:

> I would not use any of the "user space" ioctl API to represent the
> hardware bclk requirements. The applications should know just the DMA
> memory layout.

> Also, think about the multiple simultaneous paths for the audio output
> in the sound controller (so one DMA from the user space to the
> controller, but the controller can do multiple simultaneous outputs
> using different clocks combining different wire buses or so). Yes, it's
> the corner case, but it's another reason to have the bclk code totally
> separated from the user space ALSA's PCM API.

There's also a range of devices that either don't have visible buses at
all due to integration or which are on buses that look nothing like the
I2S/DSP mode style of bus, rendering the parameters meaningless.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

Re: [RFC PATCH 1/4] alsa: make hw_params negotiation infrastructure 'bclk_ratio aware'

[Takashi Sakamoto]

Hi,

On Tue, Mar 12, 2019 at 03:03:00PM +0000, Mark Brown wrote:
> On Mon, Mar 11, 2019 at 04:43:39PM +0100, Jaroslav Kysela wrote:
> 
> > I would not use any of the "user space" ioctl API to represent the
> > hardware bclk requirements. The applications should know just the DMA
> > memory layout.
> 
> > Also, think about the multiple simultaneous paths for the audio output
> > in the sound controller (so one DMA from the user space to the
> > controller, but the controller can do multiple simultaneous outputs
> > using different clocks combining different wire buses or so). Yes, it's
> > the corner case, but it's another reason to have the bclk code totally
> > separated from the user space ALSA's PCM API.
> 
> There's also a range of devices that either don't have visible buses at
> all due to integration or which are on buses that look nothing like the
> I2S/DSP mode style of bus, rendering the parameters meaningless.

Indeed. That's resonable to add no changes to ALSA PCM interface. When
suggesting usage of 'rate_num' and 'rate_den', I assumed to change
ALSA SoC part internal to have constraints and rules for them, with no
changes of ALSA PCM inteface itself. I agree that the dicision of
on-wire format should not be exposed to userspace as well.


[1] https://mailman.alsa-project.org/pipermail/alsa-devel/2019-March/146261.html

Thanks

Takashi Sakamoto