From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTPS id A76D86EE64 for ; Wed, 6 Oct 2021 20:19:43 +0000 (UTC) Date: Wed, 06 Oct 2021 13:19:42 -0700 Message-ID: <87tuhtvrc1.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" In-Reply-To: <49591b5a838cdda9298bda1c395de9353ce38f38.camel@intel.com> References: <20211006161444.904476-1-rodrigo.vivi@intel.com> <20211006161444.904476-2-rodrigo.vivi@intel.com> <877deqvzre.wl-ashutosh.dixit@intel.com> <8735pevxdt.wl-ashutosh.dixit@intel.com> <49591b5a838cdda9298bda1c395de9353ce38f38.camel@intel.com> MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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: "Vivi, Rodrigo" Cc: "igt-dev@lists.freedesktop.org" , "Teres Alexis, Alan Previn" , "Latvala, Petri" List-ID: On Wed, 06 Oct 2021 12:40:01 -0700, Vivi, Rodrigo wrote: > > On Wed, 2021-10-06 at 11:09 -0700, Dixit, Ashutosh wrote: > > 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? > > > > > > > > > > > > > > /* =A0* It is necessary on occasion to add uapi declarations to I= GT > > > > > before they =A0* appear in imported kernel uapi headers. This > > > > > header is provided for this =A0* purpose. > > > > > > > > > > =A0* Early uapi declarations should be added here exactly as they > > > > > are =A0* expected to appear in the kernel uapi headers, > > > > > i.e. without the LOCAL_ =A0* or local_ prefix and without any > > > > > #ifndef's. Attempt should be made to =A0* clean these up when > > > > > kernel uapi headers are sync'd. =A0*/ > > > > > > 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. > > I had identical declarations of a struct and it caused conflict for me. You are right, compiler will complain about identical duplicate structs, just not about identical duplicate #define's :( Probably not a bad idea since it will force stuff to be deleted from i915_drm_local.h :) > This is why I had to squash the changes of patches 1 to the revert on > the other patch to revert the i915_drm.h: > > https://patchwork.freedesktop.org/patch/457774/ > > That's why I came to vent :) > > But anyway, I got the point and I believe it is a good thing overall. > > Could you please help with the review of the patch in this link above? > > > 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 :) >