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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 11 Dec 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> 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@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)"

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@lists.freedesktop.org, 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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx