On Wed, Dec 11, 2024 at 07:32:08PM +0100, Maarten Lankhorst wrote:
> Hey Jani,
>
> I believe at least for the platforms xe cares about (gen12+), display is
> sufficiently separated that everything can be performed in a single init
> call before we enable interrupts.
>
> Because of the strict separation between xe and display, it should be fine
> to keep the ordering as-is from this patch.
>
> (some xe init here)
> xe_display_init_nommio()
> (some xe init here)
> xe_display_init_noirq()
> (some xe init here)
> xe_display_init_noaccel()
> (some xe init here)
> irq_enable()
>
> should functionally be the same as
>
> (some xe init here)
> xe_display_init_early()
> (some xe init here)
> irq_enable()
hmm, but also I don't see a problem of this:
xe_display_init_nommio()
some xe init here)
xe_display_init_noirq()
xe_display_init_noaccel()
(some xe init here)
>
> When you look at it from the xe driver point of view.
> Nothing else in xe depends on display, and interrupts are not enabled until
> after this init call.
>
> We must deviate from i915 with interrupts, because enabling interrupts may
> require memirqs, which performs a GGTT allocation.
We will eventually have to deal with the IRQ handling in a more unified way.
But for now, it could be 2 sequences in the call like that.
My goal is to make xe_display.c only pure wrapper for checking the xe
module parameter for display and calling the exact i915 function.
I understand that we might end up with some difference sequences, but the
difference sequence needs to live in i915/display and the xe_display.c
be a pure wrapper. That's the only way that we will be able to get to
a fully separate display at some point.
If we continue to add deviations we will delay that day even further...
>
> The long explanation might be too long to stuff into the commit message, but
> I hope it makes sense. :-)
>
> Cheers,
> ~Maarten
>
> Den 2024-12-10 kl. 10:35, skrev Jani Nikula:
> > On Mon, 09 Dec 2024, Maarten Lankhorst <dev@xxxxxxxxxxxx> wrote:
> > > Instead of 3 different calls, it should be safe to unify to a single
> > > call now. This makes the init sequence cleaner, and display less
> > > tangled.
> >
> > Needs more explanation.
> >
> > I thought the goal was to *unify* i915 and xe display init/cleanup. This
> > diverges them more, with actually functionally different things rather
> > than just slightly different ordering.
> >
> > BR,
> > Jani.
> >
> >
> > >
> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > > Signed-off-by: Maarten Lankhorst <dev@xxxxxxxxxxxx>
> > > ---
> > > Rebase
> > >
> > > drivers/gpu/drm/xe/display/xe_display.c | 73 +++++++------------------
> > > drivers/gpu/drm/xe/display/xe_display.h | 8 +--
> > > drivers/gpu/drm/xe/xe_device.c | 10 +---
> > > 3 files changed, 22 insertions(+), 69 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> > > index 317fa66adf189..b013a4db11183 100644
> > > --- a/drivers/gpu/drm/xe/display/xe_display.c
> > > +++ b/drivers/gpu/drm/xe/display/xe_display.c
> > > @@ -101,19 +101,25 @@ int xe_display_create(struct xe_device *xe)
> > > return drmm_add_action_or_reset(&xe->drm, display_destroy, NULL);
> > > }
> > > -static void xe_display_fini_nommio(struct drm_device *dev, void *dummy)
> > > +static void xe_display_fini_early(void *arg)
> > > {
> > > - struct xe_device *xe = to_xe_device(dev);
> > > + struct xe_device *xe = arg;
> > > struct intel_display *display = &xe->display;
> > > if (!xe->info.probe_display)
> > > return;
> > > + intel_display_driver_remove_nogem(display);
> > > + intel_display_driver_remove_noirq(display);
> > > + intel_opregion_cleanup(display);
> > > intel_power_domains_cleanup(display);
> > > }
> > > -int xe_display_init_nommio(struct xe_device *xe)
> > > +int xe_display_init_early(struct xe_device *xe)
> > > {
> > > + struct intel_display *display = &xe->display;
> > > + int err;
> > > +
> > > if (!xe->info.probe_display)
> > > return 0;
> > > @@ -123,29 +129,6 @@ int xe_display_init_nommio(struct xe_device *xe)
> > > /* This must be called before any calls to HAS_PCH_* */
> > > intel_detect_pch(xe);
> > > - return drmm_add_action_or_reset(&xe->drm, xe_display_fini_nommio, xe);
> > > -}
> > > -
> > > -static void xe_display_fini_noirq(void *arg)
> > > -{
> > > - struct xe_device *xe = arg;
> > > - struct intel_display *display = &xe->display;
> > > -
> > > - if (!xe->info.probe_display)
> > > - return;
> > > -
> > > - intel_display_driver_remove_noirq(display);
> > > - intel_opregion_cleanup(display);
> > > -}
> > > -
> > > -int xe_display_init_noirq(struct xe_device *xe)
> > > -{
> > > - struct intel_display *display = &xe->display;
> > > - int err;
> > > -
> > > - if (!xe->info.probe_display)
> > > - return 0;
> > > -
> > > intel_display_driver_early_probe(display);
> > > /* Early display init.. */
> > > @@ -162,38 +145,20 @@ int xe_display_init_noirq(struct xe_device *xe)
> > > intel_display_device_info_runtime_init(display);
> > > err = intel_display_driver_probe_noirq(display);
> > > - if (err) {
> > > - intel_opregion_cleanup(display);
> > > - return err;
> > > - }
> > > -
> > > - return devm_add_action_or_reset(xe->drm.dev, xe_display_fini_noirq, xe);
> > > -}
> > > -
> > > -static void xe_display_fini_noaccel(void *arg)
> > > -{
> > > - struct xe_device *xe = arg;
> > > - struct intel_display *display = &xe->display;
> > > -
> > > - if (!xe->info.probe_display)
> > > - return;
> > > -
> > > - intel_display_driver_remove_nogem(display);
> > > -}
> > > -
> > > -int xe_display_init_noaccel(struct xe_device *xe)
> > > -{
> > > - struct intel_display *display = &xe->display;
> > > - int err;
> > > -
> > > - if (!xe->info.probe_display)
> > > - return 0;
> > > + if (err)
> > > + goto err_opregion;
> > > err = intel_display_driver_probe_nogem(display);
> > > if (err)
> > > - return err;
> > > + goto err_noirq;
> > > - return devm_add_action_or_reset(xe->drm.dev, xe_display_fini_noaccel, xe);
> > > + return devm_add_action_or_reset(xe->drm.dev, xe_display_fini_early, xe);
> > > +err_noirq:
> > > + intel_display_driver_remove_noirq(display);
> > > + intel_power_domains_cleanup(display);
> > > +err_opregion:
> > > + intel_opregion_cleanup(display);
> > > + return err;
> > > }
> > > int xe_display_init(struct xe_device *xe)
> > > diff --git a/drivers/gpu/drm/xe/display/xe_display.h b/drivers/gpu/drm/xe/display/xe_display.h
> > > index 233f81a26c255..e2a99624f7064 100644
> > > --- a/drivers/gpu/drm/xe/display/xe_display.h
> > > +++ b/drivers/gpu/drm/xe/display/xe_display.h
> > > @@ -20,9 +20,7 @@ int xe_display_create(struct xe_device *xe);
> > > int xe_display_probe(struct xe_device *xe);
> > > -int xe_display_init_nommio(struct xe_device *xe);
> > > -int xe_display_init_noirq(struct xe_device *xe);
> > > -int xe_display_init_noaccel(struct xe_device *xe);
> > > +int xe_display_init_early(struct xe_device *xe);
> > > int xe_display_init(struct xe_device *xe);
> > > void xe_display_fini(struct xe_device *xe);
> > > @@ -54,9 +52,7 @@ static inline int xe_display_create(struct xe_device *xe) { return 0; }
> > > static inline int xe_display_probe(struct xe_device *xe) { return 0; }
> > > -static inline int xe_display_init_nommio(struct xe_device *xe) { return 0; }
> > > -static inline int xe_display_init_noirq(struct xe_device *xe) { return 0; }
> > > -static inline int xe_display_init_noaccel(struct xe_device *xe) { return 0; }
> > > +static inline int xe_display_init_early(struct xe_device *xe) { return 0; }
> > > static inline int xe_display_init(struct xe_device *xe) { return 0; }
> > > static inline void xe_display_fini(struct xe_device *xe) {}
> > > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> > > index fbec176ee64ad..c9c0b74c74ddb 100644
> > > --- a/drivers/gpu/drm/xe/xe_device.c
> > > +++ b/drivers/gpu/drm/xe/xe_device.c
> > > @@ -639,10 +639,6 @@ int xe_device_probe(struct xe_device *xe)
> > > return err;
> > > xe->info.mem_region_mask = 1;
> > > - err = xe_display_init_nommio(xe);
> > > - if (err)
> > > - return err;
> > > -
> > > err = xe_set_dma_info(xe);
> > > if (err)
> > > return err;
> > > @@ -697,10 +693,6 @@ int xe_device_probe(struct xe_device *xe)
> > > if (err)
> > > return err;
> > > - err = xe_display_init_noirq(xe);
> > > - if (err)
> > > - return err;
> > > -
> > > err = probe_has_flat_ccs(xe);
> > > if (err)
> > > goto err;
> > > @@ -724,7 +716,7 @@ int xe_device_probe(struct xe_device *xe)
> > > * This is the reason the first allocation needs to be done
> > > * inside display.
> > > */
> > > - err = xe_display_init_noaccel(xe);
> > > + err = xe_display_init_early(xe);
> > > if (err)
> > > goto err;
> >
>
