[bkokoszx]

This commit simplifies mux processing functions and
does not allow mux to mix channels.

Signed-off-by: Bartosz Kokoszko bartoszx.kokoszko@linux.intel.com

[lgirdwood]

Please see my comments here #2959

The mux n:1 and demux 1:n mapping should be worked out during prepare so that an array is built that contains all the copy mappings. e.g.

/* stores all copy src/dest with increments - built during prepare() */
struct copy_elem {
    void * src;
    int src_increment;
    void *dest;
    int dest_increment;
};

/* copy - check for overflow BEFORE copy and alter num_copies to fit */
for (i = 0; i < num_copies; i++) {
    for (j = 0; j < num_elems; j++) {
        memcpy(elem[j].dest, elem[j].src, size); // memcpy used as example, but do this directly to save func call
        elem[j].dest += elem[j].dest_increment;
        elem[j].src += elem[j].src_increment;
    }
}

This greatly simplifies the copy() and will reduce the MPCS considerably too (we are currently 70MCPS, I would expect to be < 10).

[lgirdwood]

debug ?

[bkokoszx]

Yes, just removed it.

[lgirdwood]

not following the comment, we should not be mixing ?

[bkokoszx]

Maybe my comment was not clear. Basically, I'm checking here whether user sends to us proper config. If user sends a configurations requiring channels mixing, mux returns an error.

[lgirdwood]

Sorry, still dont understand the comment.

[lgirdwood]

What is this used for ?

[bkokoszx]

In order to create look up table. I utilized it mux/demux process functions.

[bkokoszx]

