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

[Balasubramani Vivekanandan]

On 14.09.2023 17:35, Jani Nikula wrote:
> On Wed, 13 Sep 2023, Balasubramani Vivekanandan <balasubramani.vivekanandan at intel.com> wrote:
> > 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 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()?
> 
> I think just move the intel_display_device_info_print() call at the end
> of intel_display_driver_register(). That's the last thing called in
> display inits.

Created the following new patch for i915 moving
intel_display_device_info_print() inside
intel_display_driver_register().

https://patchwork.freedesktop.org/series/124032/

Regards,
Bala

> 
> Except, whoa, nobody seems to be calling intel_display_driver_register()
> in xe. That needs to be fixed!
> 
> We can tweak the order later, I think.
> 
> 
> BR,
> Jani.
> 
> 
> 
> >> 
> >> 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 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
> 
> -- 
> Jani Nikula, Intel