Re: [PATCH v3 12/16] drm/i915: Simplify intel_initial_plane_config() calling convention

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Feb 02, 2024 at 05:14:37PM +0200, Jani Nikula wrote:
> On Tue, 16 Jan 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > 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@chromium.org>
> > Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  .../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