On Tue, 16 Jan 2024, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > There's no reason the caller of intel_initial_plane_config() should > have to loop over the CRTCs. Pull the loop into the function to > make life simpler for the caller. > > Cc: Paz Zcharya <pazz@xxxxxxxxxxxx> > Reviewed-by: Andrzej Hajda <andrzej.hajda@xxxxxxxxx> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > --- > .../drm/i915/display/intel_display_driver.c | 7 +--- > .../drm/i915/display/intel_plane_initial.c | 40 +++++++++++-------- > .../drm/i915/display/intel_plane_initial.h | 4 +- > 3 files changed, 26 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c > index ecf9cb74734b..f3fe1743243b 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_driver.c > +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c > @@ -415,7 +415,6 @@ int intel_display_driver_probe_nogem(struct drm_i915_private *i915) > { > struct drm_device *dev = &i915->drm; > enum pipe pipe; > - struct intel_crtc *crtc; > int ret; > > if (!HAS_DISPLAY(i915)) > @@ -467,11 +466,7 @@ int intel_display_driver_probe_nogem(struct drm_i915_private *i915) > intel_acpi_assign_connector_fwnodes(i915); > drm_modeset_unlock_all(dev); > > - for_each_intel_crtc(dev, crtc) { > - if (!to_intel_crtc_state(crtc->base.state)->uapi.active) > - continue; > - intel_crtc_initial_plane_config(crtc); > - } > + intel_initial_plane_config(i915); > > /* > * Make sure hardware watermarks really match the state we read out. > diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c > index 78bff6181ceb..b7e12b60d68b 100644 > --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c > +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c > @@ -357,25 +357,31 @@ static void plane_config_fini(struct intel_initial_plane_config *plane_config) > i915_vma_put(plane_config->vma); > } > > -void intel_crtc_initial_plane_config(struct intel_crtc *crtc) > +void intel_initial_plane_config(struct drm_i915_private *i915) So this fails to build on CONFIG_DRM_XE=m, because it has its own version of intel_plane_initial.c which has intel_crtc_initial_plane_config(), but not intel_initial_plane_config(). You'll get this as the first indication: CC [M] drivers/gpu/drm/xe/display/xe_plane_initial.o ../drivers/gpu/drm/xe/display/xe_plane_initial.c:270:6: error: no previous prototype for ‘intel_crtc_initial_plane_config’ [-Werror=missing-prototypes] 270 | void intel_crtc_initial_plane_config(struct intel_crtc *crtc) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ but if you bypass that, eventually: MODPOST Module.symvers ERROR: modpost: "intel_initial_plane_config" [drivers/gpu/drm/xe/xe.ko] undefined! Needs to be fixed before merging. BR, Jani. > { > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > - struct intel_initial_plane_config plane_config = {}; > + struct intel_crtc *crtc; > > - /* > - * Note that reserving the BIOS fb up front prevents us > - * from stuffing other stolen allocations like the ring > - * on top. This prevents some ugliness at boot time, and > - * can even allow for smooth boot transitions if the BIOS > - * fb is large enough for the active pipe configuration. > - */ > - dev_priv->display.funcs.display->get_initial_plane_config(crtc, &plane_config); > + for_each_intel_crtc(&i915->drm, crtc) { > + struct intel_initial_plane_config plane_config = {}; > > - /* > - * If the fb is shared between multiple heads, we'll > - * just get the first one. > - */ > - intel_find_initial_plane_obj(crtc, &plane_config); > + if (!to_intel_crtc_state(crtc->base.state)->uapi.active) > + continue; > > - plane_config_fini(&plane_config); > + /* > + * Note that reserving the BIOS fb up front prevents us > + * from stuffing other stolen allocations like the ring > + * on top. This prevents some ugliness at boot time, and > + * can even allow for smooth boot transitions if the BIOS > + * fb is large enough for the active pipe configuration. > + */ > + i915->display.funcs.display->get_initial_plane_config(crtc, &plane_config); > + > + /* > + * If the fb is shared between multiple heads, we'll > + * just get the first one. > + */ > + intel_find_initial_plane_obj(crtc, &plane_config); > + > + plane_config_fini(&plane_config); > + } > } > diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.h b/drivers/gpu/drm/i915/display/intel_plane_initial.h > index c7e35ab3182b..64ab95239cd4 100644 > --- a/drivers/gpu/drm/i915/display/intel_plane_initial.h > +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.h > @@ -6,8 +6,8 @@ > #ifndef __INTEL_PLANE_INITIAL_H__ > #define __INTEL_PLANE_INITIAL_H__ > > -struct intel_crtc; > +struct drm_i915_private; > > -void intel_crtc_initial_plane_config(struct intel_crtc *crtc); > +void intel_initial_plane_config(struct drm_i915_private *i915); > > #endif -- Jani Nikula, Intel