Re: [PATCH 1/9] drm/i915/display: get rid of encoder param in intel_audio_compute_config

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

On Tue, 08 Oct 2024, Vinod Govindapillai <vinod.govindapillai@intel.com> wrote:
> The parameter "encoder" is used only to get the drm_i915_private
> object. As we could get the drm_i915_private from connector, we
> dont need this encoder to be passed to this function. So get
> rid of the encoder parametet and update all the corrsponding
> intel_audio_compute_config calls. In the followup patches, we
> don't even need the drm_i915_private object in this function.

I've got mixed feelings about this. On the one hand, removing extra
params is good. On the other hand, I'm thinking we should just pass
along all the encoder->compute_config hook parameters all the way to all
functions named _compute_config(), because we just keep removing and
adding these parameters as the driver evolves. We go in circles with
this.

BR,
Jani.



>
> Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_audio.c | 5 ++---
>  drivers/gpu/drm/i915/display/intel_audio.h | 3 +--
>  drivers/gpu/drm/i915/display/intel_dp.c    | 2 +-
>  drivers/gpu/drm/i915/display/intel_hdmi.c  | 2 +-
>  drivers/gpu/drm/i915/display/intel_sdvo.c  | 2 +-
>  5 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c
> index 32aa9ec1a204..1afd3b99e3ea 100644
> --- a/drivers/gpu/drm/i915/display/intel_audio.c
> +++ b/drivers/gpu/drm/i915/display/intel_audio.c
> @@ -690,12 +690,11 @@ void intel_audio_sdp_split_update(const struct intel_crtc_state *crtc_state)
>  			     crtc_state->sdp_split_enable ? AUD_ENABLE_SDP_SPLIT : 0);
>  }
>  
> -bool intel_audio_compute_config(struct intel_encoder *encoder,
> -				struct intel_crtc_state *crtc_state,
> +bool intel_audio_compute_config(struct intel_crtc_state *crtc_state,
>  				struct drm_connector_state *conn_state)
>  {
> -	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>  	struct drm_connector *connector = conn_state->connector;
> +	struct drm_i915_private *i915 = to_i915(connector->dev);
>  	const struct drm_display_mode *adjusted_mode =
>  		&crtc_state->hw.adjusted_mode;
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_audio.h b/drivers/gpu/drm/i915/display/intel_audio.h
> index 576c061d72a4..9b327b677d89 100644
> --- a/drivers/gpu/drm/i915/display/intel_audio.h
> +++ b/drivers/gpu/drm/i915/display/intel_audio.h
> @@ -14,8 +14,7 @@ struct intel_crtc_state;
>  struct intel_encoder;
>  
>  void intel_audio_hooks_init(struct drm_i915_private *dev_priv);
> -bool intel_audio_compute_config(struct intel_encoder *encoder,
> -				struct intel_crtc_state *crtc_state,
> +bool intel_audio_compute_config(struct intel_crtc_state *crtc_state,
>  				struct drm_connector_state *conn_state);
>  void intel_audio_codec_enable(struct intel_encoder *encoder,
>  			      const struct intel_crtc_state *crtc_state,
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index fbb096be02ad..0fd9c1c51a43 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -3001,7 +3001,7 @@ intel_dp_audio_compute_config(struct intel_encoder *encoder,
>  {
>  	pipe_config->has_audio =
>  		intel_dp_has_audio(encoder, conn_state) &&
> -		intel_audio_compute_config(encoder, pipe_config, conn_state);
> +		intel_audio_compute_config(pipe_config, conn_state);
>  
>  	pipe_config->sdp_split_enable = pipe_config->has_audio &&
>  					intel_dp_is_uhbr(pipe_config);
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 72ac910bf6ec..45ff2ee7cb70 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2333,7 +2333,7 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder,
>  
>  	pipe_config->has_audio =
>  		intel_hdmi_has_audio(encoder, pipe_config, conn_state) &&
> -		intel_audio_compute_config(encoder, pipe_config, conn_state);
> +		intel_audio_compute_config(pipe_config, conn_state);
>  
>  	/*
>  	 * Try to respect downstream TMDS clock limits first, if
> diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
> index b83bf813677d..2fbb69ee7b45 100644
> --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> @@ -1430,7 +1430,7 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
>  
>  	pipe_config->has_audio =
>  		intel_sdvo_has_audio(encoder, pipe_config, conn_state) &&
> -		intel_audio_compute_config(encoder, pipe_config, conn_state);
> +		intel_audio_compute_config(pipe_config, conn_state);
>  
>  	pipe_config->limited_color_range =
>  		intel_sdvo_limited_color_range(encoder, pipe_config,

-- 
Jani Nikula, Intel