RFC: PCM extra attributes

RFC: PCM extra attributes

[Takashi Iwai]

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)

The caller sets type and len fields.  For getting attributes, the len
points the max buffer length, and in return, the actual size is stored
there.  For setting, it's the size to set.

Now, for getting the control ids associated to a PCM substream, it'll
be like

	unsigned int buf[32];
	buf[0] = SNDRV_PCM_ATTR_ASSOC_CTLS;
	buf[1] = ARRAY_SIZE(buf) - 2; /* minus header size */
	ioctl(fd, SNDRV_PCM_IOCTL_GET_ATTR, buf);

The kernel side implementation (a core part) is attached below.
In this prototype, each driver is supposed to allocate and set
substream->assoc_ctls and substream->num_assoc_ctls appropriately.
I chose it since the control ids are statically associated to the PCM
substreams at the driver loading time.  It could be, of course, a
liked list with kmalloc, but I thought it's overkill.

Well, that's a pretty subtle thing and can be changed at any time.
But, we need to define ABI properly from the beginning.

So, if you have any comments on ABI or on the whole design, please
speak up.

An important note is that I'm planning to use this framework for
getting/setting the PCM (surround) channel mapping.  It's
straightforward for this ABI.  The internal implementation may vary,
but the interface is the question now.  For example, one could imagine
that the channel mapping belongs to hw_params.  But, it looks
difficult because of the nature of channel mapping; it doesn't match
with a simple integer parameter as hw_parmas has.

The PCM channel mapping feature will be certainly useful for PA (and
even for normal apps with a new alsa-lib plugin to convert the correct
the channel mapping properly on demand).  So, this is rather more
interesting stuff to me, too...


thanks,

Takashi

---
diff --git a/include/sound/asound.h b/include/sound/asound.h
index 82aed3f..159826b 100644
--- a/include/sound/asound.h
+++ b/include/sound/asound.h
@@ -445,6 +445,21 @@ struct snd_xfern {
 	snd_pcm_uframes_t frames;
 };
 
+/* 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 */
+};
+
+/* PCM attribute types */
+enum {
+	SNDRV_PCM_ATTR_ASSOC_CTLS,	/* array of associated ctl ids */
+};
+
 enum {
 	SNDRV_PCM_TSTAMP_TYPE_GETTIMEOFDAY = 0,	/* gettimeofday equivalent */
 	SNDRV_PCM_TSTAMP_TYPE_MONOTONIC,	/* posix_clock_monotonic equivalent */
@@ -459,6 +474,8 @@ enum {
 #define SNDRV_PCM_IOCTL_HW_PARAMS	_IOWR('A', 0x11, struct snd_pcm_hw_params)
 #define SNDRV_PCM_IOCTL_HW_FREE		_IO('A', 0x12)
 #define SNDRV_PCM_IOCTL_SW_PARAMS	_IOWR('A', 0x13, struct snd_pcm_sw_params)
+#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)
 #define SNDRV_PCM_IOCTL_STATUS		_IOR('A', 0x20, struct snd_pcm_status)
 #define SNDRV_PCM_IOCTL_DELAY		_IOR('A', 0x21, snd_pcm_sframes_t)
 #define SNDRV_PCM_IOCTL_HWSYNC		_IO('A', 0x22)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 2389352..98416b0 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -392,6 +392,10 @@ struct snd_pcm_substream {
 	struct snd_info_entry *proc_prealloc_entry;
 	struct snd_info_entry *proc_prealloc_max_entry;
 #endif
+	/* associated controls */
+	unsigned int num_assoc_ctls;
+	unsigned int *assoc_ctls;
+
 	/* misc flags */
 	unsigned int hw_opened: 1;
 };
diff --git a/sound/core/pcm.c b/sound/core/pcm.c
index 145931a..7a46674 100644
--- a/sound/core/pcm.c
+++ b/sound/core/pcm.c
@@ -748,6 +748,7 @@ static void snd_pcm_free_stream(struct snd_pcm_str * pstr)
 		substream_next = substream->next;
 		snd_pcm_timer_done(substream);
 		snd_pcm_substream_proc_done(substream);
+		kfree(substream->assoc_ctls);
 		kfree(substream);
 		substream = substream_next;
 	}
