Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DPIB/posbuf pointer mode enabling #123

Merged
merged 9 commits into from
Sep 14, 2018

Conversation

keyonjie
Copy link

This series is to enable DPIB/posbuf pointer mode for SOF on SKL+ platforms:

  1. refine BDL settings.
    2. add pointer callback to read postion from DPIB/posbuf;
    3. add Kconfig to force IPC update mode on SKL+, that is:
    for SKL-, we will always use IPC update mode;
    for SKL+, we will use DPIB/posbuf by default, but be able to force IPC mode for debug.

Hi, @keqiaozhang @ZhendanYang, can you help verified if this will cause any regression on existed(e.g. byt and apl)?

Hi, @lgirdwood please only merge after verification passed.

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most minor, but pls remember comments and commit messages.

struct hdac_bus *bus = sof_to_bus(sdev);
snd_pcm_uframes_t pos = 0;

if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please comment on logic here and why there is difference for playback and capture.

(AZX_REG_VS_SDXDPIB_XINTERVAL *
hstream->index));
} else {
usleep_range(20, 30);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this time, pls comment

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK let me add comments to this logic.


pos = bytes_to_frames(substream->runtime, pos);

dev_dbg(sdev->dev, "PCM: stream %d dir %d position %lu\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dev_vdbg since this will be high frequency ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good, let me change.

@@ -591,7 +591,14 @@ static void ipc_stream_message(struct snd_sof_dev *sdev, u32 msg_cmd)

switch (msg_type) {
case SOF_IPC_STREAM_POSITION:
ipc_period_elapsed(sdev, msg_id);
if (!sdev->ops->pcm_pointer ||
IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_FORCE_IPC_POSITION)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you make this #if like the others for consistency

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keyonjie is this line correct, should this be !IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_FORCE_IPC_POSITION)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ranj063 we handle ipc and run ipc_elapse here when force_ipc_position is configured, or we don't have pcm_pointer callback, so this line is correct.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keyonjie makes sense. I misunderstood the config to be to force no ipc update. Thanks!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgirdwood , if use #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_FORCE_IPC_POSITION) here, the logic may looks more ugly here?

@@ -36,57 +36,97 @@
#include "hda.h"

/*
* set up Buffer Descriptor List (BDL) for host memory transfer
* BDL describes the location of the individual buffers and is little endian.
* set up one of BDL entries for a stream
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the change, there is no explanation in the commit message. What is wrong with existing BDL func ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the existing BDL, we don't support interrupt mode with DIPB/posbuf, let me add commit message to explain it.

@ranj063
Copy link
Collaborator

ranj063 commented Sep 12, 2018

@keyonjie recently while trying to no_irq mode in the FW, I ran into trouble with the playback not being able to continue without the perio update ipc from the FW.

Will these commit help with that?

@ranj063
Copy link
Collaborator

ranj063 commented Sep 12, 2018

@keyonjie if we use psbuf by default, should the no_irq mode be enabled by default too?

@keyonjie
Copy link
Author

@ranj063 how did you try no_irq mode?

@ranj063
Copy link
Collaborator

ranj063 commented Sep 12, 2018

@keyonjie setting host_period_bytes in stream params to 0 enables no_irq mode in the FW.

@lgirdwood
Copy link
Member

@ranj063 no_irq mode works by userspace asking driver for position updates. driver then has to read from SRAM window or DPIB. The playback may stop if the position does not move enough in the given time period.

@ranj063
Copy link
Collaborator

ranj063 commented Sep 12, 2018

@lgirdwood, does the DPIB posbuf update apply to normal SSP playback as well or is it meant for HDMI?

@ranj063
Copy link
Collaborator

ranj063 commented Sep 12, 2018

@keyonjie I tried your building your patchset for glk and saw a couple of compilation errors related to unused variables in hda-stream.c lines 99 and 102 (addr and pos_align)

@ranj063
Copy link
Collaborator

ranj063 commented Sep 12, 2018

@keyonjie this patchset breaks HDMI playback on glk

@keyonjie
Copy link
Author

@ranj063 thanks for your feedback, actually it's fixed locally but looks update not pushed yet, doing that now.

@keyonjie
Copy link
Author

@ranj063 can you share how it breaking HDMI?

@keyonjie
Copy link
Author

@lgirdwood can you share how to configure to use no_irq(SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP) when run aplay?

@ranj063
Copy link
Collaborator

ranj063 commented Sep 13, 2018

@keyonjie I get the "Unable to install hw params" error with aplay -D hw:0,5 for HDMI playback on the Yorp

If use IPC poistion update, read position from IPC
position.

For Playback, Use DPIB register from HDA space which
reflects the actual data transferred.

For Capture, Use the position buffer for pointer, as
DPIB is not accurate enough, its update may be
completed earlier than the data written to DDR.

Signed-off-by: Keyon Jie <[email protected]>
Which will make it possible to retrieve stream_tag and change
host_period_bytes in for HDA hs_params(), the latter will make
disable position update IPC for HDA stream possbile.

Signed-off-by: Keyon Jie <[email protected]>
@keyonjie
Copy link
Author

@lgirdwood @ranj063 all comments are addressed, I tried on APL, both set/unset FORCE_IPC_POSITION, the position update for HDMI works on my side.

@keyonjie
Copy link
Author

@keqiaozhang @ZhendanYang @xiulipan or anybody who can you help with a quick try on your byt/cnl SSP/DMIC to check if there is any regression with it?

@keqiaozhang
Copy link
Collaborator

@keyonjie @lgirdwood
There's no regression on BYT. But on APL, there is a obvious noise, checked with trace log and dmesg, no error message output.
I also tried to enable/disable CONFIG_SND_SOC_SOF_DEBUG_FORCE_IPC_POSITION in kernel config, found that when enabling CONFIG_SND_SOC_SOF_DEBUG_FORCE_IPC_POSITION, the noise is worse than disabling it.

@keyonjie
Copy link
Author

thanks @keqiaozhang , it's not too bad to me actually. Let me debug to root cause why it introduce noise on APL.

In DPIB/posbuf position pointer mode, we don't have position
update IPC messages, we may need(depends on if no_irq is set)
set different IOC flags for each BDL entry.

Without IOC setting for BDL entries, irq mode won't work as
there is no interrupt happens at each period finished.

Last, we set BDL IOC at DPIB/pos mode only.

Signed-off-by: Keyon Jie <[email protected]>
pcm_runtime is not ready yet at hw_params() stage, so we need to
retrieve period settings from params here.

Signed-off-by: Keyon Jie <[email protected]>
@keyonjie
Copy link
Author

@lgirdwood @keqiaozhang just root caused and fixed the noise issue.
I just verified it works fine for both DPIB/IPC position mode, with SSP/HDMI/DP, on BYT/APL/CNL.

I think it's ready for merge now.

@keyonjie
Copy link
Author

@ranj063 please use this latest version for your HDMI related works also.

@lgirdwood lgirdwood merged commit a224ab2 into thesofproject:topic/sof-dev Sep 14, 2018
keyonjie pushed a commit that referenced this pull request Oct 15, 2018
For a failing smc_listen_rdma_finish() smc_listen_decline() is
called. If fallback is possible, the new socket is already enqueued
to be accepted in smc_listen_decline(). Avoid enqueuing a second time
afterwards in this case, otherwise the smc_create_lgr_pending lock
is released twice:
[  373.463976] WARNING: bad unlock balance detected!
[  373.463978] 4.18.0-rc7+ #123 Tainted: G           O
[  373.463979] -------------------------------------
[  373.463980] kworker/1:1/30 is trying to release lock (smc_create_lgr_pending) at:
[  373.463990] [<000003ff801205fc>] smc_listen_work+0x22c/0x5d0 [smc]
[  373.463991] but there are no more locks to release!
[  373.463991]
other info that might help us debug this:
[  373.463993] 2 locks held by kworker/1:1/30:
[  373.463994]  #0: 00000000772cbaed ((wq_completion)"events"){+.+.}, at: process_one_work+0x1ec/0x6b0
[  373.464000]  #1: 000000003ad0894a ((work_completion)(&new_smc->smc_listen_work)){+.+.}, at: process_one_work+0x1ec/0x6b0
[  373.464003]
stack backtrace:
[  373.464005] CPU: 1 PID: 30 Comm: kworker/1:1 Kdump: loaded Tainted: G           O      4.18.0-rc7uschi+ #123
[  373.464007] Hardware name: IBM 2827 H43 738 (LPAR)
[  373.464010] Workqueue: events smc_listen_work [smc]
[  373.464011] Call Trace:
[  373.464015] ([<0000000000114100>] show_stack+0x60/0xd8)
[  373.464019]  [<0000000000a8c9bc>] dump_stack+0x9c/0xd8
[  373.464021]  [<00000000001dcaf8>] print_unlock_imbalance_bug+0xf8/0x108
[  373.464022]  [<00000000001e045c>] lock_release+0x114/0x4f8
[  373.464025]  [<0000000000aa87fa>] __mutex_unlock_slowpath+0x4a/0x300
[  373.464027]  [<000003ff801205fc>] smc_listen_work+0x22c/0x5d0 [smc]
[  373.464029]  [<0000000000197a68>] process_one_work+0x2a8/0x6b0
[  373.464030]  [<0000000000197ec2>] worker_thread+0x52/0x410
[  373.464033]  [<000000000019fd0e>] kthread+0x15e/0x178
[  373.464035]  [<0000000000aaf58a>] kernel_thread_starter+0x6/0xc
[  373.464052]  [<0000000000aaf584>] kernel_thread_starter+0x0/0xc
[  373.464054] INFO: lockdep is turned off.

Signed-off-by: Ursula Braun <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
paulstelian97 pushed a commit to paulstelian97/linux that referenced this pull request May 4, 2020
Documentation: teaching: labs: networking: Add networking lab
aiChaoSONG pushed a commit to aiChaoSONG/linux that referenced this pull request May 6, 2021
  - Add Alex and Miguel as maintainers.
  - Sort the keys properly.
  - Remove the unexisting `F:` key.
  - Add `B:` and `T:` keys.

Fixes thesofproject#123.

Reported-by: Josh Abraham <[email protected]>
Signed-off-by: Miguel Ojeda <[email protected]>
vijendarmukunda pushed a commit to vijendarmukunda/linux that referenced this pull request Feb 13, 2024
Like commit 1cf3bfc ("bpf: Support 64-bit pointers to kfuncs")
for s390x, add support for 64-bit pointers to kfuncs for LoongArch.
Since the infrastructure is already implemented in BPF core, the only
thing need to be done is to override bpf_jit_supports_far_kfunc_call().

Before this change, several test_verifier tests failed:

  # ./test_verifier | grep # | grep FAIL
  thesofproject#119/p calls: invalid kfunc call: ptr_to_mem to struct with non-scalar FAIL
  thesofproject#120/p calls: invalid kfunc call: ptr_to_mem to struct with nesting depth > 4 FAIL
  thesofproject#121/p calls: invalid kfunc call: ptr_to_mem to struct with FAM FAIL
  thesofproject#122/p calls: invalid kfunc call: reg->type != PTR_TO_CTX FAIL
  thesofproject#123/p calls: invalid kfunc call: void * not allowed in func proto without mem size arg FAIL
  thesofproject#124/p calls: trigger reg2btf_ids[reg->type] for reg->type > __BPF_REG_TYPE_MAX FAIL
  thesofproject#125/p calls: invalid kfunc call: reg->off must be zero when passed to release kfunc FAIL
  thesofproject#126/p calls: invalid kfunc call: don't match first member type when passed to release kfunc FAIL
  thesofproject#127/p calls: invalid kfunc call: PTR_TO_BTF_ID with negative offset FAIL
  thesofproject#128/p calls: invalid kfunc call: PTR_TO_BTF_ID with variable offset FAIL
  thesofproject#129/p calls: invalid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID FAIL
  thesofproject#130/p calls: valid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID FAIL
  thesofproject#486/p map_kptr: ref: reference state created and released on xchg FAIL

This is because the kfuncs in the loaded module are far away from
__bpf_call_base:

  ffff800002009440 t bpf_kfunc_call_test_fail1    [bpf_testmod]
  9000000002e128d8 T __bpf_call_base

The offset relative to __bpf_call_base does NOT fit in s32, which breaks
the assumption in BPF core. Enable bpf_jit_supports_far_kfunc_call() lifts
this limit.

Note that to reproduce the above result, tools/testing/selftests/bpf/config
should be applied, and run the test with JIT enabled, unpriv BPF enabled.

With this change, the test_verifier tests now all passed:

  # ./test_verifier
  ...
  Summary: 777 PASSED, 0 SKIPPED, 0 FAILED

Tested-by: Tiezhu Yang <[email protected]>
Signed-off-by: Hengqi Chen <[email protected]>
Signed-off-by: Huacai Chen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants