[RFC] ALSA: add new alsa control byte extended

[RFC] ALSA: add new alsa control byte extended

[Vinod Koul]

As discussed in the last Audio uConf we need to improve byte controls to allow
larger size data to be sent to DSPs  The modern DSPs require alsa byte controls
size which far exceeds the today 512bytes limit. In order for usermode to send
larger sizes (few 100s of KBs) along with size information we add extended byte
control interface which sends any size bytes parameter buffer for DSPs to use
Obviosly the size must be supported by the device and would be required to
inform the max size allowed for the control.

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 include/uapi/sound/asound.h |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 9fc6219..8c10359 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -782,7 +782,8 @@ typedef int __bitwise snd_ctl_elem_type_t;
 #define	SNDRV_CTL_ELEM_TYPE_BYTES	((__force snd_ctl_elem_type_t) 4) /* byte array */
 #define	SNDRV_CTL_ELEM_TYPE_IEC958	((__force snd_ctl_elem_type_t) 5) /* IEC958 (S/PDIF) setup */
 #define	SNDRV_CTL_ELEM_TYPE_INTEGER64	((__force snd_ctl_elem_type_t) 6) /* 64-bit integer type */
-#define	SNDRV_CTL_ELEM_TYPE_LAST	SNDRV_CTL_ELEM_TYPE_INTEGER64
+#define SNDRV_CTL_ELEM_TYPE_BYTES_EXT	((__force snd_ctl_elem_type_t) 7) /* bytes array extended */
+#define	SNDRV_CTL_ELEM_TYPE_LAST	SNDRV_CTL_ELEM_TYPE_BYTES_EXT
 
 typedef int __bitwise snd_ctl_elem_iface_t;
 #define	SNDRV_CTL_ELEM_IFACE_CARD	((__force snd_ctl_elem_iface_t) 0) /* global control */
@@ -891,6 +892,11 @@ struct snd_ctl_elem_value {
 			unsigned char data[512];
 			unsigned char *data_ptr;	/* obsoleted */
 		} bytes;
+		struct {
+			unsigned long size;	/* size of buffer */
+			__u64 data;	i	/* user data pointer */
+		} bytes_ext;
+
 		struct snd_aes_iec958 iec958;
 	} value;		/* RO */
 	struct timespec tstamp;
-- 
1.7.0.4

Re: [RFC] ALSA: add new alsa control byte extended

[Vinod Koul]

On Fri, Nov 29, 2013 at 09:27:21AM +0200, Jarkko Nikula wrote:
> Hi
> 
> On 11/29/2013 06:29 AM, Vinod Koul wrote:
> >@@ -891,6 +892,11 @@ struct snd_ctl_elem_value {
> >  			unsigned char data[512];
> >  			unsigned char *data_ptr;	/* obsoleted */
> >  		} bytes;
> >+		struct {
> >+			unsigned long size;	/* size of buffer */
> >+			__u64 data;	i	/* user data pointer */
> >
> Typo here? (__u64 data;    i)
yup :( I aligned the comment in patch and looks like this got added. Thanks for
pointing...

--
~Vinod

Re: [RFC] ALSA: add new alsa control byte extended

[Jarkko Nikula]

Hi

On 11/29/2013 06:29 AM, Vinod Koul wrote:
> @@ -891,6 +892,11 @@ struct snd_ctl_elem_value {
>   			unsigned char data[512];
>   			unsigned char *data_ptr;	/* obsoleted */
>   		} bytes;
> +		struct {
> +			unsigned long size;	/* size of buffer */
> +			__u64 data;	i	/* user data pointer */
>
Typo here? (__u64 data;    i)

-- 
Jarkko

Re: [RFC] ALSA: add new alsa control byte extended

[Vinod Koul]

On Fri, Nov 29, 2013 at 08:46:03AM +0100, Takashi Iwai wrote:
> At Fri, 29 Nov 2013 09:59:57 +0530,
> Vinod Koul wrote:
> > 
> > As discussed in the last Audio uConf we need to improve byte controls to allow
> > larger size data to be sent to DSPs  The modern DSPs require alsa byte controls
> > size which far exceeds the today 512bytes limit. In order for usermode to send
> > larger sizes (few 100s of KBs) along with size information we add extended byte
> > control interface which sends any size bytes parameter buffer for DSPs to use
> > Obviosly the size must be supported by the device and would be required to
> > inform the max size allowed for the control.
> 
> My primary question is -- must this be a control element?
I think yes. With controls we have an easy way to send parameter bytes and a
good support from both kernel and usermode, so leveraging that would make sense.
Mostly here the limit of 512 is hitting folks and IMO arbitrarily increasing
size doesn't help. For DSPs the algorithm coefficients can be larger 

Here i was thinking of just adding a new API in libraries to access this new
control type so that users can send large parameter blobs.

mixer_ctl_set_bytes_ext(struct mixer_ctl *ctl, const void *data, size_t len)
{
	struct snd_ctl_elem_value ev;

	/*usual init code */

	switch (ctl->info->type) {
	case SNDRV_CTL_ELEM_TYPE_BYTES_EXT:
		ev.value.bytes_ext.size = len;
		ev.value.bytes_ext.data = (__u64)data;
	break;
	default:
		return -EINVAL;
	}
	return ioctl(ctl->mixer->fd, SNDRV_CTL_IOCTL_ELEM_WRITE, &ev);
}

in kernel-side, the .put handler for kcontrol implemented in drivers would then
copy the userdata and send it to DSPs.

So in that respect this becomes quite simple to implement without adding lots of
parallel code and uses the infrastruture we already have.

> Why it can't be implemented in hwdep, for example?  In the past, we
> already implemented DSP firmware loader in the hwdep side.  It's a bit
> complicated (and limited) for a few driver usages, but it was more or
> less standardized for them.
> 
> My concern extending control element is that it'd need the rewrite in
> the whole control API in user-space side, too.  alsactl accesses the
> whole control elements and save/restore them, for example.
I am not sure I got your comment on rewriting control API. I though the union
here makes the ABI itself not to change so existing implementations which don't
use this new control type will not be impacted and for newer DSP we can add new
APIs like above to take benifit.

For save-restore part (though i havent looked at alsactl so might be wrong here)
yes, for this new control type, we would have to allocate buffers and then
destroy on restore. Eventually we will need to do so if we need alsactl suppport
for such devices. But shouldnt the change be less intrusive as it would be
limited to one control type.

> 
> Also, sharing scalar data and a reference pointer in the same union is
> a bit error-prone.
Can you elobrate a bit here and if we go down this path then how do we go about
it

One orthogonal question: why do we have bunch of deprecated pointers in the
unions and is anyone still using it? I guess this would be bit of history lesson
:)

--
~Vinod
> 
> 
> Takashi
> 
> > 
> > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > ---
> >  include/uapi/sound/asound.h |    8 +++++++-
> >  1 files changed, 7 insertions(+), 1 deletions(-)
> > 
> > diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> > index 9fc6219..8c10359 100644
> > --- a/include/uapi/sound/asound.h
> > +++ b/include/uapi/sound/asound.h
> > @@ -782,7 +782,8 @@ typedef int __bitwise snd_ctl_elem_type_t;
> >  #define	SNDRV_CTL_ELEM_TYPE_BYTES	((__force snd_ctl_elem_type_t) 4) /* byte array */
> >  #define	SNDRV_CTL_ELEM_TYPE_IEC958	((__force snd_ctl_elem_type_t) 5) /* IEC958 (S/PDIF) setup */
> >  #define	SNDRV_CTL_ELEM_TYPE_INTEGER64	((__force snd_ctl_elem_type_t) 6) /* 64-bit integer type */
> > -#define	SNDRV_CTL_ELEM_TYPE_LAST	SNDRV_CTL_ELEM_TYPE_INTEGER64
> > +#define SNDRV_CTL_ELEM_TYPE_BYTES_EXT	((__force snd_ctl_elem_type_t) 7) /* bytes array extended */
> > +#define	SNDRV_CTL_ELEM_TYPE_LAST	SNDRV_CTL_ELEM_TYPE_BYTES_EXT
> >  
> >  typedef int __bitwise snd_ctl_elem_iface_t;
> >  #define	SNDRV_CTL_ELEM_IFACE_CARD	((__force snd_ctl_elem_iface_t) 0) /* global control */
> > @@ -891,6 +892,11 @@ struct snd_ctl_elem_value {
> >  			unsigned char data[512];
> >  			unsigned char *data_ptr;	/* obsoleted */
> >  		} bytes;
> > +		struct {
> > +			unsigned long size;	/* size of buffer */
> > +			__u64 data;	i	/* user data pointer */
> > +		} bytes_ext;
> > +
> >  		struct snd_aes_iec958 iec958;
> >  	} value;		/* RO */
> >  	struct timespec tstamp;
> > -- 
> > 1.7.0.4
> > 

-- 

Re: [RFC] ALSA: add new alsa control byte extended

[Takashi Iwai]

At Fri, 29 Nov 2013 09:59:57 +0530,
Vinod Koul wrote:
> 
> As discussed in the last Audio uConf we need to improve byte controls to allow
> larger size data to be sent to DSPs  The modern DSPs require alsa byte controls
> size which far exceeds the today 512bytes limit. In order for usermode to send
> larger sizes (few 100s of KBs) along with size information we add extended byte
> control interface which sends any size bytes parameter buffer for DSPs to use
> Obviosly the size must be supported by the device and would be required to
> inform the max size allowed for the control.

My primary question is -- must this be a control element?
Why it can't be implemented in hwdep, for example?  In the past, we
already implemented DSP firmware loader in the hwdep side.  It's a bit
complicated (and limited) for a few driver usages, but it was more or
less standardized for them.

My concern extending control element is that it'd need the rewrite in
the whole control API in user-space side, too.  alsactl accesses the
whole control elements and save/restore them, for example.

Also, sharing scalar data and a reference pointer in the same union is
a bit error-prone.


Takashi

> 
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
>  include/uapi/sound/asound.h |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 9fc6219..8c10359 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -782,7 +782,8 @@ typedef int __bitwise snd_ctl_elem_type_t;
>  #define	SNDRV_CTL_ELEM_TYPE_BYTES	((__force snd_ctl_elem_type_t) 4) /* byte array */
>  #define	SNDRV_CTL_ELEM_TYPE_IEC958	((__force snd_ctl_elem_type_t) 5) /* IEC958 (S/PDIF) setup */
>  #define	SNDRV_CTL_ELEM_TYPE_INTEGER64	((__force snd_ctl_elem_type_t) 6) /* 64-bit integer type */
> -#define	SNDRV_CTL_ELEM_TYPE_LAST	SNDRV_CTL_ELEM_TYPE_INTEGER64
> +#define SNDRV_CTL_ELEM_TYPE_BYTES_EXT	((__force snd_ctl_elem_type_t) 7) /* bytes array extended */
> +#define	SNDRV_CTL_ELEM_TYPE_LAST	SNDRV_CTL_ELEM_TYPE_BYTES_EXT
>  
>  typedef int __bitwise snd_ctl_elem_iface_t;
>  #define	SNDRV_CTL_ELEM_IFACE_CARD	((__force snd_ctl_elem_iface_t) 0) /* global control */
> @@ -891,6 +892,11 @@ struct snd_ctl_elem_value {
>  			unsigned char data[512];
>  			unsigned char *data_ptr;	/* obsoleted */
>  		} bytes;
> +		struct {
> +			unsigned long size;	/* size of buffer */
> +			__u64 data;	i	/* user data pointer */
> +		} bytes_ext;
> +
>  		struct snd_aes_iec958 iec958;
>  	} value;		/* RO */
>  	struct timespec tstamp;
> -- 
> 1.7.0.4
> 

