Re: [RFC PATCH 15/39] drm/i915/display: Track the Cx0 PHY enabled lane count in the PLL state

[Imre Deak <imre.deak@intel.com>]

On Wed, Oct 01, 2025 at 12:13:59PM +0300, Jani Nikula wrote:
> On Wed, 01 Oct 2025, Mika Kahola <mika.kahola@intel.com> wrote:
> [...]
> > @@ -2157,6 +2161,37 @@ static int intel_c10pll_calc_port_clock(struct intel_encoder *encoder,
> >  	return tmpclk;
> >  }
> >  
> > +static int readout_enabled_lane_count(struct intel_encoder *encoder)
> > +{
> > +	struct intel_display *display = to_intel_display(encoder);
> > +	u8 enabled_tx_lane_count = 0;
> > +	int max_tx_lane_count;
> > +	int tx_lane;
> > +
> > +	/*
> > +	 * TODO: also check inactive TX lanes in all PHY lanes owned by the
> > +	 * display. For now checking only those PHY lane(s) which are owned
> > +	 * based on the active TX lane count (i.e.
> > +	 *   1,2 active TX lanes -> PHY lane#0
> > +	 *   3,4 active TX lanes -> PHY lane#0 and PHY lane#1).
> > +	 */
> > +	max_tx_lane_count = DDI_PORT_WIDTH_GET(intel_de_read(display, DDI_BUF_CTL(encoder->port)));
> 
> Hmm, feels slightly wrong to be touching DDI_BUF_CTL() here. I think
> it's already being used in too many places. But I'm not sure what the
> clean alternative should be either.

The ideal would be to read out the PHY lane #0,#1 ownership from the
PHY/PLL registers, however I haven't yet found a way for that.

The TCSS pin assignment could be used for this as well, but that
information may not be available by this time, since the HW resets the
corresponding register when the sink is disconnected.

> > +	if (!drm_WARN_ON(display->drm, max_tx_lane_count == 0))
> > +		max_tx_lane_count = roundup_pow_of_two(max_tx_lane_count);
> 
> So I don't want to see WARN and the happy day scenario mixed like this.
> 
> This is fine, and common:
> 
> 	if (WARN_ON(something that should not happen))
> 		handle_the_error_case();
> 
> But not:
> 
> 	if (!WARN_ON(something_that_should_not_happen))
> 		handle_the_normal_case();
> 
> Looks like this could be just:
> 	
> 	if (drm_WARN_ON(display->drm, max_tx_lane_count == 0))
> 		return 0;
> 
> 	max_tx_lane_count = roundup_pow_of_two(max_tx_lane_count);
> 
> And it reads like it should.

Yes, the early return is a good idea.

> > +
> > +	for (tx_lane = 0; tx_lane < max_tx_lane_count; tx_lane++) {
> > +		u8 phy_lane_mask = tx_lane < 2 ? INTEL_CX0_LANE0 : INTEL_CX0_LANE1;
> > +		int tx = tx_lane % 2 + 1;
> > +		u8 val;
> > +
> > +		val = intel_cx0_read(encoder, phy_lane_mask, PHY_CX0_TX_CONTROL(tx, 2));
> > +		if (!(val & CONTROL2_DISABLE_SINGLE_TX))
> > +			enabled_tx_lane_count++;
> > +	}
> > +
> > +	return enabled_tx_lane_count;
> > +}
> > +
> >  static void intel_c10pll_readout_hw_state(struct intel_encoder *encoder,
> >  					  struct intel_cx0pll_state *cx0pll_state)
> >  {
> > @@ -2175,6 +2210,8 @@ static void intel_c10pll_readout_hw_state(struct intel_encoder *encoder,
> >  	 */
> >  	intel_c10_msgbus_access_begin(encoder, lane);
> >  
> > +	cx0pll_state->lane_count = readout_enabled_lane_count(encoder);
> > +
> >  	for (i = 0; i < ARRAY_SIZE(pll_state->pll); i++)
> >  		pll_state->pll[i] = intel_cx0_read(encoder, lane, PHY_C10_VDR_PLL(i));
> >  
> > @@ -2581,6 +2618,7 @@ static int intel_c20pll_calc_state(struct intel_crtc_state *crtc_state,
> >  	int err = -ENOENT;
> >  
> >  	crtc_state->dpll_hw_state.cx0pll.use_c10 = false;
> > +	crtc_state->dpll_hw_state.cx0pll.lane_count = crtc_state->lane_count;
> >  
> >  	/* try computed C20 HDMI tables before using consolidated tables */
> >  	if (!is_dp)
> > @@ -2670,6 +2708,8 @@ static void intel_c20pll_readout_hw_state(struct intel_encoder *encoder,
> >  
> >  	wakeref = intel_cx0_phy_transaction_begin(encoder);
> >  
> > +	cx0pll_state->lane_count = readout_enabled_lane_count(encoder);
> > +
> >  	/* 1. Read VDR params and current context selection */
> >  	intel_c20_readout_vdr_params(encoder, &pll_state->vdr, &cntx);
> >  
> > @@ -3107,7 +3147,7 @@ static u32 intel_cx0_get_pclk_pll_ack(u8 lane_mask)
> >  
> >  static void __intel_cx0pll_enable(struct intel_encoder *encoder,
> >  				  const struct intel_cx0pll_state *pll_state,
> > -				  bool is_dp, int port_clock, int lane_count)
> > +				  bool is_dp, int port_clock)
> >  {
> >  	struct intel_display *display = to_intel_display(encoder);
> >  	enum phy phy = intel_encoder_to_phy(encoder);
> > @@ -3149,7 +3189,7 @@ static void __intel_cx0pll_enable(struct intel_encoder *encoder,
> >  	 * 6. Program the enabled and disabled owned PHY lane
> >  	 * transmitters over message bus
> >  	 */
> > -	intel_cx0_program_phy_lane(encoder, lane_count, lane_reversal);
> > +	intel_cx0_program_phy_lane(encoder, pll_state->lane_count, lane_reversal);
> >  
> >  	/*
> >  	 * 7. Follow the Display Voltage Frequency Switching - Sequence
> > @@ -3192,7 +3232,7 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
> >  {
> >  	__intel_cx0pll_enable(encoder, &crtc_state->dpll_hw_state.cx0pll,
> >  			      intel_crtc_has_dp_encoder(crtc_state),
> > -			      crtc_state->port_clock, crtc_state->lane_count);
> > +			      crtc_state->port_clock);
> >  }
> >  
> >  int intel_mtl_tbt_calc_port_clock(struct intel_encoder *encoder)
> > @@ -3723,6 +3763,7 @@ void intel_cx0_pll_power_save_wa(struct intel_display *display)
> >  	for_each_intel_encoder(display->drm, encoder) {
> >  		struct intel_cx0pll_state pll_state = {};
> >  		int port_clock = 162000;
> > +		int lane_count = 4;
> >  
> >  		if (!intel_encoder_is_dig_port(encoder))
> >  			continue;
> > @@ -3735,7 +3776,7 @@ void intel_cx0_pll_power_save_wa(struct intel_display *display)
> >  
> >  		if (intel_c10pll_calc_state_from_table(encoder,
> >  						       mtl_c10_edp_tables,
> > -						       true, port_clock,
> > +						       true, port_clock, lane_count,
> >  						       &pll_state) < 0) {
> >  			drm_WARN_ON(display->drm,
> >  				    "Unable to calc C10 state from the tables\n");
> > @@ -3746,7 +3787,7 @@ void intel_cx0_pll_power_save_wa(struct intel_display *display)
> >  			    "[ENCODER:%d:%s] Applying power saving workaround on disabled PLL\n",
> >  			    encoder->base.base.id, encoder->base.name);
> >  
> > -		__intel_cx0pll_enable(encoder, &pll_state, true, port_clock, 4);
> > +		__intel_cx0pll_enable(encoder, &pll_state, true, port_clock);
> >  		intel_cx0pll_disable(encoder);
> >  	}
> >  }
> > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> > index 43c7200050e9..839b1a98534f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> > @@ -267,6 +267,7 @@ struct intel_cx0pll_state {
> >  		struct intel_c10pll_state c10;
> >  		struct intel_c20pll_state c20;
> >  	};
> > +	int lane_count;
> >  	bool ssc_enabled;
> >  	bool use_c10;
> >  	bool tbt_mode;
> 
> -- 
> Jani Nikula, Intel