Re: [PATCH] drm/i915/psr: simplify enable_psr handling

[Dhinakaran Pandiyan]

On Fri, 2018-12-21 at 10:23 -0700, Ross Zwisler wrote:
> The following commit:
> 
> commit 2bdd045e3a30 ("drm/i915/psr: Check if VBT says PSR can be
> enabled.")
> 
> added some code with no usable functionality.  Regardless of how the
> psr
> default is set up in the BDB_DRIVER_FEATURES section, if the
> enable_psr
> module parameter isn't specified it defaults to 0.
Right, that was intentional and the commit message even makes a note of
it 
" Note: The feature currently remains disabled by default for all
platforms irrespective of what VBT says."


Anyway, we've enabled the feature by default now and the current code
should take into account the VBT flag if the module parameter is left
to a default value. Please check git://anongit.freedesktop.org/drm-tip
drm-tip.

-DK
> 
> Remove this dead code, simplify the way that enable_psr is handled
> and
> update the module parameter string to match the actual functionality.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Signed-off-by: Ross Zwisler <zwisler@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_drv.h    | 1 -
>  drivers/gpu/drm/i915/i915_params.c | 4 +---
>  drivers/gpu/drm/i915/i915_params.h | 2 +-
>  drivers/gpu/drm/i915/intel_bios.c  | 1 -
>  drivers/gpu/drm/i915/intel_psr.c   | 7 -------
>  5 files changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 872a2e159a5f9..b4c50ba0b22a6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1115,7 +1115,6 @@ struct intel_vbt_data {
>  	} edp;
>  
>  	struct {
> -		bool enable;
>  		bool full_link;
>  		bool require_aux_wakeup;
>  		int idle_frames;
> diff --git a/drivers/gpu/drm/i915/i915_params.c
> b/drivers/gpu/drm/i915/i915_params.c
> index 295e981e4a398..80ce8758c3c69 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -87,9 +87,7 @@ i915_param_named_unsafe(enable_ppgtt, int, 0400,
>  	"(-1=auto [default], 0=disabled, 1=aliasing, 2=full, 3=full
> with extended address space)");
>  
>  i915_param_named_unsafe(enable_psr, int, 0600,
> -	"Enable PSR "
> -	"(0=disabled, 1=enabled) "
> -	"Default: -1 (use per-chip default)");
> +	"Enable PSR (default: false)");
>  
>  i915_param_named_unsafe(alpha_support, bool, 0400,
>  	"Enable alpha quality driver support for latest hardware. "
> diff --git a/drivers/gpu/drm/i915/i915_params.h
> b/drivers/gpu/drm/i915/i915_params.h
> index 6c4d4a21474b5..144572f17a83d 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -42,7 +42,7 @@ struct drm_printer;
>  	param(int, enable_dc, -1) \
>  	param(int, enable_fbc, -1) \
>  	param(int, enable_ppgtt, -1) \
> -	param(int, enable_psr, -1) \
> +	param(int, enable_psr, 0) \
>  	param(int, disable_power_well, -1) \
>  	param(int, enable_ips, 1) \
>  	param(int, invert_brightness, 0) \
> diff --git a/drivers/gpu/drm/i915/intel_bios.c
> b/drivers/gpu/drm/i915/intel_bios.c
> index 1faa494e2bc91..d676d483d5cf1 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -551,7 +551,6 @@ parse_driver_features(struct drm_i915_private
> *dev_priv,
>  	 */
>  	if (!driver->drrs_enabled)
>  		dev_priv->vbt.drrs_type = DRRS_NOT_SUPPORTED;
> -	dev_priv->vbt.psr.enable = driver->psr_enabled;
>  }
>  
>  static void
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index b6838b525502e..26e7eb318cf07 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -1065,13 +1065,6 @@ void intel_psr_init(struct drm_i915_private
> *dev_priv)
>  	if (!dev_priv->psr.sink_support)
>  		return;
>  
> -	if (i915_modparams.enable_psr == -1) {
> -		i915_modparams.enable_psr = dev_priv->vbt.psr.enable;
> -
> -		/* Per platform default: all disabled. */
> -		i915_modparams.enable_psr = 0;
> -	}
> -
>  	/* Set link_standby x link_off defaults */
>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>  		/* HSW and BDW require workarounds that we don't
> implement. */