Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 20 Jun 2023 11:02:26 -0700
From:   Lucas De Marchi <lucas.demarchi@intel.com>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
CC:     Jani Nikula <jani.nikula@linux.intel.com>,
        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>,
        Alex Deucher <alexander.deucher@amd.com>,
        "Thomas Gleixner" <tglx@linutronix.de>,
        Masahiro Yamada <masahiroy@kernel.org>,
        <intel-xe@lists.freedesktop.org>
Subject: Re: [Intel-xe] [PATCH 2/3] linux/bits.h: Add fixed-width GENMASK and
 BIT macros
Message-ID: <itxb3tdwvyxzwlzvvbwv75jssjwfpve2k4b4bcfzyp5x5rb6wu@tzlzdn6dem46>
References: <20230509051403.2748545-3-lucas.demarchi@intel.com>
 <ZF4fi5B7PPlgZBOI@smile.fi.intel.com>
 <87pm75kd0h.fsf@intel.com>
 <ZF4j0NPoBGMBT8CO@smile.fi.intel.com>
 <87mt29kc34.fsf@intel.com>
 <ZIs0CC2J7nu0LHEK@smile.fi.intel.com>
 <875y7igph5.fsf@intel.com>
 <ZJG91zMQW3Rnvdbe@smile.fi.intel.com>
 <amgwl5mthhqgvgkqnor6tjfcr3x3pgwvpqin5efwwjfpdhvvpa@vhzhiq5mzsdg>
 <ZJHkthMktY83pwvy@smile.fi.intel.com>
Content-Type: text/plain; charset="us-ascii"; format=flowed
Content-Disposition: inline
In-Reply-To: <ZJHkthMktY83pwvy@smile.fi.intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?QedjxB2CVfjvmK47jaxKL84l2aLofUUQGVW94fZDxTTciXuZ6UzCWDKVvhVy?=
 =?us-ascii?Q?wA5g/ti2Vmkr3HtOjo3h3hrrH1RdwdVPYUjyqa6vcAAAxqWjkC/guedFx5bq?=
 =?us-ascii?Q?6Cb3vdst3KU4R5b/eoAER1mksSIkHB17KFaT3S3ZUPDJKW7L+bVh9JWpczhi?=
 =?us-ascii?Q?5SP9aIg7Cgr9Ux8W8/3hQ/0xXHLeywqSFODkhsA4tEHJAkp8A2B3gTtuq858?=
 =?us-ascii?Q?/u7KDeDXm6uWgCiIs/c/aQXdL7MQy/Mwn2s9XmKhB9e2MFALnIbVbUSoI5OE?=
 =?us-ascii?Q?ZCii7AzGirADQd8Wz9MHudlFSigqn9TCLXgTRSMXJQuZquACxF7PobXhO07x?=
 =?us-ascii?Q?l6epUoLRADQbx9a7JLWW3TOU3N0P2ilL/a6hO1qYwCuJMv06KXYC/0Uma0Uk?=
 =?us-ascii?Q?SKBZ/qbHUqG8f/cER6P5qrKogKAXnWxuCcqrgogzQ5ikUv9uJsvJFLrnjH4Q?=
 =?us-ascii?Q?ttiW/wlY6rnbIGBvjgZamTL10LG7W+mUx5DVWd6ZbVGS4OM53I7KOLochvxy?=
 =?us-ascii?Q?8rvjNdAjlDUYcfrPxYLzRV7T9fpEPRbvmT2+veOIMFauKjUGn5HqneIF8bxs?=
 =?us-ascii?Q?NNxxPc1IL9EU5jZvpOGe9kxcs8Rqb6NGl/TSruySMnnYWEsAf/taq6S3Mnnd?=
 =?us-ascii?Q?aFXCa3FqlAA9YrB4tmgsSN7Ii8aDGHxK3Hluy88Uvxrv3d+IQu4RtbNL1ADY?=
 =?us-ascii?Q?V/vGr0crOOqL19VFnTAZBcOYQWTXqSomfwOKiBB8UONkp1SiF4dywMzYh23T?=
 =?us-ascii?Q?FQpxL12HYa0MtLgSqWBz8nFlXDhBsRf7uPZQ6adbRqZ1m0emeQqRtODGzWHg?=
 =?us-ascii?Q?e+cw26l4wAd/nGKpQ9UHyWgkuWGjm+EbYF07R7Se5IT/PHS5ThfOIJuELV17?=
 =?us-ascii?Q?ed0BrI3Pnj8YJ3DJWbHQLfxpq3EU3dBDRHiRIHrXXjO0AUsjtRrc4twpUyTo?=
 =?us-ascii?Q?/W5eb+tLUYUOYcNuZdl36OeLpecesMiwX9kvoFKxM4EdnTm5IR1H/0frJiGI?=
 =?us-ascii?Q?sEIITYpTQ4ZUg89Rty4DkHtsrFhxYoik/cidXC/7POv8HsaiahvJ865GSVHq?=
 =?us-ascii?Q?QJpKC/6SBt00QwepL92gxpYH/yopgjkcbXo7BM+nqplYm0JIE3QREiJCGN7h?=
 =?us-ascii?Q?sjQUSiGhUggmaAe8dAGoUnJJmLD1YgbIAscSiki+3pvpiX6PN21igpnngQL4?=
 =?us-ascii?Q?EpgkFPW0mPdculgBdd24AfdxUWH33T8V2cO3+Jratl/OmfkdAMCMRseSWT8X?=
 =?us-ascii?Q?VXipKQjl99f0FhKB33KRYlMz92P6C+AjMptYz/E+OKd83pBiXUdlJKz00ubs?=
 =?us-ascii?Q?Nl30G4wMzo7m0686HHj9YxkwlHwYV5SsgmBd5x2eZyEOwTNVoIR33sGhuR/y?=
 =?us-ascii?Q?vf6MmSFqRvTclVzzD7vMVsMOngkcQNUGaXKLzGl81QMpV214URS2xKqIcX0O?=
 =?us-ascii?Q?RZewRQUSl8PKvocB9QAsY4ke3pFdmgjgrS66JZUjIrss/n5T/DaKcHHNy09P?=
 =?us-ascii?Q?9/95r3UXDc4zUrPhJ8i6an+N6w96ioMpjHOxY8iTKg/U/MLohqXGPbOvmf5e?=
 =?us-ascii?Q?u1GumkmevTyRTZ3zpoj+phYaBkgS3yDrQcaE6v88gtER/um01OpB8U7/wZa+?=
 =?us-ascii?Q?kQ=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f57df83b-56ce-4115-e33b-08db71b882ce
