Re: [igt-dev] [PATCH i-g-t 1/8] tests/i915/gem_pxp: Add LOCAL_ UAPI defines

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

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 :)