On Mon, 2025-05-05 at 21:03 +0300, Tomi Valkeinen wrote: Hello,
Hi,
On 05/05/2025 20:47, Vitor Soares wrote:
On Mon, 2025-05-05 at 18:30 +0300, Tomi Valkeinen wrote:
Hi,
On 05/05/2025 17:45, Vitor Soares wrote:
On Tue, 2025-04-29 at 09:32 +0300, Tomi Valkeinen wrote:
Hi,
On 28/04/2025 12:40, Vitor Soares wrote:
From: Vitor Soares vitor.soares@toradex.com
The deprecated UNIVERSAL_DEV_PM_OPS() macro uses the provided callbacks for both runtime PM and system sleep. This causes the DSI clocks to be disabled twice: once during runtime suspend and again during system suspend, resulting in a WARN message from the clock framework when attempting to disable already-disabled clocks.
[ 84.384540] clk:231:5 already disabled [ 84.388314] WARNING: CPU: 2 PID: 531 at /drivers/clk/clk.c:1181 clk_core_disable+0xa4/0xac ... [ 84.579183] Call trace: [ 84.581624] clk_core_disable+0xa4/0xac [ 84.585457] clk_disable+0x30/0x4c [ 84.588857] cdns_dsi_suspend+0x20/0x58 [cdns_dsi] [ 84.593651] pm_generic_suspend+0x2c/0x44 [ 84.597661] ti_sci_pd_suspend+0xbc/0x15c [ 84.601670] dpm_run_callback+0x8c/0x14c [ 84.605588] __device_suspend+0x1a0/0x56c [ 84.609594] dpm_suspend+0x17c/0x21c [ 84.613165] dpm_suspend_start+0xa0/0xa8 [ 84.617083] suspend_devices_and_enter+0x12c/0x634 [ 84.621872] pm_suspend+0x1fc/0x368
To address this issue, replace UNIVERSAL_DEV_PM_OPS() with DEFINE_RUNTIME_DEV_PM_OPS(), which avoids redundant suspend/resume calls by checking if the device is already runtime suspended.
Cc: stable@vger.kernel.org # 6.1.x Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver") Signed-off-by: Vitor Soares vitor.soares@toradex.com
drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c index b022dd6e6b6e..62179e55e032 100644 --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c @@ -1258,7 +1258,7 @@ static const struct mipi_dsi_host_ops cdns_dsi_ops = { .transfer = cdns_dsi_transfer, }; -static int __maybe_unused cdns_dsi_resume(struct device *dev) +static int cdns_dsi_resume(struct device *dev) { struct cdns_dsi *dsi = dev_get_drvdata(dev); @@ -1269,7 +1269,7 @@ static int __maybe_unused cdns_dsi_resume(struct device *dev) return 0; } -static int __maybe_unused cdns_dsi_suspend(struct device *dev) +static int cdns_dsi_suspend(struct device *dev) { struct cdns_dsi *dsi = dev_get_drvdata(dev); @@ -1279,8 +1279,8 @@ static int __maybe_unused cdns_dsi_suspend(struct device *dev) return 0; } -static UNIVERSAL_DEV_PM_OPS(cdns_dsi_pm_ops, cdns_dsi_suspend, cdns_dsi_resume, - NULL); +static DEFINE_RUNTIME_DEV_PM_OPS(cdns_dsi_pm_ops, cdns_dsi_suspend, + cdns_dsi_resume, NULL);
I'm not sure if this, or the UNIVERSAL_DEV_PM_OPS, is right here. When the system is suspended, the bridge drivers will get a call to the *_disable() hook, which then disables the device. If the bridge driver would additionally do something in its system suspend hook, it would conflict with normal disable path.
I think bridges/panels should only deal with runtime PM.
Tomi
In the proposed change, we make use of pm_runtime_force_suspend() during system-wide suspend. If the device is already suspended, this call is a no-op and disables runtime PM to prevent spurious wakeups during the suspend period. Otherwise, it triggers the device’s runtime_suspend() callback.
I briefly reviewed other bridge drivers, and those that implement runtime PM appear to follow a similar approach, relying solely on runtime PM callbacks and using pm_runtime_force_suspend()/resume() to handle system-wide transitions.
Yes, I see such a solution in some of the bridge and panel drivers. I'm probably missing something here, as I don't think it's correct.
Why do we need to set the system suspend/resume hooks? What is the scenario where those will be called, and the pm_runtime_force_suspend()/resume() do something that's not already done via the normal DRM pipeline enable/disable?
Tomi
I'm not a DRM expert, but my understanding is that there might be edge cases where the system suspend sequence occurs without the DRM core properly disabling the bridge — for example, due to a bug or if the bridge is not bound to an active pipeline. In such cases, having suspend/resume callbacks ensures that the device is still properly suspended and resumed.
Additionally, pm_runtime_force_suspend() disables runtime PM for the device during system suspend, preventing unintended wakeups (e.g., via IRQs, delayed work, or sysfs access) until pm_runtime_force_resume() is invoked.
From my perspective, the use of pm_runtime_force_suspend() and pm_runtime_force_resume() serves as a safety mechanism to guarantee a well- defined and race-free state during system suspend.
But then we must be sure that the suspend sequence is just right.
At least in tidss's case, tidss_drv.c has tidss_suspend() which calls drm_mode_config_helper_suspend(), which, if I recall right, will then disable the pipeline. This must happen before the bridge's system suspend call, otherwise the bridge might go to suspend while the pipeline is still running, which might cause errors on the still-running pipeline entities, and probably crash the bridge's disable() call. If a bridge is a platform device, I don't think there's any ordering between the tidss's and the bridge's suspend calls.
If the bridge is not bound to a pipeline, why would it be enabled in the first place?
For the bug case... We're in random territory, then. If the driver is bugging, are you sure it's safe and useful to suspend it? Or would it be better to not do anything...
I'm not nacking the patch, as this approach seems to be used in multiple drivers. It just rings multiple alarm bells here, and I don't understand how exactly it's supposed to work. That said, the driver is using UNIVERSAL_DEV_PM_OPS(), so I think switching to DEFINE_RUNTIME_DEV_PM_OPS() is at least not worse (well, I can't be quite sure even about that =).
Tomi
I conducted further tests based on your concerns, specifically regarding the suspend ordering between the tidss_suspend() and the bridge suspend. Here are my observations: - The bridge (controlled via TI SCI PD) suspends after tidss_suspend(), which uses device-specific PM operations (platform_pm_suspend). - I attempted to influence the probe/suspend order via DT node placement and delays, but that had no effect on suspend sequencing. - I added some debug prints and the pm_runtime_force_suspend() is invoked before cdns_dsi_suspend(). However, I did not observe any misbehavior during suspend/resume. - I also tested with only runtime PM support in the driver (without pm_runtime_force_suspend/resume()), and I couldn't detect any functional difference nor the issue originally addressed in this patch.
Given that, I will send a v2 of the patch implementing only runtime PM support. If issues arise in the future due to the lack of explicit pm_runtime_force_suspend/resume() handling, we can revisit and address them at that time with clearer justification.
Best regards, Vitor Soares