On Monday 09 January 2017 10:47 PM, Vivi, Rodrigo wrote:
> On Mon, 2017-01-09 at 18:26 +0530, vathsala nagaraju wrote:
>> Psr1 and psr2 are mutually exclusive,ie when psr2 is enabled,
>> psr1 should be disabled.When psr2 is exited , bit 31 of reg
>> PSR2_CTL must be set to 0 but currently bit 31 of SRD_CTL
>> (psr1 control register)is set to 0.
>> Also ,PSR2_IDLE state is looked up from SRD_STATUS(psr1 register)
>> instead of PSR2_STATUS register, which has wrong data, resulting
>> in blankscreen.
>> hsw_enable_source is split into hsw_enable_source_psr1 and
>> hsw_enable_source_psr2 for easier code review and maintenance,
>> as suggested by rodrigo and jim.
>>
>> v2: (Rodrigo)
>> - Rename hsw_enable_source_psr* to intel_enable_source_psr*
>>
>> v3: (Rodrigo)
>> - In hsw_psr_disable ,
>> 1) for psr active case, handle psr2 followed by psr1.
>> 2) psr inactive case, handle psr2 followed by psr1
> much better, thanks, but still has one blocking issue imho:
>
>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> Cc: Jim Bride <jim.bride at linux.intel.com>
>> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju at intel.com>
>> Signed-off-by: Patil Deepti <deepti.patil at intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_reg.h | 3 +
>> drivers/gpu/drm/i915/intel_psr.c | 126
>> +++++++++++++++++++++++++++++----------
>> 2 files changed, 97 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index 00970aa..7830e6e 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -3615,6 +3615,9 @@ enum {
>> #define EDP_PSR2_FRAME_BEFORE_SU_MASK (0xf<<4)
>> #define EDP_PSR2_IDLE_MASK 0xf
>>
>> +#define EDP_PSR2_STATUS_CTL _MMIO(0x6f940)
>> +#define EDP_PSR2_STATUS_STATE_MASK (0xf<<28)
>> +
>> /* VGA port control */
>> #define ADPA _MMIO(0x61100)
>> #define PCH_ADPA _MMIO(0xe1100)
>> diff --git a/drivers/gpu/drm/i915/intel_psr.c
>> b/drivers/gpu/drm/i915/intel_psr.c
>> index c3aa649..6c161aa 100644
>> --- a/drivers/gpu/drm/i915/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/intel_psr.c
>> @@ -261,12 +261,11 @@ static void vlv_psr_activate(struct intel_dp *intel_dp)
>> VLV_EDP_PSR_ACTIVE_ENTRY);
>> }
>>
>> -static void hsw_psr_enable_source(struct intel_dp *intel_dp)
>> +static void intel_enable_source_psr1(struct intel_dp *intel_dp)
>> {
>> struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> struct drm_device *dev = dig_port->base.base.dev;
>> struct drm_i915_private *dev_priv = to_i915(dev);
>> -
>> uint32_t max_sleep_time = 0x1f;
>> /*
>> * Let's respect VBT in case VBT asks a higher idle_frame value.
>> @@ -312,14 +311,30 @@ static void hsw_psr_enable_source(struct intel_dp
>> *intel_dp)
>> val |= EDP_PSR_TP1_TP2_SEL;
>>
>> I915_WRITE(EDP_PSR_CTL, val);
>> +}
>>
>> - if (!dev_priv->psr.psr2_support)
>> - return;
>> +static void intel_enable_source_psr2(struct intel_dp *intel_dp)
>> +{
>> + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> + struct drm_device *dev = dig_port->base.base.dev;
>> + struct drm_i915_private *dev_priv = to_i915(dev);
>> +
>> + /*
>> + * Let's respect VBT in case VBT asks a higher idle_frame value.
>> + * Let's use 6 as the minimum to cover all known cases including
>> + * the off-by-one issue that HW has in some cases. Also there are
>> + * cases where sink should be able to train
>> + * with the 5 or 6 idle patterns.
>> + */
>> + uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
>> + uint32_t val = EDP_PSR_ENABLE;
>> +
>> + val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
>>
>> /* FIXME: selective update is probably totally broken because it doesn't
>> * mesh at all with our frontbuffer tracking. And the hw alone isn't
>> * good enough. */
>> - val = EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
>> + val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
>>
>> if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
>> val |= EDP_PSR2_TP2_TIME_2500;
>> @@ -333,6 +348,20 @@ static void hsw_psr_enable_source(struct intel_dp
>> *intel_dp)
>> I915_WRITE(EDP_PSR2_CTL, val);
>> }
>>
>> +
>> +static void hsw_psr_enable_source(struct intel_dp *intel_dp)
>> +{
>> + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> + struct drm_device *dev = dig_port->base.base.dev;
>> + struct drm_i915_private *dev_priv = to_i915(dev);
>> +
>> + /* psr1 and psr2 are mutually exclusive.*/
>> + if (dev_priv->psr.psr2_support)
>> + intel_enable_source_psr2(intel_dp);
>> + else
>> + intel_enable_source_psr1(intel_dp);
>> +}
>> +
>> static bool intel_psr_match_conditions(struct intel_dp *intel_dp)
>> {
>> struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> @@ -410,7 +439,10 @@ static void intel_psr_activate(struct intel_dp
>> *intel_dp)
>> struct drm_device *dev = intel_dig_port->base.base.dev;
>> struct drm_i915_private *dev_priv = to_i915(dev);
>>
>> - WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
>> + if (dev_priv->psr.psr2_support)
>> + WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
>> + else
>> + WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
>> WARN_ON(dev_priv->psr.active);
>> lockdep_assert_held(&dev_priv->psr.lock);
>>
>> @@ -462,8 +494,6 @@ void intel_psr_enable(struct intel_dp *intel_dp)
>> dev_priv->psr.busy_frontbuffer_bits = 0;
>>
>> if (HAS_DDI(dev_priv)) {
>> - hsw_psr_setup_vsc(intel_dp);
>> -
>> if (dev_priv->psr.psr2_support) {
>> /* PSR2 is restricted to work with panel resolutions
>> upto 3200x2000 */
>> if (crtc->config->pipe_src_w > 3200 ||
>> @@ -471,8 +501,10 @@ void intel_psr_enable(struct intel_dp *intel_dp)
>> dev_priv->psr.psr2_support = false;
> + hsw_psr_setup_vsc(intel_dp);
>
> without this you will end up enabling psr1 but not setting up the vsc.
>
>
> But also if I understood correctly this will fail anyways right?
> So before this patch we need one that move this restriction to
> match_conditions function returning false and fully blocking PSR.
thanks. I have posted a new patch. please review
>
>> else
>> skl_psr_setup_su_vsc(intel_dp);
>> + } else {
>> + /* set up vsc header for psr1 */
>> + hsw_psr_setup_vsc(intel_dp);
>> }
>> -
>> /*
>> * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD.
>> * Also mask LPSP to avoid dependency on other drivers that
>> @@ -557,20 +589,35 @@ static void hsw_psr_disable(struct intel_dp *intel_dp)
>> struct drm_i915_private *dev_priv = to_i915(dev);
>>
>> if (dev_priv->psr.active) {
>> - I915_WRITE(EDP_PSR_CTL,
>> - I915_READ(EDP_PSR_CTL) & ~EDP_PSR_ENABLE);
>> -
>> - /* Wait till PSR is idle */
>> - if (intel_wait_for_register(dev_priv,
>> - EDP_PSR_STATUS_CTL,
>> - EDP_PSR_STATUS_STATE_MASK,
>> - 0,
>> - 2000))
>> + if (dev_priv->psr.psr2_support) {
>> + I915_WRITE(EDP_PSR2_CTL,
>> + I915_READ(EDP_PSR2_CTL) &
>> + ~(EDP_PSR2_ENABLE |
>> + EDP_SU_TRACK_ENABLE));
>> + /* Wait till PSR2 is idle */
>> + if (intel_wait_for_register(dev_priv,
>> + EDP_PSR2_STATUS_CTL,
>> + EDP_PSR2_STATUS_STATE_MASK,
>> + 0,
>> + 2000))
>> + DRM_ERROR("Timed out waiting for PSR2 Idle State\n");
>> + } else {
>> + I915_WRITE(EDP_PSR_CTL,
>> + I915_READ(EDP_PSR_CTL) & ~EDP_PSR_ENABLE);
>> + /* Wait till PSR1 is idle */
>> + if (intel_wait_for_register(dev_priv,
>> + EDP_PSR_STATUS_CTL,
>> + EDP_PSR_STATUS_STATE_MASK,
>> + 0,
>> + 2000))
>> DRM_ERROR("Timed out waiting for PSR Idle State\n");
>> -
>> + }
>> dev_priv->psr.active = false;
>> } else {
>> - WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
>> + if (dev_priv->psr.psr2_support)
>> + WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
>> + else
>> + WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
>> }
>> }
>>
>> @@ -621,13 +668,24 @@ static void intel_psr_work(struct work_struct *work)
>> * and be ready for re-enable.
>> */
>> if (HAS_DDI(dev_priv)) {
>> - if (intel_wait_for_register(dev_priv,
>> - EDP_PSR_STATUS_CTL,
>> - EDP_PSR_STATUS_STATE_MASK,
>> - 0,
>> - 50)) {
>> - DRM_ERROR("Timed out waiting for PSR Idle for
>> re-enable\n");
>> - return;
>> + if (dev_priv->psr.psr2_support) {
>> + if (intel_wait_for_register(dev_priv,
>> + EDP_PSR2_STATUS_CTL,
>> + EDP_PSR2_STATUS_STATE_MASK,
>> + 0,
>> + 50)) {
>> + DRM_ERROR("Timed out waiting for PSR2 Idle for
>> re-enable\n");
>> + return;
>> + }
>> + } else {
>> + if (intel_wait_for_register(dev_priv,
>> + EDP_PSR_STATUS_CTL,
>> + EDP_PSR_STATUS_STATE_MASK,
>> + 0,
>> + 50)) {
>> + DRM_ERROR("Timed out waiting for PSR Idle for
>> re-enable\n");
>> + return;
>> + }
>> }
>> } else {
>> if (intel_wait_for_register(dev_priv,
>> @@ -669,11 +727,15 @@ static void intel_psr_exit(struct drm_i915_private
>> *dev_priv)
>> return;
>>
>> if (HAS_DDI(dev_priv)) {
>> - val = I915_READ(EDP_PSR_CTL);
>> -
>> - WARN_ON(!(val & EDP_PSR_ENABLE));
>> -
>> - I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
>> + if (dev_priv->psr.psr2_support) {
>> + val = I915_READ(EDP_PSR2_CTL);
>> + WARN_ON(!(val & EDP_PSR2_ENABLE));
>> + I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
>> + } else {
>> + val = I915_READ(EDP_PSR_CTL);
>> + WARN_ON(!(val & EDP_PSR_ENABLE));
>> + I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
>> + }
>> } else {
>> val = I915_READ(VLV_PSRCTL(pipe));
>>
