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

Fri Mar 10 10:11:26 UTC 2023

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.

> 2)
> Why is the user visible kconfig option needed?

To me, it's part of the justification, a feature we could have
upstream. I don't think I'm willing to accept #ifdefs for something that
can only be enabled in Makefiles. And we'll get build testing from bots
even if we don't actively do it ourselves.

>
> Regards,
>
> Tvrtko
>
> P.S. You could add compiling out code easily along the lines of 
> https://patchwork.freedesktop.org/patch/428511/?series=89069&rev=1 and 
> the corresponding series. Maybe like 
> s/IS_OPT_PLATFORM/IS_LEGACY_PLATTFORM. That does not work as well as 
> when GEN checks could also be bitmasks, but I suppose it should have 
> some effect still.

Yes, something like that would be the next steps. For now, I'm gauging
the approval for the approach, and depending on that we could take this
further.

Btw the compiler just throws out intel_i830_info etc. if it's not
referenced, so we don't have to add so much #ifdefs. And in any case I'm
looking at this more from an opportunistic angle: compile out anything
that's easy to rip out, and stuff that's needed to be removed for xe,
but don't even try to be 100% complete. For the most part, we don't even
need to add #ifdefs outside of display/ if we don't want.

BR,
Jani.

>
>> 
>> BR,
>> Jani.
>> ---
>>   drivers/gpu/drm/i915/Kconfig    | 11 +++++++++++
>>   drivers/gpu/drm/i915/Makefile   |  7 +++++++
>>   drivers/gpu/drm/i915/i915_pci.c |  2 ++
>>   3 files changed, 20 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
>> index 8eb3e60aeec9..a569c1606f51 100644
>> --- a/drivers/gpu/drm/i915/Kconfig
>> +++ b/drivers/gpu/drm/i915/Kconfig
>> @@ -53,6 +53,17 @@ config DRM_I915
>>   
>>   	  If "M" is selected, the module will be called i915.
>>   
>> +config DRM_I915_LEGACY
>> +	bool "Support legacy hardware in i915"
>> +	depends on DRM_I915
>> +	depends on EXPERT
>> +	default y
>> +	help
>> +	  Disable this option if you want the i915 driver to only support modern
>> +	  Intel Graphics, starting from Tigerlake.
>> +
>> +	  If in doubt, say "Y".
>> +
>>   config DRM_I915_FORCE_PROBE
>>   	string "Force probe i915 for selected Intel hardware IDs"
>>   	depends on DRM_I915
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index a6e7cd2185c2..653d43e5b534 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -23,6 +23,13 @@ subdir-ccflags-y += $(call cc-disable-warning, unused-but-set-variable)
>>   subdir-ccflags-y += $(call cc-disable-warning, frame-address)
>>   subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror
>>   
>> +# Legacy platform support.
>> +#
>> +# Note: Source code needs to check for I915_LEGACY instead of
>> +# CONFIG_DRM_I915_LEGACY to allow Xe driver build without legacy support
>> +# independent of the Kconfig setting.
>> +subdir-ccflags-$(CONFIG_DRM_I915_LEGACY) += -DI915_LEGACY=1
>> +
>>   # Fine grained warnings disable
>>   CFLAGS_i915_pci.o = $(call cc-disable-warning, override-init)
>>   CFLAGS_display/intel_fbdev.o = $(call cc-disable-warning, override-init)
>> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
>> index bc6fc268739d..9f421015d2bb 100644
>> --- a/drivers/gpu/drm/i915/i915_pci.c
>> +++ b/drivers/gpu/drm/i915/i915_pci.c
>> @@ -1162,6 +1162,7 @@ static const struct intel_device_info mtl_info = {
>>    * PCI ID matches, otherwise we'll use the wrong info struct above.
>>    */
>>   static const struct pci_device_id pciidlist[] = {
>> +#if IS_ENABLED(I915_LEGACY)
>>   	INTEL_I830_IDS(&i830_info),
>>   	INTEL_I845G_IDS(&i845g_info),
>>   	INTEL_I85X_IDS(&i85x_info),
>> @@ -1225,6 +1226,7 @@ static const struct pci_device_id pciidlist[] = {
>>   	INTEL_ICL_11_IDS(&icl_info),
>>   	INTEL_EHL_IDS(&ehl_info),
>>   	INTEL_JSL_IDS(&jsl_info),
>> +#endif
>>   	INTEL_TGL_12_IDS(&tgl_info),
>>   	INTEL_RKL_IDS(&rkl_info),
>>   	INTEL_ADLS_IDS(&adl_s_info),

-- 
Jani Nikula, Intel Open Source Graphics Center