[Intel-gfx] [PATCH] drm/i915/psr: enable psr1 on psr2 panels

[Pandiyan, Dhinakaran]

On Wed, 2018-04-11 at 07:40 -0700, Rodrigo Vivi wrote:
> On Wed, Apr 11, 2018 at 03:01:24PM +0530, vathsala nagaraju wrote:
> > 
> > + puthik
> > On Saturday 07 April 2018 12:36 AM, Vivi, Rodrigo wrote:
> > > On Fri, Apr 06, 2018 at 12:10:24PM -0700, Dhinakaran Pandiyan wrote:
> > > > 
> > > > 
> > > > On Sat, 2018-04-07 at 00:12 +0530, vathsala nagaraju wrote:
> > > > > From: Vathsala Nagaraju <vathsala.nagaraju at intel.com>
> > > > > 
> > > > > Adds force_psr1 mod parameter to enable psr1 on psr2 panels.
> > > > > useful in cases where psr2 fails and user wants to enable
> > > > > psr1 feature for power saving until a fix
> > > > > is provided for psr2.
> > > The parameters shouldn't be used by users to select a configuration.
> > > They are marked as unsafe. We should only enable the feature when
> > > we are comfortable it doesn't cause trouble.
> > 
> > The idea was to give user the option to switch to psr1 ,if they want to.
> 
> This is exactly the problem that I'm trying to avoid ;)
> 
> But I do understand your perspective since I had the same, at least
> until when Jani showed me an old but gold note from ajax:
> https://www.redhat.com/archives/rhl-devel-list/2008-January/msg00861.html
> 
> If we did not validated we shouldn't be allowing the choice.
> 
> Although I'm still in favor of the parameters here because it makes
> our lives easier when developing and debugging. But user should never
> be using those.
> 
> Specially on a feature full of corner cases like PSR. So, while
> we don't really fix all the known bad PSR bugs we should keep it disabled.
> 
> 
> > 
> > > 
> > > > 
> > > > We should perhaps make enable_psr=1 enable just PSR1. I am not
> > > > comfortable that we enable PSR2 at all, there are no tests in IGT for
> > > > selective update, seems like nobody really knows exactly how well it
> > > > works.
> > with enable_psr , we are deciding whether to use psr1/psr2.
> > we can reuse enable_psr.
> > 
> 
> While we are not confident PSR2 ever worked we should not be using that
> at all. So if you want any change like this I would suggest to avoid PSR2
> at all for everybody while we don't fix PSR2.
> 
> But when PSR and PSR2 gets enabled we keep the parameter to allow debug
> and triage... i.e enable_psr=0 disables PSR, enable_psr=1 enables what ever
> PSR is the default for that panel/platform.
> 


Vathsala,

Please make i915.enable_psr=1 to enable only PSR1 at this point. Until
we add new IGT's to test for selective update, PSR2 will not receive
regular testing and as such is not worthy of enabling even with a module
parameter.




> Thanks,
> Rodrigo.
> 
> > > Agreed. Probably good for now to avoid PSR2 in all situations and only
> > > allow PSR2 when we are properly testing it.
> > > 
> > > > 
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > > > Cc: José Roberto de Souza <jose.souza at intel.com>
> > > > > Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju at intel.com>
> > > > > ---
> > > > >   drivers/gpu/drm/i915/i915_params.c | 5 +++++
> > > > >   drivers/gpu/drm/i915/i915_params.h | 1 +
> > > > >   drivers/gpu/drm/i915/intel_psr.c   | 2 ++
> > > > >   3 files changed, 8 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> > > > > index 08108ce..5b6f5af 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_params.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_params.c
> > > > > @@ -95,6 +95,11 @@ struct i915_params i915_modparams __read_mostly = {
> > > > >      "(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)");
> > > > > 
> > > > > +i915_param_named_unsafe(force_psr1, int, 0600,
> > > > > +   "Enable PSR1 on PSR2 Panel "
> > > > > +   "(0=disabled, 1=enabled) "
> > > > > +   "Default: -1 (use per-chip default)");
> > > > > +
> > > > >   i915_param_named_unsafe(alpha_support, bool, 0400,
> > > > >      "Enable alpha quality driver support for latest hardware. "
> > > > >      "See also CONFIG_DRM_I915_ALPHA_SUPPORT.");
> > > > > diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> > > > > index c963603..1f5dd1c 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_params.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_params.h
> > > > > @@ -44,6 +44,7 @@
> > > > >      param(int, enable_fbc, -1) \
> > > > >      param(int, enable_ppgtt, -1) \
> > > > >      param(int, enable_psr, -1) \
> > > > > +   param(int, force_psr1, -1) \
> > > > >      param(int, disable_power_well, -1) \
> > > > >      param(int, enable_ips, 1) \
> > > > >      param(int, invert_brightness, 0) \
> > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > > > index 2d53f73..415e377 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > @@ -540,6 +540,8 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
> > > > > 
> > > > >      crtc_state->has_psr = true;
> > > > >      crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp, crtc_state);
> > > > > +   if (i915_modparams.force_psr1 == 1 && crtc_state->has_psr2)
> > > > > +           crtc_state->has_psr2 = false;
> > > > >      DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : "");
> > > > >   }
> > > > > 
> >