[Intel-gfx] [PATCH 4/4] drm/i915/psr: Add HBR3 support
Souza, Jose
jose.souza at intel.com
Fri Jan 25 01:12:47 UTC 2019
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 <manasi.d.navare at intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > ---
> >
> > 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 (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;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20190125/e2dfef58/attachment.sig>
More information about the Intel-gfx
mailing list