[PATCH 12/17] drm/i915/ddi: Configure/enable a port in DDI_BUF_CTL via read-modify-write

[Kahola, Mika]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of Imre
> Deak
> Sent: Monday, 10 February 2025 20.25
> To: Jani Nikula <jani.nikula at linux.intel.com>
> Cc: intel-gfx at lists.freedesktop.org; intel-xe at lists.freedesktop.org; Imre Deak
> <imre.deak at gmail.com>
> Subject: Re: [PATCH 12/17] drm/i915/ddi: Configure/enable a port in
> DDI_BUF_CTL via read-modify-write
> 
> On Mon, Feb 10, 2025 at 08:13:14PM +0200, Jani Nikula wrote:
> > On Wed, 29 Jan 2025, Imre Deak <imre.deak at intel.com> wrote:
> > > From: Imre Deak <imre.deak at gmail.com>
> > >
> > > The various flags in DDI_BUF_CTL must be programmed at different
> > > places during a modeset. The expected value of the register at any
> > > moment is cached in the intel_dp::DP variable and the whole register
> > > is written using this variable. A simpler way would be not
> > > maintaining the cached value of the register at all and update only
> > > specific fields in the register via read-modify-write. Some places -
> > > like D2D link enabling or the port disabling - use RMW already.
> > >
> > > Based on the above update the register via RMW during the port
> > > configuration / enabling as well. After all the places updating
> > > DDI_BUF_CTL are changed - probably the voltage-swing/pre-emphasis
> > > level setting is the only one remaining - tracking the register
> > > value in intel_dp:DP could be removed.
> > >

Reviewed-by: Mika Kahola <mika.kahola at intel.com>

> > > Signed-off-by: Imre Deak <imre.deak at gmail.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_ddi.c | 23
> > > ++++++++++++++++++++++-
> > >  1 file changed, 22 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > index ddb182550ad0f..64c42505f2ad6 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > @@ -359,6 +359,10 @@ static void intel_ddi_init_dp_buf_reg(struct
> > > intel_encoder *encoder,
> > >
> > >  	if (IS_ALDERLAKE_P(i915) && intel_encoder_is_tc(encoder)) {
> > >  		intel_dp->DP |= ddi_buf_phy_link_rate(crtc_state->port_clock);
> > > +		/*
> > > +		 * TODO: remove the following once DDI_BUF_CTL is updated
> via
> > > +		 * an RMW everywhere.
> > > +		 */
> > >  		if (!intel_tc_port_in_tbt_alt_mode(dig_port))
> > >  			intel_dp->DP |= DDI_BUF_CTL_TC_PHY_OWNERSHIP;
> > >  	}
> > > @@ -370,6 +374,22 @@ static void intel_ddi_init_dp_buf_reg(struct
> intel_encoder *encoder,
> > >  	}
> > >  }
> > >
> > > +static u32 intel_ddi_buf_ctl_config_mask(struct intel_encoder
> > > +*encoder) {
> > > +	struct intel_display *display = to_intel_display(encoder);
> > > +	u32 mask = DDI_BUF_PORT_REVERSAL | DDI_A_4_LANES |
> > > +		   DDI_PORT_WIDTH_MASK;
> >
> > The caller only sets DDI_PORT_WIDTH() for display >= 14.
> 
> It does get set everywhere. For HDMI on display < 14 it's set to 0 (ignored by the
> HW), on display >= 14 it's set to the actual port width.
> 
> > > +
> > > +	if (DISPLAY_VER(display) >= 14)
> > > +		mask |= DDI_BUF_PORT_DATA_MASK;
> >
> > But doesn't touch this?
> 
> It's always set on display >= 14.
> 
> >
> > > +	if (display->platform.alderlake_p)
> > > +		mask |= DDI_BUF_PHY_LINK_RATE_MASK;
> >
> > DDI_BUF_CTL_TC_PHY_OWNERSHIP?
> 
> This shouldn't be changed during configuring and enabling the port.
> 
> > > +	if (IS_DISPLAY_VER(display, 11, 13))
> > > +		mask |= DDI_BUF_LANE_STAGGER_DELAY_MASK;
> > > +
> > > +	return mask;
> > > +}
> >
> > I don't know, this patch is incredibly hard to follow.
> >
> > BR,
> > Jani.
> >
> >
> > > +
> > >  static int icl_calc_tbt_pll_link(struct drm_i915_private *dev_priv,
> > >  				 enum port port)
> > >  {
> > > @@ -3062,7 +3082,8 @@ static void intel_enable_ddi_buf(struct
> intel_encoder *encoder, u32 buf_ctl)
> > >  	struct intel_display *display = to_intel_display(encoder);
> > >  	enum port port = encoder->port;
> > >
> > > -	intel_de_write(display, DDI_BUF_CTL(port), buf_ctl |
> DDI_BUF_CTL_ENABLE);
> > > +	intel_de_rmw(display, DDI_BUF_CTL(port),
> > > +		     intel_ddi_buf_ctl_config_mask(encoder), buf_ctl |
> > > +DDI_BUF_CTL_ENABLE);
> > >  	intel_de_posting_read(display, DDI_BUF_CTL(port));
> > >
> > >  	intel_wait_ddi_buf_active(encoder);
> >
> > --
> > Jani Nikula, Intel