@lgirdwood
Today I do not have time to work on that PR. mux_look_up_entry was created in order to make look up table to simplify copy() function (I've measured processing and it is about 20 MCPS with this PR). I have one question regarding your comment: are we able to create fully precise copy mapping in prepare()? I mean that we should also check in copy(), which from out input/output are active. Or do you want to make copy_elem array separately for different input streams?

[lgirdwood]

Almost good. We need to stay away from audio_stream_write_frag_s32() etc as they all will do a wrap check for every frame copied. This needs to be lower level where we calculate the copy_elems during prepare and pre-calculate wrap positions before each copy block (rather than calculating for each frame). With these changes we can probably go below 10 MPCS

[lgirdwood]

can we have an inline comment on what each loop block is doing

[lgirdwood]

ditto

[lgirdwood]

This is in the right direction, but we should go down to a lower level which is far simpler

/* copy element */
struct copy_elem {
    void *dest;
    void *src;
    uint32_t dest_inc;  /* increment size for dest pointer when elem is next copied */
    uint32_t src_inc;  /* increment size for src pointer when elem is next copied */
    bool dest_wrap; /* dest needs reset after current frame block has been copied */
    bool src_wrap; /* src needs reset after current frame block has been copied */
}

int copy_s16(int num_frames)
{
   while (num_frames) {

        /* calculate mux number of frames that can be copied without overflow - set src_wrap and dest_wrap in copy_elems*/
        frames = calc_max_frames();

        /* copy directly since wrap has been pre-computed */
        for (i = 0; i < frames; i++) {
            for (j = 0; j < num_copy_elems; j++) {
                 *(int_16_t*)(elem[j].dest) = *(int16_t*)(elem[j].src);
                  elem[j].dest += elem[j].dest_inc;
                  elem[j.src += elem[j].src_inc;
            }

         /* check for wrap - loop copy_elems and reset buffers if src_wrap or dest_wrap is true */
        check_for_wrap();

         /* decrement frames */
        num_frames -= frames;
    }
}

[bkokoszx]

@lgirdwood
I've updated this PR due to your requirements (I will fix checkpatch, comments, unit tests later). I have few things/concerns:

1. I do not prepare whole look up table in prepare(). In process() I have to init *src and *dest pointers, because in prepare() basically they are not current.
2. I've tested simple mux/demux scenarious. Before merging I will ask our validation team to perform stress tests.
3. Mux/Demux performance is about ~10 MIPS at 48 kHz 8 32/32 format ATM.
4. My main concern is that this algorithm (I mean calculation available frames without wrapping pointers) will work only when buffer is multiple of frame bytes. Can we make that assumption?

[juimonen]

@bkokoszx @lgirdwood Sorry I'm late for this one, but could you still do this simplification so, that we would leave the "old mux" as it is as processing/effect component (could be optimized though) and make the "simplified version" as asoc-component (which by nature defines simplified mux).

I'm just 99% sure that within following 6 months we will need the original mux/demux capabilities somewhere and then it is lost.... I don't see problems if we would have both components? We could rename the original mux/demux to something else if it is confusing...

[bkokoszx]

@juimonen
I agree that we should store "old" mux in some way.
@lgirdwood
What do you think?

[lgirdwood]

I've updated this PR due to your requirements (I will fix checkpatch, comments, unit tests later). I have few things/concerns:

1. I do not prepare whole look up table in prepare(). In process() I have to init *src and *dest pointers, because in prepare() basically they are not current.

Ok, I guess the step sizes are known for each elem and that it.

2. I've tested simple mux/demux scenarious. Before merging I will ask our validation team to perform stress tests.
3. Mux/Demux performance is about ~10 MIPS at 48 kHz 8 32/32 format ATM.
4. My main concern is that this algorithm (I mean calculation available frames without wrapping pointers) will work only when buffer is multiple of frame bytes. Can we make that assumption?

Yes

struct audio_stream {
	/* runtime data */
	uint32_t size;	/**< Runtime buffer size in bytes (period multiple) */

[lgirdwood]

Almost there, I still think we are doing too much in the frame calc function loops (as stated and also converting to frames in the loop), usage of uint_8 wont help either. I think this will go under 10MCPS if we can improve the frame calc functions.

[lgirdwood]

ok, this looks more efficient. Btw, why do we always have lookup[0] ?

[bkokoszx]

@lgirdwood
I've changed using of look up table to loookup->copy_elem (lookup[0] could be confusing). Basically, we invoke process function with one look up table (We have one look up table for each sink - mux has one look up table (one sink), demux can have several look ups (several sinks)). For mux we invoke process function once:

cd->mux(dev, &sink->stream, &sources_stream[0], frames, &cd->active_lookup);

For demux we invoke process several times (depends on sink amount) with corresponding look up table:

/* produce output, one sink at a time */
	for (i = 0; i < MUX_MAX_STREAMS; i++) {
		if (!sinks[i])
			continue;

		buffer_invalidate(source, source_bytes);
		cd->demux(dev, &sinks[i]->stream, &source->stream, frames,
			  look_ups[i]);
		buffer_writeback(sinks[i], sinks_bytes[i]);
	}

[lgirdwood]

please just use 32bit loop counters, the 8bit assembly on xtensa is less efficient compared to 32bit

[bkokoszx]

@lgirdwood
I've updated it.

[lgirdwood]

Can you explain what this does inline. My expectation for demux is that source bytes would always be the same for each elem ? sink bytes would always be the same for mux ?

[bkokoszx]

@lgirdwood
Do you mean: min_frames = frames?
Firstly we calculate available frames in e.g. demux_copy():

for (i = 0; i < MUX_MAX_STREAMS; i++) {
		if (!sinks[i])
			continue;
		buffer_lock(sinks[i], &flags);
		avail = audio_stream_avail_frames(&source->stream,
						  &sinks[i]->stream);
		frames = MIN(frames, avail);
		buffer_unlock(sinks[i], flags);
	}

And we do not want in calc_frames_without_wrap() function, that frames_without_wrap will exceed available frames (frames_without_wrap calculation is based on buffer end address).

[lgirdwood]

I think this is over complex. Ideally we need a stream API that allows us to get avail/free bytes to wrap.

static inline int audio_stream_get_avail_nowrap()
{
    if (buffer->r_ptr > buffer->w_ptr)
        return buffer->end - buffer->r_ptr;
    else
       return buffer->w_ptr - buffer->r_ptr;
} 

demux_calc_frames()
{
    /* get source bytes */
    source_bytes = audio_stream_get_avail_nowrap();

   /* get min sink bytes */
    for (elem) {
        sink_bytes = audio_stream_get_free_nowrap();
         if (sink_bytes < min_sink_bytes)
             min_sink_bytes = sink_bytes;
    }

    /* get min bytes */
    min = source_bytes < min_sink_bytes ? source_bytes : min_sink_bytes;

    /* convert to frames */
    return bytes_to_frames(min);
}

[bkokoszx]

@lgirdwood
Ok, I will work on it, but I have one concern regarding this common API. Basically, in calc_frames_without_wrap() our calculations are based on copy_elem w/r_ptr's (not directly buffer read and write pointers):

size_to_end = (char *)source->end_addr -(char *)lookup->copy_elem[elem].src;

, so yours audio_stream_get_avail_nowrap() example will not work, because it uses buffer write/read pointers directly. We updated buffer w/r_ptr's at the and of copy() functions:

	/* update components */
	for (i = 0; i < MUX_MAX_STREAMS; i++) {
		if (!sinks[i])
			continue;
		comp_update_buffer_produce(sinks[i], sinks_bytes[i]);
	}
	comp_update_buffer_consume(source, source_bytes);

	return 0;
}

[bkokoszx]

I've just seen that we have something like:

static inline int
audio_stream_bytes_without_wrap(const struct audio_stream *source,
				const void *ptr)

in audio_stream.h

[lgirdwood]

That sounds like the one to use. Do you have any new MCPS numbers ?

[bkokoszx]

@lgirdwood
I have modified mux/demux_calc_frames_without_wrap a little bit. I've added inline audio_stream_frames_without_wrap() function. I've measured MCPS:

• mux - 48 kHz 8 channels 32/32 with 2 input streams ~8500 MCPS:
  [ 2912365.750000] ( 13988.833008) c0 mux 4.10 src/audio/pipeline.c:206 perf comp_copy peak plat 508 cpu 8501

• demux - 48 kHz 8 channels 32/32 with 2 output streams ~9200 MCPS
  [ 1159658.916667] ( 28.208334) c0 demux 5.15 src/audio/pipeline.c:206 perf comp_copy peak plat 550 cpu 9182

BTW before merging I will ask our validation for stress tests.

[lgirdwood]

@bkokoszx great work ! that's alsmost and order of magnitude faster than before. Ping me when stress tests done and I'll do a final review.

[lgirdwood]

@bkokoszx @lgirdwood Sorry I'm late for this one, but could you still do this simplification so, that we would leave the "old mux" as it is as processing/effect component (could be optimized though) and make the "simplified version" as asoc-component (which by nature defines simplified mux).

I'm just 99% sure that within following 6 months we will need the original mux/demux capabilities somewhere and then it is lost.... I don't see problems if we would have both components? We could rename the original mux/demux to something else if it is confusing...

@juimonen This can still all be done with a mixer and a mux in series and is far simpler. A component should only do one thing.

[lgirdwood]

Can you check the [0] usage, if it's indended please use a inline comment next to each one explaining why.

[lgirdwood]

Almost there, I assume we are DNM status due to pending validation ?

[bkokoszx]

@lgirdwood
Yes, there was problem with echo ref tests, but I think we resolve it. I will confirm on Monday.

[lgirdwood]

SOFCI TEST

[lgirdwood]

@bkokoszx @zrombel any idea why the CI is failing on ICL for unrelated tests ?

[bkokoszx]

@lgirdwood
Now it only fails on mux CI test what is expected, because our CI mux tests allows mixing (we should update tests just after merging this PR).

@zrombel made stress tests and we saw one panic on mux simple 24 24/32 1ch format - I will look into it.

[bkokoszx]

@lgirdwood
@zrombel ran tests with this PR and slim driver and now it looks correct, but please do not merge it until we prepare update fo MUX CI tests (now they assume mixing by mux).

[lgirdwood]

@bkokoszx thanks, that's great. Please let me know when it's good to merge !

[zrombel]

Python tests are ready, so PR can be merged.