| Message ID | 20180712052715.8177-1-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> What do you think of using enable_psr=1 to force PSR1 and enable_psr=2 to force PSR2 on PSR2 capable panels? > 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); > }
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, }; 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); > > }
On Thu, Jul 12, 2018 at 12:09:01AM -0700, Dhinakaran Pandiyan wrote: > 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. ops, I had forgotten about that. I'm sorry.... well, I thought what I had in mind would be slight simpler, but specially because we are now removing these link options. Or maybe because I forgot something and over simplify it. So if Maarten is going to send a v2 I just drop my initial stashed changes here, otherwise I can also put in a pastebin later. But up to Maarten. And the only change on test cases would be the order, instead setting up the parameter and doing the modeset, it would do the modeset and then toggle the debugfs. We will need it sooner or later. and it is no more complicated than dropping sink_crc in favor of the mmio reads ;) Thanks, Rodrigo. > > > -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); > > > > }
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); > > > }
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> pushed, thanks > > What do you think of using enable_psr=1 to force PSR1 and enable_psr=2 > to force PSR2 on PSR2 capable panels? > > > 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
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); }
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. Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> 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(-)