On Fri, Feb 02, 2024 at 05:14:37PM +0200, Jani Nikula wrote:
> 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.
Can't see anything off in the CI results. Are we not at
least build testing xe in i915 CI?
>
> 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
--
Ville Syrjälä
Intel
