[Intel-gfx] [PATCH v5 3/3] drm/i915: Make PSR registers relative to transcoders
Souza, Jose
jose.souza at intel.com
Wed Jun 19 00:27:01 UTC 2019
On Fri, 2019-06-14 at 21:27 -0700, Dhinakaran Pandiyan wrote:
> "drm/i915/psr" in the subject.
Done
>
> On Sat, 2019-04-20 at 13:55 -0700, José Roberto de Souza wrote:
> > PSR registers are a mess, some have the full address while others
> > just
> > have the additional offset from psr_mmio_base.
> >
> > For BDW+ psr_mmio_base is nothing more than TRANSCODER_EDP_OFFSET +
> > 0x800 and using it makes more difficult for people with an PSR
> > register address or PSR register name from from BSpec as i915 also
> > don't match the BSpec names.
> > For HSW psr_mmio_base is _DDI_BUF_CTL_A + 0x800 and PSR registers
> > are
> > only available in DDIA.
> >
> > Other reason to make relative to transcoder is that since BDW every
> > transcoder have PSR registers, so in theory it should be possible
> > to
> > have PSR enabled in a non-eDP transcoder.
> >
> > So for BDW+ we can use _TRANS2() to get the register offset of any
> > PSR register in any transcoder while for HSW we have _HSW_PSR_ADJ
> > that will calculate the register offset for the single PSR
> > instance,
> > noting that we are already guarded about trying to enable PSR in
> > other
> > port than DDIA on HSW by the 'if (dig_port->base.port != PORT_A)'
> > in
> > intel_psr_compute_config(), this check should only be valid for HSW
> > and will be changed in future.
> > PSR2 registers and PSR_EVENT was added after Haswell so that is why
> > _PSR_ADJ() is not used in some macros.
> >
> > The only registers that can not be relative to transcoder are
> > PSR_IMR and PSR_IIR that are not relative to anything, so keeping
> > it
> > hardcoded.
> >
> > Also removing BDW_EDP_PSR_BASE from GVT because it is not used as
> > it
> > is the only PSR register that GVT have.
> >
> > v5:
> > - Macros changed to be more explicit about HSW (Dhinakaran)
> > - Squashed with the patch that added the tran parameter to the
> > macros (Dhinakaran)
> >
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > Cc: Jani Nikula <jani.nikula at linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Cc: Zhi Wang <zhi.a.wang at intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > ---
> > drivers/gpu/drm/i915/gvt/handlers.c | 1 -
> > drivers/gpu/drm/i915/i915_debugfs.c | 18 +++++----
> > drivers/gpu/drm/i915/i915_drv.h | 3 +-
> > drivers/gpu/drm/i915/i915_reg.h | 57 ++++++++++++++++++++-----
> > ----
> > drivers/gpu/drm/i915/intel_psr.c | 55 ++++++++++++++++---------
> > ---
> > 5 files changed, 83 insertions(+), 51 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/handlers.c
> > b/drivers/gpu/drm/i915/gvt/handlers.c
> > index 18f01eeb2510..749e3e4204f2 100644
> > --- a/drivers/gpu/drm/i915/gvt/handlers.c
> > +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> > @@ -2776,7 +2776,6 @@ static int init_broadwell_mmio_info(struct
> > intel_gvt
> > *gvt)
> > MMIO_D(CHICKEN_PIPESL_1(PIPE_C), D_BDW_PLUS);
> >
> > MMIO_D(WM_MISC, D_BDW);
> > - MMIO_D(_MMIO(BDW_EDP_PSR_BASE), D_BDW);
> Change this to _SRD_CTL_EDP to keep the change non-functional? Any
> case, we'll
> need an ack to modify this.
Ping Zhi?
Do you think we should replace with _SRD_CTL_EDP or is okay to remove
it?
>
> > MMIO_D(_MMIO(0x66c00), D_BDW_PLUS);
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 5823ffb17821..2a0f5871e9a8 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2470,7 +2470,7 @@ psr_source_status(struct drm_i915_private
> > *dev_priv,
> > struct seq_file *m)
> > "BUF_ON",
> > "TG_ON"
> > };
> > - val = I915_READ(EDP_PSR2_STATUS);
> > + val = I915_READ(EDP_PSR2_STATUS(dev_priv-
> > >psr.transcoder));
> > status_val = (val & EDP_PSR2_STATUS_STATE_MASK) >>
> > EDP_PSR2_STATUS_STATE_SHIFT;
> > if (status_val < ARRAY_SIZE(live_status))
> > @@ -2486,7 +2486,7 @@ psr_source_status(struct drm_i915_private
> > *dev_priv,
> > struct seq_file *m)
> > "SRDOFFACK",
> > "SRDENT_ON",
> > };
> > - val = I915_READ(EDP_PSR_STATUS);
> > + val = I915_READ(EDP_PSR_STATUS(dev_priv-
> > >psr.transcoder));
> > status_val = (val & EDP_PSR_STATUS_STATE_MASK) >>
> > EDP_PSR_STATUS_STATE_SHIFT;
> > if (status_val < ARRAY_SIZE(live_status))
> > @@ -2529,10 +2529,10 @@ static int i915_edp_psr_status(struct
> > seq_file *m,
> > void *data)
> > goto unlock;
> >
> > if (psr->psr2_enabled) {
> > - val = I915_READ(EDP_PSR2_CTL);
> > + val = I915_READ(EDP_PSR2_CTL(dev_priv-
> > >psr.transcoder));
> > enabled = val & EDP_PSR2_ENABLE;
> > } else {
> > - val = I915_READ(EDP_PSR_CTL);
> > + val = I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder));
> > enabled = val & EDP_PSR_ENABLE;
> > }
> > seq_printf(m, "Source PSR ctl: %s [0x%08x]\n",
> > @@ -2545,7 +2545,8 @@ static int i915_edp_psr_status(struct
> > seq_file *m, void
> > *data)
> > * SKL+ Perf counter is reset to 0 everytime DC state is
> > entered
> > */
> > if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > - val = I915_READ(EDP_PSR_PERF_CNT) &
> > EDP_PSR_PERF_CNT_MASK;
> > + val = I915_READ(EDP_PSR_PERF_CNT(dev_priv-
> > >psr.transcoder));
> > + val &= EDP_PSR_PERF_CNT_MASK;
> > seq_printf(m, "Performance counter: %u\n", val);
> > }
> >
> > @@ -2563,8 +2564,11 @@ static int i915_edp_psr_status(struct
> > seq_file *m, void
> > *data)
> > * Reading all 3 registers before hand to minimize
> > crossing a
> > * frame boundary between register reads
> > */
> > - for (frame = 0; frame < PSR2_SU_STATUS_FRAMES; frame +=
> > 3)
> > - su_frames_val[frame / 3] =
> > I915_READ(PSR2_SU_STATUS(frame));
> > + for (frame = 0; frame < PSR2_SU_STATUS_FRAMES; frame +=
> > 3) {
> > + val = I915_READ(PSR2_SU_STATUS(dev_priv-
> > >psr.transcoder,
> > + frame));
> > + su_frames_val[frame / 3] = val;
> > + }
> >
> > seq_puts(m, "Frame:\tPSR2 SU blocks:\n");
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index dc74d33c20aa..89f2401e80a4 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -508,6 +508,7 @@ struct i915_psr {
> > bool enabled;
> > struct intel_dp *dp;
> > enum pipe pipe;
> > + enum transcoder transcoder;
> > bool active;
> > struct work_struct work;
> > unsigned busy_frontbuffer_bits;
> > @@ -1534,8 +1535,6 @@ struct drm_i915_private {
> > /* MMIO base address for MIPI regs */
> > u32 mipi_mmio_base;
> >
> > - u32 psr_mmio_base;
> > -
> > u32 pps_mmio_base;
> >
> > wait_queue_head_t gmbus_wait_queue;
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 31163415479d..572a9f661d60 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4212,10 +4212,19 @@ enum {
> > #define PIPESRC(trans) _MMIO_TRANS2(trans, _PIPEASRC)
> > #define PIPE_MULT(trans) _MMIO_TRANS2(trans, _PIPE_MULT_A)
> >
> > -/* HSW+ eDP PSR registers */
> > -#define HSW_EDP_PSR_BASE 0x64800
> > -#define BDW_EDP_PSR_BASE 0x6f800
> > -#define EDP_PSR_CTL _MMIO(dev_priv-
> > >psr_mmio_base +
> > 0)
> > +/*
> > + * HSW+ eDP PSR registers
> > + *
> > + * HSW PSR registers are relative to DDIA(_DDI_BUF_CTL_A + 0x800)
> > with just
> > one
> > + * instance of it
> > + */
> > +#define _HSW_EDP_PSR_BASE 0x64800
> > +#define _SRD_CTL_A 0x60800
>
> /* BDW+ */
> > +#define _SRD_CTL_EDP 0x6F800
> > +#define _HSW_PSR_ADJ(reg) ((reg) - _SRD_CTL_A +
> > _HSW_EDP_PSR_BASE)
> > +#define _PSR_ADJ(tran, reg) (IS_HASWELL(dev
> > _priv) ?
> > _HSW_PSR_ADJ(reg) : _TRANS2(tran, reg))
> > +#define _MMIO_PSR_ADJ(tran, reg) _MMIO(_PSR_ADJ(tran,
> > reg))
> nit: We can skip this macro
>
> > +#define EDP_PSR_CTL(tran) _MMIO_PSR_ADJ(tran,
> > _SRD_CTL_A)
> #define EDP_PSR_CTL(tran) _MMIO(_PSR_ADJ(tran,
> _SRD_CTL_A))
>
> It doesn't do much other than adding another layer.
>
> > #define EDP_PSR_ENABLE (1 << 31)
> > #define BDW_PSR_SINGLE_FRAME (1 << 30)
> > #define EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK (1 << 29) /* SW
> > can't
> > modify */
> > @@ -4252,16 +4261,22 @@ enum {
> > #define EDP_PSR_TRANSCODER_A_SHIFT 8
> > #define EDP_PSR_TRANSCODER_EDP_SHIFT 0
> >
> > -#define EDP_PSR_AUX_CTL _MMIO(dev_priv-
> > > psr_mmio_base + 0x10)
> > +#define _SRD_AUX_CTL_A 0x60810
> > +#define _SRD_AUX_CTL_EDP 0x6F810
> > +#define EDP_PSR_AUX_CTL(tran) _MMIO_PSR_ADJ(t
> > ran,
> > _SRD_AUX_CTL_A)
> > #define EDP_PSR_AUX_CTL_TIME_OUT_MASK (3 << 26)
> > #define EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK (0x1f << 20)
> > #define EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK (0xf << 16)
> > #define EDP_PSR_AUX_CTL_ERROR_INTERRUPT (1 << 11)
> > #define EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK (0x7ff)
> >
> > -#define EDP_PSR_AUX_DATA(i) _MMIO(dev_priv-
> > >psr_mmio_base +
> > 0x14 + (i) * 4) /* 5 registers */
> > +#define _SRD_AUX_DATA_A 0x60814
> > +#define _SRD_AUX_DATA_EDP 0x6F814
> > +#define EDP_PSR_AUX_DATA(tran, i) _MMIO(_PSR_ADJ(tran,
> > _SRD_AUX_DATA_A) + (i) + 4) /* 5 registers */
> >
> > -#define EDP_PSR_STATUS _MMIO(dev_priv-
> > > psr_mmio_base + 0x40)
> > +#define _SRD_STATUS_A 0x60840
> > +#define _SRD_STATUS_EDP 0x6F840
> The documentation recommends using lower case hex numbers.
Okay, done
>
> > +#define EDP_PSR_STATUS(tran) _MMIO_PSR_ADJ(t
> > ran,
> > _SRD_STATUS_A)
> > #define EDP_PSR_STATUS_STATE_MASK (7 << 29)
> > #define EDP_PSR_STATUS_STATE_SHIFT 29
> > #define EDP_PSR_STATUS_STATE_IDLE (0 << 29)
> > @@ -4286,10 +4301,15 @@ enum {
> > #define EDP_PSR_STATUS_SENDING_TP1 (1 << 4)
> > #define EDP_PSR_STATUS_IDLE_MASK 0xf
> >
> > -#define EDP_PSR_PERF_CNT _MMIO(dev_priv->psr_mmio_base +
> > 0x44)
> > +#define _SRD_PERF_CNT_A 0x60844
> > +#define _SRD_PERF_CNT_EDP 0x6F844
> > +#define EDP_PSR_PERF_CNT(tran) _MMIO_PSR_ADJ(tran,
> > _SRD_PERF_CNT_A)
> > #define EDP_PSR_PERF_CNT_MASK 0xffffff
> >
> > -#define EDP_PSR_DEBUG _MMIO(dev_priv-
> > > psr_mmio_base + 0x60) /* PSR_MASK on SKL+ */
> > +/* PSR_MASK on SKL+ */
> > +#define _SRD_DEBUG_A 0x60860
> > +#define _SRD_DEBUG_EDP 0x6F860
> > +#define EDP_PSR_DEBUG(tran) _MMIO_PSR_ADJ(t
> > ran,
> > _SRD_DEBUG_A)
> > #define EDP_PSR_DEBUG_MASK_MAX_SLEEP (1 << 28)
> > #define EDP_PSR_DEBUG_MASK_LPSP (1 << 27)
> > #define EDP_PSR_DEBUG_MASK_MEMUP (1 << 26)
> > @@ -4297,7 +4317,9 @@ enum {
> > #define EDP_PSR_DEBUG_MASK_DISP_REG_WRITE (1 << 16) /*
> > Reserved in ICL+
> > */
> > #define EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15) /* SKL+
> > */
> >
> > -#define EDP_PSR2_CTL _MMIO(0x6f900)
> > +#define _PSR2_CTL_A 0x60900
> > +#define _PSR2_CTL_EDP 0x6F900
> > +#define EDP_PSR2_CTL(tran) _MMIO_TRANS2(tran, _PSR2_CTL_A)
> > #define EDP_PSR2_ENABLE (1 << 31)
> > #define EDP_SU_TRACK_ENABLE (1 << 30)
> > #define EDP_Y_COORDINATE_VALID (1 << 26) /* GLK and CNL+ */
> > @@ -4320,7 +4342,7 @@ enum {
> > #define _PSR_EVENT_TRANS_C 0x62848
> > #define _PSR_EVENT_TRANS_D 0x63848
> > #define _PSR_EVENT_TRANS_EDP 0x6F848
> > -#define PSR_EVENT(trans) _MMIO_TRANS2(trans,
> > _PSR_EVENT_TRANS_A)
> > +#define PSR_EVENT(tran) _MMIO_TRANS2(tr
> > an,
> > _PSR_EVENT_TRANS_A)
> > #define PSR_EVENT_PSR2_WD_TIMER_EXPIRE (1 << 17)
> > #define PSR_EVENT_PSR2_DISABLED (1 << 16)
> > #define PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN (1 << 15)
> > @@ -4338,15 +4360,16 @@ enum {
> > #define PSR_EVENT_LPSP_MODE_EXIT (1 << 1)
> > #define PSR_EVENT_PSR_DISABLE (1 << 0)
> >
> > -#define EDP_PSR2_STATUS _MMIO(0x6f940)
> > +#define _PSR2_STATUS_A 0x60940
> > +#define _PSR2_STATUS_EDP 0x6F940
> > +#define EDP_PSR2_STATUS(tran) _MMIO_TRANS2(tran,
> > _PSR2_STATUS_A)
> > #define EDP_PSR2_STATUS_STATE_MASK (0xf << 28)
> > #define EDP_PSR2_STATUS_STATE_SHIFT 28
> >
> > -#define _PSR2_SU_STATUS_0 0x6F914
> > -#define _PSR2_SU_STATUS_1 0x6F918
> > -#define _PSR2_SU_STATUS_2 0x6F91C
> > -#define _PSR2_SU_STATUS(index) _MMIO(_PICK_EVEN((index
> > ),
> > _PSR2_SU_STATUS_0, _PSR2_SU_STATUS_1))
> > -#define PSR2_SU_STATUS(frame) (_PSR2_SU_STATUS((frame
> > ) / 3))
> > +#define _PSR2_SU_STATUS_A 0x60914
> > +#define _PSR2_SU_STATUS_EDP 0x6F914
> > +#define _PSR2_SU_STATUS(tran, index) _MMIO(_TRANS2(tran,
> > _PSR2_SU_STATUS_A) +
> > (index) * 4)
> > +#define PSR2_SU_STATUS(tran, frame) (_PSR2_SU_STATUS(tran,
> > (frame) / 3))
> > #define PSR2_SU_STATUS_SHIFT(frame) (((frame) % 3) * 10)
> > #define PSR2_SU_STATUS_MASK(frame) (0x3ff <<
> > PSR2_SU_STATUS_SHIFT(frame))
> > #define PSR2_SU_STATUS_FRAMES 8
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 963663ba0edf..fe98041819ec 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
>
> Now that we track the transcoder, intel_psr_irq_control() can be
> modified
> to enable debug interrupts only that transcoder, can be done as a
> follow up.
Sure, will be done in a follow up patch.
>
>
> > @@ -399,7 +399,7 @@ static void hsw_psr_setup_aux(struct intel_dp
> > *intel_dp)
> >
> > BUILD_BUG_ON(sizeof(aux_msg) > 20);
> > for (i = 0; i < sizeof(aux_msg); i += 4)
> > - I915_WRITE(EDP_PSR_AUX_DATA(i >> 2),
> > + I915_WRITE(EDP_PSR_AUX_DATA(dev_priv->psr.transcoder, i
> > >> 2),
> > intel_dp_pack_aux(&aux_msg[i],
> > sizeof(aux_msg) - i));
> >
> > aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp,
> > 0);
> > @@ -410,7 +410,7 @@ static void hsw_psr_setup_aux(struct intel_dp
> > *intel_dp)
> >
> > /* Select only valid bits for SRD_AUX_CTL */
> > aux_ctl &= psr_aux_mask;
> > - I915_WRITE(EDP_PSR_AUX_CTL, aux_ctl);
> > + I915_WRITE(EDP_PSR_AUX_CTL(dev_priv->psr.transcoder), aux_ctl);
> > }
> >
> > static void intel_psr_enable_sink(struct intel_dp *intel_dp)
> > @@ -500,8 +500,9 @@ static void hsw_activate_psr1(struct intel_dp
> > *intel_dp)
> > if (INTEL_GEN(dev_priv) >= 8)
> > val |= EDP_PSR_CRC_ENABLE;
> >
> > - val |= I915_READ(EDP_PSR_CTL) &
> > EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK;
> > - I915_WRITE(EDP_PSR_CTL, val);
> > + val |= (I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder)) &
> > + EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK);
> > + I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), val);
> > }
> >
> > static void hsw_activate_psr2(struct intel_dp *intel_dp)
> > @@ -537,9 +538,9 @@ static void hsw_activate_psr2(struct intel_dp
> > *intel_dp)
> > * PSR2 HW is incorrectly using EDP_PSR_TP1_TP3_SEL and BSpec
> > is
> > * recommending keep this bit unset while PSR2 is enabled.
> > */
> > - I915_WRITE(EDP_PSR_CTL, 0);
> > + I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), 0);
> >
> > - I915_WRITE(EDP_PSR2_CTL, val);
> > + I915_WRITE(EDP_PSR2_CTL(dev_priv->psr.transcoder), val);
> > }
> >
> > static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
> > @@ -658,8 +659,8 @@ static void intel_psr_activate(struct intel_dp
> > *intel_dp)
> > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> >
> > if (INTEL_GEN(dev_priv) >= 9)
> > - WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
> > - WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > + WARN_ON(I915_READ(EDP_PSR2_CTL(dev_priv-
> > >psr.transcoder)) &
> > EDP_PSR2_ENABLE);
> > + WARN_ON(I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder)) &
> > EDP_PSR_ENABLE);
> > WARN_ON(dev_priv->psr.active);
> > lockdep_assert_held(&dev_priv->psr.lock);
> >
> /*
> * HSW spec explicitly says PSR is tied to port A.
> * BDW+ platforms with DDI implementation of PSR have
> different
> * PSR registers per transcoder and we only implement
> transcoder EDP
> * ones. Since by Display design transcoder EDP is tied to
> port A
> * we can safely escape based on the port A.
> */
> if (dig_port->base.port != PORT_A) {
> DRM_DEBUG_KMS("PSR condition failed: Port not
> supported\n");
> return;
> }
>
> Now that we allow PSR on transcoders that support it, we can change
> this to
> if (IS_HASWELL() && cpu_transcoder != TRANSCODER_EDP)
> return;
>
> This can be a follow up work.
Will be done in a follow up patch.
>
>
> > @@ -729,7 +730,7 @@ static void intel_psr_enable_source(struct
> > intel_dp
> > *intel_dp,
> > if (INTEL_GEN(dev_priv) < 11)
> > mask |= EDP_PSR_DEBUG_MASK_DISP_REG_WRITE;
> >
> > - I915_WRITE(EDP_PSR_DEBUG, mask);
> > + I915_WRITE(EDP_PSR_DEBUG(dev_priv->psr.transcoder), mask);
> > }
> >
> > static void intel_psr_enable_locked(struct drm_i915_private
> > *dev_priv,
> > @@ -742,6 +743,7 @@ static void intel_psr_enable_locked(struct
> > drm_i915_private *dev_priv,
> > dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv,
> > crtc_state);
> > dev_priv->psr.busy_frontbuffer_bits = 0;
> > dev_priv->psr.pipe = to_intel_crtc(crtc_state->base.crtc)-
> > >pipe;
> > + dev_priv->psr.transcoder = crtc_state->cpu_transcoder;
> >
> > DRM_DEBUG_KMS("Enabling PSR%s\n",
> > dev_priv->psr.psr2_enabled ? "2" : "1");
> > @@ -791,20 +793,27 @@ static void intel_psr_exit(struct
> > drm_i915_private
> > *dev_priv)
> > u32 val;
> >
> > if (!dev_priv->psr.active) {
> > - if (INTEL_GEN(dev_priv) >= 9)
> > - WARN_ON(I915_READ(EDP_PSR2_CTL) &
> > EDP_PSR2_ENABLE);
> > - WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > + if (INTEL_GEN(dev_priv) >= 9) {
> > + val = I915_READ(EDP_PSR2_CTL(dev_priv-
> > >psr.transcoder));
> > + WARN_ON(val & EDP_PSR2_ENABLE);
> > + }
> > +
> > + val = I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder));
> > + WARN_ON(val & EDP_PSR_ENABLE);
> > +
> > return;
> > }
> >
> > if (dev_priv->psr.psr2_enabled) {
> > - val = I915_READ(EDP_PSR2_CTL);
> > + val = I915_READ(EDP_PSR2_CTL(dev_priv-
> > >psr.transcoder));
> > WARN_ON(!(val & EDP_PSR2_ENABLE));
> > - I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
> > + val &= ~EDP_PSR2_ENABLE;
> > + I915_WRITE(EDP_PSR2_CTL(dev_priv->psr.transcoder),
> > val);
> > } else {
> > - val = I915_READ(EDP_PSR_CTL);
> > + val = I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder));
> > WARN_ON(!(val & EDP_PSR_ENABLE));
> > - I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
> > + val &= ~EDP_PSR_ENABLE;
> > + I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), val);
> > }
> > dev_priv->psr.active = false;
> > }
> > @@ -826,10 +835,10 @@ static void intel_psr_disable_locked(struct
> > intel_dp
> > *intel_dp)
> > intel_psr_exit(dev_priv);
> >
> > if (dev_priv->psr.psr2_enabled) {
> > - psr_status = EDP_PSR2_STATUS;
> > + psr_status = EDP_PSR2_STATUS(dev_priv->psr.transcoder);
> > psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> > } else {
> > - psr_status = EDP_PSR_STATUS;
> > + psr_status = EDP_PSR_STATUS(dev_priv->psr.transcoder);
> > psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
> > }
> >
> > @@ -956,7 +965,8 @@ int intel_psr_wait_for_idle(const struct
> > intel_crtc_state
> > *new_crtc_state,
> > * defensive enough to cover everything.
> > */
> >
> > - return __intel_wait_for_register(&dev_priv->uncore,
> > EDP_PSR_STATUS,
> > + return __intel_wait_for_register(&dev_priv->uncore,
> > + EDP_PSR_STATUS(dev_priv-
> > > psr.transcoder),
> > EDP_PSR_STATUS_STATE_MASK,
> > EDP_PSR_STATUS_STATE_IDLE, 2,
> > 50,
> > out_value);
> > @@ -972,10 +982,10 @@ static bool __psr_wait_for_idle_locked(struct
> > drm_i915_private *dev_priv)
> > return false;
> >
> > if (dev_priv->psr.psr2_enabled) {
> > - reg = EDP_PSR2_STATUS;
> > + reg = EDP_PSR2_STATUS(dev_priv->psr.transcoder);
> > mask = EDP_PSR2_STATUS_STATE_MASK;
> > } else {
> > - reg = EDP_PSR_STATUS;
> > + reg = EDP_PSR_STATUS(dev_priv->psr.transcoder);
> > mask = EDP_PSR_STATUS_STATE_MASK;
> > }
> >
> > @@ -1206,9 +1216,6 @@ void intel_psr_init(struct drm_i915_private
> > *dev_priv)
> > if (!HAS_PSR(dev_priv))
> > return;
> >
> > - dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ?
> > - HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE;
> > -
> > if (!dev_priv->psr.sink_support)
> > return;
>
> Right below this, we have
> val = I915_READ(EDP_PSR_IIR);
> val &= EDP_PSR_ERROR(edp_psr_shift(TRANSCODER_EDP));
> if (val) {
> DRM_DEBUG_KMS("PSR interruption error set\n");
> dev_priv->psr.sink_not_reliable = true;
> }
>
> The workaround checks only eDP transcoders, fix it to check all
> transcoders? We
> are still going to be assume only one eDP sink is connected, but it
> can be
> driven by any transcoder.
Good catch, thinking again I guess is better move it to:
intel_psr_enable_locked() and check against the transcoder that will be
used.
>
>
> >
More information about the Intel-gfx
mailing list