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

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 14 Mar 2023, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 14/03/2023 11:43, Jani Nikula wrote:
>> On Thu, 09 Mar 2023, Jani Nikula <jani.nikula@intel.com> 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>
>> 
>> The discussion stalled a bit.
>> 
>> Do we have consensus to start adding this to upstream i915?
>
> I always liked the idea of compiling out platform support so I could be 
> convinced. I view that as a "power user" use case - compiles their own 
> kernel for a targeted machine. It also translates to building smaller 
> images in production settings although that is kind of not interesting 
> with the storage amounts these days. So overall feels could be 
> justified. There is some benefit and could be done with minimal 
> maintenance cost.
>
> But to add a Kconfig calling something "legacy", by the definition of 
> what Xe will support feels maybe a bit premature. Sure it will become 
> super useful once Xe is in the tree, to allow exactly the same class 
> use-case as above, but until then it feels questionable under your own 
> criteria too.

I don't disagree. Partially the idea with "legacy" was to be a bit
vague, so we could tweak what it really means later.

> If you could add a set of more generic options, which Xe could later tie 
> into that would work for me. For instance we have some more natural 
> cross-over points than Tigerlake. So if not per individual platform, 
> maybe for like ring buffer -> execlists -> GuC transitions. And naming 
> them without saying legacy for now, but use some descriptive names, and 
> listing platform code names in help text. "Select this to support 
> Broadwell, Skylake, etc..", "Select this to support Sandybridge..". Out 
> of the tree Xe build can then just not use the corresponding defines in 
> its own build and it would achieve what you need?

I kind of wanted to avoid adding a lot of config options, because I
think they'll be difficult to maintain and get all the combos right. I
don't particularly want all the build bot reports about various kconfig
combos failing.

One other problem is that I can't think of a way to do this by using the
kconfig CONFIG_FOO macros directly; you have to add separate variables
because the same files are built for two drivers. You can't force the
CONFIG_FOO macros to different settings for different drivers. So we'd
get a lot of proxy macros too.

I wonder if there's a name we could use instead of legacy to reasonably
match what Xe needs to avoid adding tons of configs at once?

BR,
Jani.


>
> Once in tree we can have a "legacy" kconfig which toggles a whole group 
> of those. Like "CONFIG_DRM_I915_SUPPORT_XE_PLATFORMS" or something.




-- 
Jani Nikula, Intel Open Source Graphics Center