| Message ID | 20231016111658.3432581-22-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Framework for display parameters | expand |
On Mon, 2023-10-16 at 14:16 +0300, Jouni Högander wrote: > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.h | 2 +- > drivers/gpu/drm/i915/display/intel_display_params.c | 3 +++ > drivers/gpu/drm/i915/display/intel_display_params.h | 1 + > drivers/gpu/drm/i915/i915_params.c | 3 --- > drivers/gpu/drm/i915/i915_params.h | 1 - > 5 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h > index ba3548f9768d..bc95fb377386 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.h > +++ b/drivers/gpu/drm/i915/display/intel_display.h > @@ -552,7 +552,7 @@ bool assert_port_valid(struct drm_i915_private *i915, enum port port); > struct drm_device *drm = &(__i915)->drm; \ > int __ret_warn_on = !!(condition); \ > if (unlikely(__ret_warn_on)) \ > - if (!drm_WARN(drm, __i915->params.verbose_state_checks, format)) \ > + if (!drm_WARN(drm, __i915->display.params.verbose_state_checks, format)) \ > drm_err(drm, format); \ > unlikely(__ret_warn_on); \ > }) > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c > index 06e68c7fec1c..e86766639396 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_params.c > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c > @@ -87,6 +87,9 @@ intel_display_param_named_unsafe(force_reset_modeset_test, bool, 0400, > intel_display_param_named(disable_display, bool, 0400, > "Disable display (default: false)"); > > +intel_display_param_named(verbose_state_checks, bool, 0400, > + "Enable verbose logs (ie. WARN_ON()) in case of unexpected hw state conditions."); > + > intel_display_param_named_unsafe(enable_fbc, int, 0400, > "Enable frame buffer compression for power savings " > "(default: -1 (use per-chip default))"); > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.h b/drivers/gpu/drm/i915/display/intel_display_params.h > index 60d9c3d59fe4..b35443f51375 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_params.h > +++ b/drivers/gpu/drm/i915/display/intel_display_params.h > @@ -39,6 +39,7 @@ struct drm_i915_private; > param(bool, load_detect_test, false, 0600) \ > param(bool, force_reset_modeset_test, false, 0600) \ > param(bool, disable_display, false, 0400) \ > + param(bool, verbose_state_checks, true, 0) \ Why is this one 0? Why can't we even read it? -- Cheers, Luca.
On Mon, 2023-10-23 at 17:00 +0300, Luca Coelho wrote: > On Mon, 2023-10-16 at 14:16 +0300, Jouni Högander wrote: > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display.h | 2 +- > > drivers/gpu/drm/i915/display/intel_display_params.c | 3 +++ > > drivers/gpu/drm/i915/display/intel_display_params.h | 1 + > > drivers/gpu/drm/i915/i915_params.c | 3 --- > > drivers/gpu/drm/i915/i915_params.h | 1 - > > 5 files changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h > > b/drivers/gpu/drm/i915/display/intel_display.h > > index ba3548f9768d..bc95fb377386 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.h > > +++ b/drivers/gpu/drm/i915/display/intel_display.h > > @@ -552,7 +552,7 @@ bool assert_port_valid(struct drm_i915_private > > *i915, enum port port); > > struct drm_device *drm = &(__i915)- > > >drm; \ > > int __ret_warn_on = > > !!(condition); \ > > if > > (unlikely(__ret_warn_on)) \ > > - if (!drm_WARN(drm, __i915- > > >params.verbose_state_checks, format)) \ > > + if (!drm_WARN(drm, __i915- > > >display.params.verbose_state_checks, format)) \ > > drm_err(drm, > > format); \ > > unlikely(__ret_warn_on); > > \ > > }) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c > > b/drivers/gpu/drm/i915/display/intel_display_params.c > > index 06e68c7fec1c..e86766639396 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_params.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c > > @@ -87,6 +87,9 @@ > > intel_display_param_named_unsafe(force_reset_modeset_test, bool, > > 0400, > > intel_display_param_named(disable_display, bool, 0400, > > "Disable display (default: false)"); > > > > +intel_display_param_named(verbose_state_checks, bool, 0400, > > + "Enable verbose logs (ie. WARN_ON()) in case of unexpected > > hw state conditions."); > > + > > intel_display_param_named_unsafe(enable_fbc, int, 0400, > > "Enable frame buffer compression for power savings " > > "(default: -1 (use per-chip default))"); > > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.h > > b/drivers/gpu/drm/i915/display/intel_display_params.h > > index 60d9c3d59fe4..b35443f51375 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_params.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_params.h > > @@ -39,6 +39,7 @@ struct drm_i915_private; > > param(bool, load_detect_test, false, 0600) \ > > param(bool, force_reset_modeset_test, false, 0600) \ > > param(bool, disable_display, false, 0400) \ > > + param(bool, verbose_state_checks, true, 0) \ > > Why is this one 0? Why can't we even read it? I found this comment in older commit message written by Jani Nikula: "0 mode will bypass debugfs creation. Use it for verbose_state_checks which will need special attention in follow-up work." BR, Jouni Högander > > > -- > Cheers, > Luca.
On Tue, 2023-10-24 at 08:22 +0000, Hogander, Jouni wrote: > On Mon, 2023-10-23 at 17:00 +0300, Luca Coelho wrote: > > On Mon, 2023-10-16 at 14:16 +0300, Jouni Högander wrote: > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_display.h | 2 +- > > > drivers/gpu/drm/i915/display/intel_display_params.c | 3 +++ > > > drivers/gpu/drm/i915/display/intel_display_params.h | 1 + > > > drivers/gpu/drm/i915/i915_params.c | 3 --- > > > drivers/gpu/drm/i915/i915_params.h | 1 - > > > 5 files changed, 5 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h > > > b/drivers/gpu/drm/i915/display/intel_display.h > > > index ba3548f9768d..bc95fb377386 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.h > > > +++ b/drivers/gpu/drm/i915/display/intel_display.h > > > @@ -552,7 +552,7 @@ bool assert_port_valid(struct drm_i915_private > > > *i915, enum port port); > > > struct drm_device *drm = &(__i915)- > > > > drm; \ > > > int __ret_warn_on = > > > !!(condition); \ > > > if > > > (unlikely(__ret_warn_on)) \ > > > - if (!drm_WARN(drm, __i915- > > > > params.verbose_state_checks, format)) \ > > > + if (!drm_WARN(drm, __i915- > > > > display.params.verbose_state_checks, format)) \ > > > drm_err(drm, > > > format); \ > > > unlikely(__ret_warn_on); > > > \ > > > }) > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c > > > b/drivers/gpu/drm/i915/display/intel_display_params.c > > > index 06e68c7fec1c..e86766639396 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_params.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c > > > @@ -87,6 +87,9 @@ > > > intel_display_param_named_unsafe(force_reset_modeset_test, bool, > > > 0400, > > > intel_display_param_named(disable_display, bool, 0400, > > > "Disable display (default: false)"); > > > > > > +intel_display_param_named(verbose_state_checks, bool, 0400, > > > + "Enable verbose logs (ie. WARN_ON()) in case of unexpected > > > hw state conditions."); > > > + > > > intel_display_param_named_unsafe(enable_fbc, int, 0400, > > > "Enable frame buffer compression for power savings " > > > "(default: -1 (use per-chip default))"); > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.h > > > b/drivers/gpu/drm/i915/display/intel_display_params.h > > > index 60d9c3d59fe4..b35443f51375 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_params.h > > > +++ b/drivers/gpu/drm/i915/display/intel_display_params.h > > > @@ -39,6 +39,7 @@ struct drm_i915_private; > > > param(bool, load_detect_test, false, 0600) \ > > > param(bool, force_reset_modeset_test, false, 0600) \ > > > param(bool, disable_display, false, 0400) \ > > > + param(bool, verbose_state_checks, true, 0) \ > > > > Why is this one 0? Why can't we even read it? > > I found this comment in older commit message written by Jani Nikula: > > "0 mode will bypass debugfs creation. Use it for verbose_state_checks > which will need special attention in follow-up work." This sounds pretty odd, why wouldn't we want it to be even read? In any case, it's not related to this patch, so: Reviewed-by: Luca Coelho <luciano.coelho@intel.com> -- Cheers, Luca.
On Tue, 24 Oct 2023, Luca Coelho <luca@coelho.fi> wrote: > On Tue, 2023-10-24 at 08:22 +0000, Hogander, Jouni wrote: >> On Mon, 2023-10-23 at 17:00 +0300, Luca Coelho wrote: >> > On Mon, 2023-10-16 at 14:16 +0300, Jouni Högander wrote: >> > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> >> > > --- >> > > drivers/gpu/drm/i915/display/intel_display.h | 2 +- >> > > drivers/gpu/drm/i915/display/intel_display_params.c | 3 +++ >> > > drivers/gpu/drm/i915/display/intel_display_params.h | 1 + >> > > drivers/gpu/drm/i915/i915_params.c | 3 --- >> > > drivers/gpu/drm/i915/i915_params.h | 1 - >> > > 5 files changed, 5 insertions(+), 5 deletions(-) >> > > >> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h >> > > b/drivers/gpu/drm/i915/display/intel_display.h >> > > index ba3548f9768d..bc95fb377386 100644 >> > > --- a/drivers/gpu/drm/i915/display/intel_display.h >> > > +++ b/drivers/gpu/drm/i915/display/intel_display.h >> > > @@ -552,7 +552,7 @@ bool assert_port_valid(struct drm_i915_private >> > > *i915, enum port port); >> > > struct drm_device *drm = &(__i915)- >> > > > drm; \ >> > > int __ret_warn_on = >> > > !!(condition); \ >> > > if >> > > (unlikely(__ret_warn_on)) \ >> > > - if (!drm_WARN(drm, __i915- >> > > > params.verbose_state_checks, format)) \ >> > > + if (!drm_WARN(drm, __i915- >> > > > display.params.verbose_state_checks, format)) \ >> > > drm_err(drm, >> > > format); \ >> > > unlikely(__ret_warn_on); >> > > \ >> > > }) >> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c >> > > b/drivers/gpu/drm/i915/display/intel_display_params.c >> > > index 06e68c7fec1c..e86766639396 100644 >> > > --- a/drivers/gpu/drm/i915/display/intel_display_params.c >> > > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c >> > > @@ -87,6 +87,9 @@ >> > > intel_display_param_named_unsafe(force_reset_modeset_test, bool, >> > > 0400, >> > > intel_display_param_named(disable_display, bool, 0400, >> > > "Disable display (default: false)"); >> > > >> > > +intel_display_param_named(verbose_state_checks, bool, 0400, >> > > + "Enable verbose logs (ie. WARN_ON()) in case of unexpected >> > > hw state conditions."); >> > > + >> > > intel_display_param_named_unsafe(enable_fbc, int, 0400, >> > > "Enable frame buffer compression for power savings " >> > > "(default: -1 (use per-chip default))"); >> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.h >> > > b/drivers/gpu/drm/i915/display/intel_display_params.h >> > > index 60d9c3d59fe4..b35443f51375 100644 >> > > --- a/drivers/gpu/drm/i915/display/intel_display_params.h >> > > +++ b/drivers/gpu/drm/i915/display/intel_display_params.h >> > > @@ -39,6 +39,7 @@ struct drm_i915_private; >> > > param(bool, load_detect_test, false, 0600) \ >> > > param(bool, force_reset_modeset_test, false, 0600) \ >> > > param(bool, disable_display, false, 0400) \ >> > > + param(bool, verbose_state_checks, true, 0) \ >> > >> > Why is this one 0? Why can't we even read it? >> >> I found this comment in older commit message written by Jani Nikula: >> >> "0 mode will bypass debugfs creation. Use it for verbose_state_checks >> which will need special attention in follow-up work." > > This sounds pretty odd, why wouldn't we want it to be even read? I *think* I remember why. When I added the device parameters, I915_STATE_WARN(), the only user of verbose_state_checks, did not have the i915 parameter yet. So it could not access the device parameter. Thus the verbose_state_checks *module* parameter had to have 0600 mode, and modifying that runtime meant that the *device* parameter, even as read-only, would have gone out of sync and shown a different value. I only added the i915 parameter to I915_STATE_WARN() last May, but clearly did not follow through with the parameter change. From now on, it should use the device param like the rest of the code, it should have a mutable debugfs file, and the module parameter should be 0400. BR, Jani. > > In any case, it's not related to this patch, so: > > Reviewed-by: Luca Coelho <luciano.coelho@intel.com> > > -- > Cheers, > Luca.
On Tue, 2023-10-24 at 15:12 +0300, Jani Nikula wrote: > On Tue, 24 Oct 2023, Luca Coelho <luca@coelho.fi> wrote: > > On Tue, 2023-10-24 at 08:22 +0000, Hogander, Jouni wrote: > > > On Mon, 2023-10-23 at 17:00 +0300, Luca Coelho wrote: > > > > On Mon, 2023-10-16 at 14:16 +0300, Jouni Högander wrote: > > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_display.h | 2 +- > > > > > drivers/gpu/drm/i915/display/intel_display_params.c | 3 +++ > > > > > drivers/gpu/drm/i915/display/intel_display_params.h | 1 + > > > > > drivers/gpu/drm/i915/i915_params.c | 3 --- > > > > > drivers/gpu/drm/i915/i915_params.h | 1 - > > > > > 5 files changed, 5 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h > > > > > b/drivers/gpu/drm/i915/display/intel_display.h > > > > > index ba3548f9768d..bc95fb377386 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.h > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.h > > > > > @@ -552,7 +552,7 @@ bool assert_port_valid(struct > > > > > drm_i915_private > > > > > *i915, enum port port); > > > > > struct drm_device *drm = &(__i915)- > > > > > > drm; \ > > > > > int __ret_warn_on = > > > > > !!(condition); \ > > > > > if > > > > > (unlikely(__ret_warn_on)) > > > > > \ > > > > > - if (!drm_WARN(drm, __i915- > > > > > > params.verbose_state_checks, format)) \ > > > > > + if (!drm_WARN(drm, __i915- > > > > > > display.params.verbose_state_checks, format)) \ > > > > > drm_err(drm, > > > > > format); \ > > > > > unlikely(__ret_warn_on); > > > > > > > > > > \ > > > > > }) > > > > > diff --git > > > > > a/drivers/gpu/drm/i915/display/intel_display_params.c > > > > > b/drivers/gpu/drm/i915/display/intel_display_params.c > > > > > index 06e68c7fec1c..e86766639396 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_params.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c > > > > > @@ -87,6 +87,9 @@ > > > > > intel_display_param_named_unsafe(force_reset_modeset_test, > > > > > bool, > > > > > 0400, > > > > > intel_display_param_named(disable_display, bool, 0400, > > > > > "Disable display (default: false)"); > > > > > > > > > > +intel_display_param_named(verbose_state_checks, bool, 0400, > > > > > + "Enable verbose logs (ie. WARN_ON()) in case of > > > > > unexpected > > > > > hw state conditions."); > > > > > + > > > > > intel_display_param_named_unsafe(enable_fbc, int, 0400, > > > > > "Enable frame buffer compression for power savings " > > > > > "(default: -1 (use per-chip default))"); > > > > > diff --git > > > > > a/drivers/gpu/drm/i915/display/intel_display_params.h > > > > > b/drivers/gpu/drm/i915/display/intel_display_params.h > > > > > index 60d9c3d59fe4..b35443f51375 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_params.h > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_params.h > > > > > @@ -39,6 +39,7 @@ struct drm_i915_private; > > > > > param(bool, load_detect_test, false, 0600) \ > > > > > param(bool, force_reset_modeset_test, false, 0600) \ > > > > > param(bool, disable_display, false, 0400) \ > > > > > + param(bool, verbose_state_checks, true, 0) \ > > > > > > > > Why is this one 0? Why can't we even read it? > > > > > > I found this comment in older commit message written by Jani > > > Nikula: > > > > > > "0 mode will bypass debugfs creation. Use it for > > > verbose_state_checks > > > which will need special attention in follow-up work." > > > > This sounds pretty odd, why wouldn't we want it to be even read? > > I *think* I remember why. > > When I added the device parameters, I915_STATE_WARN(), the only user > of > verbose_state_checks, did not have the i915 parameter yet. So it > could > not access the device parameter. > > Thus the verbose_state_checks *module* parameter had to have 0600 > mode, > and modifying that runtime meant that the *device* parameter, even as > read-only, would have gone out of sync and shown a different value. > > I only added the i915 parameter to I915_STATE_WARN() last May, but > clearly did not follow through with the parameter change. > > From now on, it should use the device param like the rest of the > code, > it should have a mutable debugfs file, and the module parameter > should > be 0400. Ok, I will still do this change and resend. BR, Jouni Högander > > > BR, > Jani. > > > > > > In any case, it's not related to this patch, so: > > > > Reviewed-by: Luca Coelho <luciano.coelho@intel.com> > > > > -- > > Cheers, > > Luca. > > -- > Jani Nikula, Intel
diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h index ba3548f9768d..bc95fb377386 100644 --- a/drivers/gpu/drm/i915/display/intel_display.h +++ b/drivers/gpu/drm/i915/display/intel_display.h @@ -552,7 +552,7 @@ bool assert_port_valid(struct drm_i915_private *i915, enum port port); struct drm_device *drm = &(__i915)->drm; \ int __ret_warn_on = !!(condition); \ if (unlikely(__ret_warn_on)) \ - if (!drm_WARN(drm, __i915->params.verbose_state_checks, format)) \ + if (!drm_WARN(drm, __i915->display.params.verbose_state_checks, format)) \ drm_err(drm, format); \ unlikely(__ret_warn_on); \ }) diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c index 06e68c7fec1c..e86766639396 100644 --- a/drivers/gpu/drm/i915/display/intel_display_params.c +++ b/drivers/gpu/drm/i915/display/intel_display_params.c @@ -87,6 +87,9 @@ intel_display_param_named_unsafe(force_reset_modeset_test, bool, 0400, intel_display_param_named(disable_display, bool, 0400, "Disable display (default: false)"); +intel_display_param_named(verbose_state_checks, bool, 0400, + "Enable verbose logs (ie. WARN_ON()) in case of unexpected hw state conditions."); + intel_display_param_named_unsafe(enable_fbc, int, 0400, "Enable frame buffer compression for power savings " "(default: -1 (use per-chip default))"); diff --git a/drivers/gpu/drm/i915/display/intel_display_params.h b/drivers/gpu/drm/i915/display/intel_display_params.h index 60d9c3d59fe4..b35443f51375 100644 --- a/drivers/gpu/drm/i915/display/intel_display_params.h +++ b/drivers/gpu/drm/i915/display/intel_display_params.h @@ -39,6 +39,7 @@ struct drm_i915_private; param(bool, load_detect_test, false, 0600) \ param(bool, force_reset_modeset_test, false, 0600) \ param(bool, disable_display, false, 0400) \ + param(bool, verbose_state_checks, true, 0) \ param(int, enable_fbc, -1, 0600) \ param(int, enable_psr, -1, 0600) \ param(bool, psr_safest_params, false, 0400) \ diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 4e8c088c69fd..72614c139222 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -93,9 +93,6 @@ i915_param_named(mmio_debug, int, 0400, "Enable the MMIO debug code for the first N failures (default: off). " "This may negatively affect performance."); -i915_param_named(verbose_state_checks, bool, 0400, - "Enable verbose logs (ie. WARN_ON()) in case of unexpected hw state conditions."); - i915_param_named_unsafe(nuclear_pageflip, bool, 0400, "Force enable atomic functionality on platforms that don't have full support yet."); diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index 8bce7d057634..4b543beb17ca 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -64,7 +64,6 @@ struct drm_printer; /* leave bools at the end to not create holes */ \ param(bool, enable_hangcheck, true, 0600) \ param(bool, error_capture, true, IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR) ? 0600 : 0) \ - param(bool, verbose_state_checks, true, 0) \ param(bool, nuclear_pageflip, false, 0400) \ param(bool, enable_dp_mst, true, 0600) \ param(bool, enable_gvt, false, IS_ENABLED(CONFIG_DRM_I915_GVT) ? 0400 : 0)
Signed-off-by: Jouni Högander <jouni.hogander@intel.com> --- drivers/gpu/drm/i915/display/intel_display.h | 2 +- drivers/gpu/drm/i915/display/intel_display_params.c | 3 +++ drivers/gpu/drm/i915/display/intel_display_params.h | 1 + drivers/gpu/drm/i915/i915_params.c | 3 --- drivers/gpu/drm/i915/i915_params.h | 1 - 5 files changed, 5 insertions(+), 5 deletions(-)