diff mbox series

[1/6] drm/i915/adlp: Add MST FEC BS jitter WA (Wa_14013163432)

Message ID 20240129175533.904590-2-imre.deak@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/dp: Add jitter WAs for MST/FEC/DSC links | expand

Commit Message

Imre Deak Jan. 29, 2024, 5:55 p.m. UTC 
Add a workaround to fix BS (blank start) to BS jitter issues on MST
links when FEC is enabled. Neither Bspec requires this nor Windows
clears the WA when disabling the output - presumedly because
CHICKEN_MISC_3 gets reset after disabling the pipe/transcoder - so
follow suit.

Bspec: 50050, 55424

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 24 +++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h             |  3 +++
 2 files changed, 27 insertions(+)

Comments

Nautiyal, Ankit K Jan. 30, 2024, 1:48 p.m. UTC | #1 
On 1/29/2024 11:25 PM, Imre Deak wrote:
> Add a workaround to fix BS (blank start) to BS jitter issues on MST
> links when FEC is enabled. Neither Bspec requires this nor Windows
> clears the WA when disabling the output - presumedly because
> CHICKEN_MISC_3 gets reset after disabling the pipe/transcoder - so
> follow suit.
>
> Bspec: 50050, 55424
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_dp_mst.c | 24 +++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_reg.h             |  3 +++
>   2 files changed, 27 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 5fa25a5a36b55..22c1759f912db 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -1106,6 +1106,28 @@ static void intel_mst_pre_enable_dp(struct intel_atomic_state *state,
>   	intel_ddi_set_dp_msa(pipe_config, conn_state);
>   }
>   
> +static void enable_bs_jitter_was(const struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> +	u32 clear = 0;
> +	u32 set = 0;
> +
> +	if (!IS_ALDERLAKE_P(i915))
> +		return;
> +
> +	if (!IS_DISPLAY_STEP(i915, STEP_D0, STEP_FOREVER))
> +		return;
> +
> +	/* Wa_14013163432:adlp */
> +	if (crtc_state->fec_enable || intel_dp_is_uhbr(crtc_state))

Is this for DP MST + UHBR and DP MST + FEC?

 From Bspec it seems this is meant only for MST+ FEC case, unless I am 
missing something.


> +		set |= DP_MST_FEC_BS_JITTER_WA(crtc_state->cpu_transcoder);
> +
> +	if (!clear && !set)
> +		return;
> +
> +	intel_de_rmw(i915, CHICKEN_MISC_3, clear, set);
> +}
> +
>   static void intel_mst_enable_dp(struct intel_atomic_state *state,
>   				struct intel_encoder *encoder,
>   				const struct intel_crtc_state *pipe_config,
> @@ -1134,6 +1156,8 @@ static void intel_mst_enable_dp(struct intel_atomic_state *state,
>   			       TRANS_DP2_VFREQ_PIXEL_CLOCK(crtc_clock_hz & 0xffffff));
>   	}
>   
> +	enable_bs_jitter_was(pipe_config);
> +
>   	intel_ddi_enable_transcoder_func(encoder, pipe_config);
>   
>   	clear_act_sent(encoder, pipe_config);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 75bc08081fce9..67b7d02ea37bf 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4555,6 +4555,9 @@
>   #define   GLK_CL1_PWR_DOWN		REG_BIT(11)
>   #define   GLK_CL0_PWR_DOWN		REG_BIT(10)
>   
> +#define CHICKEN_MISC_3		_MMIO(0x42088)
> +#define   DP_MST_FEC_BS_JITTER_WA(trans)	REG_BIT(0 + (trans) - TRANSCODER_A)


Should we mention /* adlp */ here?


Regards,

Ankit

> +
>   #define CHICKEN_MISC_4		_MMIO(0x4208c)
>   #define   CHICKEN_FBC_STRIDE_OVERRIDE	REG_BIT(13)
>   #define   CHICKEN_FBC_STRIDE_MASK	REG_GENMASK(12, 0)
Imre Deak Jan. 30, 2024, 2:05 p.m. UTC | #2 
On Tue, Jan 30, 2024 at 07:18:25PM +0530, Nautiyal, Ankit K wrote:
> 
> On 1/29/2024 11:25 PM, Imre Deak wrote:
> > Add a workaround to fix BS (blank start) to BS jitter issues on MST
> > links when FEC is enabled. Neither Bspec requires this nor Windows
> > clears the WA when disabling the output - presumedly because
> > CHICKEN_MISC_3 gets reset after disabling the pipe/transcoder - so
> > follow suit.
> > 
> > Bspec: 50050, 55424
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >   drivers/gpu/drm/i915/display/intel_dp_mst.c | 24 +++++++++++++++++++++
> >   drivers/gpu/drm/i915/i915_reg.h             |  3 +++
> >   2 files changed, 27 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index 5fa25a5a36b55..22c1759f912db 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -1106,6 +1106,28 @@ static void intel_mst_pre_enable_dp(struct intel_atomic_state *state,
> >   	intel_ddi_set_dp_msa(pipe_config, conn_state);
> >   }
> > +static void enable_bs_jitter_was(const struct intel_crtc_state *crtc_state)
> > +{
> > +	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> > +	u32 clear = 0;
> > +	u32 set = 0;
> > +
> > +	if (!IS_ALDERLAKE_P(i915))
> > +		return;
> > +
> > +	if (!IS_DISPLAY_STEP(i915, STEP_D0, STEP_FOREVER))
> > +		return;
> > +
> > +	/* Wa_14013163432:adlp */
> > +	if (crtc_state->fec_enable || intel_dp_is_uhbr(crtc_state))
> 
> Is this for DP MST + UHBR and DP MST + FEC?
> 
> From Bspec it seems this is meant only for MST+ FEC case, unless I am
> missing something.

You mean not meant for UHBR? The register description is clearer than
the modeset page, requiring it for both non-UHBR and UHBR. Windows also
enables it for both.

> > +		set |= DP_MST_FEC_BS_JITTER_WA(crtc_state->cpu_transcoder);
> > +
> > +	if (!clear && !set)
> > +		return;
> > +
> > +	intel_de_rmw(i915, CHICKEN_MISC_3, clear, set);
> > +}
> > +
> >   static void intel_mst_enable_dp(struct intel_atomic_state *state,
> >   				struct intel_encoder *encoder,
> >   				const struct intel_crtc_state *pipe_config,
> > @@ -1134,6 +1156,8 @@ static void intel_mst_enable_dp(struct intel_atomic_state *state,
> >   			       TRANS_DP2_VFREQ_PIXEL_CLOCK(crtc_clock_hz & 0xffffff));
> >   	}
> > +	enable_bs_jitter_was(pipe_config);
> > +
> >   	intel_ddi_enable_transcoder_func(encoder, pipe_config);
> >   	clear_act_sent(encoder, pipe_config);
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 75bc08081fce9..67b7d02ea37bf 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4555,6 +4555,9 @@
> >   #define   GLK_CL1_PWR_DOWN		REG_BIT(11)
> >   #define   GLK_CL0_PWR_DOWN		REG_BIT(10)
> > +#define CHICKEN_MISC_3		_MMIO(0x42088)
> > +#define   DP_MST_FEC_BS_JITTER_WA(trans)	REG_BIT(0 + (trans) - TRANSCODER_A)
> 
> 
> Should we mention /* adlp */ here?

In the register description the flag is valid for other platforms too,
even though it's only enabled for ADLP/RPLP.

