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 X-Spam-Level: X-Spam-Status: No, score=-6.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id AEEF5C433DF for ; Thu, 27 Aug 2020 11:29:12 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 7902B22CAE for ; Thu, 27 Aug 2020 11:29:12 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="x+G/aTo/" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7902B22CAE Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=KfftbL6ifA86c4eCf+p9dTzxm/1tvD1vncoUvh7tV4Q=; b=x+G/aTo/chSzqKqFHgjUogP2R g7WSM474I7unNS1yI/hPwdCGyVwrlTcKk+8Z1sfBdUYlryxncrHD+dPgsr2g8N6PDQoeI0PoGU12j eQoXFcj5MOtlpzLRDV766rhv7MXO8FsARtFBH4CExEXEwMiRy08be9flcBnf8ylUrnBXPOyp6dmG4 BEfojsE2xB16/d9ymuVTwhzcJuHVTxcWVBxPXI57kzaA/rCKS4shm+biyTdGGPLxBt9Abm3f6Nbng inyXTBAZVFcDn0bLWgTXaiWgE0j/kWeN4Sde/fIgYToUgNw4HVhVLElqgJZ/Bw0inatJfTd3cTt1L elUtHUe/Q==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kBG4S-0006In-Ff; Thu, 27 Aug 2020 11:27:56 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kBG4Q-0006I9-2E for linux-arm-kernel@lists.infradead.org; Thu, 27 Aug 2020 11:27:55 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6082212FC; Thu, 27 Aug 2020 04:27:42 -0700 (PDT) Received: from localhost (unknown [10.1.199.49]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 015683F68F; Thu, 27 Aug 2020 04:27:41 -0700 (PDT) Date: Thu, 27 Aug 2020 12:27:40 +0100 From: Ionela Voinescu To: Viresh Kumar Subject: Re: [RFC 0/3] cpufreq: cppc: Add support for frequency invariance Message-ID: <20200827112740.GA9923@arm.com> References: <20200709124349.GA15342@arm.com> <20200710030032.3yq3lqqybhy5m744@vireshk-i7> <20200825095629.GA15469@arm.com> <20200827075149.ixunmyi3m6ygtehu@vireshk-i7> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200827075149.ixunmyi3m6ygtehu@vireshk-i7> User-Agent: Mutt/1.9.4 (2018-02-28) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200827_072754_302826_E1997749 X-CRM114-Status: GOOD ( 31.30 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Juri Lelli , Vincent Guittot , "Rafael J. Wysocki" , Peter Zijlstra , Catalin Marinas , "open list:THERMAL" , Sudeep Holla , "Rafael J. Wysocki" , linux-kernel , Steven Rostedt , Ben Segall , Ingo Molnar , Mel Gorman , Greg Kroah-Hartman , Peter Puhov , Will Deacon , Dietmar Eggemann , LAK Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Viresh, On Thursday 27 Aug 2020 at 13:21:49 (+0530), Viresh Kumar wrote: > On 25-08-20, 10:56, Ionela Voinescu wrote: > > I've been putting some more thought/code into this one and I believe > > something as follows might look nicer as well as cover a few corner cases > > (ignore implementation details for now, they can be changed): > > I saw the other patchset you sent where AMU can be used as the backend > for CPPC driver, which means that if AMU IP is present on the platform > it will be used by the CPPC to get the perf counts, right ? > > > - Have a per cpu value that marks the use of either AMUs, CPPC, or > > cpufreq for freq invariance (can be done with per-cpu variable or with > > cpumasks) > > > > - arch_topology.c: initialization code as follows: > > > > for_each_present_cpu(cpu) { > > if (freq_inv_amus_valid(cpu) && > > !freq_inv_set_max_ratio(cpufreq_get_hw_max_freq(cpu) * 1000, > > arch_timer_get_rate(), cpu)) { > > per_cpu(inv_source, cpu) = INV_AMU_COUNTERS; > > continue; > > } > > if (freq_inv_cppc_counters_valid(cpu) && > > !freq_inv_set_max_ratio(cppc_max_perf, cppc_ref_perf, cpu)) { > > per_cpu(inv_source, cpu) = INV_CPPC_COUNTERS; > > continue; > > } > > if (!cpufreq_supports_freq_invariance() || > > freq_inv_set_max_ratio(cpufreq_get_hw_max_freq(cpu), > > 1, cpu)) { > > pr_info("FIE disabled: no valid source for CPU%d.", cpu); > > return 0; > > } > > } > > Based on that (your other patchset), I think this can get further > simplified to whomsoever can register first for freq invariance. > I don't see it as anyone registering for freq invariance, rather the freq invariance framework chooses its source of information (AMU, CPPC, cpufreq). > i.e. if CPPC registers for it first then there is no need to check > AMUs further (as CPPC will be using AMUs anyway), else we will ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Not necessarily. Even if AMUs are present, they are only used for CPPC's delivered and reference performance counters if the ACPI _CPC entry specifies FFH as method: ResourceTemplate(){Register(FFixedHW, 0x40, 0, 1, 0x4)}, ResourceTemplate(){Register(FFixedHW, 0x40, 0, 0, 0x4)}, > fallback to AMU, else cpufreq. > > Is that understanding correct ? While I understand your point (accessing AMUs through CPPC's read functions to implement invariance) I don't think it's worth tying the two together. I see the two functionalities as independent: - frequency invariance with whichever source of information is valid (AMUs, cpufreq, etc) is separate from - CPPC's delivered and reference performance counters, which currently are used in cpufreq's .get() function. Therefore, taking each of the scenarios one by one: - All CPUs support AMUs: the freq invariance initialisation code will find AMUs valid and it will use them to set the scale factor; completely independently, if the FFH method is specified for CPPC's delivered and reference performance counters, it will also use AMUs, even if, let's say, invariance is disabled. - None of the CPUs support AMUs, but the _CPC entry specifies some platform specific counters for delivered and reference performance. With the current mainline code neither cpufreq or counter based invariance is supported, but the CPPC counters can be used in the cppc_cpufreq driver for the .get() function. But with the above new functionality we can detect that AMUs are not supported and expose the CPPC counters to replace them in implementing invariance. - Mixed scenarios are also supported if we play our cards right and implement the above per-cpu. I'm thinking that having some well defined invariance sources might work well: it will simplify the init function (go through all registered sources and choose (per-cpu) the one that's valid) and allow for otherwise generic invariance support. Something like: enum freq_inv_source {INV_CPUFREQ, INV_AMU_COUNTERS, INV_CPPC_COUNTERS}; struct freq_inv_source { enum freq_inv_source source; bool (*valid)(int cpu); u64 (*read_corecnt)(int cpu); u64 (*read_constcnt)(int cpu); u64 (*max_rate)(int cpu); u64 (*ref_rate)(int cpu); } I am in the middle of unifying AMU counter and cpufreq invariance through something like this, so if you like the idea and you don't think I'm stepping too much on your toes with this, I can consider the usecase in my (what should be) generic support. So in the end this might end up being just a matter of adding a new invariance source (CPPC counters). My only worry is that while I know how a cpufreq source behaves and how AMU counters behave, I'm not entirely sure what to expect from CPPC counters: if they are always appropriate for updates on the tick (not blocking), if they both stop during idle, if there is save/restore functionality before/after idle, etc. What do you think? Regards, Ionela. > -- > viresh _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel