[PATCH 6/8] drm/i915: Enable/Disable PSR

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

So now I finally have the correct spec in hands!

2013/2/25 Rodrigo Vivi <rodrigo.vivi at gmail.com>:
> Adding Enable and Disable PSR functionalities. This includes setting the
> PSR configuration over AUX, sending SDP VSC DIP over the eDP PIPE config,
> enabling PSR in the sink via DPCD register and finally enabling PSR on
> the host.
>
> This patch is heavily based on initial PSR code by Sateesh Kavuri and
> Kumar Shobhit but in a different implementation.
>
> Credits-by: Sateesh Kavuri <sateesh.kavuri at intel.com>
> Credits-by: Shobhit Kumar <shobhit.kumar at intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  40 +++++++++
>  drivers/gpu/drm/i915/intel_dp.c  | 183 +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |   3 +
>  3 files changed, 226 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b715ecd..1e31f23 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -844,6 +844,8 @@
>  #define   SNB_CPU_FENCE_ENABLE (1<<29)
>  #define DPFC_CPU_FENCE_OFFSET  0x100104
>
> +/* Framebuffer compression for Haswell */
> +#define HSW_FBC_CONTROL                0x43208
>
>  /*
>   * GPIO regs
> @@ -1580,6 +1582,44 @@
>  #define BCLRPAT(pipe) _PIPE(pipe, _BCLRPAT_A, _BCLRPAT_B)
>  #define VSYNCSHIFT(trans) _TRANSCODER(trans, _VSYNCSHIFT_A, _VSYNCSHIFT_B)
>
> +/* HSW eDP PSR registers */
> +#define EDP_PSR_CTL                            0x64800
> +#define   EDP_PSR_ENABLE                       (1<<31)
> +#define   EDP_PSR_LINK_DISABLE                 (0<<27)
> +#define   EDP_PSR_LINK_STANDBY                 (1<<27)
> +#define   EDP_PSR_MIN_LINK_ENTRY_TIME_MASK     (3<<25)
> +#define   EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES  (0<<25)
> +#define   EDP_PSR_MIN_LINK_ENTRY_TIME_4_LINES  (1<<25)
> +#define   EDP_PSR_MIN_LINK_ENTRY_TIME_2_LINES  (2<<25)
> +#define   EDP_PSR_MIN_LINK_ENTRY_TIME_0_LINES  (3<<25)
> +#define   EDP_PSR_MAX_SLEEP_TIME_SHIFT         20
> +#define   EDP_PSR_SKIP_AUX_EXIT                        (1<<12)
> +#define   EDP_PSR_TP1_TP2_SEL                  (0<<11)
> +#define   EDP_PSR_TP1_TP3_SEL                  (1<<11)
> +#define   EDP_PSR_TP2_TP3_TIME_500us           (0<<8)
> +#define   EDP_PSR_TP2_TP3_TIME_100us           (1<<8)
> +#define   EDP_PSR_TP2_TP3_TIME_2500us          (2<<8)
> +#define   EDP_PSR_TP2_TP3_TIME_0us             (3<<8)
> +#define   EDP_PSR_TP1_TIME_500us               (0<<4)
> +#define   EDP_PSR_TP1_TIME_100us               (1<<4)
> +#define   EDP_PSR_TP1_TIME_2500us              (2<<4)
> +#define   EDP_PSR_TP1_TIME_0us                 (3<<4)
> +#define   EDP_PSR_IDLE_FRAME_SHIFT             0
> +
> +#define EDP_PSR_AUX_CTL                        0x64810
> +#define EDP_PSR_AUX_DATA1              0x64814
> +#define EDP_PSR_AUX_DATA2              0x64818
> +#define EDP_PSR_AUX_DATA3              0x6481c
> +#define EDP_PSR_AUX_DATA4              0x64820
> +#define EDP_PSR_AUX_DATA5              0x64824
> +
> +#define EDP_PSR_STATUS_CTL                     0x64840
> +#define   EDP_PSR_STATUS_STATE_MASK            (7<<29)
> +
> +#define EDP_PSR_DEBUG_CTL              0x64860
> +#define   EDP_PSR_DEBUG_MASK_MEMUP     (1<<26)
> +#define   EDP_PSR_DEBUG_MASK_HPD       (1<<25)
> +
>  /* VGA port control */
>  #define ADPA                   0x61100
>  #define PCH_ADPA                0xe1100
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 5cfa9f4..a420f0d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -83,6 +83,13 @@ static struct drm_device *intel_dp_to_dev(struct intel_dp *intel_dp)
>         return intel_dig_port->base.base.dev;
>  }
>
> +static struct drm_crtc *intel_dp_to_crtc(struct intel_dp *intel_dp)
> +{
> +       struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +
> +       return intel_dig_port->base.base.crtc;
> +}
> +
>  static struct intel_dp *intel_attached_dp(struct drm_connector *connector)
>  {
>         return enc_to_intel_dp(&intel_attached_encoder(connector)->base);
> @@ -1443,6 +1450,182 @@ static bool is_edp_psr(struct intel_dp *intel_dp)
>                 (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED));
>  }
>
> +static bool
> +intel_edp_is_psr_enabled(struct intel_dp* intel_dp)
> +{
> +       struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       return I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
> +}
> +
> +static void
> +intel_edp_psr_enable_src(struct intel_dp *intel_dp)
> +{
> +       struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       uint32_t max_sleep_time = 0x1f;
> +       uint32_t val = 0x0;
> +
> +       if (dev_priv->vbt.psr_idle_frames)
> +           val |= dev_priv->vbt.psr_idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
> +       else
> +           val |= 1 << EDP_PSR_IDLE_FRAME_SHIFT;
> +
> +       if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT) {
> +               val |= EDP_PSR_LINK_STANDBY;

If we set this bit, shouldn't we also set DPCD 0x170 bit 1 to 1?

> +               val |= EDP_PSR_TP2_TP3_TIME_0us;
> +               val |= EDP_PSR_TP1_TIME_0us;
> +               val |= EDP_PSR_SKIP_AUX_EXIT;
> +       } else {
> +               val |= EDP_PSR_LINK_DISABLE;
> +               /* Use these Values from VBT
> +                * Case values are timings for HSW as of now
> +                * in multiple of 100us
> +                */
> +               switch(dev_priv->vbt.psr_wakeup_tp1) {
> +                       case 1:
> +                               val |= EDP_PSR_TP1_TIME_100us;
> +                               break;
> +                       case 5:
> +                               val |= EDP_PSR_TP1_TIME_500us;
> +                               break;
> +                       case 25:
> +                               val |= EDP_PSR_TP1_TIME_2500us;
> +                               break;
> +                       default:
> +                               val |= EDP_PSR_TP1_TIME_500us;
> +                               break;
> +               };
> +               switch(dev_priv->vbt.psr_wakeup_tp2_tp3) {
> +                       case 1:
> +                               val |= EDP_PSR_TP2_TP3_TIME_100us;
> +                               break;
> +                       case 5:
> +                               val |= EDP_PSR_TP2_TP3_TIME_500us;
> +                               break;
> +                       case 25:
> +                               val |= EDP_PSR_TP2_TP3_TIME_2500us;
> +                               break;
> +                       default:
> +                               val |= EDP_PSR_TP2_TP3_TIME_500us;
> +                               break;
> +               };
> +       }
> +
> +       I915_WRITE(EDP_PSR_CTL, val |
> +                  EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES |
> +                  max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT |
> +                  EDP_PSR_ENABLE);
> +}
> +
> +void intel_edp_write_vsc_psr(struct intel_dp* intel_dp,
> +                            struct edp_vsc_psr *vsc_psr)
> +{
> +       struct drm_device *dev =  intel_dp_to_dev(intel_dp);
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct intel_crtc *intel_crtc =  to_intel_crtc(intel_dp_to_crtc(intel_dp));

I'd reorganize the declarations to avoid calling both intel_dp_to_crtc
and intel_dp_to_dev :)

> +       u32 ctl_reg = HSW_TVIDEO_DIP_CTL(intel_crtc->cpu_transcoder);

Please don't preserve the register contents here. We don't use this
register anywhere else for DP yet.

> +       u32 data_reg = HSW_TVIDEO_DIP_VSC_DATA(intel_crtc->cpu_transcoder);
> +       uint32_t *data = (uint32_t *)vsc_psr;
> +       unsigned int i;
> +       u32 val = I915_READ(ctl_reg);
> +
> +       if (data_reg == 0)
> +               return;

I don't think this is ever expected, so you could probably WARN() or
BUG() here. I see this statement comes from hsw_write_infoframe
(intel_hdmi.c), we should probably try to do the WARN/BUG there too.

> +
> +       /* As per eDP spec, wait for vblank to send SDP VSC packet */
> +       intel_wait_for_vblank(dev, intel_crtc->pipe);
> +

Please see BSpec page "Pipe Video Data Island Packet". After waiting
the vsync you need to disable the DIP first, then write the data.

> +       mmiowb();
> +       for (i = 0; i < sizeof(struct edp_vsc_psr); i += 4) {
> +               I915_WRITE(data_reg + i, *data);
> +               data++;
> +       }
> +       /* Write every possible data byte to force correct ECC calculation. */
> +       for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
> +               I915_WRITE(data_reg + i, 0);
> +       mmiowb();
> +
> +       val |= VIDEO_DIP_ENABLE_VSC_HSW;
> +       I915_WRITE(ctl_reg, val);
> +       POSTING_READ(ctl_reg);

