On Fri, 11 Dec 2015, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> wrote:
> 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.
>
> 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)"
Since this parameter is _unsafe, I'm fine with adding the alternatives.
> + "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.");
However I object to the implied plan of quirking this in the driver. I
do not think quirking should be perceived as a viable option for
anything. It's a maintainability nightmare.
BR,
Jani.
>
> 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) {
> + DRM_DEBUG_KMS("PSR condition failed: Link off requested/needed but not supported on this platform\n");
> + return false;
> + }
> +
> 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);
> }
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
