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

[Rodrigo Vivi]

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.

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" : "");
> > > >   }
> > > > 
>