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

[Jani Nikula]

On Fri, 10 Mar 2023, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> On Fri, Mar 10, 2023 at 03:54:24PM +0200, Jani Nikula wrote:
>> On Fri, 10 Mar 2023, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
>> > On Fri, Mar 10, 2023 at 03:36:05PM +0200, Jani Nikula wrote:
>> >> On Fri, 10 Mar 2023, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
>> >> > On Fri, Mar 10, 2023 at 12:11:26PM +0200, Jani Nikula wrote:
>> >> >> On Fri, 10 Mar 2023, Tvrtko Ursulin <tvrtko.ursulin at 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 at intel.com>
>> >> >> >> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> >> >> >> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
>> >> >> >> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> >> >> >> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>> >> >> >> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> >> >> >> Signed-off-by: Jani Nikula <jani.nikula at 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.
>> >> 
>> >> In general, we build the same display source files to two sets of object
>> >> files, in i915 and xe, with different build flags. IOW, in the same
>> >> kernel build, the display files get built twice, once for i915, once for
>> >> xe, provided both are enabled in kconfig. They become two completely
>> >> independent binary .ko.
>> >> 
>> >> As to the legacy, with this patch, i915 Makefile would pass
>> >> -DI915_LEGACY=1 for CONFIG_DRM_I915_LEGACY=y, while the xe Makefile
>> >> would do no such thing.
>> >> 
>> >> As to probing, both have the module device tables for the PCI IDs they
>> >> support, and you need to play with the force_probe parameter in both to
>> >> force/block probing. Maybe modprobe blacklisting could also be used to
>> >> choose the driver for the devices supported by both drivers.
>> >
>> > Hmm. I suppose one option might be to remove those platforms from
>> > the PCI ID table in i915, but still allow the driver to probe them.
>> 
>> I'm not sure I follow here. What purpose does that serve?
>
> It would make the system load xe by default for those devices.

Right, I thought you were talking about pre-tgl platforms.

Eventually, we'll have two cutoff points by platforms supported:

	i915-only | i915 & xe | xe-only

I don't know where those |'s will be exactly. For now, the first one is
around tgl. We might also have a third cutoff somewhere within "i915 &
xe" where the default shifts from i915 to xe.


BR,
Jani.


>
>> 
>> > And it should still require force_probe so that if you have old+new
>> > GPU in the system and i915 loads first it wont't snatch up the
>> > new GPU by default.
>> 
>> 
>> 
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center