Re: [PATCH] drm/i915/psr: simplify enable_psr handling
On Fri, Dec 21, 2018 at 8:53 PM Ross Zwisler wrote:
>
> On Fri, Dec 21, 2018 at 11:23:07AM -0800, Dhinakaran Pandiyan wrote:
> > On Fri, 2018-12-21 at 10:23 -0700, Ross Zwisler wrote:
> > > The following commit:
> > >
> > > commit 2bdd045e3a30 ("drm/i915/psr: Check if VBT says PSR can be
> > > enabled.")
> > >
> > > added some code with no usable functionality. Regardless of how the
> > > psr
> > > default is set up in the BDB_DRIVER_FEATURES section, if the
> > > enable_psr
> > > module parameter isn't specified it defaults to 0.
> > Right, that was intentional and the commit message even makes a note of
> > it
> > " Note: The feature currently remains disabled by default for all
> > platforms irrespective of what VBT says."
> >
> >
> > Anyway, we've enabled the feature by default now and the current code
> > should take into account the VBT flag if the module parameter is left
> > to a default value. Please check git://anongit.freedesktop.org/drm-tip
> > drm-tip.
>
> Fair enough. It's a bad pattern to introduce dead code as a placeholder for
> some future work, though. This code has been in the tree for three major
> kernel releases (v4.{18,19,20}) without providing any useful functionality.
Getting PSR enabled by default on at least a few platforms took years.
Insisting that we do not ever merge such code also doesn't work.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/i915/psr: simplify enable_psr handling
The following commit:
commit 2bdd045e3a30 ("drm/i915/psr: Check if VBT says PSR can be enabled.")
added some code with no usable functionality. Regardless of how the psr
default is set up in the BDB_DRIVER_FEATURES section, if the enable_psr
module parameter isn't specified it defaults to 0.
Remove this dead code, simplify the way that enable_psr is handled and
update the module parameter string to match the actual functionality.
Cc: Dhinakaran Pandiyan
Cc: Rodrigo Vivi
Signed-off-by: Ross Zwisler
---
drivers/gpu/drm/i915/i915_drv.h| 1 -
drivers/gpu/drm/i915/i915_params.c | 4 +---
drivers/gpu/drm/i915/i915_params.h | 2 +-
drivers/gpu/drm/i915/intel_bios.c | 1 -
drivers/gpu/drm/i915/intel_psr.c | 7 ---
5 files changed, 2 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 872a2e159a5f9..b4c50ba0b22a6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1115,7 +1115,6 @@ struct intel_vbt_data {
} edp;
struct {
- bool enable;
bool full_link;
bool require_aux_wakeup;
int idle_frames;
diff --git a/drivers/gpu/drm/i915/i915_params.c
b/drivers/gpu/drm/i915/i915_params.c
index 295e981e4a398..80ce8758c3c69 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -87,9 +87,7 @@ i915_param_named_unsafe(enable_ppgtt, int, 0400,
"(-1=auto [default], 0=disabled, 1=aliasing, 2=full, 3=full with
extended address space)");
i915_param_named_unsafe(enable_psr, int, 0600,
- "Enable PSR "
- "(0=disabled, 1=enabled) "
- "Default: -1 (use per-chip default)");
+ "Enable PSR (default: false)");
i915_param_named_unsafe(alpha_support, bool, 0400,
"Enable alpha quality driver support for latest hardware. "
diff --git a/drivers/gpu/drm/i915/i915_params.h
b/drivers/gpu/drm/i915/i915_params.h
index 6c4d4a21474b5..144572f17a83d 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -42,7 +42,7 @@ struct drm_printer;
param(int, enable_dc, -1) \
param(int, enable_fbc, -1) \
param(int, enable_ppgtt, -1) \
- param(int, enable_psr, -1) \
+ param(int, enable_psr, 0) \
param(int, disable_power_well, -1) \
param(int, enable_ips, 1) \
param(int, invert_brightness, 0) \
diff --git a/drivers/gpu/drm/i915/intel_bios.c
b/drivers/gpu/drm/i915/intel_bios.c
index 1faa494e2bc91..d676d483d5cf1 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -551,7 +551,6 @@ parse_driver_features(struct drm_i915_private *dev_priv,
*/
if (!driver->drrs_enabled)
dev_priv->vbt.drrs_type = DRRS_NOT_SUPPORTED;
- dev_priv->vbt.psr.enable = driver->psr_enabled;
}
static void
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index b6838b525502e..26e7eb318cf07 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -1065,13 +1065,6 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
if (!dev_priv->psr.sink_support)
return;
- if (i915_modparams.enable_psr == -1) {
- i915_modparams.enable_psr = dev_priv->vbt.psr.enable;
-
- /* Per platform default: all disabled. */
- i915_modparams.enable_psr = 0;
- }
-
/* Set link_standby x link_off defaults */
if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
/* HSW and BDW require workarounds that we don't implement. */
--
2.20.1.415.g653613c723-goog
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915/psr: simplify enable_psr handling
On Fri, Dec 21, 2018 at 11:23:07AM -0800, Dhinakaran Pandiyan wrote:
> On Fri, 2018-12-21 at 10:23 -0700, Ross Zwisler wrote:
> > The following commit:
> >
> > commit 2bdd045e3a30 ("drm/i915/psr: Check if VBT says PSR can be
> > enabled.")
> >
> > added some code with no usable functionality. Regardless of how the
> > psr
> > default is set up in the BDB_DRIVER_FEATURES section, if the
> > enable_psr
> > module parameter isn't specified it defaults to 0.
> Right, that was intentional and the commit message even makes a note of
> it
> " Note: The feature currently remains disabled by default for all
> platforms irrespective of what VBT says."
>
>
> Anyway, we've enabled the feature by default now and the current code
> should take into account the VBT flag if the module parameter is left
> to a default value. Please check git://anongit.freedesktop.org/drm-tip
> drm-tip.
Fair enough. It's a bad pattern to introduce dead code as a placeholder for
some future work, though. This code has been in the tree for three major
kernel releases (v4.{18,19,20}) without providing any useful functionality.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915/psr: simplify enable_psr handling
On Fri, 2018-12-21 at 10:23 -0700, Ross Zwisler wrote:
> The following commit:
>
> commit 2bdd045e3a30 ("drm/i915/psr: Check if VBT says PSR can be
> enabled.")
>
> added some code with no usable functionality. Regardless of how the
> psr
> default is set up in the BDB_DRIVER_FEATURES section, if the
> enable_psr
> module parameter isn't specified it defaults to 0.
Right, that was intentional and the commit message even makes a note of
it
" Note: The feature currently remains disabled by default for all
platforms irrespective of what VBT says."
Anyway, we've enabled the feature by default now and the current code
should take into account the VBT flag if the module parameter is left
to a default value. Please check git://anongit.freedesktop.org/drm-tip
drm-tip.
-DK
>
> Remove this dead code, simplify the way that enable_psr is handled
> and
> update the module parameter string to match the actual functionality.
>
> Cc: Dhinakaran Pandiyan
> Cc: Rodrigo Vivi
> Signed-off-by: Ross Zwisler
> ---
> drivers/gpu/drm/i915/i915_drv.h| 1 -
> drivers/gpu/drm/i915/i915_params.c | 4 +---
> drivers/gpu/drm/i915/i915_params.h | 2 +-
> drivers/gpu/drm/i915/intel_bios.c | 1 -
> drivers/gpu/drm/i915/intel_psr.c | 7 ---
> 5 files changed, 2 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 872a2e159a5f9..b4c50ba0b22a6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1115,7 +1115,6 @@ struct intel_vbt_data {
> } edp;
>
> struct {
> - bool enable;
> bool full_link;
> bool require_aux_wakeup;
> int idle_frames;
> diff --git a/drivers/gpu/drm/i915/i915_params.c
> b/drivers/gpu/drm/i915/i915_params.c
> index 295e981e4a398..80ce8758c3c69 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -87,9 +87,7 @@ i915_param_named_unsafe(enable_ppgtt, int, 0400,
> "(-1=auto [default], 0=disabled, 1=aliasing, 2=full, 3=full
> with extended address space)");
>
> i915_param_named_unsafe(enable_psr, int, 0600,
> - "Enable PSR "
> - "(0=disabled, 1=enabled) "
> - "Default: -1 (use per-chip default)");
> + "Enable PSR (default: false)");
>
> i915_param_named_unsafe(alpha_support, bool, 0400,
> "Enable alpha quality driver support for latest hardware. "
> diff --git a/drivers/gpu/drm/i915/i915_params.h
> b/drivers/gpu/drm/i915/i915_params.h
> index 6c4d4a21474b5..144572f17a83d 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -42,7 +42,7 @@ struct drm_printer;
> param(int, enable_dc, -1) \
> param(int, enable_fbc, -1) \
> param(int, enable_ppgtt, -1) \
> - param(int, enable_psr, -1) \
> + param(int, enable_psr, 0) \
> param(int, disable_power_well, -1) \
> param(int, enable_ips, 1) \
> param(int, invert_brightness, 0) \
> diff --git a/drivers/gpu/drm/i915/intel_bios.c
> b/drivers/gpu/drm/i915/intel_bios.c
> index 1faa494e2bc91..d676d483d5cf1 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -551,7 +551,6 @@ parse_driver_features(struct drm_i915_private
> *dev_priv,
>*/
> if (!driver->drrs_enabled)
> dev_priv->vbt.drrs_type = DRRS_NOT_SUPPORTED;
> - dev_priv->vbt.psr.enable = driver->psr_enabled;
> }
>
> static void
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index b6838b525502e..26e7eb318cf07 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -1065,13 +1065,6 @@ void intel_psr_init(struct drm_i915_private
> *dev_priv)
> if (!dev_priv->psr.sink_support)
> return;
>
> - if (i915_modparams.enable_psr == -1) {
> - i915_modparams.enable_psr = dev_priv->vbt.psr.enable;
> -
> - /* Per platform default: all disabled. */
> - i915_modparams.enable_psr = 0;
> - }
> -
> /* Set link_standby x link_off defaults */
> if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> /* HSW and BDW require workarounds that we don't
> implement. */
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
