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

[Takashi Iwai <tiwai@suse.de>]

On Wed, 03 Aug 2016 18:06:14 +0200,
Vinod Koul wrote:
> +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;

Superfluous cast.

> +	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);

You can use
		snd_hdac_chip_writel(azx_bus(chip), GTSCC, dma_select);

The use of _snd_hdac_chip_write() is for non-constant registers.  When
you pass AZX_REG_XXX, you can use the standard macro.  For example:


> +		/* 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);

This can be simplified with snd_hdac_chip_updatel().

> +
> +		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;
> +
> +		/*
> +		 * 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 i.e.,
> +		 * HDA_MAX_CYCLE_OFFSET
> +		 */
> +		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");

Hrm, this has a danger to spew huge amount of error messages, since
this gets called so often.


> +		return -EIO;
> +	}
> +
> +	*device = ns_to_ktime(azx_scale64(ll_counter,
> +				NSEC_PER_SEC, runtime->rate));
> +	*device = ktime_add_ns(*device, (wallclk_cycles * NSEC_PER_SEC) /
> +			       ((HDA_MAX_CYCLE_VALUE + 1) * runtime->rate));
> +
> +	*system = convert_art_to_tsc(tsc_counter);
> +
> +	return 0;
> +}
> +
> +#else
> +static int azx_get_sync_time(ktime_t *device,
> +		struct system_counterval_t *system, void *ctx)
> +{
> +	return -ENXIO;
> +}
> +#endif
> +
> +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 +524,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)) {

The indentation is hard to follow here...

> +
> +		azx_get_crosststamp(substream, &xtstamp);

No error check?


thanks,

Takashi

> +
> +		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
>