Received-SPF: pass (google.com: domain of linux-kernel+bounces-30630-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 18 Jan 2024 17:25:00 -0600
From: Lucas De Marchi <lucas.demarchi@intel.com>
To: Yury Norov <yury.norov@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>,
	<intel-gfx@lists.freedesktop.org>, Kevin Brodsky <kevin.brodsky@arm.com>,
	<linux-kernel@vger.kernel.org>, <dri-devel@lists.freedesktop.org>, Christian
 =?utf-8?B?S8O2bmln?= <christian.koenig@amd.com>, Masahiro Yamada
	<masahiroy@kernel.org>, Alex Deucher <alexander.deucher@amd.com>, "Thomas
 Gleixner" <tglx@linutronix.de>, Andy Shevchenko
	<andriy.shevchenko@linux.intel.com>, <intel-xe@lists.freedesktop.org>
Subject: Re: Re: Re: [Intel-xe] [PATCH 2/3] linux/bits.h: Add fixed-width
 GENMASK and BIT macros
Message-ID: <clamvpymzwiehjqd6jhuigymyg5ikxewxyeee2eae4tgzmaz7u@6rposizee3t6>
References: <20230509051403.2748545-1-lucas.demarchi@intel.com>
 <20230509051403.2748545-3-lucas.demarchi@intel.com>
 <ZJOwC5LIEySpduQJ@yury-ThinkPad>
 <4ezps56sdj7fmr27ivkaqjakv4ex46f5cvmy6oqr3z6gkhiorl@us4qd53jzq34>
 <Zamcu7tts8mqX0b4@yury-ThinkPad>
Content-Type: text/plain; charset="us-ascii"; format=flowed
Content-Disposition: inline
In-Reply-To: <Zamcu7tts8mqX0b4@yury-ThinkPad>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?NwGG/xDlszN1Xx6SdT7aWrfadshhOAxvU8kDvjWIoj/LD9n2vqUmLxpNRr5g?=
 =?us-ascii?Q?5cdV/KW0tKz/ndELKcu3V5g90LItp607ynGgwphmApDZqRTZGyog0jdtkcbf?=
 =?us-ascii?Q?Kn3j4qN0W8ttBJC9kdo81SUO8bqjT5EfHWg8vKdTGQmc40iOlO6ifOcJcdh4?=
 =?us-ascii?Q?ryeUTcf3dEz88dtFe8mEAaCOcbDXQ39QnHbertlMTZ3ac63JwuEa5y7voojr?=
 =?us-ascii?Q?QjbtROg8KEasYfqyqa0EbjxcG/BHMoEZWyHkoR0lNnPYakh11+ZYKfdIeI0X?=
 =?us-ascii?Q?udlYwde+qIaE+nmoyP3YN/t9uzWCeYDWZHu1HjNvNxMKdvmIjZmE8TeiGog7?=
 =?us-ascii?Q?rBlThclItN+rOAXQJQR8emMQGSHiNNSvxLPXNsZPjKLRj1moexafLsOezaMw?=
 =?us-ascii?Q?0klclETMCee9cKwGdNyobxroLxUwtJh3vVMhXZ8p8N8XQLnQsgNh6XKVn4bu?=
 =?us-ascii?Q?hXT+kJD9lNz0xyQF4ASRqQByHW4dgDWMXp3zlBLk5hwOm+HO3mUjnrYs6A69?=
 =?us-ascii?Q?+htI1SAl2MTpdSiyrjaF3QE9Bp0x/obtpBRo2LT6ZeBdkUh9Tg7RIANh454c?=
 =?us-ascii?Q?DkVDOC62gmbTF51Wed5czCmFFe0jI6Ws0liX4D12b8Rb3y42xojaRLfTeSKG?=
 =?us-ascii?Q?uUcShaKaUR2+lqsGWKGVP2WwOfGqAgE2TdFXhoyx70oMNoVKYRsOo2h5kaUk?=
 =?us-ascii?Q?z1aQo7jD8pQO7zVRYaTTwT51j1dHkXlxNDaJyuNYIgIpaf0tBywHm3FzuRqE?=
 =?us-ascii?Q?i4Xa3S8/pOE8edd3BmymPgIrFWji5F0+MJLcEcG3QMzBWlutu96e4dM5jfbU?=
 =?us-ascii?Q?XXSNdZ6xIgLybFWH5/VimMLvyyirpZpKpb33GAsiED+8Co8X027c0sPUubqa?=
 =?us-ascii?Q?ZqYpeXD5NkJjTngjdRGhm41Ayl2+YuxB4I/K9OHMCI+aq8mWeUyiM1NctIOi?=
 =?us-ascii?Q?fRRFVaul9ItLcTXvw3HuOROiunnAWw5Q/I4WoRZT+4dMydwzqq9O1mVOW4fr?=
 =?us-ascii?Q?jMf/XK0AdBnVGtrFvfx4YHJyjQurs3eyvBT66i8R8M16WWfGzUdrEDdWY+jZ?=
 =?us-ascii?Q?EJxaEU5I5dCU8QJ0tSifKaWya8EHcl6Bqaib7N3byJj4ymx8pXUbU9WgU1sB?=
 =?us-ascii?Q?vL7ce882umgIYoj4v8HYUnNR6TTNwzqKTCuqMCZAVhvbB2jmh5g6rcJ16u40?=
 =?us-ascii?Q?jkuuQh1iLOXd+FBi1bzKjzPjvah6WK3UTknwJy4x6FK7+1LQQb/A3rYYclfY?=
 =?us-ascii?Q?K3rDXJI7SfXva4rRRrK2tDKLQSxWg0xqn3xtFHPkPVi+WZcGPt1k3hq9nMLV?=
 =?us-ascii?Q?k4Kahwt7wSxgBV+a/0wOws/Sz0zfrXeSe32A+yHJkpOjKqaJ2PFaJ9GwGX1G?=
 =?us-ascii?Q?71AILPGVlIZDjmKE1elFvdI0qZgqXFaqlTj4Lrnj8h94dXuu/QjjznmGsdi4?=
 =?us-ascii?Q?/i/gAz/dPLOUKYDSNB9YD8don3AtYoe9D3oS8Bh2hw9/yvw9hmb0eQC8HEK9?=
 =?us-ascii?Q?Ysm9+ZkDAYxzklTNcPcfNURztR65VDRZyUKWsG3iAlSoB18RuLr+/pyGjyzK?=
 =?us-ascii?Q?gIaxhgHPiHqSQF6fn7InnCDOU2XwPs6DCTUmFRf/qmr1+5JxbNnyw5dLSH2p?=
 =?us-ascii?Q?OA=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 08a23386-6489-45e1-80dd-08dc187cb28a
X-MS-Exchange-CrossTenant-AuthSource: CY5PR11MB6139.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Jan 2024 23:25:03.1986
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 4r13IIjrVR5nS8osaC2d7GuNh7TrZ8I55XAVFlSaj+EZ/5CFOzZ9cK2LKd67vfr/enAknRUGBoieAIV1kQmzZEPa+OwDgTwzg2mAAjLuzJA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR11MB4874
X-OriginatorOrg: intel.com

On Thu, Jan 18, 2024 at 01:48:43PM -0800, Yury Norov wrote:
>On Thu, Jan 18, 2024 at 02:42:12PM -0600, Lucas De Marchi wrote:
>> Hi,
>>
>> Reviving this thread as now with xe driver merged we have 2 users for
>> a fixed-width BIT/GENMASK.
>
>Can you point where and why?

See users of REG_GENMASK and REG_BIT in drivers/gpu/drm/i915 and
drivers/gpu/drm/xe. I  think the register definition in the xe shows it
in a good way:

	drivers/gpu/drm/xe/regs/xe_gt_regs.h

The GPU registers are mostly 32-bit wide. We don't want to accidently do
something like below (s/30/33/ added for illustration purposes):

#define LSC_CHICKEN_BIT_0                       XE_REG_MCR(0xe7c8)
#define   DISABLE_D8_D16_COASLESCE              REG_BIT(33)

Same thing for GENMASK family of macros and for registers that are 16 or
8 bits. See e.g. drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h


>
>> On Wed, Jun 21, 2023 at 07:20:59PM -0700, Yury Norov wrote:
>> > Hi Lucas, all!
>> >
>> > (Thanks, Andy, for pointing to this thread.)
>> >
>> > On Mon, May 08, 2023 at 10:14:02PM -0700, Lucas De Marchi wrote:
>> > > Add GENMASK_U32(), GENMASK_U16() and GENMASK_U8()  macros to create
>> > > masks for fixed-width types and also the corresponding BIT_U32(),
>> > > BIT_U16() and BIT_U8().
>> >
>> > Can you split BIT() and GENMASK() material to separate patches?
>> >
>> > > All of those depend on a new "U" suffix added to the integer constant.
>> > > Due to naming clashes it's better to call the macro U32. Since C doesn't
>> > > have a proper suffix for short and char types, the U16 and U18 variants
>> > > just use U32 with one additional check in the BIT_* macros to make
>> > > sure the compiler gives an error when the those types overflow.
>> >
>> > I feel like I don't understand the sentence...
>> >
>> > > The BIT_U16() and BIT_U8() need the help of GENMASK_INPUT_CHECK(),
>> > > as otherwise they would allow an invalid bit to be passed. Hence
>> > > implement them in include/linux/bits.h rather than together with
>> > > the other BIT* variants.
>> >
>> > I don't think it's a good way to go because BIT() belongs to a more basic
>> > level than GENMASK(). Not mentioning possible header dependency issues.
>> > If you need to test against tighter numeric region, I'd suggest to
>> > do the same trick as  GENMASK_INPUT_CHECK() does, but in uapi/linux/const.h
>> > directly. Something like:
>> >        #define _U8(x)		(CONST_GT(U8_MAX, x) + _AC(x, U))
>>
>> but then make uapi/linux/const.h include linux/build_bug.h?
>> I was thinking about leaving BIT() define where it is, and add the
>> fixed-width versions in this header. I was thinking uapi/linux/const.h
>> was more about allowing the U/ULL suffixes for things shared with asm.
>
>You can't include kernel headers in uapi code. But you can try doing
>vice-versa: implement or move the pieces you need to share to the
>uapi/linux/const.h, and use them in the kernel code.

but in this CONST_GE() should trigger a BUG/static_assert
on U8_MAX < x. AFAICS that check can't be on the uapi/ side,
so there's nothing much left to change in uapi/linux/const.h.

I'd expect drivers to be the primary user of these fixed-width BIT
variants, hence the proposal to do  in include/linux/bits.h.
Ssomething like this WIP/untested diff (on top of your previous patch):


diff --git a/include/linux/bits.h b/include/linux/bits.h
index cb94128171b2..409cd10f7597 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -24,12 +24,16 @@
  #define GENMASK_INPUT_CHECK(h, l) \
  	(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
  		__is_constexpr((l) > (h)), (l) > (h), 0)))
+#define BIT_INPUT_CHECK(type, b) \
+	((BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
+		__is_constexpr(b), (b) >= BITS_PER_TYPE(type), 0))))
  #else
  /*
   * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
   * disable the input check if that is the case.
   */
  #define GENMASK_INPUT_CHECK(h, l) 0
+#define BIT_INPUT_CHECK(type, b) 0
  #endif
  
  #define __GENMASK(t, h, l) \
@@ -44,4 +48,9 @@
  #define GENMASK_U32(h, l)	__GENMASK(u32, h, l)
  #define GENMASK_U64(h, l)	__GENMASK(u64, h, l)
  
+#define BIT_U8(b)		(u8)(BIT_INPUT_CHECK(u8, b) + BIT(b))
+#define BIT_U16(b)		(u16)(BIT_INPUT_CHECK(u16, b) + BIT(b))
+#define BIT_U32(b)		(u32)(BIT_INPUT_CHECK(u32, b) + BIT(b))
+#define BIT_U64(b)		(u64)(BIT_INPUT_CHECK(u64, b) + BIT(b))
+
  #endif	/* __LINUX_BITS_H */

>
>In the worst case, you can just implement the macro you need in the
>uapi header, and make it working that way.
>
>Can you confirm that my proposal increases the kernel size? If so, is
>there any way to fix it? If it doesn't, I'd prefer to use the
>__GENMASK() approach.

