Skip to content

Conversation

@plbossart
Copy link
Member

Same work as for SST driver upstream, we have a code dependency that needs to be fixed, otherwise randconfig will complain.

Compile-tested only for now.

This shouldn't be here anyways

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Make sure we can build without any code related to HDaudio links and
codecs.

Also make sure that when we do, the same option is used for
SND_SOC_SOF_HDA and for HDAC_HDA due to code dependencies.

Compiled-tested only for now.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Copy link

@keyonjie keyonjie left a comment

Choose a reason for hiding this comment

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

It will make more sense to me if we can align on these items firstly:

  1. what's the dependency of soc codec driver and legacy HDA/HDMI drivers?
    e.g. should we let SND_SOC_HDAC_HDA select SND_HDA_INTEL or SND_HDA_CODEC_REALTEK or SND_HDA_GENERIC?
    should we let SND_SOC_HDAC_HDMI select SND_HDA_CODEC_HDMI(I believe we don't need this)?

  2. in SOF driver, should we select HDA/HDMI support manually, or select them automatically once specific machine driver(sound/soc/intel/boards/xxx) is selected?
    I prefer the latter one, which is more friendly to user IMO, you only need to select SOF and the machine/board you are running, don't need to know HDA Links specific so won't forget to select that.
    Another benefit with the latter solution is, we won't select HDA/HDMI support on machines/boards where customers don't want them.

  3. For generic/nocodec fallback machine driver, should we select all HDA/HDMI items?
    The ideal solution to me is that we can detect all of them one by one, e.g. if codec_mask has HDA, then register HDA Analog/Digital BE dai_links, if codec_mask has iDisp, then register iDisp BE dai_links, if bus->ppcap!=NULL, then register SSP dummy BE dai_links, ...

After all those are clarified, maybe we can define less SOF_HDA_xxx items, and make life easier.

@RanderWang
Copy link

It is good to me. And I discuss with keyon, can we remove SST and legacy HDA when SOF is selected.
If SST or legacy HDA is selected, SOF can't work. This is another thing.

@plbossart
Copy link
Member Author

@keyonjie The Kconfigs are based on the platform drivers first. You select which SOC you want support for. Then you have a second choice for machine drivers, based on the initial selection of SOCs. You do not select machines first, and then try to reverse-engineer what configs are needed for the platform side. I spend a considerable amount of time last year to make sure we have a hierarchical definition of configurations, we are not going back on this.

I don't understand your point #1. The selection of legacy codec driver is independent from the SOF choices. And if you look at the hdaudio-codecs-defconfig you'll see that they are all manually added. I have no way of knowing which codecs are needed or not since it's a run-time information.

For point #2, we have to change the intel/boards Kconfig so that machine drivers that require HDMI are only selected when the platform side enables HDMI. This is missing from the patchset but I wanted to align first on the platform side. Note that unless the machine driver has conditional support for HDMI (as I did for bxt_pcm512x), you will include/remove machines that need HDMI unconditionally. We cannot make magic at the Kconfig level, the machine driver code itself needs to support more granularity if this is desired.

For the point #3, I already filed an issue on this. We have a chicken-and-egg problem in that the machine selection is made before we know if there is any actual HDaudio codec, so until this is fixed all these discussions on fallback are academic. If you want nocodec support you have to disable HDaudio for now.

@RanderWang no we cannot add a mutually exclusive SOF vs. SST condition, for the simple reason that some distributions will take SST for SKL, KBL, APL but SOF for GLK. We may need to add more granularity in the SST driver but that's a different story. For now, you have to deal with this using sof-defconfig or sst-defconfig which guarantee that you will have no collision.

Only include boards with HDMI support if SOF has enabled HDA_LINK support.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart
Copy link
Member Author

@keyonjie See last update with better handling of HDMI dependencies based on what SOF supports.

Copy link

@keyonjie keyonjie left a comment

Choose a reason for hiding this comment

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

the code change looks almost fine to me except questions about the HDAC_HDMI missing and necessity of SOF_HDA_LINK for cnl-rt274.

endif ## SND_SOC_INTEL_SKYLAKE || SND_SOC_SOF_GEMINILAKE

if SND_SOC_INTEL_SKYLAKE || SND_SOC_SOF_CANNONLAKE
if SND_SOC_INTEL_SKYLAKE || SND_SOC_SOF_CANNONLAKE && SND_SOC_SOF_HDA_LINK

Choose a reason for hiding this comment

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

We don't have HDMI BE dai_links in cnl_rt274.c now so no need this adding?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have HDMI BE dai_links in cnl_rt274.c now so no need this adding?

Ah yes, this was added in a prototype code. Will remove all this anyways.

@keyonjie
Copy link

@keyonjie The Kconfigs are based on the platform drivers first. You select which SOC you want support for. Then you have a second choice for machine drivers, based on the initial selection of SOCs. You do not select machines first, and then try to reverse-engineer what configs are needed for the platform side. I spend a considerable amount of time last year to make sure we have a hierarchical definition of configurations, we are not going back on this.

OK, then we are aligned on this, select platforms first, then machines.

I don't understand your point #1. The selection of legacy codec driver is independent from the SOF choices. And if you look at the hdaudio-codecs-defconfig you'll see that they are all manually added. I have no way of knowing which codecs are needed or not since it's a run-time information.

I was wondering if possible to select hdaudio codecs via dependency as some people are missing this and then reporting that HDaudio doesn't work on SOF.
Your reply already clarified to me about that, so this is also aligned, people need to select that manually, or follow hdaudio-codecs-defconfig.

For point #2, we have to change the intel/boards Kconfig so that machine drivers that require HDMI are only selected when the platform side enables HDMI. This is missing from the patchset but I wanted to align first on the platform side. Note that unless the machine driver has conditional support for HDMI (as I did for bxt_pcm512x), you will include/remove machines that need HDMI unconditionally. We cannot make magic at the Kconfig level, the machine driver code itself needs to support more granularity if this is desired.

OK, that makes sense to me.

For the point #3, I already filed an issue on this. We have a chicken-and-egg problem in that the machine selection is made before we know if there is any actual HDaudio codec, so until this is fixed all these discussions on fallback are academic. If you want nocodec support you have to disable HDaudio for now.

I think hda_codec_probe_bus() happens before we creating machine platform device, so we still have choice to choose machine driver based on the existence of actual HDaudio codec.

@plbossart
Copy link
Member Author

I think hda_codec_probe_bus() happens before we creating machine platform device, so we still have choice to choose machine driver based on the existence of actual HDaudio codec.

That's not what I see in sof-pci-dev. We first select the machine, then create the sof-audio platform device.

@keyonjie
Copy link

I think hda_codec_probe_bus() happens before we creating machine platform device, so we still have choice to choose machine driver based on the existence of actual HDaudio codec.

That's not what I see in sof-pci-dev. We first select the machine, then create the sof-audio platform device.

We select matched machine in sof-pci-dev.c:sof_pci_probe(), but create machine platform device at sof/core.c:sof_probe(), after hda_dsp_probe() is finished and FW booted, so here we have chance to change/configure machine driver according to the result of hda-codec existence.

@plbossart
Copy link
Member Author

We select matched machine in sof-pci-dev.c:sof_pci_probe(), but create machine platform device at sof/core.c:sof_probe(), after hda_dsp_probe() is finished and FW booted, so here we have chance to change/configure machine driver according to the result of hda-codec existence.

Not sure how it would work, the sof::sof_probe also creates the PCM platform device which will load the topology, so it's too late to fall back to nocodec....Not to mention that the soc_acpi machine structure does not contains read-only parts only, so it's hard to change on the fly.

@keyonjie
Copy link

We select matched machine in sof-pci-dev.c:sof_pci_probe(), but create machine platform device at sof/core.c:sof_probe(), after hda_dsp_probe() is finished and FW booted, so here we have chance to change/configure machine driver according to the result of hda-codec existence.

Not sure how it would work, the sof::sof_probe also creates the PCM platform device which will load the topology, so it's too late to fall back to nocodec....Not to mention that the soc_acpi machine structure does not contains read-only parts only, so it's hard to change on the fly.

Actually, the topology is loaded late(after FW booted), hda_dsp_probe() finished before that, we have enough time to do that, we don't need to change the matched machine itself(so don't need to care if it is read only), we only need to change the pointer plat_data->machine to point to another machine.

