Skip to content

ASRC: SRC: Fail params() when DAI does not provide sink stream format information #2642

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
tlauda merged 2 commits into from
Mar 29, 2020
Merged

ASRC: SRC: Fail params() when DAI does not provide sink stream format information #2642

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
080cf9b
ASRC: Fail params() if sink stream rate is zero
singalsu Mar 26, 2020
94ed761
SRC: Fail params() if sink stream rate is zero
singalsu Mar 26, 2020
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
5 changes: 5 additions & 0 deletions src/audio/asrc/asrc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,11 @@ static int asrc_params(struct comp_dev *dev,
/* set source/sink_frames/rate */
cd->source_rate = sourceb->stream.rate;
cd->sink_rate = sinkb->stream.rate;
if (!cd->sink_rate) {
comp_err(dev, "asrc_params(), zero sink rate");
Copy link
Member

@lgirdwood lgirdwood Mar 26, 2020

Choose a reason for hiding this comment

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

Do we check anywhere else for supported source /sink rates ?

Copy link
Collaborator Author

@singalsu singalsu Mar 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

There's check in prepare() where the rate conversion filters are set up for copy(). But this same fail would happen in prepare() if I moved this code there. I think it's better to fail early when we know things won't later work.

There used to be in params() where this error happens a stream params rewrite for topology new() provided rate for sink or source but it was removed when pipeline walk was enhanced to be bi-directional.

The topology provided 48k value for the fixed side buffer is still available (ignored now) and I could revert that pipeline upstream walk impact for sink_rate == 0 or source_rate == 0 cases. In addition I should copy the PCM format from the known buffer. But I don't know if such is appropriate thing to do.

(edited to make my description better understandable for both playback and capture directions)

Copy link
Collaborator

@ranj063 ranj063 Mar 26, 2020

Choose a reason for hiding this comment

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

@singalsu for my education, why would the sink_rate be 0 for HDA or ALH? Is it because they are not using the timer-based pipeline?

Copy link
Collaborator Author

@singalsu singalsu Mar 26, 2020

Choose a reason for hiding this comment

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

It's because in hda.c (and alh.c).

static int hda_get_hw_params(struct dai *dai,
			     struct sof_ipc_stream_params  *params, int dir)
{
	/* 0 means variable */
	params->rate = 0;
	params->channels = 0;
	params->buffer_fmt = 0;
	params->frame_fmt = 0;

	return 0;
}

Copy link
Collaborator

@ranj063 ranj063 Mar 26, 2020

Choose a reason for hiding this comment

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

@singalsu thanks but I was wondering conceptually why it is so

Copy link
Collaborator Author

@singalsu singalsu Mar 26, 2020

Choose a reason for hiding this comment

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

I'd assume HDA DMA can adapt to variable stream format. But I doubt it can do rate conversion or change rate per streaming session. Can you comment @tlauda @mmaka1 ?

Copy link
Member

@lgirdwood lgirdwood Mar 26, 2020

Choose a reason for hiding this comment

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

@singalsu it's best to check as early as possible and base the check on the supported kconfig ASRC settings and topology settings.

Copy link
Collaborator Author

@singalsu singalsu Mar 27, 2020

Choose a reason for hiding this comment

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

The initialization is split to new(), params(), and prepare(). We've had a lot of changes in streaming start and done fixes by moving parts of initialization between these functions. As result (make small easy to review patches) it might not be the most logical any more.

But this issue is not related to Kconfig (where reject for non-supported rate happens now in prepare()).

This is related to topology such way the with the bi-directional pipeline walk the capability of SRC and ASRC to alter downstream parameters via topology provided rate was removed. We get now that information from SSP and DMIC DAI types. But not from HDA and ALH.

I could ugly fix this by implemeting to SRC components a pipeline parameters override when the stream parameter appear as zero but didn't do it because pipeline should take care of such, not individual components.

Copy link
Collaborator

@ranj063 ranj063 Mar 27, 2020

Choose a reason for hiding this comment

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

@singalsu this makes sense but with thesofproject/linux#1943 this problem shouldnt be seen no? But nevertheless, a check is a good thing to have.

Copy link
Collaborator Author

@singalsu singalsu Mar 30, 2020

Choose a reason for hiding this comment

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

Yes, similar PR is needed for HDA kernel and for both the FW counterpart. Wrong parameters can and will happen in future too so it's useful to have check in component for sane parameters, especially when a crash happens.

return -EINVAL;
}

cd->sink_frames = dev->frames;
cd->source_frames = ceil_divide(dev->frames * cd->source_rate,
cd->sink_rate);
Expand Down
4 changes: 4 additions & 0 deletions src/audio/src/src.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,6 +581,10 @@ static int src_params(struct comp_dev *dev,
/* Set source/sink_rate/frames */
cd->source_rate = sourceb->stream.rate;
cd->sink_rate = sinkb->stream.rate;
if (!cd->sink_rate) {
comp_err(dev, "src_params(), zero sink rate");
return -EINVAL;
Copy link
Member

@lgirdwood lgirdwood Mar 26, 2020

Choose a reason for hiding this comment

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

ditto

Copy link
Collaborator Author

@singalsu singalsu Mar 27, 2020

Choose a reason for hiding this comment

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

Like in a more fuzzy way described above I think this is the best way to handle streaming abort to prevent firmware crash.

When the rate converter filter is initialized in prepare() there's need to know the max frame size. Getting the output rate 0 Hz to fail similarly as request to initialize for 48000 Hz to 45678 Hz is not less code than this.

}

cd->source_frames = dev->frames * cd->source_rate / cd->sink_rate;
cd->sink_frames = dev->frames;
Expand Down