Re: [Intel-gfx] [PATCH 4/4] drm/i915/psr: Add HBR3 support
On Tue, 2019-01-22 at 14:42 -0800, Dhinakaran Pandiyan wrote:
> On Wed, 2019-01-16 at 15:43 -0800, José Roberto de Souza wrote:
> > If the sink and source supports HBR3, TP4 should be used as link
> > training pattern.
> > For PSR2 there is no register to set and enable TP4 but according
> > to
> > eDP spec TP3 is still a training pattern acceptable for HBR3
> > panels.
> >
> Sounds like TP3 and TP4 are used only with PSR1, please document that
> in the commit message.
The line above is not enough?
> > Cc: Manasi Navare
> > Cc: Dhinakaran Pandiyan
> > Signed-off-by: José Roberto de Souza
> > ---
> >
> > Still trying to understand how PSR1 was working on ICL while
> > sending
> > TP4 to a panel that only supports HBR2.
>
> That's a good point, along with that please find out what Bit 11:
> "TPS4
> Control" does. I'd like us get these questions answered, if possible,
> before merging this series.
So according to eDP spec, DPCD 00071h, bit 0 - Link Training
Requirement of Sink on PSR Exit when Main-Link is OFF documentation:
"New to eDP v1.4, PSR2 Only: This bit is “Don’t Care” for devices
that support PSR2 because Fast Sleep/Wake support is required
for those devices."
So for sinks thats supports alpm is not necessary send the training
patterns, so it was sending TPS4 but sync was just ignoring it that is
why it was working on ICL.
But I guess is better for us to keep sending training patterns for now,
after a couple of kernel releases we could think in remove it.
I'm still looking for a panel without alpm to test if the current code
would break PSR on ICL.
>
> > drivers/gpu/drm/i915/i915_reg.h | 1 +
> > drivers/gpu/drm/i915/intel_dp_link_training.c | 2 +-
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_psr.c | 24 ++-
> > --
> > --
> > 4 files changed, 21 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 5faca634ee70..1e792309a79e 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4162,6 +4162,7 @@ enum {
> > #define EDP_PSR_TP1_TP3_SEL (1 << 11)
> > #define EDP_PSR_CRC_ENABLE (1 << 10) /*
> > BDW+ */
> > #define EDP_PSR_TP2_TP3_TIME_SHIFT (8)
> > +#define EDP_PSR_TP4_TIME_SHIFT (6) /* ICL+ */
> > #define EDP_PSR_TP1_TIME_SHIFT (4)
> > #define EDP_PSR_IDLE_FRAME_SHIFT 0
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > index 30be0e39bd5f..3e9798a5498c 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > @@ -238,7 +238,7 @@ intel_dp_link_training_clock_recovery(struct
> > intel_dp *intel_dp)
> > * or for 1.4 devices that support it, training Pattern 3 for HBR2
> > * or 1.2 devices that support it, Training Pattern 2 otherwise.
> > */
> > -static u32 intel_dp_training_pattern(struct intel_dp *intel_dp)
> > +u32 intel_dp_training_pattern(struct intel_dp *intel_dp)
> > {
> > bool source_tps3, sink_tps3, source_tps4, sink_tps4;
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index e5a436c33307..fc3e6ae92276 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1807,6 +1807,7 @@ int
> > intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
> > int link_rate, uint8_t
> > lane_count);
> > void intel_dp_start_link_train(struct intel_dp *intel_dp);
> > void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> > +u32 intel_dp_training_pattern(struct intel_dp *intel_dp);
> > int intel_dp_retrain_link(struct intel_encoder *encoder,
> > struct drm_modeset_acquire_ctx *ctx);
> > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 2fc537fb6e78..b0525940e5e9 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -440,6 +440,7 @@ static void hsw_activate_psr1(struct intel_dp
> > *intel_dp)
> > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > u32 max_sleep_time = 0x1f;
> > u32 val = EDP_PSR_ENABLE;
> > + u32 tp;
> >
> > /* Let's use 6 as the minimum to cover all known cases
> > including the
> > * off-by-one issue that HW has in some cases.
> > @@ -460,13 +461,24 @@ static void hsw_activate_psr1(struct intel_dp
> > *intel_dp)
> > val |= EDP_PSR_LINK_STANDBY;
> >
> > val |= dev_priv->vbt.psr.tp1_wakeup_time <<
> > EDP_PSR_TP1_TIME_SHIFT;
> > - val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time <<
> > EDP_PSR_TP2_TP3_TIME_SHIFT;
> >
> > - if
Re: [Intel-gfx] [PATCH 4/4] drm/i915/psr: Add HBR3 support
On Wed, 2019-01-16 at 15:43 -0800, José Roberto de Souza wrote:
> If the sink and source supports HBR3, TP4 should be used as link
> training pattern.
> For PSR2 there is no register to set and enable TP4 but according to
> eDP spec TP3 is still a training pattern acceptable for HBR3 panels.
>
Sounds like TP3 and TP4 are used only with PSR1, please document that
in the commit message.
> Cc: Manasi Navare
> Cc: Dhinakaran Pandiyan
> Signed-off-by: José Roberto de Souza
> ---
>
> Still trying to understand how PSR1 was working on ICL while sending
> TP4 to a panel that only supports HBR2.
That's a good point, along with that please find out what Bit 11: "TPS4
Control" does. I'd like us get these questions answered, if possible,
before merging this series.
>
> drivers/gpu/drm/i915/i915_reg.h | 1 +
> drivers/gpu/drm/i915/intel_dp_link_training.c | 2 +-
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_psr.c | 24 ++---
> --
> 4 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index 5faca634ee70..1e792309a79e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4162,6 +4162,7 @@ enum {
> #define EDP_PSR_TP1_TP3_SEL(1 << 11)
> #define EDP_PSR_CRC_ENABLE (1 << 10) /* BDW+ */
> #define EDP_PSR_TP2_TP3_TIME_SHIFT (8)
> +#define EDP_PSR_TP4_TIME_SHIFT (6) /* ICL+ */
> #define EDP_PSR_TP1_TIME_SHIFT (4)
> #define EDP_PSR_IDLE_FRAME_SHIFT 0
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c
> b/drivers/gpu/drm/i915/intel_dp_link_training.c
> index 30be0e39bd5f..3e9798a5498c 100644
> --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> @@ -238,7 +238,7 @@ intel_dp_link_training_clock_recovery(struct
> intel_dp *intel_dp)
> * or for 1.4 devices that support it, training Pattern 3 for HBR2
> * or 1.2 devices that support it, Training Pattern 2 otherwise.
> */
> -static u32 intel_dp_training_pattern(struct intel_dp *intel_dp)
> +u32 intel_dp_training_pattern(struct intel_dp *intel_dp)
> {
> bool source_tps3, sink_tps3, source_tps4, sink_tps4;
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index e5a436c33307..fc3e6ae92276 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1807,6 +1807,7 @@ int
> intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
> int link_rate, uint8_t
> lane_count);
> void intel_dp_start_link_train(struct intel_dp *intel_dp);
> void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> +u32 intel_dp_training_pattern(struct intel_dp *intel_dp);
> int intel_dp_retrain_link(struct intel_encoder *encoder,
> struct drm_modeset_acquire_ctx *ctx);
> void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 2fc537fb6e78..b0525940e5e9 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -440,6 +440,7 @@ static void hsw_activate_psr1(struct intel_dp
> *intel_dp)
> struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> u32 max_sleep_time = 0x1f;
> u32 val = EDP_PSR_ENABLE;
> + u32 tp;
>
> /* Let's use 6 as the minimum to cover all known cases
> including the
>* off-by-one issue that HW has in some cases.
> @@ -460,13 +461,24 @@ static void hsw_activate_psr1(struct intel_dp
> *intel_dp)
> val |= EDP_PSR_LINK_STANDBY;
>
> val |= dev_priv->vbt.psr.tp1_wakeup_time <<
> EDP_PSR_TP1_TIME_SHIFT;
> - val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time <<
> EDP_PSR_TP2_TP3_TIME_SHIFT;
>
> - if (intel_dp_source_supports_hbr2(intel_dp) &&
Now that you are removing a caller, make this function static and move
to intel_dp_link_tranining.c instead ? Or just inline the code there.
> - drm_dp_tps3_supported(intel_dp->dpcd))
> - val |= EDP_PSR_TP1_TP3_SEL;
> - else
> - val |= EDP_PSR_TP1_TP2_SEL;
> + tp = intel_dp_training_pattern(intel_dp);
> + if (tp == DP_TRAINING_PATTERN_4) {
> + /*
> + * TP4 is selected by setting EDP_PSR_TP4_TIME with
> other value
> + * than PSR_TP_WAKEUP_TIME_NONE
> + */
IMHO I think we should skip this comment, we'd have to write
documentation for every other register if we are going to do this :)
> + val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time <<
> EDP_PSR_TP4_TIME_SHIFT;
So, the EDP_PSR_TP1_TP3_SEL bit has no effect when the TP4 duration is
non-zero, seems like an indirect way of switching to TP4 compared to
TP2 and TP3.
>
Re: [Intel-gfx] [PATCH 4/4] drm/i915/psr: Add HBR3 support
On Wed, Jan 16, 2019 at 03:43:20PM -0800, José Roberto de Souza wrote:
> If the sink and source supports HBR3, TP4 should be used as link
> training pattern.
> For PSR2 there is no register to set and enable TP4 but according to
> eDP spec TP3 is still a training pattern acceptable for HBR3 panels.
>
The spec refers to training pattern 4 as TPS4, 3 as TPS3 etc so its better
to stick with the same abbreviations in this patch commit message and comments
as well since the spec uses TP2/TP3 etc for test point 2/3.
> Cc: Manasi Navare
> Cc: Dhinakaran Pandiyan
> Signed-off-by: José Roberto de Souza
> ---
>
> Still trying to understand how PSR1 was working on ICL while sending
> TP4 to a panel that only supports HBR2.
>
> drivers/gpu/drm/i915/i915_reg.h | 1 +
> drivers/gpu/drm/i915/intel_dp_link_training.c | 2 +-
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_psr.c | 24 ++-
> 4 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 5faca634ee70..1e792309a79e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4162,6 +4162,7 @@ enum {
> #define EDP_PSR_TP1_TP3_SEL(1 << 11)
> #define EDP_PSR_CRC_ENABLE (1 << 10) /* BDW+ */
> #define EDP_PSR_TP2_TP3_TIME_SHIFT (8)
> +#define EDP_PSR_TP4_TIME_SHIFT (6) /* ICL+ */
> #define EDP_PSR_TP1_TIME_SHIFT (4)
> #define EDP_PSR_IDLE_FRAME_SHIFT 0
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c
> b/drivers/gpu/drm/i915/intel_dp_link_training.c
> index 30be0e39bd5f..3e9798a5498c 100644
> --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> @@ -238,7 +238,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp
> *intel_dp)
> * or for 1.4 devices that support it, training Pattern 3 for HBR2
> * or 1.2 devices that support it, Training Pattern 2 otherwise.
> */
> -static u32 intel_dp_training_pattern(struct intel_dp *intel_dp)
> +u32 intel_dp_training_pattern(struct intel_dp *intel_dp)
> {
> bool source_tps3, sink_tps3, source_tps4, sink_tps4;
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index e5a436c33307..fc3e6ae92276 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1807,6 +1807,7 @@ int intel_dp_get_link_train_fallback_values(struct
> intel_dp *intel_dp,
> int link_rate, uint8_t lane_count);
> void intel_dp_start_link_train(struct intel_dp *intel_dp);
> void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> +u32 intel_dp_training_pattern(struct intel_dp *intel_dp);
> int intel_dp_retrain_link(struct intel_encoder *encoder,
> struct drm_modeset_acquire_ctx *ctx);
> void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 2fc537fb6e78..b0525940e5e9 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -440,6 +440,7 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
> struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> u32 max_sleep_time = 0x1f;
> u32 val = EDP_PSR_ENABLE;
> + u32 tp;
I would prefer calling this tps like the intel_dp_training_pattern function
calls it source_tps sink_tps.
Apart from that looks good. So with these changes
Reviewed-by: Manasi Navare
Manasi
>
> /* Let's use 6 as the minimum to cover all known cases including the
>* off-by-one issue that HW has in some cases.
> @@ -460,13 +461,24 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
> val |= EDP_PSR_LINK_STANDBY;
>
> val |= dev_priv->vbt.psr.tp1_wakeup_time << EDP_PSR_TP1_TIME_SHIFT;
> - val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time <<
> EDP_PSR_TP2_TP3_TIME_SHIFT;
>
> - if (intel_dp_source_supports_hbr2(intel_dp) &&
> - drm_dp_tps3_supported(intel_dp->dpcd))
> - val |= EDP_PSR_TP1_TP3_SEL;
> - else
> - val |= EDP_PSR_TP1_TP2_SEL;
> + tp = intel_dp_training_pattern(intel_dp);
> + if (tp == DP_TRAINING_PATTERN_4) {
> + /*
> + * TP4 is selected by setting EDP_PSR_TP4_TIME with other value
> + * than PSR_TP_WAKEUP_TIME_NONE
> + */
> + val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time <<
> EDP_PSR_TP4_TIME_SHIFT;
> + } else {
> + if (INTEL_GEN(dev_priv) >= 11)
> + val |= PSR_TP_WAKEUP_TIME_NONE <<
> EDP_PSR_TP4_TIME_SHIFT;
> +
> + val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time <<
> EDP_PSR_TP2_TP3_TIME_SHIFT;
> + if (tp ==
[Intel-gfx] [PATCH 4/4] drm/i915/psr: Add HBR3 support
If the sink and source supports HBR3, TP4 should be used as link
training pattern.
For PSR2 there is no register to set and enable TP4 but according to
eDP spec TP3 is still a training pattern acceptable for HBR3 panels.
Cc: Manasi Navare
Cc: Dhinakaran Pandiyan
Signed-off-by: José Roberto de Souza
---
Still trying to understand how PSR1 was working on ICL while sending
TP4 to a panel that only supports HBR2.
drivers/gpu/drm/i915/i915_reg.h | 1 +
drivers/gpu/drm/i915/intel_dp_link_training.c | 2 +-
drivers/gpu/drm/i915/intel_drv.h | 1 +
drivers/gpu/drm/i915/intel_psr.c | 24 ++-
4 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5faca634ee70..1e792309a79e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4162,6 +4162,7 @@ enum {
#define EDP_PSR_TP1_TP3_SEL (1 << 11)
#define EDP_PSR_CRC_ENABLE (1 << 10) /* BDW+ */
#define EDP_PSR_TP2_TP3_TIME_SHIFT (8)
+#define EDP_PSR_TP4_TIME_SHIFT (6) /* ICL+ */
#define EDP_PSR_TP1_TIME_SHIFT (4)
#define EDP_PSR_IDLE_FRAME_SHIFT 0
diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c
b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 30be0e39bd5f..3e9798a5498c 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -238,7 +238,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp
*intel_dp)
* or for 1.4 devices that support it, training Pattern 3 for HBR2
* or 1.2 devices that support it, Training Pattern 2 otherwise.
*/
-static u32 intel_dp_training_pattern(struct intel_dp *intel_dp)
+u32 intel_dp_training_pattern(struct intel_dp *intel_dp)
{
bool source_tps3, sink_tps3, source_tps4, sink_tps4;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e5a436c33307..fc3e6ae92276 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1807,6 +1807,7 @@ int intel_dp_get_link_train_fallback_values(struct
intel_dp *intel_dp,
int link_rate, uint8_t lane_count);
void intel_dp_start_link_train(struct intel_dp *intel_dp);
void intel_dp_stop_link_train(struct intel_dp *intel_dp);
+u32 intel_dp_training_pattern(struct intel_dp *intel_dp);
int intel_dp_retrain_link(struct intel_encoder *encoder,
struct drm_modeset_acquire_ctx *ctx);
void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 2fc537fb6e78..b0525940e5e9 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -440,6 +440,7 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
u32 max_sleep_time = 0x1f;
u32 val = EDP_PSR_ENABLE;
+ u32 tp;
/* Let's use 6 as the minimum to cover all known cases including the
* off-by-one issue that HW has in some cases.
@@ -460,13 +461,24 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
val |= EDP_PSR_LINK_STANDBY;
val |= dev_priv->vbt.psr.tp1_wakeup_time << EDP_PSR_TP1_TIME_SHIFT;
- val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time <<
EDP_PSR_TP2_TP3_TIME_SHIFT;
- if (intel_dp_source_supports_hbr2(intel_dp) &&
- drm_dp_tps3_supported(intel_dp->dpcd))
- val |= EDP_PSR_TP1_TP3_SEL;
- else
- val |= EDP_PSR_TP1_TP2_SEL;
+ tp = intel_dp_training_pattern(intel_dp);
+ if (tp == DP_TRAINING_PATTERN_4) {
+ /*
+* TP4 is selected by setting EDP_PSR_TP4_TIME with other value
+* than PSR_TP_WAKEUP_TIME_NONE
+*/
+ val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time <<
EDP_PSR_TP4_TIME_SHIFT;
+ } else {
+ if (INTEL_GEN(dev_priv) >= 11)
+ val |= PSR_TP_WAKEUP_TIME_NONE <<
EDP_PSR_TP4_TIME_SHIFT;
+
+ val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time <<
EDP_PSR_TP2_TP3_TIME_SHIFT;
+ if (tp == DP_TRAINING_PATTERN_3)
+ val |= EDP_PSR_TP1_TP3_SEL;
+ else
+ val |= EDP_PSR_TP1_TP2_SEL;
+ }
if (INTEL_GEN(dev_priv) >= 8)
val |= EDP_PSR_CRC_ENABLE;
--
2.20.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
