Re: [PATCH 2/2] drm/i915: kill intel_ddi_pll_select()

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

On Wed, Mar 22, 2017 at 03:58:45PM -0300, Paulo Zanoni wrote:
> All it does is pick the encoder and call intel_get_shared_dpll(). We
> can just do this in the caller. One less indirection level during code
> reading.
> 
> As another plus, now the two callers of intel_get_shared_dpll() are
> {ironlake,haswell}_crtc_compute_clock().
> 
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c     | 25 ++-----------------------
>  drivers/gpu/drm/i915/intel_display.c |  8 +++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |  4 ++--
>  3 files changed, 11 insertions(+), 26 deletions(-)
> 
> 
> To be honest I'm not super sure if this change is an improvement. I like keeping
> DDI-related code in intel_ddi.c. I'll let you guys decide with the
> upvote/downvote buttons in your email clients.

I guess the thing that sort of ruins is the
intel_ddi_get_crtc_new_encoder() call. First I thought we could nuke
that by using output_types instead of encoder->type, but looks like
BXT wants to disagree with us and really requires the encoder in
.get_dpll() :(

Oh well, we trade one ddi thing for another. Although
intel_ddi_get_crtc_new_encoder() doesn't really have anything truly ddi
specific in it so we could even move it elsewhere or even just inline it
in .compute_clock() since it's not even called from anywhere else.

I do like the fact that there's at least one less hoop to jump
through when tracing through the code. So this works for me.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 08a810d..3fbe498 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -837,7 +837,8 @@ intel_ddi_get_crtc_encoder(struct intel_crtc *crtc)
>  	return ret;
>  }
>  
> -static struct intel_encoder *
> +/* Finds the only possible encoder associated with the given CRTC. */
> +struct intel_encoder *
>  intel_ddi_get_crtc_new_encoder(struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> @@ -1127,28 +1128,6 @@ void intel_ddi_clock_get(struct intel_encoder *encoder,
>  		bxt_ddi_clock_get(encoder, pipe_config);
>  }
>  
> -/*
> - * Tries to find a *shared* PLL for the CRTC and store it in
> - * intel_crtc->ddi_pll_sel.
> - *
> - * For private DPLLs, compute_config() should do the selection for us. This
> - * function should be folded into compute_config() eventually.
> - */
> -bool intel_ddi_pll_select(struct intel_crtc *intel_crtc,
> -			  struct intel_crtc_state *crtc_state)
> -{
> -	struct intel_encoder *encoder =
> -		intel_ddi_get_crtc_new_encoder(crtc_state);
> -	struct intel_shared_dpll *pll;
> -
> -	pll = intel_get_shared_dpll(intel_crtc, crtc_state, encoder);
> -	if (!pll)
> -		DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
> -				  pipe_name(intel_crtc->pipe));
> -
> -	return pll != NULL;
> -}
> -
>  void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 010e5dd..9cdf9b2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8862,8 +8862,14 @@ static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
>  				      struct intel_crtc_state *crtc_state)
>  {
>  	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI)) {
> -		if (!intel_ddi_pll_select(crtc, crtc_state))
> +		struct intel_encoder *encoder =
> +			intel_ddi_get_crtc_new_encoder(crtc_state);
> +
> +		if (!intel_get_shared_dpll(crtc, crtc_state, encoder)) {
> +			DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
> +					  pipe_name(crtc->pipe));
>  			return -EINVAL;
> +		}
>  	}
>  
>  	crtc->lowfreq_avail = false;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 51228fe..ad7bdf5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1236,8 +1236,8 @@ void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv,
>  				       enum transcoder cpu_transcoder);
>  void intel_ddi_enable_pipe_clock(const struct intel_crtc_state *crtc_state);
>  void intel_ddi_disable_pipe_clock(const  struct intel_crtc_state *crtc_state);
> -bool intel_ddi_pll_select(struct intel_crtc *crtc,
> -			  struct intel_crtc_state *crtc_state);
> +struct intel_encoder *
> +intel_ddi_get_crtc_new_encoder(struct intel_crtc_state *crtc_state);
>  void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state);
>  void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp);
>  bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector);
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx