IssueTracker

There are two problems that have clear solutions:

1. The parent of parkable RCGs is always assumed to be XO at registration time even when that isn't true
2. The rcg root enable bit is set in the bootloader and so disabling the child branch doesn't turn off the rcg as expected

We can fix these problems by clearing the root enable bit and reading the rcg at clk registration time so that the parent is in sync and the root enable bit is cleared.

This still leaves us with the problem that the RCG isn't parked when a child branch is disabled during late init. To fix that we can simply mark those branches as CLK_IGNORE_UNUSED and let the display driver eventually probe and turn the clks off.

This is a problem with the chromeos-5.15 kernel now.

[    0.508609] disp_cc_mdss_rot_clk status stuck at 'on'
[    0.508621] WARNING: CPU: 7 PID: 1 at drivers/clk/qcom/clk-branch.c:92 clk_branch_toggle+0x124/0x16c
[    0.508628] Modules linked in:
[    0.508633] CPU: 7 PID: 1 Comm: swapper/0 Not tainted 5.15.148-21808-gf0ad352e8fc4 #1 648f4578b1491a64607b0febe095e018a7a58716
[    0.508637] Hardware name: Google CoachZ (rev1 - 2) (DT)
[    0.508640] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    0.508643] pc : clk_branch_toggle+0x124/0x16c
[    0.508646] lr : clk_branch_toggle+0x120/0x16c

I've got a two patch series to fix this on sc7180. It implements number 1 and 2 bullet points from comment #3 and then adds CLK_IGNORE_UNUSED to disp_cc_pll0 and disp_cc_mdss_rot_clk so that the rot branch and display pll can't be turned off during late init when the gdsc is already disabled.

I have another idea, which is to implement a struct clk_ops::disable_unused clk_op that we would set on a parkable rcg branch so that when the branch is found to be enabled during disable_unused, we go up to the parent rcg and park it on XO before disabling the branch. This may solve it better because we don't need to set the CLK_IGNORE_UNUSED flag. But if clk_ignore_unused is on the kernel commandline we'll never run this code to park the rcg on XO, and we still have the problem where the display pll (disp_cc_pll0) may be disabled because the enable_count of the PLL went to zero while disp_cc_mdss_rot_clk is enabled and sourcing from disp_cc_pll0.

In fact, I see that disp_cc_pll0 is disabled and enabled many times before disabling unused clks, so it must be causing disp_cc_mdss_rot_clk to get stuck on after fixing the parent reported for disp_cc_mdss_rot_clk and disp_cc_mdss_mdp_clk to be disp_cc_pll0. Maybe we should manually disable disp_cc_mdss_rot_clk at display driver probe to avoid this scenario. We don't care about this clk being on through boot on sc7180 so this seems acceptable.

TL;DR: the best fix is to implement parking from the gdsc disable path. Doing so will require some register spinlock and pointers to the rcgs that the gdsc manages. And when the gdsc is turned on, we can skip restoring anything because all clks should be disabled when a gdsc is disabled. The corner case is handoff from boot, where qcom,sc7180-mdss compatible node is the device that is runtime resuming and suspending, causing the display pll to be disabled and the gdsc to be turned off, when it really needs to not do that until the child nodes have all probed, e.g. qcom,sc7180-dpu compatible node, and those child devices enable and disable clks like DISP_CC_MDSS_ROT_CLK.

In fact, I see that disp_cc_pll0 is disabled and enabled many times before disabling unused clks

This also points out that the "qcom,sc7180-mdss" driver is runtime suspending before child devices like "qcom,sc7180-dpu" probe. That sounds wrong.

I hoped that it would be enough to propagate status of the children to the MDSS device

I'm still tracking down where mdss first gets turned off. I think device links duplicate what parent child relationships do with runtime PM, so I hope they're not necessary. Can we keep mdss runtime PM active until DPU probes and enables the rotator clk?

I put some printks in

[    0.303093] sboyd: enabling runtime pm on mdss
[    0.305025] sboyd: disabling mdss_gdsc
[    0.366780] sboyd: disabling mdss_gdsc
[    0.369592] sboyd: disabling mdss_gdsc
[    0.512409] sboyd: disabling mdss_gdsc
[    0.519450] sboyd: disabling mdss_gdsc
[    0.521547] [drm:dpu_kms_init:1196] sboyd: enabling runtime pm on dpu
[    0.521560] [drm:dpu_kms_hw_init:1042] sboyd: runtime resume and get ae01000.display-controller
[    0.522558] [drm:dpu_kms_hw_init:1160] sboyd: runtime put sync ae01000.display-controller
[    0.522821] sboyd: disabling mdss_gdsc

all those disabling mdss_gdsc lines between mdss and dpu are unexpected.

The first rpm_resume() is from of_platform_populate() where __driver_probe_device() calls pm_runtime_get_sync(dev->parent). I have this dump_stack()

[    0.291929] CPU: 6 PID: 11 Comm: kworker/u16:0 Not tainted 6.8.0-00005-g0e2cb559efb7-dirty #1 8bb035a8a74abeafcde1984f53c37e9e728e208b
[    0.291934] Hardware name: Google Lazor (rev3 - 8) with KB Backlight (DT)
[    0.291938] Workqueue: events_unbound deferred_probe_work_func
[    0.291953] Call trace:
[    0.291956]  dump_backtrace+0xf0/0x10c
[    0.291963]  show_stack+0x20/0x2c
[    0.291967]  dump_stack_lvl+0x84/0xac
[    0.291971]  dump_stack+0x18/0x30
[    0.291974]  rpm_resume+0x334/0x5ac
[    0.291979]  __pm_runtime_resume+0x54/0x98
[    0.291984]  __device_attach+0xe0/0x170
[    0.291989]  device_initial_probe+0x1c/0x28
[    0.291993]  bus_probe_device+0x48/0xa4
[    0.291997]  device_add+0x210/0x3cc
[    0.292001]  of_device_add+0x40/0x50
[    0.292006]  of_platform_device_create_pdata+0x9c/0x118
[    0.292010]  of_platform_bus_create+0x1c8/0x370
[    0.292015]  of_platform_populate+0x7c/0xe4
[    0.292019]  mdss_probe+0x260/0x364

and then it increments another time for pm_runtime_get_suppliers(dev) because fw_devlink decided that the parent is supplying something. Then it looks like __driver_probe_device() calls pm_runtime_put(dev->parent) and pm_runtime_put_suppliers(dev) and that runtime suspends mdss. We can probably put a pm_runtime_get() call around of_platform_populate() to simplify this, but if the dpu driver isn't probing because it isn't builtin or it isn't created as a runtime active device I don't know how we keep mdss out of runtime suspend. I'll look at usb for clues.

The doc has some info:

However, if the device has a parent and the parent’s runtime PM is enabled, calling
pm_runtime_set_active() for the device will affect the parent, unless the parent’s
‘power.ignore_children’ flag is set. Namely, in that case the parent won’t be able 
to suspend at run time, using the PM core’s helper functions, as long as the child’s 
status is ‘active’, even if the child’s runtime PM is still disabled (i.e. 
pm_runtime_enable() hasn’t been called for the child yet or pm_runtime_disable() 
has been called for it). For this reason, once pm_runtime_set_active() has been 
called for the device, pm_runtime_enable() should be called for it too as soon as 
reasonably possible or its runtime PM status should be changed back to ‘suspended’ 
with the help of pm_runtime_set_suspended().

so maybe we should probe mdss, runtime_pm_get() mdss, set all child devices to active, and then put the runtime pm reference on mdss. If a child is active and not probed yet then I think it will keep mdss runtime active, and thus mdss_gdsc will stay enabled as well.

I applied this patch but I ran into an SError at boot on lazor.

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index 35423d10aafa..106f51de917d 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -484,6 +484,12 @@ static int mdss_probe(struct platform_device *pdev)
 
        platform_set_drvdata(pdev, mdss);
 
