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

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

On Mon, 11 Sep 2023, Balasubramani Vivekanandan <balasubramani.vivekanandan@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.


BR,
Jani.


>
> Signed-off-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@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