From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 06/11] drm/i915: Match all PSR mode entry conditions before enabling it. Date: Thu, 18 Jul 2013 10:02:50 +0200 Message-ID: <20130718080250.GK4550@phenom.ffwll.local> 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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ea0-f178.google.com (mail-ea0-f178.google.com [209.85.215.178]) by gabe.freedesktop.org (Postfix) with ESMTP id 3B8CDE680D for ; Thu, 18 Jul 2013 01:02:50 -0700 (PDT) Received: by mail-ea0-f178.google.com with SMTP id l15so1561381eak.9 for ; Thu, 18 Jul 2013 01:02:49 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20130715140622.GF2823@cantiga.alporthouse.com> 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: Chris Wilson , Rodrigo Vivi , intel-gfx@lists.freedesktop.org, Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org 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. -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