I agree on continuing with your approach. The bloat-o-meter indeed
showed almost no difference. `size ....i915.o`  on the other hand
increased, but then decreased when I replaced our current REG_GENMASK()
implementation to reuse the new GENMASK_U*()

	$ # test-genmask.00: before any change
	$ # test-genmask.01: after your patch to GENMASK
	$ # test-genmask.01: after converting drivers/gpu/drm/i915/i915_reg_defs.h
	    to use the new macros
	$ size build64/drivers/gpu/drm/i915/i915.o-test-genmask.*
	   text    data     bss     dec     hex filename
	4506628  215083    7168 4728879  48282f build64/drivers/gpu/drm/i915/i915.o-test-genmask.00
	4511084  215083    7168 4733335  483997 build64/drivers/gpu/drm/i915/i915.o-test-genmask.01
	4493292  215083    7168 4715543  47f417 build64/drivers/gpu/drm/i915/i915.o-test-genmask.02

	$ ./scripts/bloat-o-meter  build64/drivers/gpu/drm/i915/i915.o-test-genmask.0[01]
	add/remove: 0/0 grow/shrink: 2/1 up/down: 4/-5 (-1)
	Function                                     old     new   delta
	intel_drrs_activate                          399     402      +3
	intel_psr_invalidate                         546     547      +1
	intel_psr_flush                              880     875      -5
	Total: Before=2980530, After=2980529, chg -0.00%

	$ ./scripts/bloat-o-meter  build64/drivers/gpu/drm/i915/i915.o-test-genmask.0[12]
	add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
	Function                                     old     new   delta
	Total: Before=2980529, After=2980529, chg +0.00%

thanks
Lucas De Marchi

>
>Thanks,
>Yury