* [PATCH] ucm: add cset-tlv command
@ 2016-03-22 9:10 Hsin-Yu Chao
2016-03-22 11:48 ` Takashi Iwai
0 siblings, 1 reply; 6+ messages in thread
From: Hsin-Yu Chao @ 2016-03-22 9:10 UTC (permalink / raw)
To: alsa-devel; +Cc: vinod.koul, dgreid, Hsin-Yu Chao
This patch enables UCM to set a file in TLV format to kcontrol by:
cset-tlv "name='<kcontrol-name>' <path-to-file>"
This new 'cset-tlv' command will be used to write audio DSP to
specific alsa control, where the driver expectes a file in TLV
format.
Signed-off-by: Hsin-Yu Chao <hychao@chromium.org>
---
src/ucm/main.c | 80 ++++++++++++++++++++++++++++++++++++++++++++---------
src/ucm/parser.c | 10 +++++++
src/ucm/ucm_local.h | 1 +
3 files changed, 78 insertions(+), 13 deletions(-)
diff --git a/src/ucm/main.c b/src/ucm/main.c
index 7e44603..a4ccb65 100644
--- a/src/ucm/main.c
+++ b/src/ucm/main.c
@@ -161,6 +161,47 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr,
return 0;
}
+static int tlv_parse(char **res,
+ snd_ctl_elem_info_t *info,
+ const char *filepath)
+{
+ int err = 0;
+ int fd;
+ struct stat st;
+ size_t sz;
+ ssize_t sz_read;
+
+ if (!snd_ctl_elem_info_is_tlv_writable(info)) {
+ err = -EINVAL;
+ return err;
+ }
+ fd = open(filepath, O_RDONLY);
+ if (fd < 0) {
+ err = -errno;
+ return err;
+ }
+ if (stat(filepath, &st) == -1) {
+ err = -errno;
+ goto __fail;
+ }
+ sz = st.st_size;
+ *res = malloc(sz);
+ if (res == NULL) {
+ err = -ENOMEM;
+ goto __fail;
+ }
+ sz_read = read(fd, *res, sz);
+ if (sz_read < 0 || (size_t)sz_read != sz) {
+ err = -errno;
+ free(*res);
+ *res = NULL;
+ }
+
+ __fail:
+ close(fd);
+ return err;
+}
+
static int binary_file_parse(snd_ctl_elem_value_t *dst,
snd_ctl_elem_info_t *info,
const char *filepath)
@@ -226,6 +267,7 @@ static int execute_cset(snd_ctl_t *ctl, const char *cset, unsigned int type)
snd_ctl_elem_id_t *id;
snd_ctl_elem_value_t *value;
snd_ctl_elem_info_t *info;
+ char *res = NULL;
snd_ctl_elem_id_malloc(&id);
snd_ctl_elem_value_malloc(&value);
@@ -241,23 +283,32 @@ static int execute_cset(snd_ctl_t *ctl, const char *cset, unsigned int type)
err = -EINVAL;
goto __fail;
}
- snd_ctl_elem_value_set_id(value, id);
snd_ctl_elem_info_set_id(info, id);
- err = snd_ctl_elem_read(ctl, value);
- if (err < 0)
- goto __fail;
err = snd_ctl_elem_info(ctl, info);
if (err < 0)
goto __fail;
- if (type == SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE)
- err = binary_file_parse(value, info, pos);
- else
- err = snd_ctl_ascii_value_parse(ctl, value, info, pos);
- if (err < 0)
- goto __fail;
- err = snd_ctl_elem_write(ctl, value);
- if (err < 0)
- goto __fail;
+ if (type == SEQUENCE_ELEMENT_TYPE_CSET_TLV) {
+ err = tlv_parse(&res, info, pos);
+ if (err < 0)
+ goto __fail;
+ err = snd_ctl_elem_tlv_write(ctl, id, (unsigned int *)res);
+ if (err < 0)
+ goto __fail;
+ } else {
+ snd_ctl_elem_value_set_id(value, id);
+ err = snd_ctl_elem_read(ctl, value);
+ if (err < 0)
+ goto __fail;
+ if (type == SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE)
+ err = binary_file_parse(value, info, pos);
+ else
+ err = snd_ctl_ascii_value_parse(ctl, value, info, pos);
+ if (err < 0)
+ goto __fail;
+ err = snd_ctl_elem_write(ctl, value);
+ if (err < 0)
+ goto __fail;
+ }
err = 0;
__fail:
if (id != NULL)
@@ -266,6 +317,8 @@ static int execute_cset(snd_ctl_t *ctl, const char *cset, unsigned int type)
free(value);
if (info != NULL)
free(info);
+ if (res != NULL)
+ free(res);
return err;
}
@@ -298,6 +351,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
break;
case SEQUENCE_ELEMENT_TYPE_CSET:
case SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE:
+ case SEQUENCE_ELEMENT_TYPE_CSET_TLV:
if (cdev == NULL) {
char *playback_ctl = NULL;
char *capture_ctl = NULL;
diff --git a/src/ucm/parser.c b/src/ucm/parser.c
index 9e1cb41..d781e1b 100644
--- a/src/ucm/parser.c
+++ b/src/ucm/parser.c
@@ -316,6 +316,16 @@ static int parse_sequence(snd_use_case_mgr_t *uc_mgr ATTRIBUTE_UNUSED,
continue;
}
+ if (strcmp(cmd, "cset-tlv") == 0) {
+ curr->type = SEQUENCE_ELEMENT_TYPE_CSET_TLV;
+ err = parse_string(n, &curr->data.cset);
+ if (err < 0) {
+ uc_error("error: cset-tlv requires a string!");
+ return err;
+ }
+ continue;
+ }
+
if (strcmp(cmd, "usleep") == 0) {
curr->type = SEQUENCE_ELEMENT_TYPE_SLEEP;
err = snd_config_get_integer(n, &curr->data.sleep);
diff --git a/src/ucm/ucm_local.h b/src/ucm/ucm_local.h
index 3a5d2c2..b89de2a 100644
--- a/src/ucm/ucm_local.h
+++ b/src/ucm/ucm_local.h
@@ -48,6 +48,7 @@
#define SEQUENCE_ELEMENT_TYPE_SLEEP 3
#define SEQUENCE_ELEMENT_TYPE_EXEC 4
#define SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE 5
+#define SEQUENCE_ELEMENT_TYPE_CSET_TLV 6
struct ucm_value {
struct list_head list;
--
2.1.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ucm: add cset-tlv command
2016-03-22 9:10 [PATCH] ucm: add cset-tlv command Hsin-Yu Chao
@ 2016-03-22 11:48 ` Takashi Iwai
2016-03-23 9:15 ` Hsin-yu Chao
0 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2016-03-22 11:48 UTC (permalink / raw)
To: Hsin-Yu Chao; +Cc: vinod.koul, alsa-devel, dgreid
On Tue, 22 Mar 2016 10:10:34 +0100,
Hsin-Yu Chao wrote:
>
> This patch enables UCM to set a file in TLV format to kcontrol by:
> cset-tlv "name='<kcontrol-name>' <path-to-file>"
> This new 'cset-tlv' command will be used to write audio DSP to
> specific alsa control, where the driver expectes a file in TLV
> format.
>
> Signed-off-by: Hsin-Yu Chao <hychao@chromium.org>
One problem in this approach is that the provided TLV data file isn't
portable. Since we deal TLV as int arrays, it's endian-sensitive.
At least, some endian check would be needed.
Some other nitpicks:
> ---
> src/ucm/main.c | 80 ++++++++++++++++++++++++++++++++++++++++++++---------
> src/ucm/parser.c | 10 +++++++
> src/ucm/ucm_local.h | 1 +
> 3 files changed, 78 insertions(+), 13 deletions(-)
>
> diff --git a/src/ucm/main.c b/src/ucm/main.c
> index 7e44603..a4ccb65 100644
> --- a/src/ucm/main.c
> +++ b/src/ucm/main.c
> @@ -161,6 +161,47 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr,
> return 0;
> }
>
> +static int tlv_parse(char **res,
> + snd_ctl_elem_info_t *info,
> + const char *filepath)
The function name is somehow confusing. It appears as if it parses
TLV. And what it actually does are two things:
- check snd_ctl_elem_info_is_tlv_writable(),
and
- allocate the whole TLV file.
IMO, snd_ctl_elem_info_is_tlv_writable() check can be in the caller
side, and this function (rename whatever better) can just do allocate
and read the TLV data into a buffer.
> +{
> + int err = 0;
> + int fd;
> + struct stat st;
> + size_t sz;
> + ssize_t sz_read;
> +
> + if (!snd_ctl_elem_info_is_tlv_writable(info)) {
> + err = -EINVAL;
> + return err;
> + }
> + fd = open(filepath, O_RDONLY);
> + if (fd < 0) {
> + err = -errno;
> + return err;
> + }
> + if (stat(filepath, &st) == -1) {
> + err = -errno;
> + goto __fail;
> + }
> + sz = st.st_size;
> + *res = malloc(sz);
What if a crazy large file was passed? A sanity check of the file
size might be good.
thanks,
Takashi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ucm: add cset-tlv command
2016-03-22 11:48 ` Takashi Iwai
@ 2016-03-23 9:15 ` Hsin-yu Chao
2016-03-23 9:21 ` Takashi Iwai
0 siblings, 1 reply; 6+ messages in thread
From: Hsin-yu Chao @ 2016-03-23 9:15 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Koul, Vinod, alsa-devel, Dylan Reid
On Tue, Mar 22, 2016 at 7:48 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Tue, 22 Mar 2016 10:10:34 +0100,
> Hsin-Yu Chao wrote:
>>
>> This patch enables UCM to set a file in TLV format to kcontrol by:
>> cset-tlv "name='<kcontrol-name>' <path-to-file>"
>> This new 'cset-tlv' command will be used to write audio DSP to
>> specific alsa control, where the driver expectes a file in TLV
>> format.
>>
>> Signed-off-by: Hsin-Yu Chao <hychao@chromium.org>
>
> One problem in this approach is that the provided TLV data file isn't
> portable. Since we deal TLV as int arrays, it's endian-sensitive.
> At least, some endian check would be needed.
Thanks for the review. I think by extracting the length attribute (i.e
tlv[1]) and compare it with the file size is sufficient here, since
there is no way to figure out the endianness of the binary file passed
in.
Also I agree that additional check for crazy large or small file is
necessary. According to the dsp files I test with, I'll set the tlv
file size limit to between 8 bytes and 1MB.
Will upload a v2 patch shortly, thanks!
Hsin-yu
>
> Some other nitpicks:
>
>> ---
>> src/ucm/main.c | 80 ++++++++++++++++++++++++++++++++++++++++++++---------
>> src/ucm/parser.c | 10 +++++++
>> src/ucm/ucm_local.h | 1 +
>> 3 files changed, 78 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/ucm/main.c b/src/ucm/main.c
>> index 7e44603..a4ccb65 100644
>> --- a/src/ucm/main.c
>> +++ b/src/ucm/main.c
>> @@ -161,6 +161,47 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr,
>> return 0;
>> }
>>
>> +static int tlv_parse(char **res,
>> + snd_ctl_elem_info_t *info,
>> + const char *filepath)
>
> The function name is somehow confusing. It appears as if it parses
> TLV. And what it actually does are two things:
> - check snd_ctl_elem_info_is_tlv_writable(),
> and
> - allocate the whole TLV file.
>
> IMO, snd_ctl_elem_info_is_tlv_writable() check can be in the caller
> side, and this function (rename whatever better) can just do allocate
> and read the TLV data into a buffer.
>
>
>> +{
>> + int err = 0;
>> + int fd;
>> + struct stat st;
>> + size_t sz;
>> + ssize_t sz_read;
>> +
>> + if (!snd_ctl_elem_info_is_tlv_writable(info)) {
>> + err = -EINVAL;
>> + return err;
>> + }
>> + fd = open(filepath, O_RDONLY);
>> + if (fd < 0) {
>> + err = -errno;
>> + return err;
>> + }
>> + if (stat(filepath, &st) == -1) {
>> + err = -errno;
>> + goto __fail;
>> + }
>> + sz = st.st_size;
>> + *res = malloc(sz);
>
> What if a crazy large file was passed? A sanity check of the file
> size might be good.
>
>
> thanks,
>
> Takashi
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ucm: add cset-tlv command
2016-03-23 9:15 ` Hsin-yu Chao
@ 2016-03-23 9:21 ` Takashi Iwai
2016-03-23 9:44 ` Hsin-yu Chao
0 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2016-03-23 9:21 UTC (permalink / raw)
To: Hsin-yu Chao; +Cc: Koul, Vinod, alsa-devel, Dylan Reid
On Wed, 23 Mar 2016 10:15:21 +0100,
Hsin-yu Chao wrote:
>
> On Tue, Mar 22, 2016 at 7:48 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > On Tue, 22 Mar 2016 10:10:34 +0100,
> > Hsin-Yu Chao wrote:
> >>
> >> This patch enables UCM to set a file in TLV format to kcontrol by:
> >> cset-tlv "name='<kcontrol-name>' <path-to-file>"
> >> This new 'cset-tlv' command will be used to write audio DSP to
> >> specific alsa control, where the driver expectes a file in TLV
> >> format.
> >>
> >> Signed-off-by: Hsin-Yu Chao <hychao@chromium.org>
> >
> > One problem in this approach is that the provided TLV data file isn't
> > portable. Since we deal TLV as int arrays, it's endian-sensitive.
> > At least, some endian check would be needed.
> Thanks for the review. I think by extracting the length attribute (i.e
> tlv[1]) and compare it with the file size is sufficient here, since
> there is no way to figure out the endianness of the binary file passed
> in.
Yeah, that should be good enough.
> Also I agree that additional check for crazy large or small file is
> necessary. According to the dsp files I test with, I'll set the tlv
> file size limit to between 8 bytes and 1MB.
I guess the size will grow quickly in near future, so it'd be safer to
take a bit bigger.
thanks,
Takashi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ucm: add cset-tlv command
2016-03-23 9:21 ` Takashi Iwai
@ 2016-03-23 9:44 ` Hsin-yu Chao
2016-03-23 16:52 ` Takashi Iwai
0 siblings, 1 reply; 6+ messages in thread
From: Hsin-yu Chao @ 2016-03-23 9:44 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Koul, Vinod, alsa-devel, Dylan Reid
On Wed, Mar 23, 2016 at 5:21 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Wed, 23 Mar 2016 10:15:21 +0100,
> Hsin-yu Chao wrote:
>>
>> On Tue, Mar 22, 2016 at 7:48 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> > On Tue, 22 Mar 2016 10:10:34 +0100,
>> > Hsin-Yu Chao wrote:
>> >>
>> >> This patch enables UCM to set a file in TLV format to kcontrol by:
>> >> cset-tlv "name='<kcontrol-name>' <path-to-file>"
>> >> This new 'cset-tlv' command will be used to write audio DSP to
>> >> specific alsa control, where the driver expectes a file in TLV
>> >> format.
>> >>
>> >> Signed-off-by: Hsin-Yu Chao <hychao@chromium.org>
>> >
>> > One problem in this approach is that the provided TLV data file isn't
>> > portable. Since we deal TLV as int arrays, it's endian-sensitive.
>> > At least, some endian check would be needed.
>> Thanks for the review. I think by extracting the length attribute (i.e
>> tlv[1]) and compare it with the file size is sufficient here, since
>> there is no way to figure out the endianness of the binary file passed
>> in.
>
> Yeah, that should be good enough.
>
>
>> Also I agree that additional check for crazy large or small file is
>> necessary. According to the dsp files I test with, I'll set the tlv
>> file size limit to between 8 bytes and 1MB.
>
> I guess the size will grow quickly in near future, so it'd be safer to
> take a bit bigger.
>
How about using 16MB as upper limit?
DSP file larger than that would take 1 second or more to load through USB 2.0.
Hsin-yu
>
> thanks,
>
> Takashi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ucm: add cset-tlv command
2016-03-23 9:44 ` Hsin-yu Chao
@ 2016-03-23 16:52 ` Takashi Iwai
0 siblings, 0 replies; 6+ messages in thread
From: Takashi Iwai @ 2016-03-23 16:52 UTC (permalink / raw)
To: Hsin-yu Chao; +Cc: Koul, Vinod, alsa-devel, Dylan Reid
On Wed, 23 Mar 2016 10:44:31 +0100,
Hsin-yu Chao wrote:
>
> On Wed, Mar 23, 2016 at 5:21 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > On Wed, 23 Mar 2016 10:15:21 +0100,
> > Hsin-yu Chao wrote:
> >>
> >> On Tue, Mar 22, 2016 at 7:48 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >> > On Tue, 22 Mar 2016 10:10:34 +0100,
> >> > Hsin-Yu Chao wrote:
> >> >>
> >> >> This patch enables UCM to set a file in TLV format to kcontrol by:
> >> >> cset-tlv "name='<kcontrol-name>' <path-to-file>"
> >> >> This new 'cset-tlv' command will be used to write audio DSP to
> >> >> specific alsa control, where the driver expectes a file in TLV
> >> >> format.
> >> >>
> >> >> Signed-off-by: Hsin-Yu Chao <hychao@chromium.org>
> >> >
> >> > One problem in this approach is that the provided TLV data file isn't
> >> > portable. Since we deal TLV as int arrays, it's endian-sensitive.
> >> > At least, some endian check would be needed.
> >> Thanks for the review. I think by extracting the length attribute (i.e
> >> tlv[1]) and compare it with the file size is sufficient here, since
> >> there is no way to figure out the endianness of the binary file passed
> >> in.
> >
> > Yeah, that should be good enough.
> >
> >
> >> Also I agree that additional check for crazy large or small file is
> >> necessary. According to the dsp files I test with, I'll set the tlv
> >> file size limit to between 8 bytes and 1MB.
> >
> > I guess the size will grow quickly in near future, so it'd be safer to
> > take a bit bigger.
> >
> How about using 16MB as upper limit?
> DSP file larger than that would take 1 second or more to load through USB 2.0.
Yes, 16MB should be big enough.
thanks,
Takashi
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-03-23 16:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-22 9:10 [PATCH] ucm: add cset-tlv command Hsin-Yu Chao
2016-03-22 11:48 ` Takashi Iwai
2016-03-23 9:15 ` Hsin-yu Chao
2016-03-23 9:21 ` Takashi Iwai
2016-03-23 9:44 ` Hsin-yu Chao
2016-03-23 16:52 ` Takashi Iwai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.