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 <[email protected]> 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 <[email protected]> > > > Signed-off-by: Maarten Lankhorst <[email protected]> > > > --- > > > 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; > > >
