Skip to content

Volume: Fix handling of mute and unmute control #2196

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jajanusz merged 1 commit into from
Dec 15, 2019
Merged

Volume: Fix handling of mute and unmute control #2196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 60 additions & 46 deletions src/audio/volume/volume.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ static enum task_state vol_work(void *data)
int again = 0;
int i;

cd->vol_ramp_active = true;

/* inc/dec each volume if it's not at target */
for (i = 0; i < PLATFORM_MAX_CHANNELS; i++) {
/* skip if target reached */
Expand All @@ -96,6 +98,7 @@ static enum task_state vol_work(void *data)
/* ramp up, check if ramp completed */
if (vol >= cd->tvolume[i] || vol >= cd->vol_max) {
vol_update(cd, i);
cd->ramp_increment[i] = 0;
} else {
cd->volume[i] = vol;
again = 1;
Expand All @@ -105,11 +108,13 @@ static enum task_state vol_work(void *data)
if (vol <= 0) {
/* cannot ramp down below 0 */
vol_update(cd, i);
cd->ramp_increment[i] = 0;
} else {
/* ramp completed ? */
if (vol <= cd->tvolume[i] ||
vol <= cd->vol_min) {
vol_update(cd, i);
cd->ramp_increment[i] = 0;
} else {
cd->volume[i] = vol;
again = 1;
Expand All @@ -122,7 +127,11 @@ static enum task_state vol_work(void *data)
}

/* do we need to continue ramping */
return again ? SOF_TASK_STATE_RESCHEDULE : SOF_TASK_STATE_COMPLETED;
if (again)
return SOF_TASK_STATE_RESCHEDULE;

cd->vol_ramp_active = 0;
return SOF_TASK_STATE_COMPLETED;
}

/**
Expand Down Expand Up @@ -212,9 +221,12 @@ static struct comp_dev *volume_new(struct sof_ipc_comp *comp)
for (i = 0; i < PLATFORM_MAX_CHANNELS; i++) {
cd->volume[i] = MAX(MIN(cd->vol_max, VOL_ZERO_DB),
cd->vol_min);
cd->tvolume[i] = cd->volume[i];
cd->tvolume[i] = cd->volume[i];
cd->mvolume[i] = cd->volume[i];
cd->muted[i] = false;
}

cd->vol_ramp_active = false;

@lyakh lyakh Dec 12, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<rant> I find we use zero-initialising allocations (rzalloc()) exceedingly, sometimes also where they aren't really required, and then we also additionally initialise fields of the just allocated object with 0 or NULL of false... Seems strange to me </rant>

@paulstelian97 paulstelian97 Dec 12, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyakh I'd find it safer than forgetting to zero-initialize where it's needed, for one.

@singalsu singalsu Dec 12, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do whatever you recommend. I'm myself preferring to be explicit with things that need to be initialized. Let me know your preference.

trace_volume_with_ids(dev,
"vol->initial_ramp = %d, vol->ramp = %d, "
"vol->min_value = %d, vol->max_value = %d",
Expand Down Expand Up @@ -360,10 +372,11 @@ static inline void volume_set_chan_mute(struct comp_dev *dev, int chan)
{
struct comp_data *cd = comp_get_drvdata(dev);

/* Check if not muted already */
if (cd->volume[chan] != 0)
cd->mvolume[chan] = cd->volume[chan];
cd->tvolume[chan] = 0;
if (!cd->muted[chan]) {
cd->mvolume[chan] = cd->tvolume[chan];
volume_set_chan(dev, chan, 0);
cd->muted[chan] = true;
}
}

/**
Expand All @@ -375,9 +388,10 @@ static inline void volume_set_chan_unmute(struct comp_dev *dev, int chan)
{
struct comp_data *cd = comp_get_drvdata(dev);

/* Check if muted */
if (cd->volume[chan] == 0)
cd->tvolume[chan] = cd->mvolume[chan];
if (cd->muted[chan]) {
cd->muted[chan] = false;
volume_set_chan(dev, chan, cd->mvolume[chan]);
}
}

