Skip to content

Fix Bug：Amp: eliminat pops when switch device between speaker and headphone. #7048

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
lgirdwood merged 1 commit into from
Feb 14, 2023
Merged

Fix Bug：Amp: eliminat pops when switch device between speaker and headphone. #7048

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
35 changes: 16 additions & 19 deletions src/audio/smart_amp/smart_amp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,19 +598,19 @@ static int smart_amp_copy(struct comp_dev *dev)
avail_feedback_frames =

@kv2019i kv2019i Feb 7, 2023

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.

Some style and other minor issues, can you address the checkpatch findings in https://github.com/thesofproject/sof/actions/runs/4112079634/jobs/7099324790

@lgirdwood lgirdwood Feb 7, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There are two pops caused by SOF Firmware.
@LongWang100 thanks for the patch, are you saying this bug is present in general SOF FW code or just specific to the smart AMP logic which you have fixed ?

@johnylin76 johnylin76 Feb 8, 2023

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.

@lgirdwood that's a good question.

It's difficult to tell. However during previous investigation we used to adopt smart_amp_test (SAMPLE_SMART_AMP=y) to run on ADL-P max98373 device, the pop noise was still able to be reproduced. Nevertheless, we had never met the pop noise issue on TGL max98373 devices.

Another thing which had confused me for a long time is that smart_amp.c sounds like to play as the role of common adapter code implementation while there exists smart_amp_maxim_dsm.c and smart_amp_generic.c for Maxim DSM and generic solution respectively. However, I then realized that smart_amp.c basically serves Maxim DSM only according that UUID is directly defined as Maxim DSM. Also, the initial author of smart_amp.c is from Maxim.

I wonder if we would expect smart_amp to be a generic component which may have various solution implementations (somewhat similar to module_adapter). If so, it will need additional (future) commits to partition the current codes into common and solution-specific parts.

@lgirdwood lgirdwood Feb 8, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good point @johnylin76 - we have named this in a generic matter, and some of the logic will probably be generic and the other logic will be specific for Maxim only. I think it would make sense in the future to split out and partition smart amp into generic and vendor files ? @cujomalainey @mwasko any thoughts ?

audio_stream_get_avail_frames(&feedback_buf->stream);

avail_frames = MIN(avail_passthrough_frames,
avail_feedback_frames);
avail_feedback_frames = MIN(avail_passthrough_frames,
avail_feedback_frames);

feedback_bytes = avail_frames *
feedback_bytes = avail_feedback_frames *
audio_stream_frame_bytes(&feedback_buf->stream);

comp_dbg(dev, "smart_amp_copy(): processing %d feedback frames (avail_passthrough_frames: %d)",
avail_frames, avail_passthrough_frames);
avail_feedback_frames, avail_passthrough_frames);

/* perform buffer writeback after source_buf process */
buffer_stream_invalidate(feedback_buf, feedback_bytes);
sad->process(dev, &feedback_buf->stream,
&sink_buf->stream, avail_frames,
&sink_buf->stream, avail_feedback_frames,
sad->config.feedback_ch_map, true);

comp_update_buffer_consume(feedback_buf, feedback_bytes);
Expand All @@ -621,7 +621,6 @@ static int smart_amp_copy(struct comp_dev *dev)

/* bytes calculation */
source_bytes = avail_frames * audio_stream_frame_bytes(&source_buf->stream);

sink_bytes = avail_frames * audio_stream_frame_bytes(&sink_buf->stream);

/* process data */
Expand Down Expand Up @@ -727,20 +726,18 @@ static int smart_amp_prepare(struct comp_dev *dev)
goto error;
}

if (sad->mod_handle->bitwidth != bitwidth) {
sad->mod_handle->bitwidth = bitwidth;
comp_info(dev, "[DSM] Re-initialized for %d bit processing", bitwidth);
sad->mod_handle->bitwidth = bitwidth;

@kv2019i kv2019i Feb 7, 2023

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.

I guess there is no performance impact to doing this every time.

comp_info(dev, "[DSM] Re-initialized for %d bit processing", bitwidth);

ret = smart_amp_init(sad->mod_handle, dev);
if (ret) {
comp_err(dev, "[DSM] Re-initialization error.");
goto error;
}
ret = maxim_dsm_restore_param(sad->mod_handle, dev);
if (ret) {
comp_err(dev, "[DSM] Restoration error.");
goto error;
}
ret = smart_amp_init(sad->mod_handle, dev);
if (ret) {
comp_err(dev, "[DSM] Re-initialization error.");
goto error;
}
ret = maxim_dsm_restore_param(sad->mod_handle, dev);
if (ret) {
comp_err(dev, "[DSM] Restoration error.");
goto error;
}

sad->process = get_smart_amp_process(dev);
Expand Down