-
Notifications
You must be signed in to change notification settings - Fork 140
ASOC: SOF: choose hda machine driver if hda codes detected #508
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
ASOC: SOF: choose hda machine driver if hda codes detected #508
Conversation
0a6d4a8 to
28807ac
Compare
|
Test it on hda and i2s platforms |
plbossart
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice try but not what I had in mind.
What this patch does is add duplicate code to check if there are HDAudio codecs. We'd want to do this as part of the probe without new code introduced.
|
@plbossart Do you mean to do it in sof_dsp probe? I submitted a PR before(#321), you comment was: The ask is that we can have a single kernel with HDaudio compiled in, and the behavior is as follows |
My point was to try and to the codec probe earlier. that didn't mean duplicating code. |
|
Get it, I will try to make it. |
9a1f482 to
17634ab
Compare
|
update my PR. Thanks! |
plbossart
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code looks good but this PR has a couple of issues
a) the error flow needs to be reworked completely, it's obviously broken
b) it results in some inits being done multiple times. That's not good, we need to have a simple call flow where we don't repeat things and add complexity.
c) I would really prefer it if we didn't have an HDaudio-specific flow and API added. The code feels like a hack if we don't use an abstraction or the normal flow.
sound/soc/sof/sof-pci-dev.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is still adding a new step and new API to the core that's HDaudio specific. You will also do things multiple times since the "bus" variable is local and all inits are gone once you leave the scope of this block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, you mean: only initialize hda one time in pci probe and pass it to sof core ?
I will try to make it.
sound/soc/sof/intel/hda-codec.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we have a helper for this already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I will refine it
sound/soc/sof/intel/hda-codec.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I will refine it
sound/soc/sof/intel/hda-codec.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bam. your error flow did not turn i915 power off. this needs to be reworked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I will refine it
sound/soc/sof/intel/hda-codec.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be part of the error flow.
Also this means you will initialize the i912 connection two times, and it's not clear to me why this was needed if the first step is successful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I will refine it
|
On 1/9/19 8:28 PM, RanderWang wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In sound/soc/sof/sof-pci-dev.c
<#508 (comment)>:
> @@ -207,14 +207,13 @@ static int sof_pci_probe(struct pci_dev *pci,
mach = snd_soc_acpi_find_machine(desc->machines);
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
if (!mach) {
- dev_warn(dev, "No matching ASoC machine driver found - falling back to HDA codec\n");
- mach = snd_soc_acpi_intel_hda_machines;
- mach->sof_fw_filename = desc->nocodec_fw_filename;
-
- /*
- * TODO: we need to find a way to check if codecs are actually
- * present
- */
+ struct hdac_bus bus;
+
+ if (hda_codec_detect(pci, &bus, FALSE) > 0) {
yes, you mean: only initialize hda one time in pci probe and pass it
to sof core ?
I will try to make it.
No. This is just moving things around, what I really would like is that
the codec_mask is available before the machine driver selection is made.
|
286cad6 to
51be44c
Compare
|
Update my PR. Now hda codec is initialized only one time. Sorry to make CI failed, but it works in my local machine. Today I can't fix it. This PR is just for review. |
|
to be clearer, i'd like the detection to rely on the abstraction that exists for SOF. calling directly a low-level function directly from sof-pci-dev isn't exactly aligned with what we do. It introduces a short-cut that isn't elegant at all. maybe this means we need a .hardware_detect before sof_probe indeed but there's no reason to have a low-level bypass only for the hdaudio case. |
@plbossart I agree that those HDaudio-specific code in sof-pci-dev.c looks hack. Shall we postpone the machine driver selection to sof_probe(): |
|
On 1/11/19 12:09 AM, Keyon wrote:
c) I would really prefer it if we didn't have an HDaudio-specific
flow and API added. The code feels like a hack if we don't use an
abstraction or the normal flow.
@plbossart <https://github.com/plbossart> I agree that those
HDaudio-specific code in sof-pci-dev.c looks hack. Shall we postpone
the machine driver selection to sof_probe():
|static int sof_probe(struct platform_device *pdev) { ... /* probe the
DSP hardware */ ret = snd_sof_probe(sdev); ... //Let's move the
HDA/nocodec machine driver selection here? ...|
|I initially thought this was a good idea but this would also add an
HDAudio specific test in the middle of the most common piece of code.
Let's think a bit more on this.|
… |/* register machine driver, pass machine info as pdata */
plat_data->pdev_mach = platform_device_register_data(sdev->dev,
drv_name, PLATFORM_DEVID_NONE, mach, size); ... } |
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#508 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABGIa0zo1EkSet14xZrFZOkX5BqHuhnnks5vCCqegaJpZM4Zy93N>.
|
How about adding an general ops there to do the hda codec_mask check and matched machine update inside it: |
81869d9 to
698b310
Compare
|
@plbossart @keyonjie I submit a prototype of keyon's idea and test it on WHL. |
plbossart
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea, it's simple enough. That said, this will not work with e.g. Up2 with HDMI enabled, so the criterion to select the HDAudio machine driver needs to be modified, e.g. with the codec_mask that has at bitfield 2 set (0x4) and possibly one additional field for the HDaudio codec.
sound/soc/sof/intel/hda.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks wrong. You are overriding the machine already selected based on ACPI ID. this should only happen if there was no machine found earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't set .sof_fw_filename in hda_machines, I think that's why we need set it above.
Please see below:
struct snd_soc_acpi_mach snd_soc_acpi_intel_hda_machines[] = {
{
/* .id is not used in this file */
.drv_name = "skl_hda_dsp_generic",
/* .fw_filename is dynamically set in skylake driver */
/* .sof_fw_filename is dynamically set in sof/intel driver */
.sof_tplg_filename = "intel/sof-hda-generic.tplg",
...
},
{},
};
We actually use same FW filename for different machines with same chip, e.g. "sof-apl.ri" for all I2S, HDA, nocodec machines.
struct sof_dev_desc {
...
/* defaults for no codec mode */
const char *nocodec_fw_filename;
const char *nocodec_tplg_filename;
...
};
So, how about rename struct sof_dev_desc.nocodec_fw_filename to "sof_dev_desc.default_fw_filename"?
struct sof_dev_desc {
...
/* default binary names */
const char *default_fw_filename;
const char *nocodec_tplg_filename;
...
};
And then, we can do it like this:
} else {
mach = snd_soc_acpi_intel_hda_machines;
if (!mach->sof_fw_filename)
/* use default fw name */
mach->sof_fw_filename = pdata->desc->default_fw_filename;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option is get sof_fw_filename from other I2S machine, but this is still not natural
mach = snd_soc_acpi_intel_hda_machines;
mach->sof_fw_filename = pdata->desc->machines[0].sof_fw_filename;
pdata->machine = mach;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. The feedback was on the No. The feedback was mach = snd_soc_acpi_intel_hda_machines;
You essentially remove all capability of using I2S codecs if there is HDMI detected.
Please test on Up2 w/ Hifiberry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. The feedback was on the No. The feedback was mach = snd_soc_acpi_intel_hda_machines;
You essentially remove all capability of using I2S codecs if there is HDMI detected.
Please test on Up2 w/ Hifiberry.
ah, right, good catch. @RanderWang we should do this codec_mask checking and pdata->machine updating only at "pdata->machine == NULL" case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I will only use hda machine driver if both hdac codecs and hdmi are detected. This is a advice from pierre. I also want to do this. And I also struggle about : if only hdmi is detected, to use hda machine , which supports this, or nocodec machine ? And hdmi could be detected with I2S machine. So I prefer to use hda machine only hdac codecs and hdmi codec are detected.
sound/soc/sof/sof-pci-dev.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to check the error flow. now there is no longer an error when no machine driver is found, so this is a new flow which needs to be checked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I will refine it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I check the error flow and find that there is only one error label release_regions which is shared by many error checks. I will check the status in sof-probe and release_regions there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@plbossart I check machine in sof core and sof_probe would be failed if machine is NULL. Now the flow would be sof_probe failed -> pci_probe failed. I know this issue you are working with @libinyang @ranj063, so it is a generic issue.
sound/soc/sof/nocodec.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename as sof_nocodec_bes_setup then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
|
@RanderWang @plbossart a question unrelated to this PR but related to the choice of machine drivers in general. For boards like WHL RVP without any codecs, can we just test HDMI playback with SOF and if so which machine driver should be chosen in this case? |
Actually, there is a hda codec (ALC700) on WHL RVP. Just test HDMI is not enough. For example |
9c75e9a to
41aad7f
Compare
|
update PR. Thanks! |
The skl_generic_hda machine driver should just work, it supports either HDMI or HDMI+HDaudio codec combinations. |
Oh thats cool. The reason I asked this was to see if we could test HDMI playback on the CML boards that dont have any codecs. Let me give this a try. |
plbossart
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @RanderWang and @keyonjie, this is starting to take shape, I have only a couple of comments on the selection of the HDaudio machine driver. As @ranj063 was suggesting in a comment, we need to deal with headless devices where HDMI is the only form of user interaction
sound/soc/sof/intel/hda.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good start but we can make it more general to handle multiple situations:
0. codec_num = 0 -> use nocodec if enabled in the build
- codec_num = 1 -> use nocodec if enabled in the build (for validation)
- codec_num = 1 -> use HDMI (for user interaction e.g. on a headless device like Up2)
- codec_num = 2 -> use HDMI + HDAudio
Also for HDMI you need to verify that bit2 is set, it's not just the number of codecs that matters.
Also you would need to set the HDAudio machine driver only if there is no ACPI ID (or machine already set).
Makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes , 1 and 2 is what I want but I was failed to get.
And I get the number of codecs because I am not sure HDMI is always identified by bit2. I searched the HDA spec, still didn't find it. But on our platforms, I find it is always at bit2.
I will check machine driver is none or not here before using HDA machine driver.
9d54217 to
6d76576
Compare
|
update my PR according to new criterion. |
sound/soc/sof/sof-pci-dev.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RanderWang no need for braces here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I will refine it. Thanks!
plbossart
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there. I'd like a minor tweak since the machine driver can only support one external HDaudio codec, and a fix on a macro prefix. Let's merge this week.
sound/soc/sof/intel/hda.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually for the first case we need to double check HDMI is present (codec_num>1 is not enough in theory, there could be two HDaudio codecs which isn't supported). Something like this would work:
if (HDA_IDISP_CODEC(bus->codec_mask)) {
if (codec_num == 2 || !IS_ENABLED(CONFIG_SND_SOC_SOF_NOCODEC)) {
hda_mach = snd_soc_acpi_intel_hda_machines;
}
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this I am uncertain about. Thanks!
sound/soc/sof/intel/hda.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for extra indirection, machine = pdata->machine would work (and be consistent with code above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I will refine it. Thanks!
sound/soc/sof/intel/hda.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HDA_IDISP_CODEC
let's make the macros consistent in terms of prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I will refine it. Thanks!
sound/soc/sof/intel/hda.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is one HDMI codec and one external HDAudio codec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I will refine it
|
Sorry fellows, we have to rethink the whole thing due to new technical information (which is less than a week old). Issue #560 exposes a new requirement that the HDaudio initialization be done in a work queue to avoid deadlocks with request_module being called in a probe function. In other words, with the recommended approach, we will not know when snd_sof_probe() returns if there are HDaudio codecs detected. In all cases what this means is that the probe issues cannot be directly reported to the PCI probe layers, making our PM stuff even more iffy. |
|
@plbossart @keyonjie All my change in sof core is done after snd_sof_probe which is now move to sof_probe_continue. So I think the rework for PR is not difficult. |
Indeed, I ended-up trying the "alternate solution" and it's compatible with your approach. Only need to get the PM parts right now. |
So what this means is that you can provide updates and I'll merge them, thanks. don't put this on the back burner waiting for the workqueue to be handled, it can be done later. |
@plbossart i thought about a bit more and for me to get this to work, I need a new topology with just the HDMI pipelines. Is there a way we could combine the nocodec machine driver to add the iDisp DAI's. That way we could have one topology to test both no i2s codecs and hdmi playback. |
You could test hdmi with hda machine and i2s with nocodec machine. |
6d76576 to
d90e25e
Compare
It is ugly in pci probing function to check hda for machine driver. Now select hda machine driver in hda probing function and use nocodec machine driver if none of I2S or hda is selected after dsp probing Check following conditions to select machine driver 1. codec_num = 2 -> use hda with HDMI + HDAudio 2. codec_num = 1 -> use hda with HDMI if nocdec is disabled 3. codec_num = 1 -> use nocodec if enabled in the build (for validation) 4. codec_num = 0 -> use nocodec if enabled in the build step 2 is for on a headless device like Up2 Signed-off-by: Rander Wang <rander.wang@linux.intel.com>
d90e25e to
556c652
Compare
The change for machine driver would make a error that sof and nocodec module are dependent on each other. Signed-off-by: Rander Wang <rander.wang@linux.intel.com>
|
Update my PR. Thanks! |
@ranj063 indeed, when we use HDMI only we should use a topology that doesn't have the HDAudio parts, but that's a small change in the topology filename. We can do this in a follow-up PR. |
Just to make it a bit more complex, we can also have DMICs attached on up2, and DMICs attached on a platform with HDAudio codecs (as on the Acer Swift3 laptop), so we'll have to deal with all of these combinations.
That's why I suggested a long time ago a notion of "incremental" topology, where we'd have multiple files instead of multiple topologies. |
|
merging for now, we'll deal with the headless devices in another PR. |
|
@RanderWang travis captured an building warning, can you send another PR to fix it please? https://travis-ci.org/thesofproject/linux/jobs/482310276#L1043 |
done |
|
should you do the same on the acpi path (sof-acpi-dev.c), too? @RanderWang |
|
should you do the same on the acpi path (sof-acpi-dev.c), too?
@RanderWang <https://github.com/RanderWang>
No, this is not needed. For the ACPI path, we know already what the
codecs are (as defined by their ACPI HID exposed in DSDT tables). There
is no discoverability, ACPI tells you everything you need to know already.
… |
…ntries Jiri Olsa reported a fault when running: # cat /proc/kallsyms | grep ksys_read ffffffff8136d580 T ksys_read # objdump -d --start-address=0xffffffff8136d580 --stop-address=0xffffffff8136d590 /proc/kcore /proc/kcore: file format elf64-x86-64 Segmentation fault general protection fault, probably for non-canonical address 0xf887ffcbff000: 0000 [thesofproject#1] SMP PTI CPU: 12 PID: 1079 Comm: objdump Not tainted 5.14.0-rc5qemu+ thesofproject#508 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.14.0-4.fc34 04/01/2014 RIP: 0010:kern_addr_valid Call Trace: read_kcore ? rcu_read_lock_sched_held ? rcu_read_lock_sched_held ? rcu_read_lock_sched_held ? trace_hardirqs_on ? rcu_read_lock_sched_held ? lock_acquire ? lock_acquire ? rcu_read_lock_sched_held ? lock_acquire ? rcu_read_lock_sched_held ? rcu_read_lock_sched_held ? rcu_read_lock_sched_held ? lock_release ? _raw_spin_unlock ? __handle_mm_fault ? rcu_read_lock_sched_held ? lock_acquire ? rcu_read_lock_sched_held ? lock_release proc_reg_read ? vfs_read vfs_read ksys_read do_syscall_64 entry_SYSCALL_64_after_hwframe The fault happens because kern_addr_valid() dereferences existent but not present PMD in the high kernel mappings. Such PMDs are created when free_kernel_image_pages() frees regions larger than 2Mb. In this case, a part of the freed memory is mapped with PMDs and the set_memory_np_noalias() -> ... -> __change_page_attr() sequence will mark the PMD as not present rather than wipe it completely. Have kern_addr_valid() check whether higher level page table entries are present before trying to dereference them to fix this issue and to avoid similar issues in the future. Stable backporting note: ------------------------ Note that the stable marking is for all active stable branches because there could be cases where pagetable entries exist but are not valid - see 9a14aef ("x86: cpa, fix lookup_address"), for example. So make sure to be on the safe side here and use pXY_present() accessors rather than pXY_none() which could #GP when accessing pages in the direct map. Also see: c40a56a ("x86/mm/init: Remove freed kernel image areas from alias mapping") for more info. Reported-by: Jiri Olsa <jolsa@redhat.com> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com> Signed-off-by: Borislav Petkov <bp@suse.de> Reviewed-by: David Hildenbrand <david@redhat.com> Acked-by: Dave Hansen <dave.hansen@intel.com> Tested-by: Jiri Olsa <jolsa@redhat.com> Cc: <stable@vger.kernel.org> # 4.4+ Link: https://lkml.kernel.org/r/20210819132717.19358-1-rppt@kernel.org
It is ugly in pci probing function to check hda for machine driver. Now
select hda machine driver after hda codec has been detected. And
And select nocodec driver if no hda codec is detected and no I2S
machine driver is applied.
This fixes #284