From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH RFC 5/9] ALSA: core: selection of audio_tstamp type and accuracy reports Date: Wed, 10 Dec 2014 21:08:48 +0100 Message-ID: References: <1418077426-8309-1-git-send-email-pierre-louis.bossart@linux.intel.com> <1418077426-8309-6-git-send-email-pierre-louis.bossart@linux.intel.com> <5488827E.4030403@linux.intel.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id E1B3C2605C1 for ; Wed, 10 Dec 2014 21:08:49 +0100 (CET) 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: Pierre-Louis Bossart Cc: Nick Stoughton , alsa-devel@alsa-project.org, Tim Cussins List-Id: alsa-devel@alsa-project.org 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