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.

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

Refactoring topology name fixup of Intel mach #5037

Merged
merged 3 commits into from
Jul 26, 2024

Conversation

brentlu
Copy link

@brentlu brentlu commented Jun 4, 2024

This pull request tries to use the code which fixup I2S topology file name to fixup HDA/SDW topology as well. Then redundant code could be removed.

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.

Sorry, not following this at all, it's a lot of code changes and I don't really see what you are trying to fix.

@@ -51,6 +51,9 @@ extern struct snd_soc_acpi_mach snd_soc_acpi_intel_ptl_sdw_machines[];
* generic table used for HDA codec-based platforms, possibly with
* additional ACPI-enumerated codecs
*/
#define SND_SOC_ACPI_INTEL_HDA_MACH_SKL 0
#define SND_SOC_ACPI_INTEL_HDA_MACH_SOF 1
Copy link
Member

Choose a reason for hiding this comment

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

do we need to do this?

Most of the SKL/KBL platforms with the DSP are supposed to use the AVS driver now.

Rather than deal with differences with an obsolete driver, maybe we should remove support for HDaudio in SKL?

@crojewsk what do you think?

Choose a reason for hiding this comment

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

Sorry for the delay. I'll take a look today and respond either today or tomorrow. Thank you @plbossart for informing me about this.

Choose a reason for hiding this comment

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

ACK for ignoring skylake-driver here.

@brentlu
Copy link
Author

brentlu commented Jun 4, 2024

There is no bug/issue here. I just try to use same code to fixup topology name of I2S/HDA machine drivers. And yes, we could use same code for SDW as well. Let me upload an new version after local testing. Thanks.

@brentlu brentlu force-pushed the hda-mach-1 branch 2 times, most recently from 7951c54 to c904c90 Compare June 5, 2024 01:35
@brentlu brentlu changed the title HDA machine driver update - 1 Unifi topology name fixup for Intel mach Jun 5, 2024
@brentlu brentlu force-pushed the hda-mach-1 branch 2 times, most recently from 1a44930 to edc8426 Compare June 5, 2024 11:12
@brentlu
Copy link
Author

brentlu commented Jun 5, 2024

I've uploaded new patches which are focusing on the topology name fixup and minimizing each patch. Now all mach could use same code to handle DMIC name fixup (-dmicXch or -Xch). The flow to enumerate mach is not touched. Only the code related to topology name fixup is modified.

@plbossart
Copy link
Member

Not sure what's going on but we've got a nasty TGL IPC timeout I don't recall having seen before:

https://sof-ci.01.org/linuxpr/PR5037/build3377/devicetest/index.html?model=TGLU_UP_HDA-ipc4&testcase=check-signal-stop-start-capture-10

@brentlu
Copy link
Author

brentlu commented Jun 11, 2024

Not sure what's going on but we've got a nasty TGL IPC timeout I don't recall having seen before:

https://sof-ci.01.org/linuxpr/PR5037/build3377/devicetest/index.html?model=TGLU_UP_HDA-ipc4&testcase=check-signal-stop-start-capture-10

This PR only touch topology name string. Should not result in runtime IPC error. Maybe we could rerun the test?

@plbossart
Copy link
Member

SOFCI TEST

1 similar comment
@plbossart
Copy link
Member

SOFCI TEST

@brentlu
Copy link
Author

brentlu commented Jul 5, 2024

Hi @plbossart @bardliao , is this PR looking good to you?


hda_mach->mach_params.dmic_num = dmic_num;
pdata->tplg_filename = tplg_filename;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we need to assign pdata->tplg_filename?

Copy link
Author

Choose a reason for hiding this comment

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

We need to keep it NULL because caller (hda_machine_select()) will check this variable again for the value of its tplg_fixup local variable. So here we update the file name string in mach.

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.

I am afraid we missed the train for 6.11 while we were busy with other things. This will have to wait a bit @brentlu before being merged.

