On Fri, Jun 14, 2024 at 05:17:51PM +0300, Jani Nikula wrote:
> On Fri, 14 Jun 2024, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> > On Fri, Jun 14, 2024 at 01:16:04PM +0300, Jani Nikula wrote:
> >> The driver handles splitter (for MSO) and joiner/dsc configuration in
> >> different places. Avoid messing up the splitter hardware state when
> >> enabling/disabling joiner or dsc. It should not be possible to enable
> >> both joiner and splitter at the same time, but add more clarity to the
> >> register use overall.
> >>
> >> Note: We should probably handle splitter for MSO as well as dual-link
> >> DSI in intel_vdsc.c. Also, we have intel_uncompressed_joiner_enable()
> >> but no corresponding disable.
> >>
> >> Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/i915/display/intel_ddi.c | 5 ++---
> >> drivers/gpu/drm/i915/display/intel_vdsc.c | 12 +++++++++---
> >> drivers/gpu/drm/i915/display/intel_vdsc_regs.h | 1 +
> >> 3 files changed, 12 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> >> index bb13a3ca8c7c..49509a6599fe 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> >> @@ -2417,9 +2417,8 @@ static void intel_ddi_mso_configure(const struct intel_crtc_state *crtc_state)
> >> dss1 |= SPLITTER_CONFIGURATION_4_SEGMENT;
> >> }
> >>
> >> - intel_de_rmw(i915, ICL_PIPE_DSS_CTL1(pipe),
> >> - SPLITTER_ENABLE | SPLITTER_CONFIGURATION_MASK |
> >> - OVERLAP_PIXELS_MASK, dss1);
> >> + /* Only touch the splitter */
> >> + intel_de_rmw(i915, ICL_PIPE_DSS_CTL1(pipe), SPLITTER_STATE, dss1);
> >> }
> >>
> >> static u8 mtl_get_port_width(u8 lane_count)
> >> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
> >> index b9687b7692b8..a8671d3f1d41 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> >> @@ -766,7 +766,9 @@ void intel_uncompressed_joiner_enable(const struct intel_crtc_state *crtc_state)
> >> else
> >> dss_ctl1_val |= UNCOMPRESSED_JOINER_PRIMARY;
> >>
> >> - intel_de_write(dev_priv, dss_ctl1_reg(crtc, crtc_state->cpu_transcoder), dss_ctl1_val);
> >> + /* Avoid touching the splitter */
> >> + intel_de_rmw(dev_priv, dss_ctl1_reg(crtc, crtc_state->cpu_transcoder),
> >> + ~SPLITTER_STATE, dss_ctl1_val);
> >> }
> >> }
> >>
> >> @@ -793,7 +795,9 @@ void intel_dsc_enable(const struct intel_crtc_state *crtc_state)
> >> if (!intel_crtc_is_joiner_secondary(crtc_state))
> >> dss_ctl1_val |= PRIMARY_BIG_JOINER_ENABLE;
> >> }
> >> - intel_de_write(dev_priv, dss_ctl1_reg(crtc, crtc_state->cpu_transcoder), dss_ctl1_val);
> >> + /* Avoid touching the splitter */
> >> + intel_de_rmw(dev_priv, dss_ctl1_reg(crtc, crtc_state->cpu_transcoder),
> >> + ~SPLITTER_STATE, dss_ctl1_val);
> >> intel_de_write(dev_priv, dss_ctl2_reg(crtc, crtc_state->cpu_transcoder), dss_ctl2_val);
> >> }
> >>
> >> @@ -805,7 +809,9 @@ void intel_dsc_disable(const struct intel_crtc_state *old_crtc_state)
> >> /* Disable only if either of them is enabled */
> >> if (old_crtc_state->dsc.compression_enable ||
> >> old_crtc_state->joiner_pipes) {
> >> - intel_de_write(dev_priv, dss_ctl1_reg(crtc, old_crtc_state->cpu_transcoder), 0);
> >> + /* Avoid touching the splitter */
> >> + intel_de_rmw(dev_priv, dss_ctl1_reg(crtc, old_crtc_state->cpu_transcoder),
> >> + ~SPLITTER_STATE, 0);
> >> intel_de_write(dev_priv, dss_ctl2_reg(crtc, old_crtc_state->cpu_transcoder), 0);
> >> }
> >> }
> >> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc_regs.h b/drivers/gpu/drm/i915/display/intel_vdsc_regs.h
> >> index f921ad67b587..3734cd96f55e 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_vdsc_regs.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_vdsc_regs.h
> >> @@ -37,6 +37,7 @@
> >> #define SPLITTER_CONFIGURATION_MASK REG_GENMASK(26, 25)
> >> #define SPLITTER_CONFIGURATION_2_SEGMENT REG_FIELD_PREP(SPLITTER_CONFIGURATION_MASK, 0)
> >> #define SPLITTER_CONFIGURATION_4_SEGMENT REG_FIELD_PREP(SPLITTER_CONFIGURATION_MASK, 1)
> >> +#define SPLITTER_STATE (SPLITTER_ENABLE | SPLITTER_CONFIGURATION_MASK | OVERLAP_PIXELS_MASK)
> >
> > Not a big fan of this. I'd rather explicicitly list the bits
> > we actually want to modify in each call site.
> >
> > Also not a big fan of the rmws. I think in the future we might be
> > able to adjust some DSC stuff via fastsets, and that means no rmws
> > because we then want to do it via DSB. But not sure if the DSS
> > registers specifically would be involved in that, and I guess we
> > already had some rmws in there so it'll require work anyway. So
> > no hard objection to using rmw for now.
>
> I'm definitely not a fan of the rmws myself. It's just that there's
> already rmw for splitter in both dual-link DSI and eDP MSO. And I think
> they're both bust when combined with DSC.
>
> I think the proper fix would be to handle everything related to DSS CTL
> in the same place, but I'm not even sure the modeset sequences for DSI
> and MSO are flexible enough for that. I don't know. Maybe the stuff
> could be written in two places as long as the single point of truth
> knows what to put there and when. Seems a bit involved right now.
>
> So, uh, is the compromise to drop this SPLITTER_STATE, and have each
> place rmw into the register just what each place needs?
I think that's an OK solution for now. Though it might need a bit
of care to make sure we also clear out all the bits at some point
if everyone ends up doing rmws.
intel_ddi_mso_configure() (+whatever is the DSI counterpart)
should perhaps be the thing that always clears everything at the
start.
--
Ville Syrjälä
Intel