+       ret = pm_runtime_resume_and_get(dev);
+       if (ret) {
+               msm_mdss_destroy(mdss);
+               return ret;
+       }
+
        /*
         * MDP5/DPU based devices don't have a flat hierarchy. There is a top
         * level parent: MDSS, and children: MDP5/DPU, DSI, HDMI, eDP etc.
@@ -494,10 +500,11 @@ static int mdss_probe(struct platform_device *pdev)
        if (ret) {
                DRM_DEV_ERROR(dev, "failed to populate children devices\n");
                msm_mdss_destroy(mdss);
-               return ret;
        }
 
-       return 0;
+       pm_runtime_put(dev);
+
+       return ret;
 }
 
 static void mdss_remove(struct platform_device *pdev)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index b7708a06dc78..7f8b9d538748 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -20,6 +20,7 @@
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/sysfb.h>
 
 #include "of_private.h"
@@ -181,6 +182,14 @@ static struct platform_device *of_platform_device_create_pdata(
        dev->dev.platform_data = platform_data;
        of_msi_configure(&dev->dev, dev->dev.of_node);
 
+       /*
+        * Initialize devices populated by runtime PM active devices as runtime
+        * PM active as well so that the parent can't runtime suspend until all
+        * active children are runtime suspended.
+        */
+       if (parent && pm_runtime_active(parent))
+               pm_runtime_set_active(&dev->dev);
+
        if (of_device_add(dev) != 0) {
                platform_device_put(dev);
                goto err_clear_flag;

It looks like it's coming from the DP driver.

[    0.528263] msm_dpu ae01000.display-controller: bound ae94000.dsi (ops 0xffffffe24abe1fe0)
[    0.528521] SError Interrupt on CPU7, code 0x00000000be000411 -- SError
[    0.528524] CPU: 7 PID: 122 Comm: kworker/7:25 Tainted: G        W          6.8.0-00005-g0e2cb559efb7-dirty #1 99a8035332e11cd1ac2ef7f9244e5239b4958e1f
[    0.528526] Hardware name: Google Lazor (rev3 - 8) with KB Backlight (DT)
[    0.528527] Workqueue: pm pm_runtime_work
[    0.528531] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    0.528534] pc : mutex_unlock+0x28/0x60
[    0.528537] lr : dp_aux_deinit+0x40/0x50
[    0.528541] sp : ffffffc080d3bbc0
[    0.528542] pmr_save: 000000e0
[    0.528543] x29: ffffffc080d3bbc0 x28: 00000000fffffff5 x27: ffffffc080d3bcf8
[    0.528546] x26: 0000000000000002 x25: 0000000000000000 x24: 0000000000000000
[    0.528549] x23: ffffff80832faae0 x22: ffffffe24a3c4c6c x21: 0000000000000000
[    0.528551] x20: ffffff8082565890 x19: ffffff80825658e8 x18: 0000000000000000
[    0.528554] x17: 0000000000000000 x16: 0000000000000007 x15: ffffffe24b11aa70
[    0.528557] x14: 000000000000007c x13: 0000000000c0000e x12: 0000000000f0000f
[    0.528559] x11: 00000000510f8040 x10: 0000000000000000 x9 : ffffff808251da00
[    0.528562] x8 : ffffff808251da00 x7 : 0000000000000000 x6 : 672e38462d4bb1a6
[    0.528564] x5 : 0000000000000001 x4 : 000000001f8066e6 x3 : 0000000000000010
[    0.528567] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffffff8082565890
[    0.528570] Kernel panic - not syncing: Asynchronous SError Interrupt
[    0.528571] CPU: 7 PID: 122 Comm: kworker/7:25 Tainted: G        W          6.8.0-00005-g0e2cb559efb7-dirty #1 99a8035332e11cd1ac2ef7f9244e5239b4958e1f
[    0.528574] Hardware name: Google Lazor (rev3 - 8) with KB Backlight (DT)
[    0.528574] Workqueue: pm pm_runtime_work
[    0.528577] Call trace:
[    0.528578]  dump_backtrace+0xf0/0x10c
[    0.528580]  show_stack+0x20/0x2c
[    0.528582]  dump_stack_lvl+0x84/0xac
[    0.528584]  dump_stack+0x18/0x30
[    0.528585]  panic+0x13c/0x348
[    0.528589]  nmi_panic+0x4c/0x90
[    0.528590]  arm64_serror_panic+0x70/0x7c
[    0.528593]  is_valid_bugaddr+0x0/0x10
[    0.528595]  el1h_64_error_handler+0x34/0x48
[    0.528596]  el1h_64_error+0x7c/0x80
[    0.528598]  mutex_unlock+0x28/0x60
[    0.528600]  dp_aux_deinit+0x40/0x50
[    0.528603]  dp_pm_runtime_suspend+0x84/0xa4
[    0.528605]  pm_generic_runtime_suspend+0x30/0x44
[    0.528607]  genpd_runtime_suspend+0xfc/0x220
[    0.528611]  __rpm_callback+0x84/0x138
[    0.528614]  rpm_callback+0x30/0x88
[    0.528616]  rpm_suspend+0x168/0x520
[    0.528619]  pm_runtime_work+0x84/0xa8
[    0.528622]  process_scheduled_works+0x1ac/0x3f4
[    0.528625]  worker_thread+0x208/0x31c
[    0.528628]  kthread+0x100/0x1b8
[    0.528630]  ret_from_fork+0x10/0x20
[    0.528633] SMP: stopping secondary CPUs
[    0.528727] Kernel Offset: 0x21c9e00000 from 0xffffffc080000000
[    0.528729] PHYS_OFFSET: 0x80000000
[    0.528730] CPU features: 0x0,80000061,5002814a,e100720b
[    0.528733] Memory Limit: none

I also see an error with geni so maybe child devices that are of_platform_populate()d need more care if the parent is runtime active before populating.

[    0.259996] gcc_qupv3_wrap1_s0_clk already disabled
[    0.260026] WARNING: CPU: 0 PID: 10 at drivers/clk/clk.c:1178 clk_core_disable+0x90/0x1b0
[    0.260049] Modules linked in:
[    0.260060] CPU: 0 PID: 10 Comm: kworker/0:1 Not tainted 6.8.0-00005-g0e2cb559efb7-dirty #1 9d3d495907444b6453520dc7dde1f9993743cd6d
[    0.260072] Hardware name: Google Lazor (rev3 - 8) with KB Backlight (DT)
[    0.260079] Workqueue: pm pm_runtime_work
[    0.260096] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    0.260106] pc : clk_core_disable+0x90/0x1b0
[    0.260117] lr : clk_core_disable+0x90/0x1b0
[    0.260127] sp : ffffffc0800a3b60
[    0.260134] pmr_save: 00000060
[    0.260139] x29: ffffffc0800a3b60 x28: 00000000fffffff5 x27: ffffffc0800a3cf8
[    0.260154] x26: 0000000000000002 x25: 0000000000000000 x24: 0000000000000000
[    0.260167] x23: ffffff8082547de0 x22: ffffffe0563c4c48 x21: 0000000000000000
[    0.260181] x20: 00000000000000e0 x19: ffffff8081de3a00 x18: ffffffe056aeceb4
[    0.260194] x17: 0000000000000000 x16: 00000000000000c0 x15: ffffffe056ab6fd4
[    0.260208] x14: 0000000000000003 x13: 0000000000000004 x12: 0000000000000001
[    0.260221] x11: c0000000ffffdfff x10: ffffffe057167e20 x9 : 852f8c4ebbcfe700
[    0.260234] x8 : 852f8c4ebbcfe700 x7 : 0000000000000000 x6 : 322e30202020205b
[    0.260247] x5 : ffffffe0573fb193 x4 : 0000000000000000 x3 : ffffffc0800a3888
[    0.260261] x2 : ffffffc0800a3890 x1 : 00000000ffffdfff x0 : 0000000000000027
[    0.260275] Call trace:
[    0.260281]  clk_core_disable+0x90/0x1b0
[    0.260293]  clk_disable+0x38/0x50
[    0.260302]  geni_se_clks_off+0x24/0x5c
[    0.260314]  geni_se_resources_off+0x34/0x48
[    0.260325]  spi_geni_runtime_suspend+0x30/0x4c
[    0.260337]  pm_generic_runtime_suspend+0x30/0x44
[    0.260346]  genpd_runtime_suspend+0xfc/0x220
[    0.260356]  __rpm_callback+0x84/0x138
[    0.260365]  rpm_callback+0x30/0x88
[    0.260375]  rpm_suspend+0x168/0x520
[    0.260384]  pm_runtime_work+0x84/0xa8
[    0.260393]  process_scheduled_works+0x1ac/0x3f4
[    0.260405]  worker_thread+0x208/0x31c
[    0.260415]  kthread+0x100/0x1b8
[    0.260426]  ret_from_fork+0x10/0x20

