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 gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 8CEADC761AF for ; Wed, 5 Apr 2023 19:42:40 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id A631F10E43A; Wed, 5 Apr 2023 19:42:35 +0000 (UTC) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTPS id 34C2D10E43A; Wed, 5 Apr 2023 19:42:33 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1680723753; x=1712259753; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=zOWt2TVon+Ce0FhL+7YyUEoG0mBxX0tjMyMVZfTU0Jk=; b=C9AXz6+pYuV0nNdkDITke1HYbARG8TUPaGvx77JItIweWo9Rz1FPMt8O nq8fQCJPqMCVHHVn+GGLf/ACuDufIy58zzqnOYHQxQh+xygvT3N8+fHrc /pbQGE3p22UQg/2H+TGuBQb9InbS2WOP5fYCfKQ3eaPpg6keZlqILJOlc mxdUwvlZNGVnw5vfoQhhl+/1iX3ck0uZz7iFEvlWAbVZnRCRBKCMgQmmo xTooQ4gc1kmri3EQn565SDcpHH9xl3IyistrviFtB5CHKhKe9OMn0bs8X iqCpT5eoIpJxR/BgVRbMwbEQ+8xpEzCmD7CcMZArZ4tMW8geLJ7ZSiPtY A==; X-IronPort-AV: E=McAfee;i="6600,9927,10671"; a="345139781" X-IronPort-AV: E=Sophos;i="5.98,321,1673942400"; d="scan'208";a="345139781" Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Apr 2023 12:42:32 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10671"; a="756106929" X-IronPort-AV: E=Sophos;i="5.98,321,1673942400"; d="scan'208";a="756106929" Received: from adixit-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.209.39.173]) by fmsmga004-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Apr 2023 12:42:30 -0700 Date: Wed, 05 Apr 2023 12:42:30 -0700 Message-ID: <871qkyp13t.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Rodrigo Vivi In-Reply-To: References: <20230401020049.3843873-1-vinay.belgaumkar@intel.com> <877cuwguu6.wl-ashutosh.dixit@intel.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Subject: Re: [Intel-gfx] [PATCH] i915/guc/slpc: Provide sysfs for efficient freq X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Wed, 05 Apr 2023 06:57:42 -0700, Rodrigo Vivi wrote: > Hi Rodrigo, > On Fri, Mar 31, 2023 at 08:11:29PM -0700, Dixit, Ashutosh wrote: > > On Fri, 31 Mar 2023 19:00:49 -0700, Vinay Belgaumkar wrote: > > > > > > > Hi Vinay, > > > > > @@ -478,20 +507,15 @@ int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val) > > > val > slpc->max_freq_softlimit) > > > return -EINVAL; > > > > > > + /* Ignore efficient freq if lower min freq is requested */ > > > + ret = intel_guc_slpc_set_ignore_eff_freq(slpc, val < slpc->rp1_freq); > > > + if (ret) > > > + goto out; > > > + > > > > I don't agree with this. If we are now providing an interface explicitly to > > ignore RPe, that should be /only/ way to ignore RPe. There should be no > > other "under the hood" ignoring of RPe. In other words, ignoring RPe should > > be minimized unless explicitly requested. > > > > I don't clearly understand why this was done previously but it makes even > > less sense to me now after this patch. > > well, I had suggested this previously. And just because without this we would > be breaking API expectations. > > When user selects a minimal frequency it expect that to stick. But with the > efficient freq enabled in guc if minimal is less than the efficient one, > this request is likely ignored. > > Well, even worse is that we are actually caching the request in the soft values. > So we show a minimal, but the hardware without any workload is operating at > efficient. > > So, the thought process was: 'if user requested a very low minimal, we give them > the minimal requested, even if that means to disable the efficient freq.' Hmm, I understand this even less now :) * Why is RPe ignored when min < RPe? Since the freq can be between min and max? Shouldn't the condition be min > RPe, that is turn RPe off if min higher that RPe is requested? * Also isn't RPe dynamic, so we can't say RPe == rp1 when using in KMD? * Finally, we know that enabling RPe broke the kernel freq API because RPe could go over max_freq. So it is actually the max freq which is not obeyed after RPe is enabled. So we ignore RPe in some select cases (which also I don't understand as mentioned above but maybe I am missing something) to claim that we are obeying the freq API, but let the freq API stay broken in other cases? > So, that was introduced to avoid API breakage. Removing it now would mean > breaking API. (Not sure if the IGT tests for the API got merged already, > but think that as the API contract). I think we should take this patch as an opportunity to fix this and give the user a clean interface to ignore RPe and remove this other implicit way to ignore RPe. All IGT changes are unmerged at present. Thanks. -- Ashutosh > > But I do agree with you that having something selected from multiple places > also has the potential to cause some miss-expectations. So I was thinking > about multiple even orders where the user select the RP0 as minimal, then > enable the efficient or vice versa, but I couldn't think of a bad case. > Or at least not as bad as the user asking to get RP0 as minimal and only > getting RPe back. > > With this in mind, and having checked the code: > > Reviewed-by: Rodrigo Vivi > > But I won't push this immediately because I'm still open to hear another > side/angle. > > > > > Thanks. > > -- > > Ashutosh > > > > > > > /* Need a lock now since waitboost can be modifying min as well */ > > > mutex_lock(&slpc->lock); > > > wakeref = intel_runtime_pm_get(&i915->runtime_pm); > > > > > > - /* Ignore efficient freq if lower min freq is requested */ > > > - ret = slpc_set_param(slpc, > > > - SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY, > > > - val < slpc->rp1_freq); > > > - if (ret) { > > > - guc_probe_error(slpc_to_guc(slpc), "Failed to toggle efficient freq: %pe\n", > > > - ERR_PTR(ret)); > > > - goto out; > > > - } > > > - > > > ret = slpc_set_param(slpc, > > > SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ, > > > val);