dai: Remove frame_fmt from private data by ktrzcinx · Pull Request #3174 · thesofproject/sof

ktrzcinx · 2020-07-14T07:35:09Z

This value is one of audio_stream property, so should be
saved in this place to keep data synchronised, and allow
to usage generic function working on audio_stream, like
stream converters.

Signed-off-by: Karol Trzcinski karolx.trzcinski@linux.intel.com

ktrzcinx · 2020-07-14T08:44:13Z

SOFCI TEST

This value is always set in dai_new() function, value 0 represents SOF_IPC_FRAME_S16_LE so such an check doesn't give any value. Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>

mmaka1 · 2020-07-14T11:48:01Z

Simple oneliner audio_stream_override_frame_fmt() could be helpful (1/ easier to grep, 2/ improves encapsulation).
Edit: Or a one on the buffer api level (with consistent force_update flag handling).

Another solution would be to create a local copy of params, override frame_fmt and just set with a single audio-stream API call.

lgirdwood · 2020-07-14T15:40:16Z

What's the rational for all the buffer/stream changes today ?

ktrzcinx · 2020-07-15T10:07:19Z

What's the rational for all the buffer/stream changes today ?

After #3146 I took a closer look to dma_buffers usage, and I saw that there are sometimes treated as usual audio_stream buffers (pcm_converter, invalidation, writebacks) and sometimes like special, dma buffer, without any special data type (unset stream parameters like fmt, rate, channels). This is quite error prone. Moreover audio_stream is newer than host/dai implementation, so there are places where audio_stream API may be used, instead doing the same thing manually.

ktrzcinx · 2020-07-15T10:15:04Z

Another solution would be to create a local copy of params, override frame_fmt and just set with a single audio-stream API call.

Then new API function with single user won't be needed (buffer_set_frame_fmt()), updated to this version

plbossart · 2020-07-15T17:03:00Z

What's the rational for all the buffer/stream changes today ?
After #3146 I took a closer look to dma_buffers usage, and I saw that there are sometimes treated as usual audio_stream buffers (pcm_converter, invalidation, writebacks) and sometimes like special, dma buffer, without any special data type (unset stream parameters like fmt, rate, channels). This is quite error prone. Moreover audio_stream is newer than host/dai implementation, so there are places where audio_stream API may be used, instead doing the same thing manually.

it may be error prone, but by the same token we should not assume that we know how many channels a DMA buffer contains. It could contain binary data or compressed data unrelated to PCM, or compressed data padded to fit the PCM rate.

DMAs are general purpose tools to move data from point A to point B, we don't want to assume we only transfer PCM.

ktrzcinx · 2020-07-16T08:08:27Z

I agree, from DMA point of view, it doesn't matter what is the format of a data, and here such an information should be useless. Looking from pcm_converter point of view, stream metadata should be set (it's general purpose tool, not DMA specific).
So we need to have a point where binary data from DMA becomes an audio_stream, it should be done before pcm_converter usage. So far, this point is hidden somewhere in host component, during compressed audio stream implementation, it will be defined explicit, but it's not now.

paulstelian97 · 2020-07-16T10:02:14Z

I agree, from DMA point of view, it doesn't matter what is the format of a data, and here such an information should be useless. Looking from pcm_converter point of view, stream metadata should be set (it's general purpose tool, not DMA specific).
So we need to have a point where binary data from DMA becomes an audio_stream, it should be done before pcm_converter usage. So far, this point is hidden somewhere in host component, during compressed audio stream implementation, it will be defined explicit, but it's not know.

Can't the host component be adapted to be more flexible and to allow for raw data to exit it whenever adequate? As in being able to set as params the format to "raw", some reserved value for raw data. And the decoder component would take in raw and push out proper PCM.

plbossart · 2020-07-16T15:47:20Z

