On Wed, Nov 19, 2014 at 11:22 AM, R, Durgadoss <durgadoss.r@xxxxxxxxx> wrote: >>-----Original Message----- >>From: Rodrigo Vivi [mailto:rodrigo.vivi@xxxxxxxxx] >>Sent: Wednesday, November 19, 2014 11:51 PM >>To: R, Durgadoss >>Cc: Vivi, Rodrigo; intel-gfx@xxxxxxxxxxxxxxxxxxxxx >>Subject: Re: [PATCH 11/15] drm/i915: PSR VLV/CHV: Introduce setup, enable and disable >>functions >> >>On Tue, Nov 18, 2014 at 10:32 AM, R, Durgadoss <durgadoss.r@xxxxxxxxx> wrote: >>>>-----Original Message----- >>>>From: Vivi, Rodrigo >>>>Sent: Friday, November 14, 2014 10:23 PM >>>>To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx >>>>Cc: Vivi, Rodrigo; R, Durgadoss >>>>Subject: [PATCH 11/15] drm/i915: PSR VLV/CHV: Introduce setup, enable and disable functions >>>> >>>>The biggest difference from HSW/BDW PSR here is that VLV enable_source >>>>function enables PSR but let it in Inactive state. So it might be called >>>>on early stage along with setup and enable_sink ones. >>>> >>>>v2: Rebase over intel_psr.c; >>>> Remove docs from static functions; >>>> Merge vlv_psr_active_on_pipe; >>>> Timeout for psr transition is 250us; >>>> Remove SRC_TRASMITTER_STATE; >>> >>> With SRC_TRANSMITTER_STATE not set to 1 >>> explicitly, if entry/exit works, I would like to know what DPCD >>> register 71h is reading in your panel ? >>> >>> I would expect bit 0 of 71h to be 1. >>> Is it the case ? >> >>no. It is always 0. >> >>DP_PSR_CAPS = 0xA > > 0xA ? could be.. but surprising how exit works > Without doing link training.. > May be we are looking at 01h..?, but your > Description seems fine .. it is DP_PSR_CAPS. > Can we double check once ? + u8 caps; drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, DP_PSR_ENABLE); + + drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_CAPS, + &caps); + DRM_ERROR("PSR Caps %x\n", caps); The works part is that if I + drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_CAPS, DP_PSR_NO_TRAIN_ON_EXIT); and read again it is still 0xA > > eDP spec v1.3 claims default setup time is 330 us. > So, I would expect bits[1:3] to be 0 (usually) > > This is the DPCD hex dump from 00h to 0xf in the > Panels that I use: {from dmesg} > DPCD: 11 0a 84 41 00 00 01 c0 02 03 00 00 00 0b 00 My one is DPCD: 12 0a 82 41 00 00 01 80 02 00 00 00 0f 0b 00 and it is funny but even with different platforms and apparently different panel I get the same... but maybe just a coincidence with similar panels... > >> >>> Can we check once ? >>> >>> Thanks, >>> Durga >> >>What do you suggest? get SRC_TRANSMITTER_STATE back? > > Yes. The reason I say is because not all panels come with > a 1 in that bit and ideally panels with a 0 in that bit 71h[0] > are supposed to require link training after PSR exit > when the main link is off. The main link will be off if we do > not set the SRC_TRANSMITTER_STATE bit to 1. Ok, I'm resending with SRC_TRANSMITTER_STATE back to 1. > > So, removing it is not safe in my opinion. I would suggest > Keeping it there. And also, the same bit in the DPCD as well. However I'm not sure about setting PSR_CAPS to 1 since my attempt here still shows 0xA when reading it right after writing it. > > Thanks, > Durga > >> >>> >>>> >>>>Cc: Durgadoss R <durgadoss.r@xxxxxxxxx> >>>>Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> >>>>--- >>>> drivers/gpu/drm/i915/intel_psr.c | 154 ++++++++++++++++++++++++++++++++------- >>>> 1 file changed, 129 insertions(+), 25 deletions(-) >>>> >>>>diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c >>>>index c296a89..bdb28f2 100644 >>>>--- a/drivers/gpu/drm/i915/intel_psr.c >>>>+++ b/drivers/gpu/drm/i915/intel_psr.c >>>>@@ -81,6 +81,17 @@ bool intel_psr_is_enabled(struct drm_device *dev) >>>> return (bool)dev_priv->psr.enabled; >>>> } >>>> >>>>+static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe) >>>>+{ >>>>+ struct drm_i915_private *dev_priv = dev->dev_private; >>>>+ uint32_t val; >>>>+ >>>>+ val = I915_READ(VLV_PSRSTAT(pipe)) & >>>>+ VLV_EDP_PSR_CURR_STATE_MASK; >>>>+ return (val == VLV_EDP_PSR_ACTIVE_NORFB_UP) || >>>>+ (val == VLV_EDP_PSR_ACTIVE_SF_UPDATE); >>>>+} >>>>+ >>>> static void intel_psr_write_vsc(struct intel_dp *intel_dp, >>>> struct edp_vsc_psr *vsc_psr) >>>> { >>>>@@ -110,7 +121,23 @@ static void intel_psr_write_vsc(struct intel_dp *intel_dp, >>>> POSTING_READ(ctl_reg); >>>> } >>>> >>>>-static void intel_psr_setup_vsc(struct intel_dp *intel_dp) >>>>+static void vlv_psr_setup_vsc(struct intel_dp *intel_dp) >>>>+{ >>>>+ struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); >>>>+ struct drm_device *dev = intel_dig_port->base.base.dev; >>>>+ struct drm_i915_private *dev_priv = dev->dev_private; >>>>+ struct drm_crtc *crtc = intel_dig_port->base.base.crtc; >>>>+ enum pipe pipe = to_intel_crtc(crtc)->pipe; >>>>+ uint32_t val; >>>>+ >>>>+ /* VLV auto-generate VSC package as per EDP 1.3 spec, Table 3.10 */ >>>>+ val = I915_READ(VLV_VSCSDP(pipe)); >>>>+ val &= ~VLV_EDP_PSR_SDP_FREQ_MASK; >>>>+ val |= VLV_EDP_PSR_SDP_FREQ_EVFRAME; >>>>+ I915_WRITE(VLV_VSCSDP(pipe), val); >>>>+} >>>>+ >>>>+static void hsw_psr_setup_vsc(struct intel_dp *intel_dp) >>>> { >>>> struct edp_vsc_psr psr_vsc; >>>> >>>>@@ -123,7 +150,13 @@ static void intel_psr_setup_vsc(struct intel_dp *intel_dp) >>>> intel_psr_write_vsc(intel_dp, &psr_vsc); >>>> } >>>> >>>>-static void intel_psr_enable_sink(struct intel_dp *intel_dp) >>>>+static void vlv_psr_enable_sink(struct intel_dp *intel_dp) >>>>+{ >>>>+ drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, >>>>+ DP_PSR_ENABLE); >>>>+} >>>>+ >>>>+static void hsw_psr_enable_sink(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; >>>>@@ -167,7 +200,21 @@ static void intel_psr_enable_sink(struct intel_dp *intel_dp) >>>> (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT)); >>>> } >>>> >>>>-static void intel_psr_enable_source(struct intel_dp *intel_dp) >>>>+static void vlv_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 = dev->dev_private; >>>>+ struct drm_crtc *crtc = dig_port->base.base.crtc; >>>>+ enum pipe pipe = to_intel_crtc(crtc)->pipe; >>>>+ >>>>+ /* Transition from PSR_state 0 to PSR_state 1, i.e. PSR Inactive */ >>>>+ I915_WRITE(VLV_PSRCTL(pipe), >>>>+ VLV_EDP_PSR_MODE_SW_TIMER | >>>>+ VLV_EDP_PSR_ENABLE); >>>>+} >>>>+ >>>>+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; >>>>@@ -247,7 +294,7 @@ static bool intel_psr_match_conditions(struct intel_dp *intel_dp) >>>> return true; >>>> } >>>> >>>>-static void intel_psr_do_enable(struct intel_dp *intel_dp) >>>>+static void intel_psr_activate(struct intel_dp *intel_dp) >>>> { >>>> struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); >>>> struct drm_device *dev = intel_dig_port->base.base.dev; >>>>@@ -257,9 +304,12 @@ static void intel_psr_do_enable(struct intel_dp *intel_dp) >>>> WARN_ON(dev_priv->psr.active); >>>> lockdep_assert_held(&dev_priv->psr.lock); >>>> >>>>- /* Enable/Re-enable PSR on the host */ >>>>- intel_psr_enable_source(intel_dp); >>>>- >>>>+ /* Enable/Re-enable PSR on the host >>>>+ * On HSW+ after we enable PSR on source it will activate it >>>>+ * as soon as it match configure idle_frame count. So >>>>+ * we just actually enable it here on activation time. >>>>+ */ >>>>+ hsw_psr_enable_source(intel_dp); >>>> dev_priv->psr.active = true; >>>> } >>>> >>>>@@ -296,37 +346,67 @@ void intel_psr_enable(struct intel_dp *intel_dp) >>>> >>>> dev_priv->psr.busy_frontbuffer_bits = 0; >>>> >>>>- intel_psr_setup_vsc(intel_dp); >>>>+ if (HAS_DDI(dev)) { >>>>+ hsw_psr_setup_vsc(intel_dp); >>>> >>>>- /* Avoid continuous PSR exit by masking memup and hpd */ >>>>- I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP | >>>>- EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP); >>>>+ /* Avoid continuous PSR exit by masking memup and hpd */ >>>>+ I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP | >>>>+ EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP); >>>> >>>>- /* Enable PSR on the panel */ >>>>- intel_psr_enable_sink(intel_dp); >>>>+ /* Enable PSR on the panel */ >>>>+ hsw_psr_enable_sink(intel_dp); >>>>+ } else { >>>>+ vlv_psr_setup_vsc(intel_dp); >>>>+ >>>>+ /* Enable PSR on the panel */ >>>>+ vlv_psr_enable_sink(intel_dp); >>>>+ >>>>+ /* On HSW+ enable_source also means go to PSR entry/active >>>>+ * state as soon as idle_frame achieved and here would be >>>>+ * to soon. However on VLV enable_source just enable PSR >>>>+ * but let it on inactive state. So we might do this prior >>>>+ * to active transition, i.e. here. >>>>+ */ >>>>+ vlv_psr_enable_source(intel_dp); >>>>+ } >>>> >>>> dev_priv->psr.enabled = intel_dp; >>>> unlock: >>>> mutex_unlock(&dev_priv->psr.lock); >>>> } >>>> >>>>-/** >>>>- * intel_psr_disable - Disable PSR >>>>- * @intel_dp: Intel DP >>>>- * >>>>- * This function needs to be called before disabling pipe. >>>>- */ >>>>-void intel_psr_disable(struct intel_dp *intel_dp) >>>>+static void vlv_psr_disable(struct intel_dp *intel_dp) >>>> { >>>> struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); >>>> struct drm_device *dev = intel_dig_port->base.base.dev; >>>> struct drm_i915_private *dev_priv = dev->dev_private; >>>>+ struct intel_crtc *intel_crtc = >>>>+ to_intel_crtc(intel_dig_port->base.base.crtc); >>>>+ uint32_t val; >>>> >>>>- mutex_lock(&dev_priv->psr.lock); >>>>- if (!dev_priv->psr.enabled) { >>>>- mutex_unlock(&dev_priv->psr.lock); >>>>- return; >>>>+ if (dev_priv->psr.active) { >>>>+ /* Put VLV PSR back to PSR_state 0 that is PSR Disabled. */ >>>>+ if (wait_for((I915_READ(VLV_PSRSTAT(intel_crtc->pipe)) & >>>>+ VLV_EDP_PSR_IN_TRANS) == 0, 0.250)) >>>>+ WARN(1, "PSR transition took longer than expected\n"); >>>>+ >>>>+ val = I915_READ(VLV_PSRCTL(intel_crtc->pipe)); >>>>+ val &= ~VLV_EDP_PSR_ACTIVE_ENTRY; >>>>+ val &= ~VLV_EDP_PSR_ENABLE; >>>>+ val &= ~VLV_EDP_PSR_MODE_MASK; >>>>+ I915_WRITE(VLV_PSRCTL(intel_crtc->pipe), val); >>>>+ >>>>+ dev_priv->psr.active = false; >>>>+ } else { >>>>+ WARN_ON(vlv_is_psr_active_on_pipe(dev, intel_crtc->pipe)); >>>> } >>>>+} >>>>+ >>>>+static void hsw_psr_disable(struct intel_dp *intel_dp) >>>>+{ >>>>+ struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); >>>>+ struct drm_device *dev = intel_dig_port->base.base.dev; >>>>+ struct drm_i915_private *dev_priv = dev->dev_private; >>>> >>>> if (dev_priv->psr.active) { >>>> I915_WRITE(EDP_PSR_CTL(dev), >>>>@@ -341,6 +421,30 @@ void intel_psr_disable(struct intel_dp *intel_dp) >>>> } else { >>>> WARN_ON(I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE); >>>> } >>>>+} >>>>+ >>>>+/** >>>>+ * intel_psr_disable - Disable PSR >>>>+ * @intel_dp: Intel DP >>>>+ * >>>>+ * This function needs to be called before disabling pipe. >>>>+ */ >>>>+void intel_psr_disable(struct intel_dp *intel_dp) >>>>+{ >>>>+ struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); >>>>+ struct drm_device *dev = intel_dig_port->base.base.dev; >>>>+ struct drm_i915_private *dev_priv = dev->dev_private; >>>>+ >>>>+ mutex_lock(&dev_priv->psr.lock); >>>>+ if (!dev_priv->psr.enabled) { >>>>+ mutex_unlock(&dev_priv->psr.lock); >>>>+ return; >>>>+ } >>>>+ >>>>+ if (HAS_DDI(dev)) >>>>+ hsw_psr_disable(intel_dp); >>>>+ else >>>>+ vlv_psr_disable(intel_dp); >>>> >>>> dev_priv->psr.enabled = NULL; >>>> mutex_unlock(&dev_priv->psr.lock); >>>>@@ -379,7 +483,7 @@ static void intel_psr_work(struct work_struct *work) >>>> if (dev_priv->psr.busy_frontbuffer_bits) >>>> goto unlock; >>>> >>>>- intel_psr_do_enable(intel_dp); >>>>+ intel_psr_activate(intel_dp); >>>> unlock: >>>> mutex_unlock(&dev_priv->psr.lock); >>>> } >>>>-- >>>>1.9.3 >>> >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> >> >>-- >>Rodrigo Vivi >>Blog: http://blog.vivi.eng.br -- Rodrigo Vivi Blog: http://blog.vivi.eng.br _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
