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

[Lucas De Marchi]

On Thu, May 11, 2023 at 11:02:20AM -0700, Lucas De Marchi wrote:
>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

wrong paste to this link, should had been
https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/265

Lucas De Marchi

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