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

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

On Tue, 2016-02-16 at 11:49 +0000, Zanoni, Paulo R wrote:
> Em Ter, 2016-02-16 às 12:27 +0200, Jani Nikula escreveu:
> > 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.


To be honest I shared this feeling. So when I was looking to the code
to do this change I tried different ways but all of them for me were
ugly. So in the end this one here seemed for me the cleanest way.

In the end -1 means driver will choose and nowadays driver is choosing
0, if we were enabling by default for all platforms driver would choose
1. So we are just moving driver's choice from the params to the
psr_init function. 

Also I didn't want to impact igt functionality at this point that
relies on the ability of changing the paramenter at runtime. If I was
adding another variable to control that I would have to change igt as
well.

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

Yeap, the dynamic part is needed for igt that relies on it for
enabling/disabling psr. 
Since sysfs interface wasn't accepted yet I decided not changing this b
ehavior.
I also noticed that force-standby and force-link-off were just
available at boot time but this is all we need for now so while we
don't have the sysfs interface in place I believe we can live with this
difference.

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

Exactly. At this point it would require IGT changes as well that would
block stuff or, worst, not noticing new regressions or bugs.

I'm confident that sysfs changes will land soon for power stuff as we
agreed on design reviews so powertop could take advantage of
controlling and showing all of our power related stuff. So when that
properly lands we can re-visit this parameter and change PSR paramenter
and IGT properly.

> 
> > 
> > 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. */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx