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 17:22, Lucas De Marchi wrote:
>> On Tue, Mar 14, 2023 at 06:42:56PM +0200, Jani Nikula wrote:
>>> 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.
>
> Hm these should not be tricky to that extent to cause any combinatorial 
> issues. Famous last words? :)

Let's say you have some feature foobar that's required on platforms foo
and bar.

At the Makefile level a single option is simple:

i915-$(CONFIG_FOO) += foo.o

But foobar.o that's needed by both CONFIG_FOO and CONFIG_BAR already
gets tedious:

ifneq ($(CONFIG_FOO)$(CONFIG_BAR),nn)
	i915-y += foobar.o
endif

Similarly in code:

#if IS_ENABLED(CONFIG_FOO) || IS_ENABLED(CONFIG_BAR)

I think it's easy to mess up the combos when you add a lot of them. And
it'll look seriously ugly too, just with the two above! And kind of
defeats the purpose of "not having those ugly #ifdef I915" in the
driver. :/

>>> 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?
>> 
>> considering TGL itself is also "xe" arch (not to be confused with xe,
>> the kernel module), maybe CONFIG_DRM_I915_PRE_XE?
>
> Makes sense as a name I think. But why is the tricky question? :) Why do 
> Xe+ users deserve the privilege of being able to compile out the old 
> cruft but users of old machines cannot compile out Xe+? In other words 
> how to make the thing attractive for not just Xe (the driver).
>
> Mind you I am not really opposed, as long as it will be made in a 
> flexible way. Just would like to explore the option of making it more 
> widely useful.

Mmh, I just always thought there was no return on the investment. Too
few people need it to take on the burden?


BR,
Jani.

>
> Regards,
>
> Tvrtko

-- 
Jani Nikula, Intel Open Source Graphics Center