[PATCH 0/3] ALSA - hda: Add support for link audio time reporting

[PATCH 0/3] ALSA - hda: Add support for link audio time reporting

[Vinod Koul]

Skylake onwards we support GTS capability and thus can report audio link
time. This series adds support for parsing the capability and then reports
link time.

While at it, fix the timestamping Documentation.

Guneshwor Singh (2):
  ALSA - hda: Add support for GTS capability
  ALSA - hda: Add support for link audio time reporting

Vinod Koul (1):
  ALSA - hda: Fix timestamping documentation

 Documentation/sound/alsa/timestamping.txt |  12 +-
 include/sound/hda_register.h              |  23 ++++
 sound/pci/hda/hda_controller.c            | 192 +++++++++++++++++++++++++++++-
 sound/pci/hda/hda_controller.h            |  27 +++++
 sound/pci/hda/hda_intel.c                 |   1 +
 5 files changed, 248 insertions(+), 7 deletions(-)

-- 
1.9.1

[PATCH 1/3] ALSA - hda: Add support for GTS capability

[Vinod Koul]

From: Guneshwor Singh <guneshwor.o.singh@intel.com>

Skylake onwards HDA controller supports Global Time Stamping
(GTS) capability. So add support to parse this capability in HDA
driver.

Signed-off-by: Guneshwor Singh <guneshwor.o.singh@intel.com>
Signed-off-by: Hardik T Shah <hardik.t.shah@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 include/sound/hda_register.h   | 23 +++++++++++++++++++++++
 sound/pci/hda/hda_controller.c | 33 +++++++++++++++++++++++++++++++++
 sound/pci/hda/hda_controller.h | 27 +++++++++++++++++++++++++++
 sound/pci/hda/hda_intel.c      |  1 +
 4 files changed, 84 insertions(+)

diff --git a/include/sound/hda_register.h b/include/sound/hda_register.h
index ff1aecf325e8..e4178328e8c8 100644
--- a/include/sound/hda_register.h
+++ b/include/sound/hda_register.h
@@ -242,6 +242,29 @@ enum { SDI0, SDI1, SDI2, SDI3, SDO0, SDO1, SDO2, SDO3 };
 /* Interval used to calculate the iterating register offset */
 #define AZX_DRSM_INTERVAL		0x08
 
+/* Global time synchronization registers */
+#define GTSCC_TSCCD_MASK		0x80000000
+#define GTSCC_TSCCD_SHIFT		31
+#define GTSCC_TSCCI_MASK		0x20
+#define GTSCC_CDMAS_DMA_DIR_SHIFT	4
+
+#define WALFCC_CIF_MASK			0x1FF
+#define WALFCC_FN_SHIFT			9
+#define HDA_CLK_CYCLES_PER_FRAME	512
+
+/*
+ * An error occurs near frame "rollover". The clocks in frame value indicates
+ * whether this error may have occurred. Here we use the value of 10. Please
+ * see the errata for the right number [<10]
+ */
+#define HDA_MAX_CYCLE_VALUE		499
+#define HDA_MAX_CYCLE_OFFSET		10
+#define HDA_MAX_CYCLE_READ_RETRY	10
+
+#define TSCCU_CCU_SHIFT			32
+#define LLPC_CCU_SHIFT			32
+
+
 /*
  * helpers to read the stream position
  */
diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
index 27de8015717d..9833666c6108 100644
--- a/sound/pci/hda/hda_controller.c
+++ b/sound/pci/hda/hda_controller.c
@@ -393,6 +393,34 @@ static struct snd_pcm_hardware azx_pcm_hw = {
 	.fifo_size =		0,
 };
 
+void azx_parse_capabilities(struct azx *chip)
+{
+	unsigned int cur_cap;
+	unsigned int offset;
+
+	offset = azx_readl(chip, LLCH);
+
+	/* Lets walk the linked capabilities list */
+	do {
+		cur_cap = _snd_hdac_chip_read(l, azx_bus(chip), offset);
+
+		switch ((cur_cap & CAP_HDR_ID_MASK) >> CAP_HDR_ID_OFF) {
+		case GTS_CAP_ID:
+			dev_dbg(chip->card->dev, "Found GTS capability");
+			chip->gts_present = 1;
+			break;
+
+		default:
+			break;
+		}
+
+		/* read the offset of next capabiity */
+		offset = cur_cap & CAP_HDR_NXT_PTR_MASK;
+	} while (offset);
+
+}
+EXPORT_SYMBOL_GPL(azx_parse_capabilities);
+
 static int azx_pcm_open(struct snd_pcm_substream *substream)
 {
 	struct azx_pcm *apcm = snd_pcm_substream_chip(substream);
@@ -412,6 +440,11 @@ static int azx_pcm_open(struct snd_pcm_substream *substream)
 		goto unlock;
 	}
 	runtime->private_data = azx_dev;
+
+	/* CPU must have ART for reporting link synchronized time */
+	if (chip->gts_present && boot_cpu_has(X86_FEATURE_ART))
+		azx_pcm_hw.info = azx_pcm_hw.info |
+			SNDRV_PCM_INFO_HAS_LINK_SYNCHRONIZED_ATIME;
 	runtime->hw = azx_pcm_hw;
 	runtime->hw.channels_min = hinfo->channels_min;
 	runtime->hw.channels_max = hinfo->channels_max;
diff --git a/sound/pci/hda/hda_controller.h b/sound/pci/hda/hda_controller.h
index ec63bbf1ec6d..fc57eef9fd88 100644
--- a/sound/pci/hda/hda_controller.h
+++ b/sound/pci/hda/hda_controller.h
@@ -159,9 +159,14 @@ struct azx {
 	unsigned int region_requested:1;
 	unsigned int disabled:1; /* disabled by vga_switcheroo */
 
+	/* GTS present */
+	unsigned int gts_present:1;
+
 #ifdef CONFIG_SND_HDA_DSP_LOADER
 	struct azx_dev saved_azx_dev;
 #endif
+	unsigned int link_count;
+	unsigned int mlcap_offset;
 };
 
 #define azx_bus(chip)	(&(chip)->bus.core)
