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 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?
> > > >
> > > > >
> > > > > /*  * 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.
>
> 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 :)
>