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 35187ECAAD3 for ; Wed, 14 Sep 2022 16:11:38 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id C207410E00D; Wed, 14 Sep 2022 16:11:36 +0000 (UTC) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTPS id C9F1D10E96C for ; Wed, 14 Sep 2022 16:11:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1663171891; x=1694707891; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version:content-transfer-encoding; bh=lIYsLFvoycSkWRjRXgEXBbp0HKfnXavb7EBqlRlCtNA=; b=gFZM5zW0HJBGOyxit3ddhu0FtRVae9Z5Nd739Dh5NxJ9t3MnioyzZTfh fbEiL4X6/jnt6JQ8zQ72rZbg/C7YwKif7JT44wiB3PpHTn1q64bC7Lobh zQ8doeBfZL4jZLPQAG8drfacmpXvz6UR7Nw7nFw4/i8pU89iojG7eYDfS sY8efVyEsQhuP4wI8Fxyv/RtGsmxwBoRk5utyPIitZZIzdQSlGb9WkwKX CAVVaqDEEIWvOdR1e1mbCFSYKbVpzqd17j1e8OL7LfvKXxEI2iNKCjUKC VaHWSzSa5wdhZ/1QpO+QINxmf1NZvavOBi75lscM1V315NmdrnGxHQa31 A==; X-IronPort-AV: E=McAfee;i="6500,9779,10470"; a="324725622" X-IronPort-AV: E=Sophos;i="5.93,315,1654585200"; d="scan'208";a="324725622" Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Sep 2022 09:11:30 -0700 X-IronPort-AV: E=Sophos;i="5.93,315,1654585200"; d="scan'208";a="612558460" Received: from adixit-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.209.38.224]) by orsmga007-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Sep 2022 09:11:29 -0700 Date: Wed, 14 Sep 2022 09:11:29 -0700 Message-ID: <87leqmdjgu.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: "Nilawar, Badal" In-Reply-To: <5443c3cb-1ece-226f-bef1-55f031359f83@intel.com> References: <20220909025646.3397620-1-badal.nilawar@intel.com> <20220909025646.3397620-5-badal.nilawar@intel.com> <87k06c577l.wl-ashutosh.dixit@intel.com> <1ce34139-0b3f-6709-597f-e55437bccc0d@intel.com> <87czc05e53.wl-ashutosh.dixit@intel.com> <9a6763df-2ab0-4136-2727-e6d24f039ea3@linux.intel.com> <5443c3cb-1ece-226f-bef1-55f031359f83@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.1 (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=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Intel-gfx] [PATCH 4/6] drm/i915: Use GEN12 RPSTAT register 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 Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Wed, 14 Sep 2022 02:56:26 -0700, Nilawar, Badal wrote: > > On 13-09-2022 13:17, Tvrtko Ursulin wrote: > > > > On 13/09/2022 01:09, Dixit, Ashutosh wrote: > >> On Mon, 12 Sep 2022 04:29:38 -0700, Nilawar, Badal wrote: > >>> > >>>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c > >>>>> b/drivers/gpu/drm/i915/i915_pmu.c > >>>>> index 958b37123bf1..a24704ec2c18 100644 > >>>>> --- a/drivers/gpu/drm/i915/i915_pmu.c > >>>>> +++ b/drivers/gpu/drm/i915/i915_pmu.c > >>>>> @@ -371,7 +371,6 @@ static void > >>>>> =A0=A0 frequency_sample(struct intel_gt *gt, unsigned int period_ns) > >>>>> =A0=A0 { > >>>>> =A0=A0=A0=A0struct drm_i915_private *i915 =3D gt->i915; > >>>>> -=A0=A0=A0 struct intel_uncore *uncore =3D gt->uncore; > >>>>> =A0=A0=A0=A0struct i915_pmu *pmu =3D &i915->pmu; > >>>>> =A0=A0=A0=A0struct intel_rps *rps =3D >->rps; > >>>>> > >>>>> @@ -394,7 +393,7 @@ frequency_sample(struct intel_gt *gt, unsigned > >>>>> int period_ns) > >>>>> =A0=A0=A0=A0=A0=A0=A0=A0 * case we assume the system is running at = the intended > >>>>> =A0=A0=A0=A0=A0=A0=A0=A0 * frequency. Fortunately, the read should = rarely fail! > >>>>> =A0=A0=A0=A0=A0=A0=A0=A0 */ > >>>>> -=A0=A0=A0=A0=A0=A0=A0 val =3D intel_uncore_read_fw(uncore, GEN6_RP= STAT1); > >>>>> +=A0=A0=A0=A0=A0=A0=A0 val =3D intel_rps_read_rpstat(rps); > >>>> > >>>> Hmm, we got rid of _fw which the comment above refers to. Maybe we > >>>> need a > >>>> fw flag to intel_rps_read_rpstat? > >>> > >>> Above function before reading rpstat it checks if gt is awake. > >> > >> Ok, so you are referring to intel_gt_pm_get_if_awake check in > >> frequency_sample. > >> > >>> So when gt is awake shouldn't matter if we read GEN6_RPSTAT1 with > >>> forcewake.In that case we can remove above comment.=A0 Let me know yo= ur > >>> thoughts on this. > >> > >> I am not entirely sure about this. For example in c1c82d267ae8 > >> intel_uncore_read_fw was introduced with the same > >> intel_gt_pm_get_if_awake > >> check. So this would mean even if gt is awake not taking forcewake mak= es > >> a > >> difference. The same code pattern was retained in b66ecd0438bf. Maybe > >> it's > >> because there are no locks? > > > > Its about power. As c1c82d267ae8 ("drm/i915/pmu: Cheat when reading the > > actual frequency to avoid fw") explains the _fw variant is to avoid > > preventing RC6, and so increased GPU power draw, just because someone h= as > > PMU open. (Because of the 200Hz sampling timer that is needed for PMU > > frequency reporting.) > > > >> Under the circumstances I think we could do one of two things: > >> 1. If we want to drop _fw, we should do it as a separate patch with its > >> own > >> =A0=A0=A0 justification so it can be reviewed separately. > >> 2. Otherwise as I mentioned we should retain the _fw and add a fw flag= to > >> =A0=A0=A0 intel_rps_read_rpstat. > > > > Agreed. Or instead of the flag, the usual pattern of having > > intel_rps_read_rpstat_fw and make intel_rps_read_rpsstat get the > > forcewake. > > > > Also, may I ask, this patch is in the MTL enablement series but the > > commit message and patch content seem like it is fixing a wider Gen12 > > issue? What is the extent of incorrect behaviour without it? Should it = be > > tagged for stable for first Tigerlake supporting kernel? > > GEN6_RPSTAT1(0xa01c) and GEN12_RPSTAT1(0x1381b4) both are supported by > gen12 and above. The difference between two is GEN6_RPSTAT1 falls under > RENDER forcewake domain and GEN12_RPSTAT1 does not require forcewake to > access. GEN12_RPSTAT1 is punit register and when GT is in RC6 it will give > frequency as 0. Correct, so no changes needed for stable kernels. But going forward Badal is proposing (which I sort of agree with but may need some discussion) that we change i915 behavior to return 0 freq (instead of cur_freq or RPn) when GT is idle or in RC6 (so we don't take forcewake to read freq when GT is in RC6). > Reason for clubbing this patch with MTL series is due to common function > intel_rps_read_rpstat. I think I should send this patch in separate serie= s. Agree! Thanks. -- Ashutosh