ASoC: Realtek codecs: wait codec init in hw_params #5424
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR aims to reduce the resume time for Realtek codecs by removing the blocking wait for codec initialization during resume and ensuring that regcache synchronization is conditionally performed based on the device’s unattach request.
- In the update_status functions, the return value from the I/O initialization call is captured and used.
- In the resume functions for rt722, rt712, and rt1320 codecs, the wait_for_completion_timeout block has been removed in favor of an immediate return.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| sound/soc/codecs/rt722-sdca-sdw.c | Removed wait_for_completion in dev_resume and conditionally syncing regcache. |
| sound/soc/codecs/rt712-sdca-sdw.c | Similar removal of waiting logic in dev_resume and additional syncing for multiple regmaps. |
| sound/soc/codecs/rt1320-sdw.c | Wait timeout removal in device resume with conditional regcache sync. |
Comments suppressed due to low confidence (3)
sound/soc/codecs/rt722-sdca-sdw.c:524
- Returning 0 unconditionally in the resume function bypasses the original timeout and error reporting, potentially masking hardware initialization failures. Please ensure that error conditions are handled appropriately elsewhere or document the assumptions made.
return 0;
sound/soc/codecs/rt712-sdca-sdw.c:479
- Replacing the wait_for_completion_timeout with an immediate return in the resume function may hide potential initialization issues. Please verify that failure conditions are addressed or clearly documented as an intentional design choice.
return 0;
sound/soc/codecs/rt1320-sdw.c:1496
- The removal of the timeout-based error handling in the device resume function could result in unreported initialization failures. Confirm that this change is safe and that any error scenarios are managed in other parts of the code or are no longer applicable.
return 0;
|
|
|
SOFCI TEST |
2 similar comments
|
SOFCI TEST |
|
SOFCI TEST |
bardliao
force-pushed
the
for-codec-resume-time
branch
from
961f16c to
7ac176b
Compare
|
Unfortunately, it will introduce a race condition issue. Programing SoundWire registers are required in stream process. And it has to be done after the codec is attached. Adding a new commit to wait in the sdw_stream_add_slave() function to avoid the race condition. @shumingfan FYI |
drivers/soundwire/stream.c
Outdated
| sdw_show_ping_status(slave->bus, true); | ||
|
|
||
| return -ETIMEDOUT; | ||
| } |
There was a problem hiding this comment.
Definitely have some concerns about moving this to the core, could we do this in the hw_params of the affected drivers instead? Not all drivers use this initialization complete stuff, and this will have impacts across everything.
There was a problem hiding this comment.
Yes, we could move this to the hw_params of each codec drivers. But given that complete_all(&slave->initialization_complete); is done in the core, and the codec should be initialized (or at least be attached) when it is added to the stream, it also makes sense to wait here, right? Would it harm in some corner case?
There was a problem hiding this comment.
BTW, I will remove wait_for_completion_timeout(&slave->initialization_complete for all codec drivers if we go with this commit. Right now I just do it for the 3 Realtek codec drivers because the 3 codecs are used by our CI machines. And I want to let CI confirm that I didn't break anything with the change.
There was a problem hiding this comment.
My problem really is that I am not sure that initialization_complete bit should live in the core at all (well unless we did something more detailed with actually probing after the device has shown up on the bus). Different drivers are likely to want to handle that initialization stuff in different ways so I am uncomfortable with it being in the core. Perhaps we could take an approach rather than adding it to the add_slave callback make it a separate helper function? That way you could still save duplicating the code in many codec drivers, but without forcing every codec to run that code?
waiting for slave->initialization_complete is commonly used by SoundWire codec drivers. Add a helper to reduce the duplicated code. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
bardliao
changed the title
bardliao
force-pushed
the
for-codec-resume-time
branch
2 times, most recently
from
2c5c476 to
7d86ddc
Compare
bardliao
force-pushed
the
for-codec-resume-time
branch
2 times, most recently
from
2dc6014 to
e335027
Compare
Move regmap sync to rt722_sdca_update_status() when unattach_request is set, and only do regmap sync in resume when no reattach needed. And move waiting codec init to hw_params when the codec really need to be initialized. The change can shorten the resume time. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Move regmap sync to rt712_sdca_update_status() when unattach_request is set, and only do regmap sync in resume when no reattach needed. And move waiting codec init to hw_params when the codec really need to be initialized. The change can shorten the resume time. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Move regmap sync to rt1320_update_status() when unattach_request is set, and only do regmap sync in resume when no reattach needed. And move waiting codec init to hw_params when the codec really need to be initialized. The change can shorten the resume time. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Move regmap sync to rt721_sdca_update_status() when unattach_request is set, and only do regmap sync in resume when no reattach needed. And move waiting codec init to hw_params when the codec really need to be initialized. The change can shorten the resume time. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Move regmap sync to rt715_sdca_update_status() when unattach_request is set, and only do regmap sync in resume when no reattach needed. And move waiting codec init to hw_params when the codec really need to be initialized. The change can shorten the resume time. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Move regmap sync to rt711_sdca_update_status() when unattach_request is set, and only do regmap sync in resume when no reattach needed. And move waiting codec init to hw_params when the codec really need to be initialized. The change can shorten the resume time. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Move regmap sync to rt711_update_status() when unattach_request is set, and only do regmap sync in resume when no reattach needed. And move waiting codec init to hw_params when the codec really need to be initialized. The change can shorten the resume time. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Move regmap sync to rt715_update_status() when unattach_request is set, and only do regmap sync in resume when no reattach needed. And move waiting codec init to hw_params when the codec really need to be initialized. The change can shorten the resume time. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Move regmap sync to rt700_update_status() when unattach_request is set, and only do regmap sync in resume when no reattach needed. And move waiting codec init to hw_params when the codec really need to be initialized. The change can shorten the resume time. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Move regmap sync to rt1316_update_status() when unattach_request is set, and only do regmap sync in resume when no reattach needed. And move waiting codec init to hw_params when the codec really need to be initialized. The change can shorten the resume time. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Move regmap sync to rt1318_update_status() when unattach_request is set, and only do regmap sync in resume when no reattach needed. And move waiting codec init to hw_params when the codec really need to be initialized. The change can shorten the resume time. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Move regmap sync to rt1308_update_status() when unattach_request is set, and only do regmap sync in resume when no reattach needed. And move waiting codec init to hw_params when the codec really need to be initialized. The change can shorten the resume time. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Move regmap sync to rt5682_update_status() when unattach_request is set, and only do regmap sync in resume when no reattach needed. And move waiting codec init to hw_params when the codec really need to be initialized. The change can shorten the resume time Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Move regmap sync to rt1017_sdca_update_status() when unattach_request is set, and only do regmap sync in resume when no reattach needed. And move waiting codec init to hw_params when the codec really need to be initialized. The change can shorten the resume time. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
bardliao
force-pushed
the
for-codec-resume-time
branch
from
e335027 to
7bf6f5b
Compare
|
SOFCI TEST |
Try to reduce the resume time