> Regards,
> 
> Ankit
> 
> > +
> >   #define CHICKEN_MISC_4		_MMIO(0x4208c)
> >   #define   CHICKEN_FBC_STRIDE_OVERRIDE	REG_BIT(13)
> >   #define   CHICKEN_FBC_STRIDE_MASK	REG_GENMASK(12, 0)
Nautiyal, Ankit K Jan. 31, 2024, 5:05 a.m. UTC | #3 
On 1/30/2024 7:35 PM, Imre Deak wrote:
> On Tue, Jan 30, 2024 at 07:18:25PM +0530, Nautiyal, Ankit K wrote:
>> On 1/29/2024 11:25 PM, Imre Deak wrote:
>>> Add a workaround to fix BS (blank start) to BS jitter issues on MST
>>> links when FEC is enabled. Neither Bspec requires this nor Windows
>>> clears the WA when disabling the output - presumedly because
>>> CHICKEN_MISC_3 gets reset after disabling the pipe/transcoder - so
>>> follow suit.
>>>
>>> Bspec: 50050, 55424
>>>
>>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/display/intel_dp_mst.c | 24 +++++++++++++++++++++
>>>    drivers/gpu/drm/i915/i915_reg.h             |  3 +++
>>>    2 files changed, 27 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>> index 5fa25a5a36b55..22c1759f912db 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>> @@ -1106,6 +1106,28 @@ static void intel_mst_pre_enable_dp(struct intel_atomic_state *state,
>>>    	intel_ddi_set_dp_msa(pipe_config, conn_state);
>>>    }
>>> +static void enable_bs_jitter_was(const struct intel_crtc_state *crtc_state)
>>> +{
>>> +	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
>>> +	u32 clear = 0;
>>> +	u32 set = 0;
>>> +
>>> +	if (!IS_ALDERLAKE_P(i915))
>>> +		return;
>>> +
>>> +	if (!IS_DISPLAY_STEP(i915, STEP_D0, STEP_FOREVER))
>>> +		return;
>>> +
>>> +	/* Wa_14013163432:adlp */
>>> +	if (crtc_state->fec_enable || intel_dp_is_uhbr(crtc_state))
>> Is this for DP MST + UHBR and DP MST + FEC?
>>
>>  From Bspec it seems this is meant only for MST+ FEC case, unless I am
>> missing something.
> You mean not meant for UHBR? The register description is clearer than
> the modeset page, requiring it for both non-UHBR and UHBR. Windows also
> enables it for both.
>
>>> +		set |= DP_MST_FEC_BS_JITTER_WA(crtc_state->cpu_transcoder);
>>> +
>>> +	if (!clear && !set)
>>> +		return;
>>> +
>>> +	intel_de_rmw(i915, CHICKEN_MISC_3, clear, set);
>>> +}
>>> +
>>>    static void intel_mst_enable_dp(struct intel_atomic_state *state,
>>>    				struct intel_encoder *encoder,
>>>    				const struct intel_crtc_state *pipe_config,
>>> @@ -1134,6 +1156,8 @@ static void intel_mst_enable_dp(struct intel_atomic_state *state,
>>>    			       TRANS_DP2_VFREQ_PIXEL_CLOCK(crtc_clock_hz & 0xffffff));
>>>    	}
>>> +	enable_bs_jitter_was(pipe_config);
>>> +
>>>    	intel_ddi_enable_transcoder_func(encoder, pipe_config);
>>>    	clear_act_sent(encoder, pipe_config);
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index 75bc08081fce9..67b7d02ea37bf 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -4555,6 +4555,9 @@
>>>    #define   GLK_CL1_PWR_DOWN		REG_BIT(11)
>>>    #define   GLK_CL0_PWR_DOWN		REG_BIT(10)
>>> +#define CHICKEN_MISC_3		_MMIO(0x42088)
>>> +#define   DP_MST_FEC_BS_JITTER_WA(trans)	REG_BIT(0 + (trans) - TRANSCODER_A)
>>
>> Should we mention /* adlp */ here?
> In the register description the flag is valid for other platforms too,
> even though it's only enabled for ADLP/RPLP.

Yes indeed.

Patch looks good to me.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>


>
>> Regards,
>>
>> Ankit
>>
>>> +
>>>    #define CHICKEN_MISC_4		_MMIO(0x4208c)
>>>    #define   CHICKEN_FBC_STRIDE_OVERRIDE	REG_BIT(13)
>>>    #define   CHICKEN_FBC_STRIDE_MASK	REG_GENMASK(12, 0)
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 5fa25a5a36b55..22c1759f912db 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -1106,6 +1106,28 @@  static void intel_mst_pre_enable_dp(struct intel_atomic_state *state,
 	intel_ddi_set_dp_msa(pipe_config, conn_state);
 }
 
+static void enable_bs_jitter_was(const struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
+	u32 clear = 0;
+	u32 set = 0;
+
+	if (!IS_ALDERLAKE_P(i915))
+		return;
+
+	if (!IS_DISPLAY_STEP(i915, STEP_D0, STEP_FOREVER))
+		return;
+
+	/* Wa_14013163432:adlp */
+	if (crtc_state->fec_enable || intel_dp_is_uhbr(crtc_state))
+		set |= DP_MST_FEC_BS_JITTER_WA(crtc_state->cpu_transcoder);
+
+	if (!clear && !set)
+		return;
+
+	intel_de_rmw(i915, CHICKEN_MISC_3, clear, set);
+}
+
 static void intel_mst_enable_dp(struct intel_atomic_state *state,
 				struct intel_encoder *encoder,
 				const struct intel_crtc_state *pipe_config,
@@ -1134,6 +1156,8 @@  static void intel_mst_enable_dp(struct intel_atomic_state *state,
 			       TRANS_DP2_VFREQ_PIXEL_CLOCK(crtc_clock_hz & 0xffffff));
 	}
 
+	enable_bs_jitter_was(pipe_config);
+
 	intel_ddi_enable_transcoder_func(encoder, pipe_config);
 
 	clear_act_sent(encoder, pipe_config);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 75bc08081fce9..67b7d02ea37bf 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4555,6 +4555,9 @@ 
 #define   GLK_CL1_PWR_DOWN		REG_BIT(11)
 #define   GLK_CL0_PWR_DOWN		REG_BIT(10)
 
+#define CHICKEN_MISC_3		_MMIO(0x42088)
+#define   DP_MST_FEC_BS_JITTER_WA(trans)	REG_BIT(0 + (trans) - TRANSCODER_A)
+
 #define CHICKEN_MISC_4		_MMIO(0x4208c)
 #define   CHICKEN_FBC_STRIDE_OVERRIDE	REG_BIT(13)
 #define   CHICKEN_FBC_STRIDE_MASK	REG_GENMASK(12, 0)