sound/soc/sof/intel/hda.c Outdated Show resolved Hide resolved
@@ -1348,7 +1366,7 @@ struct snd_soc_acpi_mach *hda_machine_select(struct snd_sof_dev *sdev)
tplg_filename = devm_kasprintf(sdev->dev, GFP_KERNEL,
"%s%s%d%s",
sof_pdata->tplg_filename,
"-dmic",
i2s_mach_found ? "-dmic" : "-",
Copy link
Member

Choose a reason for hiding this comment

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

that's almost the same code as line 580, so there's likely additional simplifications possible.

sound/soc/sof/intel/hda.c Show resolved Hide resolved
@brentlu brentlu force-pushed the hda-mach-1 branch 2 times, most recently from 4168024 to 0208fa3 Compare July 19, 2024 05:36

/*
* If tplg file name is overridden, use it instead of
* the one set in mach table
*/
if (!sof_pdata->tplg_filename) {
sof_pdata->tplg_filename = mach->sof_tplg_filename;
/* remove file extension if exist */
Copy link
Member

Choose a reason for hiding this comment

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

if is exists

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@@ -1359,8 +1272,24 @@ struct snd_soc_acpi_mach *hda_machine_select(struct snd_sof_dev *sdev)
/* report to machine driver if any DMICs are found */
mach->mach_params.dmic_num = check_dmic_num(sdev);

/* SDW mach does not use DMIC quirk flag. */
Copy link
Member

Choose a reason for hiding this comment

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

I can't figure out what this comment means, and same for the commit message

SDW mach does not use the DMIC quirk flag and pass full topology name
with file extension to SOF driver. Therefore, we add extra code to
hda_generic_machine_select() function to cope with the difference.

if the goal was to unify, why do we still need differences with SoundWire handling for the dmic case? it's the same in the end, we need the -2ch or -4ch suffix appended, no?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry my wording is misleading. In this PR I am trying to use same code to do topology name fixup for all I2S/HDA/SDW mach. There is no functional change in these commits. I'm updating the git commit/comment for more accurate description.

@brentlu brentlu changed the title Unifi topology name fixup for Intel mach Refactoring topology name fixup of Intel mach Jul 22, 2024
Currently both Skylake and SOF platform driver enumerate same HDA
machine driver 'skl_hda_dsp_generic' for HDA external codec. This
commit removes HDA external codec support on Skylake platform so the
machine driver will be used by SOF only.

Signed-off-by: Brent Lu <[email protected]>
Move I2S mach's topology name fixup code to the end of machine driver
enumeration flow so HDA mach could also use same code to fixup its
topology file name as well. No functional change in this commit.

Signed-off-by: Brent Lu <[email protected]>
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.

@bardliao can you please take a look?

@@ -511,6 +511,8 @@ static int check_dmic_num(struct snd_sof_dev *sdev)
if (nhlt)
dmic_num = intel_nhlt_get_dmic_geo(sdev->dev, nhlt);

dev_info(sdev->dev, "DMICs detected in NHLT tables: %d\n", dmic_num);

Copy link
Member

Choose a reason for hiding this comment

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

typo in commit message: extenion

Copy link
Author

Choose a reason for hiding this comment

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

fixed

Remove SDW mach's topology name fixup code and use the code in
hda_machine_select() to fixup its topology file name. No functional
change in this commit.

Compared with I2S/HDA mach, SDW mach always fixup topology file name
with dmic num without using DMIC quirk flag and pass topology name
with file extension to SOF driver. Therefore, we add extra code to
remove file extension if it exists.

Signed-off-by: Brent Lu <[email protected]>
@plbossart plbossart merged commit dc9dd7b into thesofproject:topic/sof-dev Jul 26, 2024
11 of 15 checks passed
@marc-hb
Copy link
Collaborator

marc-hb commented Jul 30, 2024

if the goal was to unify, why do we still need differences with SoundWire handling for the dmic case? it's the same in the end, we need the -2ch or -4ch suffix appended, no?

There is no functional change in these commits.

Are you sure? The firmware just stopped loading in some configurations with 2 DMICs:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants