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

Stop dtrace suspend #3332

Merged
merged 2 commits into from
Jan 5, 2022

Conversation

libinyang
Copy link

When system enters s0ix, the dma trace won't be used. Otherwise,
the DMA will access the host memory, which will prevent entering
S0ix.

@libinyang
Copy link
Author

fixes #3034

/* non fatal */
dev_warn(sdev->dev,
"failed to enable trace after resume %d\n",
ret);
Copy link
Collaborator

Choose a reason for hiding this comment

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

one line or maybe 2 lines?

Copy link
Author

Choose a reason for hiding this comment

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

It will exceed 80 word/line. I checked the pm.c, and found this file always strictly follows 80w/l in other places.

Copy link
Collaborator

Choose a reason for hiding this comment

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

the limit is 100 chars

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe in new code you can use upto 100 chars and I think here it makes sense.

@@ -374,6 +374,12 @@ struct snd_sof_ipc {
struct snd_sof_ipc_msg msg;
};

enum sof_dtrace_state {
SOF_DTRACE_DISABLED = 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

= 0 is not necessary

return -EINVAL;

if (sdev->dtrace_state == SOF_DTRACE_STOPPED)
goto start_only;
Copy link
Collaborator

Choose a reason for hiding this comment

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

just start is good no instead start_only

Copy link
Collaborator

Choose a reason for hiding this comment

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

start alone does not carry any meaning here. I would have something here which say that we are going to start only and will skip the configuration and initialization

Copy link
Collaborator

Choose a reason for hiding this comment

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

not to split hair here. but goto start already mean you're skipping the next few things whatever they are.

@@ -566,12 +573,20 @@ void snd_sof_release_trace(struct snd_sof_dev *sdev)
dev_err(sdev->dev, "DMA_TRACE_FREE failed with error: %d\n", ret);
}

only_stop = (snd_sof_dsp_power_target(sdev) == SOF_DSP_PM_D0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

the boolean variable only_stop is not needed, just if (snd_sof_dsp_power_target(sdev) == SOF_DSP_PM_D0) is good enough

Copy link
Collaborator

Choose a reason for hiding this comment

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

besides you have already stopped, so sdev->dtrace_state = SOF_DTRACE_STOPPED; needs to go before the if()

Copy link
Collaborator

Choose a reason for hiding this comment

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

and the target state should be passed to the release function and not queried via the snd_sof_release_trace()

@@ -80,11 +80,18 @@ static int sof_resume(struct device *dev, bool runtime_resume)

/*
* Nothing further to be done for platforms that support the low power
* D0 substate.
* D0 substate, and we need to re-enable the DMA trace if it was stopped
Copy link
Collaborator

Choose a reason for hiding this comment

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

CHange to comments to 'Re-enabled trace and return when resuming from low-power D0 substate"

* D-states. Platform-specific substates, if any, should be
* handled by the platform-specific parts.
*/
static inline u32 snd_sof_dsp_power_target(struct snd_sof_dev *sdev)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd keep this function in pm.c and modify the signature for snd_sof_release_trace(struct snd_sof_dev *sdev, bool release) to pass true/false for releasing. and it should be false when entering D0i3 and true otherwise

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean make this function as external function? I'm OK with it. I don't understand "pass true/false". Do you mean make the snd_sof_dsp_power_target() return true/false?
This change is recommended by @ujfalusi . What's do you think, @ujfalusi ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mean make this function as external function? I'm OK with it. I don't understand "pass true/false". Do you mean make the snd_sof_dsp_power_target() return true/false? This change is recommended by @ujfalusi . What's do you think, @ujfalusi ?

I don't think I have suggested to move the snd_sof_dsp_power_target(), if I recall I was asking for you to pass the target power state directly to release_trace() so that it can decide to not send the IPC if the DSP is going to remain enabled.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can pass the pm_state to the snd_sof_release_trace() which is also sent to the SOF clients if they are interested.

Copy link
Author

Choose a reason for hiding this comment

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

@ujfalusi @ranj063 After a second thought, if we pass bool release to the function snd_sof_release_trace(), we don't need export snd_sof_dsp_power_target(). snd_sof_release_trace() is called in pm.c except in snd_sof_free_trace(), which we have definite 'release' value. So I will follow @ranj063 suggestion

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ujfalusi @ranj063 After a second thought, if we pass bool release to the function snd_sof_release_trace(), we don't need export snd_sof_dsp_power_target(). snd_sof_release_trace() is called in pm.c except in snd_sof_free_trace(), which we have definite 'release' value. So I will follow @ranj063 suggestion

No, we should pass the pm_state to the snd_sof_release_trace()

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree with @ujfalusi , please pass pm_state to snd_sof_release_trace() and keep snd_sof_dsp_power_target() in pm.c

Copy link
Collaborator

Choose a reason for hiding this comment

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

@libinyang @ujfalusi What happened to passing "pm_state" to snd_sof_release_trace() ? I'm not fond of passing a boolean, it's not very readable. If we want to keep the target PM state out of trace.c, I'd then rather have a separate "snd_sof_stop_trace()" call (and snd_sof_release_trace() could call that internally). That would make the code more readable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@libinyang @ujfalusi What happened to passing "pm_state" to snd_sof_release_trace() ? I'm not fond of passing a boolean, it's not very readable. If we want to keep the target PM state out of trace.c, I'd then rather have a separate "snd_sof_stop_trace()" call (and snd_sof_release_trace() could call that internally). That would make the code more readable.

@kv2019i, we should not pass boolean from pm.c to trace.c, I proposed to @libinyang that we should have a new snd_sof_trace_suspend(sdev, pm_state); and snd_sof_trace_resume(sdev); as an external interface. Internally we can pass around boolean, which we might need to to do the right thing in case of module removal.

Copy link
Collaborator

@ujfalusi ujfalusi left a comment

Choose a reason for hiding this comment

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

Please do not touch the snd_sof_dsp_power_target(), instead pass the pm_state to the trace release.

@@ -566,12 +573,20 @@ void snd_sof_release_trace(struct snd_sof_dev *sdev)
dev_err(sdev->dev, "DMA_TRACE_FREE failed with error: %d\n", ret);
}

only_stop = (snd_sof_dsp_power_target(sdev) == SOF_DSP_PM_D0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

and the target state should be passed to the release function and not queried via the snd_sof_release_trace()

@ujfalusi
Copy link
Collaborator

@libinyang, have you tried only the second patch? Does that also work?

With this series the dtrace in firmware remains enabled, if any new trace got printed, it will go the the first buffer and from there the firmware will initiate a mem to mem transfer to move it up to the host's buffer for the trace information.

How does the firmware knows that it should not initiate the DMA to do the copy from the 1st to host buffer? I suppose this is what makes it possible that we can reach lower power state.

@libinyang
Copy link
Author

@libinyang, have you tried only the second patch? Does that also work?

With this series the dtrace in firmware remains enabled, if any new trace got printed, it will go the the first buffer and from there the firmware will initiate a mem to mem transfer to move it up to the host's buffer for the trace information.

How does the firmware knows that it should not initiate the DMA to do the copy from the 1st to host buffer? I suppose this is what makes it possible that we can reach lower power state.

@ujfalusi Yes, the most import patch is the second one. My initial patch doesn't include the first one. I remember the first one is to meet your pm or auxiliary device or DMA restructure? I can't remember it clearly now. Please see:
#3099 (comment)
#3099 (comment)

@ujfalusi
Copy link
Collaborator

@libinyang, have you tried only the second patch? Does that also work?
With this series the dtrace in firmware remains enabled, if any new trace got printed, it will go the the first buffer and from there the firmware will initiate a mem to mem transfer to move it up to the host's buffer for the trace information.
How does the firmware knows that it should not initiate the DMA to do the copy from the 1st to host buffer? I suppose this is what makes it possible that we can reach lower power state.

@ujfalusi Yes, the most import patch is the second one. My initial patch doesn't include the first one. I remember the first one is to meet your pm or auxiliary device or DMA restructure? I can't remember it clearly now. Please see: #3099 (comment) #3099 (comment)

@libinyang, I have requested those changes because you needed the two different ways to stop the dtrace because the second ipc would fail.
Now it is not strictly required, but we might not want to disable the dtrace in firmware..

So, alone the second patch works?

@libinyang
Copy link
Author

libinyang commented Dec 16, 2021

@libinyang, I have requested those changes because you needed the two different ways to stop the dtrace because the second ipc would fail. Now it is not strictly required, but we might not want to disable the dtrace in firmware..

So, alone the second patch works?

@ujfalusi I understand your idea now. I have done the smoke test without the first patch on both adl-rvp-hda and adl chrome. Both are OK. The stress test is ongoing.

Only with the second patch, it will release dma trace totally. On this solution, I had 2 concerns:

  1. Will the dma trace log be lost, including the log at the end of the suspend and the log at the beginning of the resume.
    The answer is: It seems OK. No need to worry.
  2. The latency of suspend/resume.
    I tested on ADL chrome platform several times. With or without the first patch, the latency doesn't make big difference. Test with the first patch only be faster by 0.x ms. But When I did the test on ADL RVP, test with the first patch will be faster than the test without the first patch by 1 or 2 ms.
    So, do we care about this latency? Comments are appreciated.

Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

I think this looks very nice. A few changed needed as per comments made by Peter and Ranjani, but otherwise looks good to go. I can do a test to this tomorrow as well.

/* non fatal */
dev_warn(sdev->dev,
"failed to enable trace after resume %d\n",
ret);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe in new code you can use upto 100 chars and I think here it makes sense.

* D-states. Platform-specific substates, if any, should be
* handled by the platform-specific parts.
*/
static inline u32 snd_sof_dsp_power_target(struct snd_sof_dev *sdev)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree with @ujfalusi , please pass pm_state to snd_sof_release_trace() and keep snd_sof_dsp_power_target() in pm.c

@libinyang
Copy link
Author

Thanks @kv2019i @ujfalusi @ranj063 's comments. I will update the patch today.,

@ujfalusi I have done the stress test. The test result is positive. What do you think, removing the first patch or not?

@kv2019i
Copy link
Collaborator

kv2019i commented Dec 17, 2021

@ujfalusi @libinyang Wouldn't this fail on older FW without the first patch? I mean the TRACE_DMA_FREE is only supported in SOF1.9.1 and newer. With older FW, reinit of the DMA trace would fail when system comes out of S0ix, right?

Otherwise looks good. I tested the patch (slight modification to backport to chrome-5.10 kernel) and I can confirm this fixes #3034 .

@ujfalusi
Copy link
Collaborator

@ujfalusi @libinyang Wouldn't this fail on older FW without the first patch? I mean the TRACE_DMA_FREE is only supported in SOF1.9.1 and newer. With older FW, reinit of the DMA trace would fail when system comes out of S0ix, right?

Otherwise looks good. I tested the patch (slight modification to backport to chrome-5.10 kernel) and I can confirm this fixes #3034 .

@kv2019i, the point of this series is to not send the SOF_IPC_TRACE_DMA_FREE because it fails on older firmware.
It would be much cleaner to use the full sequence, but it turns out that we have more issues on the kernel side with re-configuration: thesofproject/sof#5106

The Issue I have with the let's do this 'workaround' without involving the firmware is that now the firmware will happily going to try to push trace data from it's dmatb to host buffer with dma_copy_to_host_nowait() and it will believe that all is good (it is not).

But I think this is the safest thing that we can do.

@libinyang
Copy link
Author

@ujfalusi @libinyang Wouldn't this fail on older FW without the first patch? I mean the TRACE_DMA_FREE is only supported in SOF1.9.1 and newer. With older FW, reinit of the DMA trace would fail when system comes out of S0ix, right?

Otherwise looks good. I tested the patch (slight modification to backport to chrome-5.10 kernel) and I can confirm this fixes #3034 .

@kv2019i Thanks for the test. I agree with your concern. Besides, I talked with @ujfalusi last Friday offline. And we had some other concerns about removing the first patch. So we will keep the first patch.

@libinyang
Copy link
Author

patch updated.
Changelog: update the patches based on the review comments.

return -EINVAL;

if (sdev->dtrace_state == SOF_DTRACE_STOPPED)
goto start_only;
Copy link
Collaborator

Choose a reason for hiding this comment

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

not to split hair here. but goto start already mean you're skipping the next few things whatever they are.

@@ -221,7 +221,7 @@ static int sof_suspend(struct device *dev, bool runtime_suspend)
sof_tear_down_pipelines(sdev, false);

/* release trace */
snd_sof_release_trace(sdev);
snd_sof_release_trace(sdev, 0);
Copy link
Collaborator

@ranj063 ranj063 Dec 20, 2021

Choose a reason for hiding this comment

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

the commit message needs to be expanded to bit to explain how you're using the new state. It is very hard to follow what you're doing here without any explanation. And why 0? did you mean false?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, you must pass the pm_state

@@ -566,12 +572,19 @@ void snd_sof_release_trace(struct snd_sof_dev *sdev)
dev_err(sdev->dev, "DMA_TRACE_FREE failed with error: %d\n", ret);
}

if (only_stop) {
sdev->dtrace_state = SOF_DTRACE_STOPPED;
Copy link
Collaborator

Choose a reason for hiding this comment

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

please take previous feedback into account. trace was topped when the IPC to stop is completed succesfully. Please set it to that right state right after the IPC is successful. this if should only have the goto.

Copy link
Author

Choose a reason for hiding this comment

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

please take previous feedback into account. trace was topped when the IPC to stop is completed succesfully. Please set it to that right state right after the IPC is successful. this if should only have the goto.

@ranj063 the trigger stop won't set this flag. And at that time, we don't know which value we should set. At that time, it can be SOF_DTRACE_STOPPED or SOF_Dtrace_DISABLED

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry @libinyang the final state can be DISABLED or STOPPPED depending on what you end up doing. But if the IPC is successful, the flag should be set right after.

@@ -582,7 +595,7 @@ void snd_sof_free_trace(struct snd_sof_dev *sdev)
if (!sdev->dtrace_is_supported)
return;

snd_sof_release_trace(sdev);
snd_sof_release_trace(sdev, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

false? I think we should pass what the final state we want instead of true/false. For example if you want to stop and release, you pass SOF_DTRACE_DISABLED and SOF_DTRACE_STOPPED otherwise.

Copy link
Author

Choose a reason for hiding this comment

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

false? I think we should pass what the final state we want instead of true/false. For example if you want to stop and release, you pass SOF_DTRACE_DISABLED and SOF_DTRACE_STOPPED otherwise.

@ujfalusi What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

@ranj063 @ujfalusi Let me explain why I prefer void snd_sof_release_trace(struct snd_sof_dev *sdev, bool only_stop). Actually, I'm also satisfied with @ranj063 's first suggestion snd_sof_release_trace(struct snd_sof_dev *sdev, bool release) . And after talking with @ujfalusi , we thought 'only_stop' may be better. Anyway, the 2 name are both OK to me. And I'm prefer bool only_stop or bool release to sof_dtrace_state. This is because:

  1. snd_sof_release_trace() is just executing what the caller tells it to do. This function should not determine what to do itself. But maybe the caller need different behavior in the same pm state in different scenarios in the future, for example releasing the dma trace even it is in D0 state. So this function should be flexible and easy. Based on this theory, snd_sof_release_trace() should not judge what to do itself, but let the caller tells it what to do.
  2. In the snd_sof_free_trace, the target state is not D3. We didn't set the audio state to D3 anywhere in the free. So we should set the parameter to D0 if we use sof_dtrace_state for the precise description of the power state. But this will make the function do the wrong thing. (In the free case, we must set the state to D3). But if we set the parameter to D3, this is misleading.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@libinyang, from the pm.c on suspend you should pass the pm_state to the dtrace, likely by adding a new function: snd_sof_trace_suspend(sdev, pm_state) (align the name with the rest of external trace API).
Internally in trace.c you could then convert this pm_state to whatever bool you end up using for snd_sof_release_trace(), but this function must be static to trace.c.
For the resume part I would add a snd_sof_trace_resume(sdev) for symmetry and sanity. Sure, if we were in D3 we will do a full startup (sans host memory and buffer allocation, debugfs and whatnot), but at the end it we are resuming the dtrace functionality after a suspend.

Copy link
Collaborator

Choose a reason for hiding this comment

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

passing the pm_state makes a lot more sense than a bool. That way the trace logic makes the decision on what the final state should be during release

Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be indeed considered from both directions @libinyang , but if you consider the caller here, e.g. pm.c, it's not really ideal place either to decide what exactly should happen when trace is released. And if you pass a boolean, then it's really pm.c as the caller here that needs to know details about how trace works. We want to decouple SOF core (like pm.c) from clients (like trace.c). DMA trace is still a bit of a special case, but basicly same/similar principles should apply. So in the end snd_sof_release_trace() call should be similar to sof_suspend_clients() and for that we do have:
"sof_suspend_clients(sdev, pm_state);". I.e. the idea is that the client (trace, probe, audio, injector) knows best what actions to take given core is moving to D3 or D0i.

Copy link
Author

Choose a reason for hiding this comment

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

@kv2019i , You comments make sense to me. Yes, if we want to make trace to be a client, we should use the decoupled implementation, and let trace decide what to do. And if so, @ujfalusi 's suggestion looks good to me. BTW, we will really make the trace to be a client finally, right?

I will refine the patch and send it out as a RFC without full test. The full test takes to much time. After we all agree with the new PR, I will do the full test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The second parameter for snd_sof_release_trace() is bool, so pass false

@libinyang
Copy link
Author

not to split hair here. but goto start already mean you're skipping the next few things whatever they are.

@ranj063 I agree with you. But it seems @ujfalusi has different idea and no one objects his comments. @ujfalusi What do you think of using 'start' instead of start_only?

Copy link
Collaborator

@ujfalusi ujfalusi left a comment

Choose a reason for hiding this comment

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

Please do not pass a boolean', the dtrace should receive the pm_state and decide what to do based on that.

@@ -221,7 +221,7 @@ static int sof_suspend(struct device *dev, bool runtime_suspend)
sof_tear_down_pipelines(sdev, false);

/* release trace */
snd_sof_release_trace(sdev);
snd_sof_release_trace(sdev, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

No, you must pass the pm_state

@libinyang
Copy link
Author

PR updated based on the review comments. smoke test passed. Stress test is ongoing.
changelog:

  1. add snd_sof_trace_suspend(sdev, pm_state) and snd_sof_trace_resume(sdev), so let trace handle the suspend/resume itself
  2. start_only -> start
  3. move sdev->dtrace_state = SOF_DTRACE_STOPPED; after the IPC is successful

if (pm_state == SOF_DSP_PM_D0)
snd_sof_release_trace(sdev, 1);
else
snd_sof_release_trace(sdev, 0);
Copy link

Choose a reason for hiding this comment

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

how about just

snd_sof_release_trace(sdev, pm_state == SOF_DSP_PM_D0);

Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Thanks @libinyang ! Code looks now good. Remaining issues are minor and only bigger issue the signature the trace suspend call. I understood @ujfalusi specifically wants to align with the client interface and pass "struct pm_message" as the arg. Functionality this has little impact as the pm_message only has one integer field, but to keep the code uniform, I think we should align.

else
snd_sof_release_trace(sdev, 0);

/* always return 0 */
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment on L598 is not really necessary.

sound/soc/sof/trace.c Show resolved Hide resolved
@@ -582,7 +587,8 @@ void sof_print_oops_and_stack(struct snd_sof_dev *sdev, const char *level,
u32 panic_code, u32 tracep_code, void *oops,
struct sof_ipc_panic_info *panic_info,
void *stack, size_t stack_words);
int snd_sof_init_trace_ipc(struct snd_sof_dev *sdev);
int snd_sof_trace_suspend(struct snd_sof_dev *sdev, int pm_state);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think to align with the SOF client interface, this should be:

int sof_suspend_clients(struct snd_sof_dev *sdev, pm_message_t pm_state);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, you need to pass the pm_message_t

@@ -213,6 +220,7 @@ static int sof_suspend(struct device *dev, bool runtime_suspend)

/* Skip to platform-specific suspend if DSP is entering D0 */
if (target_state == SOF_DSP_PM_D0) {
snd_sof_trace_suspend(sdev, SOF_DSP_PM_D0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

And to align with SOF client, this would be:

snd_sof_trace_suspend(sdev, pm_state);

/* release trace */
snd_sof_release_trace(sdev);
/* suspend DMA trace */
snd_sof_trace_suspend(sdev, SOF_DSP_PM_D3);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to pass the pm_state, not a specific state which might not be the right state.

sound/soc/sof/sof-priv.h Show resolved Hide resolved
@@ -582,7 +587,8 @@ void sof_print_oops_and_stack(struct snd_sof_dev *sdev, const char *level,
u32 panic_code, u32 tracep_code, void *oops,
struct sof_ipc_panic_info *panic_info,
void *stack, size_t stack_words);
int snd_sof_init_trace_ipc(struct snd_sof_dev *sdev);
int snd_sof_trace_suspend(struct snd_sof_dev *sdev, int pm_state);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, you need to pass the pm_message_t

sound/soc/sof/trace.c Show resolved Hide resolved
@@ -582,7 +595,7 @@ void snd_sof_free_trace(struct snd_sof_dev *sdev)
if (!sdev->dtrace_is_supported)
return;

snd_sof_release_trace(sdev);
snd_sof_release_trace(sdev, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The second parameter for snd_sof_release_trace() is bool, so pass false

@libinyang
Copy link
Author

patches updated based on the feedback

Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Code is ok, but the first commit message needs a refresh. My suggestion inline.

sound/soc/sof/sof-priv.h Show resolved Hide resolved
@libinyang
Copy link
Author

Stress test passed.

kv2019i
kv2019i previously approved these changes Dec 22, 2021
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Thanks @libinyang , looks good now!

sdev->dtrace_draining = true;
wake_up(&sdev->trace_sleep);
}
EXPORT_SYMBOL(snd_sof_release_trace);

int snd_sof_trace_suspend(struct snd_sof_dev *sdev, pm_message_t pm_state)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe make this return void too?

Copy link
Author

Choose a reason for hiding this comment

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

@ranj063 Not sure which is good, void or int? I saw sof_suspend_clients() returns int. To make them aligned, I used int. @ujfalusi What's your option?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, the suspend for the clients also returns int but it ignores the return value from the auxdev's suspend.
I'm not sure what would we do if something fail. In most cases this is problematic if we keep the DSP on.
Not sure, I think the clients should be returning error and if it make sense for the dtrace, it should also, if not then make it void

ujfalusi
ujfalusi previously approved these changes Dec 22, 2021
Copy link
Collaborator

@ujfalusi ujfalusi left a comment

Choose a reason for hiding this comment

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

@libinyang, looks nice, thanks for the updates and the persistence

@marc-hb
Copy link
Collaborator

marc-hb commented Dec 23, 2021

Trying to draw the attention of some DMA trace experts on this hopefully unrelated ring buffer corruption: thesofproject/sof/issues/5120

It's happening with Zephyr for the most part but not 100% Zephyr.

Anything helps! Off-topic sorry.

Change the interface to stop the DMA trace for suspend. Replace the
snd_sof_init_trace_ipc() and snd_sof_release_trace() calls with more
explicit interface for PM (the sole user for this interface).

The new snd_sof_trace_suspend() call takes the target PM state as argument,
allowing the trace implementation to decide how to handle the transition.
Use this information to release DMA resources only if DSP is suspended and
will not remain in D0.

Signed-off-by: Peter Ujfalusi <[email protected]>
Signed-off-by: Libin Yang <[email protected]>
When system enters s0ix, the dma trace won't be used. Otherwise,
the DMA will access the host memory, which will prevent entering
S0ix. Driver has notified firmware not to send message through
dma trace. Let's also trigger stop dma trace in driver side.

Signed-off-by: Libin Yang <[email protected]>
@libinyang libinyang dismissed stale reviews from ujfalusi and kv2019i via 2173e4f December 24, 2021 08:14
@ujfalusi
Copy link
Collaborator

ujfalusi commented Jan 4, 2022

SOFCI TEST

@kv2019i kv2019i requested review from lyakh and ranj063 January 4, 2022 12:38
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.

6 participants