Skip to content

EQ FIR: Add support for 16 and 24 bit data#454

Merged
lgirdwood merged 1 commit intothesofproject:masterfrom
singalsu:eq_fir_add_16_24_data_proposal
Oct 10, 2018
Merged

EQ FIR: Add support for 16 and 24 bit data#454
lgirdwood merged 1 commit intothesofproject:masterfrom
singalsu:eq_fir_add_16_24_data_proposal

Conversation

@singalsu
Copy link
Copy Markdown
Collaborator

@singalsu singalsu commented Oct 2, 2018

This patch adds to equalizer capability to process 16 and 24 bit
pipelines in addition to 32 bit similarly as IIR. The generic C,
HiFiEP, and HiFi3 versions are updated with the capability.

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

Comment thread src/audio/eq_fir.c Outdated
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.

Instead of having all this condition code, can we have a block of conditional code at the start of the file that creates some static inline function to set this mapping for HIFI3 and generic modes. That way we only have one area that is conditionally compiled (and makes it easier to add HIFI2 or HIFI4)

@singalsu singalsu force-pushed the eq_fir_add_16_24_data_proposal branch from 6585f48 to 9b52ecb Compare October 4, 2018 12:02
@singalsu
Copy link
Copy Markdown
Collaborator Author

singalsu commented Oct 4, 2018

@lgirdwood I just pushed update for this PR.

Comment thread src/audio/eq_fir.c Outdated
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.

sorry, I mean this function will call a

static inline void set_fir_func(cd);

This function will be defined in condition code

#if HIFI3
static inline void set_fir_func(..)
{
}
#elif HIFI2
static inline void set_fir_func()
{
}
#else
/* generic */
static inline void set_fir_func()
{
}
\endif

@singalsu
Copy link
Copy Markdown
Collaborator Author

singalsu commented Oct 5, 2018

I have done a version of this but I can't push it to PR because git master has stopped to work with EQ topology. Need to find a fix for it first.

But just to confirm I did the right thing is it OK to have the 16/24/32 bit format switch-case inside each static inline void set_fir_func()?

@singalsu singalsu force-pushed the eq_fir_add_16_24_data_proposal branch from 9b52ecb to 9bd86cb Compare October 5, 2018 12:04
@singalsu
Copy link
Copy Markdown
Collaborator Author

singalsu commented Oct 5, 2018

@lgirdwood I just pushed update for this PR with earlier mentioned set_fir_func() style.

I was able to test this but I had to remove earlier commit b5308ee (1k buffers re-introduce + some FIR traces) to avoid DSP panic.

@singalsu
Copy link
Copy Markdown
Collaborator Author

singalsu commented Oct 5, 2018

@lgirdwood @tlauda We suspect memory leak that need to be debugged so better to close this and revert the 1k buffers introduce commit b5308ee.

@singalsu
Copy link
Copy Markdown
Collaborator Author

singalsu commented Oct 5, 2018

Closing.

@singalsu singalsu closed this Oct 5, 2018
@singalsu
Copy link
Copy Markdown
Collaborator Author

singalsu commented Oct 5, 2018

Sorry, I closed wrong PR -- reopen.

@singalsu singalsu reopened this Oct 5, 2018
@singalsu
Copy link
Copy Markdown
Collaborator Author

singalsu commented Oct 5, 2018

Please ignore my comments about memory leak in this thread context. This PR is proposed.

@lgirdwood
Copy link
Copy Markdown
Member

@singalsu can you fix the CI failure and resend

@singalsu
Copy link
Copy Markdown
Collaborator Author

singalsu commented Oct 8, 2018

@lgirdwood This CI issue does not look like EQ related. If the topology doesn't instantiate EQs it only increases a bit the code size. There are similar CI issues for other PRs too.

"ERROR:Exception TimeoutError occurred at 'c:\work\tools\cavs_scripts-py\utilities\fw_loader.py':163 FW not ready after 1.0 seconds!"

Comment thread src/audio/eq_fir.c Outdated
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.

Sorry I mean have one switch statement, but have multiple conditionally compiled versions of

static inline void set_s16_fir(struct comp_data *cd)
{
    cd->eq_func_even = blah;
    cd->eq->func = blah;
}

So the single switch just calls the static inline which is conditionally compiled.

static inline int set_fir_func(struct comp_dev *dev)
{
	struct comp_data *cd = comp_get_drvdata(dev);
	struct sof_ipc_comp_config *config = COMP_GET_CONFIG(dev);
 	switch (config->frame_fmt) {
	case SOF_IPC_FRAME_S16_LE:
		trace_eq("f16");
		set_s16_fir(cd);
		break;
	case SOF_IPC_FRAME_S24_4LE:
		trace_eq("f24");
		set_s24_fir(cd);
		break;
	case SOF_IPC_FRAME_S32_LE:

@singalsu singalsu force-pushed the eq_fir_add_16_24_data_proposal branch from 9bd86cb to b5a5f5d Compare October 9, 2018 10:01
@singalsu
Copy link
Copy Markdown
Collaborator Author

singalsu commented Oct 9, 2018

@lgirdwood I've test pushed a fix a tested this with UP2 with needed PR #464. Sorry for iterations, I hope I got it right this time!

This patch adds to equalizer capability to process 16 and 24 bit
pipelines in addition to 32 bit similarly as IIR. The generic C,
HiFiEP, and HiFi3 versions are updated with the capability.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
@singalsu singalsu force-pushed the eq_fir_add_16_24_data_proposal branch from b5a5f5d to 0c1756c Compare October 10, 2018 10:22
@singalsu
Copy link
Copy Markdown
Collaborator Author

I just pushed this rebased version with conflict fix.

@lgirdwood lgirdwood merged commit bad7b99 into thesofproject:master Oct 10, 2018
@singalsu singalsu deleted the eq_fir_add_16_24_data_proposal branch April 29, 2019 08:46
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.

2 participants