Re: [PATCH] drm/xe/display: Use a single early init call for display

[Maarten Lankhorst <dev@xxxxxxxxxxxx>]

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()

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.

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;