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

[singalsu]

This can happen with HDA and ALH and avoids a firmware crash.

[singalsu]

PR #2539 kind of add can avoid the issue for HDA. In addition the stream rate need to be set.

[lgirdwood]

Would it be worth while to validate this against supported rates ?

[lgirdwood]

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

[singalsu]

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)

[ranj063]

@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?

[singalsu]

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;
}

[ranj063]

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

[singalsu]

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 ?

[lgirdwood]

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

[singalsu]

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.

[ranj063]

@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.

[singalsu]

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.

[lgirdwood]

ditto

[singalsu]

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.

[ranj063]

if (!cd->sink_rate)?

[singalsu]

Yep, I'll change.

Off topic I wonder what's the rationale for this Linux kernel rule. The compiler can issue the same compare to zero operation when == 0 seen plus it's more readable in my opinion. Is it to avoid accidental if (variable = 0) {} ?

[ranj063]

not sure about the rationale but seems like the universally accepted pattern. @lyakh any insights?

[singalsu]

The just pushed version fixes this.

[tlauda]

SOFCI TEST