From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rodrigo Vivi Subject: Re: [PATCH 06/11] drm/i915: Match all PSR mode entry conditions before enabling it. Date: Thu, 18 Jul 2013 13:36:30 -0300 Message-ID: References: <1373579105-1732-1-git-send-email-rodrigo.vivi@gmail.com> <1373579105-1732-7-git-send-email-rodrigo.vivi@gmail.com> <20130715140622.GF2823@cantiga.alporthouse.com> <20130718080250.GK4550@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-vc0-f181.google.com (mail-vc0-f181.google.com [209.85.220.181]) by gabe.freedesktop.org (Postfix) with ESMTP id 59BD9E5DEB for ; Thu, 18 Jul 2013 09:36:31 -0700 (PDT) Received: by mail-vc0-f181.google.com with SMTP id lf11so2512200vcb.12 for ; Thu, 18 Jul 2013 09:36:31 -0700 (PDT) In-Reply-To: <20130718080250.GK4550@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Daniel Vetter Cc: intel-gfx , Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org On Thu, Jul 18, 2013 at 5:02 AM, Daniel Vetter wrote: > On Mon, Jul 15, 2013 at 03:06:22PM +0100, Chris Wilson wrote: >> On Thu, Jul 11, 2013 at 06:45:00PM -0300, Rodrigo Vivi wrote: >> > v2: Prefer seq_puts to seq_printf by Paulo Zanoni. >> > v3: small changes like avoiding calling dp_to_dig_port twice as noticed by >> > Paulo Zanoni. >> > v4: Avoiding reading non-existent registers - noticed by Paulo >> > on first psr debugfs patch. >> > v5: Accepting more suggestions from Paulo: >> > * check sw interlace flag instead of i915_read >> > * introduce PSR_S3D_ENABLED to avoid forgeting it whenever added. >> > >> > Cc: Paulo Zanoni >> > Signed-off-by: Rodrigo Vivi >> > --- >> > drivers/gpu/drm/i915/i915_debugfs.c | 44 ++++++++++++++++++---- >> > drivers/gpu/drm/i915/i915_drv.h | 13 +++++++ >> > drivers/gpu/drm/i915/i915_reg.h | 7 ++++ >> > drivers/gpu/drm/i915/intel_dp.c | 74 ++++++++++++++++++++++++++++++++++++- >> > 4 files changed, 130 insertions(+), 8 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c >> > index fe3cd5a..e679968 100644 >> > --- a/drivers/gpu/drm/i915/i915_debugfs.c >> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c >> > @@ -1948,17 +1948,47 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) >> > struct drm_info_node *node = m->private; >> > struct drm_device *dev = node->minor->dev; >> > struct drm_i915_private *dev_priv = dev->dev_private; >> > - u32 psrctl, psrstat, psrperf; >> > + u32 psrstat, psrperf; >> > >> > - if (!IS_HASWELL(dev)) { >> > - seq_puts(m, "PSR not supported on this platform\n"); >> > + if (IS_HASWELL(dev) && I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE) { >> > + seq_puts(m, "PSR enabled\n"); >> > + } else { >> > + seq_puts(m, "PSR disabled: "); >> > + switch (dev_priv->no_psr_reason) { >> > + case PSR_NO_SOURCE: >> >> I am not a fan of using a continually expanding set of enums for >> no_psr_reason (and no_fbc_reason). If we just stored a const char >> *no_psr_reason, it would make like much easier for us (fewer lines and a >> smaller object). >> >> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> > index d4b52a9..c0bd887 100644 >> > --- a/drivers/gpu/drm/i915/intel_dp.c >> > +++ b/drivers/gpu/drm/i915/intel_dp.c >> > @@ -1497,11 +1497,83 @@ static void intel_edp_psr_enable_source(struct intel_dp *intel_dp) >> > EDP_PSR_ENABLE); >> > } >> > >> > +static bool intel_edp_psr_match_conditions(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; >> > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> > + struct drm_i915_gem_object *obj = to_intel_framebuffer(crtc->fb)->obj; >> > + struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; >> > + >> > + if (!IS_HASWELL(dev)) { >> > + DRM_DEBUG_KMS("PSR not supported on this platform\n"); >> > + dev_priv->no_psr_reason = PSR_NO_SOURCE; >> > + return false; >> > + } >> > + >> > + if ((intel_encoder->type != INTEL_OUTPUT_EDP) || >> > + (dig_port->port != PORT_A)) { >> > + DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n"); >> > + dev_priv->no_psr_reason = PSR_HSW_NOT_DDIA; >> > + return false; >> > + } >> > + >> > + if (!is_edp_psr(intel_dp)) { >> > + DRM_DEBUG_KMS("PSR not supported by this panel\n"); >> > + dev_priv->no_psr_reason = PSR_NO_SINK; >> > + return false; >> > + } >> > + >> > + if (!intel_crtc->active || !crtc->fb || !crtc->mode.clock) { >> > + DRM_DEBUG_KMS("crtc not active for PSR\n"); >> > + dev_priv->no_psr_reason = PSR_CRTC_NOT_ACTIVE; >> > + return false; >> > + } >> > + >> > + if ((I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE) || >> > + (I915_READ(HSW_PWR_WELL_KVMR) & HSW_PWR_WELL_ENABLE)) { >> >> Any time we resort to reading back register state is a failure in our >> state tracking (and pipe_config). > > Highly agreed, especially if it's a resource which is out of our control > like the KVMR bits. Since that's just plain broken I've removed it from > the patch. Since Audio guys will enable power well and let it enabled all the time, I'm wondering in mask LPSP at PSR_DEBUG_CTL. So PSR can work even with power well enabled. What do you think? > -Daniel > >> -Chris >> >> -- >> Chris Wilson, Intel Open Source Technology Centre >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Rodrigo Vivi Blog: http://blog.vivi.eng.br