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

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

On Fri, 10 Mar 2023, Ville Syrjälä <ville.syrjala@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@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@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.
>> 
>> 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?

> 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