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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 11 May 2023, "Souza, Jose" <jose.souza@intel.com> wrote:
> 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?

Yeah.

There was a lot of conflation in i915 what the various "has display" and
"disable display" configurations actually meant, and how they needed to
be implemented, and xe adds kconfig and this flag on top. Need to be
careful not to make a mess of it!

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

-- 
Jani Nikula, Intel Open Source Graphics Center