ah yeah, pm_runtime_active() also returns true for devices that don't have runtime PM enabled for them, so weird. Here's a patch that uses pm_runtime_status_suspended() and also moves the DP device back to being suspended.

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 4c72124ffb5d..c1e01208d87a 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1306,6 +1306,7 @@ static int dp_display_probe(struct platform_device *pdev)
 
        platform_set_drvdata(pdev, &dp->dp_display);
 
+       pm_runtime_set_suspended(&pdev->dev);
        rc = devm_pm_runtime_enable(&pdev->dev);
        if (rc)
                goto err;
diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index 35423d10aafa..106f51de917d 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -484,6 +484,12 @@ static int mdss_probe(struct platform_device *pdev)
 
        platform_set_drvdata(pdev, mdss);
 
+       ret = pm_runtime_resume_and_get(dev);
+       if (ret) {
+               msm_mdss_destroy(mdss);
+               return ret;
+       }
+
        /*
         * MDP5/DPU based devices don't have a flat hierarchy. There is a top
         * level parent: MDSS, and children: MDP5/DPU, DSI, HDMI, eDP etc.
@@ -494,10 +500,11 @@ static int mdss_probe(struct platform_device *pdev)
        if (ret) {
                DRM_DEV_ERROR(dev, "failed to populate children devices\n");
                msm_mdss_destroy(mdss);
-               return ret;
        }
 
-       return 0;
+       pm_runtime_put(dev);
+
+       return ret;
 }
 
 static void mdss_remove(struct platform_device *pdev)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index b7708a06dc78..e22e7d4e7392 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -20,6 +20,7 @@
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/sysfb.h>
 
 #include "of_private.h"
@@ -181,6 +182,14 @@ static struct platform_device *of_platform_device_create_pdata(
        dev->dev.platform_data = platform_data;
        of_msi_configure(&dev->dev, dev->dev.of_node);
 
+       /*
+        * Initialize devices populated by runtime PM active devices as runtime
+        * PM active as well so that the parent can't runtime suspend until all
+        * active children are runtime suspended.
+        */
+       if (parent && !pm_runtime_status_suspended(parent))
+               pm_runtime_set_active(&dev->dev);
+
        if (of_device_add(dev) != 0) {
                platform_device_put(dev);
                goto err_clear_flag;

This patch seems to work nicely. I don't need to forcibly disable the rot clk if I do this. Maybe I'll make a new of_platform_populate() that pushes through the runtime PM active state so that other drivers have to opt in.

I get a bunch of these splats though. Marking the device as runtime PM active before probing it causes pm_runtime_get() and friends to skip the runtime resume callback that the driver uses to turn on clks. But it also prevents the mdss_gdsc from being turned off until the child devices probe and enable runtime PM.

[    0.525175] disp_cc_mdss_vsync_clk already disabled
[    0.525202] WARNING: CPU: 1 PID: 13 at drivers/clk/clk.c:1178 clk_core_disable+0x90/0x1b0
[    0.525220] Modules linked in:
[    0.525230] CPU: 1 PID: 13 Comm: kworker/u16:1 Not tainted 6.8.0-00006-g55380d555520-dirty #1 f2a10dccee3a324871e43bb62817fac4a1855722
[    0.525242] Hardware name: Google Lazor (rev3 - 8) with KB Backlight (DT)
[    0.525250] Workqueue: events_unbound deferred_probe_work_func
[    0.525264] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    0.525274] pc : clk_core_disable+0x90/0x1b0
[    0.525283] lr : clk_core_disable+0x90/0x1b0
[    0.525292] sp : ffffffc0800bb630
[    0.525298] pmr_save: 00000060
[    0.525303] x29: ffffffc0800bb630 x28: 00000000fffffff5 x27: ffffffc0800bb7b8
[    0.525317] x26: 0000000000000000 x25: 0000000000000000 x24: 0000000000000000
[    0.525330] x23: ffffff80823a0a80 x22: ffffffd814fc4c48 x21: ffffff8082ba9080
[    0.525342] x20: 00000000000000e0 x19: ffffff8080d99600 x18: 0000000000000000
[    0.525354] x17: 0000000000000000 x16: 00000000000000c0 x15: ffffffd8156b7094
[    0.525366] x14: 0000000000000003 x13: 0000000000000004 x12: 0000000000000001
[    0.525378] x11: c0000000ffffdfff x10: ffffffd815d67e20 x9 : a1387faf952ba900
[    0.525390] x8 : a1387faf952ba900 x7 : 0000000000000000 x6 : 352e30202020205b
[    0.525402] x5 : ffffffd815ffb193 x4 : 0000000000000000 x3 : ffffffc0800bb358
[    0.525414] x2 : ffffffc0800bb360 x1 : 00000000ffffdfff x0 : 0000000000000027
[    0.525428] Call trace:
[    0.525434]  clk_core_disable+0x90/0x1b0
[    0.525443]  clk_disable+0x38/0x50
[    0.525452]  clk_bulk_disable+0x34/0x4c
[    0.525460]  dpu_runtime_suspend+0x40/0x94
[    0.525472]  pm_generic_runtime_suspend+0x30/0x44
[    0.525482]  genpd_runtime_suspend+0xfc/0x220
[    0.525492]  __rpm_callback+0x84/0x138
[    0.525501]  rpm_callback+0x30/0x88
[    0.525510]  rpm_suspend+0x168/0x520
[    0.525518]  rpm_idle+0x5c/0x2b4
[    0.525527]  __pm_runtime_idle+0x48/0x100
[    0.525535]  dpu_kms_hw_init+0x478/0x49c
[    0.525545]  msm_drm_kms_init+0xb4/0x228
[    0.525554]  msm_drm_bind+0x278/0x3b8
[    0.525562]  try_to_bring_up_aggregate_device+0x16c/0x1b8
[    0.525572]  __component_add+0xb4/0x178
[    0.525581]  component_add+0x1c/0x28
[    0.525590]  dsi_dev_attach+0x24/0x30
[    0.525599]  dsi_host_attach+0xe8/0x11c
[    0.525609]  devm_mipi_dsi_attach+0x40/0xc8
[    0.525618]  ti_sn_bridge_probe+0x248/0x2c0
[    0.525628]  auxiliary_bus_probe+0x4c/0x94
[    0.525636]  really_probe+0xf8/0x270
[    0.525645]  __driver_probe_device+0xa8/0x130
[    0.525654]  driver_probe_device+0x44/0x104
[    0.525664]  __device_attach_driver+0xa4/0xcc
[    0.525673]  bus_for_each_drv+0x94/0xe8
[    0.525681]  __device_attach+0xf8/0x170
[    0.525690]  device_initial_probe+0x1c/0x28
[    0.525698]  bus_probe_device+0x48/0xa4
[    0.525706]  deferred_probe_work_func+0x9c/0xd8
[    0.525714]  process_scheduled_works+0x1ac/0x3f4
[    0.525725]  worker_thread+0x208/0x31c
[    0.525735]  kthread+0x100/0x1b8
[    0.525744]  ret_from_fork+0x10/0x20

I've realized that trying to make sure the mdss device doesn't runtime suspend until all child devices probe is the sync_state problem. We don't know if the child device drivers will ever probe, and we want the device drivers to probe so they can get their clks and enable them to match the runtime PM state of the device as "active". The problem here is much more concrete than the abstract sync_state problem though. We know what child devices form the display pipeline, and we don't probe the component device until all those display pipeline drivers probe. One idea is to runtime_pm_get() mdss in probe and then put that reference in the component driver probe. Another idea is to move the clk enabling and disabling logic into pm domains that are attached to the devices (not the drivers) in the mdss driver. Doing this would let us power on all those pm_domains and associated clks in mdss probe and power them down together. Or we teach gdsc code to park RCGs when the gdsc is powered down. Once we start reporting the real parent out of boot we'll have a problem where the display pll will be turned off and the gdsc driver will try to park the rcg and it won't be able to because the shared parent (display pll) was disabled by another clk.

Sent patch series at https://lore.kernel.org/r/20240327202740.3075378-1-swboyd@chromium.org

This problem comes about because of a combination of issues. The clk framework doesn't handle the case where two clks share the same parent and are enabled at boot. The first clk to enable and disable will turn off the parent.

side note (not to derail the discussion), but this is kinda where I got stuck many years back when I attempted to implement efifb->drm handover (on apq8016, so a simpler clk tree).. I wonder if we could try to get the prepare/enable counts to match hw state when kernel starts? Ie. for each enabled leaf node, walk up the current parents to the root incrementing the prepare+enable counts?

Sent an alternate approach at https://lore.kernel.org/r/20240502224703.103150-1-swboyd@chromium.org which is hopefully more acceptable.

It's merge window now so this will have to wait until that closes. I hope that Bjorn picks it up shortly after the window closes and lets it soak in linux-next for a while.

bugjuggler: wait 2w

So what's the plan here? Is it a good time for the patches to land upstream now?

Also: do we keep this bug open even though we've also got a (non-public) upstream tracking bug at b/331677760 ?

what's the plan here?

I've asked Bjorn and Dmitry on IRC to test and review the patch on the list. I expect Bjorn to merge it through the qcom clk driver branch so it gets plenty of time to bake in linux-next.

do we keep this bug open

That's my plan. I don't mind having two bugs for basically the same thing.

Sounds good. Let's schedule a nag in another 3 weeks then...

bugjuggler: wait 3 weeks

We're at -rc6 now. This feels like the kind of thing that should show up earlier in the merge window? I guess that means snooze for another 4 weeks?

bugjuggler: wait 4 weeks

...or Stephen will just apply it. ;-)

https://lore.kernel.org/r/CAE-0n50VDgsg-4QnynvLOzykr3KP5JsnHqeFPA=uRT3EfgL19g@mail.gmail.com

I guess now we just decide if we want to revert the FROMLIST and reland as FROMGIT or not...

Patch IDs match. I guess keeping as FROMLIST is fine. Closing.

# Downstream:
$ git show 01a0a6cc8cfd | git patch-id
2f52f110c3d40a4c25b8657724551ddf400be4b0 01a0a6cc8cfd9952e72677d48d56cf6bc4e3a561

# Upstream:
$ git show 9ac74b9ae59b8d62d886dad141fbd7f88a9c25e9 | git patch-id
2f52f110c3d40a4c25b8657724551ddf400be4b0 9ac74b9ae59b8d62d886dad141fbd7f88a9c25e9