Re: [RFC] ALSA: add new alsa control byte extended

[Vinod Koul]

On Fri, Nov 29, 2013 at 10:08:06AM +0100, Takashi Iwai wrote:
> At Fri, 29 Nov 2013 13:14:59 +0530,
> Vinod Koul wrote:
> > 
> > On Fri, Nov 29, 2013 at 08:46:03AM +0100, Takashi Iwai wrote:
> > > At Fri, 29 Nov 2013 09:59:57 +0530,
> > > Vinod Koul wrote:
> > > > 
> > > > As discussed in the last Audio uConf we need to improve byte controls to allow
> > > > larger size data to be sent to DSPs  The modern DSPs require alsa byte controls
> > > > size which far exceeds the today 512bytes limit. In order for usermode to send
> > > > larger sizes (few 100s of KBs) along with size information we add extended byte
> > > > control interface which sends any size bytes parameter buffer for DSPs to use
> > > > Obviosly the size must be supported by the device and would be required to
> > > > inform the max size allowed for the control.
> > > 
> > > My primary question is -- must this be a control element?
> > I think yes. With controls we have an easy way to send parameter bytes and a
> > good support from both kernel and usermode, so leveraging that would make sense.
> 
> But it means that we have to extend / fix allover places using the
> control elements.  And the variable size isn't handled there, so far.
> It's the biggest concern.
> 
> For example, how would you read such a control element?  Currently,
> all the control element values have the same static size, so the data
> is just there to read.  OTOH, if you read data with an unknown size,
> you have to query the size at first, allocate the buffer, then read
> the data.  It's a completely different flow.
> 
> > Mostly here the limit of 512 is hitting folks and IMO arbitrarily increasing
> > size doesn't help. For DSPs the algorithm coefficients can be larger 
> 
> Well, it's basically a kind of abuse of control elements, IMO...

well we can go off and do another API for parameters by adding a new
ioctl and then add support in current usermode code for this new interface.
Your concerns and compatiblity questions are certainly valid, i was hoping that
we could still manage to use control interface and keep reuse of existing
infrastructure.

Btw would it be posssible that we still ensumerate the "parameter" element as
control but use different read-write mechanism for variable size data. I am
thinking No, but if think something is doable here then it would be good.

--
~Vinod



> 
> > Here i was thinking of just adding a new API in libraries to access this new
> > control type so that users can send large parameter blobs.
> > 
> > mixer_ctl_set_bytes_ext(struct mixer_ctl *ctl, const void *data, size_t len)
> > {
> > 	struct snd_ctl_elem_value ev;
> > 
> > 	/*usual init code */
> > 
> > 	switch (ctl->info->type) {
> > 	case SNDRV_CTL_ELEM_TYPE_BYTES_EXT:
> > 		ev.value.bytes_ext.size = len;
> > 		ev.value.bytes_ext.data = (__u64)data;
> > 	break;
> > 	default:
> > 		return -EINVAL;
> > 	}
> > 	return ioctl(ctl->mixer->fd, SNDRV_CTL_IOCTL_ELEM_WRITE, &ev);
> > }
> > 
> > in kernel-side, the .put handler for kcontrol implemented in drivers would then
> > copy the userdata and send it to DSPs.
> 
> If you create a new API function, why we need to stick with the
> control elements?
> 
> > So in that respect this becomes quite simple to implement without adding lots of
> > parallel code and uses the infrastruture we already have.
> > 
> > > Why it can't be implemented in hwdep, for example?  In the past, we
> > > already implemented DSP firmware loader in the hwdep side.  It's a bit
> > > complicated (and limited) for a few driver usages, but it was more or
> > > less standardized for them.
> > > 
> > > My concern extending control element is that it'd need the rewrite in
> > > the whole control API in user-space side, too.  alsactl accesses the
> > > whole control elements and save/restore them, for example.
> > I am not sure I got your comment on rewriting control API. I though the union
> > here makes the ABI itself not to change so existing implementations which don't
> > use this new control type will not be impacted and for newer DSP we can add new
> > APIs like above to take benifit.
> 
> You'll change SNDRV_CTL_ELEM_TYPE_LAST.  It means you'll need to look
> over all codes that refer to this and fix them.  (Not only in
> alsa-lib.)  That is, it's difficult to know how an old app reacts when
> it received an unknown data type.
> 
> I just don't want to go the way "let's break and wait to see whether
> people start whining".  At least for the old rock solid stuff.
> 
> > For save-restore part (though i havent looked at alsactl so might be wrong here)
> > yes, for this new control type, we would have to allocate buffers and then
> > destroy on restore. Eventually we will need to do so if we need alsactl suppport
> > for such devices. But shouldnt the change be less intrusive as it would be
> > limited to one control type.
> 
> So, you'd suggest to do save/restore unlimited size of data via
> alsactl?
> 
> > > Also, sharing scalar data and a reference pointer in the same union is
> > > a bit error-prone.
> > Can you elobrate a bit here and if we go down this path then how do we go about
> > it
> 
> Currently the data are stored in the fixed size of union.  You can
> read the all data there.  If a program sets some wrong type or value
> in the data, nothing severe would happen.  Just a wrong data is
> transferred.  But, with a reference pointer, a wrong data triggers the
> segfault, crashes the whole program.
> 
> > One orthogonal question: why do we have bunch of deprecated pointers in the
> > unions and is anyone still using it? I guess this would be bit of history lesson
> > :)
> 
> No it's never used in reality, and this was a big pain in the kernel
> side because we had to convert the data for the 32/64bit compatible
> ioctls.  That's why it was deprecated.  The data field remains there
> just for ABI compatibility reason.
> 
> 
> Takashi

