Get the device type before query endpoint blob #4667
Conversation
brentlu
requested review from
lgirdwood,
plbossart,
ranj063,
kv2019i,
dbaluta,
bardliao,
ujfalusi,
vijendarmukunda,
kuanhsuncheng and
yaochunhung
as code owners
sound/hda/intel-nhlt.c
Outdated
| int i; | ||
|
|
||
| if (!nhlt) | ||
| return 0; |
There was a problem hiding this comment.
Caller should provide a valid nhlt table. Thanks for pointing this error out.
brentlu
force-pushed
the
ep-devtype-mismatch
branch
from
273c704 to
ea971b8
Compare
|
|
||
| /* find NHLT blob with matching params */ | ||
| cfg = intel_nhlt_get_endpoint_blob(sdev->dev, ipc4_data->nhlt, dai_index, nhlt_type, | ||
| bit_depth, bit_depth, channel_count, sample_rate, | ||
| dir, 0); | ||
| dir, dev_type); |
There was a problem hiding this comment.
Hrm so, this change basically makes the dev_type check kind of bypassed at the end?
What is expected if we have two sets of blobs for the same SSP index but one with dev_type=0 and the other with dev_type=4 in the NHLT blob? We will only going to be able to use the dev_type which is the first in the NHLT.
Is this acceptable and OK?
There was a problem hiding this comment.
Yes correct. In fact, dev_type is useless if we attach only one device to one SSP port.
A SSP port could not be shared between cnvi BT and external codec (not sure if it still holds true on MTL). If the board does share the port between external BT and codec, the problem is the same (you always get the first set of blobs) even using fixed value 0 or 4 for all NHLT blobs. All devices attached to same SSP port need to share same one set of blob or we will get into trouble. That's my understanding.
There was a problem hiding this comment.
Right, so this could be done in nhlt side just to ignore the dev_type?
I don't see how this dev_type have anything to do with the SSP configuration. SSP does not car if there is anything connected to the I2S pins when it is the clock provider, in what way can possibly a mono, 16bit, 44.1KHz differs if the dev_type is BT offload or a codec?
Different FIFO thresholds probably?
Do we have examples of SSP NHLT blobs from BIOS? Does that contains different sets of configurations for the same SSP port per dev_type?
There was a problem hiding this comment.
After a bit of reading I think we are having this wrong when requesting the blob.
The endpoint description describes the endpoint in a given system since I2S bus is not a discoverable bus.
So, you scan the SSP descriptors and you will have these metadata info about the connection, but for us what matters is the LinkType (SSP) Virtual Bus ID (the SSP port), the format that we are looking for and perhaps the direction.
The found descriptor will give additional info (if anyone cares) like the device_type, VendorID, DeviceID, etc.
The dev_type is not a selection parameter but it is a metadata in the NHLT which can be used by some OS, Linux is not using it.
@plbossart, @ranj063, @kv2019i, you know better the NHLT and things around it, do you see it differently?
OK, reading more and it is a bit more complicated than that, but the dev_type feels that it should not be a deciding factor when looking for the configuration of a SSP port.
There was a problem hiding this comment.
The difference device types were used by another OS to load different drivers. Indeed that's not something we use.
The only time we used the device type was to find which SSP was connected to the ES8336 codec. That wasn't to manage the SSP differently but rather select the relevant topology.
The point is that we need to ignore the device type and that's precisely what this PR does.
Add a helper function intel_nhlt_ssp_device_type() to detect the type of specific SSP port. The result is nhlt_device_type enum type which could be NHLT_DEVICE_BT or NHLT_DEVICE_I2S. Signed-off-by: Brent Lu <[email protected]>
sound/soc/sof/ipc4-topology.c
Outdated
| @@ -1357,18 +1358,24 @@ static int snd_sof_get_nhlt_endpoint_data(struct snd_sof_dev *sdev, struct snd_s | |||
| &bit_depth); | |||
| if (ret < 0) | |||
| return ret; | |||
|
|
|||
| /* for SSP, dev type could be 0 (BT sideband) or 4 (analog codec) */ | |||
There was a problem hiding this comment.
I would expand this comment and make it clear that the SSP connection is static and the NHLT have board specific configuration which describes the endpoint dev_type.
The dev_type is not specified in tplg, just the SSP port and this is why we query the dev_type for the port and pass that back to the blob lookup function.
Iow, in SOF we are using the SSP port to find the blob and not the dev_type.
or something around these lines
brentlu
force-pushed
the
ep-devtype-mismatch
branch
from
ea971b8 to
8c3ea43
Compare
| * specified in topology. Here we query the type for the port | ||
| * and then pass that information back to the blob lookup | ||
| * function. | ||
| */ |
There was a problem hiding this comment.
I think this would be cleaner:
/*
* We need to know the type of the external device attached to a SSP
* port to retrieve the blob from NHLT. However, device type is not
* specified in topology.
* Query the type for the port and then pass that information back
* to the blob lookup function.
*/
brentlu
force-pushed
the
ep-devtype-mismatch
branch
from
8c3ea43 to
13cef15
Compare
The endpoint in NHLT table for a SSP port could have the device type NHLT_DEVICE_BT or NHLT_DEVICE_I2S. Use intel_nhlt_ssp_device_type() function to retrieve the device type before querying the endpoint blob to make sure we are always using correct device type parameter. Signed-off-by: Brent Lu <[email protected]>
Due to regression of thesofproject/sof#8396, the device type of NHLT endpoints is changed from 0(bt sideband) to 4(i2s codec). This PR add a helper function to get device type of specific SSP port so we could always use correct device type to query endpoint blob