Re: [PATCH 3/4] drm/i915: Instrument PSR parameter for possible quirks with link standby.

["Thulasimani, Sivakumar" <sivakumar.thulasimani@intel.com>]

On 1/21/2016 5:26 PM, Zanoni, Paulo R wrote:
> Em Qui, 2016-01-21 às 08:35 +0530, Thulasimani, Sivakumar escreveu:
>> On 1/20/2016 10:32 PM, Zanoni, Paulo R wrote:
>>> Em Sex, 2015-12-11 às 08:39 -0800, Rodrigo Vivi escreveu:
>>>> Unfortunately we don't know all panels and platforms out there
>>>> and we
>>>> found internal prototypes without VBT proper set but where only
>>>> link in standby worked well.
>> :) if it is internal i assume someone has to set the vbt ,we
>> encountered
>> an issue
>> sometime back that blamed vbt as incorrect only to later learn that
>> the person who created the setup didn't care to configure the VBT.
> But in order to discover if the VBT is incorrect, the parameter can be
> used.
>
>>>> So, before enable PSR by default let's instrument the PSR
>>>> parameter
>>>> in a way that we can identify different panels out there that
>>>> might
>>>> require or work better with link standby mode.
>>>>
>>>> It is also useful to say that for backward compatibility I'm not
>>>> changing the meaning of this flag. So "0" still means disabled
>>>> and "1" means enabled with full support and maximum power
>>>> savings.
>>>>
>>>> v2: Use positive value instead of negative for different
>>>> operation
>>>> mode
>>>>       as suggested by Daniel.
>>>>
>>>> v3: As Paulo suggested use 2 to force link standby and 3 to force
>>>> link
>>>>       fully on. Also split the link_standby introduction in a
>>>> separated
>>>> patch.
>>>>
>>>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_params.c |  7 ++++++-
>>>>    drivers/gpu/drm/i915/intel_psr.c   | 17 +++++++++++++++++
>>>>    2 files changed, 23 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_params.c
>>>> b/drivers/gpu/drm/i915/i915_params.c
>>>> index 835d609..f78ddf3 100644
>>>> --- a/drivers/gpu/drm/i915/i915_params.c
>>>> +++ b/drivers/gpu/drm/i915/i915_params.c
>>>> @@ -126,7 +126,12 @@ MODULE_PARM_DESC(enable_execlists,
>>>>    	"(-1=auto [default], 0=disabled, 1=enabled)");
>>>>    
>>>>    module_param_named_unsafe(enable_psr, i915.enable_psr, int,
>>>> 0600);
>>>> -MODULE_PARM_DESC(enable_psr, "Enable PSR (default: false)");
>>>> +MODULE_PARM_DESC(enable_psr, "Enable PSR "
>>>> +		 "(0=disabled [default], 1=enabled - link mode
>>>> chosen per-platform, 2=force link-standby mode, 3=force link-off
>>>> mode)"
>>>> +		 "In case you needed to force any different
>>>> option,
>>>> please "
>>>> +		 "report PCI device ID, subsystem vendor and
>>>> subsystem device ID "
>>>> +		 "to intel-gfx@lists.freedesktop.org, if your
>>>> machine needs it. "
>>>> +		 "It will then be included in an upcoming module
>>>> version.");
>>> Are we making a promise here? Isn't that dangerous? :P
>>> I'd just tell the users to open bug reports.
>>> (I'm not requiring you to change anything here, but something
>>> something
>>> lawyers something)
>>>
>>>>    
>>>>    module_param_named_unsafe(preliminary_hw_support,
>>>> i915.preliminary_hw_support, int, 0600);
>>>>    MODULE_PARM_DESC(preliminary_hw_support,
>>>> diff --git a/drivers/gpu/drm/i915/intel_psr.c
>>>> b/drivers/gpu/drm/i915/intel_psr.c
>>>> index b84ec80..c3c2bb8 100644
>>>> --- a/drivers/gpu/drm/i915/intel_psr.c
>>>> +++ b/drivers/gpu/drm/i915/intel_psr.c
>>>> @@ -335,6 +335,12 @@ static bool
>>>> intel_psr_match_conditions(struct
>>>> intel_dp *intel_dp)
>>>>    		return false;
>>>>    	}
>>>>    
>>>> +	if ((IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) &&
>>>> +	    dev_priv->psr.link_standby) {
>> IS_VALLEYVIEW() will return true for both valleyview and cherryview
>> so
>> the above check for cherryview can be removed.
> Not anymore:
> commit 666a45379e2c29bc16e60648e5ad8f6f8b7fa6ce
> Author: Wayne Boyer <wayne.boyer@intel.com>
> Date:   Wed Dec 9 12:29:35 2015 -0800
>                                                   
>      drm/i915: Separate cherryview from valleyview
Thanks for commit id of patch :). jumping on upstream every
now and then will lead to missing a lot of changes happening.
>
>>> s/dev_priv->psr.link_standby/!dev_priv->psr.link_standby/
>>>
>>>
>>> Also, I'm not sure if this chunk belongs here or at
>>> intel_psr_init(),
>>> since it effectively disables PSR. This means that
>>> i915.enable_psr=3
>>> disables PSR on VLV/CHV. But maybe we shouldn't care since users
>>> shouldn't be using the option anyway. On the other hand, users may
>>> start claiming that i915.enable_psr=X "fixed PSR" for them while
>>> effectively it just disabled PSR, so perhaps DRM_ERROR would be
>>> better.
>>> Anyway, I'm not requesting any change, just pointing things in case
>>> you
>>> or someone else has any idea, but maybe I'd go with DRM_ERROR since
>>> users usually don't know which platform supports what, so the loud
>>> message may help them.
>> i agree, psr_match_conditions should check for parameters that can
>> change
>> dynamically post boot to decide if we can enable psr or not,
>> if link_standby cannot be changed post boot we should check for it in
>> init
>>    so we can avoid psr being enabled in the first place.
>>> Another check which we seem to be missing is "if (HAS_DDI(dev_priv)
>>> &&
>>> transcoder != TRANSCODER_EDP && !dev_priv->psr.link_standby)", but
>>> this
>>> depends on the result of the discussion of patch 1.
>>>
>>> Everything else looks good, but it would be nice to see the
>>> opinions of
>>> maintainers here since they always have something to say about new
>>> i915.ko options.
>>>
>>>
>>>> +		DRM_DEBUG_KMS("PSR condition failed: Link off
>>>> requested/needed but not supported on this platform\n");
>>>> +		return false;
>>>> +	}
>>>> +
>> sorry i came late to this thread, but can you point me to some issues
>> for
>> link off in CHT/VLV ? we have enabled link off in CHT Android and it
>> seems
>> to be working fine. we can check again if we have missed something.
> The issue is that the upstream Kernel does not appear to support it,
> since vlv_psr_enable_sink() always sets DP_PSR_MAIN_LINK_ACTIVE. But I
> didn't check if VLV_PSRCTL is properly setting link active too.
>
We too initially failed to get link off working, the main culprit being 
link training.
Our LT is too slow (~10ms) to bring up the panel from link off. we had 
to write an
optimized version of LT that will train the link back again. ideally the 
code
to reuse last trained values is similar to what we tried (try EQ 
directly since we
expect both CR & EQ to be passing with those values)  it should help in 
allowing
link off on CHV & VLV.
>>>>    	if (HAS_DDI(dev) && !dev_priv->psr.link_standby &&
>>>>    	    dig_port->port != PORT_A) {
>>>>    		DRM_DEBUG_KMS("PSR condition failed: Link Off
>>>> requested/needed but not supported on this port\n");
>>>> @@ -771,6 +777,7 @@ void intel_psr_init(struct drm_device *dev)
>>>>    	dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ?
>>>>    		HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE;
>>>>    
>>>> +	/* Set link_standby x link_off defaults */
>>>>    	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>>>>    		/*
>>>>    		 * On HSW and BDW Source implementation as an
>>>> issue
>>>> with PSR
>>>> @@ -786,6 +793,16 @@ void intel_psr_init(struct drm_device *dev)
>>>>    		/* For new platforms let's respect VBT back
>>>> again */
>>>>    		dev_priv->psr.link_standby = dev_priv-
>>>>> vbt.psr.full_link;
>>>>    
>>>> +	/* Override link_standby x link_off defaults */
>>>> +	if (i915.enable_psr == 2 && !dev_priv->psr.link_standby)
>>>> {
>>>> +		DRM_DEBUG_KMS("PSR: Forcing link standby\n");
>>>> +		dev_priv->psr.link_standby = true;
>>>> +	}
>>>> +	if (i915.enable_psr == 3 && dev_priv->psr.link_standby)
>>>> {
>>>> +		DRM_DEBUG_KMS("PSR: Forcing main link off\n");
>>>> +		dev_priv->psr.link_standby = false;
>>>> +	}
>>>> +
>>>>    	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
>>>>    	mutex_init(&dev_priv->psr.lock);
>>>>    }
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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