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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Mar 10, 2023 at 03:54:24PM +0200, Jani Nikula wrote:
> 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?

It would make the system load xe by default for those devices.

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

-- 
Ville Syrjälä
Intel