-- 

Re: [RFC] ALSA: add new alsa control byte extended

[Takashi Iwai]

At Fri, 29 Nov 2013 13:14:59 +0530,
Vinod Koul wrote:
> 
> On Fri, Nov 29, 2013 at 08:46:03AM +0100, Takashi Iwai wrote:
> > At Fri, 29 Nov 2013 09:59:57 +0530,
> > Vinod Koul wrote:
> > > 
> > > As discussed in the last Audio uConf we need to improve byte controls to allow
> > > larger size data to be sent to DSPs  The modern DSPs require alsa byte controls
> > > size which far exceeds the today 512bytes limit. In order for usermode to send
> > > larger sizes (few 100s of KBs) along with size information we add extended byte
> > > control interface which sends any size bytes parameter buffer for DSPs to use
> > > Obviosly the size must be supported by the device and would be required to
> > > inform the max size allowed for the control.
> > 
> > My primary question is -- must this be a control element?
> I think yes. With controls we have an easy way to send parameter bytes and a
> good support from both kernel and usermode, so leveraging that would make sense.

But it means that we have to extend / fix allover places using the
control elements.  And the variable size isn't handled there, so far.
It's the biggest concern.

For example, how would you read such a control element?  Currently,
all the control element values have the same static size, so the data
is just there to read.  OTOH, if you read data with an unknown size,
you have to query the size at first, allocate the buffer, then read
the data.  It's a completely different flow.

> Mostly here the limit of 512 is hitting folks and IMO arbitrarily increasing
> size doesn't help. For DSPs the algorithm coefficients can be larger 

Well, it's basically a kind of abuse of control elements, IMO...

> Here i was thinking of just adding a new API in libraries to access this new
> control type so that users can send large parameter blobs.
> 
> mixer_ctl_set_bytes_ext(struct mixer_ctl *ctl, const void *data, size_t len)
> {
> 	struct snd_ctl_elem_value ev;
> 
> 	/*usual init code */
> 
> 	switch (ctl->info->type) {
> 	case SNDRV_CTL_ELEM_TYPE_BYTES_EXT:
> 		ev.value.bytes_ext.size = len;
> 		ev.value.bytes_ext.data = (__u64)data;
> 	break;
> 	default:
> 		return -EINVAL;
> 	}
> 	return ioctl(ctl->mixer->fd, SNDRV_CTL_IOCTL_ELEM_WRITE, &ev);
> }
> 
> in kernel-side, the .put handler for kcontrol implemented in drivers would then
> copy the userdata and send it to DSPs.

If you create a new API function, why we need to stick with the
control elements?

> So in that respect this becomes quite simple to implement without adding lots of
> parallel code and uses the infrastruture we already have.
> 
> > Why it can't be implemented in hwdep, for example?  In the past, we
> > already implemented DSP firmware loader in the hwdep side.  It's a bit
> > complicated (and limited) for a few driver usages, but it was more or
> > less standardized for them.
> > 
> > My concern extending control element is that it'd need the rewrite in
> > the whole control API in user-space side, too.  alsactl accesses the
> > whole control elements and save/restore them, for example.
> I am not sure I got your comment on rewriting control API. I though the union
> here makes the ABI itself not to change so existing implementations which don't
> use this new control type will not be impacted and for newer DSP we can add new
> APIs like above to take benifit.

You'll change SNDRV_CTL_ELEM_TYPE_LAST.  It means you'll need to look
over all codes that refer to this and fix them.  (Not only in
alsa-lib.)  That is, it's difficult to know how an old app reacts when
it received an unknown data type.

I just don't want to go the way "let's break and wait to see whether
people start whining".  At least for the old rock solid stuff.

> For save-restore part (though i havent looked at alsactl so might be wrong here)
> yes, for this new control type, we would have to allocate buffers and then
> destroy on restore. Eventually we will need to do so if we need alsactl suppport
> for such devices. But shouldnt the change be less intrusive as it would be
> limited to one control type.

So, you'd suggest to do save/restore unlimited size of data via
alsactl?

> > Also, sharing scalar data and a reference pointer in the same union is
> > a bit error-prone.
> Can you elobrate a bit here and if we go down this path then how do we go about
> it

Currently the data are stored in the fixed size of union.  You can
read the all data there.  If a program sets some wrong type or value
in the data, nothing severe would happen.  Just a wrong data is
transferred.  But, with a reference pointer, a wrong data triggers the
segfault, crashes the whole program.

> One orthogonal question: why do we have bunch of deprecated pointers in the
> unions and is anyone still using it? I guess this would be bit of history lesson
> :)

No it's never used in reality, and this was a big pain in the kernel
side because we had to convert the data for the 32/64bit compatible
ioctls.  That's why it was deprecated.  The data field remains there
just for ABI compatibility reason.