@plbossart plbossart merged commit aff80d7 into thesofproject:topic/sof-dev Nov 25, 2018
bardliao pushed a commit that referenced this pull request Jun 11, 2019
BugLink: https://bugs.launchpad.net/bugs/1828410

[ Upstream commit f6d9758 ]

The following false positive lockdep splat has been observed.

======================================================
WARNING: possible circular locking dependency detected
4.20.0+ #302 Not tainted
------------------------------------------------------
systemd-udevd/160 is trying to acquire lock:
edea6080 (&chip->reg_lock){+.+.}, at: __setup_irq+0x640/0x704

but task is already holding lock:
edff0340 (&desc->request_mutex){+.+.}, at: __setup_irq+0xa0/0x704

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #1 (&desc->request_mutex){+.+.}:
       mutex_lock_nested+0x1c/0x24
       __setup_irq+0xa0/0x704
       request_threaded_irq+0xd0/0x150
       mv88e6xxx_probe+0x41c/0x694 [mv88e6xxx]
       mdio_probe+0x2c/0x54
       really_probe+0x200/0x2c4
       driver_probe_device+0x5c/0x174
       __driver_attach+0xd8/0xdc
       bus_for_each_dev+0x58/0x7c
       bus_add_driver+0xe4/0x1f0
       driver_register+0x7c/0x110
       mdio_driver_register+0x24/0x58
       do_one_initcall+0x74/0x2e8
       do_init_module+0x60/0x1d0
       load_module+0x1968/0x1ff4
       sys_finit_module+0x8c/0x98
       ret_fast_syscall+0x0/0x28
       0xbedf2ae8

-> #0 (&chip->reg_lock){+.+.}:
       __mutex_lock+0x50/0x8b8
       mutex_lock_nested+0x1c/0x24
       __setup_irq+0x640/0x704
       request_threaded_irq+0xd0/0x150
       mv88e6xxx_g2_irq_setup+0xcc/0x1b4 [mv88e6xxx]
       mv88e6xxx_probe+0x44c/0x694 [mv88e6xxx]
       mdio_probe+0x2c/0x54
       really_probe+0x200/0x2c4
       driver_probe_device+0x5c/0x174
       __driver_attach+0xd8/0xdc
       bus_for_each_dev+0x58/0x7c
       bus_add_driver+0xe4/0x1f0
       driver_register+0x7c/0x110
       mdio_driver_register+0x24/0x58
       do_one_initcall+0x74/0x2e8
       do_init_module+0x60/0x1d0
       load_module+0x1968/0x1ff4
       sys_finit_module+0x8c/0x98
       ret_fast_syscall+0x0/0x28
       0xbedf2ae8

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&desc->request_mutex);
                               lock(&chip->reg_lock);
                               lock(&desc->request_mutex);
  lock(&chip->reg_lock);

&desc->request_mutex refer to two different mutex. #1 is the GPIO for
the chip interrupt. #2 is the chained interrupt between global 1 and
global 2.

Add lockdep classes to the GPIO interrupt to avoid this.

Reported-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: David S. Miller <davem@davemloft.net>

Signed-off-by: Sasha Levin <sashal@kernel.org>

Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
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.

3 participants