On Mon, Jul 01, 2013 at 05:47:39PM -0300, Rodrigo Vivi wrote:
> Again, Thank you very much for your comments.
>
> Replying what I did and why I didn't here and patches coming later.
Paulo asked me to drop by maintainer bikeshed on this patch. So here I'l
comply ;-)
>
>
> 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.
Imo adding the S3D_ENABLED reason is good, since that increases the
chances that we won'd forget about it. Maybe even add a comment in the
psr_match_conditions function below saying that we need to do this.
Wrt hotplug I've just chatted a bit and it sounds like the hw doesn't like
our hpd setup for port A. I guess we should fix that up first ...
>
> >
> >
> >> +};
> >> +
> >> 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
Just to check: Does this really work for DDI port D with dekstop eDP? I'm
pretty sure you actually want to check for cpu_transcoder = EPD here ...
>
> >
> >
> >> + 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.
That check is to avoid division-by-zero on state we've taken over from the
BIOS. Since we should only ever enable psr after a modeset that should be
a problem.
Now one thing which is a bit urky here with the psr code (same
comment applies to the fbc code really) is that a lot of the conditions
here _never_ change between modesets. So I think a ->fbc_possible and a
->psr_possible flag in pipe_config would make an awful lot of sense.
But I guess we can do that later on, once it's a bit clearer how this will
all work out.
>
> >
> >
> >> + 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.
tbh I don't understand why we need to check for the power well at all.
Is this really a hw requirement?
Paulo is right that we can't check for the power well enabling from the
kvmr in a race-free manner, so this needs some more thinking.
>
> >
> >
> >> + 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.
I agree with paulo, pls check sw tracked state, not hw state. If we move
to state precomputation and similar tricks you simply can't rely on the hw
state. Similar comment applies to the power well check above (if we really
need that one, I kinda expect that this is not so).
>
> >
> >
> >> + 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.
Where? Again I think we should use sw tracked state ...
>
> >
> >
> >> + 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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