Takashi

Re: [RFC] ALSA: add new alsa control byte extended

[Jaroslav Kysela]

Date 29.11.2013 10:08, Takashi Iwai wrote:
> At Fri, 29 Nov 2013 13:14:59 +0530,
> Vinod Koul wrote:
>>
>> On Fri, Nov 29, 2013 at 08:46:03AM +0100, Takashi Iwai wrote:
>>> At Fri, 29 Nov 2013 09:59:57 +0530,
>>> Vinod Koul wrote:
>>>>
>>>> As discussed in the last Audio uConf we need to improve byte controls to allow
>>>> larger size data to be sent to DSPs  The modern DSPs require alsa byte controls
>>>> size which far exceeds the today 512bytes limit. In order for usermode to send
>>>> larger sizes (few 100s of KBs) along with size information we add extended byte
>>>> control interface which sends any size bytes parameter buffer for DSPs to use
>>>> Obviosly the size must be supported by the device and would be required to
>>>> inform the max size allowed for the control.
>>>
>>> My primary question is -- must this be a control element?
>> I think yes. With controls we have an easy way to send parameter bytes and a
>> good support from both kernel and usermode, so leveraging that would make sense.
> 
> But it means that we have to extend / fix allover places using the
> control elements.  And the variable size isn't handled there, so far.
> It's the biggest concern.
> 
> For example, how would you read such a control element?  Currently,
> all the control element values have the same static size, so the data
> is just there to read.  OTOH, if you read data with an unknown size,
> you have to query the size at first, allocate the buffer, then read
> the data.  It's a completely different flow.
> 
>> Mostly here the limit of 512 is hitting folks and IMO arbitrarily increasing
>> size doesn't help. For DSPs the algorithm coefficients can be larger 
> 
> Well, it's basically a kind of abuse of control elements, IMO...

I basically agree, but... I believe that these chunks can be divided to
the 512 limit using continuous indexes (kcontrol->count) and a simple
rule in the driver "write all to a DSP when the last control (index) is
touched" may be enough. No API extensions are required. The question is:
Do you rellay need 100+KB for coefficients? Do you expect to handle
these data in standard tools like alsactl?

>> Here i was thinking of just adding a new API in libraries to access this new
>> control type so that users can send large parameter blobs.
>>
>> mixer_ctl_set_bytes_ext(struct mixer_ctl *ctl, const void *data, size_t len)
>> {
>> 	struct snd_ctl_elem_value ev;
>>
>> 	/*usual init code */
>>
>> 	switch (ctl->info->type) {
>> 	case SNDRV_CTL_ELEM_TYPE_BYTES_EXT:
>> 		ev.value.bytes_ext.size = len;
>> 		ev.value.bytes_ext.data = (__u64)data;
>> 	break;
>> 	default:
>> 		return -EINVAL;
>> 	}
>> 	return ioctl(ctl->mixer->fd, SNDRV_CTL_IOCTL_ELEM_WRITE, &ev);
>> }
>>
>> in kernel-side, the .put handler for kcontrol implemented in drivers would then
>> copy the userdata and send it to DSPs.
> 
> If you create a new API function, why we need to stick with the
> control elements?
> 
>> So in that respect this becomes quite simple to implement without adding lots of
>> parallel code and uses the infrastruture we already have.
>>
>>> Why it can't be implemented in hwdep, for example?  In the past, we
>>> already implemented DSP firmware loader in the hwdep side.  It's a bit
>>> complicated (and limited) for a few driver usages, but it was more or
>>> less standardized for them.
>>>
>>> My concern extending control element is that it'd need the rewrite in
>>> the whole control API in user-space side, too.  alsactl accesses the
>>> whole control elements and save/restore them, for example.
>> I am not sure I got your comment on rewriting control API. I though the union
>> here makes the ABI itself not to change so existing implementations which don't
>> use this new control type will not be impacted and for newer DSP we can add new
>> APIs like above to take benifit.
> 
> You'll change SNDRV_CTL_ELEM_TYPE_LAST.  It means you'll need to look
> over all codes that refer to this and fix them.  (Not only in
> alsa-lib.)  That is, it's difficult to know how an old app reacts when
> it received an unknown data type.
> 
> I just don't want to go the way "let's break and wait to see whether
> people start whining".  At least for the old rock solid stuff.
> 
>> For save-restore part (though i havent looked at alsactl so might be wrong here)
>> yes, for this new control type, we would have to allocate buffers and then
>> destroy on restore. Eventually we will need to do so if we need alsactl suppport
>> for such devices. But shouldnt the change be less intrusive as it would be
>> limited to one control type.
> 
> So, you'd suggest to do save/restore unlimited size of data via
> alsactl?
> 
>>> Also, sharing scalar data and a reference pointer in the same union is
>>> a bit error-prone.
>> Can you elobrate a bit here and if we go down this path then how do we go about
>> it
> 
> Currently the data are stored in the fixed size of union.  You can
> read the all data there.  If a program sets some wrong type or value
> in the data, nothing severe would happen.  Just a wrong data is
> transferred.  But, with a reference pointer, a wrong data triggers the
> segfault, crashes the whole program.
> 
>> One orthogonal question: why do we have bunch of deprecated pointers in the
>> unions and is anyone still using it? I guess this would be bit of history lesson
>> :)
> 
> No it's never used in reality, and this was a big pain in the kernel
> side because we had to convert the data for the 32/64bit compatible
> ioctls.  That's why it was deprecated.  The data field remains there
> just for ABI compatibility reason.

I and Abramo though it may be useful, but until 2008 no one required
these big data so you removed this without complains.. But ... the room
for this extension is available or it may be implemented using another
WRITE_LARGE_BYTES ioctl to satisfy the 32/64 bit conversions.

					Jaroslav

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

Re: [RFC] ALSA: add new alsa control byte extended

[Takashi Iwai]

At Fri, 29 Nov 2013 10:40:51 +0100,
Jaroslav Kysela wrote:
> 
> Date 29.11.2013 10:08, Takashi Iwai wrote:
> > At Fri, 29 Nov 2013 13:14:59 +0530,
> > Vinod Koul wrote:
> >>
> >> On Fri, Nov 29, 2013 at 08:46:03AM +0100, Takashi Iwai wrote:
> >>> At Fri, 29 Nov 2013 09:59:57 +0530,
> >>> Vinod Koul wrote:
> >>>>
> >>>> As discussed in the last Audio uConf we need to improve byte controls to allow
> >>>> larger size data to be sent to DSPs  The modern DSPs require alsa byte controls
> >>>> size which far exceeds the today 512bytes limit. In order for usermode to send
> >>>> larger sizes (few 100s of KBs) along with size information we add extended byte
> >>>> control interface which sends any size bytes parameter buffer for DSPs to use
> >>>> Obviosly the size must be supported by the device and would be required to
> >>>> inform the max size allowed for the control.
> >>>
> >>> My primary question is -- must this be a control element?
> >> I think yes. With controls we have an easy way to send parameter bytes and a
> >> good support from both kernel and usermode, so leveraging that would make sense.
> > 
> > But it means that we have to extend / fix allover places using the
> > control elements.  And the variable size isn't handled there, so far.
> > It's the biggest concern.
> > 
> > For example, how would you read such a control element?  Currently,
> > all the control element values have the same static size, so the data
> > is just there to read.  OTOH, if you read data with an unknown size,
> > you have to query the size at first, allocate the buffer, then read
> > the data.  It's a completely different flow.
> > 
> >> Mostly here the limit of 512 is hitting folks and IMO arbitrarily increasing
> >> size doesn't help. For DSPs the algorithm coefficients can be larger 
> > 
> > Well, it's basically a kind of abuse of control elements, IMO...
> 
> I basically agree, but... I believe that these chunks can be divided to
> the 512 limit using continuous indexes (kcontrol->count) and a simple
> rule in the driver "write all to a DSP when the last control (index) is
> touched" may be enough. No API extensions are required. The question is:
> Do you rellay need 100+KB for coefficients? Do you expect to handle
> these data in standard tools like alsactl?

Yeah, I suggested a similar workaround when the issue came up ago
(IIRC, it was about WM codecs).  But it's certainly ugly, indeed.


Takashi

Re: [RFC] ALSA: add new alsa control byte extended

[Jaroslav Kysela]

Date 29.11.2013 10:48, Takashi Iwai wrote:
> At Fri, 29 Nov 2013 10:40:51 +0100,
> Jaroslav Kysela wrote:
>>
>> Date 29.11.2013 10:08, Takashi Iwai wrote:
>>> At Fri, 29 Nov 2013 13:14:59 +0530,
>>> Vinod Koul wrote:
>>>>
>>>> On Fri, Nov 29, 2013 at 08:46:03AM +0100, Takashi Iwai wrote:
>>>>> At Fri, 29 Nov 2013 09:59:57 +0530,
>>>>> Vinod Koul wrote:
>>>>>>
>>>>>> As discussed in the last Audio uConf we need to improve byte controls to allow
>>>>>> larger size data to be sent to DSPs  The modern DSPs require alsa byte controls
>>>>>> size which far exceeds the today 512bytes limit. In order for usermode to send
>>>>>> larger sizes (few 100s of KBs) along with size information we add extended byte
>>>>>> control interface which sends any size bytes parameter buffer for DSPs to use
>>>>>> Obviosly the size must be supported by the device and would be required to
>>>>>> inform the max size allowed for the control.
>>>>>
>>>>> My primary question is -- must this be a control element?
>>>> I think yes. With controls we have an easy way to send parameter bytes and a
>>>> good support from both kernel and usermode, so leveraging that would make sense.
>>>
>>> But it means that we have to extend / fix allover places using the
>>> control elements.  And the variable size isn't handled there, so far.
>>> It's the biggest concern.
>>>
>>> For example, how would you read such a control element?  Currently,
>>> all the control element values have the same static size, so the data
>>> is just there to read.  OTOH, if you read data with an unknown size,
>>> you have to query the size at first, allocate the buffer, then read
>>> the data.  It's a completely different flow.
>>>
>>>> Mostly here the limit of 512 is hitting folks and IMO arbitrarily increasing
>>>> size doesn't help. For DSPs the algorithm coefficients can be larger 
>>>
>>> Well, it's basically a kind of abuse of control elements, IMO...
>>
>> I basically agree, but... I believe that these chunks can be divided to
>> the 512 limit using continuous indexes (kcontrol->count) and a simple
>> rule in the driver "write all to a DSP when the last control (index) is
>> touched" may be enough. No API extensions are required. The question is:
>> Do you rellay need 100+KB for coefficients? Do you expect to handle
>> these data in standard tools like alsactl?
> 
> Yeah, I suggested a similar workaround when the issue came up ago
> (IIRC, it was about WM codecs).  But it's certainly ugly, indeed.

