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 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.
> Also intel_device_info_runtime_init() needs to be called earlier, as pipes could be fused off and there is a case for discrete platforms with display
>>= 14 that can have display fused off too.

Agreed, seems like this duplicates HAS_DISPLAY() which operates on
pipe_mask, and takes fusing into account.

BR,
Jani.

>
>> 
>> Since now the display is created after the complete device info is
>> populated, the new has_display flag can be consider to enable it.
>> 
>> References: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/265
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_device.c  | 1 -
>>  drivers/gpu/drm/xe/xe_display.c | 6 ++++--
>>  drivers/gpu/drm/xe/xe_pci.c     | 6 +++++-
>>  3 files changed, 9 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>> index 32cc83c43b2a..1daacc533083 100644
>> --- a/drivers/gpu/drm/xe/xe_device.c
>> +++ b/drivers/gpu/drm/xe/xe_device.c
>> @@ -192,7 +192,6 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
>>  	xe->info.devid = pdev->device;
>>  	xe->info.revid = pdev->revision;
>>  	xe->info.enable_guc = enable_guc;
>> -	xe->info.enable_display = enable_display;
>>  
>>  	spin_lock_init(&xe->irq.lock);
>>  
>> diff --git a/drivers/gpu/drm/xe/xe_display.c b/drivers/gpu/drm/xe/xe_display.c
>> index 77a158ecf4cb..43c5af1ff80e 100644
>> --- a/drivers/gpu/drm/xe/xe_display.c
>> +++ b/drivers/gpu/drm/xe/xe_display.c
>> @@ -487,8 +487,10 @@ __diag_ignore_all("-Woverride-init", "Allow field overrides in table");
>>  
>>  void xe_display_info_init(struct xe_device *xe)
>>  {
>> -	if (!xe->info.enable_display)
>> +	if (!xe->info.enable_display) {
>> +		unset_driver_hooks(xe);
>>  		return;
>> +	}
>>  
>>  	switch (xe->info.platform) {
>>  	case XE_TIGERLAKE:
>> @@ -535,7 +537,7 @@ void xe_display_info_init(struct xe_device *xe)
>>  		xe->info.display = (struct xe_device_display_info) { XE_LPDP };
>>  		break;
>>  	default:
>> -		drm_dbg(&xe->drm, "No display IP, skipping\n");
>> +		drm_warn(&xe->drm, "Unknown display IP\n");
>>  		xe->info.enable_display = false;
>>  		unset_driver_hooks(xe);
>>  		return;
>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>> index 7dfbc4fa138a..3fa3e8706d98 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.c
>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>> @@ -529,6 +529,9 @@ static int xe_info_init(struct xe_device *xe,
>>  	xe->info.has_range_tlb_invalidation = graphics_desc->has_range_tlb_invalidation;
>>  	xe->info.has_link_copy_engine = graphics_desc->has_link_copy_engine;
>>  
>> +	xe->info.enable_display = IS_ENABLED(CONFIG_DRM_DISPLAY) && \
>> +				  enable_display && \
>> +				  desc->has_display;
>>  	/*
>>  	 * All platforms have at least one primary GT.  Any platform with media
>>  	 * version 13 or higher has an additional dedicated media GT.  And
>> @@ -629,7 +632,7 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  
>>  	xe_display_info_init(xe);
>>  
>> -	drm_dbg(&xe->drm, "%s %s %04x:%04x dgfx:%d gfx:%s (%d.%02d) media:%s (%d.%02d) dma_m_s:%d tc:%d",
>> +	drm_dbg(&xe->drm, "%s %s %04x:%04x dgfx:%d gfx:%s (%d.%02d) media:%s (%d.%02d) display:%s dma_m_s:%d tc:%d",
>>  		desc->platform_name,
>>  		subplatform_desc ? subplatform_desc->name : "",
>>  		xe->info.devid, xe->info.revid,
>> @@ -640,6 +643,7 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  		xe->info.media_name,
>>  		xe->info.media_verx100 / 100,
>>  		xe->info.media_verx100 % 100,
>> +		str_yes_no(xe->info.enable_display),
>>  		xe->info.dma_mask_size, xe->info.tile_count);
>>  
>>  	drm_dbg(&xe->drm, "Stepping = (G:%s, M:%s, D:%s, B:%s)\n",
>

-- 
Jani Nikula, Intel Open Source Graphics Center