X-MS-Exchange-CrossTenant-AuthSource: CY5PR11MB6139.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Jun 2023 18:02:28.7099
 (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: EdzEgRi3xKlfUfRe8iOpOjmHFu2fKA0jE2/UPPAHd6liODoOFYP2uuhITpyPp3oUAkEei2rKGjqw7Wcwqfo4MLxgKv6UE++kHVCb3T1o8Fc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB7114
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Jun 20, 2023 at 08:41:10PM +0300, Andy Shevchenko wrote:
>On Tue, Jun 20, 2023 at 10:25:21AM -0700, Lucas De Marchi wrote:
>> On Tue, Jun 20, 2023 at 05:55:19PM +0300, Andy Shevchenko wrote:
>> > On Tue, Jun 20, 2023 at 05:47:34PM +0300, Jani Nikula wrote:
>> > > On Thu, 15 Jun 2023, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>> > > > On Fri, May 12, 2023 at 02:45:19PM +0300, Jani Nikula wrote:
>> > > >> On Fri, 12 May 2023, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>> > > >> > On Fri, May 12, 2023 at 02:25:18PM +0300, Jani Nikula wrote:
>> > > >> >> On Fri, 12 May 2023, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>> > > >> >> > 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().
>
>> > > >> >> > Why?
>> > > >> >>
>> > > >> >> The main reason is that GENMASK() and BIT() size varies for 32/64 bit
>> > > >> >> builds.
>> > > >> >
>> > > >> > When needed GENMASK_ULL() can be used (with respective castings perhaps)
>> > > >> > and BIT_ULL(), no?
>> > > >>
>> > > >> How does that help with making them the same 32-bit size on both 32 and
>> > > >> 64 bit builds?
>> > > >
>> > > > 	u32 x = GENMASK();
>> > > > 	u64 y = GENMASK_ULL();
>> > > >
>> > > > No? Then use in your code either x or y. Note that I assume that the parameters
>> > > > to GENMASK*() are built-time constants. Is it the case for you?
>> > >
>> > > What's wrong with wanting to define macros with specific size, depending
>> > > on e.g. hardware registers instead of build size?
>> >
>> > Nothing, but I think the problem is smaller than it's presented.
>>
>> not sure about big/small problem you are talking about. It's a problem
>> for when the *device* register is a 32b fixed width, which is
>> independent from the CPU you are running on. We also have registers that
>> are u16 and u64. Having fixed-width GENMASK and BIT helps avoiding
>> mistakes like below. Just to use one example, the diff below builds
>> fine on my 64b machine, yet it's obviously wrong:
>>
>> 	$ git diff 	diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
>> b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
>> 	index 0b414eae1683..692a0ad9a768 100644
>> 	--- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
>> 	+++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
>> 	@@ -261,8 +261,8 @@ static u32 rw_with_mcr_steering_fw(struct intel_gt *gt,
>> 			 * No need to save old steering reg value.
>> 			 */
>> 			intel_uncore_write_fw(uncore, MTL_MCR_SELECTOR,
>> 	-                                     REG_FIELD_PREP(MTL_MCR_GROUPID, group) |
>> 	-                                     REG_FIELD_PREP(MTL_MCR_INSTANCEID, instance) |
>> 	+                                     FIELD_PREP(MTL_MCR_GROUPID, group) |
>> 	+                                     FIELD_PREP(MTL_MCR_INSTANCEID, instance) |
>> 					      (rw_flag == FW_REG_READ ? GEN11_MCR_MULTICAST : 0));
>> 		} else if (GRAPHICS_VER(uncore->i915) >= 11) {
>> 			mcr_mask = GEN11_MCR_SLICE_MASK | GEN11_MCR_SUBSLICE_MASK;
>> 	diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
>> 	index 718cb2c80f79..c42bc2900c6a 100644
>> 	--- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
>> 	+++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
>> 	@@ -80,8 +80,8 @@
>> 	 #define   GEN11_MCR_SLICE_MASK                 GEN11_MCR_SLICE(0xf)
>> 	 #define   GEN11_MCR_SUBSLICE(subslice)         (((subslice) & 0x7) << 24)
>> 	 #define   GEN11_MCR_SUBSLICE_MASK              GEN11_MCR_SUBSLICE(0x7)
>> 	-#define   MTL_MCR_GROUPID                      REG_GENMASK(11, 8)
>> 	-#define   MTL_MCR_INSTANCEID                   REG_GENMASK(3, 0)
>> 	+#define   MTL_MCR_GROUPID                      GENMASK(32, 8)
>> 	+#define   MTL_MCR_INSTANCEID                   GENMASK(3, 0)
>> 	 	 #define IPEIR_I965                             _MMIO(0x2064)
>> 	 #define IPEHR_I965                             _MMIO(0x2068)
>>
>> If the driver didn't support 32b CPUs, this would even go unnoticed.
>
>So, what does prevent you from using GENMASK_ULL()?

nothing is preventing me to write the wrong code, which is what we are
trying to solve. GENMASK_ULL() would generate the wrong code as that
particular register is 32b, not 64b, on the GPU.

>
>Another point, you may teach GENMASK() to issue a warning if hi and/or lo
>bigger than BITS_PER_LONG.

Which varies depending on the CPU you are building for, so it misses the
point.  GENMASK_U32/GENMASK_U16/GENMASK_U8 and BIT counterparts would
emit a warning if hi is bigger than _exactly_ 32, 16 or 8, regardless
of the CPU you built the code for.

Lucas De Marchi

>
>I still don't see the usefulness of that churn.
>
>> Lucas De Marchi
>>
>> > And there are already header for bitfields with a lot of helpers
>> > for (similar) cases if not yours.
>> >
>> > > What would you use for printk format if you wanted to to print
>> > > GENMASK()?
>> >
>> > %lu, no?
>
>-- 
>With Best Regards,
>Andy Shevchenko
>
>