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@xxxxxxxxxxxxxxx
> Signed-off-by: Frank Oltmanns <frank@xxxxxxxxxxxx>
> ---
> 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?
Can you please confirm which remoteproc is which in your numbering? (In
particular, which remoteproc instance is the audio DSP?)
> 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! Seems to
me that we have a race-condition in the pdr helper.
Regards,
Bjorn
> Thanks,
> Frank
>
> [1]: https://lore.kernel.org/linux-arm-msm/Zwj3jDhc9fRoCCn6@xxxxxxxxxx/
> ---
> 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@xxxxxxxxxxxx>
>
