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

[Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>]

On 11.09.2023 09:01, Lucas De Marchi wrote:
> On Mon, Sep 11, 2023 at 02:04:30PM +0300, Jani Nikula wrote:
> > 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.
> 
> 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

Hi Jani,
   can you please provide your opinion to the Lucas' query? I would send
a new revision based on the conclusion. Thanks.

Regards,
Bala
> 
> > 
> > 
> > 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