Repository navigation
Conversation
Wrong directions for the feedback! Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The definitions are similar, only the direction changes. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Need to explicitly have a reference to playback reference Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Multiple errors in the BE pipelines a) invalid comments b) dai type inverted on one of the two files. Playback and capture cannot be the same... The convention is dai_in for capture and dai_out for playback. Additional opens: 1) what the point of declaring formats with 4 channels if the pipeline defines a max at 2? Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
That helps find a couple of nice inversions. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The ibs and obs values are inverted quite a few times. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
copy-paste error with one format using the wrong buffer size. For playback the host copiers need to use ibs for the DMA size. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Make sure the playback and capture host copier have a consistent setup. The only acceptable versions are: playback: copier_type "host" type "aif_in" node_type $HDA_HOST_OUTPUT_CLASS capture: copier_type "host" type "aif_out" node_type $HDA_HOST_INPUT_CLASS Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The convention is dai_in for capture and dai_out for playback. Half of the team got it wrong. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Do we have no test coverage for these? If not, why and how were they submitted? |
| ] | ||
|
|
||
| passthrough-be [ | ||
| passthrough-capture-be [ |
There was a problem hiding this comment.
@plbossart this is not the wrong direction. The original passthrough-be is applicable for both directions but the passthrough-capture-be limits to 32-bit support only
There was a problem hiding this comment.
@ranj063 with all due respect, how on earth can anyone know about such an undocumented "feature"?
There was a problem hiding this comment.
And btw why is this named passthrough in the first place. This should be 'copier-be' or 'generic-be'. The passthrough property depends on how copiers are connected. A copier does not have in itself any passthrough attribute.
| <include/common/audio_format.conf> | ||
| <include/components/copier.conf> | ||
| <include/components/pipeline.conf> | ||
| <include/common/audio_format.conf> |
There was a problem hiding this comment.
is this an unrelated change? the original order is in alphabetical order no?
There was a problem hiding this comment.
it's to align passthrough-capture and passthrough-playback. One of them doesn't follow the convention. I don't care what the convention is, I just want the two files to be aligned.
| ] | ||
|
|
||
| passthrough-be [ | ||
| passthrough-playback-be [ |
There was a problem hiding this comment.
this is wrong @plbossart. Like I mentioned passthrough-be is not only for playback. If you look at the direction below, it is meant for capture. So changing this to playback is wrong
There was a problem hiding this comment.
It's all needlessly complicated. No wonder no one can figure this out.
| Object.Widget { | ||
| copier."1" { | ||
| type dai_in | ||
| node_type $HDA_LINK_OUTPUT_CLASS |
There was a problem hiding this comment.
if you remove the default node_type, I think we should make sure it is set in all top-level conf files first. I think we avoid setting it for HDA
There was a problem hiding this comment.
I don't know how to progress. This is wrong in general. It should never have been set this way, if there is an attribute that is not valid by default across all uses, then it cannot be initialized by default in the base class.
| copier_type "host" | ||
| type "aif_in" | ||
| node_type $HDA_HOST_OUTPUT_CLASS | ||
|
|
There was a problem hiding this comment.
no, this is intentional.
this was to make sure all uses of the copier_type, type and node_type are well isolated to make sure we can detect any differences.
| mixout."1" {} | ||
| copier."1" { | ||
| type dai_in | ||
| type dai_out |
There was a problem hiding this comment.
I am not sure about this one @plbossart . I think you made too many changes in this PR that are not correct and you've got failures left, right & center. It might be a good idea to go step by step and separate the clean ups one by one
There was a problem hiding this comment.
I asked about the conventions, and playback meant dai_out. If this is not correct, I don't know what the rules are any longer.
| out_bit_depth 32 | ||
| out_valid_bit_depth 32 | ||
| dma_buffer_size "$[$obs * 2]" | ||
| dma_buffer_size "$[$ibs * 2]" |
There was a problem hiding this comment.
and oh I forgot t mention, we need to remove dma_buffer_size altogether. This is no longer used by the kernel and it computed based on ibs/obs and stream direction
There was a problem hiding this comment.
goodness. That just proves my point that these files are not properly maintained. We need to collectively revisit all this, it's just not acceptable to be sloppy at this stage.
|
Can one of the admins verify this patch? |
broken CI @lgirdwood |
|
@plbossart any chance you can split this PR up as it could be only 1 or 2 patches that are problematic. i.e. we can apply the fixes that are passing CI and revisiste the failure patches ? |
I'll close this PR and let @ranj063 take over. I broke too many eggs with my changes. |
5 participants
I don't know how we managed to have so many blatant logical inversions and inconsistencies.
dai_in/out
ibs/obs
aif_in/aif_out
capture/playback direction and names
and all of the above.
If it looks bad it's because it is. We need stronger class definitions to avoid such issues.