Again, Thank you very much for your comments.
Replying what I did and why I didn't here and patches coming later.
On Fri, Jun 28, 2013 at 5:46 PM, Paulo Zanoni <przanoni at gmail.com> wrote:
> 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?
Since it isn't implemented I'm not sure how to check that.
Maybe we will have a function, maybe just a bit in some register or
maybe somehow else.
So I prefer to stay without it until we have a proper way.
>
>
>> +};
>> +
>> 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.
done.
>
>
>> +
>> + 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.
done
>
>
>> + 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.
This is how it is done at update_fbc checking for active crtc and it
is properly workig.
So I prefer to let it like this.
>
>
>> + 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.
done
>
>
>> + (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.
You are right, it can be enabled by someone else later, but this will
only cause the continue PSR exit like mem up and hpd if they weren't
being masked.
So, this is just a check saying psr wont work if this is enabled but
not really needed to worry if this is enabled later. Wont break
anything for sure.
>
>
>> + 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.
I prefer to stay this way that is the way used on
asser_sprite_disabled in intel_display.
>
>
>> + 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".
I prefer to stay this way that is the way used in intel_display.
>
>
>> + 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
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
