| Message ID | 20210909230725.33735-2-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] drm/i915/display/adlp: Fix PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR calculation | expand |
On 9/10/21 2:07 AM, José Roberto de Souza wrote: > Wa_16014451276 fixes the starting coordinate for PSR2 selective > updates. CHICKEN_TRANS definition of the workaround bit has a wrong > name based on workaround definition and HSD. > > Wa_14014971508 allows the screen to continue to be updated when > coming back from DC5/DC6 and SF_SINGLE_FULL_FRAME bit is not kept > set in PSR2_MAN_TRK_CTL. > > Wa_16012604467 fixes underruns when exiting PSR2 when it is in one > of its internal states. > > Wa_14014971508 is still in pending status in BSpec but by > the time this is reviewed and ready to be merged it will be finalized. > > BSpec: 54369 > BSpec: 50054 > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/display/intel_psr.c | 23 ++++++++++++++++++++++- > drivers/gpu/drm/i915/i915_reg.h | 4 ++++ > 2 files changed, 26 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > index 36816abb3bcc0..92c0b2159559f 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.c > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > @@ -1086,6 +1086,12 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp) > intel_de_write(dev_priv, reg, chicken); > } > > + /* Wa_16014451276:adlp */ > + if (IS_ALDERLAKE_P(dev_priv) && > + intel_dp->psr.psr2_enabled) > + intel_de_rmw(dev_priv, CHICKEN_TRANS(cpu_transcoder), 0, > + D13_1_BASED_X_GRANULARITY); Depending on the capability of the PSR panel, the following setting may not be necessary, could you add some comments such as "force enable 1-based X granularity on PSR2 VSC SDP"? > + > /* > * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD also > * mask LPSP to avoid dependency on other drivers that might block > @@ -1131,6 +1137,11 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp) > TRANS_SET_CONTEXT_LATENCY(intel_dp->psr.transcoder), > TRANS_SET_CONTEXT_LATENCY_MASK, > TRANS_SET_CONTEXT_LATENCY_VALUE(1)); > + > + /* Wa_16012604467:adlp */ > + if (IS_ALDERLAKE_P(dev_priv) && intel_dp->psr.psr2_enabled) > + intel_de_rmw(dev_priv, CLKGATE_DIS_MISC, 0, > + CLKGATE_DIS_MISC_DMASC_GATING_DIS); > } > > static bool psr_interrupt_error_check(struct intel_dp *intel_dp) > @@ -1320,6 +1331,11 @@ static void intel_psr_disable_locked(struct intel_dp *intel_dp) > TRANS_SET_CONTEXT_LATENCY(intel_dp->psr.transcoder), > TRANS_SET_CONTEXT_LATENCY_MASK, 0); > > + /* Wa_16012604467:adlp */ > + if (IS_ALDERLAKE_P(dev_priv) && intel_dp->psr.psr2_enabled) > + intel_de_rmw(dev_priv, CLKGATE_DIS_MISC, > + CLKGATE_DIS_MISC_DMASC_GATING_DIS, 0); > + > intel_snps_phy_update_psr_power_state(dev_priv, phy, false); > > /* Disable PSR on Sink */ > @@ -1488,8 +1504,13 @@ static void psr2_man_trk_ctl_calc(struct intel_crtc_state *crtc_state, > u32 val = PSR2_MAN_TRK_CTL_ENABLE; > > if (full_update) { > + /* > + * Wa_14014971508:adlp > + * SINGLE_FULL_FRAME bit is not hold in register so can not be > + * restored by DMC, so using CONTINUOS_FULL_FRAME to mimic that > + */ > if (IS_ALDERLAKE_P(dev_priv)) > - val |= ADLP_PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME; > + val |= ADLP_PSR2_MAN_TRK_CTL_SF_CONTINUOS_FULL_FRAME; > else > val |= PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME; > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index c2853cc005ee6..0de2f7541da6c 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -8235,6 +8235,7 @@ enum { > #define VSC_DATA_SEL_SOFTWARE_CONTROL REG_BIT(25) /* GLK */ > #define FECSTALL_DIS_DPTSTREAM_DPTTG REG_BIT(23) > #define DDI_TRAINING_OVERRIDE_ENABLE REG_BIT(19) > +#define D13_1_BASED_X_GRANULARITY REG_BIT(18) The meaning of this macro is to set "force enable 1-based X granularity on PSR2 VSC SDP" in Display 13.1 ADL, so the meaning of the macro may be a little ambiguous. > #define DDI_TRAINING_OVERRIDE_VALUE REG_BIT(18) > #define DDIE_TRAINING_OVERRIDE_ENABLE REG_BIT(17) /* CHICKEN_TRANS_A only */ > #define DDIE_TRAINING_OVERRIDE_VALUE REG_BIT(16) /* CHICKEN_TRANS_A only */ > @@ -12789,4 +12790,7 @@ enum skl_power_gate { > #define CLKREQ_POLICY _MMIO(0x101038) > #define CLKREQ_POLICY_MEM_UP_OVRD REG_BIT(1) > > +#define CLKGATE_DIS_MISC _MMIO(0x46534) > +#define CLKGATE_DIS_MISC_DMASC_GATING_DIS REG_BIT(21) > + > #endif /* _I915_REG_H_ */ >
On Fri, 2021-09-10 at 16:38 +0300, Gwan-gyeong Mun wrote: > > On 9/10/21 2:07 AM, José Roberto de Souza wrote: > > Wa_16014451276 fixes the starting coordinate for PSR2 selective > > updates. CHICKEN_TRANS definition of the workaround bit has a wrong > > name based on workaround definition and HSD. > > > > Wa_14014971508 allows the screen to continue to be updated when > > coming back from DC5/DC6 and SF_SINGLE_FULL_FRAME bit is not kept > > set in PSR2_MAN_TRK_CTL. > > > > Wa_16012604467 fixes underruns when exiting PSR2 when it is in one > > of its internal states. > > > > Wa_14014971508 is still in pending status in BSpec but by > > the time this is reviewed and ready to be merged it will be finalized. > > > > BSpec: 54369 > > BSpec: 50054 > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_psr.c | 23 ++++++++++++++++++++++- > > drivers/gpu/drm/i915/i915_reg.h | 4 ++++ > > 2 files changed, 26 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > > index 36816abb3bcc0..92c0b2159559f 100644 > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > @@ -1086,6 +1086,12 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp) > > intel_de_write(dev_priv, reg, chicken); > > } > > > > + /* Wa_16014451276:adlp */ > > + if (IS_ALDERLAKE_P(dev_priv) && > > + intel_dp->psr.psr2_enabled) > > + intel_de_rmw(dev_priv, CHICKEN_TRANS(cpu_transcoder), 0, > > + D13_1_BASED_X_GRANULARITY); > Depending on the capability of the PSR panel, the following setting may > not be necessary, could you add some comments such as "force enable > 1-based X granularity on PSR2 VSC SDP"? It was made sure that all alderlake-P BOM panels will have 1-based X granularity, I can add something like that. > > + > > /* > > * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD also > > * mask LPSP to avoid dependency on other drivers that might block > > @@ -1131,6 +1137,11 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp) > > TRANS_SET_CONTEXT_LATENCY(intel_dp->psr.transcoder), > > TRANS_SET_CONTEXT_LATENCY_MASK, > > TRANS_SET_CONTEXT_LATENCY_VALUE(1)); > > + > > + /* Wa_16012604467:adlp */ > > + if (IS_ALDERLAKE_P(dev_priv) && intel_dp->psr.psr2_enabled) > > + intel_de_rmw(dev_priv, CLKGATE_DIS_MISC, 0, > > + CLKGATE_DIS_MISC_DMASC_GATING_DIS); > > } > > > > static bool psr_interrupt_error_check(struct intel_dp *intel_dp) > > @@ -1320,6 +1331,11 @@ static void intel_psr_disable_locked(struct intel_dp *intel_dp) > > TRANS_SET_CONTEXT_LATENCY(intel_dp->psr.transcoder), > > TRANS_SET_CONTEXT_LATENCY_MASK, 0); > > > > + /* Wa_16012604467:adlp */ > > + if (IS_ALDERLAKE_P(dev_priv) && intel_dp->psr.psr2_enabled) > > + intel_de_rmw(dev_priv, CLKGATE_DIS_MISC, > > + CLKGATE_DIS_MISC_DMASC_GATING_DIS, 0); > > + > > intel_snps_phy_update_psr_power_state(dev_priv, phy, false); > > > > /* Disable PSR on Sink */ > > @@ -1488,8 +1504,13 @@ static void psr2_man_trk_ctl_calc(struct intel_crtc_state *crtc_state, > > u32 val = PSR2_MAN_TRK_CTL_ENABLE; > > > > if (full_update) { > > + /* > > + * Wa_14014971508:adlp > > + * SINGLE_FULL_FRAME bit is not hold in register so can not be > > + * restored by DMC, so using CONTINUOS_FULL_FRAME to mimic that > > + */ > > if (IS_ALDERLAKE_P(dev_priv)) > > - val |= ADLP_PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME; > > + val |= ADLP_PSR2_MAN_TRK_CTL_SF_CONTINUOS_FULL_FRAME; > > else > > val |= PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME; > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index c2853cc005ee6..0de2f7541da6c 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -8235,6 +8235,7 @@ enum { > > #define VSC_DATA_SEL_SOFTWARE_CONTROL REG_BIT(25) /* GLK */ > > #define FECSTALL_DIS_DPTSTREAM_DPTTG REG_BIT(23) > > #define DDI_TRAINING_OVERRIDE_ENABLE REG_BIT(19) > > +#define D13_1_BASED_X_GRANULARITY REG_BIT(18) > The meaning of this macro is to set "force enable 1-based X granularity > on PSR2 VSC SDP" in Display 13.1 ADL, so the meaning of the macro may be > a little ambiguous. The name of registers are set to match specification name as close as possible not the use or meaning. > > #define DDI_TRAINING_OVERRIDE_VALUE REG_BIT(18) > > #define DDIE_TRAINING_OVERRIDE_ENABLE REG_BIT(17) /* CHICKEN_TRANS_A only */ > > #define DDIE_TRAINING_OVERRIDE_VALUE REG_BIT(16) /* CHICKEN_TRANS_A only */ > > @@ -12789,4 +12790,7 @@ enum skl_power_gate { > > #define CLKREQ_POLICY _MMIO(0x101038) > > #define CLKREQ_POLICY_MEM_UP_OVRD REG_BIT(1) > > > > +#define CLKGATE_DIS_MISC _MMIO(0x46534) > > +#define CLKGATE_DIS_MISC_DMASC_GATING_DIS REG_BIT(21) > > + > > #endif /* _I915_REG_H_ */ > >
On 9/10/21 7:29 PM, Souza, Jose wrote: > On Fri, 2021-09-10 at 16:38 +0300, Gwan-gyeong Mun wrote: >> >> On 9/10/21 2:07 AM, José Roberto de Souza wrote: >>> Wa_16014451276 fixes the starting coordinate for PSR2 selective >>> updates. CHICKEN_TRANS definition of the workaround bit has a wrong >>> name based on workaround definition and HSD. >>> >>> Wa_14014971508 allows the screen to continue to be updated when >>> coming back from DC5/DC6 and SF_SINGLE_FULL_FRAME bit is not kept >>> set in PSR2_MAN_TRK_CTL. >>> >>> Wa_16012604467 fixes underruns when exiting PSR2 when it is in one >>> of its internal states. >>> >>> Wa_14014971508 is still in pending status in BSpec but by >>> the time this is reviewed and ready to be merged it will be finalized. >>> >>> BSpec: 54369 >>> BSpec: 50054 >>> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> >>> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> >>> --- >>> drivers/gpu/drm/i915/display/intel_psr.c | 23 ++++++++++++++++++++++- >>> drivers/gpu/drm/i915/i915_reg.h | 4 ++++ >>> 2 files changed, 26 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c >>> index 36816abb3bcc0..92c0b2159559f 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_psr.c >>> +++ b/drivers/gpu/drm/i915/display/intel_psr.c >>> @@ -1086,6 +1086,12 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp) >>> intel_de_write(dev_priv, reg, chicken); >>> } >>> >>> +/* Wa_16014451276:adlp */ >>> +if (IS_ALDERLAKE_P(dev_priv) && >>> + intel_dp->psr.psr2_enabled) >>> +intel_de_rmw(dev_priv, CHICKEN_TRANS(cpu_transcoder), 0, >>> + D13_1_BASED_X_GRANULARITY); >> Depending on the capability of the PSR panel, the following setting may >> not be necessary, could you add some comments such as "force enable >> 1-based X granularity on PSR2 VSC SDP"? > > It was made sure that all alderlake-P BOM panels will have 1-based X granularity, I can add something like that. > > >>> + >>> /* >>> * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD also >>> * mask LPSP to avoid dependency on other drivers that might block >>> @@ -1131,6 +1137,11 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp) >>> TRANS_SET_CONTEXT_LATENCY(intel_dp->psr.transcoder), >>> TRANS_SET_CONTEXT_LATENCY_MASK, >>> TRANS_SET_CONTEXT_LATENCY_VALUE(1)); >>> + >>> +/* Wa_16012604467:adlp */ >>> +if (IS_ALDERLAKE_P(dev_priv) && intel_dp->psr.psr2_enabled) >>> +intel_de_rmw(dev_priv, CLKGATE_DIS_MISC, 0, >>> + CLKGATE_DIS_MISC_DMASC_GATING_DIS); >>> } >>> >>> static bool psr_interrupt_error_check(struct intel_dp *intel_dp) >>> @@ -1320,6 +1331,11 @@ static void intel_psr_disable_locked(struct intel_dp *intel_dp) >>> TRANS_SET_CONTEXT_LATENCY(intel_dp->psr.transcoder), >>> TRANS_SET_CONTEXT_LATENCY_MASK, 0); >>> >>> +/* Wa_16012604467:adlp */ >>> +if (IS_ALDERLAKE_P(dev_priv) && intel_dp->psr.psr2_enabled) >>> +intel_de_rmw(dev_priv, CLKGATE_DIS_MISC, >>> + CLKGATE_DIS_MISC_DMASC_GATING_DIS, 0); >>> + >>> intel_snps_phy_update_psr_power_state(dev_priv, phy, false); >>> >>> /* Disable PSR on Sink */ >>> @@ -1488,8 +1504,13 @@ static void psr2_man_trk_ctl_calc(struct intel_crtc_state *crtc_state, >>> u32 val = PSR2_MAN_TRK_CTL_ENABLE; >>> >>> if (full_update) { >>> +/* >>> + * Wa_14014971508:adlp >>> + * SINGLE_FULL_FRAME bit is not hold in register so can not be >>> + * restored by DMC, so using CONTINUOS_FULL_FRAME to mimic that >>> + */ >>> if (IS_ALDERLAKE_P(dev_priv)) >>> -val |= ADLP_PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME; >>> +val |= ADLP_PSR2_MAN_TRK_CTL_SF_CONTINUOS_FULL_FRAME; >>> else >>> val |= PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME; >>> >>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >>> index c2853cc005ee6..0de2f7541da6c 100644 >>> --- a/drivers/gpu/drm/i915/i915_reg.h >>> +++ b/drivers/gpu/drm/i915/i915_reg.h >>> @@ -8235,6 +8235,7 @@ enum { >>> #define VSC_DATA_SEL_SOFTWARE_CONTROLREG_BIT(25) /* GLK */ >>> #define FECSTALL_DIS_DPTSTREAM_DPTTGREG_BIT(23) >>> #define DDI_TRAINING_OVERRIDE_ENABLEREG_BIT(19) >>> +#define D13_1_BASED_X_GRANULARITYREG_BIT(18) >> The meaning of this macro is to set "force enable 1-based X granularity >> on PSR2 VSC SDP" in Display 13.1 ADL, so the meaning of the macro may be >> a little ambiguous. > > The name of registers are set to match specification name as close as possible not the use or meaning. Yes, just looking at the macro, I thought that it could be interpreted in two ways: D13 / 1_BASED_X_GRANULARITY or D13_1 / BASED_X_GRANULARITY. If our macro naming convention is fine in this case, then I don't think the code is the problem either. > >>> #define DDI_TRAINING_OVERRIDE_VALUEREG_BIT(18) >>> #define DDIE_TRAINING_OVERRIDE_ENABLEREG_BIT(17) /* CHICKEN_TRANS_A only */ >>> #define DDIE_TRAINING_OVERRIDE_VALUEREG_BIT(16) /* CHICKEN_TRANS_A only */ >>> @@ -12789,4 +12790,7 @@ enum skl_power_gate { >>> #define CLKREQ_POLICY_MMIO(0x101038) >>> #define CLKREQ_POLICY_MEM_UP_OVRDREG_BIT(1) >>> >>> +#define CLKGATE_DIS_MISC_MMIO(0x46534) >>> +#define CLKGATE_DIS_MISC_DMASC_GATING_DISREG_BIT(21) >>> + >>> #endif /* _I915_REG_H_ */ >>> >
On Mon, 2021-09-13 at 19:09 +0300, Gwan-gyeong Mun wrote: > > On 9/10/21 7:29 PM, Souza, Jose wrote: > > On Fri, 2021-09-10 at 16:38 +0300, Gwan-gyeong Mun wrote: > > > > > > On 9/10/21 2:07 AM, José Roberto de Souza wrote: > > > > Wa_16014451276 fixes the starting coordinate for PSR2 selective > > > > updates. CHICKEN_TRANS definition of the workaround bit has a wrong > > > > name based on workaround definition and HSD. > > > > > > > > Wa_14014971508 allows the screen to continue to be updated when > > > > coming back from DC5/DC6 and SF_SINGLE_FULL_FRAME bit is not kept > > > > set in PSR2_MAN_TRK_CTL. > > > > > > > > Wa_16012604467 fixes underruns when exiting PSR2 when it is in one > > > > of its internal states. > > > > > > > > Wa_14014971508 is still in pending status in BSpec but by > > > > the time this is reviewed and ready to be merged it will be finalized. > > > > > > > > BSpec: 54369 > > > > BSpec: 50054 > > > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_psr.c | 23 ++++++++++++++++++++++- > > > > drivers/gpu/drm/i915/i915_reg.h | 4 ++++ > > > > 2 files changed, 26 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > > > > index 36816abb3bcc0..92c0b2159559f 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > > @@ -1086,6 +1086,12 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp) > > > > intel_de_write(dev_priv, reg, chicken); > > > > } > > > > > > > > +/* Wa_16014451276:adlp */ > > > > +if (IS_ALDERLAKE_P(dev_priv) && > > > > + intel_dp->psr.psr2_enabled) > > > > +intel_de_rmw(dev_priv, CHICKEN_TRANS(cpu_transcoder), 0, > > > > + D13_1_BASED_X_GRANULARITY); > > > Depending on the capability of the PSR panel, the following setting may > > > not be necessary, could you add some comments such as "force enable > > > 1-based X granularity on PSR2 VSC SDP"? > > > > It was made sure that all alderlake-P BOM panels will have 1-based X granularity, I can add something like that. > > > > > > > > + > > > > /* > > > > * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD also > > > > * mask LPSP to avoid dependency on other drivers that might block > > > > @@ -1131,6 +1137,11 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp) > > > > TRANS_SET_CONTEXT_LATENCY(intel_dp->psr.transcoder), > > > > TRANS_SET_CONTEXT_LATENCY_MASK, > > > > TRANS_SET_CONTEXT_LATENCY_VALUE(1)); > > > > + > > > > +/* Wa_16012604467:adlp */ > > > > +if (IS_ALDERLAKE_P(dev_priv) && intel_dp->psr.psr2_enabled) > > > > +intel_de_rmw(dev_priv, CLKGATE_DIS_MISC, 0, > > > > + CLKGATE_DIS_MISC_DMASC_GATING_DIS); > > > > } > > > > > > > > static bool psr_interrupt_error_check(struct intel_dp *intel_dp) > > > > @@ -1320,6 +1331,11 @@ static void intel_psr_disable_locked(struct intel_dp *intel_dp) > > > > TRANS_SET_CONTEXT_LATENCY(intel_dp->psr.transcoder), > > > > TRANS_SET_CONTEXT_LATENCY_MASK, 0); > > > > > > > > +/* Wa_16012604467:adlp */ > > > > +if (IS_ALDERLAKE_P(dev_priv) && intel_dp->psr.psr2_enabled) > > > > +intel_de_rmw(dev_priv, CLKGATE_DIS_MISC, > > > > + CLKGATE_DIS_MISC_DMASC_GATING_DIS, 0); > > > > + > > > > intel_snps_phy_update_psr_power_state(dev_priv, phy, false); > > > > > > > > /* Disable PSR on Sink */ > > > > @@ -1488,8 +1504,13 @@ static void psr2_man_trk_ctl_calc(struct intel_crtc_state *crtc_state, > > > > u32 val = PSR2_MAN_TRK_CTL_ENABLE; > > > > > > > > if (full_update) { > > > > +/* > > > > + * Wa_14014971508:adlp > > > > + * SINGLE_FULL_FRAME bit is not hold in register so can not be > > > > + * restored by DMC, so using CONTINUOS_FULL_FRAME to mimic that > > > > + */ > > > > if (IS_ALDERLAKE_P(dev_priv)) > > > > -val |= ADLP_PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME; > > > > +val |= ADLP_PSR2_MAN_TRK_CTL_SF_CONTINUOS_FULL_FRAME; > > > > else > > > > val |= PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME; > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > > > index c2853cc005ee6..0de2f7541da6c 100644 > > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > > @@ -8235,6 +8235,7 @@ enum { > > > > #define VSC_DATA_SEL_SOFTWARE_CONTROLREG_BIT(25) /* GLK */ > > > > #define FECSTALL_DIS_DPTSTREAM_DPTTGREG_BIT(23) > > > > #define DDI_TRAINING_OVERRIDE_ENABLEREG_BIT(19) > > > > +#define D13_1_BASED_X_GRANULARITYREG_BIT(18) > > > The meaning of this macro is to set "force enable 1-based X granularity > > > on PSR2 VSC SDP" in Display 13.1 ADL, so the meaning of the macro may be > > > a little ambiguous. > > > > The name of registers are set to match specification name as close as possible not the use or meaning. > Yes, just looking at the macro, I thought that it could be interpreted > in two ways: D13 / 1_BASED_X_GRANULARITY or D13_1 / BASED_X_GRANULARITY. > If our macro naming convention is fine in this case, then I don't think > the code is the problem either. Okay yes someone could interpret into those 2 ways but checking bspec makes it clears that it is the first one. I can rename to ADLP_1_BASED_X_GRANULARITY if you think it would make it better. > > > > > > #define DDI_TRAINING_OVERRIDE_VALUEREG_BIT(18) > > > > #define DDIE_TRAINING_OVERRIDE_ENABLEREG_BIT(17) /* CHICKEN_TRANS_A only */ > > > > #define DDIE_TRAINING_OVERRIDE_VALUEREG_BIT(16) /* CHICKEN_TRANS_A only */ > > > > @@ -12789,4 +12790,7 @@ enum skl_power_gate { > > > > #define CLKREQ_POLICY_MMIO(0x101038) > > > > #define CLKREQ_POLICY_MEM_UP_OVRDREG_BIT(1) > > > > > > > > +#define CLKGATE_DIS_MISC_MMIO(0x46534) > > > > +#define CLKGATE_DIS_MISC_DMASC_GATING_DISREG_BIT(21) > > > > + > > > > #endif /* _I915_REG_H_ */ > > > > > >
On 9/13/21 8:00 PM, Souza, Jose wrote: > On Mon, 2021-09-13 at 19:09 +0300, Gwan-gyeong Mun wrote: >> >> On 9/10/21 7:29 PM, Souza, Jose wrote: >>> On Fri, 2021-09-10 at 16:38 +0300, Gwan-gyeong Mun wrote: >>>> >>>> On 9/10/21 2:07 AM, José Roberto de Souza wrote: >>>>> Wa_16014451276 fixes the starting coordinate for PSR2 selective >>>>> updates. CHICKEN_TRANS definition of the workaround bit has a wrong >>>>> name based on workaround definition and HSD. >>>>> >>>>> Wa_14014971508 allows the screen to continue to be updated when >>>>> coming back from DC5/DC6 and SF_SINGLE_FULL_FRAME bit is not kept >>>>> set in PSR2_MAN_TRK_CTL. >>>>> >>>>> Wa_16012604467 fixes underruns when exiting PSR2 when it is in one >>>>> of its internal states. >>>>> >>>>> Wa_14014971508 is still in pending status in BSpec but by >>>>> the time this is reviewed and ready to be merged it will be finalized. >>>>> >>>>> BSpec: 54369 >>>>> BSpec: 50054 >>>>> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> >>>>> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> >>>>> --- >>>>> drivers/gpu/drm/i915/display/intel_psr.c | 23 ++++++++++++++++++++++- >>>>> drivers/gpu/drm/i915/i915_reg.h | 4 ++++ >>>>> 2 files changed, 26 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c >>>>> index 36816abb3bcc0..92c0b2159559f 100644 >>>>> --- a/drivers/gpu/drm/i915/display/intel_psr.c >>>>> +++ b/drivers/gpu/drm/i915/display/intel_psr.c >>>>> @@ -1086,6 +1086,12 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp) >>>>> intel_de_write(dev_priv, reg, chicken); >>>>> } >>>>> >>>>> +/* Wa_16014451276:adlp */ >>>>> +if (IS_ALDERLAKE_P(dev_priv) && >>>>> + intel_dp->psr.psr2_enabled) >>>>> +intel_de_rmw(dev_priv, CHICKEN_TRANS(cpu_transcoder), 0, >>>>> + D13_1_BASED_X_GRANULARITY); >>>> Depending on the capability of the PSR panel, the following setting may >>>> not be necessary, could you add some comments such as "force enable >>>> 1-based X granularity on PSR2 VSC SDP"? >>> >>> It was made sure that all alderlake-P BOM panels will have 1-based X granularity, I can add something like that. >>> >>> >>>>> + >>>>> /* >>>>> * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD also >>>>> * mask LPSP to avoid dependency on other drivers that might block >>>>> @@ -1131,6 +1137,11 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp) >>>>> TRANS_SET_CONTEXT_LATENCY(intel_dp->psr.transcoder), >>>>> TRANS_SET_CONTEXT_LATENCY_MASK, >>>>> TRANS_SET_CONTEXT_LATENCY_VALUE(1)); >>>>> + >>>>> +/* Wa_16012604467:adlp */ >>>>> +if (IS_ALDERLAKE_P(dev_priv) && intel_dp->psr.psr2_enabled) >>>>> +intel_de_rmw(dev_priv, CLKGATE_DIS_MISC, 0, >>>>> + CLKGATE_DIS_MISC_DMASC_GATING_DIS); >>>>> } >>>>> >>>>> static bool psr_interrupt_error_check(struct intel_dp *intel_dp) >>>>> @@ -1320,6 +1331,11 @@ static void intel_psr_disable_locked(struct intel_dp *intel_dp) >>>>> TRANS_SET_CONTEXT_LATENCY(intel_dp->psr.transcoder), >>>>> TRANS_SET_CONTEXT_LATENCY_MASK, 0); >>>>> >>>>> +/* Wa_16012604467:adlp */ >>>>> +if (IS_ALDERLAKE_P(dev_priv) && intel_dp->psr.psr2_enabled) >>>>> +intel_de_rmw(dev_priv, CLKGATE_DIS_MISC, >>>>> + CLKGATE_DIS_MISC_DMASC_GATING_DIS, 0); >>>>> + >>>>> intel_snps_phy_update_psr_power_state(dev_priv, phy, false); >>>>> >>>>> /* Disable PSR on Sink */ >>>>> @@ -1488,8 +1504,13 @@ static void psr2_man_trk_ctl_calc(struct intel_crtc_state *crtc_state, >>>>> u32 val = PSR2_MAN_TRK_CTL_ENABLE; >>>>> >>>>> if (full_update) { >>>>> +/* >>>>> + * Wa_14014971508:adlp >>>>> + * SINGLE_FULL_FRAME bit is not hold in register so can not be >>>>> + * restored by DMC, so using CONTINUOS_FULL_FRAME to mimic that >>>>> + */ >>>>> if (IS_ALDERLAKE_P(dev_priv)) >>>>> -val |= ADLP_PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME; >>>>> +val |= ADLP_PSR2_MAN_TRK_CTL_SF_CONTINUOS_FULL_FRAME; >>>>> else >>>>> val |= PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME; >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >>>>> index c2853cc005ee6..0de2f7541da6c 100644 >>>>> --- a/drivers/gpu/drm/i915/i915_reg.h >>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h >>>>> @@ -8235,6 +8235,7 @@ enum { >>>>> #define VSC_DATA_SEL_SOFTWARE_CONTROLREG_BIT(25) /* GLK */ >>>>> #define FECSTALL_DIS_DPTSTREAM_DPTTGREG_BIT(23) >>>>> #define DDI_TRAINING_OVERRIDE_ENABLEREG_BIT(19) >>>>> +#define D13_1_BASED_X_GRANULARITYREG_BIT(18) >>>> The meaning of this macro is to set "force enable 1-based X granularity >>>> on PSR2 VSC SDP" in Display 13.1 ADL, so the meaning of the macro may be >>>> a little ambiguous. >>> >>> The name of registers are set to match specification name as close as possible not the use or meaning. >> Yes, just looking at the macro, I thought that it could be interpreted >> in two ways: D13 / 1_BASED_X_GRANULARITY or D13_1 / BASED_X_GRANULARITY. >> If our macro naming convention is fine in this case, then I don't think >> the code is the problem either. > > Okay yes someone could interpret into those 2 ways but checking bspec makes it clears that it is the first one. > I can rename to ADLP_1_BASED_X_GRANULARITY if you think it would make it better. > the ADLP_1_BASED_X_GRANULARITY you suggested looks better. >>> >>>>> #define DDI_TRAINING_OVERRIDE_VALUEREG_BIT(18) >>>>> #define DDIE_TRAINING_OVERRIDE_ENABLEREG_BIT(17) /* CHICKEN_TRANS_A only */ >>>>> #define DDIE_TRAINING_OVERRIDE_VALUEREG_BIT(16) /* CHICKEN_TRANS_A only */ >>>>> @@ -12789,4 +12790,7 @@ enum skl_power_gate { >>>>> #define CLKREQ_POLICY_MMIO(0x101038) >>>>> #define CLKREQ_POLICY_MEM_UP_OVRDREG_BIT(1) >>>>> >>>>> +#define CLKGATE_DIS_MISC_MMIO(0x46534) >>>>> +#define CLKGATE_DIS_MISC_DMASC_GATING_DISREG_BIT(21) >>>>> + >>>>> #endif /* _I915_REG_H_ */ >>>>> >>> >
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c index 36816abb3bcc0..92c0b2159559f 100644 --- a/drivers/gpu/drm/i915/display/intel_psr.c +++ b/drivers/gpu/drm/i915/display/intel_psr.c @@ -1086,6 +1086,12 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp) intel_de_write(dev_priv, reg, chicken); } + /* Wa_16014451276:adlp */ + if (IS_ALDERLAKE_P(dev_priv) && + intel_dp->psr.psr2_enabled) + intel_de_rmw(dev_priv, CHICKEN_TRANS(cpu_transcoder), 0, + D13_1_BASED_X_GRANULARITY); + /* * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD also * mask LPSP to avoid dependency on other drivers that might block @@ -1131,6 +1137,11 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp) TRANS_SET_CONTEXT_LATENCY(intel_dp->psr.transcoder), TRANS_SET_CONTEXT_LATENCY_MASK, TRANS_SET_CONTEXT_LATENCY_VALUE(1)); + + /* Wa_16012604467:adlp */ + if (IS_ALDERLAKE_P(dev_priv) && intel_dp->psr.psr2_enabled) + intel_de_rmw(dev_priv, CLKGATE_DIS_MISC, 0, + CLKGATE_DIS_MISC_DMASC_GATING_DIS); } static bool psr_interrupt_error_check(struct intel_dp *intel_dp) @@ -1320,6 +1331,11 @@ static void intel_psr_disable_locked(struct intel_dp *intel_dp) TRANS_SET_CONTEXT_LATENCY(intel_dp->psr.transcoder), TRANS_SET_CONTEXT_LATENCY_MASK, 0); + /* Wa_16012604467:adlp */ + if (IS_ALDERLAKE_P(dev_priv) && intel_dp->psr.psr2_enabled) + intel_de_rmw(dev_priv, CLKGATE_DIS_MISC, + CLKGATE_DIS_MISC_DMASC_GATING_DIS, 0); + intel_snps_phy_update_psr_power_state(dev_priv, phy, false); /* Disable PSR on Sink */ @@ -1488,8 +1504,13 @@ static void psr2_man_trk_ctl_calc(struct intel_crtc_state *crtc_state, u32 val = PSR2_MAN_TRK_CTL_ENABLE; if (full_update) { + /* + * Wa_14014971508:adlp + * SINGLE_FULL_FRAME bit is not hold in register so can not be + * restored by DMC, so using CONTINUOS_FULL_FRAME to mimic that + */ if (IS_ALDERLAKE_P(dev_priv)) - val |= ADLP_PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME; + val |= ADLP_PSR2_MAN_TRK_CTL_SF_CONTINUOS_FULL_FRAME; else val |= PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME; diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index c2853cc005ee6..0de2f7541da6c 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -8235,6 +8235,7 @@ enum { #define VSC_DATA_SEL_SOFTWARE_CONTROL REG_BIT(25) /* GLK */ #define FECSTALL_DIS_DPTSTREAM_DPTTG REG_BIT(23) #define DDI_TRAINING_OVERRIDE_ENABLE REG_BIT(19) +#define D13_1_BASED_X_GRANULARITY REG_BIT(18) #define DDI_TRAINING_OVERRIDE_VALUE REG_BIT(18) #define DDIE_TRAINING_OVERRIDE_ENABLE REG_BIT(17) /* CHICKEN_TRANS_A only */ #define DDIE_TRAINING_OVERRIDE_VALUE REG_BIT(16) /* CHICKEN_TRANS_A only */ @@ -12789,4 +12790,7 @@ enum skl_power_gate { #define CLKREQ_POLICY _MMIO(0x101038) #define CLKREQ_POLICY_MEM_UP_OVRD REG_BIT(1) +#define CLKGATE_DIS_MISC _MMIO(0x46534) +#define CLKGATE_DIS_MISC_DMASC_GATING_DIS REG_BIT(21) + #endif /* _I915_REG_H_ */
Wa_16014451276 fixes the starting coordinate for PSR2 selective updates. CHICKEN_TRANS definition of the workaround bit has a wrong name based on workaround definition and HSD. Wa_14014971508 allows the screen to continue to be updated when coming back from DC5/DC6 and SF_SINGLE_FULL_FRAME bit is not kept set in PSR2_MAN_TRK_CTL. Wa_16012604467 fixes underruns when exiting PSR2 when it is in one of its internal states. Wa_14014971508 is still in pending status in BSpec but by the time this is reviewed and ready to be merged it will be finalized. BSpec: 54369 BSpec: 50054 Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/display/intel_psr.c | 23 ++++++++++++++++++++++- drivers/gpu/drm/i915/i915_reg.h | 4 ++++ 2 files changed, 26 insertions(+), 1 deletion(-)