[Intel-xe] [PATCH 3/4] drm/xe/display: Consider has_display to enable it

[Lucas De Marchi]

On Thu, May 11, 2023 at 05:22:34PM +0000, Jose Souza wrote:
>On Thu, 2023-05-11 at 10:12 -0700, Lucas De Marchi wrote:
>> On Thu, May 11, 2023 at 12:47:26PM +0000, Jose Souza wrote:
>> > On Wed, 2023-05-10 at 12:54 -0700, Lucas De Marchi wrote:
>> > > Stop the dance of enabling the display-related driver_features to later
>> > > disable them on display info init if the platform doesn't have display
>> > > IP. Besides being needless work, it wasn't covering the ATS-M case that
>> > > is the same platform as DG2, but without display.
>> >
>> > Xe should set pipe_mask = 0 and call i915 functions that will handle no display cases.
>>
>> in xe, enable_display is the runtime config to be the equivalent of
>> DRM_XE_DISPLAY=n. It is *not* to meant disabling the display.
>>
>> history why this ever came to be was:
>>
>> 1) display integration back in the day was less than ideal (still is),
>>     and developers couldn't test things ignoring display
>
>Xe CI is now testing display, in my opinion this option to disable display should be removed and display support always built.

maybe. Last time I checked there was a 50% divide on having or not
having it and afair I was against. I'd not touch that until display
integration settles and stop moving in the branch.

	95dd4dbb5f89 ("drm/xe: Make compilation of display optional")
	6a53b00afa0b ("drm/xe: Add big warning for !CONFIG_DRM_XE_DISPLAY path.")

Unfortunately I can't easily map to the MR where that was discussed.

+Maarten

Also, "Xe CI is now testing display" is not a good justification wrt ats-m.
Besides not not covering all ats-m (see
https://intel-gfx-ci.01.org/tree/intel-xe/bat-all.html?), it's actually
running display tests on the AST driver.  See
https://intel-gfx-ci.01.org/tree/intel-xe/bat-all.html?

>
>> 2) have a way to tell the driver "don't ever touch display IP" for
>>     bring-up situations.
>
>pipe_mask = 0/HAS_DISPLAY() should take care of it.
>developers could force pipe_mask = 0 for giving platform bring-up.

The hole is deeper than you think. See:
drivers/gpu/drm/xe/display/ext/intel_device_info.c

In i915, pipe_mask set the device info based on the PCI ID:

	static const struct intel_device_info ats_m_info = {
		....
	        NO_DISPLAY,
	};

	INTEL_ATS_M_IDS(&ats_m_info),

This is how i915 differentiates DG2 from ATS-M. Now look at xe with the
device info split in 2 places, xe_pci.c and xe_display.c.
We can't use the PCI ID mapping to know ATS-M is not DG2. We can't use
the subplatform neither, as they are already part of other subplatforms:

	static const u16 dg2_g10_ids[] = { XE_DG2_G10_IDS(NOP), XE_ATS_M150_IDS(NOP), 0 };
	static const u16 dg2_g11_ids[] = { XE_DG2_G11_IDS(NOP), XE_ATS_M75_IDS(NOP), 0 };

Changing that for the sake of display basically means having to treat
them differently throughout the driver, which is worse than simply
having a `has_display` flag added. When we eventually migrate to disable
display, the has_display flag in the xe's  device info can still stay
and how we tell xe_display.c that the platform has or not display (as
opposed to setting info.enable_display)


As I said above, I'm not really against the long term plan of removing
the kernel config. In that long term plan we'd not have a
enable_display= module param neither as we'd have already a replacement
for the global module configuration, allowing it to be done per device.
But I don't think it should block the fix for ats-m done here.

Lucas De Marchi

>
>>
>> For (1) we may turn that into "disable display" now, but not for (2).
>>
>> I'll take a look on how much work it would be to migrate to a
>> disable-display scenario rather than the simple "Fix ats-m" being done
>> here.
>>
>> Lucas De Marchi
>