According to the PSR enabling guide, "HW also enables VSC DIP when
required", so I guess the 3 lines above are not needed.

> +}
> +
> +void intel_edp_enable_psr(struct intel_dp* intel_dp)
> +{
> +       struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct edp_vsc_psr psr_vsc;
> +       int msg_size = 5;       /* Header(4) + Message(1) */
> +
> +       if (!is_edp_psr(intel_dp) || intel_edp_is_psr_enabled(intel_dp))

Is it possible to have intel_edp_is_psr_enable here? If not, maybe WARN it.

> +               return;
> +
> +       /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */
> +       memset(&psr_vsc, 0, sizeof(psr_vsc));
> +       psr_vsc.sdp_header.id = 0;
> +       psr_vsc.sdp_header.type = 0x7;
> +       psr_vsc.sdp_header.revision = 0x2;
> +       psr_vsc.sdp_header.valid_payload_bytes = 0x8;
> +
> +       intel_edp_write_vsc_psr(intel_dp, &psr_vsc);
> +
> +       /* Enable PSR in sink by setting bit 0 in DPCD config reg
> +        * along with the transmitter state during PSR active
> +        * Transmitter state later can be ready from VBT. As of now
> +        * program the full link down

This comment is probably missing a few periods.

> +        *
> +        */
> +       intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
> +                                   DP_PSR_ENABLE);
> +
> +       /* Setup AUX registers */
> +       /* Write command on DPCD 0x0600 */
> +       I915_WRITE(EDP_PSR_AUX_DATA1, 0x80060000);
> +
> +       /* Set the state to normal operation D0 in DPCD 0x0600 */
> +       I915_WRITE(EDP_PSR_AUX_DATA2, 0x01000000);

I see the lines above match our spec, but I still think Jani's comment
from patch "drm/i915: Setup EDP PSR AUX Registers" is applicable here.

> +
> +       I915_WRITE(EDP_PSR_AUX_CTL,
> +                  msg_size << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT |
> +                  I915_READ(DPA_AUX_CH_CTL));

I don't think this is safe, especially since we're not erasing the
original msg size bits from DPA_AUX_CH_CTL. I think we should try to
set all the bits of this register without reading DPA_AUX_CH_CTL
(maybe extract some code from intel_dp_aux_ch into new functions and
call from both here and intel_dp_aux_ch).

Also, I still think you should patch intel_dp_aux_ch to put a WARN in
case EDP_PSR_CTL bit 31 is 1.

> +
> +       /* Enable PSR on the host */
> +       intel_edp_psr_enable_src(intel_dp);
> +
> +       /* WA: FBC/PSR Underrun/Corruption Workaround */
> +       I915_WRITE(HSW_FBC_CONTROL, ~FBC_CTL_EN);

This FBC WA won't be needed on real world Hardware.

> +
> +       /* WA: Continuous PSR exit Workaround */
> +       I915_WRITE(EDP_PSR_DEBUG_CTL, EDP_PSR_DEBUG_MASK_MEMUP);
> +       I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);

All they ask is to "disabled unused interrupts", not "disable all interrupts".

> +}
> +
> +void intel_edp_disable_psr(struct intel_dp* intel_dp)
> +{
> +       struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct intel_crtc *intel_crtc = to_intel_crtc(intel_dp_to_crtc(intel_dp));
> +       uint32_t reg = HSW_TVIDEO_DIP_CTL(intel_crtc->cpu_transcoder);
> +       uint32_t val;
> +
> +       if (!intel_edp_is_psr_enabled(intel_dp))

Is this possible? Should we WARN here?

> +               return;
> +
> +       val = I915_READ(EDP_PSR_CTL);
> +       I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
> +
> +       /* Wait till PSR is idle */
> +       if (_wait_for((I915_READ(EDP_PSR_STATUS_CTL) &
> +                      EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10))

Some big sleeps here. Can't we make them faster?

> +               DRM_ERROR("Timed out waiting for PSR Idle State\n");
> +
> +       intel_wait_for_vblank(dev, intel_crtc->pipe);
> +
> +       val = I915_READ(reg);
> +       I915_WRITE(reg, val & ~VIDEO_DIP_ENABLE_VSC_HSW);

So, based on this and on the spec, it means the HW will automagically
do the link training for us if it's needed?

> +}
> +
>  static void intel_enable_dp(struct intel_encoder *encoder)
>  {
>         struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 865ede6..cfda739 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -696,4 +696,7 @@ extern bool
>  intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector);
>  extern void intel_ddi_fdi_disable(struct drm_crtc *crtc);
>
> +extern void intel_edp_enable_psr(struct intel_dp* intel_dp);
> +extern void intel_edp_disable_psr(struct intel_dp* intel_dp);
> +
>  #endif /* __INTEL_DRV_H__ */
> --
> 1.8.1.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Paulo Zanoni


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux