[alsa-devel] [PATCH - UCM 2/2] ucm: add binary configure file parse

[Takashi Iwai]

At Tue, 13 Jan 2015 18:26:18 +0000,
Liam Girdwood wrote:
> 
> On Tue, 2015-01-13 at 17:52 +0100, Takashi Iwai wrote:
> > At Tue, 13 Jan 2015 11:00:39 +0800,
> > han.lu at intel.com wrote:
> > > 
> > > From: "Lu, Han" <han.lu at 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:
> > >     bcsetf "name='<KCONTROL_NAME>' <path/to/file>"
> > > where "bcsetf" is a newly added keyword alongside of "cset", to
> > > indicate binary cset with file; and <path/to/file> is the
> > > configure file storing parameters in bytes array, up to 512 Bytes
> > > (the maxim value that struct snd_ctl_elem_value can hold).
> > 
> > Why binary?  It's not portable.  You can't carry it to a different
> > architecture.
> > 
> 
> The intention here is that the binary data is not meant for the host but
> for audio DSPs so it's just passed by UCM/ALSA as raw data.

In that case, we should limit to certain element data types.
Otherwise people would abuse it for passing data even to integer or
enum ctls.

And of course it'd be better to clarify the reason in the patch
description :)

BTW, I'm still not so convinced by bcsetf...  Can't it be more verbose
or readable?


thanks,

Takashi

> We do have some DSP processing algos that have tuning tools where the
> tool will generate a binary file of coefficients (depending on various
> physical device properties etc). These coefficient files can then be
> uploaded to the DSP by UCM on use case change.
> 
> Liam
> 
> > 
> > Takashi
> > 
> > > 
> > > Signed-off-by: Lu, Han <han.lu at intel.com>
> > > 
> > > diff --git a/src/ucm/main.c b/src/ucm/main.c
> > > index 37ae4c8..1496b22 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_BCSETF:
> > >  			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..686c883 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, "bcsetf") == 0) {
> > > +			curr->type = SEQUENCE_ELEMENT_TYPE_BCSETF;
> > > +			curr->isbin = 1;
> > > +			err = parse_string(n, &curr->data.cset);
> > > +			if (err < 0) {
> > > +				uc_error("error: bcsetf 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..80d7335 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_BCSETF	5
> > >  
> > >  struct ucm_value {
> > >          struct list_head list;
> > > @@ -63,6 +64,7 @@ struct sequence_element {
> > >  		char *cset;
> > >  		char *exec;
> > >  	} data;
> > > +	int isbin;          /* Indicate cset is binary array or ascii array */
> > >  };
> > >  
> > >  /*
> > > -- 
> > > 2.1.0
> > > 
> 
>