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

[Souza, Jose]

On Thu, 2023-05-11 at 11:02 -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
> 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.

As short term solution ack on this patch from me.
Do you agree too Jani?

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