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

["Zanoni, Paulo R" <paulo.r.zanoni@intel.com>]

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?

> 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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.

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.

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

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.

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.

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


>  	}
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx