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

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

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

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@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