diff --git a/sound/core/pcm_compat.c b/sound/core/pcm_compat.c
index 08bfed5..07512b3 100644
--- a/sound/core/pcm_compat.c
+++ b/sound/core/pcm_compat.c
@@ -485,6 +485,8 @@ static long snd_pcm_ioctl_compat(struct file *file, unsigned int cmd, unsigned l
 	case SNDRV_PCM_IOCTL_TTSTAMP:
 	case SNDRV_PCM_IOCTL_HWSYNC:
 	case SNDRV_PCM_IOCTL_PREPARE:
+	case SNDRV_PCM_IOCTL_GET_ATTR:
+	case SNDRV_PCM_IOCTL_SET_ATTR:
 	case SNDRV_PCM_IOCTL_RESET:
 	case SNDRV_PCM_IOCTL_START:
 	case SNDRV_PCM_IOCTL_DROP:
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 84da3ba..46129c7 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2492,6 +2492,43 @@ static int snd_pcm_tstamp(struct snd_pcm_substream *substream, int __user *_arg)
 	return 0;
 }
 		
+/* store back the associated ctl ids to the given PCM substream */
+static int snd_pcm_get_attr_assoc_ctls(struct snd_pcm_substream *substream,
+				       unsigned int __user *arg)
+{
+	unsigned int len, i;
+
+	arg++;
+	if (get_user(len, arg))
+		return -EFAULT;
+	for (i = 0; i < len && i < substream->num_assoc_ctls; i++)
+		put_user(substream->assoc_ctls[i], arg + i + 1);
+	put_user(i, arg);
+	return 0;
+}
+
+/* GET_ATTR ioctl */
+static int snd_pcm_get_attr(struct snd_pcm_substream *substream,
+			    unsigned int __user *arg)
+{
+	unsigned int type;
+
+	if (get_user(type, arg))
+		return -EFAULT;
+	switch (type) {
+	case SNDRV_PCM_ATTR_ASSOC_CTLS:
+		return snd_pcm_get_attr_assoc_ctls(substream, arg);
+	}
+	return -ENOENT;
+}
+
+/* SET_ATTR ioctl */
+static int snd_pcm_set_attr(struct snd_pcm_substream *substream,
+			    struct snd_pcm_attr __user *arg)
+{
+	return -ENOENT;
+}
+
 static int snd_pcm_common_ioctl1(struct file *file,
 				 struct snd_pcm_substream *substream,
 				 unsigned int cmd, void __user *arg)
