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

[Rodrigo Vivi <rodrigo.vivi@gmail.com>]

On Fri, Jul 5, 2013 at 5:32 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> 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 ;-)

Mainteiner's complies are always good ;)
Thanks for jumping in this discussion

>
>>
>>
>> On Fri, Jun 28, 2013 at 5:46 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> > 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?
>>
>> 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.

ok, I'll do that then.

>
> 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 ...

Since we don't have any known issue with psr, couldn't we just move
forward and after we get hotplug fixed we remove this workaround from
here?
Also, if we wait this we will have to say Chris and Ville finish that
rework with vblank waits you mentioned on the other email.

If you perfer, on next series I can send psr disabled by default.

>
>>
>> >
>> >
>> >> +};
>> >> +
>> >>  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 ...

doc says psr on hsw is tied to DDI A (eDP)... I used same check Paulo
used on IPS.

>
>>
>> >
>> >
>> >> +               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.
>

I liked your _possible flag in pipe_config suggestion..

> But I guess we can do that later on, once it's a bit clearer how this will
> all work out.

... but yes, I think we can do this later and fix both psr and fbc


>
>>
>> >
>> >
>> >> +               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?

yes, it is... if it is on it forces psr exit by definition... so if it
is on since the begining it is not worth turn psr on...

>
> 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.

also later? because as I said it doesn't cause any bug if power well
is on... it just forces psr exit state...

>
>>
>> >
>> >
>> >> +               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).

I just don't understand why... Should we change the assert_sprite
also? is it wrong?

>
>>
>> >
>> >
>> >> +               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 ...

at ironlake_enable_pch_transcoder.... is it wrong there? should we fix?

but anyway, how is the correct way to check it in 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@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@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



--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br