Skip to content

Conversation

@slawblauciak
Copy link
Collaborator

@slawblauciak slawblauciak commented Mar 30, 2020

This allows the firmware to become aware of the programmed format on ALH DAIs.
The change is backwards compatible, in-case of ABI mismatch, the data will be filled with 0s,
which is consistent with previous values.

This change is required in the case of pipelines, that contain components capable
of modifying the parameters of a stream, for example (de)mux.

Signed-off-by: Slawomir Blauciak slawomir.blauciak@linux.intel.com

Copy link

@mmaka1 mmaka1 left a comment

Choose a reason for hiding this comment

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

The commit message says nothing about how this impacts the FW behavior and does not explain what is the reason of this change.

@slawblauciak
Copy link
Collaborator Author

Changes applied. Updated commit and PR descriptions.

Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

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

Not entirely convinced that this proposal addresses all cases of demux configurations?

@slawblauciak
Copy link
Collaborator Author

slawblauciak commented Mar 31, 2020

For clarification, this change doesn't have anything to do with demux. It merely allows components that modify the stream parameters to work correctly. It does not provide any unique information to demux, rather, it provides essential information about what is expected on the output of the DAI.

@slawblauciak
Copy link
Collaborator Author

Changes applied.

@plbossart
Copy link
Member

@slawblauciak I think you missed my point
the demux is configured statically with the topology.
The dailink stream params may or may not be aligned with what you specify in the topology for demux.
Specifically if your demux only takes one channel for left and one channel for right amps, you will have an issue here. The dai configuration would be 2 channels but you expect 1 for ALH.

So rather than passing the params from the dai, what I am asking is whether the rate and channels should come from topology. That way you can guarantee that the configurations of demux and ALH are compatible.

@slawblauciak
Copy link
Collaborator Author

@plbossart I think I understand now. Okay. But doesn't that ultimately shift the issue somewhere else?
Because then instead of having a risk of the demux blob in the topology being incompatible with the factual DAI format, we end up with a risk of the hardcoded DAI format in the topology being incompatible with what is actually programmed on the DAI.
I think either way we need to ensure that the formats in the topology are correct given the actual DAI configuration during run-time.

Regardless, the demux should be able to detect the mismatch in configuration and should error out.

@plbossart
Copy link
Member

@slawblauciak: @bardliao already pushed a branch that takes the information from topology in thesofproject/linux#1943 I think the only missing part is that there would be 2 IPCs now, one on topology load and one during hw_params, maybe we could pass all the valid information during the hw_params state only? Or you would have to discard the missing info during the hw_params state.

You are right that the topology is assumed to be correct, same as an SSP configuration using bclk rates that makes sense. Here the demux could make sure the upstream and downstream rates are the same, and the channel configuration is compatible.

@bardliao
Copy link
Collaborator

I think the only missing part is that there would be 2 IPCs now, one on topology load and one during hw_params, maybe we could pass all the valid information during the hw_params state only? Or you would have to discard the missing info during the hw_params state.

@plbossart Yes, I discussed with @slawblauciak and we will discard the info if both rate and channels are 0 since it is easier than getting topology tokens during hw_params

This allows the firmware to become aware of
the programmed format on ALH DAIs.

The change is backwards compatible,
in-case of ABI mismatch, the data will be filled with 0s,
which is consistent with previous values.

This change is required in the case of pipelines,
that contain components capable
of modifying the parameters of a stream, for example (de)mux.

Signed-off-by: Slawomir Blauciak <slawomir.blauciak@linux.intel.com>
@slawblauciak
Copy link
Collaborator Author

Hm, I force pushed but the PR is not updating...

@slawblauciak slawblauciak force-pushed the alh_hw_info branch 3 times, most recently from 1c98e20 to 682da3a Compare March 31, 2020 12:57
Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

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

Thanks @slawblauciak and @bardliao, seems fine now.

@slawblauciak
Copy link
Collaborator Author

Hm, tests are failing since last forcepushes. Gotta check that out

@slawblauciak
Copy link
Collaborator Author

SOFCI TEST

@slawblauciak
Copy link
Collaborator Author

CI tests fail, because Python tests need to be updated. Seems like they didn't fill the reserved fields with 0s, causing some param conflicts.

@slawblauciak
Copy link
Collaborator Author

Actually, it was a problem with params not being initialized by default. So, I always take the first config batch, even if it's set to 0.

@slawblauciak
Copy link
Collaborator Author

SOFCI TEST

Copy link
Collaborator

@bardliao bardliao left a comment

Choose a reason for hiding this comment

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

The if (rate || channels) test should be done in alh_set_config()

This provides basic guidelines to clear up how to allocate
and use DAI private data.

Signed-off-by: Slawomir Blauciak <slawomir.blauciak@linux.intel.com>
Copy link
Collaborator

@bardliao bardliao left a comment

Choose a reason for hiding this comment

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

Thanks @slawblauciak

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ABI ABI change involved

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants