| Message ID | 20241107100140.292928-5-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/9] drm/xe: Remove double pageflip | expand |
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@linux.intel.com> > Link: https://patchwork.freedesktop.org/patch/msgid/20241105121857.17389-5-maarten.lankhorst@linux.intel.com > Signed-off-by: Maarten Lankhorst,,, <dev@lankhorst.se> > --- > 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 >
Hey, Den 2024-11-12 kl. 19:39, skrev Rodrigo Vivi: > 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@linux.intel.com> >> Link: https://patchwork.freedesktop.org/patch/msgid/20241105121857.17389-5-maarten.lankhorst@linux.intel.com >> Signed-off-by: Maarten Lankhorst,,, <dev@lankhorst.se> >> --- >> 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... > It should not matter much. I believe i915 had more split because of display being more connected to hardware. In Xe the display is from the start a separate driver or module on the chip , so we can do the different init calls in one go, as there are no interrupts calling into the display driver yet. So in Xe we can do a 2-stage init of display, xe_display_init_early which initialises everything required before enabling interrupts, completed with the takeover of the initial FB, and xe_display_init() for the normal init sequence that has to run afterwards. Because of memirq's performing allocations by design, we really have to run the FB handover code without interrupts anyway. I believe the same thing could be done on i915 on BDW+, but might be harder on legacy platforms. Cheers, ~Maarten
On Wed, Nov 13, 2024 at 11:30:36AM +0100, Maarten Lankhorst wrote: > Hey, > > Den 2024-11-12 kl. 19:39, skrev Rodrigo Vivi: > > 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@linux.intel.com> > > > Link: https://patchwork.freedesktop.org/patch/msgid/20241105121857.17389-5-maarten.lankhorst@linux.intel.com > > > Signed-off-by: Maarten Lankhorst,,, <dev@lankhorst.se> > > > --- > > > 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... > > > It should not matter much. I believe i915 had more split because of display > being more connected to hardware. In Xe the display is from the start a > separate driver or module on the chip , so we can do the different init > calls in one go, as there are no interrupts > calling into the display driver yet. > > So in Xe we can do a 2-stage init of display, xe_display_init_early which > initialises everything required before enabling interrupts, completed with > the takeover of the initial FB, and xe_display_init() for the normal init > sequence that has to run afterwards. > > Because of memirq's performing allocations by design, we really have to run > the FB handover code without interrupts anyway. > > I believe the same thing could be done on i915 on BDW+, but might be harder > on legacy platforms. Okay... it shouldn't matter much indeed. Fix your signed off by and feel free to use Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cheers, > ~Maarten
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 @@ -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;