[PATCH - UCM 1/1] ucm: add binary configure file parse

[PATCH - UCM 1/1] ucm: add binary configure file parse

[han.lu]

From: "Lu, Han" <han.lu@intel.com>

with cset command, UCM set kcontrol parameters directly:
    cset "name='<KCONTROL_NAME>' 1<,2,3,...>"
This patch enables UCM to set kcontrol with parameters from
configure file:
    cset-bin-file "name='<KCONTROL_NAME>' <path/to/file>"
where "cset-bin-file" is a newly added keyword alongside of "cset",
to indicate cset with binary data in file.
The binary data in file is parameter for audio DSPs, and it's just
passed by UCM/ALSA as raw data. The data type of parameter elements
must be byte, and the count must matches driver definition.

Signed-off-by: Lu, Han <han.lu@intel.com>

diff --git a/src/ucm/main.c b/src/ucm/main.c
index 37ae4c8..4c1e4b2 100644
--- a/src/ucm/main.c
+++ b/src/ucm/main.c
@@ -160,11 +160,62 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr,
 	return 0;
 }
 
+static int binary_file_parse(snd_ctl_elem_value_t *dst,
+			      snd_ctl_elem_info_t *info,
+			      const char *filepath)
+{
+	int err, fd;
+	struct stat st;
+	size_t sz;
+	char *res;
+	snd_ctl_elem_type_t type;
+	unsigned int idx, count;
+
+	type = snd_ctl_elem_info_get_type(info);
+	if (type != SND_CTL_ELEM_TYPE_BYTES) {
+		uc_error("only support byte type!");
+		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;
+	count = snd_ctl_elem_info_get_count(info);
+	if (sz != count || sz > sizeof(dst->value.bytes)) {
+		uc_error("invalid parameter size %d!", sz);
+		err = -EINVAL;
+		goto __fail;
+	}
+	res = malloc(sz);
+	if (res == NULL) {
+		err = -ENOMEM;
+		goto __fail;
+	}
+	if (read(fd, res, sz) != sz) {
+		err = -errno;
+		goto __fail_read;
+	}
+	for (idx = 0; idx < sz; idx++)
+		snd_ctl_elem_value_set_byte(dst, idx, *(res + idx));
+      __fail_read:
+	free(res);
+      __fail:
+	close(fd);
+	return err;
+}
+
 extern int __snd_ctl_ascii_elem_id_parse(snd_ctl_elem_id_t *dst,
 					 const char *str,
 					 const char **ret_ptr);
 
-static int execute_cset(snd_ctl_t *ctl, const char *cset)
+static int execute_cset(snd_ctl_t *ctl, const char *cset, unsigned int type)
 {
 	const char *pos;
 	int err;
@@ -194,7 +245,10 @@ static int execute_cset(snd_ctl_t *ctl, const char *cset)
 	err = snd_ctl_elem_info(ctl, info);
 	if (err < 0)
 		goto __fail;
-	err = snd_ctl_ascii_value_parse(ctl, value, info, pos);
+	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);
@@ -239,6 +293,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
 				goto __fail_nomem;
 			break;
 		case SEQUENCE_ELEMENT_TYPE_CSET:
+		case SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE:
 			if (cdev == NULL) {
 				const char *cdev1 = NULL, *cdev2 = NULL;
 				err = get_value3(&cdev1, "PlaybackCTL",
@@ -274,7 +329,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
 					goto __fail;
 				}
 			}
-			err = execute_cset(ctl, s->data.cset);
+			err = execute_cset(ctl, s->data.cset, s->type);
 			if (err < 0) {
 				uc_error("unable to execute cset '%s'\n", s->data.cset);
 				goto __fail;
diff --git a/src/ucm/parser.c b/src/ucm/parser.c
index d7517f6..9e1cb41 100644
--- a/src/ucm/parser.c
+++ b/src/ucm/parser.c
@@ -306,6 +306,16 @@ static int parse_sequence(snd_use_case_mgr_t *uc_mgr ATTRIBUTE_UNUSED,
 			continue;
 		}
 
+		if (strcmp(cmd, "cset-bin-file") == 0) {
+			curr->type = SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE;
+			err = parse_string(n, &curr->data.cset);
+			if (err < 0) {
+				uc_error("error: cset-bin-file 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 87f14a2..c1655c7 100644
--- a/src/ucm/ucm_local.h
+++ b/src/ucm/ucm_local.h
@@ -47,6 +47,7 @@
 #define SEQUENCE_ELEMENT_TYPE_CSET	2
 #define SEQUENCE_ELEMENT_TYPE_SLEEP	3
 #define SEQUENCE_ELEMENT_TYPE_EXEC	4
+#define SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE	5
 
 struct ucm_value {
         struct list_head list;
-- 
2.1.0

Re: [PATCH - UCM 1/1] ucm: add binary configure file parse

[Takashi Iwai]

At Wed, 21 Jan 2015 09:55:16 +0800,
han.lu@intel.com wrote:
> 
> From: "Lu, Han" <han.lu@intel.com>
> 
> with cset command, UCM set kcontrol parameters directly:
>     cset "name='<KCONTROL_NAME>' 1<,2,3,...>"
> This patch enables UCM to set kcontrol with parameters from
> configure file:
>     cset-bin-file "name='<KCONTROL_NAME>' <path/to/file>"
> where "cset-bin-file" is a newly added keyword alongside of "cset",
> to indicate cset with binary data in file.
> The binary data in file is parameter for audio DSPs, and it's just
> passed by UCM/ALSA as raw data. The data type of parameter elements
> must be byte, and the count must matches driver definition.
> 
> Signed-off-by: Lu, Han <han.lu@intel.com>

The contents are almost good, but I got a few compile warnings.

================
main.c: In function 'binary_file_parse':
main.c:185:2: warning: implicit declaration of function 'stat' [-Wimplicit-function-declaration]
  if (stat(filepath, &st) == -1) {
  ^
main.c:201:24: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
  if (read(fd, res, sz) != sz) {
                        ^
main.c: In function 'execute_sequence':
main.c:252:5: warning: 'err' may be used uninitialized in this function [-Wmaybe-uninitialized]
  if (err < 0)
     ^
main.c:285:6: note: 'err' was declared here
  int err = 0;
      ^
================


Takashi

Re: [PATCH - UCM 1/1] ucm: add binary configure file parse

[Lu, Han]

Hi Takashi,

BR,
Han Lu

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Thursday, January 22, 2015 4:01 AM
> To: Lu, Han
> Cc: alsa-devel@alsa-project.org; Girdwood, Liam R
> Subject: Re: [alsa-devel] [PATCH - UCM 1/1] ucm: add binary configure file
> parse
> 
> At Wed, 21 Jan 2015 09:55:16 +0800,
> han.lu@intel.com wrote:
> >
> > From: "Lu, Han" <han.lu@intel.com>
> >
> > with cset command, UCM set kcontrol parameters directly:
> >     cset "name='<KCONTROL_NAME>' 1<,2,3,...>"
> > This patch enables UCM to set kcontrol with parameters from configure
> > file:
> >     cset-bin-file "name='<KCONTROL_NAME>' <path/to/file>"
> > where "cset-bin-file" is a newly added keyword alongside of "cset", to
> > indicate cset with binary data in file.
> > The binary data in file is parameter for audio DSPs, and it's just
> > passed by UCM/ALSA as raw data. The data type of parameter elements
> > must be byte, and the count must matches driver definition.
> >
> > Signed-off-by: Lu, Han <han.lu@intel.com>
> 
> The contents are almost good, but I got a few compile warnings.
> 
> ================
> main.c: In function 'binary_file_parse':
> main.c:185:2: warning: implicit declaration of function 'stat' [-Wimplicit-
> function-declaration]
>   if (stat(filepath, &st) == -1) {
>   ^
> main.c:201:24: warning: comparison between signed and unsigned integer
> expressions [-Wsign-compare]
>   if (read(fd, res, sz) != sz) {
>                         ^
> main.c: In function 'execute_sequence':
> main.c:252:5: warning: 'err' may be used uninitialized in this function [-
> Wmaybe-uninitialized]
>   if (err < 0)
>      ^
> main.c:285:6: note: 'err' was declared here
>   int err = 0;
>       ^
> ================
> 

Sorry, I didn't find the warnings. I have sent the modified patch.
BTW, with -Wall/-Wimplicit-function-declaration, I can find warning 1; and with -Wsign-compare I can find warning 2; but with -Wmaybe-uninitialized I cannot find warning 3. (although I did ignore initialization in binary_file_parse(), not execute_sequence())
My platform is Ubuntu 12.10 with gcc-4.9.1, and for alsa-lib.git I used default compile settings. Could you please share your compile setting if convenient? Thanks.

> 
> Takashi

[PATCH - UCM 1/1] ucm: add binary configure file parse

[han.lu]

From: "Lu, Han" <han.lu@intel.com>

with cset command, UCM set kcontrol parameters directly:
    cset "name='<KCONTROL_NAME>' 1<,2,3,...>"
This patch enables UCM to set kcontrol with parameters from
configure file:
    cset-bin-file "name='<KCONTROL_NAME>' <path/to/file>"
where "cset-bin-file" is a newly added keyword alongside of "cset",
to indicate cset with binary data in file.
The binary data in file is parameter for audio DSPs, and it's just
passed by UCM/ALSA as raw data. The data type of parameter elements
must be byte, and the count must matches driver definition.

Signed-off-by: Lu, Han <han.lu@intel.com>

diff --git a/src/ucm/main.c b/src/ucm/main.c
index 37ae4c8..182f174 100644
--- a/src/ucm/main.c
+++ b/src/ucm/main.c
@@ -34,6 +34,7 @@
 #include <ctype.h>
 #include <stdarg.h>
 #include <pthread.h>
+#include <sys/stat.h>
 
 /*
  * misc
@@ -160,11 +161,65 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr,
 	return 0;
 }
 
+static int binary_file_parse(snd_ctl_elem_value_t *dst,
+			      snd_ctl_elem_info_t *info,
+			      const char *filepath)
+{
+	int err = 0;
+	int fd;
+	struct stat st;
+	size_t sz;
+	ssize_t sz_read;
+	char *res;
+	snd_ctl_elem_type_t type;
+	unsigned int idx, count;
+
+	type = snd_ctl_elem_info_get_type(info);
+	if (type != SND_CTL_ELEM_TYPE_BYTES) {
+		uc_error("only support byte type!");
+		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;
+	count = snd_ctl_elem_info_get_count(info);
+	if (sz != count || sz > sizeof(dst->value.bytes)) {
+		uc_error("invalid parameter size %d!", sz);
+		err = -EINVAL;
+		goto __fail;
+	}
+	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;
+		goto __fail_read;
+	}
+	for (idx = 0; idx < sz; idx++)
+		snd_ctl_elem_value_set_byte(dst, idx, *(res + idx));
+      __fail_read:
+	free(res);
+      __fail:
+	close(fd);
+	return err;
+}
+
 extern int __snd_ctl_ascii_elem_id_parse(snd_ctl_elem_id_t *dst,
 					 const char *str,
 					 const char **ret_ptr);
 
-static int execute_cset(snd_ctl_t *ctl, const char *cset)
+static int execute_cset(snd_ctl_t *ctl, const char *cset, unsigned int type)
 {
 	const char *pos;
 	int err;
@@ -194,7 +249,10 @@ static int execute_cset(snd_ctl_t *ctl, const char *cset)
 	err = snd_ctl_elem_info(ctl, info);
 	if (err < 0)
 		goto __fail;
-	err = snd_ctl_ascii_value_parse(ctl, value, info, pos);
+	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);
@@ -239,6 +297,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
 				goto __fail_nomem;
 			break;
 		case SEQUENCE_ELEMENT_TYPE_CSET:
+		case SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE:
 			if (cdev == NULL) {
 				const char *cdev1 = NULL, *cdev2 = NULL;
 				err = get_value3(&cdev1, "PlaybackCTL",
@@ -274,7 +333,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
 					goto __fail;
 				}
 			}
-			err = execute_cset(ctl, s->data.cset);
+			err = execute_cset(ctl, s->data.cset, s->type);
 			if (err < 0) {
 				uc_error("unable to execute cset '%s'\n", s->data.cset);
 				goto __fail;
diff --git a/src/ucm/parser.c b/src/ucm/parser.c
index d7517f6..9e1cb41 100644
--- a/src/ucm/parser.c
+++ b/src/ucm/parser.c
@@ -306,6 +306,16 @@ static int parse_sequence(snd_use_case_mgr_t *uc_mgr ATTRIBUTE_UNUSED,
 			continue;
 		}
 
+		if (strcmp(cmd, "cset-bin-file") == 0) {
+			curr->type = SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE;
+			err = parse_string(n, &curr->data.cset);
+			if (err < 0) {
+				uc_error("error: cset-bin-file 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 87f14a2..c1655c7 100644
--- a/src/ucm/ucm_local.h
+++ b/src/ucm/ucm_local.h
@@ -47,6 +47,7 @@
 #define SEQUENCE_ELEMENT_TYPE_CSET	2
 #define SEQUENCE_ELEMENT_TYPE_SLEEP	3
 #define SEQUENCE_ELEMENT_TYPE_EXEC	4
+#define SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE	5
 
 struct ucm_value {
         struct list_head list;
-- 
2.1.0

Re: [PATCH - UCM 1/1] ucm: add binary configure file parse

[Takashi Iwai]

At Thu, 22 Jan 2015 09:32:47 +0800,
han.lu@intel.com wrote:
> 
> From: "Lu, Han" <han.lu@intel.com>
> 
> with cset command, UCM set kcontrol parameters directly:
>     cset "name='<KCONTROL_NAME>' 1<,2,3,...>"
> This patch enables UCM to set kcontrol with parameters from
> configure file:
>     cset-bin-file "name='<KCONTROL_NAME>' <path/to/file>"
> where "cset-bin-file" is a newly added keyword alongside of "cset",
> to indicate cset with binary data in file.
> The binary data in file is parameter for audio DSPs, and it's just
> passed by UCM/ALSA as raw data. The data type of parameter elements
> must be byte, and the count must matches driver definition.
> 
> Signed-off-by: Lu, Han <han.lu@intel.com>

Applied, thanks.


Takashi

> 
> diff --git a/src/ucm/main.c b/src/ucm/main.c
> index 37ae4c8..182f174 100644
> --- a/src/ucm/main.c
> +++ b/src/ucm/main.c
> @@ -34,6 +34,7 @@
>  #include <ctype.h>
>  #include <stdarg.h>
>  #include <pthread.h>
> +#include <sys/stat.h>
>  
>  /*
>   * misc
> @@ -160,11 +161,65 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr,
>  	return 0;
>  }
>  
> +static int binary_file_parse(snd_ctl_elem_value_t *dst,
> +			      snd_ctl_elem_info_t *info,
> +			      const char *filepath)
> +{
> +	int err = 0;
> +	int fd;
> +	struct stat st;
> +	size_t sz;
> +	ssize_t sz_read;
> +	char *res;
> +	snd_ctl_elem_type_t type;
> +	unsigned int idx, count;
> +
> +	type = snd_ctl_elem_info_get_type(info);
> +	if (type != SND_CTL_ELEM_TYPE_BYTES) {
> +		uc_error("only support byte type!");
> +		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;
> +	count = snd_ctl_elem_info_get_count(info);
> +	if (sz != count || sz > sizeof(dst->value.bytes)) {
> +		uc_error("invalid parameter size %d!", sz);
> +		err = -EINVAL;
> +		goto __fail;
> +	}
> +	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;
> +		goto __fail_read;
> +	}
> +	for (idx = 0; idx < sz; idx++)
> +		snd_ctl_elem_value_set_byte(dst, idx, *(res + idx));
> +      __fail_read:
> +	free(res);
> +      __fail:
> +	close(fd);
> +	return err;
> +}
> +
>  extern int __snd_ctl_ascii_elem_id_parse(snd_ctl_elem_id_t *dst,
>  					 const char *str,
>  					 const char **ret_ptr);
>  
> -static int execute_cset(snd_ctl_t *ctl, const char *cset)
> +static int execute_cset(snd_ctl_t *ctl, const char *cset, unsigned int type)
>  {
>  	const char *pos;
>  	int err;
> @@ -194,7 +249,10 @@ static int execute_cset(snd_ctl_t *ctl, const char *cset)
>  	err = snd_ctl_elem_info(ctl, info);
>  	if (err < 0)
>  		goto __fail;
> -	err = snd_ctl_ascii_value_parse(ctl, value, info, pos);
> +	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);
> @@ -239,6 +297,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
>  				goto __fail_nomem;
>  			break;
>  		case SEQUENCE_ELEMENT_TYPE_CSET:
> +		case SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE:
>  			if (cdev == NULL) {
>  				const char *cdev1 = NULL, *cdev2 = NULL;
>  				err = get_value3(&cdev1, "PlaybackCTL",
> @@ -274,7 +333,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
>  					goto __fail;
>  				}
>  			}
> -			err = execute_cset(ctl, s->data.cset);
> +			err = execute_cset(ctl, s->data.cset, s->type);
>  			if (err < 0) {
>  				uc_error("unable to execute cset '%s'\n", s->data.cset);
>  				goto __fail;
> diff --git a/src/ucm/parser.c b/src/ucm/parser.c
> index d7517f6..9e1cb41 100644
> --- a/src/ucm/parser.c
> +++ b/src/ucm/parser.c
> @@ -306,6 +306,16 @@ static int parse_sequence(snd_use_case_mgr_t *uc_mgr ATTRIBUTE_UNUSED,
>  			continue;
>  		}
>  
> +		if (strcmp(cmd, "cset-bin-file") == 0) {
> +			curr->type = SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE;
> +			err = parse_string(n, &curr->data.cset);
> +			if (err < 0) {
> +				uc_error("error: cset-bin-file 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 87f14a2..c1655c7 100644
> --- a/src/ucm/ucm_local.h
> +++ b/src/ucm/ucm_local.h
> @@ -47,6 +47,7 @@
>  #define SEQUENCE_ELEMENT_TYPE_CSET	2
>  #define SEQUENCE_ELEMENT_TYPE_SLEEP	3
>  #define SEQUENCE_ELEMENT_TYPE_EXEC	4
> +#define SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE	5
>  
>  struct ucm_value {
>          struct list_head list;
> -- 
> 2.1.0
> 

[PATCH - UCM 1/1] ucm: add binary configure file parse

[han.lu]

From: "Lu, Han" <han.lu@intel.com>

with cset command, UCM set kcontrol parameters directly:
    cset "name='<KCONTROL_NAME>' 1<,2,3,...>"
This patch enables UCM to set kcontrol with parameters from
configure file:
    cset-bin-file "name='<KCONTROL_NAME>' <path/to/file>"
where "cset-bin-file" is a newly added keyword alongside of "cset",
to indicate cset with binary data in file.
The binary data in file is parameter for audio DSPs, and it's just
passed by UCM/ALSA as raw data. The data type of the parameter
element must be byte, and the count is limited by the size of
struct snd_ctl_elem_value and driver definition.

Signed-off-by: Lu, Han <han.lu@intel.com>

diff --git a/src/ucm/main.c b/src/ucm/main.c
index 37ae4c8..818465a 100644
--- a/src/ucm/main.c
+++ b/src/ucm/main.c
@@ -160,11 +160,62 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr,
 	return 0;
 }
 
+static int binary_file_parse(snd_ctl_elem_value_t *dst,
+			      snd_ctl_elem_info_t *info,
+			      const char *filepath)
+{
+	int err, fd;
+	struct stat st;
+	size_t sz;
+	char *res;
+	snd_ctl_elem_type_t type;
+	unsigned int idx, count;
+
+	type = snd_ctl_elem_info_get_type(info);
+	if (type != SND_CTL_ELEM_TYPE_BYTES) {
+		uc_error("only support byte type!");
+		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;
+	count = snd_ctl_elem_info_get_count(info);
+	if (sz == 0 || sz > count || sz > sizeof(dst->value.bytes)) {
+		uc_error("invalid parameter size!");
+		err = -EINVAL;
+		goto __fail;
+	}
+	res = calloc(sz, 1);
+	if (res == NULL) {
+		err = -ENOMEM;
+		goto __fail;
+	}
+	if (read(fd, res, sz) != sz) {
+		err = -errno;
+		goto __fail_read;
+	}
+	for (idx = 0; idx < sz; idx++)
+		snd_ctl_elem_value_set_byte(dst, idx, *(res + idx));
+      __fail_read:
+	free(res);
+      __fail:
+	close(fd);
+	return err;
+}
+
 extern int __snd_ctl_ascii_elem_id_parse(snd_ctl_elem_id_t *dst,
 					 const char *str,
 					 const char **ret_ptr);
 
-static int execute_cset(snd_ctl_t *ctl, const char *cset)
+static int execute_cset(snd_ctl_t *ctl, const char *cset, unsigned int type)
 {
 	const char *pos;
 	int err;
@@ -194,7 +245,10 @@ static int execute_cset(snd_ctl_t *ctl, const char *cset)
 	err = snd_ctl_elem_info(ctl, info);
 	if (err < 0)
 		goto __fail;
-	err = snd_ctl_ascii_value_parse(ctl, value, info, pos);
+	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);
@@ -239,6 +293,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
 				goto __fail_nomem;
 			break;
 		case SEQUENCE_ELEMENT_TYPE_CSET:
+		case SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE:
 			if (cdev == NULL) {
 				const char *cdev1 = NULL, *cdev2 = NULL;
 				err = get_value3(&cdev1, "PlaybackCTL",
@@ -274,7 +329,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
 					goto __fail;
 				}
 			}
-			err = execute_cset(ctl, s->data.cset);
+			err = execute_cset(ctl, s->data.cset, s->type);
 			if (err < 0) {
 				uc_error("unable to execute cset '%s'\n", s->data.cset);
 				goto __fail;
diff --git a/src/ucm/parser.c b/src/ucm/parser.c
index d7517f6..9e1cb41 100644
--- a/src/ucm/parser.c
+++ b/src/ucm/parser.c
@@ -306,6 +306,16 @@ static int parse_sequence(snd_use_case_mgr_t *uc_mgr ATTRIBUTE_UNUSED,
 			continue;
 		}
 
+		if (strcmp(cmd, "cset-bin-file") == 0) {
+			curr->type = SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE;
+			err = parse_string(n, &curr->data.cset);
+			if (err < 0) {
+				uc_error("error: cset-bin-file 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 87f14a2..c1655c7 100644
--- a/src/ucm/ucm_local.h
+++ b/src/ucm/ucm_local.h
@@ -47,6 +47,7 @@
 #define SEQUENCE_ELEMENT_TYPE_CSET	2
 #define SEQUENCE_ELEMENT_TYPE_SLEEP	3
 #define SEQUENCE_ELEMENT_TYPE_EXEC	4
+#define SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE	5
 
 struct ucm_value {
         struct list_head list;
-- 
2.1.0

Re: [PATCH - UCM 1/1] ucm: add binary configure file parse

[Takashi Iwai]

At Tue, 20 Jan 2015 16:33:09 +0800,
han.lu@intel.com wrote:
> 
> From: "Lu, Han" <han.lu@intel.com>
> 
> with cset command, UCM set kcontrol parameters directly:
>     cset "name='<KCONTROL_NAME>' 1<,2,3,...>"
> This patch enables UCM to set kcontrol with parameters from
> configure file:
>     cset-bin-file "name='<KCONTROL_NAME>' <path/to/file>"
> where "cset-bin-file" is a newly added keyword alongside of "cset",
> to indicate cset with binary data in file.
> The binary data in file is parameter for audio DSPs, and it's just
> passed by UCM/ALSA as raw data. The data type of the parameter
> element must be byte, and the count is limited by the size of
> struct snd_ctl_elem_value and driver definition.
> 
> Signed-off-by: Lu, Han <han.lu@intel.com>
> 
> diff --git a/src/ucm/main.c b/src/ucm/main.c
> index 37ae4c8..818465a 100644
> --- a/src/ucm/main.c
> +++ b/src/ucm/main.c
> @@ -160,11 +160,62 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr,
>  	return 0;
>  }
>  
> +static int binary_file_parse(snd_ctl_elem_value_t *dst,
> +			      snd_ctl_elem_info_t *info,
> +			      const char *filepath)
> +{
> +	int err, fd;
> +	struct stat st;
> +	size_t sz;
> +	char *res;
> +	snd_ctl_elem_type_t type;
> +	unsigned int idx, count;
> +
> +	type = snd_ctl_elem_info_get_type(info);
> +	if (type != SND_CTL_ELEM_TYPE_BYTES) {
> +		uc_error("only support byte type!");
> +		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;
> +	count = snd_ctl_elem_info_get_count(info);
> +	if (sz == 0 || sz > count || sz > sizeof(dst->value.bytes)) {
> +		uc_error("invalid parameter size!");

A question here is what if the file size is smaller than the expected
data size.  Should we allow this explicitly?  I thought it'd be safer
to bail out, but I'd like to know more about the scenarios behind this
patch.

> +		err = -EINVAL;
> +		goto __fail;
> +	}
> +	res = calloc(sz, 1);

This can be malloc() as we'll overwrite the whole data in anyway.


thanks,

Takashi

> +	if (res == NULL) {
> +		err = -ENOMEM;
> +		goto __fail;
> +	}
> +	if (read(fd, res, sz) != sz) {
> +		err = -errno;
> +		goto __fail_read;
> +	}
> +	for (idx = 0; idx < sz; idx++)
> +		snd_ctl_elem_value_set_byte(dst, idx, *(res + idx));
> +      __fail_read:
> +	free(res);
> +      __fail:
> +	close(fd);
> +	return err;
> +}
> +
>  extern int __snd_ctl_ascii_elem_id_parse(snd_ctl_elem_id_t *dst,
>  					 const char *str,
>  					 const char **ret_ptr);
>  
> -static int execute_cset(snd_ctl_t *ctl, const char *cset)
> +static int execute_cset(snd_ctl_t *ctl, const char *cset, unsigned int type)
>  {
>  	const char *pos;
>  	int err;
> @@ -194,7 +245,10 @@ static int execute_cset(snd_ctl_t *ctl, const char *cset)
>  	err = snd_ctl_elem_info(ctl, info);
>  	if (err < 0)
>  		goto __fail;
> -	err = snd_ctl_ascii_value_parse(ctl, value, info, pos);
> +	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);
> @@ -239,6 +293,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
>  				goto __fail_nomem;
>  			break;
>  		case SEQUENCE_ELEMENT_TYPE_CSET:
> +		case SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE:
>  			if (cdev == NULL) {
>  				const char *cdev1 = NULL, *cdev2 = NULL;
>  				err = get_value3(&cdev1, "PlaybackCTL",
> @@ -274,7 +329,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
>  					goto __fail;
>  				}
>  			}
> -			err = execute_cset(ctl, s->data.cset);
> +			err = execute_cset(ctl, s->data.cset, s->type);
>  			if (err < 0) {
>  				uc_error("unable to execute cset '%s'\n", s->data.cset);
>  				goto __fail;
> diff --git a/src/ucm/parser.c b/src/ucm/parser.c
> index d7517f6..9e1cb41 100644
> --- a/src/ucm/parser.c
> +++ b/src/ucm/parser.c
> @@ -306,6 +306,16 @@ static int parse_sequence(snd_use_case_mgr_t *uc_mgr ATTRIBUTE_UNUSED,
>  			continue;
>  		}
>  
> +		if (strcmp(cmd, "cset-bin-file") == 0) {
> +			curr->type = SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE;
> +			err = parse_string(n, &curr->data.cset);
> +			if (err < 0) {
> +				uc_error("error: cset-bin-file 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 87f14a2..c1655c7 100644
> --- a/src/ucm/ucm_local.h
> +++ b/src/ucm/ucm_local.h
> @@ -47,6 +47,7 @@
>  #define SEQUENCE_ELEMENT_TYPE_CSET	2
>  #define SEQUENCE_ELEMENT_TYPE_SLEEP	3
>  #define SEQUENCE_ELEMENT_TYPE_EXEC	4
> +#define SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE	5
>  
>  struct ucm_value {
>          struct list_head list;
> -- 
> 2.1.0
> 

Re: [PATCH - UCM 1/1] ucm: add binary configure file parse

[Lu, Han]

Hi Takashi,

BR,
Han Lu

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Tuesday, January 20, 2015 5:19 PM
> To: Lu, Han
> Cc: Girdwood, Liam R; alsa-devel@alsa-project.org
> Subject: Re: [PATCH - UCM 1/1] ucm: add binary configure file parse
> 
> At Tue, 20 Jan 2015 16:33:09 +0800,
> han.lu@intel.com wrote:
> >
> > From: "Lu, Han" <han.lu@intel.com>
> >
> > with cset command, UCM set kcontrol parameters directly:
> >     cset "name='<KCONTROL_NAME>' 1<,2,3,...>"
> > This patch enables UCM to set kcontrol with parameters from configure
> > file:
> >     cset-bin-file "name='<KCONTROL_NAME>' <path/to/file>"
> > where "cset-bin-file" is a newly added keyword alongside of "cset", to
> > indicate cset with binary data in file.
> > The binary data in file is parameter for audio DSPs, and it's just
> > passed by UCM/ALSA as raw data. The data type of the parameter element
> > must be byte, and the count is limited by the size of struct
> > snd_ctl_elem_value and driver definition.
> >
> > Signed-off-by: Lu, Han <han.lu@intel.com>
> >
> > diff --git a/src/ucm/main.c b/src/ucm/main.c index 37ae4c8..818465a
> > 100644
> > --- a/src/ucm/main.c
> > +++ b/src/ucm/main.c
> > @@ -160,11 +160,62 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr,
> >  	return 0;
> >  }
> >
> > +static int binary_file_parse(snd_ctl_elem_value_t *dst,
> > +			      snd_ctl_elem_info_t *info,
> > +			      const char *filepath)
> > +{
> > +	int err, fd;
> > +	struct stat st;
> > +	size_t sz;
> > +	char *res;
> > +	snd_ctl_elem_type_t type;
> > +	unsigned int idx, count;
> > +
> > +	type = snd_ctl_elem_info_get_type(info);
> > +	if (type != SND_CTL_ELEM_TYPE_BYTES) {
> > +		uc_error("only support byte type!");
> > +		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;
> > +	count = snd_ctl_elem_info_get_count(info);
> > +	if (sz == 0 || sz > count || sz > sizeof(dst->value.bytes)) {
> > +		uc_error("invalid parameter size!");
> 
> A question here is what if the file size is smaller than the expected data size.
> Should we allow this explicitly?  I thought it'd be safer to bail out, but I'd like
> to know more about the scenarios behind this patch.
> 

The scenario here is to load multiple (~160) binary configure files, but their length may be variable from about 10 to 512 bytes. (most of them is about 10 to 100 bytes) 
In this case, the count defined in driver have to be 512. If I return when size != 512, all configure files must be extended to 512 bytes, it will take about 80 Kbytes. So I used 512 as a range. But Liam and I agreed that it will be safer to bail out in this case. I will change the patch. Thanks.

> > +		err = -EINVAL;
> > +		goto __fail;
> > +	}
> > +	res = calloc(sz, 1);
> 
> This can be malloc() as we'll overwrite the whole data in anyway.
> 

OK, will change it. Thanks.

> 
> thanks,
> 
> Takashi
> 
> > +	if (res == NULL) {
> > +		err = -ENOMEM;
> > +		goto __fail;
> > +	}
> > +	if (read(fd, res, sz) != sz) {
> > +		err = -errno;
> > +		goto __fail_read;
> > +	}
> > +	for (idx = 0; idx < sz; idx++)
> > +		snd_ctl_elem_value_set_byte(dst, idx, *(res + idx));
> > +      __fail_read:
> > +	free(res);
> > +      __fail:
> > +	close(fd);
> > +	return err;
> > +}
> > +
> >  extern int __snd_ctl_ascii_elem_id_parse(snd_ctl_elem_id_t *dst,
> >  					 const char *str,
> >  					 const char **ret_ptr);
> >
> > -static int execute_cset(snd_ctl_t *ctl, const char *cset)
> > +static int execute_cset(snd_ctl_t *ctl, const char *cset, unsigned
> > +int type)
> >  {
> >  	const char *pos;
> >  	int err;
> > @@ -194,7 +245,10 @@ static int execute_cset(snd_ctl_t *ctl, const char
> *cset)
> >  	err = snd_ctl_elem_info(ctl, info);
> >  	if (err < 0)
> >  		goto __fail;
> > -	err = snd_ctl_ascii_value_parse(ctl, value, info, pos);
> > +	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); @@ -239,6 +293,7 @@ static int
> > execute_sequence(snd_use_case_mgr_t *uc_mgr,
> >  				goto __fail_nomem;
> >  			break;
> >  		case SEQUENCE_ELEMENT_TYPE_CSET:
> > +		case SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE:
> >  			if (cdev == NULL) {
> >  				const char *cdev1 = NULL, *cdev2 = NULL;
> >  				err = get_value3(&cdev1, "PlaybackCTL", @@
> -274,7 +329,7 @@
> > static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
> >  					goto __fail;
> >  				}
> >  			}
> > -			err = execute_cset(ctl, s->data.cset);
> > +			err = execute_cset(ctl, s->data.cset, s->type);
> >  			if (err < 0) {
> >  				uc_error("unable to execute cset '%s'\n", s-
> >data.cset);
> >  				goto __fail;
> > diff --git a/src/ucm/parser.c b/src/ucm/parser.c index
> > d7517f6..9e1cb41 100644
> > --- a/src/ucm/parser.c
> > +++ b/src/ucm/parser.c
> > @@ -306,6 +306,16 @@ static int parse_sequence(snd_use_case_mgr_t
> *uc_mgr ATTRIBUTE_UNUSED,
> >  			continue;
> >  		}
> >
> > +		if (strcmp(cmd, "cset-bin-file") == 0) {
> > +			curr->type =
> SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE;
> > +			err = parse_string(n, &curr->data.cset);
> > +			if (err < 0) {
> > +				uc_error("error: cset-bin-file 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 87f14a2..c1655c7
> > 100644
> > --- a/src/ucm/ucm_local.h
> > +++ b/src/ucm/ucm_local.h
> > @@ -47,6 +47,7 @@
> >  #define SEQUENCE_ELEMENT_TYPE_CSET	2
> >  #define SEQUENCE_ELEMENT_TYPE_SLEEP	3
> >  #define SEQUENCE_ELEMENT_TYPE_EXEC	4
> > +#define SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE	5
> >
> >  struct ucm_value {
> >          struct list_head list;
> > --
> > 2.1.0
> >

Re: [PATCH - UCM 1/1] ucm: add binary configure file parse

[Takashi Iwai]

At Wed, 21 Jan 2015 01:12:27 +0000,
Lu, Han wrote:
> 
> Hi Takashi,
> 
> BR,
> Han Lu
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Tuesday, January 20, 2015 5:19 PM
> > To: Lu, Han
> > Cc: Girdwood, Liam R; alsa-devel@alsa-project.org
> > Subject: Re: [PATCH - UCM 1/1] ucm: add binary configure file parse
> > 
> > At Tue, 20 Jan 2015 16:33:09 +0800,
> > han.lu@intel.com wrote:
> > >
> > > From: "Lu, Han" <han.lu@intel.com>
> > >
> > > with cset command, UCM set kcontrol parameters directly:
> > >     cset "name='<KCONTROL_NAME>' 1<,2,3,...>"
> > > This patch enables UCM to set kcontrol with parameters from configure
> > > file:
> > >     cset-bin-file "name='<KCONTROL_NAME>' <path/to/file>"
> > > where "cset-bin-file" is a newly added keyword alongside of "cset", to
> > > indicate cset with binary data in file.
> > > The binary data in file is parameter for audio DSPs, and it's just
> > > passed by UCM/ALSA as raw data. The data type of the parameter element
> > > must be byte, and the count is limited by the size of struct
> > > snd_ctl_elem_value and driver definition.
> > >
> > > Signed-off-by: Lu, Han <han.lu@intel.com>
> > >
> > > diff --git a/src/ucm/main.c b/src/ucm/main.c index 37ae4c8..818465a
> > > 100644
> > > --- a/src/ucm/main.c
> > > +++ b/src/ucm/main.c
> > > @@ -160,11 +160,62 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr,
> > >  	return 0;
> > >  }
> > >
> > > +static int binary_file_parse(snd_ctl_elem_value_t *dst,
> > > +			      snd_ctl_elem_info_t *info,
> > > +			      const char *filepath)
> > > +{
> > > +	int err, fd;
> > > +	struct stat st;
> > > +	size_t sz;
> > > +	char *res;
> > > +	snd_ctl_elem_type_t type;
> > > +	unsigned int idx, count;
> > > +
> > > +	type = snd_ctl_elem_info_get_type(info);
> > > +	if (type != SND_CTL_ELEM_TYPE_BYTES) {
> > > +		uc_error("only support byte type!");
> > > +		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;
> > > +	count = snd_ctl_elem_info_get_count(info);
> > > +	if (sz == 0 || sz > count || sz > sizeof(dst->value.bytes)) {
> > > +		uc_error("invalid parameter size!");
> > 
> > A question here is what if the file size is smaller than the expected data size.
> > Should we allow this explicitly?  I thought it'd be safer to bail out, but I'd like
> > to know more about the scenarios behind this patch.
> > 
> 
> The scenario here is to load multiple (~160) binary configure files, but their length may be variable from about 10 to 512 bytes. (most of them is about 10 to 100 bytes) 
> In this case, the count defined in driver have to be 512. If I return when size != 512, all configure files must be extended to 512 bytes, it will take about 80 Kbytes. So I used 512 as a range. But Liam and I agreed that it will be safer to bail out in this case. I will change the patch. Thanks.

OK.  I wasn't objecting to allowing smaller sizes, but just wondered.
Meanwhile, limiting the size is obviously safer.  If this limitation
would become a real problem, we can loosen the condition somehow
later, which is much easier than making more strict later.


thanks,

Takashi

[PATCH - UCM 1/1] ucm: add binary configure file parse

[han.lu]

From: "Lu, Han" <han.lu@intel.com>

with cset command, UCM set kcontrol parameters directly:
    cset "name='<KCONTROL_NAME>' 1<,2,3,...>"
This patch enables UCM to set kcontrol with parameters from
configure file:
    cset-bin-file "name='<KCONTROL_NAME>' <path/to/file>"
where "cset-bin-file" is a newly added keyword alongside of "cset",
to indicate cset with binary data in file.
The binary data in file is parameter for audio DSPs, and it's just
passed by UCM/ALSA as raw data. The data type of the parameter
element must be byte, and the count is limited by the size of
struct snd_ctl_elem_value and driver definition.

Signed-off-by: Lu, Han <han.lu@intel.com>

diff --git a/src/ucm/main.c b/src/ucm/main.c
index 37ae4c8..a1b1fb6 100644
--- a/src/ucm/main.c
+++ b/src/ucm/main.c
@@ -160,11 +160,57 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr,
 	return 0;
 }
 
+static int binary_file_parse(snd_ctl_elem_value_t *dst,
+			      snd_ctl_elem_info_t *info,
+			      const char *filepath)
+{
+	int err, fd;
+	struct stat st;
+	size_t sz;
+	char *res;
+	snd_ctl_elem_type_t type;
+	unsigned int idx, count;
+
+	type = snd_ctl_elem_info_get_type(info);
+	if (type != SND_CTL_ELEM_TYPE_BYTES) {
+		uc_error("only support byte type!");
+		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;
+	if (sz > sizeof(dst->value.bytes))
+		sz = sizeof(dst->value.bytes);
+	count = snd_ctl_elem_info_get_count(info);
+	if (sz > count)
+		sz = count;
+	res = calloc(sz, 1);
+	if (res == NULL) {
+		err = -ENOMEM;
+		goto __fail;
+	}
+	read(fd, res, sz);
+	for (idx = 0; idx < sz; idx++)
+		snd_ctl_elem_value_set_byte(dst, idx, *(res + idx));
+	free(res);
+      __fail:
+	close(fd);
+	return err;
+}
+
 extern int __snd_ctl_ascii_elem_id_parse(snd_ctl_elem_id_t *dst,
 					 const char *str,
 					 const char **ret_ptr);
 
-static int execute_cset(snd_ctl_t *ctl, const char *cset)
+static int execute_cset(snd_ctl_t *ctl, const char *cset, unsigned int type)
 {
 	const char *pos;
 	int err;
@@ -194,7 +240,10 @@ static int execute_cset(snd_ctl_t *ctl, const char *cset)
 	err = snd_ctl_elem_info(ctl, info);
 	if (err < 0)
 		goto __fail;
-	err = snd_ctl_ascii_value_parse(ctl, value, info, pos);
+	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);
@@ -239,6 +288,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
 				goto __fail_nomem;
 			break;
 		case SEQUENCE_ELEMENT_TYPE_CSET:
+		case SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE:
 			if (cdev == NULL) {
 				const char *cdev1 = NULL, *cdev2 = NULL;
 				err = get_value3(&cdev1, "PlaybackCTL",
@@ -274,7 +324,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
 					goto __fail;
 				}
 			}
-			err = execute_cset(ctl, s->data.cset);
+			err = execute_cset(ctl, s->data.cset, s->type);
 			if (err < 0) {
 				uc_error("unable to execute cset '%s'\n", s->data.cset);
 				goto __fail;
diff --git a/src/ucm/parser.c b/src/ucm/parser.c
index d7517f6..9e1cb41 100644
--- a/src/ucm/parser.c
+++ b/src/ucm/parser.c
@@ -306,6 +306,16 @@ static int parse_sequence(snd_use_case_mgr_t *uc_mgr ATTRIBUTE_UNUSED,
 			continue;
 		}
 
+		if (strcmp(cmd, "cset-bin-file") == 0) {
+			curr->type = SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE;
+			err = parse_string(n, &curr->data.cset);
+			if (err < 0) {
+				uc_error("error: cset-bin-file 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 87f14a2..c1655c7 100644
--- a/src/ucm/ucm_local.h
+++ b/src/ucm/ucm_local.h
@@ -47,6 +47,7 @@
 #define SEQUENCE_ELEMENT_TYPE_CSET	2
 #define SEQUENCE_ELEMENT_TYPE_SLEEP	3
 #define SEQUENCE_ELEMENT_TYPE_EXEC	4
+#define SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE	5
 
 struct ucm_value {
         struct list_head list;
-- 
2.1.0

Re: [PATCH - UCM 1/1] ucm: add binary configure file parse

[Takashi Iwai]

At Mon, 19 Jan 2015 15:31:10 +0800,
han.lu@intel.com wrote:
> 
> From: "Lu, Han" <han.lu@intel.com>
> 
> with cset command, UCM set kcontrol parameters directly:
>     cset "name='<KCONTROL_NAME>' 1<,2,3,...>"
> This patch enables UCM to set kcontrol with parameters from
> configure file:
>     cset-bin-file "name='<KCONTROL_NAME>' <path/to/file>"
> where "cset-bin-file" is a newly added keyword alongside of "cset",
> to indicate cset with binary data in file.
> The binary data in file is parameter for audio DSPs, and it's just
> passed by UCM/ALSA as raw data. The data type of the parameter
> element must be byte, and the count is limited by the size of
> struct snd_ctl_elem_value and driver definition.
> 
> Signed-off-by: Lu, Han <han.lu@intel.com>

The patch looks now much better, but...

> 
> diff --git a/src/ucm/main.c b/src/ucm/main.c
> index 37ae4c8..a1b1fb6 100644
> --- a/src/ucm/main.c
> +++ b/src/ucm/main.c
> @@ -160,11 +160,57 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr,
>  	return 0;
>  }
>  
> +static int binary_file_parse(snd_ctl_elem_value_t *dst,
> +			      snd_ctl_elem_info_t *info,
> +			      const char *filepath)
> +{
> +	int err, fd;
> +	struct stat st;
> +	size_t sz;
> +	char *res;
> +	snd_ctl_elem_type_t type;
> +	unsigned int idx, count;
> +
> +	type = snd_ctl_elem_info_get_type(info);
> +	if (type != SND_CTL_ELEM_TYPE_BYTES) {
> +		uc_error("only support byte type!");
> +		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;
> +	if (sz > sizeof(dst->value.bytes))
> +		sz = sizeof(dst->value.bytes);
> +	count = snd_ctl_elem_info_get_count(info);
> +	if (sz > count)
> +		sz = count;

Actually you have here both the expected data size and the actual data
size.  If the actual data size doesn't match with the expected size,
it means rather an error.  (Imagine to read an empty file, for
example.)  So, better to return an error instead of silently
truncating.

> +	res = calloc(sz, 1);
> +	if (res == NULL) {
> +		err = -ENOMEM;
> +		goto __fail;
> +	}
> +	read(fd, res, sz);

Don't forget to check the return value from read().

Also, I'd like to have an ack from Liam, at least, for UCM stuff, so
please add him to Cc (or put his ack tag if you already got it).


thanks,

Takashi

> +	for (idx = 0; idx < sz; idx++)
> +		snd_ctl_elem_value_set_byte(dst, idx, *(res + idx));
> +	free(res);
> +      __fail:
> +	close(fd);
> +	return err;
> +}
> +
>  extern int __snd_ctl_ascii_elem_id_parse(snd_ctl_elem_id_t *dst,
>  					 const char *str,
>  					 const char **ret_ptr);
>  
> -static int execute_cset(snd_ctl_t *ctl, const char *cset)
> +static int execute_cset(snd_ctl_t *ctl, const char *cset, unsigned int type)
>  {
>  	const char *pos;
>  	int err;
> @@ -194,7 +240,10 @@ static int execute_cset(snd_ctl_t *ctl, const char *cset)
>  	err = snd_ctl_elem_info(ctl, info);
>  	if (err < 0)
>  		goto __fail;
> -	err = snd_ctl_ascii_value_parse(ctl, value, info, pos);
> +	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);
> @@ -239,6 +288,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
>  				goto __fail_nomem;
>  			break;
>  		case SEQUENCE_ELEMENT_TYPE_CSET:
> +		case SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE:
>  			if (cdev == NULL) {
>  				const char *cdev1 = NULL, *cdev2 = NULL;
>  				err = get_value3(&cdev1, "PlaybackCTL",
> @@ -274,7 +324,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
>  					goto __fail;
>  				}
>  			}
> -			err = execute_cset(ctl, s->data.cset);
> +			err = execute_cset(ctl, s->data.cset, s->type);
>  			if (err < 0) {
>  				uc_error("unable to execute cset '%s'\n", s->data.cset);
>  				goto __fail;
> diff --git a/src/ucm/parser.c b/src/ucm/parser.c
> index d7517f6..9e1cb41 100644
> --- a/src/ucm/parser.c
> +++ b/src/ucm/parser.c
> @@ -306,6 +306,16 @@ static int parse_sequence(snd_use_case_mgr_t *uc_mgr ATTRIBUTE_UNUSED,
>  			continue;
>  		}
>  
> +		if (strcmp(cmd, "cset-bin-file") == 0) {
> +			curr->type = SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE;
> +			err = parse_string(n, &curr->data.cset);
> +			if (err < 0) {
> +				uc_error("error: cset-bin-file 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 87f14a2..c1655c7 100644
> --- a/src/ucm/ucm_local.h
> +++ b/src/ucm/ucm_local.h
> @@ -47,6 +47,7 @@
>  #define SEQUENCE_ELEMENT_TYPE_CSET	2
>  #define SEQUENCE_ELEMENT_TYPE_SLEEP	3
>  #define SEQUENCE_ELEMENT_TYPE_EXEC	4
> +#define SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE	5
>  
>  struct ucm_value {
>          struct list_head list;
> -- 
> 2.1.0
> 

[PATCH - UCM 1/1] ucm: add binary configure file parse

[han.lu]

From: "Lu, Han" <han.lu@intel.com>

with cset command, UCM set kcontrol parameters directly:
    cset "name='<KCONTROL_NAME>' 1<,2,3,...>"
This patch enables UCM to set kcontrol with parameters from
configure file:
    cset-bin-file "name='<KCONTROL_NAME>' <path/to/file>"
where "cset-bin-file" is a newly added keyword alongside of "cset",
to indicate cset with binary data in file.
The binary data is parameter only for audio DSPs, and it's just
passed by UCM/ALSA as raw data. The data type of the parameter
element must be byte, and the maxim number of parameter elements is
512 (the maxim value that struct snd_ctl_elem_value can hold).

Signed-off-by: Lu, Han <han.lu@intel.com>

diff --git a/src/ucm/main.c b/src/ucm/main.c
index 37ae4c8..d2b1e15 100644
--- a/src/ucm/main.c
+++ b/src/ucm/main.c
@@ -160,11 +160,45 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr,
 	return 0;
 }
 
+static int binary_file_parse(snd_ctl_elem_value_t *dst,
+			      const char *filepath)
+{
+	int err = 0;
+	FILE *in;
+	long len;
+	char *res;
+	unsigned int idx;
+
+	in = fopen(filepath, "r");
+	if (!in) {
+		err = -errno;
+		goto __fail;
+	}
+	fseek(in, 0L, SEEK_END);
+	len = ftell(in);
+	rewind(in);
+	if (len > 512)
+		len = 512;
+	res = calloc(1, (size_t)len);
+	if (res == NULL) {
+		err = -ENOMEM;
+		goto __fail_nomem;
+	}
+	fread(res, (size_t)len, 1, in);
+	for (idx = 0; idx < len; idx++)
+		snd_ctl_elem_value_set_byte(dst, idx, *(res + idx));
+	free(res);
+      __fail_nomem:
+	fclose(in);
+      __fail:
+	return err;
+}
+
 extern int __snd_ctl_ascii_elem_id_parse(snd_ctl_elem_id_t *dst,
 					 const char *str,
 					 const char **ret_ptr);
 
-static int execute_cset(snd_ctl_t *ctl, const char *cset)
+static int execute_cset(snd_ctl_t *ctl, const char *cset, int isbin)
 {
 	const char *pos;
 	int err;
@@ -194,7 +228,10 @@ static int execute_cset(snd_ctl_t *ctl, const char *cset)
 	err = snd_ctl_elem_info(ctl, info);
 	if (err < 0)
 		goto __fail;
-	err = snd_ctl_ascii_value_parse(ctl, value, info, pos);
+	if (isbin)
+		err = binary_file_parse(value, pos);
+	else
+		err = snd_ctl_ascii_value_parse(ctl, value, info, pos);
 	if (err < 0)
 		goto __fail;
 	err = snd_ctl_elem_write(ctl, value);
@@ -239,6 +276,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
 				goto __fail_nomem;
 			break;
 		case SEQUENCE_ELEMENT_TYPE_CSET:
+		case SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE:
 			if (cdev == NULL) {
 				const char *cdev1 = NULL, *cdev2 = NULL;
 				err = get_value3(&cdev1, "PlaybackCTL",
@@ -274,7 +312,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
 					goto __fail;
 				}
 			}
-			err = execute_cset(ctl, s->data.cset);
+			err = execute_cset(ctl, s->data.cset, s->isbin);
 			if (err < 0) {
 				uc_error("unable to execute cset '%s'\n", s->data.cset);
 				goto __fail;
diff --git a/src/ucm/parser.c b/src/ucm/parser.c
index d7517f6..fb5a033 100644
--- a/src/ucm/parser.c
+++ b/src/ucm/parser.c
@@ -306,6 +306,17 @@ static int parse_sequence(snd_use_case_mgr_t *uc_mgr ATTRIBUTE_UNUSED,
 			continue;
 		}
 
+		if (strcmp(cmd, "cset-bin-file") == 0) {
+			curr->type = SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE;
+			curr->isbin = 1;
+			err = parse_string(n, &curr->data.cset);
+			if (err < 0) {
+				uc_error("error: cset-bin-file 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 87f14a2..d0a609b 100644
--- a/src/ucm/ucm_local.h
+++ b/src/ucm/ucm_local.h
@@ -47,6 +47,7 @@
 #define SEQUENCE_ELEMENT_TYPE_CSET	2
 #define SEQUENCE_ELEMENT_TYPE_SLEEP	3
 #define SEQUENCE_ELEMENT_TYPE_EXEC	4
+#define SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE	5
 
 struct ucm_value {
         struct list_head list;
@@ -63,6 +64,7 @@ struct sequence_element {
 		char *cset;
 		char *exec;
 	} data;
+	int isbin;          /* Indicate cset to handle binary or ascii array */
 };
 
 /*
-- 
1.9.1

Re: [PATCH - UCM 1/1] ucm: add binary configure file parse

[Takashi Iwai]

At Thu, 15 Jan 2015 09:26:28 +0800,
han.lu@intel.com wrote:
> 
> From: "Lu, Han" <han.lu@intel.com>
> 
> with cset command, UCM set kcontrol parameters directly:
>     cset "name='<KCONTROL_NAME>' 1<,2,3,...>"
> This patch enables UCM to set kcontrol with parameters from
> configure file:
>     cset-bin-file "name='<KCONTROL_NAME>' <path/to/file>"
> where "cset-bin-file" is a newly added keyword alongside of "cset",
> to indicate cset with binary data in file.
> The binary data is parameter only for audio DSPs, and it's just
> passed by UCM/ALSA as raw data. The data type of the parameter
> element must be byte, and the maxim number of parameter elements is
> 512 (the maxim value that struct snd_ctl_elem_value can hold).
> 
> Signed-off-by: Lu, Han <han.lu@intel.com>
> 
> diff --git a/src/ucm/main.c b/src/ucm/main.c
> index 37ae4c8..d2b1e15 100644
> --- a/src/ucm/main.c
> +++ b/src/ucm/main.c
> @@ -160,11 +160,45 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr,
>  	return 0;
>  }
>  
> +static int binary_file_parse(snd_ctl_elem_value_t *dst,
> +			      const char *filepath)
> +{
> +	int err = 0;
> +	FILE *in;
> +	long len;
> +	char *res;
> +	unsigned int idx;
> +
> +	in = fopen(filepath, "r");
> +	if (!in) {
> +		err = -errno;
> +		goto __fail;
> +	}
> +	fseek(in, 0L, SEEK_END);
> +	len = ftell(in);
> +	rewind(in);

There is a better way to identify the length of a file.
Also, for reading a file as a chunk, no need to use the buffered
access.

> +	if (len > 512)
> +		len = 512;
> +	res = calloc(1, (size_t)len);
> +	if (res == NULL) {
> +		err = -ENOMEM;
> +		goto __fail_nomem;
> +	}
> +	fread(res, (size_t)len, 1, in);
> +	for (idx = 0; idx < len; idx++)
> +		snd_ctl_elem_value_set_byte(dst, idx, *(res + idx));

What if the ctl element isn't a binary type?

> +	free(res);
> +      __fail_nomem:
> +	fclose(in);
> +      __fail:
> +	return err;
> +}
> +
>  extern int __snd_ctl_ascii_elem_id_parse(snd_ctl_elem_id_t *dst,
>  					 const char *str,
>  					 const char **ret_ptr);
>  
> -static int execute_cset(snd_ctl_t *ctl, const char *cset)
> +static int execute_cset(snd_ctl_t *ctl, const char *cset, int isbin)
>  {
>  	const char *pos;
>  	int err;
> @@ -194,7 +228,10 @@ static int execute_cset(snd_ctl_t *ctl, const char *cset)
>  	err = snd_ctl_elem_info(ctl, info);
>  	if (err < 0)
>  		goto __fail;
> -	err = snd_ctl_ascii_value_parse(ctl, value, info, pos);
> +	if (isbin)
> +		err = binary_file_parse(value, pos);
> +	else
> +		err = snd_ctl_ascii_value_parse(ctl, value, info, pos);
>  	if (err < 0)
>  		goto __fail;
>  	err = snd_ctl_elem_write(ctl, value);
> @@ -239,6 +276,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
>  				goto __fail_nomem;
>  			break;
>  		case SEQUENCE_ELEMENT_TYPE_CSET:
> +		case SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE:
>  			if (cdev == NULL) {
>  				const char *cdev1 = NULL, *cdev2 = NULL;
>  				err = get_value3(&cdev1, "PlaybackCTL",
> @@ -274,7 +312,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
>  					goto __fail;
>  				}
>  			}
> -			err = execute_cset(ctl, s->data.cset);
> +			err = execute_cset(ctl, s->data.cset, s->isbin);

isbin doesn't have to be a struct member at all since it's referred
only here can be identified via s->type.


Takashi


>  			if (err < 0) {
>  				uc_error("unable to execute cset '%s'\n", s->data.cset);
>  				goto __fail;
> diff --git a/src/ucm/parser.c b/src/ucm/parser.c
> index d7517f6..fb5a033 100644
> --- a/src/ucm/parser.c
> +++ b/src/ucm/parser.c
> @@ -306,6 +306,17 @@ static int parse_sequence(snd_use_case_mgr_t *uc_mgr ATTRIBUTE_UNUSED,
>  			continue;
>  		}
>  
> +		if (strcmp(cmd, "cset-bin-file") == 0) {
> +			curr->type = SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE;
> +			curr->isbin = 1;
> +			err = parse_string(n, &curr->data.cset);
> +			if (err < 0) {
> +				uc_error("error: cset-bin-file 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 87f14a2..d0a609b 100644
> --- a/src/ucm/ucm_local.h
> +++ b/src/ucm/ucm_local.h
> @@ -47,6 +47,7 @@
>  #define SEQUENCE_ELEMENT_TYPE_CSET	2
>  #define SEQUENCE_ELEMENT_TYPE_SLEEP	3
>  #define SEQUENCE_ELEMENT_TYPE_EXEC	4
> +#define SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE	5
>  
>  struct ucm_value {
>          struct list_head list;
> @@ -63,6 +64,7 @@ struct sequence_element {
>  		char *cset;
>  		char *exec;
>  	} data;
> +	int isbin;          /* Indicate cset to handle binary or ascii array */
>  };
>  
>  /*
> -- 
> 1.9.1
>