[Intel-gfx] [PATCH 1/4] drm/i915: PSR Fix standby logic for PSR on non DDI-A for certain platforms.

[Vivi, Rodrigo]

On Wed, 2016-01-20 at 14:11 +0000, Zanoni, Paulo R wrote:
> Em Sex, 2015-12-11 às 08:39 -0800, Rodrigo Vivi escreveu:
> > Current platforms that support PSR on other port than A only 
> > support
> > link
> > standby mode.
> > 
> > The logic here was wrong since 'commit 89251b177b ("drm/i915: PSR:
> > deprecate link_standby support for core platforms.")
> 
> What's wrong/broken with the logic?

If we request link_standby via VBT and we aren't on TransEDP-at-DDI-A
we are nowadays avoiding PSR

With this patch if full_link disabled (== link off == main link off ==
not in link standby mode) and we are not using DDIA than we block PSR
and lets get PSR on the links_standby on non portA that we are
currently blocking...

> 
> > 
> > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 9ccff30..4a9c062 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -327,9 +327,9 @@ static bool intel_psr_match_conditions(struct
> > intel_dp *intel_dp)
> >  		return false;
> >  	}
> >  
> > -	if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) &&
> > -	    ((dev_priv->vbt.psr.full_link) || (dig_port->port !=
> > PORT_A))) {
> > -		DRM_DEBUG_KMS("PSR condition failed: Link Standby
> > requested/needed but not supported on this platform\n");
> > +	if (HAS_DDI(dev) && !dev_priv->vbt.psr.full_link &&
> > +	    dig_port->port != PORT_A) {
> > +		DRM_DEBUG_KMS("PSR condition failed: Link Off
> > requested/needed but not supported on this port\n");
> >  		return false;
> 
> I spent a long time looking at this patch, trying to understand what
> exactly changed. You didn't really mention which specific case was
> wrong and why. Please explicitly mention the case and platform, also
> replacing "certain platforms" for something more precise.

Your comments made me to keep looking to my own patch a long time as
well trying to understand what I had done... 

> 
> The code mentions "full_link", "link standby" and "link off", but
> there's nothing clarifying the relationship between them. It seems
> "full_link" means "link standby", but I suppose we could put a 
> comment
> somewhere clarifying this. We previously had dev_priv-
> > psr.link_standby, but we don't have this anymore.

yeah, this is indeed confusing names and also was the reason that I
staid a long time also confused by my patch.
I will try to explain a bit here and put this comment somewhere in the
code and in the commit message.

We have 2 situations:
1 - Main/Full link needs to be enabled the whole time in stand by mode
to allow source-sink communication when exiting PSR.
2 - All links can be disabled, go off on PSR entry. 

This is optimal case for power savings, but depend on the panel
implementation it can fail badly. So if we identify this earlier during
product development OEMs can set this VBT bit to force the link
standby.

So, full_link == standby == link on
link-off == ! stand-by == ! full link

The confusion happens because VESA uses one way and our spec another
and VBT another.


> 
> It seems that with the previous code we only supported link off on 
> port
> A, nothing else. This is reflected both by the check you're changing
> and by hsw_psr_enable_source(). Now we're only going to reject link 
> off
> for ports B/C/D/E. 

nope the previous one support link off for ports B/C/D/E and skip on
standy, and this is exactly the issue.

> This means that this patch is both enabling ports
> B/C/D/E _and_ enabling link standby support on port A. Maybe we could
> do this through two separate patches: one for the port A new feature,
> the other for the new ports. This means:
>  - We're now accepting link standby on every port, but we don't have
> the code to set link standby on port A.

Well, this was already the case on BDW+... for HSW PSR is tied to DDI-A
according spec and this restriction was removed on BDW by adding the
IS_HASWELL(dev) to the pre-existing port != DDIA

>  - We're now accepting port B/C/D/E on DDI platforms, but our code
> currently hardcodes PSR to the register associated with transcoder 
> eDP.

Oh wait, you are absolutely right here! We do not block PSR on this
function, but we just use transcoder_EDP one...

Now I believe we should just remove this entire block and that
IS_HASWELL and get tied again to DDI_A and transcoder_EDP before a
rework that would make that really work. With a low priority since we
never really target platforms with 2 eDPs or eDP on other port than not
TRANS_EDP+DDIA.

What do you think?

> 
> Also, according to 89251b177b58, we don't want link standby for
> HSW/BDW, but we're accepting this now. So this patch is a half-revert
> of that commit.

not really... because the logic was inverted on that commit causing all
the confusion...

> 
> By the way, the spec talks in terms of transcoders and our code talks
> in terms of ports. I know there was some direct relationship between
> them at some point, but as we add more platforms, we increase the
> chance that our implicit assertions between the relationship of
> transcoders and ports may not be valid anymore. So please either add
> some assertions, or some huge comment, or just change the code to 
> check
> transcoders instead of ports.

well, initial specs were also talking in terms of trans_edp + DDI_A or
! DDI_A, etc. But based on registers divided by transcoder we would
need the full rework to allow multiple PSR in different transcoders...
Also another question is where would we get a machine like this to
validate?! 

So, let's fully tie it to transcoder_edp for now?

> 
> To conclude: I still can't see what's wrong with the current code.

I hope it is clear now, but we can talk more tomorrow about it on irc.

Thank you very much for all highlights here!

> 
> 
> >  	}