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 <manasi.d.navare@xxxxxxxxx>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> ---
>
> 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.
> + } 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;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
