Re: [PATCH 2/2] drm/i915/dp: move g4x_dp_set_clock() call to g4x_dp_compute_config()

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

On Wed, 11 Dec 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Wed, Dec 11, 2024 at 02:54:31PM +0200, Jani Nikula wrote:
>> It does not look like anything in intel_dp_compute_config() after the
>> g4x_dp_set_clock() call depends on the changes it makes, namely setting
>> dpll and clock_set in crtc_state.
>
> Hmm, yeah I was a bit worried about DRRS and such, but as long
> as don't mangle the clock itself/etc. then this looks fine.
>
> Series is
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks, pushed to din.

BR,
Jani.

>
>> Move the call one level higher to
>> g4x_dp_compute_config() to reduce the clutter in
>> intel_dp_compute_config().
>> 
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/g4x_dp.c   | 13 ++++++++++---
>>  drivers/gpu/drm/i915/display/g4x_dp.h   |  6 ------
>>  drivers/gpu/drm/i915/display/intel_dp.c |  3 ---
>>  3 files changed, 10 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/g4x_dp.c b/drivers/gpu/drm/i915/display/g4x_dp.c
>> index 9ac894a7411f..e06405a3b82d 100644
>> --- a/drivers/gpu/drm/i915/display/g4x_dp.c
>> +++ b/drivers/gpu/drm/i915/display/g4x_dp.c
>> @@ -55,8 +55,8 @@ const struct dpll *vlv_get_dpll(struct drm_i915_private *i915)
>>  	return IS_CHERRYVIEW(i915) ? &chv_dpll[0] : &vlv_dpll[0];
>>  }
>>  
>> -void g4x_dp_set_clock(struct intel_encoder *encoder,
>> -		      struct intel_crtc_state *pipe_config)
>> +static void g4x_dp_set_clock(struct intel_encoder *encoder,
>> +			     struct intel_crtc_state *pipe_config)
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>  	const struct dpll *divisor = NULL;
>> @@ -1228,11 +1228,18 @@ static int g4x_dp_compute_config(struct intel_encoder *encoder,
>>  				 struct drm_connector_state *conn_state)
>>  {
>>  	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>> +	int ret;
>>  
>>  	if (HAS_PCH_SPLIT(i915) && encoder->port != PORT_A)
>>  		crtc_state->has_pch_encoder = true;
>>  
>> -	return intel_dp_compute_config(encoder, crtc_state, conn_state);
>> +	ret = intel_dp_compute_config(encoder, crtc_state, conn_state);
>> +	if (ret)
>> +		return ret;
>> +
>> +	g4x_dp_set_clock(encoder, crtc_state);
>> +
>> +	return 0;
>>  }
>>  
>>  static void g4x_dp_suspend_complete(struct intel_encoder *encoder)
>> diff --git a/drivers/gpu/drm/i915/display/g4x_dp.h b/drivers/gpu/drm/i915/display/g4x_dp.h
>> index c75e64ae79b7..839a251dc069 100644
>> --- a/drivers/gpu/drm/i915/display/g4x_dp.h
>> +++ b/drivers/gpu/drm/i915/display/g4x_dp.h
>> @@ -19,8 +19,6 @@ struct intel_encoder;
>>  
>>  #ifdef I915
>>  const struct dpll *vlv_get_dpll(struct drm_i915_private *i915);
>> -void g4x_dp_set_clock(struct intel_encoder *encoder,
>> -		      struct intel_crtc_state *pipe_config);
>>  bool g4x_dp_port_enabled(struct drm_i915_private *dev_priv,
>>  			 i915_reg_t dp_reg, enum port port,
>>  			 enum pipe *pipe);
>> @@ -31,10 +29,6 @@ static inline const struct dpll *vlv_get_dpll(struct drm_i915_private *i915)
>>  {
>>  	return NULL;
>>  }
>> -static inline void g4x_dp_set_clock(struct intel_encoder *encoder,
>> -				    struct intel_crtc_state *pipe_config)
>> -{
>> -}
>>  static inline bool g4x_dp_port_enabled(struct drm_i915_private *dev_priv,
>>  				       i915_reg_t dp_reg, int port,
>>  				       enum pipe *pipe)
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index adc51567ec17..f8100c4f4d20 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -3172,9 +3172,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>  	if (pipe_config->splitter.enable)
>>  		pipe_config->dp_m_n.data_m *= pipe_config->splitter.link_count;
>>  
>> -	if (!HAS_DDI(dev_priv))
>> -		g4x_dp_set_clock(encoder, pipe_config);
>> -
>>  	intel_vrr_compute_config(pipe_config, conn_state);
>>  	intel_dp_compute_as_sdp(intel_dp, pipe_config);
>>  	intel_psr_compute_config(intel_dp, pipe_config, conn_state);
>> -- 
>> 2.39.5

-- 
Jani Nikula, Intel