Re: [PATCH] drm/i915: Match all PSR mode entry conditions before enabling it.

[Paulo Zanoni <przanoni@gmail.com>]

2013/6/28 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> v2: Prefer seq_puts to seq_printf by Paulo Zanoni.
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni