Re: [Intel-xe] [PATCH 00/21] xe & i915 display integration ifdef cleanups

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Apr 05, 2023 at 06:49:34PM +0300, Jani Nikula wrote:
> On Wed, 05 Apr 2023, Jani Nikula <jani.nikula@intel.com> wrote:
> > Hey all, here's my first batch of #ifdef cleanups for xe and i915
> > display integration.
> >
> > Many of the patches in this series break the build, and will get fixed
> > by subsequent changes. It's pretty much unavoidable as many of the
> > changes are standalone to i915, and others are fixups.
> >
> > This is pretty straightforward stuff for starters, really. The idea is
> > that in the next rebase, the drm/i915 changes here go *before* any of
> > the current i915 changes.
> >
> > Adding the static inline stubs for xe build in i915 (the !I915 parts) is
> > very helpful in not sprinkling #ifdefs all over the place. A lot of the
> > time, the compiler is able to just compile out lots and lots of
> > unreachable static functions and data without explicitly conditionally
> > building them out. We can leave a lot of it out from "drm/i915/display:
> > Remaining changes to make xe compile". This is crucial especially while
> > upstreaming by keeping the changes to i915 minimal.
> >
> > Also, I think the conditional build and stubs in headers is the least
> > intrusive way of going about this before xe is actually upstream, and it
> > also follows the usual patterns for CONFIG_FOO=n code paths, albeit I915
> > is defined in the Makefile, not in kconfig.
> 
> To this end, would like to get up front acks for the approach from drm
> maintainers and my fellow i915 maintainers.
> 
> Are the drm/i915 patches (i.e. all the ones that are not fixups) in the
> series okay by you for the xe and i915 display integration?

Ack on these patches. I think for the overall approach of compiling
i915/display twice there might be some risk with having duplicated
symbols. Did you try to build-in both drivers and see what all breaks?

Since we shouldn't have EXPORT_SYMBOL on gen12+ anymore but only
component.c and aux-bus to glue things together across devices/drivers I
dont see an issue at least with modules. The gen5 ips stuff is I think the
only EXPORT_SYMBOL/get_symbol horror show.
-Daniel

> 
> If yes, would you also be okay with merging them to upstream i915 before
> xe is actually submitted upstream, or only as part of the xe (display)
> submission?
> 
> 
> Thanks,
> Jani.
> 
> 
> >
> > BR,
> > Jani.
> >
> >
> >
> > Jani Nikula (21):
> >   fixup! drm/i915/display: Set DISPLAY_MMIO_BASE to 0 for xe
> >   drm/i915: define I915 during i915 driver build
> >   drm/i915/display: add I915 conditional build to intel_lvds.h
> >   fixup! drm/xe/display: Implement display support
> >   drm/i915/display: add I915 conditional build to hsw_ips.h
> >   fixup! drm/i915/display: Remaining changes to make xe compile
> >   fixup! drm/xe/display: Implement display support
> >   drm/i915/display: add I915 conditional build to i9xx_plane.h
> >   fixup! drm/i915/display: Remaining changes to make xe compile
> >   fixup! drm/i915/display: Remaining changes to make xe compile
> >   drm/i915/display: add I915 conditional build to intel_lpe_audio.h
> >   fixup! drm/i915/display: Remaining changes to make xe compile
> >   drm/i915/display: add I915 conditional build to intel_pch_refclk.h
> >   fixup! drm/i915/display: Remaining changes to make xe compile
> >   drm/i915/display: add I915 conditional build to intel_pch_display.h
> >   drm/i915/display: add I915 conditional build to intel_sprite.h
> >   fixup! drm/i915/display: Remaining changes to make xe compile
> >   fixup! drm/xe/display: Implement display support
> >   drm/i915/display: add I915 conditional build to intel_overlay.h
> >   fixup! drm/i915/display: Remaining changes to make xe compile
> >   fixup! drm/xe/display: Implement display support
> >
> >  drivers/gpu/drm/i915/display/hsw_ips.h        | 35 +++++++++++
> >  drivers/gpu/drm/i915/display/i9xx_plane.h     | 23 +++++++
> >  drivers/gpu/drm/i915/display/intel_cdclk.c    |  4 +-
> >  drivers/gpu/drm/i915/display/intel_crtc.c     |  6 +-
> >  drivers/gpu/drm/i915/display/intel_display.c  | 15 +----
> >  .../gpu/drm/i915/display/intel_lpe_audio.h    | 20 ++++--
> >  drivers/gpu/drm/i915/display/intel_lvds.h     | 19 ++++++
> >  drivers/gpu/drm/i915/display/intel_overlay.h  | 35 +++++++++++
> >  .../gpu/drm/i915/display/intel_pch_display.h  | 63 +++++++++++++++----
> >  .../gpu/drm/i915/display/intel_pch_refclk.h   | 25 ++++++--
> >  drivers/gpu/drm/i915/display/intel_sprite.c   | 20 +-----
> >  drivers/gpu/drm/i915/display/intel_sprite.h   |  8 +++
> >  drivers/gpu/drm/xe/Makefile                   |  2 -
> >  .../gpu/drm/xe/compat-i915-headers/i915_drv.h |  7 +--
> >  14 files changed, 211 insertions(+), 71 deletions(-)
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch