[alsa-devel] RFC: PCM extra attributes

[Takashi Iwai]

At Fri, 19 Jun 2009 13:14:15 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> On Fri, 19 Jun 2009, Takashi Iwai wrote:
> 
> > At Fri, 19 Jun 2009 12:45:04 +0200 (CEST),
> > Jaroslav Kysela wrote:
> >>
> >> On Fri, 19 Jun 2009, Jaroslav Kysela wrote:
> >>
> >>> On Fri, 19 Jun 2009, Takashi Iwai wrote:
> >>>
> >>>> At Fri, 19 Jun 2009 10:47:30 +0200 (CEST),
> >>>> Jaroslav Kysela wrote:
> >>>>>
> >>>>> On Fri, 19 Jun 2009, Takashi Iwai wrote:
> >>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> this is yet another topic I'm (currently) working on -- the addition
> >>>>>> of PCM ioctls to get/set some extra attributes.  Basically, it adds
> >>>>>> two simple ioctls for getting/setting extra attributes to the PCM
> >>>>>> substream.  The attribute has a sort of TLV form,
> >>>>>>
> >>>>>>  /* PCM extra attributes */
> >>>>>>  struct snd_pcm_attr {
> >>>>>>  	unsigned int type;	/* SNDRC_PCM_TYPE_ATTR_XXX */
> >>>>>>  	unsigned int len;	/* GET R: the max elements in value array
> >>>>>>  				 *     W: the actually written # elements
> >>>>>>  				 * SET R/W: # elements to store
> >>>>>>  				 */
> >>>>>>  	unsigned int value[0];	/* value(s) to read / write */
> >>>>>>  };
> >>>>>>
> >>>>>> And corresponding two ioctls
> >>>>>>  #define SNDRV_PCM_IOCTL_GET_ATTR	_IOWR('A', 0x14, struct snd_pcm_attr)
> >>>>>>  #define SNDRV_PCM_IOCTL_SET_ATTR	_IOWR('A', 0x15, struct snd_pcm_attr)
> >>>>>
> >>>>> I would prefer to implement similar TLV implementation as for the control
> >>>>> API. The amount of information for reading (get) will be small, so
> >>>>> filtering in this direction is not necessary. Also, common parts of
> >>>>> implementation (future merging of more TLVs to compounds) can be shared.
> >>>>
> >>>> Actually it's a sort of TLV.  You see exactly it in snd_pcm_attr
> >>>> struct, no? :)
> >>>>
> >>>> And, thinking twice after posting (that's a good effect of posting to
> >>>> ML, BTW), I feel that using a callback would be a better way, such as
> >>>> re-using the existing ops->ioctl with a new cmd tag rather than the
> >>>> statically assigned thing.
> >>>>
> >>>> A similar method like control TLV can be used, too.  However, a
> >>>> distinct from the existing control TLV is that this is intended for
> >>>> just one type of information while the control TLV is supposed to
> >>>> contain everything in a single shot.
> >>>>
> >>>> That is, this is a query with a key.  In that sense, sharing a small
> >>>> amount of control TLV code (about 10 lines) doesn't give a big
> >>>> benefit.  In anyways, it's a implementation detail, so one could
> >>>> optimize somehow, though...
> >>>
> >>> I don't mean current implementation. TLVs can be nested. In this case, we
> >>> need a set of functions which operates with TLVs (merging). These
> >>> functions can be shared. It's also possible to share TLV code in
> >>> the user space (search). But it's really implementation detail. We should
> >>> focus on ioctl definitions now.
> >>>
> >>> I would defined 'struct snd_pcm_attr' as 'struct snd_tlv' - it's same as
> >>> for control API.
> >>>
> >>> The control API has:
> >>>
> >>> SNDRV_CTL_IOCTL_TLV_READ	- read all static information
> >>> SNDRV_CTL_IOCTL_TLV_WRITE	- write static information (userspace controls)
> >>> SNDRV_CTL_IOCTL_TLV_COMMAND	- change some setup
> >>>
> >>> So, SNDRV_CTL_IOCTL_TLV_COMMAND == SNDRV_PCM_IOCTL_SET_ATTR in your
> >>> proposal.
> >>>
> >>> SNDRV_CTL_IOCTL_TLV_WRITE is not probably useable unless we have virtual
> >>> user-space PCM interface kernel implementation.
> >>>
> >>> SNDRV_CTL_IOCTL_TLV_READ might make sense for static-only information
> >>> which don't change between open()/close() syscalls for given substream.
> >>>
> >>> SNDRV_PCM_IOCTL_GET_ATTR cannot be mapped at this time. Might be something
> >>> like TLV_READONE, TLV_CONFIG, TLV_SETUP, TLV_GET or so - what's better
> >>> for COMMAND word, if we agree on common names for all kernel interfaces.
> >>
> >> BTW: It's also question, if to divide TLVs to static/configuration ones.
> >> TLV_READ might just return all TLVs and TLV_READONE filter only one, if
> >> user space does not want to obtain all information.
> >>
> >> I would like to preserve TLV_READ to obtain all TLVs for possible user
> >> space enumeration (for example for debugging purposes) rather that do a
> >> single query for all possible TLV types.
> >
> > I disagree with "all-in-on-TLV" strategy.  That makes life much harder.
> > Sorry, it's no go.
> 
> Sorry, the implementation can be more simple than you think. Imagine just 
> a TLV callback in the lowlevel driver and switch/case statement in the 
> callback. We can define a special type in kernel that queries for all 
> present TLV types (bitmap) and the ioctl TLV_READ implementation can just 
> compose results from single queries. So, the code in the lowlevel driver 
> will grow only with 4 (or less) lines:
> 
>  	case TLV_TYPES:
>  		tlv.length = 4;
>  		tlv.data[0] = (1<<TLV_CHANNEL_MAP) | (1<<TLV_CONTROL_IDS);
>  		return 0;

Sorry I can't draw a picture from your explanation.
How can it compose a "all-in-one" TLV at each time?

What I'm talking is like the scenario below:

- app open PCM, do hw_params
- app requires the channel map, get one
- app changes hw_params again, then get channel map again

With "all-in-one" TLV, you have to read all the information at each
time you get channel maps because the channel map is to be generated
dynamically.  At each time, the kernel needs to gather all
information, compose into a single big TLV, and copy it.  Then
user-space decomposes the big TLV, look for a specific tag, then picks
up the value there.

Why not simply query a value and get a value a la normal ioctl?


Takashi