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

[Zanoni, Paulo R]

Em Ter, 2016-02-16 às 12:27 +0200, Jani Nikula escreveu:
> On Fri, 12 Feb 2016, Rodrigo Vivi <rodrigo.vivi at 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 at intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at 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.

Just as a reminder: igt relies on the 0600 permissions so it can run
PSR tests even while PSR is disabled by default. This was done because
we're trying to fix the features, so removing them from QA/CI testing
would only allow people to introduce unnoticed regressions. When I did
this, I was also expecting that at some point our pre-merge testing
would be running a huge chunk of IGT.

> 
> 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. */
>