Re: [PATCH] drm/i915/psr: Remove few mod parameters option.

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

On Wed, 2018-07-11 at 23:17 -0700, Rodrigo Vivi wrote:
> On Wed, Jul 11, 2018 at 11:08:17PM -0700, Dhinakaran Pandiyan wrote:
> > 
> > On Wed, 2018-07-11 at 22:27 -0700, Rodrigo Vivi wrote:
> > > 
> > > Reduce the module parameter to enable or disable.
> > > 
> > > The link stand by vs full link off was used only once.
> > > 
> > > And it was actually masking another bug fixed by commit
> > > '84bb2916a683 ("drm/i915/psr: Check for SET_POWER_CAPABLE
> > > bit at PSR init time.")'
> > > 
> > > So, let's remove these options for now. End goal is to
> > > fully remove the mod param, moving it to a debugfs
> > > interface in upcoming patches.
> > Sounds like a good idea, but I have not had use for this module
> > parameter anyway. 
> > > 
> > > 
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Thanks
> 
> > 
> > 
> > What do you think of using enable_psr=1 to force PSR1 and
> > enable_psr=2
> > to force PSR2 on PSR2 capable panels?
> dam... you are messing up with my next patch that I
> have here hehehe
> 
> my idea here is from the regular path we call intel_psr_enable
> that would check platform defaults and vbt and call
> _intel_psr_enable
> 
> and for debugfs I was creating a toggle function
> 1 - enable
> 0 - disable
> 
> and change _intel_psr_enable to check crtc active.
> 
> If we add psr2 selection to the picture now I have to think
> better ideas on how to handle this flow...
> 
> ------
> 
> hmmm
> 
> maybe change intel_psr_enable signature to receive the type
> 
> enum psr_type
> {
> DEFAULT = 0,
> PSR = 1,
> PSR2 = 2,
> };
> 
Yeah, but I prefer the same mapping as module params - 0 for disable.

Maarten(CC'd) had a patch that did something similar (https://patchwork
.freedesktop.org/patch/210792/). My concern at that time (and now too)
was that our tests were broken and we shouldn't be adding more
complexity to the driver. Having said that, I understand that toggling
PSR via debugfs is something we need to do.


-DK


> so on regular path we would call
> intel_psr_enable(..., DEFAULT);
> 
> on debugfs
> intel_psr_enable(..., val);
> 
> ?!
> 
> Better and/or cleaner ideas?
> 
> Thanks,
> Rodrigo.
> 
> > 
> > 
> > > 
> > > Cc: Tarun Vyas <tarun.vyas@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_params.c |  2 +-
> > >  drivers/gpu/drm/i915/intel_psr.c   | 10 ----------
> > >  2 files changed, 1 insertion(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_params.c
> > > b/drivers/gpu/drm/i915/i915_params.c
> > > index 49fcc4679db6..817576701ed7 100644
> > > --- a/drivers/gpu/drm/i915/i915_params.c
> > > +++ b/drivers/gpu/drm/i915/i915_params.c
> > > @@ -92,7 +92,7 @@ i915_param_named_unsafe(enable_ppgtt, int,
> > > 0400,
> > >  
> > >  i915_param_named_unsafe(enable_psr, int, 0600,
> > >  	"Enable PSR "
> > > -	"(0=disabled, 1=enabled - link mode chosen per-platform,
> > > 2=force link-standby mode, 3=force link-off mode) "
> > > +	"(0=disabled, 1=enabled) "
> > >  	"Default: -1 (use per-chip default)");
> > >  
> > >  i915_param_named_unsafe(alpha_support, bool, 0400,
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 23acc9ac8d4d..ea467e2798e2 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -966,16 +966,6 @@ void intel_psr_init(struct drm_i915_private
> > > *dev_priv)
> > >  		/* 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_modparams.enable_psr == 2 && !dev_priv-
> > > > 
> > > > psr.link_standby) {
> > > -		DRM_DEBUG_KMS("PSR: Forcing link standby\n");
> > > -		dev_priv->psr.link_standby = true;
> > > -	}
> > > -	if (i915_modparams.enable_psr == 3 && dev_priv-
> > > > 
> > > > psr.link_standby) {
> > > -		DRM_DEBUG_KMS("PSR: Forcing main link off\n");
> > > -		dev_priv->psr.link_standby = false;
> > > -	}
> > > -
> > >  	INIT_WORK(&dev_priv->psr.work, intel_psr_work);
> > >  	mutex_init(&dev_priv->psr.lock);
> > >  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx