On Wed, 01 Mar 2023, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > On Wed, Mar 01, 2023 at 05:14:09PM +0200, Jani Nikula wrote: >> On TGL+ the DSS control registers are at different offsets, and there's >> one per pipe. Fix the offsets to fix dual link DSI for TGL+. >> >> There would be helpers for this in the DSC code, but just do the quick >> fix now for DSI. Long term, we should probably move all the DSS handling >> into intel_vdsc.c, so exporting the helpers seems counter-productive. > > I'm not entirely happy with intel_vdsc.c since it handles > both the hardware VDSC block (which includes DSS, and so > also uncompressed joiner and MSO), and also some actual > DSC calculations/etc. Might be nice to have a cleaner > split of some sort. > > That also reminds me that MSO+dsc/joiner is probably going > to fail miserably given that neither side knows about the > other and both poke the DSS registers. > >> >> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8232 >> Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> >> Cc: stable@xxxxxxxxxxxxxxx >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx> >> --- >> drivers/gpu/drm/i915/display/icl_dsi.c | 18 +++++++++++++++--- >> 1 file changed, 15 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c >> index b5316715bb3b..5a17ab3f0d1a 100644 >> --- a/drivers/gpu/drm/i915/display/icl_dsi.c >> +++ b/drivers/gpu/drm/i915/display/icl_dsi.c >> @@ -277,9 +277,21 @@ static void configure_dual_link_mode(struct intel_encoder *encoder, >> { >> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); >> struct intel_dsi *intel_dsi = enc_to_intel_dsi(encoder); >> + i915_reg_t dss_ctl1_reg, dss_ctl2_reg; >> u32 dss_ctl1; >> >> - dss_ctl1 = intel_de_read(dev_priv, DSS_CTL1); >> + /* FIXME: Move all DSS handling to intel_vdsc.c */ >> + if (DISPLAY_VER(dev_priv) >= 12) { >> + struct intel_crtc *crtc = to_intel_crtc(pipe_config->uapi.crtc); >> + >> + dss_ctl1_reg = ICL_PIPE_DSS_CTL1(crtc->pipe); >> + dss_ctl2_reg = ICL_PIPE_DSS_CTL2(crtc->pipe); >> + } else { >> + dss_ctl1_reg = DSS_CTL1; >> + dss_ctl2_reg = DSS_CTL2; >> + } >> + >> + dss_ctl1 = intel_de_read(dev_priv, dss_ctl1_reg); > > Side note: should get rid of this rmw to make sure the thing > fully configuerd the way we want... > > Anyways, this seems fine for now: > Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> Thanks, pushed to din. BR, Jani. > >> dss_ctl1 |= SPLITTER_ENABLE; >> dss_ctl1 &= ~OVERLAP_PIXELS_MASK; >> dss_ctl1 |= OVERLAP_PIXELS(intel_dsi->pixel_overlap); >> @@ -299,14 +311,14 @@ static void configure_dual_link_mode(struct intel_encoder *encoder, >> >> dss_ctl1 &= ~LEFT_DL_BUF_TARGET_DEPTH_MASK; >> dss_ctl1 |= LEFT_DL_BUF_TARGET_DEPTH(dl_buffer_depth); >> - intel_de_rmw(dev_priv, DSS_CTL2, RIGHT_DL_BUF_TARGET_DEPTH_MASK, >> + intel_de_rmw(dev_priv, dss_ctl2_reg, RIGHT_DL_BUF_TARGET_DEPTH_MASK, >> RIGHT_DL_BUF_TARGET_DEPTH(dl_buffer_depth)); >> } else { >> /* Interleave */ >> dss_ctl1 |= DUAL_LINK_MODE_INTERLEAVE; >> } >> >> - intel_de_write(dev_priv, DSS_CTL1, dss_ctl1); >> + intel_de_write(dev_priv, dss_ctl1_reg, dss_ctl1); >> } >> >> /* aka DSI 8X clock */ >> -- >> 2.39.1 -- Jani Nikula, Intel Open Source Graphics Center
