ASoC: SOF: Intel: prepare i915 deferred probe #4645
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit 479dd2b. Signed-off-by: Pierre-Louis Bossart <[email protected]>
This reverts commit c75dbfe. Signed-off-by: Pierre-Louis Bossart <[email protected]>
…a_dev variable" This reverts commit 180286d. Signed-off-by: Pierre-Louis Bossart <[email protected]>
…er ran. In an effort to not call sof_ops_free twice, we stopped running it when probe was aborted. Check the result of cancel_work_sync to see if this was the case. Fixes: 31bb7bd ("ASoC: SOF: core: Only call sof_ops_free() on remove if the probe was successful") Cc: Peter Ujfalusi <[email protected]> Acked-by: Mark Brown <[email protected]>
The existing DSP probe may be handled in a workqueue to allow for extra time, typically for the i915 request_module and HDAudio codec handling. With the upcoming changes for i915/Xe driver relying on the -EPROBE_DEFER mechanism, we need to have a first pass of the probe which cannot be pushed to a workqueue. Introduce 2 new optional callbacks. probe_early is called before the workqueue runs. remove_late may be called from the workqueue if load is unsuccesful, but will otherwise be called on module unload. Signed-off-by: Pierre-Louis Bossart <[email protected]> Signed-off-by: Maarten Lankhorst <[email protected]> Acked-by: Mark Brown <[email protected]>
This patch moves the initial parts of the probe to the probe_early() callback, which provides a much faster decision on whether the SOF driver shall deal with a specific platform or yield to other Intel drivers. This is a limited functionality change, the bigger change is to move the i915/Xe initialization to the probe_early(). Signed-off-by: Pierre-Louis Bossart <[email protected]> Signed-off-by: Maarten Lankhorst <[email protected]> Acked-by: Mark Brown <[email protected]>
The hda_codec_i915_init() errors are ignored in hda_init() so it can never return -EPROBE_DEFER. Fix this before we move the call to hda_init() from the deferred probe to early probe. While at it, also fix error handling when hda_dsp_ctrl_get_caps fails. Suggested-by: Kai Vehmanen <[email protected]> Signed-off-by: Maarten Lankhorst <[email protected]> Reviewed-by: Kai Vehmanen <[email protected]>
Add missing pci_set_drv to NULL call on error. Signed-off-by: Maarten Lankhorst <[email protected]> Reviewed-by: Pierre-Louis Bossart <[email protected]> Reviewed-by: Peter Ujfalusi <[email protected]> Reviewed-by: Kai Vehmanen <[email protected]>
Selecting CONFIG_DRM selects CONFIG_VIDEO_NOMODESET, which exports video_firmware_drivers_only(). This can be used as a first approximation on whether i915 will be available. It's safe to use as this is only built when CONFIG_SND_HDA_I915 is selected by CONFIG_I915. It's not completely fool proof, as you can boot with "nomodeset i915.modeset=1" to make i915 load regardless, or use "i915.force_probe=!*" to never load i915, but the common case of booting with nomodeset to disable all GPU drivers this will work as intended. Because of this, we add an extra module parameter, snd_hda_core.gpu_bind that can be used to signal users intent. -1 follows nomodeset, 0 disables binding, 1 forces wait/-EPROBE_DEFER on binding. Signed-off-by: Maarten Lankhorst <[email protected]> Reviewed-by: Peter Ujfalusi <[email protected]> Reviewed-by: Kai Vehmanen <[email protected]> Reviewed-by: Pierre-Louis Bossart <[email protected]>
Xe is a new GPU driver that re-uses the display (and sound) code from i915. It's no longer possible to load i915, as the GPU can be driven by the xe driver instead. The new behavior will return -EPROBE_DEFER, and wait for a compatible driver to be loaded instead of modprobing i915. Converting all drivers at the same time is a lot of work, instead we will convert each user one by one. Signed-off-by: Maarten Lankhorst <[email protected]> Reviewed-by: Kai Vehmanen <[email protected]> Reviewed-by: Pierre-Louis Bossart <[email protected]>
Xe is a new driver for intel GPU's that shares the sound related code with i915. The modprobe mechanism is being replaced by the -EPROBE_DEFER mechanism, so we don't need to add a modprobe xe call. Adding this would have required a telepathy module to correctly guess whether to load i915 or xe. Signed-off-by: Maarten Lankhorst <[email protected]> Reviewed-by: Peter Ujfalusi <[email protected]> Reviewed-by: Kai Vehmanen <[email protected]> Reviewed-by: Pierre-Louis Bossart <[email protected]>
Now that we can use -EPROBE_DEFER, it's no longer required to spin off the snd_hdac_i915_init into a workqueue. It's likely the whole workqueue can be destroyed, but I don't have the means to test this. Removing the workqueue would simplify init even further, but is left as exercise for the reviewer. Signed-off-by: Maarten Lankhorst <[email protected]> Acked-by: Mark Brown <[email protected]> Reviewed-by: Kai Vehmanen <[email protected]> Reviewed-by: Amadeusz Sławiński <[email protected]>
Now that we can use -EPROBE_DEFER, it's no longer required to spin off the snd_hdac_i915_init into a workqueue. Use the -EPROBE_DEFER mechanism instead, which must be returned in the probe function. Signed-off-by: Maarten Lankhorst <[email protected]> Reviewed-by: Kai Vehmanen <[email protected]> Reviewed-by: Pierre-Louis Bossart <[email protected]>
Now that we can use -EPROBE_DEFER, it's no longer required to spin off the snd_hdac_i915_init into a workqueue. It's likely the whole workqueue can be destroyed, but I don't have the means to test this. Removing the workqueue would simplify init even further, but is left as exercise for the reviewer. Signed-off-by: Maarten Lankhorst <[email protected]> Acked-by: Mark Brown <[email protected]> Reviewed-by: Kai Vehmanen <[email protected]> Reviewed-by: Amadeusz Sławiński <[email protected]>
…probe Now that we can use -EPROBE_DEFER, it's no longer required to spin off the snd_hdac_i915_init into a workqueue. Use the -EPROBE_DEFER mechanism instead, which must be returned in the probe function. The previously added probe_early can be used for this, and we also use the newly added remove_late for unbinding afterwards. Signed-off-by: Maarten Lankhorst <[email protected]> Cc: Pierre-Louis Bossart <[email protected]>
Now that all drivers have moved from modprobe loading to handling -EPROBE_DEFER, we can remove the argument again. Signed-off-by: Maarten Lankhorst <[email protected]> Reviewed-by: Kai Vehmanen <[email protected]>
Change for consistency with the earlier change done for remove. Signed-off-by: Pierre-Louis Bossart <[email protected]>
|
https://sof-ci.01.org/linuxpr/PR4645/build460/devicetest/index.html shows MTL failures (despite showing a "green failure" that will make @marc-hb cringe). |
plbossart
force-pushed
the
fix/prepare-i915-deferred-probe
branch
from
c2bb4dd to
803ef8d
Compare
Allow drivers to set the GPU bind options directly. This is a temporary work-around until Xe drivers are all available. Signed-off-by: Pierre-Louis Bossart <[email protected]>
Prevent GPU bind until all drivers are upstream. Signed-off-by: Pierre-Louis Bossart <[email protected]>
plbossart
force-pushed
the
fix/prepare-i915-deferred-probe
branch
from
803ef8d to
9d95961
Compare
|
with the gpu_bind work-around, results look better on MTL-ACE: @kv2019i @mlankhorst @marc-hb do the suggested "NOT FOR UPSTREAM" patches meet basic technical sanity? |
|
@plbossart Ack, this looks great. Still a good question whether it's less work to maintain this here or in the DUT config scripts. One bonus with having this as a not-for-upstream patch here in sof-dev is that every time we do an upstream merge, we can try dropping the workaround and see if i915/xe is mature enough already (the rc1 updates are really the place where we get big i915/xe updates to sof-dev). If yes, drop the workaround, if not, keep it. PS lnl needs the same thing |
plbossart
changed the title
plbossart
requested review from
bardliao,
lyakh and
RanderWang
as code owners
plbossart
requested review from
ranj063,
dbaluta,
lgirdwood,
kv2019i,
ujfalusi,
vijendarmukunda,
kuanhsuncheng and
yaochunhung
as code owners
|
I'll go ahead and merge this, and add the LNL part later. |
aiChaoSONG
reviewed
Nov 8, 2023
Comment on lines
+678
to
+679
| snd_hdac_i915_bind(sof_to_bus(sdev), 0); | ||
| return hda_dsp_probe_early(sdev); |
There was a problem hiding this comment.
sof_to_bus() referred to sdev->pdata->hw_pdata, but hw_pdata is allocated and assigned in hda_dsp_probe_early(), is this safe?
There was a problem hiding this comment.
You gave me a good scare @aiChaoSONG, but it's fine really: the sof_to_bus(dev) argument is not used.
void snd_hdac_i915_bind(struct hdac_bus *bus, int i915_bind)
{
gpu_bind = i915_bind;
}so we could remove this argument to make sure it's never used. Care to send a patch?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Lots of small conflicts with existing patches that need to be reverted or updated.
FYI @kv2019i @marc-hb on the gpu-bind open