[Intel-gfx] [PATCH] drm/i915: Match all PSR mode entry conditions before enabling it.
Paulo Zanoni
przanoni at gmail.com
Fri Jun 28 22:46:03 CEST 2013
2013/6/28 Rodrigo Vivi <rodrigo.vivi at gmail.com>:
> v2: Prefer seq_puts to seq_printf by Paulo Zanoni.
>
> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at gmail.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 39 ++++++++++++++++++---
> drivers/gpu/drm/i915/i915_drv.h | 12 +++++++
> drivers/gpu/drm/i915/intel_dp.c | 68 ++++++++++++++++++++++++++++++++++++-
> 3 files changed, 114 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 67c777f..95b27ac 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1882,11 +1882,42 @@ 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;
>
> - psrctl = I915_READ(EDP_PSR_CTL);
> - seq_printf(m, "PSR Enabled: %s\n",
> - yesno(psrctl & EDP_PSR_ENABLE));
> + if (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_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");
> + }
>
> psrstat = I915_READ(EDP_PSR_STATUS_CTL);
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 56bd82b..f08c1d9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -543,6 +543,17 @@ enum no_fbc_reason {
> FBC_CHIP_DEFAULT, /* disabled by default on this chip */
> };
>
> +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_INTERLACED_ENABLED,
> + PSR_HSW_NOT_DDIA,
I see you left a few reasons listed on the spec, for example S3D,
which we don't support yet. I'm pretty sure that when we implement S3D
we'll totally forget about adding the PSR_S3D_ENABLED condition, so
shouldn't we do it now? Also, why did we not add the eDP hotplug
reason?
> +};
> +
> enum intel_pch {
> PCH_NONE = 0, /* No PCH present */
> PCH_IBX, /* Ibexpeak PCH */
> @@ -1146,6 +1157,7 @@ typedef struct drm_i915_private {
> struct i915_power_well power_well;
>
> enum no_fbc_reason no_fbc_reason;
> + enum no_psr_reason no_psr_reason;
>
> struct drm_mm_node *compressed_fb;
> struct drm_mm_node *compressed_llb;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c10be94..9730d6b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1471,11 +1471,77 @@ 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 drm_device *dev = intel_dp_to_dev(intel_dp);
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> + 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;
Again, you have intel_dp_to_dev and also call dp_to_dig_port twice.
> +
> + 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) ||
> + (enc_to_dig_port(&intel_encoder->base)->port != PORT_A)) {
Here you're calling enc_to_dig_port, but you already defined dig_port
above. Use it.
> + 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) {
Why do we check for crtc->fb and crtc->mode.clock here? Also, there's
intel_crtc->primary_disabled which you could use.
> + 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) |
I'd use "||" instead of "|" at the end of the line since this is a
logical statement.
> + (I915_READ(HSW_PWR_WELL_KVMR) & HSW_PWR_WELL_ENABLE)) {
I know the spec says we should check for this, but there's absolutely
no guarantee that the KVMr won't be enabled exactly after we read this
bit. I don't know if there's a sane way to check for an active KVMr
session. We should also probably get an interrupt somehow if someone
enables KVMr after PSR is enabled.
> + 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) {
We should probably check "struct intel_plane" instead of I915_READ here.
> + DRM_DEBUG_KMS("PSR condition failed: Sprite is Enabled\n");
> + dev_priv->no_psr_reason = PSR_SPRITE_ENABLED;
> + return false;
> + }
> +
> + if ((I915_READ(PIPECONF(intel_crtc->config.cpu_transcoder)) &
> + PIPECONF_INTERLACE_MASK) == PIPECONF_INTERLACED_ILK) {
I'd prefer checking for "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;
>
> /* Enable PSR on the panel */
> --
> 1.8.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
More information about the Intel-gfx
mailing list