On Mon, Jul 8, 2013 at 7:25 PM, Rodrigo Vivi <rodrigo.vivi at gmail.com> wrote:
> On Fri, Jul 5, 2013 at 5:32 PM, Daniel Vetter <daniel at 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 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.
>
> 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?
I tried to find a way to do it with sw check with intel_plane, etc but
I didn't find any other way.
Do you have any other idea how to do that?
>
>>
>>>
>>> >
>>> >
>>> >> + 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?
Changed to check interlace flag as paulo suggested. Really much better ;)
Thanks
>
>>
>>>
>>> >
>>> >
>>> >> + 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
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
