Re: [Intel-gfx] [PATCH 0/7] drm/i915/display: split out verifation, compare and dump from intel_display.c

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

On Wed, 15 Jun 2022, Jani Nikula <jani.nikula@intel.com> wrote:
> On Wed, 15 Jun 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> On Wed, Jun 15, 2022 at 03:47:54PM +0300, Jani Nikula wrote:
>>> The state verification and pipe config comparison/dumping are fairly
>>> isolated pieces of code within intel_display.c. Move them to separate
>>> files in a long overdue cleanup.
>>> 
>>> Jani Nikula (7):
>>>   drm/i915/wm: move wm state verification to intel_pm.c
>>>   drm/i915/dpll: move shared dpll state verification to intel_dpll_mgr.c
>>>   drm/i915/mpllb: use I915_STATE_WARN() for state mismatch warnings
>>>   drm/i915/mpllb: move mpllb state check to intel_snps_phy.c
>>
>> I'd perhaps go for foo_state_verify() naming convention. Would
>> match the foo_state_dump() naming convention I suggested
>> for the dumping stuff.
>
> Roger.
>
>>
>> Apart from that these ^ four are
>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>>>   drm/i915/display: split out modeset verification code
>>
>> I really hate some of that code. I guess hiding it is one option :P
>> This one ^ is
>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Yeah, there's some weird stuff. For example we only call
> verify_encoder_state() only to verify it's disabled.
>
>>
>>>   drm/i915/display: split out pipe config compare to a separate file
>>
>> Not entirely sure I like moving this one. The fastset stuff
>> within needs to stay in sync with the fastset codepaths which
>> are in intel_display.c. Not sure if we risk more bugs if it's
>> too well hidden...
>
> Mixed feelings. The problem remains, the file is still too damn big, and
> it's hard to draw the lines what to extract. Maybe all the modeset code
> needs to be lifted, along with the config compare, but then I think that
> has too many dependencies to axe out cleanly. Damned if you do, damned
> if you don't.

I've also got a patch to move intel_modeset_setup_hw_state() and all the
static functions only it calls to another file. Do you also think that
needs to be together with the modeset code...?

BR,
Jani.


>
> BR,
> Jani.
>
>
>>
>>>   drm/i915/display: split out pipe config dump to a separate file
>>> 
>>>  drivers/gpu/drm/i915/Makefile                 |    3 +
>>>  drivers/gpu/drm/i915/display/intel_display.c  | 1373 +----------------
>>>  drivers/gpu/drm/i915/display/intel_display.h  |    3 +
>>>  drivers/gpu/drm/i915/display/intel_dpll_mgr.c |   88 ++
>>>  drivers/gpu/drm/i915/display/intel_dpll_mgr.h |    5 +
>>>  .../drm/i915/display/intel_modeset_verify.c   |  247 +++
>>>  .../drm/i915/display/intel_modeset_verify.h   |   21 +
>>>  .../i915/display/intel_pipe_config_compare.c  |  581 +++++++
>>>  .../i915/display/intel_pipe_config_compare.h  |   17 +
>>>  .../drm/i915/display/intel_pipe_config_dump.c |  314 ++++
>>>  .../drm/i915/display/intel_pipe_config_dump.h |   16 +
>>>  drivers/gpu/drm/i915/display/intel_snps_phy.c |   43 +
>>>  drivers/gpu/drm/i915/display/intel_snps_phy.h |    5 +-
>>>  drivers/gpu/drm/i915/intel_pm.c               |  138 +-
>>>  drivers/gpu/drm/i915/intel_pm.h               |   14 +-
>>>  15 files changed, 1482 insertions(+), 1386 deletions(-)
>>>  create mode 100644 drivers/gpu/drm/i915/display/intel_modeset_verify.c
>>>  create mode 100644 drivers/gpu/drm/i915/display/intel_modeset_verify.h
>>>  create mode 100644 drivers/gpu/drm/i915/display/intel_pipe_config_compare.c
>>>  create mode 100644 drivers/gpu/drm/i915/display/intel_pipe_config_compare.h
>>>  create mode 100644 drivers/gpu/drm/i915/display/intel_pipe_config_dump.c
>>>  create mode 100644 drivers/gpu/drm/i915/display/intel_pipe_config_dump.h
>>> 
>>> -- 
>>> 2.30.2

-- 
Jani Nikula, Intel Open Source Graphics Center