| Message ID | 20250217223828.1166093-3-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/dp: Fix 128b/132b modeset issues | expand |
On Tue, 18 Feb 2025, Imre Deak <imre.deak@intel.com> wrote: > During disabling the transcoder in DP 128b/132b mode (both in case of an > MST master transcoder and in case of SST) the transcoder function must > be first disabled without changing any other field in the register (in > particular leaving the DDI port and mode select fields unchanged) and > clearing the DDI port and mode select fields separately, later during > the disabling sequences. Fix the sequence accordingly. > > Bspec: 54128, 65448, 68849 > Cc: Jani Nikula <jani.nikula@intel.com> > Fixes: 79a6734cd56e ("drm/i915/ddi: disable trancoder port select for 128b/132b SST") > Signed-off-by: Imre Deak <imre.deak@intel.com> Reviewed-by: Jani Nikula <jani.nikula@intel.com> Looks like I've intentionally done it this way. I think I've stumbled on the bspec text "DP v2.0/128b Primary" and the "primary" has convinced me this means MST. In most cases one should just read all things MST as being true for MTP, regardless of 8b/10b or 128b/132b, no matter what the text actually says. :p Thanks for debugging and fixing these! BR, Jani. > --- > drivers/gpu/drm/i915/display/intel_ddi.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > index 5082f38b0a02e..7937f4de66cb4 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -681,7 +681,6 @@ void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; > - bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST); > u32 ctl; > > if (DISPLAY_VER(dev_priv) >= 11) > @@ -701,8 +700,7 @@ void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state > TRANS_DDI_PORT_SYNC_MASTER_SELECT_MASK); > > if (DISPLAY_VER(dev_priv) >= 12) { > - if (!intel_dp_mst_is_master_trans(crtc_state) || > - (!is_mst && intel_dp_is_uhbr(crtc_state))) { > + if (!intel_dp_mst_is_master_trans(crtc_state)) { > ctl &= ~(TGL_TRANS_DDI_PORT_MASK | > TRANS_DDI_MODE_SELECT_MASK); > } > @@ -3138,7 +3136,7 @@ static void intel_ddi_post_disable_dp(struct intel_atomic_state *state, > intel_dp_set_power(intel_dp, DP_SET_POWER_D3); > > if (DISPLAY_VER(dev_priv) >= 12) { > - if (is_mst) { > + if (is_mst || intel_dp_is_uhbr(old_crtc_state)) { > enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder; > > intel_de_rmw(dev_priv,
On Tue, Feb 18, 2025 at 03:03:35PM +0200, Jani Nikula wrote: > On Tue, 18 Feb 2025, Imre Deak <imre.deak@intel.com> wrote: > > During disabling the transcoder in DP 128b/132b mode (both in case of an > > MST master transcoder and in case of SST) the transcoder function must > > be first disabled without changing any other field in the register (in > > particular leaving the DDI port and mode select fields unchanged) and > > clearing the DDI port and mode select fields separately, later during > > the disabling sequences. Fix the sequence accordingly. > > > > Bspec: 54128, 65448, 68849 > > Cc: Jani Nikula <jani.nikula@intel.com> > > Fixes: 79a6734cd56e ("drm/i915/ddi: disable trancoder port select for 128b/132b SST") > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > Reviewed-by: Jani Nikula <jani.nikula@intel.com> > > Looks like I've intentionally done it this way. I think I've stumbled on > the bspec text "DP v2.0/128b Primary" and the "primary" has convinced me > this means MST. In most cases one should just read all things MST as > being true for MTP, regardless of 8b/10b or 128b/132b, no matter what > the text actually says. :p Right, I also understood the spec the same way, i.e. that the 128b/132b MST and SST modeset sequences would be different. It's clearer now that this can't be the case, as far as the HW is concerned there is no difference. The only difference is the extra side-band messaging for the MST case, which the HW doesn't "care" about, since it's not aware of anything about those SB messages. Btw, I'm guessing intel_dp_mst_is_master_trans() could be renamed accordingly, intel_dp_is_mst_master_or_uhbr_trans(), or something (and crtc_state->mst_master_transcoder accordingly). > Thanks for debugging and fixing these! > > BR, > Jani. > > > > --- > > drivers/gpu/drm/i915/display/intel_ddi.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > > index 5082f38b0a02e..7937f4de66cb4 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > @@ -681,7 +681,6 @@ void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state > > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; > > - bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST); > > u32 ctl; > > > > if (DISPLAY_VER(dev_priv) >= 11) > > @@ -701,8 +700,7 @@ void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state > > TRANS_DDI_PORT_SYNC_MASTER_SELECT_MASK); > > > > if (DISPLAY_VER(dev_priv) >= 12) { > > - if (!intel_dp_mst_is_master_trans(crtc_state) || > > - (!is_mst && intel_dp_is_uhbr(crtc_state))) { > > + if (!intel_dp_mst_is_master_trans(crtc_state)) { > > ctl &= ~(TGL_TRANS_DDI_PORT_MASK | > > TRANS_DDI_MODE_SELECT_MASK); > > } > > @@ -3138,7 +3136,7 @@ static void intel_ddi_post_disable_dp(struct intel_atomic_state *state, > > intel_dp_set_power(intel_dp, DP_SET_POWER_D3); > > > > if (DISPLAY_VER(dev_priv) >= 12) { > > - if (is_mst) { > > + if (is_mst || intel_dp_is_uhbr(old_crtc_state)) { > > enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder; > > > > intel_de_rmw(dev_priv, > > -- > Jani Nikula, Intel
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index 5082f38b0a02e..7937f4de66cb4 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -681,7 +681,6 @@ void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; - bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST); u32 ctl; if (DISPLAY_VER(dev_priv) >= 11) @@ -701,8 +700,7 @@ void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state TRANS_DDI_PORT_SYNC_MASTER_SELECT_MASK); if (DISPLAY_VER(dev_priv) >= 12) { - if (!intel_dp_mst_is_master_trans(crtc_state) || - (!is_mst && intel_dp_is_uhbr(crtc_state))) { + if (!intel_dp_mst_is_master_trans(crtc_state)) { ctl &= ~(TGL_TRANS_DDI_PORT_MASK | TRANS_DDI_MODE_SELECT_MASK); } @@ -3138,7 +3136,7 @@ static void intel_ddi_post_disable_dp(struct intel_atomic_state *state, intel_dp_set_power(intel_dp, DP_SET_POWER_D3); if (DISPLAY_VER(dev_priv) >= 12) { - if (is_mst) { + if (is_mst || intel_dp_is_uhbr(old_crtc_state)) { enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder; intel_de_rmw(dev_priv,
During disabling the transcoder in DP 128b/132b mode (both in case of an MST master transcoder and in case of SST) the transcoder function must be first disabled without changing any other field in the register (in particular leaving the DDI port and mode select fields unchanged) and clearing the DDI port and mode select fields separately, later during the disabling sequences. Fix the sequence accordingly. Bspec: 54128, 65448, 68849 Cc: Jani Nikula <jani.nikula@intel.com> Fixes: 79a6734cd56e ("drm/i915/ddi: disable trancoder port select for 128b/132b SST") Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/display/intel_ddi.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)