/**
Expand All @@ -390,7 +404,8 @@ static int volume_ctrl_set_cmd(struct comp_dev *dev,
struct sof_ipc_ctrl_data *cdata)
{
struct comp_data *cd = comp_get_drvdata(dev);
int i;
uint32_t val;
int ch;
int j;
int ret = 0;

Expand All @@ -408,57 +423,56 @@ static int volume_ctrl_set_cmd(struct comp_dev *dev,
"cdata->comp_id = %u",
cdata->comp_id);
for (j = 0; j < cdata->num_elems; j++) {
trace_volume_with_ids(dev, "volume_ctrl_set_cmd(), "
"SOF_CTRL_CMD_VOLUME, "
"channel = %u, value = %u",
cdata->chanv[j].channel,
cdata->chanv[j].value);
i = cdata->chanv[j].channel;
if (i >= 0 && i < SOF_IPC_MAX_CHANNELS) {
ret = volume_set_chan(dev, i,
cdata->chanv[j].value);
} else {
ch = cdata->chanv[j].channel;
val = cdata->chanv[j].value;
trace_volume_with_ids(dev, "volume_ctrl_set_cmd(), channel = %d"
", value = %u", ch, val);
if (ch < 0 || ch >= SOF_IPC_MAX_CHANNELS) {
trace_volume_error_with_ids(dev,
"volume_ctrl_set_cmd() "
"error: "
"SOF_CTRL_CMD_VOLUME, "
"invalid i = %u",
i);
"volume_ctrl_set_cmd(), illegal channel = %d",
ch);

@lyakh lyakh Dec 12, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to error out in this case? If we do, I'd revert the test to do

if (chan_is_invalid) {
	trace();
	return -EBADERROR;
}
normal_case();

@paulstelian97 paulstelian97 Dec 12, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe some ret = -EINVAL so the following if (ret) return ret; will trigger. Agreed.

@singalsu singalsu Dec 12, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@singalsu singalsu Dec 12, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

return -EINVAL;
}

if (cd->muted[ch]) {
cd->mvolume[ch] = val;
} else {
ret = volume_set_chan(dev, ch, val);

@lyakh lyakh Dec 13, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also move the declaration here

int ret = volume_set_...();

to make it clear that ret isn't needed anywhere else. But probably not worth redoing the patch just for this.

@singalsu singalsu Dec 13, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm mistaken that's not done in SOF. I'll fix the typo any way, it's safe to change without need for me to re-test stuff.

@jajanusz jajanusz Dec 15, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO what @lyakh suggested is better, BUT @singalsu is right that it's forbidden in SOF. In kernel you can see that they allow declaration on start of the block, but in sound tree there is rule that everything goes to the start of the function, because most of maintainers decided so.

if (ret)
return ret;
}
if (ret)
return ret;
}

schedule_task(&cd->volwork, VOL_RAMP_UPDATE_US,
VOL_RAMP_UPDATE_US);
if (!cd->vol_ramp_active)
schedule_task(&cd->volwork, VOL_RAMP_UPDATE_US,
VOL_RAMP_UPDATE_US);
break;

case SOF_CTRL_CMD_SWITCH:
trace_volume_with_ids(dev, "volume_ctrl_set_cmd(), "
"SOF_CTRL_CMD_SWITCH, "
"cdata->comp_id = %u", cdata->comp_id);
for (j = 0; j < cdata->num_elems; j++) {
trace_volume_with_ids(dev, "volume_ctrl_set_cmd(), "
"SOF_CTRL_CMD_SWITCH, "
"channel = %u, value = %u",
cdata->chanv[j].channel,
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 {
ch = cdata->chanv[j].channel;
val = cdata->chanv[j].value;
trace_volume_with_ids(dev, "volume_ctrl_set_cmd(), channel = %d"
", value = %u", ch, val);
if (ch < 0 || ch >= SOF_IPC_MAX_CHANNELS) {
trace_volume_error_with_ids(dev,
"volume_ctrl_set_cmd() error: "
"SOF_CTRL_CMD_SWITCH, invalid i = %u",
i);
"volume_ctrl_set_cmd(), illegal channel = %d",
ch);

@lyakh lyakh Dec 12, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto for aborting and %d

@singalsu singalsu Dec 12, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@singalsu singalsu Dec 12, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

return -EINVAL;
}

if (val)
volume_set_chan_unmute(dev, ch);
else
volume_set_chan_mute(dev, ch);
}

schedule_task(&cd->volwork, VOL_RAMP_UPDATE_US,
VOL_RAMP_UPDATE_US);
if (!cd->vol_ramp_active)
schedule_task(&cd->volwork, VOL_RAMP_UPDATE_US,
VOL_RAMP_UPDATE_US);
break;

default:
Expand Down
2 changes: 2 additions & 0 deletions src/include/sof/audio/volume.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,8 @@ struct comp_data {
int32_t vol_min; /**< minimum volume */
int32_t vol_max; /**< maximum volume */
int32_t vol_ramp_range; /**< max ramp transition */
bool muted[SOF_IPC_MAX_CHANNELS]; /**< set if channel is muted */
bool vol_ramp_active; /**< set if volume is ramped */
/**< volume processing function */
void (*scale_vol)(struct comp_dev *dev, struct comp_buffer *sink,
struct comp_buffer *source, uint32_t frames);
Expand Down