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 40872ECAAA1 for ; Mon, 31 Oct 2022 17:31:26 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 93EBB10E130; Mon, 31 Oct 2022 17:31:25 +0000 (UTC) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTPS id D03AB10E130 for ; Mon, 31 Oct 2022 17:31:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1667237483; x=1698773483; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=fXWlvws2oFM5RNTeKEqYTRmUAQ/ZzBXI9TcgyY8qq7M=; b=lR6fjZDl+pzLwXHcsCl2Frrmg+v3mANgicFBh7fNwFmUGa1H9o0fldqU nDjPHSQszcRzlCWj7DinYwKWdMuiHZCIvS7xxemeGM06uBZr6lbeWkp+f YzSr2jFEwthJPRlLIoEyofWA4p+5+VfSHXRhe0Eodwnf/OdEaS6L9TDgL VJ6QpNSX3lFitXG5ITuc+PsLMj8+4UuqjK6bP8pD28ksArhvemfNzbVZK WTGXGiljAIx4yj35iZD0pYY8UeEnA0lSGtXA5S12ivgas33PPUZnuU6nO sSm7i7S3JoJ3VhKEY5AHC+nQObd+dXTXleGhJGJvz+0lBFcRg3u9vwVv8 w==; X-IronPort-AV: E=McAfee;i="6500,9779,10517"; a="310658131" X-IronPort-AV: E=Sophos;i="5.95,228,1661842800"; d="scan'208";a="310658131" Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 31 Oct 2022 10:31:23 -0700 X-IronPort-AV: E=McAfee;i="6500,9779,10517"; a="633605650" X-IronPort-AV: E=Sophos;i="5.95,228,1661842800"; d="scan'208";a="633605650" Received: from adixit-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.209.2.250]) by orsmga002-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 31 Oct 2022 10:31:22 -0700 Date: Mon, 31 Oct 2022 10:31:22 -0700 Message-ID: <871qqnew7p.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Gwan-gyeong Mun In-Reply-To: <49a3db03-a62c-374d-76cd-8557c761f18b@intel.com> References: <20221024210953.1572998-1-gwan-gyeong.mun@intel.com> <20221029044230.32128-1-gwan-gyeong.mun@intel.com> <878rkwefji.wl-ashutosh.dixit@intel.com> <49a3db03-a62c-374d-76cd-8557c761f18b@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 v2] drm/i915/hwmon: Fix a build error used with clang compiler 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 Sun, 30 Oct 2022 23:37:59 -0700, Gwan-gyeong Mun wrote: > Hi GG, > On 10/31/22 7:19 AM, Dixit, Ashutosh wrote: > > On Fri, 28 Oct 2022 21:42:30 -0700, Gwan-gyeong Mun wrote: > >> > >> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > >> index 9e9781493025..c588a17f97e9 100644 > >> --- a/drivers/gpu/drm/i915/i915_hwmon.c > >> +++ b/drivers/gpu/drm/i915/i915_hwmon.c > >> @@ -101,21 +101,16 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat, i915_reg_t rgadr, > >> > >> static void > >> hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr, > >> - u32 field_msk, int nshift, > >> - unsigned int scale_factor, long lval) > >> + int nshift, unsigned int scale_factor, long lval) > >> { > >> u32 nval; > >> - u32 bits_to_clear; > >> - u32 bits_to_set; > >> > >> /* Computation in 64-bits to avoid overflow. Round to nearest. */ > >> nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor); > >> > >> - bits_to_clear = field_msk; > >> - bits_to_set = FIELD_PREP(field_msk, nval); > >> - > >> hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr, > >> - bits_to_clear, bits_to_set); > >> + PKG_PWR_LIM_1, > >> + REG_FIELD_PREP(PKG_PWR_LIM_1, nval)); > > > > I registered my objection to this patch already here: > > > > https://lore.kernel.org/intel-gfx/87ilk7pwrw.wl-ashutosh.dixit@intel.com/ > > > > the crux of which is "hwm_field_scale_and_write() pairs with > > hwm_field_read_and_scale() (they are basically a set/get pair) so it is > > desirable they have identical arguments. This patch breaks that symmetry". > > > > We can merge this patch now but the moment a second caller of > > hwm_field_scale_and_write arrives this patch will need to be reverted. > > > > I have also posted my preferred way (as I previously indiecated) of fixing > > this issue here (if this needs to be fixed in i915): > > > > https://patchwork.freedesktop.org/series/110301/ > > > The given link (https://patchwork.freedesktop.org/series/110301/) is an > inline function that reduces the checks of REG_FIELD_PREP() and pursues the > same functionality. > It's not a good idea to add and use duplicate new inline functions with > reduced functionality under different names. See if you like v2 better :-) > +/* FIELD_PREP and REG_FIELD_PREP require a compile time constant mask */ > +static u32 hwm_field_prep(u32 mask, u32 val) > +{ > + return (val << __bf_shf(mask)) & mask; > +} > + > static void > hwm_locked_with_pm_intel_uncore_rmw(struct hwm_drvdata *ddat, > i915_reg_t reg, u32 clear, u32 set) > @@ -112,7 +118,7 @@ hwm_field_scale_and_write(struct hwm_drvdata *ddat, > i915_reg_t rgadr, > nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor); > > bits_to_clear = field_msk; > - bits_to_set = FIELD_PREP(field_msk, nval); > + bits_to_set = hwm_field_prep(field_msk, nval); > > hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr, > bits_to_clear, bits_to_set); > > > The patch > (https://patchwork.freedesktop.org/patch/509248/?series=110094&rev=5) that > fixed the build error in a simplified form was added as there is only one > place that calls the hwm_field_scale_and_write() function at this time. > > If more places that use the hwm_field_scale_and_write() function are added > in the future, how about changing the interface that calls this function as > Jani previously suggested? Sorry, which interface change which Jani suggested are you referring to? I don't recall seeing anything but maybe I am mistaken. Thanks. -- Ashutosh > > IMO it would be a mistake to use REG_FIELD_PREP or FIELD_PREP here since > > here the mask comes in as a function argument whereas REG_FIELD_PREP and > > FIELD_PREP require that mask to be a compile time constant.