Message-ID: <87seon9vq6.fsf@oltmanns.dev>
Date: Sun, 09 Feb 2025 12:57:21 +0100
From: Frank Oltmanns <frank@...manns.dev>
To: Bjorn Andersson <andersson@...nel.org>
Cc: Konrad Dybcio <konradybcio@...nel.org>,  Dmitry Baryshkov
 <dmitry.baryshkov@...aro.org>,  Chris Lew <quic_clew@...cinc.com>,
  linux-arm-msm@...r.kernel.org,  linux-kernel@...r.kernel.org,  Stephan
 Gerhold <stephan.gerhold@...aro.org>,  Johan Hovold
 <johan+linaro@...nel.org>,  Caleb Connolly <caleb.connolly@...aro.org>,
  Joel Selvaraj <joelselvaraj.oss@...il.com>,  Alexey Minnekhanov
 <alexeymin@...tmarketos.org>,  stable@...r.kernel.org
Subject: Re: [PATCH] soc: qcom: pd-mapper: defer probing on sdm845

On 2025-02-06 at 07:44:49 +0100, Frank Oltmanns <frank@...manns.dev> wrote:
Hi Bjorn,

> Hi again,
>
> On 2025-02-06 at 06:57:46 +0100, Frank Oltmanns <frank@...manns.dev> wrote:
>> On 2025-02-05 at 20:54:53 -0600, Bjorn Andersson <andersson@...nel.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@...r.kernel.org
>>>> Signed-off-by: Frank Oltmanns <frank@...manns.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'm sorry but there's one additional thing that I really should have
> mentioned earlier: My issue is specifically with *call* audio.
>
> Call audio is only available using out-of-tree patches. The ones I'm
> currently using are here:
> https://gitlab.com/sdm845-mainline/linux/-/commits/sdm845-6.13-rc2-r2?ref_type=tags

Just wanted to let you know that I've tested Mukesh Ojha's and Saranya
R's patch [1]. Thanks, Bjorn for cc'ing me in your response.

Unfortunately, it seems to fix a different issue than the one I'm
experiencing. The phone's mic still doesn't work. As I wrote elsewhere
[2], I don't see the PDR error messages on xiaomi-beryllium, so, as
Johan expected, the issue I'm experiencing is indeed a different one.

Best regards,
  Frank

[1]: https://lore.kernel.org/linux-arm-msm/20250129155544.1864854-1-mukesh.ojha@oss.qualcomm.com/
[2]: https://lore.kernel.org/linux-arm-msm/87wmf18m8g.fsf@oltmanns.dev/

>
> Best regards,
>   Frank
>
>>
>> (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@...manns.dev>
>>>>

Powered by blists - more mailing lists