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