Re: [PATCH RFC 5/9] ALSA: core: selection of audio_tstamp type and accuracy reports

[Takashi Iwai <tiwai@suse.de>]

At Wed, 10 Dec 2014 18:39:09 +0100,
Takashi Iwai wrote:
> 
> At Wed, 10 Dec 2014 11:27:26 -0600,
> Pierre-Louis Bossart wrote:
> > 
> > On 12/10/14, 10:35 AM, Takashi Iwai wrote:
> > > Just a minor issue before going to detailed review:
> > >
> > > At Mon,  8 Dec 2014 16:23:42 -0600,
> > > Pierre-Louis Bossart wrote:
> > >>
> > >> +/* user space provides config to kernel */
> > >> +struct snd_pcm_audio_tstamp_config {
> > >> +	__u32 type_requested:4;
> > >> +	__u32 report_delay:1; /* add total delay to A/D or D/A */
> > >> +};
> > > ....
> > >> +/* kernel provides report to user-space */
> > >> +struct snd_pcm_audio_tstamp_report {
> > >> +	/* actual type if hardware could not support requested timestamp */
> > >> +	__u32 actual_type:4;
> > >> +
> > >> +	/* accuracy represented in mantissa/exponent form */
> > >> +	__u32 accuracy_report:1; /* 0 if accuracy unknown, 1 if rest of structure is valid */
> > >> +	__u32 accuracy_m:7; /* 0..127, ~3 significant digit for mantissa */
> > >> +	__u32 accuracy_e:4; /* base10 exponent, 0 for ns, 3 for us, 6 for ms, 9 for s */
> > >> +};
> > >
> > > Please avoid the bit fields in these, since these values will be a
> > > part of ABI.  There is absolutely no compatibility when you're using
> > > bitfields.  Use the explicit bit operations.
> > 
> > Those definitions are not used as part of the kernel/user interaction, I 
> > did use pack/unpack operations as requested in previous reviews. see 
> > snd_pcm_unpack_tstamp_config and snd_pcm_pack_audio_tstamp_report.
> > A similar pack/unpack operation is provided in the alsa-lib patches
> > The data is exchanged using the reclaimed 32-bit word only.
> > 
> > Would that work?
> 
> Ah OK, then it's fine.  Then could you add more comments mentioning
> that the structs are internal only and converted by the dedicated
> functions for kernel ABI?
> 
> Also, use simple u32 instead of __u32.  Then it's clearer that it's an
> internal usage.

A few more items that came to my mind later:

- It'd be better to align both input and output; namely, the input
  struct and output struct must be compatible.
  Currently, report_delay flag is overwritten in return.  This bit
  should be kept upon read back.

  This essentially unifies snd_pcm_audio_tstamp_config and
  snd_pcm_audio_tstamp_report.  We don't have to pass two structs to
  callbacks.

- Or, we may avoid packing/unpacking by defining the struct like:

     unsigned char type;
     unsigned char flags;
     unsigned char accuracy_mantessa;
     unsigned char accuracy_exponent;

  where flags field contains the bit flags for report_delay and
  accuracy_report.

  That said, I'm worrying a bit about the complexity of the new
  callback function.

- The biggest concern I have now is whether it's really feasible to
  change the semantics of snd_pcm_status ioctl, i.e. from the
  read-only to the write-read.  I guess this would work as the padding
  field is very likely zero even in the very old alsa-lib version.

- Another concern is the compatibility with the current wallclock
  implementation.  Judging from your patch, the audio_tstamp won't be
  obtained from get_time_info callback in the default tstamp mode,
  right?  This may result in a regression, as currently the driver
  always gives the h/w audio_tstamp when the driver supports the
  wallclock.

- Last but not least: we're receiving multiple enhancement requests
  regarding tstamp at the very same time.  This patchset conflicts
  with Tim and Nick's start_at extention. 

  I believe this can be resolved later, but let's discuss the ground
  line at first: the requirement and influence on both changes.


thanks,

Takashi