@@ -173,6 +178,27 @@ struct azx {
 #define azx_snoop(chip)		true
 #endif
 
+#define AZX_REG_LLCH 0x14
+
+#define AZX_REG_GTS_BASE 0x520
+
+#define AZX_REG_GTSCC	(AZX_REG_GTS_BASE + 0x00)
+#define AZX_REG_WALFCC	(AZX_REG_GTS_BASE + 0x04)
+#define AZX_REG_TSCCL	(AZX_REG_GTS_BASE + 0x08)
+#define AZX_REG_TSCCU	(AZX_REG_GTS_BASE + 0x0C)
+#define AZX_REG_LLPFOC	(AZX_REG_GTS_BASE + 0x14)
+#define AZX_REG_LLPCL	(AZX_REG_GTS_BASE + 0x18)
+#define AZX_REG_LLPCU	(AZX_REG_GTS_BASE + 0x1C)
+
+#define CAP_HDR_VER_OFF                 28
+#define CAP_HDR_VER_MASK                (0xF << CAP_HDR_VER_OFF)
+
+#define CAP_HDR_ID_OFF                  16
+#define CAP_HDR_ID_MASK                 (0xFFF << CAP_HDR_ID_OFF)
+
+#define GTS_CAP_ID                 0x1
+#define CAP_HDR_NXT_PTR_MASK    0xFFFF
+
 /*
  * macros for easy use
  */
@@ -201,6 +227,7 @@ static inline struct azx_dev *get_azx_dev(struct snd_pcm_substream *substream)
 unsigned int azx_get_position(struct azx *chip, struct azx_dev *azx_dev);
 unsigned int azx_get_pos_lpib(struct azx *chip, struct azx_dev *azx_dev);
 unsigned int azx_get_pos_posbuf(struct azx *chip, struct azx_dev *azx_dev);
+void azx_parse_capabilities(struct azx *chip);
 
 /* Stream control. */
 void azx_stop_all_streams(struct azx *chip);
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 9a0d1445ca5c..b4ebdde59398 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1649,6 +1649,7 @@ static int azx_first_init(struct azx *chip)
 		return -ENXIO;
 	}
 
+	azx_parse_capabilities(chip);
 	if (chip->msi) {
 		if (chip->driver_caps & AZX_DCAPS_NO_MSI64) {
 			dev_dbg(card->dev, "Disabling 64bit MSI\n");
-- 
1.9.1

[PATCH 2/3] ALSA - hda: Add support for link audio time reporting

[Vinod Koul]

From: Guneshwor Singh <guneshwor.o.singh@intel.com>

Skylake onwards HDA controller supports reprting link audio
time, so add support for that.

Signed-off-by: Guneshwor Singh <guneshwor.o.singh@intel.com>
Signed-off-by: Hardik T Shah <hardik.t.shah@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/pci/hda/hda_controller.c | 159 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 158 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
index 9833666c6108..5613a403d720 100644
--- a/sound/pci/hda/hda_controller.c
+++ b/sound/pci/hda/hda_controller.c
@@ -27,6 +27,7 @@
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
+#include <asm/tsc.h>
 #include <sound/core.h>
 #include <sound/initval.h>
 #include "hda_controller.h"
@@ -34,6 +35,8 @@
 #define CREATE_TRACE_POINTS
 #include "hda_controller_trace.h"
 
+#define SEC_TO_NSEC	1000000000LL
+
 /* DSP lock helpers */
 #define dsp_lock(dev)		snd_hdac_dsp_lock(azx_stream(dev))
 #define dsp_unlock(dev)		snd_hdac_dsp_unlock(azx_stream(dev))
@@ -337,12 +340,136 @@ static snd_pcm_uframes_t azx_pcm_pointer(struct snd_pcm_substream *substream)
 			       azx_get_position(chip, azx_dev));
 }
 
+static u64 azx_scale64(u64 base, u32 num, u32 den)
+{
+	u64 rem;
+
+	rem = do_div(base, den);
+
+	base *= num;
+	rem *= num;
+
+	do_div(rem, den);
+
+	return base + rem;
+}
+
+static int azx_get_sync_time(ktime_t *device,
+		struct system_counterval_t *system, void *ctx)
+{
+	struct snd_pcm_substream *substream = (struct snd_pcm_substream *)ctx;
+	struct azx_dev *azx_dev = get_azx_dev(substream);
+	struct azx_pcm *apcm = snd_pcm_substream_chip(substream);
+	struct azx *chip = apcm->chip;
+	struct snd_pcm_runtime *runtime;
+	u64 ll_counter, ll_counter_l, ll_counter_h;
+	u64 tsc_counter, tsc_counter_l, tsc_counter_h;
+	u32 wallclk_ctr, wallclk_cycles;
+	bool direction;
+	u32 dma_select;
+	u32 timeout = 200;
+	u32 retry_count = 0;
+
+	runtime = substream->runtime;
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		direction = 1;
+	else
+		direction = 0;
+
+	/* 0th stream tag is not used, so DMA ch 0 is for 1st stream tag */
+	do {
+		timeout = 100;
+		dma_select = (direction << GTSCC_CDMAS_DMA_DIR_SHIFT) |
+					(azx_dev->core.stream_tag - 1);
+		_snd_hdac_chip_write(l, azx_bus(chip), AZX_REG_GTSCC,
+						dma_select);
+		/* Enable the capture */
+		_snd_hdac_chip_write(l, azx_bus(chip), AZX_REG_GTSCC,
+				     _snd_hdac_chip_read(l, azx_bus(chip),
+					AZX_REG_GTSCC) | GTSCC_TSCCI_MASK);
+
+		while (timeout) {
+			if (_snd_hdac_chip_read(l, azx_bus(chip),
+				AZX_REG_GTSCC) & GTSCC_TSCCD_MASK)
+				break;
+			timeout--;
+		}
+
+		if (!timeout) {
+			dev_err(chip->card->dev, "GTSCC capture Timedout!\n");
+			return -EIO;
+		}
+
+		/* Read wall clock counter */
+		wallclk_ctr = _snd_hdac_chip_read(l, azx_bus(chip),
+						  AZX_REG_WALFCC);
+
+		/* Read TSC counter */
+		tsc_counter_l = _snd_hdac_chip_read(l, azx_bus(chip),
+						    AZX_REG_TSCCL);
+		tsc_counter_h = _snd_hdac_chip_read(l, azx_bus(chip),
+						    AZX_REG_TSCCU);
+
+		/* Read Link counter */
+		ll_counter_l = _snd_hdac_chip_read(l, azx_bus(chip),
+						   AZX_REG_LLPCL);
+		ll_counter_h = _snd_hdac_chip_read(l, azx_bus(chip),
+						   AZX_REG_LLPCU);
+
+		/* Ack: registers read done */
+		_snd_hdac_chip_write(l, azx_bus(chip),
+				     AZX_REG_GTSCC,
+				     (0x1 << GTSCC_TSCCD_SHIFT));
+
+		tsc_counter = (tsc_counter_h << TSCCU_CCU_SHIFT) |
+						tsc_counter_l;
+
+		ll_counter = (ll_counter_h << LLPC_CCU_SHIFT) |	ll_counter_l;
+		wallclk_cycles = wallclk_ctr & WALFCC_CIF_MASK;
+
+		if (wallclk_cycles < HDA_MAX_CYCLE_VALUE - HDA_MAX_CYCLE_OFFSET
+		    && wallclk_cycles > HDA_MAX_CYCLE_OFFSET)
+			break;
+
+		/*
+		 * Sleep before we read again, else we may again get
+		 * value near to MAX_CYCLE. Try to sleep for different
+		 * amount of time so we dont hit the same number again
+		 */
+		udelay(retry_count++);
+	} while (retry_count != HDA_MAX_CYCLE_READ_RETRY);
+
+	if (retry_count == HDA_MAX_CYCLE_READ_RETRY) {
+		dev_err(chip->card->dev, "Error in WALFCC cycle count\n");
+		return -EIO;
+	}
+
+	*device = ns_to_ktime(azx_scale64(ll_counter,
+				SEC_TO_NSEC, runtime->rate));
+	*device = ktime_add_ns(*device, (wallclk_cycles * SEC_TO_NSEC) /
+			       ((HDA_MAX_CYCLE_VALUE+1) * runtime->rate));
+
+	*system = convert_art_to_tsc(tsc_counter);
+
+	return 0;
+}
+
+static int azx_get_crosststamp(struct snd_pcm_substream *substream,
+			      struct system_device_crosststamp *xtstamp)
+{
+	return get_device_system_crosststamp(azx_get_sync_time,
+					substream, NULL, xtstamp);
+}
+
 static int azx_get_time_info(struct snd_pcm_substream *substream,
 			struct timespec *system_ts, struct timespec *audio_ts,
 			struct snd_pcm_audio_tstamp_config *audio_tstamp_config,
 			struct snd_pcm_audio_tstamp_report *audio_tstamp_report)
 {
 	struct azx_dev *azx_dev = get_azx_dev(substream);
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct system_device_crosststamp xtstamp;
 	u64 nsec;
 
 	if ((substream->runtime->hw.info & SNDRV_PCM_INFO_HAS_LINK_ATIME) &&
@@ -361,8 +488,38 @@ static int azx_get_time_info(struct snd_pcm_substream *substream,
 		audio_tstamp_report->accuracy_report = 1; /* rest of structure is valid */
 		audio_tstamp_report->accuracy = 42; /* 24 MHz WallClock == 42ns resolution */
 
-	} else
+	} else if ((runtime->hw.info &
+			SNDRV_PCM_INFO_HAS_LINK_SYNCHRONIZED_ATIME) &&
+			(audio_tstamp_config->type_requested ==
+			SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK_SYNCHRONIZED)) {
+
+		azx_get_crosststamp(substream, &xtstamp);
+
+		switch (runtime->tstamp_type) {
+		case SNDRV_PCM_TSTAMP_TYPE_MONOTONIC:
+			return -EINVAL;
+
+		case SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW:
+			*system_ts = ktime_to_timespec(xtstamp.sys_monoraw);
+			break;
+
+		default:
+			*system_ts = ktime_to_timespec(xtstamp.sys_realtime);
+			break;
+
+		}
+
+		*audio_ts = ktime_to_timespec(xtstamp.device);
+
+		audio_tstamp_report->actual_type =
+			SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK_SYNCHRONIZED;
+		audio_tstamp_report->accuracy_report = 1;
+		/* 24 MHz WallClock == 42ns resolution */
+		audio_tstamp_report->accuracy = 42;
+
+	} else {
 		audio_tstamp_report->actual_type = SNDRV_PCM_AUDIO_TSTAMP_TYPE_DEFAULT;
+	}
 
 	return 0;
 }
-- 
1.9.1

[PATCH 3/3] ALSA - hda: Fix timestamping documentation

[Vinod Koul]

Some typos in the documentation, so fix them up.

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 Documentation/sound/alsa/timestamping.txt | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/sound/alsa/timestamping.txt b/Documentation/sound/alsa/timestamping.txt
index 1b6473f393a8..9d579aefbffd 100644
--- a/Documentation/sound/alsa/timestamping.txt
+++ b/Documentation/sound/alsa/timestamping.txt
@@ -14,7 +14,7 @@ provides a refined estimate with a delay.
 event or application query.
 The difference (tstamp - trigger_tstamp) defines the elapsed time.
 
-The ALSA API provides reports two basic pieces of information, avail
+The ALSA API provides two basic pieces of information, avail
 and delay, which combined with the trigger and current system
 timestamps allow for applications to keep track of the 'fullness' of
 the ring buffer and the amount of queued samples.
@@ -53,21 +53,21 @@ case):
 The analog time is taken at the last stage of the playback, as close
 as possible to the actual transducer
 
-The link time is taken at the output of the SOC/chipset as the samples
+The link time is taken at the output of the SoC/chipset as the samples
 are pushed on a link. The link time can be directly measured if
 supported in hardware by sample counters or wallclocks (e.g. with
 HDAudio 24MHz or PTP clock for networked solutions) or indirectly
 estimated (e.g. with the frame counter in USB).
 
 The DMA time is measured using counters - typically the least reliable
-of all measurements due to the bursty natured of DMA transfers.
+of all measurements due to the bursty nature of DMA transfers.
 
 The app time corresponds to the time tracked by an application after
 writing in the ring buffer.
 
-The application can query what the hardware supports, define which
+The application can query the hardware capabilities, define which
 audio time it wants reported by selecting the relevant settings in
-audio_tstamp_config fields, get an estimate of the timestamp
+audio_tstamp_config fields, thus get an estimate of the timestamp
 accuracy. It can also request the delay-to-analog be included in the
 measurement. Direct access to the link time is very interesting on
 platforms that provide an embedded DSP; measuring directly the link
@@ -169,7 +169,7 @@ playback: systime: 938107562 nsec, audio time 938112708 nsec, 	systime delta -51
 Example 1 shows that the timestamp at the DMA level is close to 1ms
 ahead of the actual playback time (as a side time this sort of
 measurement can help define rewind safeguards). Compensating for the
-DMA-link delay in example 2 helps remove the hardware buffering abut
+DMA-link delay in example 2 helps remove the hardware buffering but
 the information is still very jittery, with up to one sample of
 error. In example 3 where the timestamps are measured with the link
 wallclock, the timestamps show a monotonic behavior and a lower
-- 
1.9.1

Re: [PATCH 1/3] ALSA - hda: Add support for GTS capability

[Takashi Iwai]

On Mon, 11 Jul 2016 12:13:27 +0200,
Vinod Koul wrote:
> 
> From: Guneshwor Singh <guneshwor.o.singh@intel.com>
> 
> Skylake onwards HDA controller supports Global Time Stamping
> (GTS) capability. So add support to parse this capability in HDA
> driver.
> 
> Signed-off-by: Guneshwor Singh <guneshwor.o.singh@intel.com>
> Signed-off-by: Hardik T Shah <hardik.t.shah@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
>  include/sound/hda_register.h   | 23 +++++++++++++++++++++++
>  sound/pci/hda/hda_controller.c | 33 +++++++++++++++++++++++++++++++++
>  sound/pci/hda/hda_controller.h | 27 +++++++++++++++++++++++++++
>  sound/pci/hda/hda_intel.c      |  1 +
>  4 files changed, 84 insertions(+)
> 
> diff --git a/include/sound/hda_register.h b/include/sound/hda_register.h
> index ff1aecf325e8..e4178328e8c8 100644
> --- a/include/sound/hda_register.h
> +++ b/include/sound/hda_register.h
> @@ -242,6 +242,29 @@ enum { SDI0, SDI1, SDI2, SDI3, SDO0, SDO1, SDO2, SDO3 };
>  /* Interval used to calculate the iterating register offset */
>  #define AZX_DRSM_INTERVAL		0x08
>  
> +/* Global time synchronization registers */
> +#define GTSCC_TSCCD_MASK		0x80000000
> +#define GTSCC_TSCCD_SHIFT		31
> +#define GTSCC_TSCCI_MASK		0x20
> +#define GTSCC_CDMAS_DMA_DIR_SHIFT	4
> +
> +#define WALFCC_CIF_MASK			0x1FF
> +#define WALFCC_FN_SHIFT			9
> +#define HDA_CLK_CYCLES_PER_FRAME	512
> +
> +/*
> + * An error occurs near frame "rollover". The clocks in frame value indicates
> + * whether this error may have occurred. Here we use the value of 10. Please
> + * see the errata for the right number [<10]
> + */
> +#define HDA_MAX_CYCLE_VALUE		499
> +#define HDA_MAX_CYCLE_OFFSET		10
> +#define HDA_MAX_CYCLE_READ_RETRY	10
> +
> +#define TSCCU_CCU_SHIFT			32
> +#define LLPC_CCU_SHIFT			32
> +
> +
>  /*
>   * helpers to read the stream position
>   */
> diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
> index 27de8015717d..9833666c6108 100644
> --- a/sound/pci/hda/hda_controller.c
> +++ b/sound/pci/hda/hda_controller.c
> @@ -393,6 +393,34 @@ static struct snd_pcm_hardware azx_pcm_hw = {
>  	.fifo_size =		0,
>  };
>  
> +void azx_parse_capabilities(struct azx *chip)

Please give a bit more comments about the function.


> +{
> +	unsigned int cur_cap;
> +	unsigned int offset;
> +
> +	offset = azx_readl(chip, LLCH);
> +
> +	/* Lets walk the linked capabilities list */
> +	do {
> +		cur_cap = _snd_hdac_chip_read(l, azx_bus(chip), offset);
> +
> +		switch ((cur_cap & CAP_HDR_ID_MASK) >> CAP_HDR_ID_OFF) {
> +		case GTS_CAP_ID:
> +			dev_dbg(chip->card->dev, "Found GTS capability");
> +			chip->gts_present = 1;
> +			break;
> +
> +		default:
> +			break;
> +		}
> +
> +		/* read the offset of next capabiity */
> +		offset = cur_cap & CAP_HDR_NXT_PTR_MASK;
> +	} while (offset);

It's be safer to have a counter not to go over the endless loop.


> +
> +}
> +EXPORT_SYMBOL_GPL(azx_parse_capabilities);
> +
>  static int azx_pcm_open(struct snd_pcm_substream *substream)
>  {
>  	struct azx_pcm *apcm = snd_pcm_substream_chip(substream);
> @@ -412,6 +440,11 @@ static int azx_pcm_open(struct snd_pcm_substream *substream)
>  		goto unlock;
>  	}
>  	runtime->private_data = azx_dev;
> +
> +	/* CPU must have ART for reporting link synchronized time */
> +	if (chip->gts_present && boot_cpu_has(X86_FEATURE_ART))

This comes out of sudden.  It needs more explanation.


> +		azx_pcm_hw.info = azx_pcm_hw.info |
> +			SNDRV_PCM_INFO_HAS_LINK_SYNCHRONIZED_ATIME;
>  	runtime->hw = azx_pcm_hw;
>  	runtime->hw.channels_min = hinfo->channels_min;
>  	runtime->hw.channels_max = hinfo->channels_max;
> diff --git a/sound/pci/hda/hda_controller.h b/sound/pci/hda/hda_controller.h
> index ec63bbf1ec6d..fc57eef9fd88 100644
> --- a/sound/pci/hda/hda_controller.h
> +++ b/sound/pci/hda/hda_controller.h
> @@ -159,9 +159,14 @@ struct azx {
>  	unsigned int region_requested:1;
>  	unsigned int disabled:1; /* disabled by vga_switcheroo */
>  
> +	/* GTS present */
> +	unsigned int gts_present:1;
> +
>  #ifdef CONFIG_SND_HDA_DSP_LOADER
>  	struct azx_dev saved_azx_dev;
>  #endif
> +	unsigned int link_count;
> +	unsigned int mlcap_offset;
>  };
>  
>  #define azx_bus(chip)	(&(chip)->bus.core)
> @@ -173,6 +178,27 @@ struct azx {
>  #define azx_snoop(chip)		true
>  #endif
>  
> +#define AZX_REG_LLCH 0x14
> +
> +#define AZX_REG_GTS_BASE 0x520
> +
> +#define AZX_REG_GTSCC	(AZX_REG_GTS_BASE + 0x00)
> +#define AZX_REG_WALFCC	(AZX_REG_GTS_BASE + 0x04)
> +#define AZX_REG_TSCCL	(AZX_REG_GTS_BASE + 0x08)
> +#define AZX_REG_TSCCU	(AZX_REG_GTS_BASE + 0x0C)
> +#define AZX_REG_LLPFOC	(AZX_REG_GTS_BASE + 0x14)
> +#define AZX_REG_LLPCL	(AZX_REG_GTS_BASE + 0x18)
> +#define AZX_REG_LLPCU	(AZX_REG_GTS_BASE + 0x1C)
> +
> +#define CAP_HDR_VER_OFF                 28
> +#define CAP_HDR_VER_MASK                (0xF << CAP_HDR_VER_OFF)
> +
> +#define CAP_HDR_ID_OFF                  16
> +#define CAP_HDR_ID_MASK                 (0xFFF << CAP_HDR_ID_OFF)
> +
> +#define GTS_CAP_ID                 0x1
> +#define CAP_HDR_NXT_PTR_MASK    0xFFFF
> +
>  /*
>   * macros for easy use
>   */
> @@ -201,6 +227,7 @@ static inline struct azx_dev *get_azx_dev(struct snd_pcm_substream *substream)
>  unsigned int azx_get_position(struct azx *chip, struct azx_dev *azx_dev);
>  unsigned int azx_get_pos_lpib(struct azx *chip, struct azx_dev *azx_dev);
>  unsigned int azx_get_pos_posbuf(struct azx *chip, struct azx_dev *azx_dev);
> +void azx_parse_capabilities(struct azx *chip);
>  
>  /* Stream control. */
>  void azx_stop_all_streams(struct azx *chip);
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 9a0d1445ca5c..b4ebdde59398 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -1649,6 +1649,7 @@ static int azx_first_init(struct azx *chip)
>  		return -ENXIO;
>  	}
>  
> +	azx_parse_capabilities(chip);

Is it really safe to call the function no matter which chip?


thanks,

Takashi

Re: [PATCH 2/3] ALSA - hda: Add support for link audio time reporting

[Takashi Iwai]

On Mon, 11 Jul 2016 12:13:28 +0200,
Vinod Koul wrote:
> 
> From: Guneshwor Singh <guneshwor.o.singh@intel.com>
> 
> Skylake onwards HDA controller supports reprting link audio
> time, so add support for that.

It's way too few description, the text is almost same as the previous
patch.  Please give more information.

> 
> Signed-off-by: Guneshwor Singh <guneshwor.o.singh@intel.com>
> Signed-off-by: Hardik T Shah <hardik.t.shah@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
>  sound/pci/hda/hda_controller.c | 159 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 158 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
> index 9833666c6108..5613a403d720 100644
> --- a/sound/pci/hda/hda_controller.c
> +++ b/sound/pci/hda/hda_controller.c
> @@ -27,6 +27,7 @@
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/slab.h>
> +#include <asm/tsc.h>
>  #include <sound/core.h>
>  #include <sound/initval.h>
>  #include "hda_controller.h"
> @@ -34,6 +35,8 @@
>  #define CREATE_TRACE_POINTS
>  #include "hda_controller_trace.h"
>  
> +#define SEC_TO_NSEC	1000000000LL

Can we use a definition in time64.h?


>  /* DSP lock helpers */
>  #define dsp_lock(dev)		snd_hdac_dsp_lock(azx_stream(dev))
>  #define dsp_unlock(dev)		snd_hdac_dsp_unlock(azx_stream(dev))
> @@ -337,12 +340,136 @@ static snd_pcm_uframes_t azx_pcm_pointer(struct snd_pcm_substream *substream)
>  			       azx_get_position(chip, azx_dev));
>  }
>  
> +static u64 azx_scale64(u64 base, u32 num, u32 den)
> +{
> +	u64 rem;
> +
> +	rem = do_div(base, den);
> +
> +	base *= num;
> +	rem *= num;
> +
> +	do_div(rem, den);
> +
> +	return base + rem;
> +}

What is this function supposed to do?


> +static int azx_get_sync_time(ktime_t *device,
> +		struct system_counterval_t *system, void *ctx)
> +{
> +	struct snd_pcm_substream *substream = (struct snd_pcm_substream *)ctx;
> +	struct azx_dev *azx_dev = get_azx_dev(substream);
> +	struct azx_pcm *apcm = snd_pcm_substream_chip(substream);
> +	struct azx *chip = apcm->chip;
> +	struct snd_pcm_runtime *runtime;
> +	u64 ll_counter, ll_counter_l, ll_counter_h;
> +	u64 tsc_counter, tsc_counter_l, tsc_counter_h;
> +	u32 wallclk_ctr, wallclk_cycles;
> +	bool direction;
> +	u32 dma_select;
> +	u32 timeout = 200;
> +	u32 retry_count = 0;
> +
> +	runtime = substream->runtime;
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +		direction = 1;
> +	else
> +		direction = 0;
> +
> +	/* 0th stream tag is not used, so DMA ch 0 is for 1st stream tag */
> +	do {
> +		timeout = 100;
> +		dma_select = (direction << GTSCC_CDMAS_DMA_DIR_SHIFT) |
> +					(azx_dev->core.stream_tag - 1);
> +		_snd_hdac_chip_write(l, azx_bus(chip), AZX_REG_GTSCC,
> +						dma_select);
> +		/* Enable the capture */
> +		_snd_hdac_chip_write(l, azx_bus(chip), AZX_REG_GTSCC,
> +				     _snd_hdac_chip_read(l, azx_bus(chip),
> +					AZX_REG_GTSCC) | GTSCC_TSCCI_MASK);
> +
> +		while (timeout) {
> +			if (_snd_hdac_chip_read(l, azx_bus(chip),
> +				AZX_REG_GTSCC) & GTSCC_TSCCD_MASK)
> +				break;
> +			timeout--;
> +		}
> +
> +		if (!timeout) {
> +			dev_err(chip->card->dev, "GTSCC capture Timedout!\n");
> +			return -EIO;
> +		}
> +
> +		/* Read wall clock counter */
> +		wallclk_ctr = _snd_hdac_chip_read(l, azx_bus(chip),
> +						  AZX_REG_WALFCC);
> +
> +		/* Read TSC counter */
> +		tsc_counter_l = _snd_hdac_chip_read(l, azx_bus(chip),
> +						    AZX_REG_TSCCL);
> +		tsc_counter_h = _snd_hdac_chip_read(l, azx_bus(chip),
> +						    AZX_REG_TSCCU);
> +
> +		/* Read Link counter */
> +		ll_counter_l = _snd_hdac_chip_read(l, azx_bus(chip),
> +						   AZX_REG_LLPCL);
> +		ll_counter_h = _snd_hdac_chip_read(l, azx_bus(chip),
> +						   AZX_REG_LLPCU);
> +
> +		/* Ack: registers read done */
> +		_snd_hdac_chip_write(l, azx_bus(chip),
> +				     AZX_REG_GTSCC,
> +				     (0x1 << GTSCC_TSCCD_SHIFT));
> +
> +		tsc_counter = (tsc_counter_h << TSCCU_CCU_SHIFT) |
> +						tsc_counter_l;
> +
> +		ll_counter = (ll_counter_h << LLPC_CCU_SHIFT) |	ll_counter_l;
> +		wallclk_cycles = wallclk_ctr & WALFCC_CIF_MASK;
> +
> +		if (wallclk_cycles < HDA_MAX_CYCLE_VALUE - HDA_MAX_CYCLE_OFFSET
> +		    && wallclk_cycles > HDA_MAX_CYCLE_OFFSET)
> +			break;

Is this condition really correct...?  It's hard to understand.

> +
> +		/*
> +		 * Sleep before we read again, else we may again get
> +		 * value near to MAX_CYCLE. Try to sleep for different
> +		 * amount of time so we dont hit the same number again
> +		 */
> +		udelay(retry_count++);
> +	} while (retry_count != HDA_MAX_CYCLE_READ_RETRY);
> +
> +	if (retry_count == HDA_MAX_CYCLE_READ_RETRY) {
> +		dev_err(chip->card->dev, "Error in WALFCC cycle count\n");
> +		return -EIO;
> +	}
> +
> +	*device = ns_to_ktime(azx_scale64(ll_counter,
> +				SEC_TO_NSEC, runtime->rate));
> +	*device = ktime_add_ns(*device, (wallclk_cycles * SEC_TO_NSEC) /
> +			       ((HDA_MAX_CYCLE_VALUE+1) * runtime->rate));

Hmm, the calculation here looks as if there can be an optimization...


thanks,

Takashi


> +	*system = convert_art_to_tsc(tsc_counter);
> +
> +	return 0;
> +}
> +
> +static int azx_get_crosststamp(struct snd_pcm_substream *substream,
> +			      struct system_device_crosststamp *xtstamp)
> +{
> +	return get_device_system_crosststamp(azx_get_sync_time,
> +					substream, NULL, xtstamp);
> +}
> +
>  static int azx_get_time_info(struct snd_pcm_substream *substream,
>  			struct timespec *system_ts, struct timespec *audio_ts,
>  			struct snd_pcm_audio_tstamp_config *audio_tstamp_config,
>  			struct snd_pcm_audio_tstamp_report *audio_tstamp_report)
>  {
>  	struct azx_dev *azx_dev = get_azx_dev(substream);
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct system_device_crosststamp xtstamp;
>  	u64 nsec;
>  
>  	if ((substream->runtime->hw.info & SNDRV_PCM_INFO_HAS_LINK_ATIME) &&
> @@ -361,8 +488,38 @@ static int azx_get_time_info(struct snd_pcm_substream *substream,
>  		audio_tstamp_report->accuracy_report = 1; /* rest of structure is valid */
>  		audio_tstamp_report->accuracy = 42; /* 24 MHz WallClock == 42ns resolution */
>  
> -	} else
> +	} else if ((runtime->hw.info &
> +			SNDRV_PCM_INFO_HAS_LINK_SYNCHRONIZED_ATIME) &&
> +			(audio_tstamp_config->type_requested ==
> +			SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK_SYNCHRONIZED)) {
> +
> +		azx_get_crosststamp(substream, &xtstamp);
> +
> +		switch (runtime->tstamp_type) {
> +		case SNDRV_PCM_TSTAMP_TYPE_MONOTONIC:
> +			return -EINVAL;
> +
> +		case SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW:
> +			*system_ts = ktime_to_timespec(xtstamp.sys_monoraw);
> +			break;
> +
> +		default:
> +			*system_ts = ktime_to_timespec(xtstamp.sys_realtime);
> +			break;
> +
> +		}
> +
> +		*audio_ts = ktime_to_timespec(xtstamp.device);
> +
> +		audio_tstamp_report->actual_type =
> +			SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK_SYNCHRONIZED;
> +		audio_tstamp_report->accuracy_report = 1;
> +		/* 24 MHz WallClock == 42ns resolution */
> +		audio_tstamp_report->accuracy = 42;
> +
> +	} else {
>  		audio_tstamp_report->actual_type = SNDRV_PCM_AUDIO_TSTAMP_TYPE_DEFAULT;
> +	}
>  
>  	return 0;
>  }
> -- 
> 1.9.1
> 

Re: [PATCH 3/3] ALSA - hda: Fix timestamping documentation

[Takashi Iwai]

On Mon, 11 Jul 2016 12:13:29 +0200,
Vinod Koul wrote:
> 
> Some typos in the documentation, so fix them up.
> 
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>

Applied this one, thanks.


Takashi

Re: [PATCH 1/3] ALSA - hda: Add support for GTS capability

[Takashi Iwai]

On Mon, 11 Jul 2016 16:07:32 +0200,
Vinod Koul wrote:
> 
> > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > > index 9a0d1445ca5c..b4ebdde59398 100644
> > > --- a/sound/pci/hda/hda_intel.c
> > > +++ b/sound/pci/hda/hda_intel.c
> > > @@ -1649,6 +1649,7 @@ static int azx_first_init(struct azx *chip)
> > >  		return -ENXIO;
> > >  	}
> > >  
> > > +	azx_parse_capabilities(chip);
> > 
> > Is it really safe to call the function no matter which chip?
> 
> I think so, since we read base HDA registers for capablity. For older hw the
> next capablity will not be there.
> 
> But yes I will verify this on older machines.

I checked the spec 1.0 and 1.0a, and there is no definition for the
register offset 0x14.  So, it's likely unsafe to use it
unconditionally.


Takashi

Re: [PATCH 1/3] ALSA - hda: Add support for GTS capability

[Vinod Koul]

On Mon, Jul 11, 2016 at 12:22:15PM +0200, Takashi Iwai wrote:
> On Mon, 11 Jul 2016 12:13:27 +0200,
> Vinod Koul wrote:
> > 
> >  
> > +void azx_parse_capabilities(struct azx *chip)
> 
> Please give a bit more comments about the function.

Sure thing, will add.

> > +	/* Lets walk the linked capabilities list */
> > +	do {
> > +		cur_cap = _snd_hdac_chip_read(l, azx_bus(chip), offset);
> > +
> > +		switch ((cur_cap & CAP_HDR_ID_MASK) >> CAP_HDR_ID_OFF) {
> > +		case GTS_CAP_ID:
> > +			dev_dbg(chip->card->dev, "Found GTS capability");
> > +			chip->gts_present = 1;
> > +			break;
> > +
> > +		default:
> > +			break;
> > +		}
> > +
> > +		/* read the offset of next capabiity */
> > +		offset = cur_cap & CAP_HDR_NXT_PTR_MASK;
> > +	} while (offset);
> 
> It's be safer to have a counter not to go over the endless loop.

I do agree a broken hw can cause issues, so will add that as well.

> >  	runtime->private_data = azx_dev;
> > +
> > +	/* CPU must have ART for reporting link synchronized time */
> > +	if (chip->gts_present && boot_cpu_has(X86_FEATURE_ART))
> 
> This comes out of sudden.  It needs more explanation.

Ah, this was merged in last window. We cna check of cpu is capable of ART,
and in these cases only we cna report these timestamps

> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index 9a0d1445ca5c..b4ebdde59398 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -1649,6 +1649,7 @@ static int azx_first_init(struct azx *chip)
> >  		return -ENXIO;
> >  	}
> >  
> > +	azx_parse_capabilities(chip);
> 
> Is it really safe to call the function no matter which chip?

I think so, since we read base HDA registers for capablity. For older hw the
next capablity will not be there.

But yes I will verify this on older machines.

-- 
~Vinod

Re: [PATCH 2/3] ALSA - hda: Add support for link audio time reporting

[Vinod Koul]

On Mon, Jul 11, 2016 at 12:32:07PM +0200, Takashi Iwai wrote:
> On Mon, 11 Jul 2016 12:13:28 +0200,
> Vinod Koul wrote:
> > 
> > From: Guneshwor Singh <guneshwor.o.singh@intel.com>
> > 
> > Skylake onwards HDA controller supports reprting link audio
> > time, so add support for that.
> 
> It's way too few description, the text is almost same as the previous
> patch.  Please give more information.

Sure will add.

> >  #define CREATE_TRACE_POINTS
> >  #include "hda_controller_trace.h"
> >  
> > +#define SEC_TO_NSEC	1000000000LL
> 
> Can we use a definition in time64.h?

Yes NSEC_PER_SEC seems right, will update. Thanks for pointing

> >  
> > +static u64 azx_scale64(u64 base, u32 num, u32 den)
> > +{
> > +	u64 rem;
> > +
> > +	rem = do_div(base, den);
> > +
> > +	base *= num;
> > +	rem *= num;
> > +
> > +	do_div(rem, den);
> > +
> > +	return base + rem;
> > +}
> 
> What is this function supposed to do?

It is supposed to scale the timestamp values. Will try to add more comments
on this one.

> > +		ll_counter = (ll_counter_h << LLPC_CCU_SHIFT) |	ll_counter_l;
> > +		wallclk_cycles = wallclk_ctr & WALFCC_CIF_MASK;
> > +
> > +		if (wallclk_cycles < HDA_MAX_CYCLE_VALUE - HDA_MAX_CYCLE_OFFSET
> > +		    && wallclk_cycles > HDA_MAX_CYCLE_OFFSET)
> > +			break;
> 
> Is this condition really correct...?  It's hard to understand.

Looks so, i will double check

> > +	*device = ktime_add_ns(*device, (wallclk_cycles * SEC_TO_NSEC) /
> > +			       ((HDA_MAX_CYCLE_VALUE+1) * runtime->rate));
> 
> Hmm, the calculation here looks as if there can be an optimization...

Hmmm, let me try to optimize this bit

-- 
~Vinod

Re: [PATCH 3/3] ALSA - hda: Fix timestamping documentation

[Vinod Koul]

On Mon, Jul 11, 2016 at 12:46:05PM +0200, Takashi Iwai wrote:
> On Mon, 11 Jul 2016 12:13:29 +0200,
> Vinod Koul wrote:
> > 
> > Some typos in the documentation, so fix them up.
> > 
> > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> 
> Applied this one, thanks.

Thanks :)

-- 
~Vinod

Re: [PATCH 1/3] ALSA - hda: Add support for GTS capability

[Takashi Iwai]

On Mon, 11 Jul 2016 16:22:15 +0200,
Vinod Koul wrote:
> 
> On Mon, Jul 11, 2016 at 04:06:25PM +0200, Takashi Iwai wrote:
> > On Mon, 11 Jul 2016 16:07:32 +0200,
> > Vinod Koul wrote:
> > > 
> > > > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > > > > index 9a0d1445ca5c..b4ebdde59398 100644
> > > > > --- a/sound/pci/hda/hda_intel.c
> > > > > +++ b/sound/pci/hda/hda_intel.c
> > > > > @@ -1649,6 +1649,7 @@ static int azx_first_init(struct azx *chip)
> > > > >  		return -ENXIO;
> > > > >  	}
> > > > >  
> > > > > +	azx_parse_capabilities(chip);
> > > > 
> > > > Is it really safe to call the function no matter which chip?
> > > 
> > > I think so, since we read base HDA registers for capablity. For older hw the
> > > next capablity will not be there.
> > > 
> > > But yes I will verify this on older machines.
> > 
> > I checked the spec 1.0 and 1.0a, and there is no definition for the
> > register offset 0x14.  So, it's likely unsafe to use it
> > unconditionally.
> 
> Ah okay, let me check.
> 
> Worst case we cna use SKL_PLUS for this as it is defined for SKL and beyond.

Yes.


Takashi

Re: [PATCH 1/3] ALSA - hda: Add support for GTS capability

[Vinod Koul]

On Mon, Jul 11, 2016 at 04:06:25PM +0200, Takashi Iwai wrote:
> On Mon, 11 Jul 2016 16:07:32 +0200,
> Vinod Koul wrote:
> > 
> > > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > > > index 9a0d1445ca5c..b4ebdde59398 100644
> > > > --- a/sound/pci/hda/hda_intel.c
> > > > +++ b/sound/pci/hda/hda_intel.c
> > > > @@ -1649,6 +1649,7 @@ static int azx_first_init(struct azx *chip)
> > > >  		return -ENXIO;
> > > >  	}
> > > >  
> > > > +	azx_parse_capabilities(chip);
> > > 
> > > Is it really safe to call the function no matter which chip?
> > 
> > I think so, since we read base HDA registers for capablity. For older hw the
> > next capablity will not be there.
> > 
> > But yes I will verify this on older machines.
> 
> I checked the spec 1.0 and 1.0a, and there is no definition for the
> register offset 0x14.  So, it's likely unsafe to use it
> unconditionally.

Ah okay, let me check.

Worst case we cna use SKL_PLUS for this as it is defined for SKL and beyond.

-- 
~Vinod

Re: [PATCH 2/3] ALSA - hda: Add support for link audio time reporting

[Vinod Koul]

On Mon, Jul 11, 2016 at 07:43:25PM +0800, kbuild test robot wrote:
> Hi,
> 
> [auto build test ERROR on sound/for-next]
> [also build test ERROR on v4.7-rc7 next-20160711]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Vinod-Koul/ALSA-hda-Add-support-for-link-audio-time-reporting/20160711-180949
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
> config: arm-multi_v7_defconfig (attached as .config)
> compiler: arm-linux-gnueabi-gcc (Debian 5.3.1-8) 5.3.1 20160205
> reproduce:
>         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=arm 
> 
> All errors (new ones prefixed by >>):
> 
> >> sound/pci/hda/hda_controller.c:30:21: fatal error: asm/tsc.h: No such file or directory
>    compilation terminated.

Okay i think I need to move this bit into the intel code.

Takashi, I didnt see any X86 depends on SND_HDA_INTEL. I think we should add
this now. Are you okay with that?

Thanks

>     24	#include <linux/delay.h>
>     25	#include <linux/interrupt.h>
>     26	#include <linux/kernel.h>
>     27	#include <linux/module.h>
>     28	#include <linux/pm_runtime.h>
>     29	#include <linux/slab.h>
>   > 30	#include <asm/tsc.h>
>     31	#include <sound/core.h>
>     32	#include <sound/initval.h>
>     33	#include "hda_controller.h"

-- 
~Vinod

Re: [PATCH 2/3] ALSA - hda: Add support for link audio time reporting

[Takashi Iwai]

On Fri, 15 Jul 2016 06:37:21 +0200,
Vinod Koul wrote:
> 
> On Mon, Jul 11, 2016 at 07:43:25PM +0800, kbuild test robot wrote:
> > Hi,
> > 
> > [auto build test ERROR on sound/for-next]
> > [also build test ERROR on v4.7-rc7 next-20160711]
> > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> > 
> > url:    https://github.com/0day-ci/linux/commits/Vinod-Koul/ALSA-hda-Add-support-for-link-audio-time-reporting/20160711-180949
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
> > config: arm-multi_v7_defconfig (attached as .config)
> > compiler: arm-linux-gnueabi-gcc (Debian 5.3.1-8) 5.3.1 20160205
> > reproduce:
> >         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # save the attached .config to linux build tree
> >         make.cross ARCH=arm 
> > 
> > All errors (new ones prefixed by >>):
> > 
> > >> sound/pci/hda/hda_controller.c:30:21: fatal error: asm/tsc.h: No such file or directory
> >    compilation terminated.
> 
> Okay i think I need to move this bit into the intel code.
> 
> Takashi, I didnt see any X86 depends on SND_HDA_INTEL. I think we should add
> this now. Are you okay with that?

Better to have an explicit ifdef CONFIG_X86 around it.  It's not only
for x86.


Takashi

Re: [PATCH 2/3] ALSA - hda: Add support for link audio time reporting

[Takashi Iwai]

On Fri, 15 Jul 2016 07:19:06 +0200,
Vinod Koul wrote:
> 
> On Fri, Jul 15, 2016 at 07:00:41AM +0200, Takashi Iwai wrote:
> > On Fri, 15 Jul 2016 06:37:21 +0200,
> > Vinod Koul wrote:
> > > 
> > > On Mon, Jul 11, 2016 at 07:43:25PM +0800, kbuild test robot wrote:
> > > > Hi,
> > > > 
> > > > [auto build test ERROR on sound/for-next]
> > > > [also build test ERROR on v4.7-rc7 next-20160711]
> > > > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> > > > 
> > > > url:    https://github.com/0day-ci/linux/commits/Vinod-Koul/ALSA-hda-Add-support-for-link-audio-time-reporting/20160711-180949
> > > > base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
> > > > config: arm-multi_v7_defconfig (attached as .config)
> > > > compiler: arm-linux-gnueabi-gcc (Debian 5.3.1-8) 5.3.1 20160205
> > > > reproduce:
> > > >         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
> > > >         chmod +x ~/bin/make.cross
> > > >         # save the attached .config to linux build tree
> > > >         make.cross ARCH=arm 
> > > > 
> > > > All errors (new ones prefixed by >>):
> > > > 
> > > > >> sound/pci/hda/hda_controller.c:30:21: fatal error: asm/tsc.h: No such file or directory
> > > >    compilation terminated.
> > > 
> > > Okay i think I need to move this bit into the intel code.
> > > 
> > > Takashi, I didnt see any X86 depends on SND_HDA_INTEL. I think we should add
> > > this now. Are you okay with that?
> > 
> > Better to have an explicit ifdef CONFIG_X86 around it.  It's not only
> > for x86.
> 
> Need it around the whole of the timestamp code as well then..

