Re: [PATCH] soc: qcom: pd-mapper: defer probing on sdm845

On 2025-02-05 at 20:54:53 -0600, Bjorn Andersson andersson@kernel.org wrote:

On Wed, Feb 05, 2025 at 10:57:11PM +0100, Frank Oltmanns wrote:

...

On xiaomi-beryllium and oneplus-enchilada audio does not work reliably with the in-kernel pd-mapper. Deferring the probe solves these issues. Specifically, audio only works reliably with the in-kernel pd-mapper, if the probe succeeds when remoteproc3 triggers the first successful probe. I.e., probes from remoteproc0, 1, and 2 need to be deferred until remoteproc3 has been probed.

Introduce a device specific quirk that lists the first auxdev for which the probe must be executed. Until then, defer probes from other auxdevs.

Fixes: 1ebcde047c54 ("soc: qcom: add pd-mapper implementation") Cc: stable@vger.kernel.org Signed-off-by: Frank Oltmanns frank@oltmanns.dev

---

The in-kernel pd-mapper has been causing audio issues on sdm845 devices (specifically, xiaomi-beryllium and oneplus-enchilada). I observed that Stephan’s approach [1] - which defers module probing by blocklisting the module and triggering a later probe - works reliably.

Inspired by this, I experimented with delaying the probe within the module itself by returning -EPROBE_DEFER in qcom_pdm_probe() until a certain time (13.9 seconds after boot, based on ktime_get()) had elapsed. This method also restored audio functionality.

Further logging of auxdev->id in qcom_pdm_probe() led to an interesting discovery: audio only works reliably with the in-kernel pd-mapper when the first successful probe is triggered by remoteproc3. In other words, probes from remoteproc0, 1, and 2 must be deferred until remoteproc3 has been probed.

The remoteproc numbering is assigned at the time of registering each remoteproc driver, and does not necessarily relate to the order in which they are launched. That said, it sounds like what you're saying is that is that audio only works if we launch the pd-mapper after the remoteprocs has started?

Almost, but not quite. You are right, that remoteproc3 in my setup is always the last one that probes the pd-mapper.

However, when experimenting with different timings I saw that the pd-mapper really do has to respond to the probe from remoteproc3 (I'm not sure I'm using the right terminology here, but I hope my intent comes across). If the pd-mapper responds to remoteproc3's probe with -EPROBE_DEFER there will again be subsequent probes from the other remoteprocs. If we act on those probes, there is a chance that audio (mic in my case) does not work. So, my conclusion was that remoteproc3's probe has to be answered first before responding to the other probes.

Further note that in my experiments remoteproc1 was always the first to do the probing, followed by either remoteproc0 or remoteproc2. remoteproc3 was always the last.

Can you please confirm which remoteproc is which in your numbering? (In particular, which remoteproc instance is the audio DSP?)

remoteproc0: adsp remoteproc1: cdsp remoteproc2: slpi remoteproc3: 4080000.remoteproc

(I took them from the kernel messages "remoteproc remoteproc<X>: <xyz> is available".)

...

To address this, I propose introducing a quirk table (which currently only contains sdm845) to defer probing until the correct auxiliary device (remoteproc3) initiates the probe.

I look forward to your feedback.

I don't think the proposed workaround is our path forward, but I very much appreciate your initiative and the insights it provides!

Thank you! I was hoping that somebody with more experience in the QCOM universe can draw further conclusions from this.

Seems to me that we have a race-condition in the pdr helper.

If you need further experimenting or can give me rough guidance on where to look next, I'll be glad to help.

Thanks again, Frank

Regards, Bjorn

...

Thanks, Frank

[1]: https://lore.kernel.org/linux-arm-msm/Zwj3jDhc9fRoCCn6@linaro.org/

drivers/soc/qcom/qcom_pd_mapper.c | 43 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)

diff --git a/drivers/soc/qcom/qcom_pd_mapper.c b/drivers/soc/qcom/qcom_pd_mapper.c index 154ca5beb47160cc404a46a27840818fe3187420..34b26df665a888ac4872f56e948e73b561ae3b6b 100644 --- a/drivers/soc/qcom/qcom_pd_mapper.c +++ b/drivers/soc/qcom/qcom_pd_mapper.c @@ -46,6 +46,11 @@ struct qcom_pdm_data { struct list_head services; };

+struct qcom_pdm_probe_first_dev_quirk {

• const char *name;
• u32 id;

+};

static DEFINE_MUTEX(qcom_pdm_mutex); /* protects __qcom_pdm_data */ static struct qcom_pdm_data *__qcom_pdm_data;

@@ -526,6 +531,11 @@ static const struct qcom_pdm_domain_data *x1e80100_domains[] = { NULL, };

+static const struct qcom_pdm_probe_first_dev_quirk first_dev_remoteproc3 = {

• .id = 3,
• .name = "pd-mapper"

+};

static const struct of_device_id qcom_pdm_domains[] __maybe_unused = { { .compatible = "qcom,apq8016", .data = NULL, }, { .compatible = "qcom,apq8064", .data = NULL, }, @@ -566,6 +576,10 @@ static const struct of_device_id qcom_pdm_domains[] __maybe_unused = { {}, };

+static const struct of_device_id qcom_pdm_defer[] __maybe_unused = {

• { .compatible = "qcom,sdm845", .data = &first_dev_remoteproc3, },
• {},

+}; static void qcom_pdm_stop(struct qcom_pdm_data *data) { qcom_pdm_free_domains(data); @@ -637,6 +651,25 @@ static struct qcom_pdm_data *qcom_pdm_start(void) return ERR_PTR(ret); }

+static bool qcom_pdm_ready(struct auxiliary_device *auxdev) +{

• const struct of_device_id *match;
• struct device_node *root;
• struct qcom_pdm_probe_first_dev_quirk *first_dev;
• root = of_find_node_by_path("/");
• if (!root)

• return true;
  

• match = of_match_node(qcom_pdm_defer, root);
• of_node_put(root);
• if (!match)

• return true;
  

• first_dev = (struct qcom_pdm_probe_first_dev_quirk *) match->data;
• return (auxdev->id == first_dev->id) && !strcmp(auxdev->name, first_dev->name);

+}

static int qcom_pdm_probe(struct auxiliary_device *auxdev, const struct auxiliary_device_id *id)

@@ -647,6 +680,15 @@ static int qcom_pdm_probe(struct auxiliary_device *auxdev, mutex_lock(&qcom_pdm_mutex);

if (!__qcom_pdm_data) {

• if (!qcom_pdm_ready(auxdev)) {
  

• 	pr_debug("%s: Deferring probe for device %s (id: %u)\n",
  

• 		__func__, auxdev->name, auxdev->id);
  

• 	ret = -EPROBE_DEFER;
  

• 	goto probe_stop;
  

• }
  

• pr_debug("%s: Probing for device %s (id: %u), starting pdm\n",
  

• 	__func__, auxdev->name, auxdev->id);
  

• data = qcom_pdm_start();

  if (IS_ERR(data))

@@ -659,6 +701,7 @@ static int qcom_pdm_probe(struct auxiliary_device *auxdev,

auxiliary_set_drvdata(auxdev, __qcom_pdm_data);

+probe_stop: mutex_unlock(&qcom_pdm_mutex);

return ret;

---

base-commit: 7f048b202333b967782a98aa21bb3354dc379bbf change-id: 20250205-qcom_pdm_defer-3dc1271d74d9

Best regards,

Frank Oltmanns frank@oltmanns.dev