diff mbox series

[v2,2/2] drm/i915/dsc: don't mess up splitter state in joiner or dsc config

Message ID aea210a824084a0644de5a546e7ecb6dfc6bdef9.1718360103.git.jani.nikula@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: fix MSO vs. joiner issue | expand

Commit Message

Jani Nikula June 14, 2024, 10:16 a.m. UTC
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@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 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(-)

Comments

Ville Syrjälä June 14, 2024, 11:47 a.m. UTC | #1
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@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  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.

>  #define  UNCOMPRESSED_JOINER_PRIMARY		(1 << 21)
>  #define  UNCOMPRESSED_JOINER_SECONDARY		(1 << 20)
>  
> -- 
> 2.39.2
Jani Nikula June 14, 2024, 2:17 p.m. UTC | #2
On Fri, 14 Jun 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> 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@linux.intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  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?


BR,
Jani.



>
>>  #define  UNCOMPRESSED_JOINER_PRIMARY		(1 << 21)
>>  #define  UNCOMPRESSED_JOINER_SECONDARY		(1 << 20)
>>  
>> -- 
>> 2.39.2
Ville Syrjälä June 14, 2024, 2:29 p.m. UTC | #3
On Fri, Jun 14, 2024 at 05:17:51PM +0300, Jani Nikula wrote:
> On Fri, 14 Jun 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> 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@linux.intel.com>
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  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.
diff mbox series

Patch

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)
 #define  UNCOMPRESSED_JOINER_PRIMARY		(1 << 21)
 #define  UNCOMPRESSED_JOINER_SECONDARY		(1 << 20)