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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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@lankhorst.se> 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@intel.com>
> > > Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
> > > ---
> > > 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;
> > 
>