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 BFCB7C7618D for ; Thu, 6 Apr 2023 05:08:19 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 14B6E882D0; Thu, 6 Apr 2023 05:08:16 +0000 (UTC) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTPS id A047110EAC6; Thu, 6 Apr 2023 05:08:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1680757694; x=1712293694; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version:content-transfer-encoding; bh=SgSI7dXfRoFMp75HcwIO6kFbBI1rVH8Ccd2yxX6HGXU=; b=U412vdsM3YsUa3yF2yl0IlHmY56JWT8piegSOjWOfeaf5u4j8aVRu2yd 2DXRsnI+mUcAekbg9zpDVd1XGqPU62/3oKIC5tmsgmLVHpcOnL+mKde1o enF3NTz1H+33GFKHbBXdPQtbrSrOpWm1LnCu6BRdHD0DPmUoD2oiLRLk7 AdFTOFP30cSUFlfoc8EwDxwRWUvpleLks/o3fdMxvHsA0mLn7ERLwUoS+ KVBNJIlyKo3Qc3pqJa2SMpdnJs4//QaFtHuyPr9cLTn77pS/pkz2Ee1wv sPF0YHbCRn77kvnOZ6rH43LujkJ7/YDIHvGXc04ftB2HG4VwZsb3OkOY6 w==; X-IronPort-AV: E=McAfee;i="6600,9927,10671"; a="344373548" X-IronPort-AV: E=Sophos;i="5.98,322,1673942400"; d="scan'208";a="344373548" Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Apr 2023 22:08:13 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10671"; a="687002830" X-IronPort-AV: E=Sophos;i="5.98,322,1673942400"; d="scan'208";a="687002830" Received: from adixit-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.209.39.173]) by orsmga002-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Apr 2023 22:08:13 -0700 Date: Wed, 05 Apr 2023 22:08:12 -0700 Message-ID: <87r0sxoawz.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> <871qkyp13t.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 Content-Transfer-Encoding: quoted-printable 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 13:12:29 -0700, Rodrigo Vivi wrote: > > On Wed, Apr 05, 2023 at 12:42:30PM -0700, Dixit, Ashutosh wrote: > > 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 inte= l_guc_slpc *slpc, u32 val) > > > > > val > slpc->max_freq_softlimit) > > > > > return -EINVAL; > > > > > > > > > > + /* Ignore efficient freq if lower min freq is requested */ > > > > > + ret =3D intel_guc_slpc_set_ignore_eff_freq(slpc, val < slpc->rp= 1_freq); > > > > > + if (ret) > > > > > + goto out; > > > > > + > > > > > > > > I don't agree with this. If we are now providing an interface expli= citly to > > > > ignore RPe, that should be /only/ way to ignore RPe. There should b= e no > > > > other "under the hood" ignoring of RPe. In other words, ignoring RP= e should > > > > be minimized unless explicitly requested. > > > > > > > > I don't clearly understand why this was done previously but it make= s 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 wi= th the > > > efficient freq enabled in guc if minimal is less than the efficient o= ne, > > > this request is likely ignored. > > > > > > Well, even worse is that we are actually caching the request in the s= oft values. > > > So we show a minimal, but the hardware without any workload is operat= ing at > > > efficient. > > > > > > So, the thought process was: 'if user requested a very low minimal, w= e give them > > > the minimal requested, even if that means to disable the efficient fr= eq.' > > > > 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? > > that is not how guc efficient freq selection works. (unless my memory is > tricking me right now.) > > So, if we select a min that is between RPe and RP0, guc will respect and > use the selected min. So we don't need to disable guc selection of the > efficient. > > This is not true when we select a very low min like RPn. If we select RPn > as min and guc efficient freq selection is enabled guc will simply ignore > our request. So the only way to give the user what is asked, is to also > disable guc's efficient freq selection. (I probably confused you in the > previous email because I used 'RP0' when I meant 'RPn'. I hope it gets > clear now). > > > > > * Also isn't RPe dynamic, so we can't say RPe =3D=3D rp1 when using in = KMD? > > Oh... yeap, this is an issue indeed. Specially with i915 where we have > the soft values cached instead of asking guc everytime. > > That's a good point. The variance is not big, but we will hit corner case= s. > One way is to keep checking and updating everytime a sysfs is touched. This I believe not possible in all cases. Say the freq's are set through sysfs first and the workload starts later. In this case RPe will probably start changing after the workload starts, not when freq's are set in sysfs. > Other way is do what you are suggesting and let's just accept and deal > with the reality that is: "we cannot guarantee a min freq selection if us= er > doesn't disable the efficient freq selection". > > > > > * 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. > > Oh! so it was my bad memory indeed and everything is the other way around? > But I just looked to Xe code, my most recent memory, and I just needed > to toggle the efficient freq off on the case that I mentioned, when min > selection is below the efficient one. With that all the API expectation > that I coded in IGT works neatly. =46rom what I saw the following bugs: https://gitlab.freedesktop.org/drm/intel/-/issues/6806 https://gitlab.freedesktop.org/drm/intel/-/issues/6786 and the following patches in response to these bugs (as well as the discussion on these patches): https://patchwork.freedesktop.org/series/111282/ https://patchwork.freedesktop.org/series/110574/ were all due to the fact that the max freq is not obeyed after RPe is enabled. These patches were never merged but I will now try to submit them again after this ignore efficient freq patch gets reviewed and merged. Thanks. -- Ashutosh > > > > 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? > > what cases it stays broken? This is why we need the IGT tests for all the > API behavior in place. > > > > 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 alrea= dy, > > > 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. > > Yeap, the IGT needs to come with whatever we concluded here and we need to > stick with that afterwards, so let's think with care. > > Vinay, Ashutosh's strongest argument is the variable RPe. Do you have tho= ughts > on that? > > > > > 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 thin= king > > > 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 cas= e. > > > Or at least not as bad as the user asking to get RP0 as minimal and o= nly > > > getting RPe back. > > in case I let you confused here, what I meant was RPn, not RP0. > > > > > > > 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 anot= her > > > side/angle. > > > > > > > > > > > Thanks. > > > > -- > > > > Ashutosh > > > > > > > > > > > > > /* Need a lock now since waitboost can be modifying min as well */ > > > > > mutex_lock(&slpc->lock); > > > > > wakeref =3D intel_runtime_pm_get(&i915->runtime_pm); > > > > > > > > > > - /* Ignore efficient freq if lower min freq is requested */ > > > > > - ret =3D 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 =3D slpc_set_param(slpc, > > > > > SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ, > > > > > val);