| Message ID | 1373579105-1732-7-git-send-email-rodrigo.vivi@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <paulo.r.zanoni@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > --- > 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). -Chris
2013/7/11 Rodrigo Vivi <rodrigo.vivi@gmail.com>: > 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 <paulo.r.zanoni@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > --- > 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 { If we're not Haswell we fall on this "else" case, then we print "PSR disabled" and look at no_psr_reason. With that fixed: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > + seq_puts(m, "PSR disabled: "); > + switch (dev_priv->no_psr_reason) { > + case PSR_NO_SOURCE: > + seq_puts(m, "not supported on this platform"); > + break; > + case PSR_NO_SINK: > + seq_puts(m, "not supported by panel"); > + break; > + case PSR_CRTC_NOT_ACTIVE: > + seq_puts(m, "crtc not active"); > + break; > + case PSR_PWR_WELL_ENABLED: > + seq_puts(m, "power well enabled"); > + break; > + case PSR_NOT_TILED: > + seq_puts(m, "not tiled"); > + break; > + case PSR_SPRITE_ENABLED: > + seq_puts(m, "sprite enabled"); > + break; > + case PSR_S3D_ENABLED: > + seq_puts(m, "stereo 3d enabled"); > + break; > + case PSR_INTERLACED_ENABLED: > + seq_puts(m, "interlaced enabled"); > + break; > + case PSR_HSW_NOT_DDIA: > + seq_puts(m, "HSW ties PSR to DDI A (eDP)"); > + break; > + default: > + seq_puts(m, "unknown reason"); > + } > + seq_puts(m, "\n"); > return 0; > } > > - psrctl = I915_READ(EDP_PSR_CTL); > - seq_printf(m, "PSR Enabled: %s\n", > - yesno(psrctl & EDP_PSR_ENABLE)); > - > psrstat = I915_READ(EDP_PSR_STATUS_CTL); > > seq_puts(m, "PSR Current State: "); > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 842aada..d0b9483 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -560,6 +560,17 @@ struct i915_fbc { > } no_fbc_reason; > }; > > +enum no_psr_reason { > + PSR_NO_SOURCE, /* Not supported on platform */ > + PSR_NO_SINK, /* Not supported by panel */ > + PSR_CRTC_NOT_ACTIVE, > + PSR_PWR_WELL_ENABLED, > + PSR_NOT_TILED, > + PSR_SPRITE_ENABLED, > + PSR_S3D_ENABLED, > + PSR_INTERLACED_ENABLED, > + PSR_HSW_NOT_DDIA, > +}; > > enum intel_pch { > PCH_NONE = 0, /* No PCH present */ > @@ -1161,6 +1172,8 @@ typedef struct drm_i915_private { > /* Haswell power well */ > struct i915_power_well power_well; > > + enum no_psr_reason no_psr_reason; > + > struct i915_gpu_error gpu_error; > > struct drm_i915_gem_object *vlv_pctx; > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index b328ec6..3bca337 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4150,6 +4150,13 @@ > #define HSW_TVIDEO_DIP_VSC_DATA(trans) \ > _TRANSCODER(trans, HSW_VIDEO_DIP_VSC_DATA_A, HSW_VIDEO_DIP_VSC_DATA_B) > > +#define HSW_STEREO_3D_CTL_A 0x70020 > +#define S3D_ENABLE (1<<31) > +#define HSW_STEREO_3D_CTL_B 0x71020 > + > +#define HSW_STEREO_3D_CTL(trans) \ > + _TRANSCODER(trans, HSW_STEREO_3D_CTL_A, HSW_STEREO_3D_CTL_A) > + > #define _PCH_TRANS_HTOTAL_B 0xe1000 > #define _PCH_TRANS_HBLANK_B 0xe1004 > #define _PCH_TRANS_HSYNC_B 0xe1008 > 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)) { > + DRM_DEBUG_KMS("PSR condition failed: Power Well is Enabled\n"); > + dev_priv->no_psr_reason = PSR_PWR_WELL_ENABLED; > + return false; > + } > + > + if (obj->tiling_mode != I915_TILING_X || > + obj->fence_reg == I915_FENCE_REG_NONE) { > + DRM_DEBUG_KMS("PSR condition failed: fb not tiled or fenced\n"); > + dev_priv->no_psr_reason = PSR_NOT_TILED; > + return false; > + } > + > + if (I915_READ(SPRCTL(intel_crtc->pipe)) & SPRITE_ENABLE) { > + DRM_DEBUG_KMS("PSR condition failed: Sprite is Enabled\n"); > + dev_priv->no_psr_reason = PSR_SPRITE_ENABLED; > + return false; > + } > + > + if (I915_READ(HSW_STEREO_3D_CTL(intel_crtc->config.cpu_transcoder)) & > + S3D_ENABLE) { > + DRM_DEBUG_KMS("PSR condition failed: Stereo 3D is Enabled\n"); > + dev_priv->no_psr_reason = PSR_S3D_ENABLED; > + return false; > + } > + > + if (crtc->mode.flags & DRM_MODE_FLAG_INTERLACE) { > + DRM_DEBUG_KMS("PSR condition failed: Interlaced is Enabled\n"); > + dev_priv->no_psr_reason = PSR_INTERLACED_ENABLED; > + return false; > + } > + > + return true; > +} > + > void intel_edp_psr_enable(struct intel_dp *intel_dp) > { > struct drm_device *dev = intel_dp_to_dev(intel_dp); > > - if (!is_edp_psr(intel_dp) || intel_edp_is_psr_enabled(dev)) > + if (!intel_edp_psr_match_conditions(intel_dp) || > + intel_edp_is_psr_enabled(dev)) > return; > > /* Setup PSR once */ > -- > 1.7.11.7 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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 <paulo.r.zanoni@intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > > --- > > 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
On Thu, Jul 18, 2013 at 5:02 AM, Daniel Vetter <daniel@ffwll.ch> 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 <paulo.r.zanoni@intel.com> >> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> >> > --- >> > 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
On Thu, Jul 18, 2013 at 6:36 PM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote: > On Thu, Jul 18, 2013 at 5:02 AM, Daniel Vetter <daniel@ffwll.ch> 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 <paulo.r.zanoni@intel.com> >>> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> >>> > --- >>> > 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? I have honestly no idea ... have you poked Art about this? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
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: + seq_puts(m, "not supported on this platform"); + break; + case PSR_NO_SINK: + seq_puts(m, "not supported by panel"); + break; + case PSR_CRTC_NOT_ACTIVE: + seq_puts(m, "crtc not active"); + break; + case PSR_PWR_WELL_ENABLED: + seq_puts(m, "power well enabled"); + break; + case PSR_NOT_TILED: + seq_puts(m, "not tiled"); + break; + case PSR_SPRITE_ENABLED: + seq_puts(m, "sprite enabled"); + break; + case PSR_S3D_ENABLED: + seq_puts(m, "stereo 3d enabled"); + break; + case PSR_INTERLACED_ENABLED: + seq_puts(m, "interlaced enabled"); + break; + case PSR_HSW_NOT_DDIA: + seq_puts(m, "HSW ties PSR to DDI A (eDP)"); + break; + default: + seq_puts(m, "unknown reason"); + } + seq_puts(m, "\n"); return 0; } - psrctl = I915_READ(EDP_PSR_CTL); - seq_printf(m, "PSR Enabled: %s\n", - yesno(psrctl & EDP_PSR_ENABLE)); - psrstat = I915_READ(EDP_PSR_STATUS_CTL); seq_puts(m, "PSR Current State: "); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 842aada..d0b9483 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -560,6 +560,17 @@ struct i915_fbc { } no_fbc_reason; }; +enum no_psr_reason { + PSR_NO_SOURCE, /* Not supported on platform */ + PSR_NO_SINK, /* Not supported by panel */ + PSR_CRTC_NOT_ACTIVE, + PSR_PWR_WELL_ENABLED, + PSR_NOT_TILED, + PSR_SPRITE_ENABLED, + PSR_S3D_ENABLED, + PSR_INTERLACED_ENABLED, + PSR_HSW_NOT_DDIA, +}; enum intel_pch { PCH_NONE = 0, /* No PCH present */ @@ -1161,6 +1172,8 @@ typedef struct drm_i915_private { /* Haswell power well */ struct i915_power_well power_well; + enum no_psr_reason no_psr_reason; + struct i915_gpu_error gpu_error; struct drm_i915_gem_object *vlv_pctx; diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index b328ec6..3bca337 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4150,6 +4150,13 @@ #define HSW_TVIDEO_DIP_VSC_DATA(trans) \ _TRANSCODER(trans, HSW_VIDEO_DIP_VSC_DATA_A, HSW_VIDEO_DIP_VSC_DATA_B) +#define HSW_STEREO_3D_CTL_A 0x70020 +#define S3D_ENABLE (1<<31) +#define HSW_STEREO_3D_CTL_B 0x71020 + +#define HSW_STEREO_3D_CTL(trans) \ + _TRANSCODER(trans, HSW_STEREO_3D_CTL_A, HSW_STEREO_3D_CTL_A) + #define _PCH_TRANS_HTOTAL_B 0xe1000 #define _PCH_TRANS_HBLANK_B 0xe1004 #define _PCH_TRANS_HSYNC_B 0xe1008 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)) { + DRM_DEBUG_KMS("PSR condition failed: Power Well is Enabled\n"); + dev_priv->no_psr_reason = PSR_PWR_WELL_ENABLED; + return false; + } + + if (obj->tiling_mode != I915_TILING_X || + obj->fence_reg == I915_FENCE_REG_NONE) { + DRM_DEBUG_KMS("PSR condition failed: fb not tiled or fenced\n"); + dev_priv->no_psr_reason = PSR_NOT_TILED; + return false; + } + + if (I915_READ(SPRCTL(intel_crtc->pipe)) & SPRITE_ENABLE) { + DRM_DEBUG_KMS("PSR condition failed: Sprite is Enabled\n"); + dev_priv->no_psr_reason = PSR_SPRITE_ENABLED; + return false; + } + + if (I915_READ(HSW_STEREO_3D_CTL(intel_crtc->config.cpu_transcoder)) & + S3D_ENABLE) { + DRM_DEBUG_KMS("PSR condition failed: Stereo 3D is Enabled\n"); + dev_priv->no_psr_reason = PSR_S3D_ENABLED; + return false; + } + + if (crtc->mode.flags & DRM_MODE_FLAG_INTERLACE) { + DRM_DEBUG_KMS("PSR condition failed: Interlaced is Enabled\n"); + dev_priv->no_psr_reason = PSR_INTERLACED_ENABLED; + return false; + } + + return true; +} + void intel_edp_psr_enable(struct intel_dp *intel_dp) { struct drm_device *dev = intel_dp_to_dev(intel_dp); - if (!is_edp_psr(intel_dp) || intel_edp_is_psr_enabled(dev)) + if (!intel_edp_psr_match_conditions(intel_dp) || + intel_edp_is_psr_enabled(dev)) return; /* Setup PSR once */
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 <paulo.r.zanoni@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> --- 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(-)