Yes.  But why TSC is mandatory?  There is no explanation in your
patch.

> Yes the HDA controller is not x86 specfic, but the hda-intel should be,
> right?

No, the driver depends on PCI, but not on CPU.


thanks,

Takashi

Re: [PATCH 2/3] ALSA - hda: Add support for link audio time reporting

[Vinod Koul]

On Fri, Jul 15, 2016 at 07:00:41AM +0200, Takashi Iwai wrote:
> On Fri, 15 Jul 2016 06:37:21 +0200,
> Vinod Koul wrote:
> > 
> > On Mon, Jul 11, 2016 at 07:43:25PM +0800, kbuild test robot wrote:
> > > Hi,
> > > 
> > > [auto build test ERROR on sound/for-next]
> > > [also build test ERROR on v4.7-rc7 next-20160711]
> > > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> > > 
> > > url:    https://github.com/0day-ci/linux/commits/Vinod-Koul/ALSA-hda-Add-support-for-link-audio-time-reporting/20160711-180949
> > > base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
> > > config: arm-multi_v7_defconfig (attached as .config)
> > > compiler: arm-linux-gnueabi-gcc (Debian 5.3.1-8) 5.3.1 20160205
> > > reproduce:
> > >         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
> > >         chmod +x ~/bin/make.cross
> > >         # save the attached .config to linux build tree
> > >         make.cross ARCH=arm 
> > > 
> > > All errors (new ones prefixed by >>):
> > > 
> > > >> sound/pci/hda/hda_controller.c:30:21: fatal error: asm/tsc.h: No such file or directory
> > >    compilation terminated.
> > 
> > Okay i think I need to move this bit into the intel code.
> > 
> > Takashi, I didnt see any X86 depends on SND_HDA_INTEL. I think we should add
> > this now. Are you okay with that?
> 
> Better to have an explicit ifdef CONFIG_X86 around it.  It's not only
> for x86.

Need it around the whole of the timestamp code as well then..

Yes the HDA controller is not x86 specfic, but the hda-intel should be,
right? This would make me move all this code into hda_intel.c as well..

-- 
~Vinod

Re: [PATCH 2/3] ALSA - hda: Add support for link audio time reporting

[Vinod Koul]

On Fri, Jul 15, 2016 at 07:17:14AM +0200, Takashi Iwai wrote:
> On Fri, 15 Jul 2016 07:19:06 +0200,
> Vinod Koul wrote:
> > > > > >> sound/pci/hda/hda_controller.c:30:21: fatal error: asm/tsc.h: No such file or directory
> > > > >    compilation terminated.
> > > > 
> > > > Okay i think I need to move this bit into the intel code.
> > > > 
> > > > Takashi, I didnt see any X86 depends on SND_HDA_INTEL. I think we should add
> > > > this now. Are you okay with that?
> > > 
> > > Better to have an explicit ifdef CONFIG_X86 around it.  It's not only
> > > for x86.
> > 
> > Need it around the whole of the timestamp code as well then..
> 
> Yes.  But why TSC is mandatory?  There is no explanation in your
> patch.

HW reports ART values and we need to convert these to TSC.

The callflow is that the azx_get_crosststamp is called which invokes
get_device_system_crosststamp() and calls azx_get_sync_time callback.

So dependency is around convert_art_to_tsc() which is x86 API.

Thanks
-- 
~Vinod

Re: [PATCH 2/3] ALSA - hda: Add support for link audio time reporting

[Takashi Iwai]

On Fri, 15 Jul 2016 07:39:10 +0200,
Vinod Koul wrote:
> 
> On Fri, Jul 15, 2016 at 07:17:14AM +0200, Takashi Iwai wrote:
> > On Fri, 15 Jul 2016 07:19:06 +0200,
> > Vinod Koul wrote:
> > > > > > >> sound/pci/hda/hda_controller.c:30:21: fatal error: asm/tsc.h: No such file or directory
> > > > > >    compilation terminated.
> > > > > 
> > > > > Okay i think I need to move this bit into the intel code.
> > > > > 
> > > > > Takashi, I didnt see any X86 depends on SND_HDA_INTEL. I think we should add
> > > > > this now. Are you okay with that?
> > > > 
> > > > Better to have an explicit ifdef CONFIG_X86 around it.  It's not only
> > > > for x86.
> > > 
> > > Need it around the whole of the timestamp code as well then..
> > 
> > Yes.  But why TSC is mandatory?  There is no explanation in your
> > patch.
> 
> HW reports ART values and we need to convert these to TSC.
> 
> The callflow is that the azx_get_crosststamp is called which invokes
> get_device_system_crosststamp() and calls azx_get_sync_time callback.
> 
> So dependency is around convert_art_to_tsc() which is x86 API.

How is defined in the spec?  I wonder it because HD-audio spec itself
is usually CPU-neutral.


Takashi

Re: [PATCH 2/3] ALSA - hda: Add support for link audio time reporting

[Vinod Koul]

On Fri, Jul 15, 2016 at 07:39:17AM +0200, Takashi Iwai wrote:
> > 
> > So dependency is around convert_art_to_tsc() which is x86 API.
> 
> How is defined in the spec?  I wonder it because HD-audio spec itself
> is usually CPU-neutral.

You are right. HDA spec is not x86 specfic. HDA counter report the value
of ART counter for a time snapshot.

The problem is we do not get TSC value from HDA controller :(

So we need to use asm code for conversion..

Thanks
-- 
~Vinod

Re: [PATCH 2/3] ALSA - hda: Add support for link audio time reporting

[Takashi Iwai]

On Fri, 15 Jul 2016 07:50:25 +0200,
Vinod Koul wrote:
> 
> On Fri, Jul 15, 2016 at 07:39:17AM +0200, Takashi Iwai wrote:
> > > 
> > > So dependency is around convert_art_to_tsc() which is x86 API.
> > 
> > How is defined in the spec?  I wonder it because HD-audio spec itself
> > is usually CPU-neutral.
> 
> You are right. HDA spec is not x86 specfic. HDA counter report the value
> of ART counter for a time snapshot.
> 
> The problem is we do not get TSC value from HDA controller :(
> 
> So we need to use asm code for conversion..

OK, then let's cover the relevant code with ifdef CONFIG_X86 as a
temporary workaround until we get a more universal solution.


Takashi