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

[Chris Wilson <chris@chris-wilson.co.uk>]

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

-- 
Chris Wilson, Intel Open Source Technology Centre