Skip to content

Conversation

@singalsu
Copy link
Collaborator

@singalsu singalsu commented Jun 5, 2019

This patch adds feature to retrieve volume scale min and max from
IPC if the parameters are set. Volume min and max are now used
with ramp time parameter to compute gain ramp step to achieve
constant rate ramps with any volume scale. If the min and max are
zeros the previous FW operation is preserved.

Previously the firmware did not know the volume range so the
volume code uses in volume request a check to prevent exceeding
ramp length specified in topology by step size re-adjust. It was
done to prevent very long ramps to happen with volume scales with
large gain. However the smaller transitions remained longer than
necessary. This patch fixes that last remaining issue.

The ramp length check code is preserved to be compatible with
kernel versions those do not provide the min and max volume scale
values. When provided the check code has no impact.

The patch includes some cosmetic re-arranging of volume comp data
struct fields in addition to adding new the fields vol_ramp_range,
vol_min, and vol_max.

Signed-off-by: Seppo Ingalsuo seppo.ingalsuo@linux.intel.com

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here I was thinking if exceeding min and max limits VOL_MIN and VOL_MAX should be an error or silently limit it. The same is done for volume request elsewhere in this code. The VOL_MAX is a very high gain so such situation is very unlikely. But since ramp will fail with illegal values this limiting or error issuing is needed.

Copy link
Member

Choose a reason for hiding this comment

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

What about notifying the user via trace_error if requested man or max cannot be used. This way at least it's obvious in the logs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, makes sense. I'll add the error traces.

Copy link
Contributor

Choose a reason for hiding this comment

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

typo ange

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep!

@singalsu singalsu force-pushed the volume_use_min_max branch from f4470ba to 66b3ffb Compare June 10, 2019 11:17
@singalsu
Copy link
Collaborator Author

The just pushed version fixes the typo and adds comment about purpose of setting vol_ramp_range to zero.

Copy link
Member

Choose a reason for hiding this comment

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

What about notifying the user via trace_error if requested man or max cannot be used. This way at least it's obvious in the logs.

@singalsu singalsu force-pushed the volume_use_min_max branch from 66b3ffb to 55019b0 Compare June 11, 2019 10:58
@singalsu
Copy link
Collaborator Author

Just pushed new version to address comment by @lgirdwood .

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

@singalsu CI is failing now.

@singalsu
Copy link
Collaborator Author

Yep, I'm trying to find it from CI logs...

@singalsu
Copy link
Collaborator Author

SOFCI TEST

Copy link
Contributor

Choose a reason for hiding this comment

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

@singalsu Could we merge them? One trace log can handle up to 4 arguments, so we should be good here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tlauda I have problems with my computer to view traces with very long lines. I do this to keep the trace width something that fits my Ubuntu 1080p desktop with default font and console window stretched to max width. Is there a solution to make the sof-logger lines shorter by e.g. omitting or condensing other fields?

But yes sure I can combine these.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@singalsu singalsu force-pushed the volume_use_min_max branch from 55019b0 to 060300a Compare June 11, 2019 16:16
This patch adds feature to retrieve volume scale min and max from
IPC if the parameters are set. Volume min and max are now used
with ramp time parameter to compute gain ramp step to achieve
constant rate ramps with any volume scale. If the min and max are
zeros the previous FW operation is preserved.

Previously the firmware did not know the volume range so the
volume code uses in volume request a check to prevent exceeding
ramp length specified in topology by step size re-adjust. It was
done to prevent very long ramps to happen with volume scales with
large gain. However the smaller transitions remained longer than
necessary. This patch fixes that last remaining issue.

The ramp length check code is preserved to be compatible with
kernel versions those do not provide the min and max volume scale
values. When provided the check code has no impact.

The patch includes some cosmetic re-arranging of volume comp data
struct fields in addition to adding new the fields vol_ramp_range,
vol_min, and vol_max.

Also error traces were added to notify if the volume min/max or
request has been limited to fit to supported volume range.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
@singalsu singalsu force-pushed the volume_use_min_max branch from 060300a to 260f795 Compare June 11, 2019 16:25
@singalsu
Copy link
Collaborator Author

I just pushed new version with two trace prints combined to one. There was also one mistake push before where this change was missing.

@tlauda tlauda merged commit 6f65414 into thesofproject:master Jun 11, 2019
@singalsu singalsu deleted the volume_use_min_max branch January 27, 2021 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants