On Mon, 22 Apr 2024, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > In the encoder hooks we are dealing primarily with the encoder, > so derive the DPIO PHY from the encoder rather than the pipe. > Technically this doesn't matter as we can't cross connect > pipes<->port across PHY boundaries, but it does conveny the > intention more accurately. I'll note that for most places converting vlv_dig_port_to_* to vlv_encoder_to_* would be more convenient in the caller side. We have the encoder available where they're needed, and in many places we use enc_to_dig_port(encoder) just to be able to use vlv_dig_port_to_*. I'd just convert them to vlv_encoder_to_*. Regardless, this does what it says on the tin, Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx> Oh, one comment inline near the end. > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > --- > drivers/gpu/drm/i915/display/intel_dpio_phy.c | 27 ++++++++----------- > drivers/gpu/drm/i915/vlv_sideband.c | 1 - > 2 files changed, 11 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dpio_phy.c b/drivers/gpu/drm/i915/display/intel_dpio_phy.c > index e4a04c9b5b19..4fafac534967 100644 > --- a/drivers/gpu/drm/i915/display/intel_dpio_phy.c > +++ b/drivers/gpu/drm/i915/display/intel_dpio_phy.c > @@ -719,9 +719,8 @@ void chv_set_phy_signal_level(struct intel_encoder *encoder, > { > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > struct intel_digital_port *dig_port = enc_to_dig_port(encoder); > - struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > enum dpio_channel ch = vlv_dig_port_to_channel(dig_port); > - enum dpio_phy phy = vlv_pipe_to_phy(crtc->pipe); > + enum dpio_phy phy = vlv_dig_port_to_phy(dig_port); > u32 val; > int i; > > @@ -814,9 +813,9 @@ void chv_data_lane_soft_reset(struct intel_encoder *encoder, > bool reset) > { > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > - struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > - enum dpio_channel ch = vlv_dig_port_to_channel(enc_to_dig_port(encoder)); > - enum dpio_phy phy = vlv_pipe_to_phy(crtc->pipe); > + struct intel_digital_port *dig_port = enc_to_dig_port(encoder); > + enum dpio_channel ch = vlv_dig_port_to_channel(dig_port); > + enum dpio_phy phy = vlv_dig_port_to_phy(dig_port); > u32 val; > > val = vlv_dpio_read(dev_priv, phy, VLV_PCS01_DW0(ch)); > @@ -861,7 +860,7 @@ void chv_phy_pre_pll_enable(struct intel_encoder *encoder, > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > enum dpio_channel ch = vlv_dig_port_to_channel(dig_port); > - enum dpio_phy phy = vlv_pipe_to_phy(crtc->pipe); > + enum dpio_phy phy = vlv_dig_port_to_phy(dig_port); > enum pipe pipe = crtc->pipe; > unsigned int lane_mask = > intel_dp_unused_lane_mask(crtc_state->lane_count); > @@ -941,9 +940,8 @@ void chv_phy_pre_encoder_enable(struct intel_encoder *encoder, > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > - struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > enum dpio_channel ch = vlv_dig_port_to_channel(dig_port); > - enum dpio_phy phy = vlv_pipe_to_phy(crtc->pipe); > + enum dpio_phy phy = vlv_dig_port_to_phy(dig_port); > int data, i, stagger; > u32 val; > > @@ -1030,8 +1028,8 @@ void chv_phy_post_pll_disable(struct intel_encoder *encoder, > const struct intel_crtc_state *old_crtc_state) > { > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > + enum dpio_phy phy = vlv_dig_port_to_phy(enc_to_dig_port(encoder)); > enum pipe pipe = to_intel_crtc(old_crtc_state->uapi.crtc)->pipe; > - enum dpio_phy phy = vlv_pipe_to_phy(pipe); > u32 val; > > vlv_dpio_get(dev_priv); > @@ -1068,9 +1066,8 @@ void vlv_set_phy_signal_level(struct intel_encoder *encoder, > { > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > struct intel_digital_port *dig_port = enc_to_dig_port(encoder); > - struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > enum dpio_channel ch = vlv_dig_port_to_channel(dig_port); > - enum dpio_phy phy = vlv_pipe_to_phy(crtc->pipe); > + enum dpio_phy phy = vlv_dig_port_to_phy(dig_port); > > vlv_dpio_get(dev_priv); > > @@ -1095,9 +1092,8 @@ void vlv_phy_pre_pll_enable(struct intel_encoder *encoder, > { > struct intel_digital_port *dig_port = enc_to_dig_port(encoder); > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > - struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > enum dpio_channel ch = vlv_dig_port_to_channel(dig_port); > - enum dpio_phy phy = vlv_pipe_to_phy(crtc->pipe); > + enum dpio_phy phy = vlv_dig_port_to_phy(dig_port); > > /* Program Tx lane resets to default */ > vlv_dpio_get(dev_priv); > @@ -1127,8 +1123,8 @@ void vlv_phy_pre_encoder_enable(struct intel_encoder *encoder, > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > enum dpio_channel ch = vlv_dig_port_to_channel(dig_port); > + enum dpio_phy phy = vlv_dig_port_to_phy(dig_port); > enum pipe pipe = crtc->pipe; > - enum dpio_phy phy = vlv_pipe_to_phy(pipe); > u32 val; > > vlv_dpio_get(dev_priv); > @@ -1154,9 +1150,8 @@ void vlv_phy_reset_lanes(struct intel_encoder *encoder, > { > struct intel_digital_port *dig_port = enc_to_dig_port(encoder); > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > - struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc); > enum dpio_channel ch = vlv_dig_port_to_channel(dig_port); > - enum dpio_phy phy = vlv_pipe_to_phy(crtc->pipe); > + enum dpio_phy phy = vlv_dig_port_to_phy(dig_port); > > vlv_dpio_get(dev_priv); > vlv_dpio_write(dev_priv, phy, VLV_PCS_DW0(ch), 0x00000000); > diff --git a/drivers/gpu/drm/i915/vlv_sideband.c b/drivers/gpu/drm/i915/vlv_sideband.c > index ffa195560d0d..68291412f4cb 100644 > --- a/drivers/gpu/drm/i915/vlv_sideband.c > +++ b/drivers/gpu/drm/i915/vlv_sideband.c > @@ -9,7 +9,6 @@ > #include "vlv_sideband.h" > > #include "display/intel_dpio_phy.h" > -#include "display/intel_display_types.h" I guess this should be done in some other patch? > > /* > * IOSF sideband, see VLV2_SidebandMsg_HAS.docx and -- Jani Nikula, Intel