Skip to content
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

Jump to bottom

Audio: Crossover: Convert to module adapter #7871

Merged
merged 2 commits into from
Aug 18, 2023
Merged

Audio: Crossover: Convert to module adapter #7871

merged 2 commits into from
Aug 18, 2023

Conversation

singalsu
Copy link
Collaborator

This patch contains basic changes to run crossover component as module adapter client in IPC3 system. Additional changes are needed for IPC4.

@singalsu singalsu requested a review from a team June 28, 2023 16:10
@singalsu singalsu changed the title [SKIP SOF-TEST] Audio: Crossover: Convert to module adapter Audio: Crossover: Convert to module adapter Jun 29, 2023
johnylin76
johnylin76 reviewed Jul 7, 2023
View reviewed changes
src/audio/crossover/crossover.c
sinks_c[i] = buffer_acquire(sinks[i]);
avail = audio_stream_avail_frames(&source_c->stream,
&sinks_c[i]->stream);
frames = MIN(frames, avail);
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic seems to be removed by the commit, is it intentional?
It looks important that determines the processed frames with regard to sink capacities in case of overrun.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as Curtis asked? The module adapter does this for clients. Though there's also the sinks[i] NULL case if states are not equal. I have not been able to create such test situation, could a NULL sink impact the avail/free. I'm not sure.

johnylin76
johnylin76 reviewed Jul 7, 2023
View reviewed changes
src/audio/crossover/crossover.c Outdated

/* Validate frame format and buffer size of sinks */
list_for_item(sink_list, &dev->bsink_list) {
sink = container_of(sink_list, struct comp_buffer, source_list);
sink_c = buffer_acquire(sink);

/* Note: Printing pipeline IDs for debug, these are with IPC4 zeros, probably due
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the bug you mentioned still exist under IPC4? If yes, is it worked-around by the commit, or is it non-critical?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm now after vacation starting to study this again -- I hope I'll gain understanding of IPC4 to be able to address this.

@singalsu singalsu force-pushed the crossover_module_convert branch 3 times, most recently from a7b2560 to dfe5fc7 Compare August 8, 2023 16:06
@singalsu singalsu requested a review from ranj063 August 8, 2023 16:10
@singalsu
Copy link
Collaborator Author

singalsu commented Aug 8, 2023

Crossover now runs with this PR correctly with IPC4 and IPC3, so changing this to non-draft. I'm able to run 2-way configuration with UPX board in nocodec mode. I play one stream and capture back from SSP loopback paths the band filtered filters versions those appear to be OK.

The other patches those belong to this work are:

@singalsu singalsu marked this pull request as ready for review August 8, 2023 17:08
kv2019i
kv2019i reviewed Aug 11, 2023
View reviewed changes
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Thanks @singalsu ! I think there is one req-for-an-inline-comment outstanding, but otherwise looks good to go.

src/audio/crossover/crossover.c Show resolved Hide resolved
src/include/sof/audio/component.h Show resolved Hide resolved
lyakh
lyakh reviewed Aug 16, 2023
View reviewed changes
src/audio/crossover/crossover.c Outdated
&sinks_c[i]->stream);
frames = MIN(frames, avail);
/* Process crossover */
if (frames) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: an easier form would be

if (!frames)
    return -ENODATA;

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, done

src/audio/crossover/crossover.c Outdated
}

audio_stream_init_alignment_constants(1, 1, &sink_c->stream);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this needs to be done even if the formats didn't match and we're going to error out?

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, ordered this way for more compact appear with my style but done now.

@singalsu singalsu requested a review from lyakh August 17, 2023 09:53
lyakh
lyakh reviewed Aug 17, 2023
View reviewed changes
src/audio/crossover/crossover.c Outdated
goto out;
return ret;

audio_stream_init_alignment_constants(1, 1, &sink_c->stream);
Copy link
Collaborator

Choose a reason for hiding this comment

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

no, this cannot be done here - cannot use sink_c after releasing

@singalsu
Audio: Crossover: Convert to module adapter
c334078 
This patch contains the changes to run crossover component as
module adapter client in IPC3 and IPC4 systems. The largest change
is to use pin indices in IPC4 instead of pipeline IDs to identify
sink streams. Pipeline IDs on firmware side are temporary in IPC4.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
@singalsu
Audio: Crossover: Fix a mistake in s32 processing core
516750c 
The filters state need to be retrieved for every channel as
it is done in other s16 and s24 processing cores. The mistake
caused very distorted sound with waveform discontinuity every
copy period, e.g. 1 ms.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
@singalsu singalsu requested a review from lyakh August 17, 2023 12:12
btian1
btian1 approved these changes Aug 18, 2023
View reviewed changes
src/include/sof/audio/component.h Show resolved Hide resolved
@lgirdwood
Copy link
Member

@wszypelt @lrudyX good to merge ? IIUC, crossover not tested by internal CI today.

@lrudyX
Copy link

lrudyX commented Aug 18, 2023

@wszypelt @lrudyX good to merge ? IIUC, crossover not tested by internal CI today.

It is waiting in queue. I have moved it to the top. We had some problems with DUTs.

@lrudyX
Copy link

lrudyX commented Aug 18, 2023

@lgirdwood Internal CI result is pass :)

@cujomalainey cujomalainey merged commit df460c8 into thesofproject:main Aug 18, 2023
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lyakh lyakh lyakh approved these changes

@cujomalainey cujomalainey cujomalainey approved these changes

@btian1 btian1 btian1 approved these changes

@kv2019i kv2019i kv2019i approved these changes

@ranj063 ranj063 Awaiting requested review from ranj063 ranj063 is a code owner

@lgirdwood lgirdwood Awaiting requested review from lgirdwood lgirdwood is a code owner

@plbossart plbossart Awaiting requested review from plbossart plbossart is a code owner

@mmaka1 mmaka1 Awaiting requested review from mmaka1 mmaka1 is a code owner

@lbetlej lbetlej Awaiting requested review from lbetlej lbetlej is a code owner

@dbaluta dbaluta Awaiting requested review from dbaluta dbaluta is a code owner

@johnylin76 johnylin76 Awaiting requested review from johnylin76

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@singalsu @lgirdwood @lrudyX @lyakh @cujomalainey @btian1 @johnylin76 @kv2019i