@@ -2519,6 +2556,10 @@ static int snd_pcm_common_ioctl1(struct file *file,
 		return snd_pcm_channel_info_user(substream, arg);
 	case SNDRV_PCM_IOCTL_PREPARE:
 		return snd_pcm_prepare(substream, file);
+	case SNDRV_PCM_IOCTL_GET_ATTR:
+		return snd_pcm_get_attr(substream, arg);
+	case SNDRV_PCM_IOCTL_SET_ATTR:
+		return snd_pcm_set_attr(substream, arg);
 	case SNDRV_PCM_IOCTL_RESET:
 		return snd_pcm_reset(substream);
 	case SNDRV_PCM_IOCTL_START:

Re: RFC: PCM extra attributes

[Jaroslav Kysela]

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.

 					Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: RFC: PCM extra attributes

[Takashi Iwai]

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...


thanks,

Takashi

Re: RFC: PCM extra attributes

[Takashi Iwai]

At Fri, 19 Jun 2009 10:54:15 +0200,
Florian Faber wrote:
> 
> Takashi,
> 
> > An important note is that I'm planning to use this framework for
> > getting/setting the PCM (surround) channel mapping.
> 
> I don't understand what this means. Who maps what on what and why?

It's a mapping of surround channels.  Right now, ALSA assumes (usually)
the order of channels like front-left, front-rear, surround-left,
surround-rear, center, LFE, ...  But this really depends on the
device.  Some (or many) need FL/FR/C/LFE/SR/SR order, for example.
And, some devices can allow you even to reassign the channel map.

This is likely an additional information, which not all drivers have
to provide.


Takashi

Re: RFC: PCM extra attributes

[Jaroslav Kysela]

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.

 						Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: RFC: PCM extra attributes

[Takashi Iwai]

At Fri, 19 Jun 2009 12:29:17 +0200 (CEST),
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.

Well, you still missed the difference between the control TLV and the
PCM attr I proposed.  The former is one-shot information without any
query key while the latter is a reply to a query key.  The former
doesn't work well if you wan to get dynamic data.

Also, see that the data value structure is undefined.  The returned
data itself can be nested TLV components, if any.


thanks,

Takashi

Re: RFC: PCM extra attributes

[Jaroslav Kysela]

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.

 					Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: RFC: PCM extra attributes

[Jaroslav Kysela]

On Fri, 19 Jun 2009, Takashi Iwai wrote:

> At Fri, 19 Jun 2009 12:29:17 +0200 (CEST),
> 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.
>
> Well, you still missed the difference between the control TLV and the
> PCM attr I proposed.  The former is one-shot information without any
> query key while the latter is a reply to a query key.  The former
> doesn't work well if you wan to get dynamic data.

It will work, but in some cases it's not prefered, of course. I'm just 
trying to identify common parts of API.

 						Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: RFC: PCM extra attributes

[Takashi Iwai]

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.

The current control TLV works because it's only for dB information.
If we were to give more different data types such as volatile data, it
would have been a nightmare.  The PCM attributes can be such a
dynamic one.  For example the channel map can be changed differently
when you change runtime->channels or formats.  "One-for-all" really
doesn't server well even for such a practical case...


thanks,

Takashi

Re: RFC: PCM extra attributes

[Jaroslav Kysela]

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;

 						Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: RFC: PCM extra attributes

[James Courtier-Dutton]

2009/6/19 Takashi Iwai <tiwai@suse.de>:
> 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,
>

How is the association done between the pcm and the control?
I would have thought an easy approach could be (from userland):
snd_pcm_open( snd_pcm_t **handle, ...);
snd_pcm_get_attr( handle, int attribute_id, *attributeX );
snd_pcm_set_attr( handle, int attribute_id, *attributeX );

I.e Use the snd_pcm_t handle from the snd_pcm_open call to query and
set any controls associated with the pcm. We could then remove those
controls from being viewable in alsamixer and leave it to the
application to control them.
The only controls left in alsamixer would then be the global controls.
E.g. Speaker arrangement, master gain control etc.
The result being that applications would never need to access the
alsamixer controls, and instead only need to use the
snd_pcm_get/set_attr interface on a per PCM basis that is much more in
line with what applications actually need.

>An important note is that I'm planning to use this framework for
>getting/setting the PCM (surround) channel mapping.
Why would we want this channel mapping info in user space?
Can't we just standardize on a single channel mapping as seen from
user space, and get the driver to do any adaption if needed?

Kind Regards

James

Re: RFC: PCM extra attributes

[Takashi Iwai]

At Fri, 19 Jun 2009 12:58:21 +0100,
James Courtier-Dutton wrote:
> 
> 2009/6/19 Takashi Iwai <tiwai@suse.de>:
> > 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,
> >
> 
> How is the association done between the pcm and the control?
> I would have thought an easy approach could be (from userland):
> snd_pcm_open( snd_pcm_t **handle, ...);
> snd_pcm_get_attr( handle, int attribute_id, *attributeX );
> snd_pcm_set_attr( handle, int attribute_id, *attributeX );

The ioctls aren't visible explicitly on alsa-lib.  What I initially
thought of is the function like:

  int snd_pcm_get_assoc_ctls(snd_pcm_t *, int num_ids, snd_ctl_id_t *ids);

> I.e Use the snd_pcm_t handle from the snd_pcm_open call to query and
> set any controls associated with the pcm. We could then remove those
> controls from being viewable in alsamixer and leave it to the
> application to control them.

The alsamixer view isn't the issue I'm concerned.
These spontaneous controls should be IFACE_PCM in the first place.
They shouldn't be IFACE_MIXER.  For example, IEC958 status bits can be
associated to a dedicated PCM substream.

One question is the visible volume control such as VIA DXS Volume.
But, I feel we should move them also to IFACE_PCM space and hide from
the alsamixer view.

> The only controls left in alsamixer would then be the global controls.
> E.g. Speaker arrangement, master gain control etc.

There will be still many strange controls that don't belong to
specific PCM substreams :)

