[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 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.

I know it's a bit of a Makefile hack to build the files for two
different drivers, but it avoids a lot of up front infrastructure that
would be difficult to coordinate as long as xe is out-of-tree.


BR,
Jani.



-- 
Jani Nikula, Intel Open Source Graphics Center