| Message ID | 20171002160421.22696-1-ckeepax@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 02 Oct 2017 18:04:21 +0200, Charles Keepax wrote: > > ASoC can use the TLV mechanism to pass coefficient data for > controls that exceed the normal 512 byte limit. However, there > is no mechanism to inform user-space the control expects data > through the TLV mechanism. Currently amixer uses a combination > of the absense of non-TLV read/write permissions and the type > being BYTES. This is not ideal as it is not a clear signal and > prevents controls that might support both access mechanisms. > > Add a specific flag SNDRV_CTL_ELEM_ACCESS_TLV_COEFF to let > user-space know when it is dealing with a TLV control that is > being used to pass actual coefficient data. Then add this flag > to the controls generated by the current users of these types of > controls. > > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> > --- > > Hi, > > This builds on work done by Sakamoto-san [1] and the changes > mentioned to amixer in the commit message are found in [2]. I > think something like this should probably be added, what I am > less clear on is where we go from here? > > Sakamoto-san's original patches also added a tag type > SNDRV_CTL_TVLT_COEFF, to keep the data matching the normal format > found in TLV controls. The trouble is neither of the two users > really follow this convention at the moment. The ADSP code just > takes the data as is with no additional headers and the Skylake > code does have a tag and a length as part of the data passed > through the TLV however the tag part looks like it is used for > some internal purpose, so I am far from clear it would support a > fixed value to identify the data without some changes as well. > > Changing the ADSP code is probably acceptable since we have had > very few users in the field, and since this work has been done > improvements in the firmware tool chains mean most firmwares are > created with much smaller dedicated purpose controls rather than > the large block ones that were initially the reason for using > these controls on our side. > > Finally, since [3] tinymix has been updated to add some TLV > information into control writes/reads which will break the ADSP > code once it trickles down to users. So I would be keen to > discuss this at this years conference and decide how we are going > to handle these controls? As Mark says [4] maybe we should see if > there is a better approach we should all migrate across to? Currently I'm open for this although ideally we may have TLV COEFF type for consistency as you noted. Who should test the callback (whether in ASoC core or ALSA core) is another open question, but it's a cosmetic issue. In anyway, I'd happily discuss this at audio conf. thanks, Takashi > > Thanks, > Charles > > [1] https://patchwork.kernel.org/patch/9304649/ > [2] https://patchwork.kernel.org/patch/8147001/ > [3] https://github.com/tinyalsa/tinyalsa/commit/3f813e47784674a3909fe1277bc10b70d03791e2 > [4] http://mailman.alsa-project.org/pipermail/alsa-devel/2017-September/125743.html > > > include/sound/soc.h | 3 ++- > include/uapi/sound/asound.h | 1 + > sound/core/control.c | 1 + > sound/soc/codecs/wm_adsp.c | 3 ++- > sound/soc/intel/skylake/skl-topology.c | 1 + > sound/soc/soc-ops.c | 3 +++ > 6 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/include/sound/soc.h b/include/sound/soc.h > index d776cdee30d7..d82297a95caf 100644 > --- a/include/sound/soc.h > +++ b/include/sound/soc.h > @@ -323,7 +323,8 @@ > #define SND_SOC_BYTES_TLV(xname, xcount, xhandler_get, xhandler_put) \ > { .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, \ > .access = SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE | \ > - SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK, \ > + SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK | \ > + SNDRV_CTL_ELEM_ACCESS_TLV_COEFF, \ > .tlv.c = (snd_soc_bytes_tlv_callback), \ > .info = snd_soc_bytes_info_ext, \ > .private_value = (unsigned long)&(struct soc_bytes_ext) \ > diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h > index 1949923a40bf..4bf0b2f835e9 100644 > --- a/include/uapi/sound/asound.h > +++ b/include/uapi/sound/asound.h > @@ -853,6 +853,7 @@ 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 */ > +#define SNDRV_CTL_ELEM_ACCESS_TLV_COEFF (1<<7) /* Coefficient data is accepted */ > #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/sound/core/control.c b/sound/core/control.c > index 56b3e2d49c82..e7a74d48633b 100644 > --- a/sound/core/control.c > +++ b/sound/core/control.c > @@ -266,6 +266,7 @@ struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new *ncontrol, > SNDRV_CTL_ELEM_ACCESS_INACTIVE | > SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE | > SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND | > + SNDRV_CTL_ELEM_ACCESS_TLV_COEFF | > SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK); > > err = snd_ctl_new(&kctl, count, access, NULL); > diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c > index 65c059b5ffd7..1d7fe0d8a18a 100644 > --- a/sound/soc/codecs/wm_adsp.c > +++ b/sound/soc/codecs/wm_adsp.c > @@ -1156,7 +1156,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/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c > index 27bcb62568fb..01b01c6a2bf3 100644 > --- a/sound/soc/intel/skylake/skl-topology.c > +++ b/sound/soc/intel/skylake/skl-topology.c > @@ -2870,6 +2870,7 @@ static int skl_tplg_control_load(struct snd_soc_component *cmpnt, > tplg_bc = container_of(hdr, > struct snd_soc_tplg_bytes_control, hdr); > if (kctl->access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) { > + kctl->access |= SNDRV_CTL_ELEM_ACCESS_TLV_COEFF; > sb = (struct soc_bytes_ext *)kctl->private_value; > if (tplg_bc->priv.size) > return skl_init_algo_data( > diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c > index 500f98c730b9..5ac2afe12b5d 100644 > --- a/sound/soc/soc-ops.c > +++ b/sound/soc/soc-ops.c > @@ -776,6 +776,9 @@ int snd_soc_bytes_tlv_callback(struct snd_kcontrol *kcontrol, int op_flag, > unsigned int count = size < params->max ? size : params->max; > int ret = -ENXIO; > > + if (!(kcontrol->vd[0].access & SNDRV_CTL_ELEM_ACCESS_TLV_COEFF)) > + return -ENXIO; > + > switch (op_flag) { > case SNDRV_CTL_TLV_OP_READ: > if (params->get) > -- > 2.11.0 > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel >
On Thu, Oct 05, 2017 at 09:04:09AM +0200, Takashi Iwai wrote: > On Mon, 02 Oct 2017 18:04:21 +0200, > Charles Keepax wrote: > Currently I'm open for this although ideally we may have TLV COEFF > type for consistency as you noted. > I have another patch that adds just arguing with myself over some of the details of implementing it given the current implementations don't use it. I will try to ping that out shortly so we can have a look at that before the conference as well. > Who should test the callback (whether in ASoC core or ALSA core) is > another open question, but it's a cosmetic issue. > Yeah will have a think about that thanks. Thanks, Charles
diff --git a/include/sound/soc.h b/include/sound/soc.h index d776cdee30d7..d82297a95caf 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -323,7 +323,8 @@ #define SND_SOC_BYTES_TLV(xname, xcount, xhandler_get, xhandler_put) \ { .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, \ .access = SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE | \ - SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK, \ + SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK | \ + SNDRV_CTL_ELEM_ACCESS_TLV_COEFF, \ .tlv.c = (snd_soc_bytes_tlv_callback), \ .info = snd_soc_bytes_info_ext, \ .private_value = (unsigned long)&(struct soc_bytes_ext) \ diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index 1949923a40bf..4bf0b2f835e9 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -853,6 +853,7 @@ 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 */ +#define SNDRV_CTL_ELEM_ACCESS_TLV_COEFF (1<<7) /* Coefficient data is accepted */ #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/sound/core/control.c b/sound/core/control.c index 56b3e2d49c82..e7a74d48633b 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -266,6 +266,7 @@ struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new *ncontrol, SNDRV_CTL_ELEM_ACCESS_INACTIVE | SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE | SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND | + SNDRV_CTL_ELEM_ACCESS_TLV_COEFF | SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK); err = snd_ctl_new(&kctl, count, access, NULL); diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c index 65c059b5ffd7..1d7fe0d8a18a 100644 --- a/sound/soc/codecs/wm_adsp.c +++ b/sound/soc/codecs/wm_adsp.c @@ -1156,7 +1156,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/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c index 27bcb62568fb..01b01c6a2bf3 100644 --- a/sound/soc/intel/skylake/skl-topology.c +++ b/sound/soc/intel/skylake/skl-topology.c @@ -2870,6 +2870,7 @@ static int skl_tplg_control_load(struct snd_soc_component *cmpnt, tplg_bc = container_of(hdr, struct snd_soc_tplg_bytes_control, hdr); if (kctl->access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) { + kctl->access |= SNDRV_CTL_ELEM_ACCESS_TLV_COEFF; sb = (struct soc_bytes_ext *)kctl->private_value; if (tplg_bc->priv.size) return skl_init_algo_data( diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c index 500f98c730b9..5ac2afe12b5d 100644 --- a/sound/soc/soc-ops.c +++ b/sound/soc/soc-ops.c @@ -776,6 +776,9 @@ int snd_soc_bytes_tlv_callback(struct snd_kcontrol *kcontrol, int op_flag, unsigned int count = size < params->max ? size : params->max; int ret = -ENXIO; + if (!(kcontrol->vd[0].access & SNDRV_CTL_ELEM_ACCESS_TLV_COEFF)) + return -ENXIO; + switch (op_flag) { case SNDRV_CTL_TLV_OP_READ: if (params->get)
ASoC can use the TLV mechanism to pass coefficient data for controls that exceed the normal 512 byte limit. However, there is no mechanism to inform user-space the control expects data through the TLV mechanism. Currently amixer uses a combination of the absense of non-TLV read/write permissions and the type being BYTES. This is not ideal as it is not a clear signal and prevents controls that might support both access mechanisms. Add a specific flag SNDRV_CTL_ELEM_ACCESS_TLV_COEFF to let user-space know when it is dealing with a TLV control that is being used to pass actual coefficient data. Then add this flag to the controls generated by the current users of these types of controls. Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> --- Hi, This builds on work done by Sakamoto-san [1] and the changes mentioned to amixer in the commit message are found in [2]. I think something like this should probably be added, what I am less clear on is where we go from here? Sakamoto-san's original patches also added a tag type SNDRV_CTL_TVLT_COEFF, to keep the data matching the normal format found in TLV controls. The trouble is neither of the two users really follow this convention at the moment. The ADSP code just takes the data as is with no additional headers and the Skylake code does have a tag and a length as part of the data passed through the TLV however the tag part looks like it is used for some internal purpose, so I am far from clear it would support a fixed value to identify the data without some changes as well. Changing the ADSP code is probably acceptable since we have had very few users in the field, and since this work has been done improvements in the firmware tool chains mean most firmwares are created with much smaller dedicated purpose controls rather than the large block ones that were initially the reason for using these controls on our side. Finally, since [3] tinymix has been updated to add some TLV information into control writes/reads which will break the ADSP code once it trickles down to users. So I would be keen to discuss this at this years conference and decide how we are going to handle these controls? As Mark says [4] maybe we should see if there is a better approach we should all migrate across to? Thanks, Charles [1] https://patchwork.kernel.org/patch/9304649/ [2] https://patchwork.kernel.org/patch/8147001/ [3] https://github.com/tinyalsa/tinyalsa/commit/3f813e47784674a3909fe1277bc10b70d03791e2 [4] http://mailman.alsa-project.org/pipermail/alsa-devel/2017-September/125743.html include/sound/soc.h | 3 ++- include/uapi/sound/asound.h | 1 + sound/core/control.c | 1 + sound/soc/codecs/wm_adsp.c | 3 ++- sound/soc/intel/skylake/skl-topology.c | 1 + sound/soc/soc-ops.c | 3 +++ 6 files changed, 10 insertions(+), 2 deletions(-)