On Thu, Nov 07, 2024 at 11:01:36AM +0100, Maarten Lankhorst 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. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > Link: https://patchwork.freedesktop.org/patch/msgid/20241105121857.17389-5-maarten.lankhorst@xxxxxxxxxxxxxxx > Signed-off-by: Maarten Lankhorst,,, <dev@xxxxxxxxxxxx> > --- > drivers/gpu/drm/xe/display/xe_display.c | 72 +++++++------------------ > drivers/gpu/drm/xe/display/xe_display.h | 8 +-- > drivers/gpu/drm/xe/xe_device.c | 10 +--- > 3 files changed, 23 insertions(+), 67 deletions(-) > > diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c > index b5502f335f531..a9ce4f561e7aa 100644 > --- a/drivers/gpu/drm/xe/display/xe_display.c > +++ b/drivers/gpu/drm/xe/display/xe_display.c we should make every function in this file only a wrap to the i915 side with the if (!xe->info.probe_display) being the only thing extra in here... so we consolidate the display side in a way that we could later really split the display to a separate driver. I have the feeling that this patch takes display to the other direction... > @@ -100,31 +100,7 @@ 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) > -{ > - struct xe_device *xe = to_xe_device(dev); > - > - if (!xe->info.probe_display) > - return; > - > - intel_power_domains_cleanup(xe); > -} > - > -int xe_display_init_nommio(struct xe_device *xe) > -{ > - if (!xe->info.probe_display) > - return 0; > - > - /* Fake uncore lock */ > - spin_lock_init(&xe->uncore.lock); > - > - /* 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) > +static void xe_display_fini_early(void *arg) > { > struct xe_device *xe = arg; > struct intel_display *display = &xe->display; > @@ -132,11 +108,13 @@ static void xe_display_fini_noirq(void *arg) > if (!xe->info.probe_display) > return; > > + intel_display_driver_remove_nogem(xe); > intel_display_driver_remove_noirq(xe); > intel_opregion_cleanup(display); > + intel_power_domains_cleanup(xe); > } > > -int xe_display_init_noirq(struct xe_device *xe) > +int xe_display_init_early(struct xe_device *xe) > { > struct intel_display *display = &xe->display; > int err; > @@ -144,6 +122,12 @@ int xe_display_init_noirq(struct xe_device *xe) > if (!xe->info.probe_display) > return 0; > > + /* Fake uncore lock */ > + spin_lock_init(&xe->uncore.lock); > + > + /* This must be called before any calls to HAS_PCH_* */ > + intel_detect_pch(xe); > + > intel_display_driver_early_probe(xe); > > /* Early display init.. */ > @@ -160,36 +144,20 @@ int xe_display_init_noirq(struct xe_device *xe) > intel_display_device_info_runtime_init(xe); > > err = intel_display_driver_probe_noirq(xe); > - 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; > - > - if (!xe->info.probe_display) > - return; > - > - intel_display_driver_remove_nogem(xe); > -} > - > -int xe_display_init_noaccel(struct xe_device *xe) > -{ > - int err; > - > - if (!xe->info.probe_display) > - return 0; > + if (err) > + goto err_opregion; > > err = intel_display_driver_probe_nogem(xe); > 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(xe); > + intel_power_domains_cleanup(xe); > +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 17afa537aee50..255cffa328160 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); > > @@ -53,9 +51,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 b9948b2dc8d1d..f1147ebeeff31 100644 > --- a/drivers/gpu/drm/xe/xe_device.c > +++ b/drivers/gpu/drm/xe/xe_device.c > @@ -632,10 +632,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; > @@ -687,10 +683,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; > @@ -718,7 +710,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; > > -- > 2.45.2 >
