From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pierre-Louis Bossart Subject: Re: [PATCH 3/3] ALSA: pcm: Add timestamp type to sw_params Date: Thu, 10 Jul 2014 12:46:10 -0500 Message-ID: <53BED162.808@linux.intel.com> References: <1404997458-15377-1-git-send-email-tiwai@suse.de> <1404997458-15377-4-git-send-email-tiwai@suse.de> <53BEAFF9.5070501@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by alsa0.perex.cz (Postfix) with ESMTP id 0984D2607C8 for ; Thu, 10 Jul 2014 19:46:14 +0200 (CEST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Takashi Iwai Cc: alsa-devel@alsa-project.org, Mark Brown , Clemens Ladisch , Daniel Thompson List-Id: alsa-devel@alsa-project.org On 7/10/14, 10:42 AM, Takashi Iwai wrote: > At Thu, 10 Jul 2014 10:23:37 -0500, > Pierre-Louis Bossart wrote: >> >> On 7/10/14, 8:04 AM, Takashi Iwai wrote: >>> For allowing adjusting the timestamp type on the fly, add it to >>> sw_params. The existing ioctl is still kept for compatibility. >> >> I have strong objections to this extension. It will result in a >> discontinuity in the timestamps reported in pcm_status without a clear >> indication of what timestamp is reported (when does this change occur?), >> and it's completely unclear how userspace would handle a step (positive >> or negative) between ntp-adjusted and non-ntp-adjusted times. > > Well, I don't understand the logic; it's app itself who changes the > timestamp type. It must know when it's changed (because app is > changing it), and how to handle (because app chooses the timestamp > type). And the current type can be queried via sw_params_*_get() > thing. > > Of course, as default, there is no change from the current behavior -- > it takes CLOCK_MONOTONIC. So, there should be no breakage by this > change unless you change the application side to use the new call. > >> For apps >> that make use of the audio_time reported with a wall clock this could >> lead to complete nonsense. >> I would have no problems if this was a fixed parameter defined once >> before audio streaming starts. > > You don't have to change the setup after the stream starts. It's > usually set before the stream starting. The point of using sw_params > is that it's independent from the hardware driver, thus it fits better > than hw_params. The switching after stream isn't the purpose. It's > just a gratis bonus. So we agree that the dynamic switch isn't the intended usage but we allow for it anyway... I guess given that we can enable/disable timestamps dynamically this follows the same logic, it's just weird to have a sw_param whose intended use is really a hw_param, something you set once and don't modify later. If we want user-space to do the right thing we should at least document the consequences of a dynamic switch. No further objections. > > Takashi > >>> Along with this, increment the PCM protocol version. >>> >>> The extension was suggested by Clemens Ladisch. >>> >>> Signed-off-by: Takashi Iwai >>> --- >>> include/uapi/sound/asound.h | 6 ++++-- >>> sound/core/pcm_native.c | 3 +++ >>> 2 files changed, 7 insertions(+), 2 deletions(-) >>> >>> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h >>> index cbf7dc850a46..a7e062f91f39 100644 >>> --- a/include/uapi/sound/asound.h >>> +++ b/include/uapi/sound/asound.h >>> @@ -139,7 +139,7 @@ struct snd_hwdep_dsp_image { >>> * * >>> *****************************************************************************/ >>> >>> -#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 11) >>> +#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 12) >>> >>> typedef unsigned long snd_pcm_uframes_t; >>> typedef signed long snd_pcm_sframes_t; >>> @@ -391,7 +391,9 @@ struct snd_pcm_sw_params { >>> snd_pcm_uframes_t silence_threshold; /* min distance from noise for silence filling */ >>> snd_pcm_uframes_t silence_size; /* silence block size */ >>> snd_pcm_uframes_t boundary; /* pointers wrap point */ >>> - unsigned char reserved[64]; /* reserved for future */ >>> + unsigned int tstamp_type; /* timestamp type */ >>> + int pads; /* alignment, reserved */ >>> + unsigned char reserved[56]; /* reserved for future */ >>> }; >>> >>> struct snd_pcm_channel_info { >>> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c >>> index 2372c49a8e84..81dedc381efd 100644 >>> --- a/sound/core/pcm_native.c >>> +++ b/sound/core/pcm_native.c >>> @@ -543,6 +543,8 @@ static int snd_pcm_sw_params(struct snd_pcm_substream *substream, >>> >>> if (params->tstamp_mode > SNDRV_PCM_TSTAMP_LAST) >>> return -EINVAL; >>> + if (params->tstamp_type > SNDRV_PCM_TSTAMP_TYPE_LAST) >>> + return -EINVAL; >>> if (params->avail_min == 0) >>> return -EINVAL; >>> if (params->silence_size >= runtime->boundary) { >>> @@ -557,6 +559,7 @@ static int snd_pcm_sw_params(struct snd_pcm_substream *substream, >>> err = 0; >>> snd_pcm_stream_lock_irq(substream); >>> runtime->tstamp_mode = params->tstamp_mode; >>> + runtime->tstamp_type = params->tstamp_type; >>> runtime->period_step = params->period_step; >>> runtime->control->avail_min = params->avail_min; >>> runtime->start_threshold = params->start_threshold; >>> >>