| Message ID | 1404997458-15377-4-git-send-email-tiwai@suse.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5646eda5851e6cfdfa22d41895e3f5daffa643d3 |
| Delegated to: | Takashi Iwai |
| Headers | show |
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. 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. > > Along with this, increment the PCM protocol version. > > The extension was suggested by Clemens Ladisch. > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > 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; >
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. Takashi > > Along with this, increment the PCM protocol version. > > > > The extension was suggested by Clemens Ladisch. > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > --- > > 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; > > >
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 <tiwai@suse.de> >>> --- >>> 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; >>> >>
Pierre-Louis Bossart wrote: > On 7/10/14, 10:42 AM, Takashi Iwai wrote: >> 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. The difference between sw_params and hw_params is that the latter are affected by device constraints (and might have interdependencies). That sw_params can be changed at any time is just a consequence of that. > If we want user-space to do the right thing we should at least > document the consequences of a dynamic switch. Changing the clock relative to which the timestamp is reported is not only the desired but the _only_ consequence of this parameter. (And changing from/to GETTIMEOFDAY was already possible.) Regards, Clemens
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;
For allowing adjusting the timestamp type on the fly, add it to sw_params. The existing ioctl is still kept for compatibility. Along with this, increment the PCM protocol version. The extension was suggested by Clemens Ladisch. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- include/uapi/sound/asound.h | 6 ++++-- sound/core/pcm_native.c | 3 +++ 2 files changed, 7 insertions(+), 2 deletions(-)