Re: [PATCH v2 25/32] drm/i915/cx0: Add MTL+ .get_hw_state hook

[Gustavo Sousa <gustavo.sousa@intel.com>]

Mika Kahola <mika.kahola@intel.com> writes:

> Add .get_hw_state hook to MTL+ platforms for dpll framework.
>
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cx0_phy.c  |  9 ++++--
>  drivers/gpu/drm/i915/display/intel_cx0_phy.h  |  2 +-
>  drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 29 +++++++++++++++++++
>  3 files changed, 37 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> index a88169b76cfa..bde461878647 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -3594,19 +3594,24 @@ intel_mtl_port_pll_type(struct intel_encoder *encoder,
>  		return ICL_PORT_DPLL_DEFAULT;
>  }
>  
> -void intel_cx0pll_readout_hw_state(struct intel_encoder *encoder,
> +bool intel_cx0pll_readout_hw_state(struct intel_encoder *encoder,
>  				   struct intel_cx0pll_state *pll_state)
>  {
>  	memset(pll_state, 0, sizeof(*pll_state));
>  
>  	pll_state->tbt_mode = intel_tc_port_in_tbt_alt_mode(enc_to_dig_port(encoder));
>  	if (pll_state->tbt_mode)
> -		return;
> +		return true;
> +
> +	if (!intel_cx0_pll_is_enabled(encoder))
> +		return false;
>  
>  	if (intel_encoder_is_c10phy(encoder))
>  		intel_c10pll_readout_hw_state(encoder, pll_state);
>  	else
>  		intel_c20pll_readout_hw_state(encoder, pll_state);
> +
> +	return true;
>  }
>  
>  static bool mtl_compare_hw_state_c10(const struct intel_c10pll_state *a,
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.h b/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> index acfbaced22f5..37b53faa5e78 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> @@ -37,7 +37,7 @@ intel_mtl_port_pll_type(struct intel_encoder *encoder,
>  int intel_cx0pll_calc_state(const struct intel_crtc_state *crtc_state,
>  			    struct intel_encoder *encoder,
>  			    struct intel_dpll_hw_state *hw_state);
> -void intel_cx0pll_readout_hw_state(struct intel_encoder *encoder,
> +bool intel_cx0pll_readout_hw_state(struct intel_encoder *encoder,
>  				   struct intel_cx0pll_state *pll_state);
>  int intel_cx0pll_calc_port_clock(struct intel_encoder *encoder,
>  				 const struct intel_cx0pll_state *pll_state);
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> index c0ba269dc714..beaf270294ca 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -4351,7 +4351,36 @@ static const struct intel_dpll_mgr adlp_pll_mgr = {
>  	.compare_hw_state = icl_compare_hw_state,
>  };
>  
> +static struct intel_encoder *get_intel_encoder(struct intel_display *display,
> +					       const struct intel_dpll *pll)
> +{
> +	struct intel_encoder *encoder;
> +	enum intel_dpll_id mtl_id;
> +
> +	for_each_intel_encoder(display->drm, encoder) {
> +		mtl_id = mtl_port_to_pll_id(display, encoder->port);
> +
> +		if (mtl_id == pll->info->id)
> +			return encoder;
> +	}
> +
> +	return NULL;
> +}
> +
> +static bool mtl_pll_get_hw_state(struct intel_display *display,
> +				 struct intel_dpll *pll,
> +				 struct intel_dpll_hw_state *dpll_hw_state)
> +{
> +	struct intel_encoder *encoder = get_intel_encoder(display, pll);
> +
> +	if (!encoder)
> +		return false;

I came accross this code recently and I'm wondering why we need to have
this null check here.

Are there scenarios where this is possible? If so, are those possible
only starting with Xe_LPDplus?

--
Gustavo Sousa

> +
> +	return intel_cx0pll_readout_hw_state(encoder, &dpll_hw_state->cx0pll);
> +}
> +
>  static const struct intel_dpll_funcs mtl_pll_funcs = {
> +	.get_hw_state = mtl_pll_get_hw_state,
>  };
>  
>  static const struct dpll_info mtl_plls[] = {
> -- 
> 2.34.1