Re: [Intel-xe] [RFC] drm/i915: add kconfig option to enable/disable legacy platform support

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Mar 10, 2023 at 12:11:26PM +0200, Jani Nikula wrote:
> On Fri, 10 Mar 2023, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> > On 09/03/2023 19:19, Jani Nikula wrote:
> >> Add config option DRM_I915_LEGACY to enable/disable legacy platform
> >> support. This is primarily for the benefit of the drm/xe driver, and
> >> legacy is defined in terms of the platforms drm/xe does not support,
> >> i.e. anything before Tigerlake.
> >> 
> >> While the kconfig option will be CONFIG_DRM_I915_LEGACY, the intention
> >> is that it's not used in code. Instead, we'll pass -DI915_LEGACY=1 in
> >> the i915 Makefile for CONFIG_DRM_I915_LEGACY=y, while the xe Makefile
> >> does no such thing, regardless of the kconfig value.
> >> 
> >> Initially, the knob does the bare minimum: drops the legacy platforms
> >> from module PCI ID table (and the compiler in turn automagically drops
> >> all the unreferenced device infos).
> >> 
> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> 
> >> ---
> >> 
> >> *** NOTE ***
> >> 
> >> For now, I'm only sending this to the intel-xe mailing list with a bunch
> >> of Cc's for first impressions.
> >> 
> >> The xe driver reuses i915 display code, but there's a lot of unnecessary
> >> and/or incompatible code for platforms xe does not support. Currently
> >> this is handled with a bunch of #ifdef I915 added to i915 in the xe
> >> branch that isn't really upstreamble, and I'm thinking this patch might
> >> be a better option.
> >> 
> >> This patch alone does what the commit message says, and drops the legacy
> >> platform support, although all the code is left in place. Everything
> >> beyond this is basically an optimization of what more to drop out of the
> >> build. It doesn't really need to be perfect for starters but we could
> >> start converting the legacy platform related #ifdefs from I915 to
> >> I915_LEGACY, and that could be upstreamable to i915.
> >> 
> >> Not all of the #ifdef I915 in the xe branch are related to legacy
> >> platforms, and they need to be handled differently. But this kconfig
> >> knob would hopefully be a future compatible start to clean up one aspect
> >> of them.
> >> 
> >> Thoughts?
> >
> > Two questions for now:
> >
> > 1)
> > This does not still end up a sprinkling of #ifdefs in i915, just 
> > I915_LEGACY instead of I915? I mean I don't immediately follow how this 
> > leads to a more upstreamable solution?
> 
> In general, I find it difficult to accept any solutions upstream that
> cater only for out-of-tree code. Xe *alone* is not a good justification
> for making changes upstream. Everything that I've done in terms of
> refactoring stand on their own merits, but they *also* help Xe.
> 
> The current #ifdef I915 in the Xe branch are conflated between dropping
> some legacy platform support as well as for using different interfaces
> for gem, etc. Some of it might be okay when Xe is merged upstream, and
> the justification is upstream. But not now.
> 
> I'm arguing a way to build a trimmed down version of i915 with legacy
> platform support dropped is somewhat useful in itself. Something that
> I'm hoping we could take in upstream i915 much before Xe is
> upstream. And it would also help Xe by letting us remove a lot of
> out-of-tree #ifdef I915. Not everything, but a lot.

I was worried about exposing this and some crazy distros turning
it off thinking those "legacy" platforms aren't actually relevant
at all. But I guess the EXPERT dependency should deter that
somewhat.

What is the plan for building i915+xe at the same time btw? Would
we always have to disable the new platforms in i915 or can we build
support for the same platform into both drivers? I think having
both drivers available without rebuilding could be helpful in
debugging. But I don't know how the modprobe et al would deal
with that.

-- 
Ville Syrjälä
Intel