Re: [PATCH 1/2] drm/i915: Enable/disable shared dplls just the once for joined pipes

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 10 Mar 2025, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Currently we loop over all joined pipes and enable/disable the
> shared dplls for each. We don't really have to do that since
> all joined pipes will be using the same dpll. So let's just do
> the enable/disable once for the whole set of joined pipes.
> We can still keep tracking the dpll active set as pipes as long
> as we remember to flip the bits for all the joined pipes on one go.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c  | 16 +++-------------
>  drivers/gpu/drm/i915/display/intel_dpll_mgr.c |  4 ++--
>  2 files changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 3afb85fe8536..0e0e5285ad97 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1658,17 +1658,12 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>  	for_each_pipe_crtc_modeset_enable(display, pipe_crtc, new_crtc_state, i)
>  		intel_dmc_enable_pipe(display, pipe_crtc->pipe);
>  
>  	intel_encoders_pre_pll_enable(state, crtc);
>  
> -	for_each_pipe_crtc_modeset_enable(display, pipe_crtc, new_crtc_state, i) {
> -		const struct intel_crtc_state *pipe_crtc_state =
> -			intel_atomic_get_new_crtc_state(state, pipe_crtc);
> -
> -		if (pipe_crtc_state->shared_dpll)
> -			intel_enable_shared_dpll(pipe_crtc_state);
> -	}
> +	if (new_crtc_state->shared_dpll)
> +		intel_enable_shared_dpll(new_crtc_state);

Seems like the check for new_crtc_state->shared_dpll could also be moved
inside intel_enable_shared_dpll(), in a separate patch.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>  
>  	intel_encoders_pre_enable(state, crtc);
>  
>  	for_each_pipe_crtc_modeset_enable(display, pipe_crtc, new_crtc_state, i) {
>  		const struct intel_crtc_state *pipe_crtc_state =
> @@ -1795,16 +1790,11 @@ static void hsw_crtc_disable(struct intel_atomic_state *state,
>  	 * Need care with mst->ddi interactions.
>  	 */
>  	intel_encoders_disable(state, crtc);
>  	intel_encoders_post_disable(state, crtc);
>  
> -	for_each_pipe_crtc_modeset_disable(display, pipe_crtc, old_crtc_state, i) {
> -		const struct intel_crtc_state *old_pipe_crtc_state =
> -			intel_atomic_get_old_crtc_state(state, pipe_crtc);
> -
> -		intel_disable_shared_dpll(old_pipe_crtc_state);
> -	}
> +	intel_disable_shared_dpll(old_crtc_state);
>  
>  	intel_encoders_post_pll_disable(state, crtc);
>  
>  	for_each_pipe_crtc_modeset_disable(display, pipe_crtc, old_crtc_state, i)
>  		intel_dmc_disable_pipe(display, pipe_crtc->pipe);
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> index c825a507b905..c7b73cd4bc67 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -255,11 +255,11 @@ static void _intel_disable_shared_dpll(struct intel_display *display,
>  void intel_enable_shared_dpll(const struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_display *display = to_intel_display(crtc_state);
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	struct intel_shared_dpll *pll = crtc_state->shared_dpll;
> -	unsigned int pipe_mask = BIT(crtc->pipe);
> +	unsigned int pipe_mask = intel_crtc_joined_pipe_mask(crtc_state);
>  	unsigned int old_mask;
>  
>  	if (drm_WARN_ON(display->drm, !pll))
>  		return;
>  
> @@ -301,11 +301,11 @@ void intel_enable_shared_dpll(const struct intel_crtc_state *crtc_state)
>  void intel_disable_shared_dpll(const struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_display *display = to_intel_display(crtc_state);
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	struct intel_shared_dpll *pll = crtc_state->shared_dpll;
> -	unsigned int pipe_mask = BIT(crtc->pipe);
> +	unsigned int pipe_mask = intel_crtc_joined_pipe_mask(crtc_state);
>  
>  	/* PCH only available on ILK+ */
>  	if (DISPLAY_VER(display) < 5)
>  		return;

-- 
Jani Nikula, Intel