> The result being that applications would never need to access the
> alsamixer controls, and instead only need to use the
> snd_pcm_get/set_attr interface on a per PCM basis that is much more in
> line with what applications actually need.
> 
> >An important note is that I'm planning to use this framework for
> >getting/setting the PCM (surround) channel mapping.
> Why would we want this channel mapping info in user space?

Because we don't know what channel to assign to which slot.
Whether the third channel is a center or a rear-left?

> Can't we just standardize on a single channel mapping as seen from
> user space, and get the driver to do any adaption if needed?

We assume the standard channel map, but unfortunately many hardwares
don't follow and are unable to remap.  Thus currently it's done in
alsa-lib route plugin.

However, there are devices (e.g. HD-audio) that require different
channel maps for different streams depending on chip.  Since the
alsa-lib config is static, it can't be changed well on the fly.
Thus, the driver needs first to provide some information about the
channel map so that alsa-lib (or PA) can handle the remapping
appropriately.

In addition, there are demands from apps to use different chanenl
maps.  FL/FR/C/LFE/RL/RR is far more standard except for ALSA.  We are
definitely minor.  So, sometimes it's good to adapt the majority's
choice...


thanks,

Takashi

Re: 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

Re: RFC: PCM extra attributes

[James Courtier-Dutton]

2009/6/19 Takashi Iwai <tiwai@suse.de>:
>
> The alsamixer view isn't the issue I'm concerned.
> These spontaneous controls should be IFACE_PCM in the first place.
> They shouldn't be IFACE_MIXER.  For example, IEC958 status bits can be
> associated to a dedicated PCM substream.
>
Would an option could be to stream the IEC958 status bits with the PCM stream?
I.e. As an extra channel of the PCM multichannel stream.
Another option could be to stream the gain control values with the PCM stream.
I.e. Instead of attributes attached to the PCM, stream those
attributes with the PCM stream.

Re: RFC: PCM extra attributes

[Takashi Iwai]

At Fri, 19 Jun 2009 13:43:08 +0100,
James Courtier-Dutton wrote:
> 
> 2009/6/19 Takashi Iwai <tiwai@suse.de>:
> >
> > The alsamixer view isn't the issue I'm concerned.
> > These spontaneous controls should be IFACE_PCM in the first place.
> > They shouldn't be IFACE_MIXER.  For example, IEC958 status bits can be
> > associated to a dedicated PCM substream.
> >
> Would an option could be to stream the IEC958 status bits with the PCM stream?
> I.e. As an extra channel of the PCM multichannel stream.

I don't buy it because the status bits don't need resend at each time.
And for compatibility reason, this is a huge gap.

If any, we have already IEC958 raw subframe support that embeds the
status bits...

> Another option could be to stream the gain control values with the PCM stream.
> I.e. Instead of attributes attached to the PCM, stream those
> attributes with the PCM stream.

Hrm how can it be done?  Could you elaborate?


Takashi

Re: RFC: PCM extra attributes

[Jaroslav Kysela]

On Fri, 19 Jun 2009, Takashi Iwai wrote:

> 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?

The tlv_read ioctl will be something like this (simplified without 
proper length and allocation handling):

 	tlv_types.type = TLV_TYPES;
 	tlv_callback(tlv_types);
 	idx = 0;
 	while (tlv_types[0] && (tlv_types[0] & (1 << idx)) != 0) {
 		tlv.type = idx;
 		tlv_callback(tlv);
 		merge_tlv(result, tlv);
 		tlv_types[0] &= ~(1 << idx);
 		idx++;
 	}

> 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?

I'm talking about to have both "all-in-one" and single value ioctls. I see 
your optimization when one value is required to read multiple times. I 
just prefer to make things similar to the control interface (although 
single value read is not implemented in current control API, but it might 
be added later, if required). I would like to see also common naming in 
interfaces like:

SNDRV_PCM_IOCTL_TLV_READ	# read all
SNDRV_PCM_IOCTL_TLV_COMMAND	# write one or more TLVs (COMPOUND type)
SNDRV_PCM_IOCTL_TLV_TREAD	# read only one TLV specified by type
 				# TREAD might be also READONE or so..

 						Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: RFC: PCM extra attributes

[Takashi Iwai]

At Fri, 19 Jun 2009 18:58:46 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> On Fri, 19 Jun 2009, Takashi Iwai wrote:
> 
> > 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?
> 
> The tlv_read ioctl will be something like this (simplified without 
> proper length and allocation handling):
> 
>  	tlv_types.type = TLV_TYPES;
>  	tlv_callback(tlv_types);
>  	idx = 0;
>  	while (tlv_types[0] && (tlv_types[0] & (1 << idx)) != 0) {
>  		tlv.type = idx;
>  		tlv_callback(tlv);
>  		merge_tlv(result, tlv);
>  		tlv_types[0] &= ~(1 << idx);
>  		idx++;
>  	}
>
> > 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?
> 
> I'm talking about to have both "all-in-one" and single value ioctls. I see 
> your optimization when one value is required to read multiple times. I 
> just prefer to make things similar to the control interface (although 
> single value read is not implemented in current control API, but it might 
> be added later, if required).

The single read *is* required.  That's why I don't want "all-in-one"
TLV at all because it makes thing more complicated in such a case.

> I would like to see also common naming in 
> interfaces like:
> 
> SNDRV_PCM_IOCTL_TLV_READ	# read all
> SNDRV_PCM_IOCTL_TLV_COMMAND	# write one or more TLVs (COMPOUND type)
> SNDRV_PCM_IOCTL_TLV_TREAD	# read only one TLV specified by type
>  				# TREAD might be also READONE or so..

We can forget about the compatibility with the control API first.
The benefit comes only if the all-in-one TLV API fits better than
other simpler ones.

The advantage of all-in-one TLV is when the app wants to know
*everything*, like a number 42.  But, is such a situation really
often?  No, mostly you want to know a certain information.

Or, think from a different direction: what's wrong when the driver
accepts a special query key "GIVE_ME_ALL" that returns the all-in-one
TLV?  Then we don't need three calls but just two.


thanks,

Takashi

Re: RFC: PCM extra attributes

[James Courtier-Dutton]

2009/6/19 Takashi Iwai <tiwai@suse.de>:
>> Another option could be to stream the gain control values with the PCM stream.
>> I.e. Instead of attributes attached to the PCM, stream those
>> attributes with the PCM stream.
>
> Hrm how can it be done?  Could you elaborate?
>
You said that you were adding a method to identify the channel maps.
I.e. Is Front the first of second channel in the stream.
Add extra channels in the stream that can be tagged as non-audio data
and also tag the sort of non audio data they are. They could be gain
values, time stamps, etc.
So, basically just an extra way to capture metadata on a per sample basis.
For example, when capturing this sample, the analog input gain was set to X.
Obviously we don't have the functionality in the driver itself yet,
but making sure the api could permit this would be useful.

Re: RFC: PCM extra attributes

[Takashi Iwai]

At Wed, 24 Jun 2009 18:18:48 +0100,
James Courtier-Dutton wrote:
> 
> 2009/6/19 Takashi Iwai <tiwai@suse.de>:
> >> Another option could be to stream the gain control values with the PCM stream.
> >> I.e. Instead of attributes attached to the PCM, stream those
> >> attributes with the PCM stream.
> >
> > Hrm how can it be done?  Could you elaborate?
> >
> You said that you were adding a method to identify the channel maps.
> I.e. Is Front the first of second channel in the stream.
> Add extra channels in the stream that can be tagged as non-audio data
> and also tag the sort of non audio data they are. They could be gain
> values, time stamps, etc.
> So, basically just an extra way to capture metadata on a per sample basis.
> For example, when capturing this sample, the analog input gain was set to X.
> Obviously we don't have the functionality in the driver itself yet,
> but making sure the api could permit this would be useful.

I've discussed with V4L guys about a similar idea to sending
timestamps together with the PCM stream.  The concern was how to 
handle it, who multiplexes and who decodes.  Anyway, they weren't
impressed much by this.

This kind of thing is indeed technically interesting, but there aren't
so many real use cases.  It reminds me the fact that SPDIF raw format
is rarely used :)  But, timestamping would be a good candidate
requiring this kind of extension.


thanks,

Takashi