From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id EBD27C4708D for ; Mon, 12 Dec 2022 17:44:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232317AbiLLRoV (ORCPT ); Mon, 12 Dec 2022 12:44:21 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48486 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229638AbiLLRoU (ORCPT ); Mon, 12 Dec 2022 12:44:20 -0500 Received: from mx0a-0031df01.pphosted.com (mx0a-0031df01.pphosted.com [205.220.168.131]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 58F2013E2A; Mon, 12 Dec 2022 09:44:19 -0800 (PST) Received: from pps.filterd (m0279863.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 2BCBSa5n027425; Mon, 12 Dec 2022 17:44:00 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; h=message-id : date : mime-version : subject : to : cc : references : from : in-reply-to : content-type : content-transfer-encoding; s=qcppdkim1; bh=qvjyfZJG4pX6IOexrtElSNZ4ajT9HkGAW0C0w4PjHTg=; b=Jl70mLP4ogi8Pnub8j737+bMnJYPGduDgk/I6YGnEcitBerQZazwWNvlRZEfmVB+9MhJ aHo40j77XLJV3+7KvFwtquGDbb1czK0f5EwSKxt4zvaPmqnLOQSUJUk/mMN/vu9+isGJ Ag6OqF9RIJTF3+4NOOWsxyj6lEPbW8f3LvGXcUaJOJslYCET6VT58LiL4iDbE1+IxB4l PCj2ChbwVrVgSsoAHCBfjocLegcfNun247PT7BbNd903/st139W8a6no4mYQ1JWyJybI xjwLmNIHlH4gX7VkVDn5pjBCck3KNbDEoaK66JHXjfAmsvNErTRuvM/p4CmGQRknXDri 1Q== Received: from nalasppmta05.qualcomm.com (Global_NAT1.qualcomm.com [129.46.96.20]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3mcjb94qe2-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 12 Dec 2022 17:43:59 +0000 Received: from nalasex01a.na.qualcomm.com (nalasex01a.na.qualcomm.com [10.47.209.196]) by NALASPPMTA05.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 2BCHhwpu005746 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 12 Dec 2022 17:43:58 GMT Received: from [10.216.43.29] (10.80.80.8) by nalasex01a.na.qualcomm.com (10.47.209.196) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.36; Mon, 12 Dec 2022 09:43:51 -0800 Message-ID: <63347267-e610-7b7d-7bcd-43f10518fe1c@quicinc.com> Date: Mon, 12 Dec 2022 23:13:47 +0530 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.3.2 Subject: Re: [PATCH v7 0/6] clk/qcom: Support gdsc collapse polling using 'reset' interface Content-Language: en-US To: Ulf Hansson , Bjorn Andersson CC: freedreno , , , Rob Clark , Stephen Boyd , Dmitry Baryshkov , Philipp Zabel , Douglas Anderson , , Abhinav Kumar , Andy Gross , Daniel Vetter , David Airlie , "Konrad Dybcio" , Krzysztof Kozlowski , Michael Turquette , Rob Herring , Sean Paul , Stephen Boyd , , , References: <1664960824-20951-1-git-send-email-quic_akhilpo@quicinc.com> <20221201225705.46r2m35ketvzipox@builder.lan> <20221207165457.kwdwwiycbwjpogxl@builder.lan> <20221208210622.x656vbf7rum5hrl7@builder.lan> From: Akhil P Oommen In-Reply-To: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01b.na.qualcomm.com (10.46.141.250) To nalasex01a.na.qualcomm.com (10.47.209.196) X-QCInternal: smtphost X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-GUID: 4cTjd-qLTQu533sKSLZUwpocBUz5TYBX X-Proofpoint-ORIG-GUID: 4cTjd-qLTQu533sKSLZUwpocBUz5TYBX X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.923,Hydra:6.0.545,FMLib:17.11.122.1 definitions=2022-12-12_02,2022-12-12_02,2022-06-22_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 phishscore=0 adultscore=0 priorityscore=1501 lowpriorityscore=0 bulkscore=0 mlxscore=0 malwarescore=0 suspectscore=0 impostorscore=0 spamscore=0 mlxlogscore=999 clxscore=1015 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000 definitions=main-2212120160 Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org On 12/12/2022 9:09 PM, Ulf Hansson wrote: > On Fri, 9 Dec 2022 at 18:36, Ulf Hansson wrote: >> On Thu, 8 Dec 2022 at 22:06, Bjorn Andersson wrote: >>> On Thu, Dec 08, 2022 at 02:40:55PM +0100, Ulf Hansson wrote: >>>> On Wed, 7 Dec 2022 at 17:55, Bjorn Andersson wrote: >>>>> On Wed, Dec 07, 2022 at 05:00:51PM +0100, Ulf Hansson wrote: >>>>>> On Thu, 1 Dec 2022 at 23:57, Bjorn Andersson 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. > > Kind regards > Uffe > > ----- > > From: Ulf Hansson > 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 > --- > 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 */ Thanks a lot, Ulf. I will try to prototype rest of the changes on top this. -Akhil.