[Intel-gfx] [PATCH 3/4] drm/i915: Instrument PSR parameter for possible quirks with link standby.
Zanoni, Paulo R
paulo.r.zanoni at intel.com
Thu Jan 21 03:56:23 PST 2016
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 at intel.com>
> > > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at 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 at 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 at intel.com>
Date: Wed Dec 9 12:29:35 2015 -0800
drm/i915: Separate cherryview from valleyview
> > 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.
> > > 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 at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
More information about the Intel-gfx
mailing list