On Thu, Jul 18, 2013 at 5:02 AM, Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxxx>
>> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>> > ---
>> > 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@xxxxxxxxxxxxxxxxxxxxx
>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
