On 12/27/2022 11:54 PM, Bjorn Andersson wrote: > On Mon, Dec 12, 2022 at 04:39:09PM +0100, Ulf Hansson wrote: >> On Fri, 9 Dec 2022 at 18:36, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote: >>> On Thu, 8 Dec 2022 at 22:06, Bjorn Andersson <andersson@xxxxxxxxxx> wrote: >>>> On Thu, Dec 08, 2022 at 02:40:55PM +0100, Ulf Hansson wrote: >>>>> On Wed, 7 Dec 2022 at 17:55, Bjorn Andersson <andersson@xxxxxxxxxx> wrote: >>>>>> On Wed, Dec 07, 2022 at 05:00:51PM +0100, Ulf Hansson wrote: >>>>>>> On Thu, 1 Dec 2022 at 23:57, Bjorn Andersson <andersson@xxxxxxxxxx> wrote: >>>>>>>> On Wed, Oct 05, 2022 at 02:36:58PM +0530, Akhil P Oommen wrote: >>>>>>>> @Ulf, Akhil has a power-domain for a piece of hardware which may be >>>>>>>> voted active by multiple different subsystems (co-processors/execution >>>>>>>> contexts) in the system. >>>>>>>> >>>>>>>> As such, during the powering down sequence we don't wait for the >>>>>>>> power-domain to turn off. But in the event of an error, the recovery >>>>>>>> mechanism relies on waiting for the hardware to settle in a powered off >>>>>>>> state. >>>>>>>> >>>>>>>> The proposal here is to use the reset framework to wait for this state >>>>>>>> to be reached, before continuing with the recovery mechanism in the >>>>>>>> client driver. >>>>>>> I tried to review the series (see my other replies), but I am not sure >>>>>>> I fully understand the consumer part. >>>>>>> >>>>>>> More exactly, when and who is going to pull the reset and at what point? >>>>>>> >>>>>>>> Given our other discussions on quirky behavior, do you have any >>>>>>>> input/suggestions on this? >>>>>>>> >>>>>>>>> Some clients like adreno gpu driver would like to ensure that its gdsc >>>>>>>>> is collapsed at hardware during a gpu reset sequence. This is because it >>>>>>>>> has a votable gdsc which could be ON due to a vote from another subsystem >>>>>>>>> like tz, hyp etc or due to an internal hardware signal. To allow >>>>>>>>> this, gpucc driver can expose an interface to the client driver using >>>>>>>>> reset framework. Using this the client driver can trigger a polling within >>>>>>>>> the gdsc driver. >>>>>>>> @Akhil, this description is fairly generic. As we've reached the state >>>>>>>> where the hardware has settled and we return to the client, what >>>>>>>> prevents it from being powered up again? >>>>>>>> >>>>>>>> Or is it simply a question of it hitting the powered-off state, not >>>>>>>> necessarily staying there? >>>>>>> Okay, so it's indeed the GPU driver that is going to assert/de-assert >>>>>>> the reset at some point. Right? >>>>>>> >>>>>>> That seems like a reasonable approach to me, even if it's a bit >>>>>>> unclear under what conditions that could happen. >>>>>>> >>>>>> Generally the disable-path of the power-domain does not check that the >>>>>> power-domain is actually turned off, because the status might indicate >>>>>> that the hardware is voting for the power-domain to be on. >>>>> Is there a good reason why the HW needs to vote too, when the GPU >>>>> driver is already in control? >>>>> >>>>> Or perhaps that depends on the running use case? >>>>> >>>>>> As part of the recovery of the GPU after some fatal fault, the GPU >>>>>> driver does something which will cause the hardware votes for the >>>>>> power-domain to be let go, and then the driver does pm_runtime_put(). >>>>> Okay. That "something", sounds like a device specific setting for the >>>>> corresponding gdsc, right? >>>>> >>>>> So somehow the GPU driver needs to manage that setting, right? >>>>> >>>>>> But in this case the GPU driver wants to ensure that the power-domain is >>>>>> actually powered down, before it does pm_runtime_get() again. To ensure >>>>>> that the hardware lost its state... >>>>> I see. >>>>> >>>>>> The proposal here is to use a reset to reach into the power-domain >>>>>> provider and wait for the hardware to be turned off, before the GPU >>>>>> driver attempts turning the power-domain on again. >>>>>> >>>>>> >>>>>> In other words, there is no reset. This is a hack to make a normally >>>>>> asynchronous pd.power_off() to be synchronous in this particular case. >>>>> Alright, assuming I understood your clarifications above correctly >>>>> (thanks!), I think I have got a much better picture now. >>>>> >>>>> Rather than abusing the reset interface, I think we should manage this >>>>> through the genpd's power on/off notifiers (GENPD_NOTIFY_OFF). The GPU >>>>> driver should register its corresponding device for them >>>>> (dev_pm_genpd_add_notifier()). >>>>> >>>>> The trick however, is to make the behaviour of the power-domain for >>>>> the gdsc (the genpd->power_off() callback) conditional on whether the >>>>> HW is configured to vote or not. If the HW can vote, it should not >>>>> poll for the state - and vice versa when the HW can't vote. >>>>> >>>> Per Akhil's description I misunderstood who the other voters are; but >>>> either way it's not the same "HW configured" mechanism as the one we're >>>> already discussing. >>> Okay, so this is another thing then. >>> >>>> >>>> But if we based on similar means could control if the power_off() ops >>>> should be blocking, waiting for the status indication to show that the >>>> hardware is indeed powered down, I think this would meet the needs. >>> Right. >>> >>>> And GENPD_NOTIFY_OFF seems to provide the notification that it was >>>> successful (i.e. happened within the timeout etc). >>>> >>>>> Would this work? >>>>> >>>> If we can control the behavior of the genpd, I think it would. >>> Okay, it seems like we need a new dev_pm_genpd_* interface that >>> consumers can call to instruct the genpd provider, that its >>> ->power_off() callback needs to temporarily switch to become >>> synchronous. >>> >>> I guess this could be useful for other similar cases too, where the >>> corresponding PM domain isn't actually being powered off, but rather >>> just voted for to become powered off, thus relying on the HW to do the >>> aggregation. >>> >>> In any case, I am still a bit skeptical of the reset approach, as is >>> being suggested in the $subject series. Even if it's rather nice and >>> clean (but somewhat abusing the interface), it looks like there will >>> be synchronization problems between the calls to the >>> pm_runtime_put_sync() and reset_control_reset() in the GPU driver. The >>> "reset" may actually already have happened when the call to >>> reset_control_reset() is done, so we may fail to detect the power >>> collapse, right!? >>> >>> Let me cook a patch for the new genpd interface that I have in mind, >>> then we can see how that plays out together with the other parts. I >>> will post it on Monday! >> Below is the genpd patch that I had in mind. >> >> As I stated above, the GPU driver would need to register for genpd's >> power on/off notificers (GENPD_NOTIFY_OFF). Then it should call the >> new dev_pm_genpd_synced_poweroff() and finally pm_runtime_put(). >> Moreover, when the GPU driver receives the GENPD_NOTIFY_OFF >> notification, it should probably just kick a completion variable, >> allowing the path that calls pm_runtime_put() to wait for the >> notification to arrive. >> >> On the genpd provider side, the ->power_off() callback should be >> updated to check the new genpd->synced_poweroff variable, to indicate >> whether it should poll for power collapse or not. >> >> I think this should work, but if you still prefer to use the "reset" >> approach, that's entirely up to you to decide. >> > I find this to be conceptually much cleaner. Thanks for the proposal! > > Regards, > Bjorn https://patchwork.freedesktop.org/series/111966/ Bjorn, this is the new series based on this proposal. -Akhil. >> Kind regards >> Uffe >> >> ----- >> >> From: Ulf Hansson <ulf.hansson@xxxxxxxxxx> >> Date: Mon, 12 Dec 2022 16:08:05 +0100 >> Subject: [PATCH] PM: domains: Allow a genpd consumer to require a synced power >> off >> >> TODO: Write commit message >> >> Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx> >> --- >> drivers/base/power/domain.c | 22 ++++++++++++++++++++++ >> include/linux/pm_domain.h | 1 + >> 2 files changed, 23 insertions(+) >> >> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c >> index b46aa490b4cd..3402b2ea7f61 100644 >> --- a/drivers/base/power/domain.c >> +++ b/drivers/base/power/domain.c >> @@ -494,6 +494,27 @@ void dev_pm_genpd_set_next_wakeup(struct device >> *dev, ktime_t next) >> } >> EXPORT_SYMBOL_GPL(dev_pm_genpd_set_next_wakeup); >> >> +/** >> + * dev_pm_genpd_synced_poweroff - Next power off should be synchronous >> + * >> + * @dev: Device to handle >> + * >> + * TODO: Add description >> + */ >> +void dev_pm_genpd_synced_poweroff(struct device *dev) >> +{ >> + struct generic_pm_domain *genpd; >> + >> + genpd = dev_to_genpd_safe(dev); >> + if (!genpd) >> + return; >> + >> + genpd_lock(genpd); >> + genpd->synced_poweroff = true; >> + genpd_unlock(genpd); >> +} >> +EXPORT_SYMBOL_GPL(dev_pm_genpd_synced_poweroff); >> + >> static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed) >> { >> unsigned int state_idx = genpd->state_idx; >> @@ -588,6 +609,7 @@ static int _genpd_power_off(struct >> generic_pm_domain *genpd, bool timed) >> out: >> raw_notifier_call_chain(&genpd->power_notifiers, GENPD_NOTIFY_OFF, >> NULL); >> + genpd->synced_poweroff = false; >> return 0; >> busy: >> raw_notifier_call_chain(&genpd->power_notifiers, GENPD_NOTIFY_ON, NULL); >> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h >> index ebc351698090..09c6c67a4896 100644 >> --- a/include/linux/pm_domain.h >> +++ b/include/linux/pm_domain.h >> @@ -134,6 +134,7 @@ struct generic_pm_domain { >> unsigned int prepared_count; /* Suspend counter of prepared >> devices */ >> unsigned int performance_state; /* Aggregated max performance state */ >> cpumask_var_t cpus; /* A cpumask of the attached CPUs */ >> + bool synced_poweroff; /* A consumer needs a synced poweroff */ >> int (*power_off)(struct generic_pm_domain *domain); >> int (*power_on)(struct generic_pm_domain *domain); >> struct raw_notifier_head power_notifiers; /* Power on/off notifiers */ >> -- >> 2.34.1
