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

[Takashi Iwai <tiwai@suse.de>]

At Fri, 12 Dec 2014 09:20:56 -0600,
Pierre-Louis Bossart wrote:
> 
> On 12/12/14, 2:37 AM, Takashi Iwai wrote:
> > At Thu, 11 Dec 2014 20:36:48 -0600,
> > Pierre-Louis Bossart wrote:
> >>
> >>
> >>>> if someone used alsa-lib with the .get_wall_clock(), the new user-space
> >>>> code will provide the same results as today, no change (wall clock if
> >>>> supported, hw_ptr otherwise). So the library compatibility is preserved.
> >>>
> >>> You can't assume that all users always upgrade alsa-lib.
> >>> Users may use still the old alsa-lib with the new kernel.
> >>>
> >>>> I don't mind adding a compatible kernel behavior for HDAudio only, but
> >>>> is this really necessary?
> >>>
> >>> Yes, the kernel is not allowed to give any regression, if we know it
> >>> would.
> >>
> >> ok, will add a backwards-compatible mode. no problem.
> >>
> >>>> I added a set of INFO defines and the matching is_supported queries in
> >>>> alsa-lib. I just did a pretty dumb copy/paste/edit there, maybe we can
> >>>> refactor the code here with a single routine taking a type parameter.
> >>>> feedback welcome there.
> >>>
> >>> Right.  But another concern is that this method will consume one INFO
> >>> bit at each time the new tstamp type is extended.  This is another
> >>> concern.  Exposing this information in another place would be better,
> >>> IMO, if any better place is found...
> >>
> >> I asked about this one in late October... I mentioned that we are
> >> running out of INFO (half already used) and AZX_CAPS (28 bits used)
> >> fields, and for INFO it didn't seem like a big deal.
> >
> > The INFO bits are the public ABI and can't be changed later while
> > AZX_DCAPS is the internal stuff we can change at any time.  So,
> > defining a new stuff for SND_PCM_INFO must be done very carefully.
> >
> >
> >> If adding more fields to the info field is viewed as problematic, the
> >> only options I can think of are:
> >> - reclaim a reserved word in hw_params, e.g. rename to info1 to do
> >> something like this in alsa-lib:
> >> 	return !!(params->info1 & SNDRV_PCM_INFO1_IS_THIS_HARDWARE_BROKEN)
> >
> > It's OK to just add a new hw_param_mask element.  Then we can handle
> > up to 32 tstamp types and that should be enough.
> >
> > The question is whether hw_params is the best place.  Usually
> > hw_params is the place to select the one configuration from multiple
> > choices or space.  In this case, we don't choose only one, right?
> 
> I think we are not aligned here. I am only using the INFO fields as a 
> means to report what the hardware can do, not for any selection. Then 
> the actual selection of the timestamps is done as a dynamic 
> configuration parameter for the STATUS ioctl. The hw_params is not a 
> good candidate since it's frozen after the start, we do need to be able 
> to query different timestamps dynamically.

I somehow misunderstood you were suggesting hw_params mask extension,
and the comment above was for that.

Thinking of this again, IMO, we may extend hw_params by assigning one
more int field from reserved area.  This will be used as a read only
information like info bits, just indicating the available tstamp
types.

> 
> >> - keep a 32 bit word but add a paging register in the msb to reuse lsbs.
> >> Either way some more code will be required in both driver and library.
> >
> > Which word to reuse?  Could you elaborate?
> 
> Never mind, what I had in mind would require tons of changes in 
> user-space code and would break the backwards compatibility if one used 
> a new kernel and an older library.

OK.


thanks,

Takashi