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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jun 15, 2022 at 06:15:58PM +0300, Jani Nikula wrote:
> 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...?

Readout+sanitation... I guess that's pretty self contained so a fairly
good candidate for moving out.

Though it does mean I get to rebase my "nuke the legacy state pointers"
branch at some point :/ Oh well, that's life.

-- 
Ville Syrjälä
Intel