I wouldn't mark it as ugly, because the overhead is small in this
solution (except the allocated space for the whole message in the hw
driver). But if these large data are static and they are not expected to
be saved (alsactl), the control API may be extended using a new ioctl or
the hwdep interface with some custom tools may be used.

					Jaroslav

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

Re: [RFC] ALSA: add new alsa control byte extended

[Vinod Koul]

[-- Attachment #1.1: Type: text/plain, Size: 1601 bytes --]

On Fri, Nov 29, 2013 at 11:05:49AM +0000, Mark Brown wrote:
> On Fri, Nov 29, 2013 at 10:48:29AM +0100, Takashi Iwai wrote:
> > Jaroslav Kysela wrote:
> 
> > > I basically agree, but... I believe that these chunks can be divided to
> > > the 512 limit using continuous indexes (kcontrol->count) and a simple
> > > rule in the driver "write all to a DSP when the last control (index) is
> > > touched" may be enough. No API extensions are required. The question is:
> > > Do you rellay need 100+KB for coefficients? Do you expect to handle
> > > these data in standard tools like alsactl?
> 
> It's certianly possible to do something like that while maintianing the
> ABI, however if we were going to do that we'd probably want to extend
> alsa-lib and tinyalsa to do this transparently and devise a naming
> scheme for the controls to trigger that behaviour.  The main thing is
> the API offered to users.
I think this would a bit problematic for DSPs with large controls. We are
looking at 3 digit number already and splitting to multiple calls is going to be
bad from a already constrained latency problem.

Most of the controls will be few KBs at most with few special cases which can be
in MBs. I think Wolfson is also headed this way!

I am leading more towards adding new ioctl for this along with new ones for
enumerating controls. Then additional support for alsa-lib and tinyalsa.
That way existing tinymix, amxier can see these as controls while not disturbing
existing apps. I dont think we need save and restor, then alsactl need not be
modified.

--
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC] ALSA: add new alsa control byte extended

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1138 bytes --]

On Fri, Nov 29, 2013 at 10:48:29AM +0100, Takashi Iwai wrote:
> Jaroslav Kysela wrote:

> > I basically agree, but... I believe that these chunks can be divided to
> > the 512 limit using continuous indexes (kcontrol->count) and a simple
> > rule in the driver "write all to a DSP when the last control (index) is
> > touched" may be enough. No API extensions are required. The question is:
> > Do you rellay need 100+KB for coefficients? Do you expect to handle
> > these data in standard tools like alsactl?

It's certianly possible to do something like that while maintianing the
ABI, however if we were going to do that we'd probably want to extend
alsa-lib and tinyalsa to do this transparently and devise a naming
scheme for the controls to trigger that behaviour.  The main thing is
the API offered to users.

> Yeah, I suggested a similar workaround when the issue came up ago
> (IIRC, it was about WM codecs).  But it's certainly ugly, indeed.

No, that was originally for Qualcomm - they were wanting to download
megabytes of data.  There were some patches from Wolfson more recently
though they're for much smaller data sizes.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC] ALSA: add new alsa control byte extended

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 599 bytes --]

On Fri, Nov 29, 2013 at 11:10:43AM +0100, Jaroslav Kysela wrote:

> driver). But if these large data are static and they are not expected to
> be saved (alsactl), the control API may be extended using a new ioctl or
> the hwdep interface with some custom tools may be used.

I'd expect them to be handleable with standard tools, from a user
perspective these things are just another setting that gets applied when
changing use case like volumes and routing so manipulating the settings
in a separate place seems wrong - at the level where you're applying
them it's all part of the same set of data.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC] ALSA: add new alsa control byte extended

[Takashi Iwai]

At Fri, 29 Nov 2013 11:25:05 +0000,
Mark Brown wrote:
> 
> On Fri, Nov 29, 2013 at 11:10:43AM +0100, Jaroslav Kysela wrote:
> 
> > driver). But if these large data are static and they are not expected to
> > be saved (alsactl), the control API may be extended using a new ioctl or
> > the hwdep interface with some custom tools may be used.
> 
> I'd expect them to be handleable with standard tools, from a user
> perspective these things are just another setting that gets applied when
> changing use case like volumes and routing so manipulating the settings
> in a separate place seems wrong - at the level where you're applying
> them it's all part of the same set of data.

Basically agreed, but OTOH, size matters, too.  What if there are a
few MB data for several different control elements?  I don't think
managing them in the same text alsa.state file is so sensible.


Takashi

Re: [RFC] ALSA: add new alsa control byte extended

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1069 bytes --]

On Fri, Nov 29, 2013 at 03:59:37PM +0530, Vinod Koul wrote:

> I think this would a bit problematic for DSPs with large controls. We are
> looking at 3 digit number already and splitting to multiple calls is going to be
> bad from a already constrained latency problem.

System call overhead on Linux is pretty light, and we can probably
arrange for the implementations to know if the algorithm is running and
start doing the I/O immediately if it's not.

> I am leading more towards adding new ioctl for this along with new ones for
> enumerating controls. Then additional support for alsa-lib and tinyalsa.
> That way existing tinymix, amxier can see these as controls while not disturbing
> existing apps. I dont think we need save and restor, then alsactl need not be
> modified.

I doubt you need it specifically for alsactl since that's a bit basic
for most of the users but it would be good to have the ability to write
tools that allow people to tune interactively and then save the results
in a format that's useful for whatever use case management is in use.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC] ALSA: add new alsa control byte extended

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1035 bytes --]

On Fri, Nov 29, 2013 at 12:31:37PM +0100, Takashi Iwai wrote:
> Mark Brown wrote:

