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@xxxxxxxxx>
Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxxxx, 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@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
