Why cant we align indexes to windows version? @singalsu @juimonen This will only introduce unnecessary workarounds

Ack, we need to align the IDs. @ujfalusi whats the impact here for upstream kernel ?

I feel like the original architect idea was to use dmic instance index starting from bit 5. However, seems close source ACE FW implemented it by just using entire v_index as dmic index. And so this is how it is used on Windows in driver and close source FW.

yes so we are currently setting the dai_index in the kernel driver into bits 5, 6 and 7 (as the closed source dmic struct seems to indicate), then fetching the value from those bits in fw. So the solution is to remove that shift/mask from both sides kernel/fw dmic dai_index processing.

Adding @mwasko @mmaka1 for more context.

After reviewing closed source fw it seems that it is expected from code perspective to get 0 or 1 in node_id v_index
but on the other hand there are structures with clear comments that v_index for DMIC contains several fields and we can get dmic instance by shifting 5 bits just like in PR 6668 so actually it seems more like a bug in closed source fw

After reviewing closed source fw it seems that it is expected from code perspective to get 0 or 1 in node_id v_index but on the other hand there are structures with clear comments that v_index for DMIC contains several fields and we can get dmic instance by shifting 5 bits just like in PR 6668 so actually it seems more like a bug in closed source fw

ok, so are you saying we dont need this PR ?

So I think this was miss-understanding from our side, as closed source defines dai_index bit positions differently for ssp and dmic. for ssp these bit positions are used, but for dmic not (for some historical reasons.). The kernel driver side bit fiddling has been there from March, as we have not tested dmic index 1 until recently, and that's when me and Seppo added the fw side fiddling to get the index back. So if the index is 0, these fiddlings don't make too much difference, but with 1,2,3 they start to make.

So the original fix would have been to remove the fiddling from kernel driver, not to add it to fw. We just didn't thought there would be this illogical usage of the structs (we should have looked into the closed source).

So perhaps the 1) merge this Serhiy's "hot fix" 2) remove the hotfix and related fiddling, when we have kernel available with fiddling removed?

So perhaps the 1) merge this Serhiy's "hot fix" 2) remove the hotfix and related fiddling, when we have kernel available with fiddling removed?

Agree. This way we will have both Windows and Linux platforms working ASAP.

Ok, after discussing this with closed source fw guys it is clear now:

PR #6668 is wrong and needs to be fixed
This workaround is not necessary because kernel (or topology?) needs to be fixed too with correct v_index, how big effort is this? @juimonen @singalsu @lgirdwood ?

@abonislawski so you need to remove the bit fiddling in both kernel and fw, or use this PR as intermediate step. I was just suggesting above that if you want to be windows compatible fast, then use this PR and remove the fiddlings later.

@abonislawski the changes are removing 1 line from kernel driver and fw. Only thing is to sync the kernel (and release).

We are good to merge correct version immediately. In PR 6668 no one was waiting for windows version sync :D

FYI, this is v_index:

struct /Bits/
{
//! Index of the input queue within the DMIC instance.
uint8_t dmic_interface_queue_id : 3;
//! Index of the time slot group within the queue.
uint8_t time_slot_group_index : 2;
//! Index of DMIC instance.
uint8_t instance : 3;
} f; //!< Bits

and dmic_interface_queue_id should be used to determine DMIC FIFO instead of instance as in 6668
instance is always 0 (one DMIC instance)
time_slot_group_index is not used

so we can use first 3 bits (dmic_interface_queue_id ) or whole v_index directly as dai index

@abonislawski yes we have little bit different "philosophical view" in sof where fifos are mapped as dais, because they can be used at the same time. And I guess instance in ssp struct is mapped to ssp index also (bits 4-7), otherwise it would now work?

So I see that this dmic struct is just not used, and that's fine.

Anyway, we need to sync with kernel, otherwise linux will stop working. I can try to make the PR's for those both.

@juimonen yeap, this mapping dmic fifos may be misleading
This depends on individual driver and HW. In SSP actually we have several instances, in DMIC only one.
In SSP field i2s_instance is after 4 bits of time_slot_group_index, so all is okay.

Somebody has to make a decision either (1) we are merging this workaround or (2) reverting #6668. With option (1) we will have both Windows and Linux working, with option (2) DMIC1 will not work on Linux/ChromOS for weeks (DMIC0 will still work). No decision -- we will have non-working DMIC1 on Windows -- not acceptable.

I'm OK with having temporary workaround merged which makes all platforms working well.

We dont have abi verions for such simple case to sync kernel-fw?

Its normal that there can be changes which requires newest kernel and otherwise

Close in favor of #6877.