| Message ID | a21ab28c-12d5-41c2-9209-4127fe3ca79a@sakamocchi.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 02 Sep 2016 13:18:04 +0200, Takashi Sakamoto wrote: > > Clemens, > > On Aug 31 2016 20:54, Clemens Ladisch wrote: > > Takashi Iwai wrote: > >> TLV has never been and (will never be) an API to handle a > >> generic binary stream. > > > > It would be possible to define something like SNDRV_CTL_TLVT_HWDEP_BLOB > > or _COEFFICIENTS, or to reserve a range of TLVT values for driver- > > defined types. (But we cannot change soc-wm-adsp to support only proper > > TLV data, because this would introduce a regression.) > > For our information, I wrote a patch including your idea. > > > Thanks for your bright comment ;) > > Takashi Sakamoto > > ---- 8< ---- > > >From d1a0eabf1b33fae5de7cedf7aee23778f5dcf46c Mon Sep 17 00:00:00 2001 > From: Takashi Sakamoto <o-takashi@sakamocchi.jp> > Date: Fri, 2 Sep 2016 00:12:20 +0900 > Subject: [PATCH] ALSA: control: coefficient support > > --- > include/uapi/sound/asound.h | 5 +++++ > include/uapi/sound/tlv.h | 1 + > sound/soc/codecs/wm_adsp.c | 3 ++- > sound/soc/soc-ops.c | 37 ++++++++++++++++++++++++++++++++++--- > sound/soc/soc-topology.c | 3 ++- > 5 files changed, 44 insertions(+), 5 deletions(-) > > diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h > index 609cadb..69c0585 100644 > --- a/include/uapi/sound/asound.h > +++ b/include/uapi/sound/asound.h > @@ -848,6 +848,11 @@ typedef int __bitwise snd_ctl_elem_iface_t; > #define SNDRV_CTL_ELEM_ACCESS_TLV_WRITE (1<<5) /* TLV write is possible */ > #define SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE > (SNDRV_CTL_ELEM_ACCESS_TLV_READ|SNDRV_CTL_ELEM_ACCESS_TLV_WRITE) > #define SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND (1<<6) /* TLV command is > possible */ > +/* > + * Arbitrary data is accepted for coefficients, instead of pure > threshold level > + * information. > + */ > +#define SNDRV_CTL_ELEM_ACCESS_TLV_COEFF (1<<7) > #define SNDRV_CTL_ELEM_ACCESS_INACTIVE (1<<8) /* control does actually > nothing, but may be updated */ > #define SNDRV_CTL_ELEM_ACCESS_LOCK (1<<9) /* write lock */ > #define SNDRV_CTL_ELEM_ACCESS_OWNER (1<<10) /* write lock owner */ The introduction of a new flag is a good idea (although it's better to be named not specifically to coef). Then we can skip such elements to be accessed from alsactl or amixer. However... > @@ -773,19 +774,49 @@ int snd_soc_bytes_tlv_callback(struct snd_kcontrol > *kcontrol, int op_flag, > unsigned int size, unsigned int __user *tlv) > { > struct soc_bytes_ext *params = (void *)kcontrol->private_value; > - unsigned int count = size < params->max ? size : params->max; > + unsigned int count; > + unsigned int type; > int ret = -ENXIO; > > + /* > + * The TLV packet can transfer numerical ID for one control element. > + * But ALSA control core don't tell it to each implementation of > + * TLV callback. Here, instead, use the first volatile data to > + * check access information. > + */ > + if (!(kcontrol->vd[0].access & SNDRV_CTL_ELEM_ACCESS_TLV_COEFF)) > + return -ENXIO; > + > + /* The data should be constructed according to TLV protocol. */ > + if (copy_from_user(&type, tlv, sizeof(unsigned int))) > + return -EFAULT; > + > + /* The type should be an arbitrary data. */ > + if (type != SNDRV_CTL_TLVT_COEFF) > + return -ENXIO; This breaks the already existing application and is a regression, as Clemens mentioned. This should have been forced from the beginning, but it's too late to put now. So, it's likely no-go. But, the addition of the flag is helpful alone. Could you concentrate on that and resubmit? thanks, Takashi
On Sep 3 2016 01:05, Takashi Iwai wrote: > But, the addition of the flag is helpful alone. Could you concentrate > on that and resubmit? Before starting it, I'd like to wait for comments from Clemens Ladisch, Charles Keepax and Vinod Koul, in a view of application interfaces stable, safe and tolerant for long term usage. I'm unwilling to use my time more for light-minded work done to bring confusions to hardware- abstraction layers for user land applications. But, if possible, I'd request you to re-think about my proposal to return processed byte of TLV packet payload to user land, again. It looks to be intrusive, but in my taste, it doesn't bring much impacts to applications, than adding new flags to kernel/userspace interfaces. The length of processed bytes from drivers in ALSA SoC part might help developers to get current value of 'struct soc_bytes_ext.max' of each control element set. As of kernel 4.8, soc-da7218/nau8825/wm5102 modules implements the coefficients, and it's already in ALSA topology design. I believe it better to assist developers for the feature, than judging it's coarse and abuse for good APIs. Regards Takashi Sakamoto
On Fri, Sep 02, 2016 at 08:18:04PM +0900, Takashi Sakamoto wrote: > Clemens, > > On Aug 31 2016 20:54, Clemens Ladisch wrote: > > Takashi Iwai wrote: > >> TLV has never been and (will never be) an API to handle a > >> generic binary stream. > > > > It would be possible to define something like SNDRV_CTL_TLVT_HWDEP_BLOB > > or _COEFFICIENTS, or to reserve a range of TLVT values for driver- > > defined types. (But we cannot change soc-wm-adsp to support only proper > > TLV data, because this would introduce a regression.) > > For our information, I wrote a patch including your idea. > > > Thanks for your bright comment ;) > > Takashi Sakamoto > > ---- 8< ---- > > >From d1a0eabf1b33fae5de7cedf7aee23778f5dcf46c Mon Sep 17 00:00:00 2001 > From: Takashi Sakamoto <o-takashi@sakamocchi.jp> > Date: Fri, 2 Sep 2016 00:12:20 +0900 > Subject: [PATCH] ALSA: control: coefficient support > > --- > include/uapi/sound/asound.h | 5 +++++ > include/uapi/sound/tlv.h | 1 + > sound/soc/codecs/wm_adsp.c | 3 ++- > sound/soc/soc-ops.c | 37 ++++++++++++++++++++++++++++++++++--- > sound/soc/soc-topology.c | 3 ++- > 5 files changed, 44 insertions(+), 5 deletions(-) > > diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h > index 609cadb..69c0585 100644 > --- a/include/uapi/sound/asound.h > +++ b/include/uapi/sound/asound.h > @@ -848,6 +848,11 @@ typedef int __bitwise snd_ctl_elem_iface_t; > #define SNDRV_CTL_ELEM_ACCESS_TLV_WRITE (1<<5) /* TLV write is possible */ > #define SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE > (SNDRV_CTL_ELEM_ACCESS_TLV_READ|SNDRV_CTL_ELEM_ACCESS_TLV_WRITE) > #define SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND (1<<6) /* TLV command is > possible */ > +/* > + * Arbitrary data is accepted for coefficients, instead of pure > threshold level > + * information. > + */ > +#define SNDRV_CTL_ELEM_ACCESS_TLV_COEFF (1<<7) Yeah the addition of a new flag here would very much get my vote. The only way at the moment to tell if you are dealing with this type of control from user-space is to look for a control of type SNDRV_CTL_ELEM_TYPE_BYTES that doesn't have SNDRV_CTL_ELEM_ACCESS_READ or SNDRV_CTL_ELEM_ACCESS_WRITE. Which is far from ideal. > #define SNDRV_CTL_ELEM_ACCESS_INACTIVE (1<<8) /* control does actually > nothing, but may be updated */ > #define SNDRV_CTL_ELEM_ACCESS_LOCK (1<<9) /* write lock */ > #define SNDRV_CTL_ELEM_ACCESS_OWNER (1<<10) /* write lock owner */ > diff --git a/include/uapi/sound/tlv.h b/include/uapi/sound/tlv.h > index ffc4f20..fd1c867 100644 > --- a/include/uapi/sound/tlv.h > +++ b/include/uapi/sound/tlv.h > @@ -19,6 +19,7 @@ > #define SNDRV_CTL_TLVT_DB_RANGE 3 /* dB range container */ > #define SNDRV_CTL_TLVT_DB_MINMAX 4 /* dB scale with min/max */ > #define SNDRV_CTL_TLVT_DB_MINMAX_MUTE 5 /* dB scale with min/max with > mute */ > +#define SNDRV_CTL_TLVT_COEFF 6 /* Arbitrary data */ > > /* > * channel-mapping TLV items > diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c > index 21fbe7d..525d70b 100644 > --- a/sound/soc/codecs/wm_adsp.c > +++ b/sound/soc/codecs/wm_adsp.c > @@ -930,7 +930,8 @@ static unsigned int wmfw_convert_flags(unsigned int > in, unsigned int len) > wr = SNDRV_CTL_ELEM_ACCESS_TLV_WRITE; > vol = SNDRV_CTL_ELEM_ACCESS_VOLATILE; > > - out = SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK; > + out = SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK | > + SNDRV_CTL_ELEM_ACCESS_TLV_COEFF; > } else { > rd = SNDRV_CTL_ELEM_ACCESS_READ; > wr = SNDRV_CTL_ELEM_ACCESS_WRITE; > diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c > index a513a34..14cdd59 100644 > --- a/sound/soc/soc-ops.c > +++ b/sound/soc/soc-ops.c > @@ -31,6 +31,7 @@ > #include <sound/soc.h> > #include <sound/soc-dpcm.h> > #include <sound/initval.h> > +#include <sound/tlv.h> > > /** > * snd_soc_info_enum_double - enumerated double mixer info callback > @@ -773,19 +774,49 @@ int snd_soc_bytes_tlv_callback(struct snd_kcontrol > *kcontrol, int op_flag, > unsigned int size, unsigned int __user *tlv) > { > struct soc_bytes_ext *params = (void *)kcontrol->private_value; > - unsigned int count = size < params->max ? size : params->max; > + unsigned int count; > + unsigned int type; > int ret = -ENXIO; > > + /* > + * The TLV packet can transfer numerical ID for one control element. > + * But ALSA control core don't tell it to each implementation of > + * TLV callback. Here, instead, use the first volatile data to > + * check access information. > + */ > + if (!(kcontrol->vd[0].access & SNDRV_CTL_ELEM_ACCESS_TLV_COEFF)) > + return -ENXIO; > + > + /* The data should be constructed according to TLV protocol. */ > + if (copy_from_user(&type, tlv, sizeof(unsigned int))) > + return -EFAULT; > + > + /* The type should be an arbitrary data. */ > + if (type != SNDRV_CTL_TLVT_COEFF) > + return -ENXIO; > + I agree it is probably a bit late to add this now. Thanks, Charles
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index 609cadb..69c0585 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -848,6 +848,11 @@ typedef int __bitwise snd_ctl_elem_iface_t; #define SNDRV_CTL_ELEM_ACCESS_TLV_WRITE (1<<5) /* TLV write is possible */ #define SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE (SNDRV_CTL_ELEM_ACCESS_TLV_READ|SNDRV_CTL_ELEM_ACCESS_TLV_WRITE) #define SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND (1<<6) /* TLV command is possible */ +/* + * Arbitrary data is accepted for coefficients, instead of pure threshold level + * information. + */ +#define SNDRV_CTL_ELEM_ACCESS_TLV_COEFF (1<<7) #define SNDRV_CTL_ELEM_ACCESS_INACTIVE (1<<8) /* control does actually nothing, but may be updated */ #define SNDRV_CTL_ELEM_ACCESS_LOCK (1<<9) /* write lock */ #define SNDRV_CTL_ELEM_ACCESS_OWNER (1<<10) /* write lock owner */ diff --git a/include/uapi/sound/tlv.h b/include/uapi/sound/tlv.h index ffc4f20..fd1c867 100644 --- a/include/uapi/sound/tlv.h +++ b/include/uapi/sound/tlv.h @@ -19,6 +19,7 @@ #define SNDRV_CTL_TLVT_DB_RANGE 3 /* dB range container */ #define SNDRV_CTL_TLVT_DB_MINMAX 4 /* dB scale with min/max */ #define SNDRV_CTL_TLVT_DB_MINMAX_MUTE 5 /* dB scale with min/max with mute */ +#define SNDRV_CTL_TLVT_COEFF 6 /* Arbitrary data */ /* * channel-mapping TLV items diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c index 21fbe7d..525d70b 100644 --- a/sound/soc/codecs/wm_adsp.c +++ b/sound/soc/codecs/wm_adsp.c @@ -930,7 +930,8 @@ static unsigned int wmfw_convert_flags(unsigned int in, unsigned int len) wr = SNDRV_CTL_ELEM_ACCESS_TLV_WRITE; vol = SNDRV_CTL_ELEM_ACCESS_VOLATILE; - out = SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK; + out = SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK | + SNDRV_CTL_ELEM_ACCESS_TLV_COEFF; } else { rd = SNDRV_CTL_ELEM_ACCESS_READ; wr = SNDRV_CTL_ELEM_ACCESS_WRITE; diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c index a513a34..14cdd59 100644 --- a/sound/soc/soc-ops.c +++ b/sound/soc/soc-ops.c @@ -31,6 +31,7 @@ #include <sound/soc.h> #include <sound/soc-dpcm.h> #include <sound/initval.h> +#include <sound/tlv.h> /** * snd_soc_info_enum_double - enumerated double mixer info callback @@ -773,19 +774,49 @@ int snd_soc_bytes_tlv_callback(struct snd_kcontrol *kcontrol, int op_flag, unsigned int size, unsigned int __user *tlv) { struct soc_bytes_ext *params = (void *)kcontrol->private_value; - unsigned int count = size < params->max ? size : params->max; + unsigned int count; + unsigned int type; int ret = -ENXIO; + /* + * The TLV packet can transfer numerical ID for one control element. + * But ALSA control core don't tell it to each implementation of + * TLV callback. Here, instead, use the first volatile data to + * check access information. + */ + if (!(kcontrol->vd[0].access & SNDRV_CTL_ELEM_ACCESS_TLV_COEFF)) + return -ENXIO; + + /* The data should be constructed according to TLV protocol. */ + if (copy_from_user(&type, tlv, sizeof(unsigned int))) + return -EFAULT; + + /* The type should be an arbitrary data. */ + if (type != SNDRV_CTL_TLVT_COEFF) + return -ENXIO; + + /* + * The second element of TLV packet payload means the length of an + * actual data. But this implementation is going to trim it. + */ + count = min_t(unsigned int, tlv[1] - sizeof(unsigned int), params->max); + switch (op_flag) { case SNDRV_CTL_TLV_OP_READ: if (params->get) - ret = params->get(kcontrol, tlv, count); + ret = params->get(kcontrol, tlv + 2, count); break; case SNDRV_CTL_TLV_OP_WRITE: if (params->put) - ret = params->put(kcontrol, tlv, count); + ret = params->put(kcontrol, tlv + 2, count); break; } + + if (ret >= 0) { + if (copy_to_user(tlv + 1, &count, sizeof(unsigned int))) + return -EFAULT; + } + return ret; } EXPORT_SYMBOL_GPL(snd_soc_bytes_tlv_callback); diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index ee7f15a..a8f6060 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -507,7 +507,8 @@ static int soc_tplg_kcontrol_bind_io(struct snd_soc_tplg_ctl_hdr *hdr, if (hdr->ops.info == SND_SOC_TPLG_CTL_BYTES && k->iface & SNDRV_CTL_ELEM_IFACE_MIXER && k->access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE - && k->access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) { + && k->access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK + && k->access & SNDRV_CTL_ELEM_ACCESS_TLV_COEFF) { struct soc_bytes_ext *sbe; struct snd_soc_tplg_bytes_control *be;