Re: [PATCH] drm/i915/crtc: move crtc initialization loop to intel_crtc.c

["Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com>]

On 12/4/2025 8:02 PM, Jani Nikula wrote:
> intel_display_driver_probe_nogem() is too high of an abstraction level
> to be looping and initializing individual CRTCs. Move this to
> intel_crtc.c and repurpose intel_crtc_init() to initialize all
> CRTCs. Make the original a static __intel_crtc_init() for ininitializing
> a single CRTC.
> 

typo: s/ininitializing/initializing

Other than that, LGTM

Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>

> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> This is prep for doing [1] in a nicer way, without divulging the details
> at the high level.
> 
> [1] https://lore.kernel.org/r/20251119153321.2640969-1-jani.nikula@intel.com
> ---
>   drivers/gpu/drm/i915/display/intel_crtc.c     | 19 ++++++++++++++++++-
>   drivers/gpu/drm/i915/display/intel_crtc.h     |  2 +-
>   .../drm/i915/display/intel_display_driver.c   | 13 +++----------
>   3 files changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> index 5e1e02c8d9d4..778ebc5095c3 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> @@ -308,7 +308,7 @@ static const struct drm_crtc_funcs i8xx_crtc_funcs = {
>   	.get_vblank_timestamp = intel_crtc_get_vblank_timestamp,
>   };
>   
> -int intel_crtc_init(struct intel_display *display, enum pipe pipe)
> +static int __intel_crtc_init(struct intel_display *display, enum pipe pipe)
>   {
>   	struct intel_plane *primary, *cursor;
>   	const struct drm_crtc_funcs *funcs;
> @@ -406,6 +406,23 @@ int intel_crtc_init(struct intel_display *display, enum pipe pipe)
>   	return ret;
>   }
>   
> +int intel_crtc_init(struct intel_display *display)
> +{
> +	enum pipe pipe;
> +	int ret;
> +
> +	drm_dbg_kms(display->drm, "%d display pipe%s available.\n",
> +		    INTEL_NUM_PIPES(display), str_plural(INTEL_NUM_PIPES(display)));
> +
> +	for_each_pipe(display, pipe) {
> +		ret = __intel_crtc_init(display, pipe);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>   int intel_crtc_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void *data,
>   					   struct drm_file *file)
>   {
> diff --git a/drivers/gpu/drm/i915/display/intel_crtc.h b/drivers/gpu/drm/i915/display/intel_crtc.h
> index 07917e8a9ae3..12507b51ee77 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc.h
> +++ b/drivers/gpu/drm/i915/display/intel_crtc.h
> @@ -37,7 +37,7 @@ void intel_crtc_arm_vblank_event(struct intel_crtc_state *crtc_state);
>   void intel_crtc_prepare_vblank_event(struct intel_crtc_state *crtc_state,
>   				     struct drm_pending_vblank_event **event);
>   u32 intel_crtc_max_vblank_count(const struct intel_crtc_state *crtc_state);
> -int intel_crtc_init(struct intel_display *display, enum pipe pipe);
> +int intel_crtc_init(struct intel_display *display);
>   int intel_crtc_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void *data,
>   					   struct drm_file *file_priv);
>   struct intel_crtc_state *intel_crtc_state_alloc(struct intel_crtc *crtc);
> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
> index 7e000ba3e08b..e282b533d5b6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
> @@ -452,7 +452,6 @@ bool intel_display_driver_check_access(struct intel_display *display)
>   /* part #2: call after irq install, but before gem init */
>   int intel_display_driver_probe_nogem(struct intel_display *display)
>   {
> -	enum pipe pipe;
>   	int ret;
>   
>   	if (!HAS_DISPLAY(display))
> @@ -466,15 +465,9 @@ int intel_display_driver_probe_nogem(struct intel_display *display)
>   
>   	intel_gmbus_setup(display);
>   
> -	drm_dbg_kms(display->drm, "%d display pipe%s available.\n",
> -		    INTEL_NUM_PIPES(display),
> -		    INTEL_NUM_PIPES(display) > 1 ? "s" : "");
> -
> -	for_each_pipe(display, pipe) {
> -		ret = intel_crtc_init(display, pipe);
> -		if (ret)
> -			goto err_mode_config;
> -	}
> +	ret = intel_crtc_init(display);
> +	if (ret)
> +		goto err_mode_config;
>   
>   	intel_plane_possible_crtcs_init(display);
>   	intel_dpll_init(display);