Re: [PATCH 1/3] drm/i915: Change i915.enable_psr parameter to use per platform default.

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

On Fri, 12 Feb 2016, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> This will give us flexibility to enable PSR by default independently so
> issues and corner cases in one platform won't affect others were we have
> it working properly.
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_params.c | 5 +++--
>  drivers/gpu/drm/i915/intel_psr.c   | 5 +++++
>  2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 8b9f368..1b40ee6 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -38,7 +38,7 @@ struct i915_params i915 __read_mostly = {
>  	.enable_execlists = -1,
>  	.enable_hangcheck = true,
>  	.enable_ppgtt = -1,
> -	.enable_psr = 0,
> +	.enable_psr = -1,
>  	.preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
>  	.disable_power_well = -1,
>  	.enable_ips = 1,
> @@ -128,7 +128,8 @@ MODULE_PARM_DESC(enable_execlists,
>  
>  module_param_named_unsafe(enable_psr, i915.enable_psr, int, 0600);
>  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)");
> +		 "(0=disabled, 1=enabled - link mode chosen per-platform, 2=force link-standby mode, 3=force link-off mode) "
> +		 "Default: -1 (use per-chip default)");
>  
>  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 4ab7579..655bdf6 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -778,6 +778,11 @@ 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;
>  
> +	/* Per platform default */
> +	if (i915.enable_psr == -1) {
> +		i915.enable_psr = 0;
> +	}

I don't like changing module parameters in-kernel, because them changing
may be surprising to the user (and possibly developers debugging
issues). We do this anyway, but AFAICT only for read only
parameters. It's perhaps less surprising in that case.

Now, i915.enable_psr has permissions 0600, so you can change it
dynamically through sysfs. Maybe it's a good debugging thing, I don't
know. But I also notice some parts of the PSR functionality (link
standby) can only be forced with i915.enable_psr during module load, and
not dynamically.

So I think you need to either a) change enable_psr permission to 0400
and give up the ability to dynamically toggle the feature, or b) fix up
the current link standby force and not change the parameter value
in-kernel in this patch.

BR,
Jani.


> +
>  	/* Set link_standby x link_off defaults */
>  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		/* HSW and BDW require workarounds that we don't implement. */

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx