From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTPS id 646BB6EDF2 for ; Wed, 6 Oct 2021 18:23:48 +0000 (UTC) Date: Wed, 06 Oct 2021 11:09:02 -0700 Message-ID: <8735pevxdt.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" In-Reply-To: References: <20211006161444.904476-1-rodrigo.vivi@intel.com> <20211006161444.904476-2-rodrigo.vivi@intel.com> <877deqvzre.wl-ashutosh.dixit@intel.com> MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Subject: Re: [igt-dev] [PATCH i-g-t 1/8] tests/i915/gem_pxp: Add LOCAL_ UAPI defines List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: Rodrigo Vivi Cc: igt-dev@lists.freedesktop.org, Alan Previn , Petri Latvala List-ID: On Wed, 06 Oct 2021 10:37:13 -0700, Rodrigo Vivi wrote: > > On Wed, Oct 06, 2021 at 01:25:28PM -0400, Rodrigo Vivi wrote: > > On Wed, Oct 06, 2021 at 10:17:41AM -0700, Dixit, Ashutosh wrote: > > > On Wed, 06 Oct 2021 09:14:37 -0700, Rodrigo Vivi wrote: > > > > > > > > While the UAPI changes don't propagate to drm-next we should > > > > have that as LOCAL_ ones. > > > > > > Please add these in lib/i915/i915_drm_local.h as follows: > > > > Could we move this file to the include directory and document it > > in the README.md along with the uapi sync mention? > > > > > > > > /* > > > * It is necessary on occasion to add uapi declarations to IGT before they > > > * appear in imported kernel uapi headers. This header is provided for this > > > * purpose. > > > > > > * Early uapi declarations should be added here exactly as they are > > > * expected to appear in the kernel uapi headers, i.e. without the LOCAL_ > > > * or local_ prefix and without any #ifndef's. Attempt should be made to > > > * clean these up when kernel uapi headers are sync'd. > > > */ > > I'd like to highlight that I have a concern with this approach without > the local_ prefix. See the commit message for bb1c96b29234 where i915_drm_local.h was introduced for the rationale for this approach. > This approach would force anyone that is syncing the header to solve > everyone's else updates. As long as there are no conflicts (say between i915_drm.h and i915_drm_local.h) there is no problem since the compiler silently ignores identical duplicate declarations. If there is a conflict and it is not clear what to do then the developers will need to communicate and resolve the conflict. We haven't seen this yet so let's see if this causes problems and then revisit I think. > It gets even worse and uglier when the api in here diverged from the > actually merged upstream. Okay, one can say this is really rare. > > I'm okay with a centralized place for the locals... I believe we even > had that in the past. But I don't like the idea of the lack of prefix. The thinking was that the lack of local_ or LOCAL_ prefix actually makes it easier to catch conflicts between what got merged into the kernel and what was originally added to IGT since the compiler will warn us about it. So it helps to convert a possible run time failure into a compile time failure. Anyway I was tired of seeing so many stale LOCAL declarations lying around so we are trying this out and asking people to use it whenever we spot this in their patches :)