Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get the device type before query endpoint blob #4667

Merged
merged 2 commits into from
Nov 14, 2023
Merged

Get the device type before query endpoint blob #4667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1132ca3
ALSA: hda: intel-nhlt: add intel_nhlt_ssp_device_type() function
brentlu Oct 27, 2023
13cef15
ASoC: SOF: ipc4-topology: support NHLT device type
brentlu Oct 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions include/sound/intel-nhlt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,9 @@ intel_nhlt_get_endpoint_blob(struct device *dev, struct nhlt_acpi_table *nhlt,
u32 bus_id, u8 link_type, u8 vbps, u8 bps,
u8 num_ch, u32 rate, u8 dir, u8 dev_type);

int intel_nhlt_ssp_device_type(struct device *dev, struct nhlt_acpi_table *nhlt,
u8 virtual_bus_id);

#else

static inline struct nhlt_acpi_table *intel_nhlt_init(struct device *dev)
Expand Down Expand Up @@ -184,6 +187,13 @@ intel_nhlt_get_endpoint_blob(struct device *dev, struct nhlt_acpi_table *nhlt,
return NULL;
}

static inline int intel_nhlt_ssp_device_type(struct device *dev,
struct nhlt_acpi_table *nhlt,
u8 virtual_bus_id)
{
return -EINVAL;
}

#endif

#endif
26 changes: 26 additions & 0 deletions sound/hda/intel-nhlt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,3 +318,29 @@ intel_nhlt_get_endpoint_blob(struct device *dev, struct nhlt_acpi_table *nhlt,
return NULL;
}
EXPORT_SYMBOL(intel_nhlt_get_endpoint_blob);

int intel_nhlt_ssp_device_type(struct device *dev, struct nhlt_acpi_table *nhlt,
u8 virtual_bus_id)
{
struct nhlt_endpoint *epnt;
int i;

if (!nhlt)
return -EINVAL;

epnt = (struct nhlt_endpoint *)nhlt->desc;
for (i = 0; i < nhlt->endpoint_count; i++) {
/* for SSP link the virtual bus id is the SSP port number */
if (epnt->linktype == NHLT_LINK_SSP &&
epnt->virtual_bus_id == virtual_bus_id) {
dev_dbg(dev, "SSP%d: dev_type=%d\n", virtual_bus_id,
epnt->device_type);
return epnt->device_type;
}

epnt = (struct nhlt_endpoint *)((u8 *)epnt + epnt->length);
}

return -EINVAL;
}
EXPORT_SYMBOL(intel_nhlt_ssp_device_type);
19 changes: 16 additions & 3 deletions sound/soc/sof/ipc4-topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1342,6 +1342,7 @@ static int snd_sof_get_nhlt_endpoint_data(struct snd_sof_dev *sdev, struct snd_s
int sample_rate, channel_count;
int bit_depth, ret;
u32 nhlt_type;
int dev_type = 0;

/* convert to NHLT type */
switch (linktype) {
Expand All @@ -1357,18 +1358,30 @@ static int snd_sof_get_nhlt_endpoint_data(struct snd_sof_dev *sdev, struct snd_s
&bit_depth);
if (ret < 0)
return ret;

/*
* 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.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.
 */

Copy link
Author

Choose a reason for hiding this comment

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

done. thanks for suggestion.

dev_type = intel_nhlt_ssp_device_type(sdev->dev, ipc4_data->nhlt,
dai_index);
if (ret < 0)
return ret;
break;
default:
return 0;
}

dev_dbg(sdev->dev, "dai index %d nhlt type %d direction %d\n",
dai_index, nhlt_type, dir);
dev_dbg(sdev->dev, "dai index %d nhlt type %d direction %d dev type %d\n",
dai_index, nhlt_type, dir, dev_type);

/* 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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Author

@brentlu brentlu Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

@ujfalusi ujfalusi Nov 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


if (!cfg) {
dev_err(sdev->dev,
Expand Down