I agree, from DMA point of view, it doesn't matter what is the format of a data, and here such an information should be useless. Looking from pcm_converter point of view, stream metadata should be set (it's general purpose tool, not DMA specific).
So we need to have a point where binary data from DMA becomes an audio_stream, it should be done before pcm_converter usage. So far, this point is hidden somewhere in host component, during compressed audio stream implementation, it will be defined explicit, but it's not know.

It seems we are in agreement on the basics, but your comment about the host component threw me off. This PR changes the DAIs,

paulstelian97 · 2020-07-16T21:49:46Z

I agree, from DMA point of view, it doesn't matter what is the format of a data, and here such an information should be useless. Looking from pcm_converter point of view, stream metadata should be set (it's general purpose tool, not DMA specific).
So we need to have a point where binary data from DMA becomes an audio_stream, it should be done before pcm_converter usage. So far, this point is hidden somewhere in host component, during compressed audio stream implementation, it will be defined explicit, but it's not know.
It seems we are in agreement on the basics, but your comment about the host component threw me off. This PR changes the DAIs,

I chimed in with the host component because the DAI itself should always have PCM data (well, almost always; some DAIs can support other formats but it's not typical). Never compressed data. Sure, the DAI must have its own configuration readily available for the params propagation.

plbossart · 2020-07-16T23:09:55Z

I agree, from DMA point of view, it doesn't matter what is the format of a data, and here such an information should be useless. Looking from pcm_converter point of view, stream metadata should be set (it's general purpose tool, not DMA specific).
So we need to have a point where binary data from DMA becomes an audio_stream, it should be done before pcm_converter usage. So far, this point is hidden somewhere in host component, during compressed audio stream implementation, it will be defined explicit, but it's not know.
It seems we are in agreement on the basics, but your comment about the host component threw me off. This PR changes the DAIs,
I chimed in with the host component because the DAI itself should always have PCM data (well, almost always; some DAIs can support other formats but it's not typical). Never compressed data. Sure, the DAI must have its own configuration readily available for the params propagation.

It's perfectly possible to send/receive data as pseudo-PCM but the contents is packetized and could be variable bitrate internally.
People have abused isochronous interfaces big time in the past, and will continue doing so...
There's also cases where audio was sent on SPI, e.g. for bursty data or to flush a history buffer.
Don't confuse transmission/link and contents, they can differ.

Each stream should have set valid metadata information. Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>

This value is one of audio_stream property, so should be saved in this place to keep data synchronised, and allow to usage generic function working on audio_stream, like stream converters. Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>

lgirdwood · 2020-07-21T14:20:33Z

CI clipping. @aiChaoSONG difficult to tell from sof-logger and topology output which PGA is > 0dB
https://sof-ci.01.org/sofpr/PR3174/build6644/devicetest/

[ 98762163.802083] ( 1635.937500) c0 pga 3.20 ../audio/volume/volume.c:641 volume_ctrl_get_cmd(), channel = 0, value = 65536
[ 98762219.010417] ( 55.208332) c0 pga 3.20 ../audio/volume/volume.c:641 volume_ctrl_get_cmd(), channel = 1, value = 65536
[ 98764363.072917] ( 2144.062500) c0 pga 2.13 ../audio/volume/volume.c:641 volume_ctrl_get_cmd(), channel = 0, value = 65536
[ 98764418.281250] ( 55.208332) c0 pga 2.13 ../audio/volume/volume.c:641 volume_ctrl_get_cmd(), channel = 1, value = 65536
[ 98766301.354167] ( 1883.072876) c0 pga 1.3 ../audio/volume/volume.c:641 volume_ctrl_get_cmd(), channel = 0, value = 65536
[ 98766356.562500] ( 55.208332) c0 pga 1.3 ../audio/volume/volume.c:641 volume_ctrl_get_cmd(), channel = 1, value = 65536
[ 98768245.885417] ( 1889.322876) c0 pga 1.2 ../audio/volume/volume.c:641 volume_ctrl_get_cmd(), channel = 0, value = 65536
[ 98768301.093750] ( 55.208332) c0 pga 1.2 ../audio/volume/volume.c:641 volume_ctrl_get_cmd(), channel = 1, value = 65536
[102194911.302083] ( 3426610.250000) c0 pga 1.2 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 0, value = 65536
[102194976.822917] ( 65.520836) c0 pga 1.2 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 1, value = 65536
[102258245.312500] ( 63268.488281) c0 pga 1.3 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 0, value = 65536
[102258310.833333] ( 65.520836) c0 pga 1.3 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 1, value = 65536
[102366153.697917] ( 107842.867188) c0 pga 2.13 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 0, value = 413542
[102366219.635417] ( 65.937500) c0 pga 2.13 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 1, value = 413542
[102370531.927083] ( 4312.291504) c0 pga 3.20 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 0, value = 65536
[102370597.447917] ( 65.520836) c0 pga 3.20 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 1, value = 65536
[148939389.635417] ( 46568792.000000) c0 pga 1.2 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 0, value = 65536
[148939455.156250] ( 65.520836) c0 pga 1.2 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 1, value = 65536
[148985572.604167] ( 46117.449219) c0 pga 1.3 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 0, value = 65536
[148985638.125000] ( 65.520836) c0 pga 1.3 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 1, value = 65536
[149086056.406250] ( 100418.281250) c0 pga 2.13 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 0, value = 413542
[149086122.343750] ( 65.937500) c0 pga 2.13 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 1, value = 413542
[149184145.468750] ( 98023.125000) c0 pga 3.20 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 0, value = 65536
[149184210.989583] ( 65.520836) c0 pga 3.20 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 1, value = 65536

It looks like sof-logger is merging PGA 1.3 into 2.13 ??? Fwiw topology graph only shows PGA 1.0, 1.1, 2.0 and 3.0. So we have a +0dB gain still in some topology tests.

lgirdwood · 2020-07-21T14:24:06Z

@plbossart any comments with the update for compressed streams ?

plbossart · 2020-07-21T18:15:58Z

@plbossart any comments with the update for compressed streams ?

There was no answer to my comments and I don't see any changes in the code?

aiChaoSONG · 2020-07-22T02:16:41Z

CI clipping. @aiChaoSONG difficult to tell from sof-logger and topology output which PGA is > 0dB
https://sof-ci.01.org/sofpr/PR3174/build6644/devicetest/

[ 98762163.802083] ( 1635.937500) c0 pga 3.20 ../audio/volume/volume.c:641 volume_ctrl_get_cmd(), channel = 0, value = 65536
[ 98762219.010417] ( 55.208332) c0 pga 3.20 ../audio/volume/volume.c:641 volume_ctrl_get_cmd(), channel = 1, value = 65536
[ 98764363.072917] ( 2144.062500) c0 pga 2.13 ../audio/volume/volume.c:641 volume_ctrl_get_cmd(), channel = 0, value = 65536
[ 98764418.281250] ( 55.208332) c0 pga 2.13 ../audio/volume/volume.c:641 volume_ctrl_get_cmd(), channel = 1, value = 65536
[ 98766301.354167] ( 1883.072876) c0 pga 1.3 ../audio/volume/volume.c:641 volume_ctrl_get_cmd(), channel = 0, value = 65536
[ 98766356.562500] ( 55.208332) c0 pga 1.3 ../audio/volume/volume.c:641 volume_ctrl_get_cmd(), channel = 1, value = 65536
[ 98768245.885417] ( 1889.322876) c0 pga 1.2 ../audio/volume/volume.c:641 volume_ctrl_get_cmd(), channel = 0, value = 65536
[ 98768301.093750] ( 55.208332) c0 pga 1.2 ../audio/volume/volume.c:641 volume_ctrl_get_cmd(), channel = 1, value = 65536
[102194911.302083] ( 3426610.250000) c0 pga 1.2 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 0, value = 65536
[102194976.822917] ( 65.520836) c0 pga 1.2 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 1, value = 65536
[102258245.312500] ( 63268.488281) c0 pga 1.3 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 0, value = 65536
[102258310.833333] ( 65.520836) c0 pga 1.3 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 1, value = 65536
[102366153.697917] ( 107842.867188) c0 pga 2.13 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 0, value = 413542
[102366219.635417] ( 65.937500) c0 pga 2.13 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 1, value = 413542
[102370531.927083] ( 4312.291504) c0 pga 3.20 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 0, value = 65536
[102370597.447917] ( 65.520836) c0 pga 3.20 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 1, value = 65536
[148939389.635417] ( 46568792.000000) c0 pga 1.2 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 0, value = 65536
[148939455.156250] ( 65.520836) c0 pga 1.2 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 1, value = 65536
[148985572.604167] ( 46117.449219) c0 pga 1.3 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 0, value = 65536
[148985638.125000] ( 65.520836) c0 pga 1.3 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 1, value = 65536
[149086056.406250] ( 100418.281250) c0 pga 2.13 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 0, value = 413542
[149086122.343750] ( 65.937500) c0 pga 2.13 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 1, value = 413542
[149184145.468750] ( 98023.125000) c0 pga 3.20 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 0, value = 65536
[149184210.989583] ( 65.520836) c0 pga 3.20 ../audio/volume/volume.c:557 volume_ctrl_set_cmd(), channel = 1, value = 65536

It looks like sof-logger is merging PGA 1.3 into 2.13 ??? Fwiw topology graph only shows PGA 1.0, 1.1, 2.0 and 3.0. So we have a +0dB gain still in some topology tests.

@keqiaozhang could you please help to check?

ktrzcinx · 2020-07-22T09:27:13Z

@plbossart I'm not sure what you would like to know.
This PR moves stream metadata information (mainly frame_fmt) storage place from DAI component to audio_stream (not DMA), do you think it's bad direction ?
This audio_stream is used by DMA, but updated data fields are not used there, so from DMA point of view nothing has changed.
When compressed audio_stream will be passed through DMA, then DMA won't care whats inside, as it is right now (before and after my changes).

plbossart · 2020-07-22T16:58:41Z

@plbossart I'm not sure what you would like to know.
This PR moves stream metadata information (mainly frame_fmt) storage place from DAI component to audio_stream (not DMA), do you think it's bad direction ?
This audio_stream is used by DMA, but updated data fields are not used there, so from DMA point of view nothing has changed.
When compressed audio_stream will be passed through DMA, then DMA won't care whats inside, as it is right now (before and after my changes).

I was referring to this code: ba6a21a#diff-9f8de02ec82000817dd1a9bbccac158cR76

this still has DAIs handling samples, and it's rather odd to me.

ktrzcinx · 2020-07-22T17:55:19Z

As I understand you are talking about removing calculation of number of samples from dai_dma_cb.
It wasn't added in this PR, but this one is one step forward to make it possible.
In target code it may be something like:

dma_buffer_copy_to(dd->local_buffer, dd->dma_buffer, bytes,
dd->decompress_data);

and

void dma_buffer_copy_to(struct comp_buffer *source, struct comp_buffer *sink, uint32_t source_bytes,
struct converter_data *decompress)
{
struct audio_stream *ostream = &sink->stream;
int sink_bytes;
buffer_invalidate(source, source_bytes);
/* decompress data */
sink_bytes = converter_process(&source->stream, 0, ostream, 0, source_bytes, decompress);
if (sink_bytes < 0)
// errro handling
/* sink buffer contains data meant to copied to DMA */
audio_stream_writeback(ostream, sink_bytes);
ostream->w_ptr = (char *)ostream->w_ptr + sink_bytes;
ostream->w_ptr = audio_stream_wrap(ostream, ostream->w_ptr);
comp_update_buffer_consume(source, source_bytes);
}

where pcm_converter will be called internally in decompress_process (when needed). But as I said it before, this PR is not going to provide full compressed stream support, this one is only preparation for it, only one step forward.

plbossart · 2020-07-22T18:03:16Z

SOFCI TEST

ktrzcinx · 2020-07-22T18:05:29Z

Thanks @plbossart

ktrzcinx requested review from dbaluta, jajanusz, lbetlej, lgirdwood, mmaka1, plbossart and tlauda as code owners July 14, 2020 07:35

paulstelian97 approved these changes Jul 14, 2020
View reviewed changes

ktrzcinx mentioned this pull request Jul 14, 2020
Dma use audio stream API #3162
Merged

ktrzcinx force-pushed the dai-remove-fmt branch 2 times, most recently from d3a7aa9 to 67c7a78Compare July 14, 2020 09:45

dbaluta approved these changes Jul 14, 2020
View reviewed changes

ktrzcinx force-pushed the dai-remove-fmt branch from 67c7a78 to d49fadbCompare July 14, 2020 10:23

mmaka1 reviewed Jul 14, 2020
View reviewed changes

Comment threadsrc/audio/dai.c Outdated

mmaka1 reviewed Jul 14, 2020
View reviewed changes

Comment threadsrc/audio/dai.c Outdated

dai: Always overwrite hw frame_fmt in dai_comp_get_hw_params
0b988bd
This value is always set in dai_new() function, value 0 represents SOF_IPC_FRAME_S16_LE so such an check doesn't give any value. Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>

mmaka1 reviewed Jul 14, 2020
View reviewed changes

Comment threadsrc/audio/dai.c Outdated

ktrzcinx force-pushed the dai-remove-fmt branch 2 times, most recently from 8d1b9fb to d7af546Compare July 14, 2020 12:36

ktrzcinx force-pushed the dai-remove-fmt branch from d7af546 to df4f5eaCompare July 15, 2020 10:13

mmaka1 approved these changes Jul 15, 2020
View reviewed changes

ktrzcinx added 2 commits July 17, 2020 16:16

dai: Assign dma_stream params
db510a4
Each stream should have set valid metadata information. Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>

ktrzcinx force-pushed the dai-remove-fmt branch from df4f5ea to ba6a21aCompare July 17, 2020 14:17

lgirdwood approved these changes Jul 21, 2020
View reviewed changes

plbossart approved these changes Jul 22, 2020
View reviewed changes

jajanusz merged commit 60211b3 into thesofproject:masterJul 23, 2020

aiChaoSONG mentioned this pull request Dec 25, 2020
[IPC error] sof-test/multiple-pause-resume.sh reported error when dma trace was disabled on TGL #3673
Closed

Uh oh!

Conversation

ktrzcinx commented Jul 14, 2020

Uh oh!

ktrzcinx commented Jul 14, 2020

Uh oh!

Uh oh!

Uh oh!

mmaka1Jul 14, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktrzcinxJul 14, 2020

Choose a reason for hiding this comment

Uh oh!

mmaka1Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lgirdwood commented Jul 14, 2020

Uh oh!

ktrzcinx commented Jul 15, 2020

Uh oh!

ktrzcinx commented Jul 15, 2020

Uh oh!

plbossart commented Jul 15, 2020

Uh oh!

ktrzcinx commented Jul 16, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulstelian97 commented Jul 16, 2020

Uh oh!

plbossart commented Jul 16, 2020

Uh oh!

paulstelian97 commented Jul 16, 2020

Uh oh!

plbossart commented Jul 16, 2020

Uh oh!

lgirdwood commented Jul 21, 2020

Uh oh!

lgirdwood commented Jul 21, 2020

Uh oh!

plbossart commented Jul 21, 2020

Uh oh!

aiChaoSONG commented Jul 22, 2020

Uh oh!

ktrzcinx commented Jul 22, 2020

Uh oh!

plbossart commented Jul 22, 2020

Uh oh!

ktrzcinx commented Jul 22, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart commented Jul 22, 2020

Uh oh!

ktrzcinx commented Jul 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

mmaka1Jul 14, 2020•
edited
Loading

ktrzcinx commented Jul 16, 2020•
edited
Loading

ktrzcinx commented Jul 22, 2020•
edited
Loading