[Intel-xe] [PATCH v3] drm/xe/display: Print display ip version

[Lucas De Marchi]

On Mon, Sep 11, 2023 at 02:04:30PM +0300, Jani Nikula wrote:
>On Mon, 11 Sep 2023, Balasubramani Vivekanandan <balasubramani.vivekanandan at intel.com> wrote:
>> Print display ip version and flags during module load
>>
>> v3:
>> Use the existing intel_display_device_info_print() function to print the
>> display information. (Jani)
>
>So this is an improvement in the sense that xe core doesn't poke at
>display data directly to print the info.
>
>However, I still think the interface between xe (or i915) core and
>display should be minimal. I don't see why we'd need to add a function
>at the top level probe to print display stuff... the display code should
>do it internally, in some other high level probe call, when it's done.
>
>Even having the function adds complexity, because the call site now
>needs to be aware about display probe order, and ensure it's all done
>before you can safely and accurately print display info. And that's
>display code implementation details.
>
>Superficially this is all benign, but this stuff adds up. We've been
>trying to untangle i915 core and display for a long time, and the i915
>probe is still a huge mess, with a bunch of random calls to display at
>random times, and there seems to be no end to this. It's all so
>intertwined.
>
>Perhaps in the future all of the calls need to go through a framework,
>maybe aux bus. Do we really want to put all of this to that interface?
>
>So the direction should be to reduce and minimize the interfaces between
>the high level components, not add more.

Agreed. I think what is missing here is: what would be a good point
inside display to call this on. Is it ok in the end of
intel_display_driver_probe()? Are we ok with display info now showing
up before than the rest for i915 or should we also move
i915_welcome_messages() to be before intel_display_driver_probe()?

Lucas De Marchi

>
>
>BR,
>Jani.
>
>
>>
>> Signed-off-by: Balasubramani Vivekanandan <balasubramani.vivekanandan at intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_display.c | 9 +++++++++
>>  drivers/gpu/drm/xe/xe_display.h | 4 ++++
>>  drivers/gpu/drm/xe/xe_pci.c     | 2 ++
>>  3 files changed, 15 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_display.c b/drivers/gpu/drm/xe/xe_display.c
>> index a453946ad108..45ffc418e636 100644
>> --- a/drivers/gpu/drm/xe/xe_display.c
>> +++ b/drivers/gpu/drm/xe/xe_display.c
>> @@ -417,6 +417,15 @@ void xe_display_pm_resume(struct xe_device *xe)
>>  	intel_power_domains_enable(xe);
>>  }
>>
>> +void xe_display_info_print(struct xe_device *xe)
>> +{
>> +	struct drm_printer p = drm_info_printer(xe->drm.dev);
>> +
>> +	if (xe->info.enable_display)
>> +		intel_display_device_info_print(xe->info.display,
>> +						&xe->info.display_runtime, &p);
>> +}
>> +
>>  /* Display info initialization */
>>  __diag_push();
>>  __diag_ignore_all("-Woverride-init", "Allow field overrides in table");
>> diff --git a/drivers/gpu/drm/xe/xe_display.h b/drivers/gpu/drm/xe/xe_display.h
>> index 9e29de012df7..b18bf5583229 100644
>> --- a/drivers/gpu/drm/xe/xe_display.h
>> +++ b/drivers/gpu/drm/xe/xe_display.h
>> @@ -19,6 +19,8 @@ int xe_display_create(struct xe_device *xe);
>>
>>  void xe_display_info_init(struct xe_device *xe);
>>
>> +void xe_display_info_print(struct xe_device *xe);
>> +
>>  int xe_display_init_nommio(struct xe_device *xe);
>>  void xe_display_fini_nommio(struct drm_device *dev, void *dummy);
>>
>> @@ -57,6 +59,8 @@ xe_display_create(struct xe_device *xe) { return 0; }
>>
>>  static inline void xe_display_info_init(struct xe_device *xe) { }
>>
>> +static inline void xe_display_info_print(struct xe_device *xe) { }
>> +
>>  static inline int
>>  xe_display_enable(struct pci_dev *pdev, struct drm_driver *driver) { return 0; }
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>> index 24b16863bf3d..a4886ea8794f 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.c
>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>> @@ -724,6 +724,8 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  		xe_step_name(xe->info.step.display),
>>  		xe_step_name(xe->info.step.basedie));
>>
>> +	xe_display_info_print(xe);
>> +
>>  	err = xe_device_probe(xe);
>>  	if (err)
>>  		goto err_pci_disable;
>
>-- 
>Jani Nikula, Intel Open Source Graphics Center