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 X-Spam-Level: X-Spam-Status: No, score=-8.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id F3EB0C4338F for ; Thu, 29 Jul 2021 01:53:55 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id A837E6101C for ; Thu, 29 Jul 2021 01:53:55 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org A837E6101C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 426256E8BF; Thu, 29 Jul 2021 01:53:55 +0000 (UTC) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by gabe.freedesktop.org (Postfix) with ESMTPS id 587606E8BF; Thu, 29 Jul 2021 01:53:54 +0000 (UTC) X-IronPort-AV: E=McAfee;i="6200,9189,10059"; a="276566730" X-IronPort-AV: E=Sophos;i="5.84,276,1620716400"; d="scan'208";a="276566730" Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Jul 2021 18:53:52 -0700 X-IronPort-AV: E=Sophos;i="5.84,276,1620716400"; d="scan'208";a="517878583" Received: from adixit-mobl1.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.209.99.34]) by fmsmga002-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Jul 2021 18:53:51 -0700 Date: Wed, 28 Jul 2021 18:53:50 -0700 Message-ID: <87czr151ht.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Petri Latvala In-Reply-To: References: <20210726120310.1115522-1-matthew.auld@intel.com> <87fsvznqmj.wl-ashutosh.dixit@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/27.2 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Subject: Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/7] lib/i915/gem_mman: add FIXED mmap mode 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: igt-dev@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, Matthew Auld Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Tue, 27 Jul 2021 23:08:40 -0700, Petri Latvala wrote: > > On Tue, Jul 27, 2021 at 07:01:24PM -0700, Dixit, Ashutosh wrote: > > On Mon, 26 Jul 2021 05:03:04 -0700, Matthew Auld wrote: > > > > > > diff --git a/lib/i915/gem_mman.c b/lib/i915/gem_mman.c > > > index 4b4f2114..e2514f0c 100644 > > > --- a/lib/i915/gem_mman.c > > > +++ b/lib/i915/gem_mman.c > > > @@ -497,6 +497,43 @@ void *gem_mmap_offset__cpu(int fd, uint32_t handle, uint64_t offset, > > > return ptr; > > > } > > > > > > +#define LOCAL_I915_MMAP_OFFSET_FIXED 4 > > > > Cc: @Petri Latvala > > > > This use of LOCAL declarations is more related to the methodology we follow > > in IGT rather than this patch. We have seen in the past that such LOCAL's > > linger on in the code for months and years till someone decides to clean > > them so the question is can we prevent these LOCAL's from getting > > introduced in the first place. > > > > One reason for these is that we sync IGT headers with drm-next whereas IGT > > is used to test drm-tip. So the delta between the two results in such > > LOCAL's as in this case. > > > > My proposal is that even if we don't start sync'ing IGT headers with > > drm-tip (instead of drm-next) we allow direct modification of the headers > > when needed to avoid introducing such LOCAL's. So in the above case we > > would add: > > > > #define I915_MMAP_OFFSET_FIXED 4 > > > > to i915_drm.h as part of this patch and then just use > > I915_MMAP_OFFSET_FIXED. If another sync happens from drm-next before this > > #define has appeared there, the compile will break and whoever syncs will > > need to add this again to i915_drm.h. > > I don't like that kind of a breakage at all. That enforces mandatory > fixups to some poor developer working on unrelated code who doesn't > necessarily know how to even fix it easily. > > Of course an argument can be made that it's an i915 token in an i915 > header so it will be the i915 people doing it, but for a general case > that's going to cause more harm than it solves problems, I feel. OK, let's not change anything with the headers we import for now. > > What do people think about a scheme such as this? The other, perhaps > > better, option of course is to sync the headers directly with drm-tip > > (whenever and as often as needed). But the goal in both cases is to avoid > > LOCAL's, or other things like #ifndef's distributed throughout multiple > > source files which we also do in such cases. A centralized internal header > > to contain such declarations might not be so bad. Thanks. > > A separate manually written header for new tokens that are not yet in > drm-next might be the least bad of all options. Although now that I've > said it, the perfect world would have new tokens done like this: > > #ifndef I915_MMAP_OFFSET_FIXED > #define I915_MMAP_OFFSET_FIXED 4 > #else > _Static_assert(I915_MMAP_OFFSET_FIXED == 4, "ABI broken, yikes"); > #endif > > In a different language wrapping all that in a > > MAYBE_DECLARE(I915_MMAP_OFFSET_FIXED, 4) > > might be easier but with C preprocessor it's a bit more... involved. A > separate build-time script to generate that header maybe? Such a > script could also just completely omit the definition if header copies > already introduce the token. IMO all this wouldn't do much more that what gcc already does. For example, for this: #define I915_MMAP_OFFSET_FIXED 4 #define I915_MMAP_OFFSET_FIXED 4 gcc silently ignores the second #define, whereas for: #define I915_MMAP_OFFSET_FIXED 4 #define I915_MMAP_OFFSET_FIXED 5 gcc will warn that second I915_MMAP_OFFSET_FIXED is redefined. And gcc will error out on things like struct redeclaration. > Recap: > > 1) We have kernel headers copied into IGT to ensure it builds fine > without latest-and-greatest headers installed on the system. > > 2) Copies are from drm-next to ensure the next person to copy the > headers doesn't accidentally drop definitions that originate from a > vendor-specific tree. (That same reason is also for why one shouldn't > edit the headers manually) > > 3) To get to drm-next, the kernel code needs to be tested with IGT > first, so we need new definitions to test that kernel code in some > form. I guess it is possible to test with "Test-with:" and merge the kernel changes first and the IGT changes later with the correct headers but maybe it's inconvenient? But don't we merge the kernel changes before IGT? > 4) LOCAL_* definitions that are cleaned up later when actual > definitions reach drm-next sounds good in theory but in practice the > cleaning part is often forgotten. > > Either way, I think the code using new definitions should use the > intended final names so we should just entirely drop the practice of > declaring anything LOCAL_*. That way the cleanup is limited to one > place. In any case, any thoughts about the i915_drm_local.h patch I posted: https://patchwork.freedesktop.org/series/93096/ I am not asking for any other changes to anything at this this point. I have also started asking people to not use the LOCAL_ or local_ prefix any more in code reviews. But I probably still prefer that these declarations move to a central place such as i915_drm_local.h if possible so it's easier to clean them up later. Thanks. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx