Re: [PATCH v11 0/7] Qualcomm's lpass-hdmi ASoC driver to support audio over dp port

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

Hi Stephan,

On 13/01/2021 20:08, Stephan Gerhold wrote:
> Hi Srinivas,
> 
> On Thu, Oct 08, 2020 at 06:37:40AM +0100, Srinivas Kandagatla wrote:
>> On 08/10/2020 06:16, Srinivasa Rao Mandadapu wrote:
>>> These patches are to support audio over DP port on Qualcomm's SC7180 LPASS
>>> Asoc. It includes machine driver, cpu driver, platform driver updates for
>>> HDMI path support, device tree documention, lpass variant structure
>>> optimization and configuration changes.
>>> These patches depends on the DP patch series
>>> https://patchwork.kernel.org/project/dri-devel/list/?series=332029
>>> https://lore.kernel.org/patchwork/project/lkml/list/?series=464856
>>>
>>> changes since V10:
>>>       -- Moved hdmi regmap functions from lpass-hdmi.c to lpass-cpu.c
>>>       -- Moved QCOM_REGMAP_FIELD_ALLOC macro from lpass-hdmi.c to lpass.h
>>> changes since V9:
>>>       -- Removed unused structures lpass_hdmi.h
>>> changes since V8:
>>>       -- Removed redundant structure wrapper for reg map field memebrs
>>>       -- Updated lpass_hdmi_regmap_volatile API with appropriate registers as true
>>>          and others as false.
>>> changes since V7:
>>>       -- Fixed typo errors
>>>       -- Created Separate patch for buffer size change
>>> changes since V6:
>>>       -- Removed compile time define flag, which used for enabling
>>>        HDMI code, based on corresponding config param is included.
>>>       -- Updated reg map alloc API with reg map bulk API.
>>>       -- Removed unnecessary line splits
>>> changes since V5:
>>>       -- Removed unused struct regmap *map in lpass_platform_alloc_hdmidmactl_fields.
>>>       -- DMA alloc and free API signature change in lpass-apq8016.c, lpass-ipq806x.c
>>>       -- Keeping API "irqreturn_t lpass_platform_hdmiif_irq" under ifdef macro
>>> Changes Since v4:
>>>       -- Updated with single compatible node for both I2S and HDMI.
>>> Changes Since v3:
>>>       -- Removed id in lpass variant structure and used snd_soc_dai_driver id.
>>> Changes Since v2:
>>>       -- Audio buffer size(i.e. LPASS_PLATFORM_BUFFER_SIZE) in lpass-platform.c increased.
>>> Changes Since v1:
>>>       -- Commit messages are updated
>>>       -- Addressed Rob Herring review comments
>>>
>>> V Sujith Kumar Reddy (7):
>>>     ASoC: Add sc7180-lpass binding header hdmi define
>>>     ASoC: dt-bindings: Add dt binding for lpass hdmi
>>>     Asoc:qcom:lpass-cpu:Update dts property read API
>>>     Asoc: qcom: lpass:Update lpaif_dmactl members order
>>>     ASoC: qcom: Add support for lpass hdmi driver
>>>     Asoc: qcom: lpass-platform : Increase buffer size
>>>     ASoC: qcom: sc7180: Add support for audio over DP
>>>
>>>    .../devicetree/bindings/sound/qcom,lpass-cpu.yaml  |  74 ++--
>>>    include/dt-bindings/sound/sc7180-lpass.h           |   1 +
>>>    sound/soc/qcom/Kconfig                             |   5 +
>>>    sound/soc/qcom/Makefile                            |   2 +
>>>    sound/soc/qcom/lpass-apq8016.c                     |   4 +-
>>>    sound/soc/qcom/lpass-cpu.c                         | 249 ++++++++++++-
>>>    sound/soc/qcom/lpass-hdmi.c                        | 258 ++++++++++++++
>>>    sound/soc/qcom/lpass-hdmi.h                        | 102 ++++++
>>>    sound/soc/qcom/lpass-ipq806x.c                     |   4 +-
>>>    sound/soc/qcom/lpass-lpaif-reg.h                   |  49 ++-
>>>    sound/soc/qcom/lpass-platform.c                    | 395 +++++++++++++++++----
>>>    sound/soc/qcom/lpass-sc7180.c                      | 116 +++++-
>>>    sound/soc/qcom/lpass.h                             | 124 ++++++-
>>>    13 files changed, 1240 insertions(+), 143 deletions(-)
>>>    create mode 100644 sound/soc/qcom/lpass-hdmi.c
>>>    create mode 100644 sound/soc/qcom/lpass-hdmi.h
>>>
>>
>> Tested this series on DragonBoard 410c
>>
>> Tested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> 
> I spent quite some time today trying to track down another regression
> for MSM8916/DragonBoard 410c audio in 5.10 and identified this patch
> series as the cause. So I'm very surprised that you successfully tested
> this on DB410c.
> 
> Attempting to play HDMI audio results in:
> 
>    ADV7533: lpass_platform_pcmops_hw_params: invalid  interface: 3
>    ADV7533: lpass_platform_pcmops_trigger: invalid 3 interface
>    apq8016-lpass-cpu 7708000.audio-controller: ASoC: error at soc_component_trigger on 7708000.audio-controller: -22
> 
> Attempting to record analog audio results in:
> 
>    Unable to handle kernel NULL pointer dereference at virtual address 00000000000001e4
>    Internal error: Oops: 96000004 [#1] PREEMPT SMP
>    CPU: 1 PID: 1568 Comm: arecord Not tainted 5.11.0-rc3 #20
>    Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
>    pc : regmap_write+0x14/0x80
>    lr : lpass_platform_pcmops_open+0xd8/0x210 [snd_soc_lpass_platform]
>    Call trace:
>     regmap_write+0x14/0x80
>     lpass_platform_pcmops_open+0xd8/0x210 [snd_soc_lpass_platform]
>     snd_soc_component_open+0x2c/0x94
>     ...
> 
> Looking at the changes in "ASoC: qcom: Add support for lpass hdmi driver"
> there is a quite obvious mistake there. lpass.h now contains
>

We did hit these two bugs recently while June was testing a platform 
based of 410c.

we had to these 2 fixes in his dev branch

https://paste.ubuntu.com/p/MCbpBgH7JV/

and a hack:
https://paste.ubuntu.com/p/GYDSDmJt7Y/

I got side tracked with other stuff, so I could not cleanup the lpass 
hack patch to send it!

With this two patches June was able to test all the usecases for 410c.

> #include <dt-bindings/sound/sc7180-lpass.h>
> 
> and then the SC7810 DAI IDs are hardcoded all over lpass-cpu.c and
> lpass-platform.c. But apq8016 and ipq806x have completely different
> DAI IDs so now MI2S_QUATERNARY (HDMI) is invalid and
> MI2S_TERTIARY (= LPASS_DP_RX in sc7180-lpass.h) is treated as HDMI port.
> 
Yes, this basically overwritten some of the defines. Specially 
MI2S_TERTIARY and MI2S_QUATERNARY

We should probably consolidate these defines to a single lpass.h file in 
include/dt-bindings/ and not split them into soc specific.

> Effectively LPASS is broken on all platforms except SC7810.
> 
> I have a patch prepared to fix this (will send it tomorrow),
> but I wonder how you have tested this successfully on DB410c. :)
My tests are pretty basic headset/speaker playback cases, this basically 
tests PRIMARY and SECONDARY MI2S and not MI2S_TERTIARY or QUAT MI2S.

June reported this to me some time during Christmas.

Look forward to review your patches!

--srini

> 
> Stephan
>