> > I'd expect them to be handleable with standard tools, from a user
> > perspective these things are just another setting that gets applied when
> > changing use case like volumes and routing so manipulating the settings
> > in a separate place seems wrong - at the level where you're applying
> > them it's all part of the same set of data.

> Basically agreed, but OTOH, size matters, too.  What if there are a
> few MB data for several different control elements?  I don't think
> managing them in the same text alsa.state file is so sensible.

That's definitely something people doing tooling would need to consider
- there's also things like only saving changes or splitting the
  configuration up into reusable components rather than having complete
configs for each use case in order to keep the sizes down.  Even without
worrying about coefficients the sizes of the configuration files can get
large very quickly with a naive implementation.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC] ALSA: add new alsa control byte extended

[Charles Keepax]

On Fri, Nov 29, 2013 at 03:59:37PM +0530, Vinod Koul wrote:
> On Fri, Nov 29, 2013 at 11:05:49AM +0000, Mark Brown wrote:
> > On Fri, Nov 29, 2013 at 10:48:29AM +0100, Takashi Iwai wrote:
> > > Jaroslav Kysela wrote:
> > 
> > > > I basically agree, but... I believe that these chunks can be divided to
> > > > the 512 limit using continuous indexes (kcontrol->count) and a simple
> > > > rule in the driver "write all to a DSP when the last control (index) is
> > > > touched" may be enough. No API extensions are required. The question is:
> > > > Do you rellay need 100+KB for coefficients? Do you expect to handle
> > > > these data in standard tools like alsactl?
> > 
> > It's certianly possible to do something like that while maintianing the
> > ABI, however if we were going to do that we'd probably want to extend
> > alsa-lib and tinyalsa to do this transparently and devise a naming
> > scheme for the controls to trigger that behaviour.  The main thing is
> > the API offered to users.
> I think this would a bit problematic for DSPs with large controls. We are
> looking at 3 digit number already and splitting to multiple calls is going to be
> bad from a already constrained latency problem.
> 
> Most of the controls will be few KBs at most with few special cases which can be
> in MBs. I think Wolfson is also headed this way!

We certainly have stuff that transfers in the region of 30kB to
the DSPs for configuration data for some algorithms, and I
certainly wouldn't rule out larger cases in the future. At the
moment this is handled by splitting this into 512 chunks, but the
numbers of controls involved does start to feel like you are
fighting the system a little.

We haven't done a lot of investigation into the latency of this
at this time but it is certainly something that is coming up in
discussions and I would expect we will be starting to worry more
about it in the near future. I will endeavour to share what
results I can as we get into this more.

> 
> I am leading more towards adding new ioctl for this along with new ones for
> enumerating controls. Then additional support for alsa-lib and tinyalsa.
> That way existing tinymix, amxier can see these as controls while not disturbing
> existing apps. I dont think we need save and restor, then alsactl need not be
> modified.
> 
> --
> ~Vinod

It does feel a little odd to have this data set through a
different interface to other existing control data, but I guess
anything we do does have to be very careful around upsetting
existing user-space code.

Thanks,
Charles

Re: [RFC] ALSA: add new alsa control byte extended

[Vinod Koul]

[-- Attachment #1.1: Type: text/plain, Size: 966 bytes --]

On Fri, Nov 29, 2013 at 11:46:03AM +0000, Mark Brown wrote:
> On Fri, Nov 29, 2013 at 03:59:37PM +0530, Vinod Koul wrote:
> > I am leading more towards adding new ioctl for this along with new ones for
> > enumerating controls. Then additional support for alsa-lib and tinyalsa.
> > That way existing tinymix, amxier can see these as controls while not disturbing
> > existing apps. I dont think we need save and restor, then alsactl need not be
> > modified.
> 
> I doubt you need it specifically for alsactl since that's a bit basic
> for most of the users but it would be good to have the ability to write
> tools that allow people to tune interactively and then save the results
> in a format that's useful for whatever use case management is in use.
I agree alsactl may not be required here and if its felt we can always add that!

managing parameters is different problem and yes needs to be dealt sperately
with tooling as well

-- 
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC] ALSA: add new alsa control byte extended

[Vinod Koul]

On Fri, Nov 29, 2013 at 01:11:53PM +0000, Charles Keepax wrote:
> > I am leading more towards adding new ioctl for this along with new ones for
> > enumerating controls. Then additional support for alsa-lib and tinyalsa.
> > That way existing tinymix, amxier can see these as controls while not disturbing
> > existing apps. I dont think we need save and restor, then alsactl need not be
> > modified.
> 
> It does feel a little odd to have this data set through a
> different interface to other existing control data, but I guess
> anything we do does have to be very careful around upsetting
> existing user-space code.
Am thinking users and driver shouldn't feel that it is different. The approach i
am leaning now is adding three ioctls, for enumerating, reading and then
writing. The tinyalsa and alsa-lib should also enumerate these new ones.  But we
don't impact ABI so old users/apps won't change.  Similarly on kernel side we
still treat these as kcontrols and drivers use existing infrastructure to manage
these...

-- 
~Vinod

Re: [RFC] ALSA: add new alsa control byte extended

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 577 bytes --]

On Fri, Nov 29, 2013 at 01:11:53PM +0000, Charles Keepax wrote:

> We haven't done a lot of investigation into the latency of this
> at this time but it is certainly something that is coming up in
> discussions and I would expect we will be starting to worry more
> about it in the near future. I will endeavour to share what
> results I can as we get into this more.

Given that you're working with devices that aren't memory mapped I'd
expect we'd have to be doing something appauling for it to register -
the costs of doing I/O really ought to dwarf anything else for them.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]