[Sound-open-firmware] [PATCH] Volume: Remove channels mapping via channel map array from volume control
Seppo Ingalsuo
seppo.ingalsuo at linux.intel.com
Mon Nov 13 15:11:42 CET 2017
On 13.11.2017 15:45, Jie, Yang wrote:
>> -----Original Message-----
>> From: sound-open-firmware-bounces at alsa-project.org [mailto:sound-open-
>> firmware-bounces at alsa-project.org] On Behalf Of Seppo Ingalsuo
>> Sent: Monday, November 13, 2017 6:12 PM
>> To: sound-open-firmware at alsa-project.org
>> Cc: Seppo Ingalsuo <seppo.ingalsuo at linux.intel.com>
>> Subject: [Sound-open-firmware] [PATCH] Volume: Remove channels mapping via
>> channel map array from volume control
>>
>> This patch removes the channel map table lookup from all volume control since it
>> is not used by the SOF 1.0 driver. Instead of enum channel descriptors the
>> channels are addressed with indices in 0..<number of channels -1> range.
> So does this suppose that the array index is identical to the channel index?
> If so, we can delete member channel in struct sof_ipc_ctrl_value_chan.
No, the channel to apply is got from cdata->chanv[j].channel. The
channel is not directly the index to array (j).
Without this feature it would not be possible to update just single
channel M without need to update the same time all channels 0..M.
>
> But looks this array indices are not equal to channel mapping if my understanding
> Correctly, e.g. front_left and front right are set to channel index 0 and 1 usually,
> But their enumerated value are 3 and 4 in the enum sof_ipc_chmap definition:
> SOF_CHMAP_FL, /* front left */
> SOF_CHMAP_FR, /* front right */
Populating the channel map is not planned for this driver version so
this feature could not be used. When debugging the firmware it's seen
that channel map is all zeros.
The sof_ipc_chmap also is limited to ITU-R speaker locations so they
wouldn't work for lowest level speaker channels (digital multi-way
speakers or non-std speaker locations) and mic array channels not
directly associated with media content representation. So in future
ideally there should be both direct channel indexes for low level gain
control and channel map based gain for higher level gain controls where
content rendering speaker locations are known.
>
>
> Thanks,
> ~Keyon
>
>> Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo at linux.intel.com>
>> ---
>> src/audio/volume.c | 80 ++++++++++++++++++++++++++++++++++-----------------
>> ---
>> 1 file changed, 50 insertions(+), 30 deletions(-)
>>
>> diff --git a/src/audio/volume.c b/src/audio/volume.c index 415a853..fb8d9e7
>> 100644
>> --- a/src/audio/volume.c
>> +++ b/src/audio/volume.c
>> @@ -53,6 +53,7 @@
>> #define VOL_RAMP_US 2000
>> #define VOL_RAMP_STEP (1 << 11)
>> #define VOL_MAX (1 << 16)
>> +#define VOL_MIN 0
>>
>> /*
>> * Simple volume control
>> @@ -282,14 +283,14 @@ static const struct comp_func_map func_map[] = {
>> /* synchronise host mmap() volume with real value */ static void
>> vol_sync_host(struct comp_data *cd, uint32_t chan) {
>> - int i;
>> -
>> if (cd->hvol == NULL)
>> return;
>>
>> - for (i = 0; i < SOF_IPC_MAX_CHANNELS; i++) {
>> - if (cd->hvol[i].channel == cd->chan[chan])
>> - cd->hvol[i].value = cd->volume[chan];
>> + if (chan < SOF_IPC_MAX_CHANNELS)
>> + cd->hvol[chan].value = cd->volume[chan];
>> + else {
>> + trace_volume_error("veh");
>> + tracev_value(chan);
>> }
>> }
>>
>> @@ -414,10 +415,20 @@ static int volume_params(struct comp_dev *dev)
>> static inline void volume_set_chan(struct comp_dev *dev, int chan, uint32_t vol)
>> {
>> struct comp_data *cd = comp_get_drvdata(dev);
>> + uint32_t v = vol;
>> +
>> + /* Limit received volume gain to MIN..MAX range before applying it.
>> + * MAX is needed for now for the generic C gain arithmetics to prevent
>> + * multiplication overflow with the 32 bit value. Non-zero MIN option
>> + * can be useful to prevent totally muted small volume gain.
>> + */
>> + if (v < VOL_MIN)
>> + v = VOL_MIN;
>> +
>> + if (v > VOL_MAX)
>> + v = VOL_MAX;
>>
>> - /* TODO: ignore vol of 0 atm - bad IPC */
>> - if (vol > 0 && vol <= VOL_MAX)
>> - cd->tvolume[chan] = vol;
>> + cd->tvolume[chan] = v;
>> }
>>
>> static inline void volume_set_chan_mute(struct comp_dev *dev, int chan) @@ -
>> 450,12 +461,16 @@ static int volume_ctrl_set_cmd(struct comp_dev *dev, struct
>> sof_ipc_ctrl_data *c
>> switch (cdata->cmd) {
>> case SOF_CTRL_CMD_VOLUME:
>> trace_volume("vst");
>> - for (i = 0; i < SOF_IPC_MAX_CHANNELS; i++) {
>> - for (j = 0; j < cdata->num_elems; j++) {
>> - tracev_value(cdata->chanv[j].channel);
>> - tracev_value(cdata->chanv[j].value);
>> - if (cdata->chanv[j].channel == cd->chan[i])
>> - volume_set_chan(dev, i, cdata-
>>> chanv[j].value);
>> + trace_value(cdata->comp_id);
>> + for (j = 0; j < cdata->num_elems; j++) {
>> + trace_value(cdata->chanv[j].channel);
>> + trace_value(cdata->chanv[j].value);
>> + i = cdata->chanv[j].channel;
>> + if ((i >= 0) && (i < SOF_IPC_MAX_CHANNELS))
>> + volume_set_chan(dev, i, cdata->chanv[j].value);
>> + else {
>> + trace_volume_error("gs2");
>> + tracev_value(i);
>> }
>> }
>> work_schedule_default(&cd->volwork, VOL_RAMP_US); @@ -
>> 463,16 +478,19 @@ static int volume_ctrl_set_cmd(struct comp_dev *dev, struct
>> sof_ipc_ctrl_data *c
>>
>> case SOF_CTRL_CMD_SWITCH:
>> trace_volume("mst");
>> - for (i = 0; i < SOF_IPC_MAX_CHANNELS; i++) {
>> - for (j = 0; j < cdata->num_elems; j++) {
>> - tracev_value(cdata->chanv[j].channel);
>> - tracev_value(cdata->chanv[j].value);
>> - if (cdata->chanv[j].channel == cd->chan[i]) {
>> - if (cdata->chanv[j].value)
>> - volume_set_chan_unmute(dev,
>> i);
>> - else
>> - volume_set_chan_mute(dev, i);
>> - }
>> + trace_value(cdata->comp_id);
>> + for (j = 0; j < cdata->num_elems; j++) {
>> + trace_value(cdata->chanv[j].channel);
>> + trace_value(cdata->chanv[j].value);
>> + i = cdata->chanv[j].channel;
>> + if ((i >= 0) && (i < SOF_IPC_MAX_CHANNELS)) {
>> + if (cdata->chanv[j].value)
>> + volume_set_chan_unmute(dev, i);
>> + else
>> + volume_set_chan_mute(dev, i);
>> + } else {
>> + trace_volume_error("gs3");
>> + tracev_value(i);
>> }
>> }
>> work_schedule_default(&cd->volwork, VOL_RAMP_US); @@ -
>> 489,21 +507,23 @@ static int volume_ctrl_set_cmd(struct comp_dev *dev, struct
>> sof_ipc_ctrl_data *c static int volume_ctrl_get_cmd(struct comp_dev *dev,
>> struct sof_ipc_ctrl_data *cdata) {
>> struct comp_data *cd = comp_get_drvdata(dev);
>> - int i;
>> int j;
>>
>> /* validate */
>> if (cdata->num_elems == 0 || cdata->num_elems >=
>> SOF_IPC_MAX_CHANNELS) {
>> trace_volume_error("gc0");
>> + tracev_value(cdata->num_elems);
>> return -EINVAL;
>> }
>>
>> if (cdata->cmd == SOF_CTRL_CMD_VOLUME) {
>> - for (i = 0; i < cdata->num_elems; i++) {
>> - for (j = 0; j < cdata->num_elems; j++) {
>> - if (cdata->chanv[j].value == cd->chan[i])
>> - cdata->chanv[j].value = cd->tvolume[i];
>> - }
>> + trace_volume("vgt");
>> + trace_value(cdata->comp_id);
>> + for (j = 0; j < cdata->num_elems; j++) {
>> + cdata->chanv[j].channel = j;
>> + cdata->chanv[j].value = cd->tvolume[j];
>> + trace_value(cdata->chanv[j].channel);
>> + trace_value(cdata->chanv[j].value);
>> }
>> } else {
>> trace_volume_error("ec2");
>> --
>> 2.11.0
>>
>> _______________________________________________
>> Sound-open-firmware mailing list
>> Sound-open-firmware at alsa-project.org
>> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
> _______________________________________________
> Sound-open-firmware mailing list
> Sound-open-firmware at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
>
More information about the Sound-open-firmware
mailing list