Received-SPF: pass (google.com: domain of linux-kernel+bounces-31298-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Fri, 19 Jan 2024 09:07:01 -0600
From: Lucas De Marchi <lucas.demarchi@intel.com>
To: Yury Norov <yury.norov@gmail.com>
CC: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	<intel-gfx@lists.freedesktop.org>, Kevin Brodsky <kevin.brodsky@arm.com>,
	<linux-kernel@vger.kernel.org>, <dri-devel@lists.freedesktop.org>,
	<intel-xe@lists.freedesktop.org>, Thomas Gleixner <tglx@linutronix.de>, "Alex
 Deucher" <alexander.deucher@amd.com>, Andrew Morton
	<akpm@linux-foundation.org>, Masahiro Yamada <masahiroy@kernel.org>,
	Christian =?utf-8?B?S8O2bmln?= <christian.koenig@amd.com>
Subject: Re: Re: Re: Re: [Intel-xe] [PATCH 2/3] linux/bits.h: Add fixed-width
 GENMASK and BIT macros
Message-ID: <ydmngwamztwexmjbbm2l62agpeebvkuszissoq4mjbq7ccgwdv@srd4tqz7zjve>
References: <20230509051403.2748545-1-lucas.demarchi@intel.com>
 <20230509051403.2748545-3-lucas.demarchi@intel.com>
 <ZJOwC5LIEySpduQJ@yury-ThinkPad>
 <4ezps56sdj7fmr27ivkaqjakv4ex46f5cvmy6oqr3z6gkhiorl@us4qd53jzq34>
 <Zamcu7tts8mqX0b4@yury-ThinkPad>
 <clamvpymzwiehjqd6jhuigymyg5ikxewxyeee2eae4tgzmaz7u@6rposizee3t6>
 <ZanYFqS7YrVYQsjB@yury-ThinkPad>
Content-Type: text/plain; charset="us-ascii"; format=flowed
Content-Disposition: inline
In-Reply-To: <ZanYFqS7YrVYQsjB@yury-ThinkPad>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?XNS4vflk373UO3JFKKEnjpfmIykp3JlVp50u8M9pTXPkNckRTwGBOoKRKcBM?=
 =?us-ascii?Q?e52wAaJROQrhElOoPGZTaj7nd64GyJ0omhYZ7ZE/qsN9BondhKjxhSqU4mQK?=
 =?us-ascii?Q?Q6dmyL5ra4SmZSAu8INRDa7SaGzkdAG6HF16+SiQJ+VvFOLi6d5zVSAzB3gx?=
 =?us-ascii?Q?ZQNZBaMemuN1hOFYK/ZAnfdn512twKVoWNYDGr3oSbwDdijRp2TxRtGKgl3k?=
 =?us-ascii?Q?p8vyubm7k2qLdQ4oDFk1lQfcy0CYX4UQ0xSvwl6dLQZY/cCLWnc98Y31bmwP?=
 =?us-ascii?Q?X7WKJ5FYE+6HfZlJEM7mzNNK9bo/+ZrgDJAx0BAXSWKTMy5BorMsdfnTz3Du?=
 =?us-ascii?Q?gkjgBskdj4/b2gpWde7OKhBb9m2U72fp3Huq5ROP4CcqXv4UOSChQmlE86fb?=
 =?us-ascii?Q?Z0hqwhAPln858ZUm1WGfdPa4mS33KWdWc7Y27c5nZnQM6noY0s6UMz1vADH2?=
 =?us-ascii?Q?ElbF/D7KO33aigMhpcIiPcHMgcsBBF+H7FFCLwepdubVeqFHUkTUec/KV33+?=
 =?us-ascii?Q?awJ3//M7U0dmV4xXybvktfiDwepQol26g+qfHZUfTydWEhIRWHapG2C5TMw3?=
 =?us-ascii?Q?TcpLNkqDyJj3xbcmy4b6X42Tvjb8BrLAuoTjUSPoEqBwSVMYsQxWa/cSQxuf?=
 =?us-ascii?Q?WZ+bJHrYsMfKjftHAuzewsu+qIjClAsKd/UydXwa91EzCmugibfHnnGxp2a3?=
 =?us-ascii?Q?Almsp/LqQsA9oWYnSQ4YSpsOMdjwwTNx2gJLtWSd1z3NRFt9xGAfVAu7KC6W?=
 =?us-ascii?Q?/FtxDuhBZ+6Q16cDmVc10TNhViMyZrv/JKtNWHy5jVJJRplIR500BudYdb0y?=
 =?us-ascii?Q?yXua1m+1oQD8hHjUwMWtO3q//HkS8D4NdzuwExKy4tTM6ADb2gI/OyMFWyio?=
 =?us-ascii?Q?RBmV8kzz2UPxlBvMkQkSfjcGnmLrdIZswqRPq+eqcyxtKiMPbUu+BoiCeF8k?=
 =?us-ascii?Q?ZRBgQnzzCcIRH94bC9+jXbzF2EJU6THa5nEaSdLL64om5g/B1FQrKycGKa/4?=
 =?us-ascii?Q?YXMqEa+VPKz2vK5MvEOtUluzHlT3XxBgaGPtO+Z3uct/LOwwrpuywcIGEhel?=
 =?us-ascii?Q?ous6PjmPHHH9F4PZKH1Q8rNkP4mlw+/84AgSA8rmcRQVo/MSHpyp8gwR59qs?=
 =?us-ascii?Q?Nlbjykd0CWLDti1z2JFleMIB/jx0RNAHnjqkLZjwrmNtQO0osM0LUtGzrutG?=
 =?us-ascii?Q?a732kd0LHQFhR1WSR4nQwnmEKsDa5eqRr1ZMNlrl4lt1g7o6nxl0G2CICEoc?=
 =?us-ascii?Q?nvL0hTNH1eP24wpL+Gla6JBnINWwVHRzXG6VdlqatBfIp0roUPCZ304fo4C5?=
 =?us-ascii?Q?LE1ohRxtWMibC5LeSy9v+DmQyhVq9Bqo2sT/mTBho2C+6ucTkx9DlPtM3mlY?=
 =?us-ascii?Q?nvXHSOKrKGNcrPPAwXp716OShatclFP1DdhWW+QTwqhE77A62gLvQnmtbtOy?=
 =?us-ascii?Q?JWcNtLAnFXgyFl1CKt9ge+xEOp5zp2wykPNcAMZjE2FT51aTimnIywdm2nx7?=
 =?us-ascii?Q?DKDpbURPUbbQdTYSenOL8GCiypTBcIxotYGYIcpluRB+kANIyDCzXTUcCewn?=
 =?us-ascii?Q?Fr6+ceq8hpu9WErXxSpdirM9SnE4QC+tCIN17UiSP1eBu+VfhWA8iLqoQFrK?=
 =?us-ascii?Q?2Q=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: bfc94e14-6ba8-42f4-f136-08dc19004bb2
X-MS-Exchange-CrossTenant-AuthSource: CY5PR11MB6139.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Jan 2024 15:07:04.2823
 (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: DDjxKjAedItjbGR/e+HWfX0h9ljRwlesngNisE7otJOSMCJBcEU90TkFRNFaTqyU4IfFF52YyllYhpBthJLFi2XglxbA7RtC+BwYiE5Davk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR11MB6958
X-OriginatorOrg: intel.com

On Thu, Jan 18, 2024 at 06:01:58PM -0800, Yury Norov wrote:
>On Thu, Jan 18, 2024 at 05:25:00PM -0600, Lucas De Marchi wrote:
>> SA2PR11MB4874
>> X-OriginatorOrg: intel.com
>> Status: RO
>> Content-Length: 6257
>> Lines: 150
>>
>> 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))
>
>Can you add some vertical spacing here, like between GENMASK and BIT
>blocks?

I think gmail mangled this, because it does show up with more vertical
space on the email I sent:
https://lore.kernel.org/all/clamvpymzwiehjqd6jhuigymyg5ikxewxyeee2eae4tgzmaz7u@6rposizee3t6/

Anyway, I will clean this up and probably add some docs about its usage.

>
>> +
>>  #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
>
>OK then. With the above approach, fixed-type BIT() macros look like wrappers
>around the plain BIT(), and I think, we can live with that.
>
>Can you  send all the material as a proper series, including my
>GENMASK patch, your patch above and a patch that switches your driver
>to using the new API? I'll take it then in bitmap-for-next when the
>merge window will get closed.

sure, thanks


Lucas De Marchi

>
>Thanks,
>Yury