Re: [Intel-gfx] [PATCH v2 21/24] drm/i915/display: Move verbose_state_checks under display

[Jani Nikula <jani.nikula@intel.com>]

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.

-- 
Jani Nikula, Intel